Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update "write" function to work with pgx4 and higher #96

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

hlcianfagna
Copy link
Contributor

@hlcianfagna hlcianfagna commented Oct 9, 2023

Summary of the changes / Why this is an improvement

Update to write function to work with pgx4

Closes: #88

Checklist

  • Link to issue this PR refers to (if applicable):
  • CLA is signed

@hlcianfagna hlcianfagna marked this pull request as ready for review October 9, 2023 15:59
@hammerhead
Copy link
Member

hammerhead commented Oct 10, 2023

I gave the patch a try but still ran into the originally reported error message.
In the pgx documentation I then found a note that using prepared statements is typically not necessary anymore, which seems to be new since v4:

However, this is rarely necessary because pgx includes an automatic statement cache by default

After replacing the prepared statement with a regular batched insert, I ran into error closing write batch: context deadline exceeded. A fix for that was to comment out this line:

// The deadlock occurs when the batched queries to be sent are so large that the PostgreSQL
// server cannot receive it all at once. PostgreSQL received some queued queries and starts
// executing them. As PostgreSQL executes the queries it sends responses back. pgx will not
// read any of these responses until it has finished sending. Therefore, if all network
// buffers are full, pgx will not be able to finish sending the queries, and PostgreSQL will
// not be able to finish sending the responses.
//
// -- https://github.com/jackc/pgx/blob/v3.6.2/batch.go#L58-L79
//
ctx, _ = context.WithTimeout(ctx, c.writeTimeout)

Only after that, I could see data arriving in the target table. The link in that context refers to pgx v3.6.2, and I cannot find that comment in the pgx source code anymore in v5. Sounds like that concern is obsolete as well?

@hlcianfagna hlcianfagna changed the title Remove parameterOIDs and resultFormatCodes Update to write function to work with pgx4 Dec 27, 2023
@hlcianfagna hlcianfagna changed the title Update to write function to work with pgx4 Update to write function to work with pgx4 and higher Dec 27, 2023
@hlcianfagna
Copy link
Contributor Author

I got this working now in my test environment, could you take another look at it @hammerhead ?

@hammerhead
Copy link
Member

Looks good from a functional perspective. When running go run . -config.file ../config.yml on that branch, I can see data coming in successfully 👍.

@hlcianfagna
Copy link
Contributor Author

I also looked at the possibility of adding a test case to test this function but it seems pgxmock would not currently support this ( pashagolub/pgxmock#152 )

@amotl
Copy link
Member

amotl commented Dec 27, 2023

Hi. Thank you so much for discovering the root cause of GH-88, and for submitting this patch and confirming it works well. I think it will be good to merge it without further ado.

Can I ask you to squash your commits, @hlcianfagna? Otherwise, I'll just do it, hoping that you don't have any objections about it?

Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged, thank you. 💯

@amotl amotl force-pushed the hlcianfagna/issue88 branch from 93769a1 to cc06473 Compare December 27, 2023 22:19
@amotl amotl merged commit b36cb38 into main Dec 27, 2023
6 checks passed
@amotl amotl deleted the hlcianfagna/issue88 branch December 27, 2023 22:22
@amotl amotl changed the title Update to write function to work with pgx4 and higher Update "write" function to work with pgx4 and higher Dec 27, 2023
@amotl amotl mentioned this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error building query write_statement: mismatched param and argument count
3 participants