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

feat: add prefer execParams option #1172

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

olavloite
Copy link

Adds an option to skip the DescribeStatement step when the statement cache has been disabled. This reduces the number of round trips to the server by one. Instead the driver will do:

  1. Parse (including parameter OID types derived from the actual values)
  2. Bind
  3. Execute

The option has been enabled by default in this PR to verify that all tests succeed when this option is enabled. This should probably be changed to default off before merging to prevent an unexpected change in behavior.

Adds an option to skip the DescribeStatement step when the statement
cache has been disabled. This reduces the number of round trips to the
server by one.
@olavloite olavloite marked this pull request as ready for review March 25, 2022 14:59
@jackc
Copy link
Owner

jackc commented Mar 26, 2022

This feature is more subtle than it at first appears. I have recent knowledge of this because I have added this execution mode to pgx v5 a few weeks ago. In short, the binary format cannot be used without perfect knowledge of the parameter OIDs. Otherwise there can be data corruption with certain types such as Go time.Time to PostgreSQL date. See 1390a11.

@olavloite
Copy link
Author

@jackc Thanks for looking into this. I did not know that there was already an implementation for it in v5. That is interesting.

The point regarding the date/timestamp parameters is a good one. I should have checked that. I kind of assumed that date and timestamp would be sent using the text format by default.

A couple of questions regarding this:

  1. The implementation in v5 seems relatively cautious in the sense that it will always use text format. Is that observation correct? If so, what do you think of an implementation where the type mapping not only contains a mapping for Go type => OID, but also whether to use text or binary format for a given type? Where the default for types without any possible ambiguity could be binary.
  2. I noticed this comment:

    pgx/conn.go

    Line 629 in 1390a11

    // pgtype.Map.RegisterDefaultPgType. Queries will be rejected that have arguments that are unregistered or ambigious.
    How should I interpret that in relation to the inherit ambiguity with the mapping of both date and timestamp[tz] to time.Time? Won't that violate that rule? Or am I misunderstanding this comment?

And would you be open to an implementation in 4.x that takes the ambiguity of time.Time into account by specifically using the text format for those parameters? (I have not yet looked into how small/large that change would be, so I can't really say how feasible it would be yet)

@jackc
Copy link
Owner

jackc commented Apr 2, 2022

Yes, it always uses the text format. I am not confident in being able to safely determine that a type is unambiguous. I did not anticipate the problems with date / timestamp until a test detected the data corruption and I suspect there are more subtle pitfalls that would slip through.

For example, it would seem that Go int32 should unambiguously map to a PostgreSQL integer. But it doesn't. rune is a type alias of int32. But rune should be considered a one character string not a number.

Maybe that particular case is solvable, maybe not. But I'd rather error on the side of caution when the potential errors can lead to silent data corruption.

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.

2 participants