Increase bind parameters limit #298

MahdiBM · 2022-06-07T07:31:19Z

The problem

Right now PostgresNIO doesn't allow queries with bind parameters count more than Int16.max (32768).
This is despite Postgres 10+ supporting up to ~~(UInt16.max - 1)~~ UInt16.max parameters (65535).
For reference for Postgres 14, see https://www.postgresql.org/docs/14/postgres-fdw.html and search the page for "batch_size". I couldn't find documentation on this on lower Postgres versions, but manually tested the PR (see below).

Solution

This PR increases that limit to 65535 which is more appropriate.

Tests

I don't think there is a need for adding a test, but I changed one of the test's to reflect the changes. We'll see if there is anything I missed, when the CI runs the test.

Other than that, I've manually tested this on Postgres Docker images 10, 11, 12, 13, 14 (The currently supported-by-Postgres versions).

…y-layout

fabianfett

@MahdiBM thanks for bringing this up. And sorry for taking so long to get back to you. Since we are testing the boundaries of Postgres here, would you mind adding an integration test, that ensures that Postgres supports what we allow?

Sources/PostgresNIO/Connection/PostgresConnection.swift

fabianfett · 2022-09-28T07:43:52Z

The best way to integration test this will be a test case like this:

    func testBindMaximumParameters() async throws {
        let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1)
        defer { XCTAssertNoThrow(try eventLoopGroup.syncShutdownGracefully()) }
        let eventLoop = eventLoopGroup.next()

        try await withTestConnection(on: eventLoop) { connection in
            var binds = PostgresBindings()
            let max = 1664 //Int16.max - 1
            let range = (0..<Int(max))
            for num in range {
                try binds.append(num, context: .default)
            }

            let query = PostgresQuery(unsafeSQL: "SELECT \(range.map({"$\($0 + 1)"}).joined(separator: ","))", binds: binds)
            let rows = try await connection.query(query, logger: .psqlTest)
            var iterator = rows.makeAsyncIterator()
            let firstRow = try await iterator.next()
            XCTAssertEqual(firstRow?.count, Int(max))
            let done = try await iterator.next()
            XCTAssertNil(done)
        }
    }

However we will need to extend this a bit:

Apparently a row in postgres has a max of 1664 columns
- This means to test the max binding we need to create a table and then do a multirow insert into it.
- The test case should create a table with 1000 columns first (can all be integer)
Create a PSQLQuery manually, with our new max parameter binding and insert 66 rows.
There are not enough available parameters for the last row (short of 465 parameters). Just fill those spaces with the numbers directly and no parameter bindings there.
Cleanup the table after the test...

codecov-commenter · 2022-09-30T22:59:53Z

Codecov Report

Merging #298 (cd19695) into main (2fa1cb1) will decrease coverage by 0.00%.
The diff coverage is 57.14%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #298      +/-   ##
==========================================
- Coverage   43.91%   43.90%   -0.01%     
==========================================
  Files         115      115              
  Lines        9680     9677       -3     
==========================================
- Hits         4251     4249       -2     
+ Misses       5429     5428       -1

Flag	Coverage Δ
unittests	`43.90% <57.14%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...es/PostgresNIO/Connection/PostgresConnection.swift	`17.14% <0.00%> (ø)`
Sources/PostgresNIO/New/Messages/Bind.swift	`100.00% <100.00%> (ø)`
...ostgresNIO/New/Messages/ParameterDescription.swift	`100.00% <100.00%> (ø)`
Sources/PostgresNIO/New/Messages/Parse.swift	`100.00% <100.00%> (ø)`
Sources/PostgresNIO/Data/PostgresRow.swift	`49.24% <0.00%> (ø)`
Sources/PostgresNIO/New/PostgresCell.swift	`96.77% <0.00%> (ø)`
Sources/PostgresNIO/Data/PostgresData.swift	`6.89% <0.00%> (ø)`
Sources/PostgresNIO/New/Messages/DataRow.swift	`95.38% <0.00%> (ø)`
...ostgresNIO/New/PostgresBackendMessageDecoder.swift	`94.02% <0.00%> (+0.74%)`	⬆️

MahdiBM · 2022-09-30T23:34:04Z

I corrected the equality checks and added the integration test.

Please let me know if there are any problems or if any improvements can be made.

MahdiBM · 2022-10-01T03:53:03Z

Looking at your original test example, i think it would have been better if i asserted that the inserted rows count is what we expect after the insertion query.
Right now, if there are no errors thrown from the insertion query, the test assumes everything has been successful. I think (?) this could go wrong?

MahdiBM · 2022-10-02T04:55:00Z

I went ahead and added a query to assert that all the rows were inserted successfully. Since all columns are NOT NULL, that would guarantee that the max binds test has been successful.

As an alternative, in the Postgres documentation I read that the INSERT command returns the count of inserted rows, but when i tried to use that in the test, i realized there is no nice way of accessing a command's tag, so basically the count of inserted rows after a INSERT command is inaccessible and we need another query to check for that.

fabianfett · 2022-10-03T11:17:13Z

Tests/IntegrationTests/PSQLIntegrationTests.swift

+            // Mac binds limit is UInt16.max which is 65535 which is 3 * 5 * 17 * 257
+            // Max columns limit is 1664, so we will only make 5 * 257 columns which is less
+            // Then we will insert 3 * 17 rows
+            // In the insertion, there will be a total of 3 * 17 * 5 * 257 == UInt16.max bindings
+            // If the test is successful, it means Postgres supports UInt16.max bindings
+            let columnsCount = 5 * 257
+            let rowsCount = 3 * 17


Huge props to the prime factorization here! 🥰

fabianfett · 2022-10-03T11:21:46Z

Tests/IntegrationTests/PSQLIntegrationTests.swift

+
+            let insertionValues = (0..<rowsCount).map { rowIndex in
+                let indices = (0..<columnsCount).map { columnIndex -> String in
+                    "$\(rowIndex * 1_000 + columnIndex + 1)"


Where is the 1_000 coming from? I would suspect this to be 1285 (5 * 257 which is the columns per row).

I'll need to fix that. I probably forgot about it after the first stages of writing the test function.

fabianfett · 2022-10-03T11:25:34Z

Tests/IntegrationTests/PSQLIntegrationTests.swift

+            let insertionValues = (0..<rowsCount).map { rowIndex in
+                let indices = (0..<columnsCount).map { columnIndex -> String in
+                    "$\(rowIndex * 1_000 + columnIndex + 1)"
+                }
+                return "(\(indices.joined(separator: ", ")))"
+            }.joined(separator: ", ")


Since we know, we want to write exactly 65535 values, it might be easier to just reach for:

let insertionValues = (0..<UInt16.max).map { "$\(Int($0) + 1)" }.joined(separator: ", ")

However, doesn't seem to work since we'll be missing some parentheses.
Currently, and correctly, the values will look like ($1, $2, ...), ($x, $x+1, ...), ... but with the simplified calculation, we'll be missing the parentheses, which postgres doesn't like.

yes makes sense!

fabianfett

This is really awesome. One question remaining

MahdiBM added 5 commits June 7, 2022 10:47

Increase bind parameters limit to 65535

ca6c68d

Use UInt16 for parameters count read/write

b19902f

Use UIn16 for bind count write

0c364aa

Suitably decoding max parameters count for bigger numbers

78e7d8e

Fixed reading parameterCount with an integer with inappropriate memor…

c3e38fa

…y-layout

fabianfett self-requested a review June 8, 2022 00:16

fabianfett reviewed Sep 28, 2022

View reviewed changes

Sources/PostgresNIO/Connection/PostgresConnection.swift Outdated Show resolved Hide resolved

Sources/PostgresNIO/Connection/PostgresConnection.swift Outdated Show resolved Hide resolved

Sources/PostgresNIO/Connection/PostgresConnection.swift Outdated Show resolved Hide resolved

fabianfett added the enhancement New feature or request label Sep 28, 2022

MahdiBM added 2 commits September 28, 2022 12:07

fix problems caused by wrong UInt16.max assumption

08af86e

add integration test for max bind parameters

96f95bc

Use availability check so tests pass on Swift 5.4

bfd7d73

MahdiBM requested a review from fabianfett September 30, 2022 23:10

max binds test assert inserted rows count

cd19695

fabianfett reviewed Oct 3, 2022

View reviewed changes

fix incorrect bind-number in max-binds test

d05fb3d

fabianfett approved these changes Oct 3, 2022

View reviewed changes

Merge branch 'main' into main

b664d02

fabianfett merged commit bfd17ae into vapor:main Oct 3, 2022

Uh oh!

Increase bind parameters limit #298

Increase bind parameters limit #298

Uh oh!

Conversation

MahdiBM commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

Solution

Tests

Uh oh!

fabianfett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fabianfett commented Sep 28, 2022

Uh oh!

codecov-commenter commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MahdiBM commented Sep 30, 2022

Uh oh!

MahdiBM commented Oct 1, 2022

Uh oh!

MahdiBM commented Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabianfett Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianfett Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

MahdiBM Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

fabianfett Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

MahdiBM Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

MahdiBM Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

fabianfett Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

fabianfett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MahdiBM commented Jun 7, 2022 •

edited

Loading

codecov-commenter commented Sep 30, 2022 •

edited

Loading

MahdiBM commented Oct 2, 2022 •

edited

Loading

fabianfett Oct 3, 2022 •

edited

Loading