Skip to content

feat: allow explicit use of Simple Query Protocol (v3) #118

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

Merged
merged 4 commits into from
Oct 1, 2023

Conversation

osaxma
Copy link
Contributor

@osaxma osaxma commented Aug 19, 2023

The Simple Query Protocol is the only protocol that can be used when the connection is in Replication Mode in order to query system information such as IDENTIFY_SYSTEM; query.

Currently, the only way to use the Simple Protocol is to set ignoreRows to true but this will return no result and there's no way around it.

While taking @simolus3 comment from PR #102 into consideration, I added an option to explicit use the Simple Query Protocol. In addition, it is used when there are no parameters and ignoreRows is set to true.

@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (cb091e0) 86.56% compared to head (703c909) 87.26%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   86.56%   87.26%   +0.70%     
==========================================
  Files          29       29              
  Lines        2493     2498       +5     
==========================================
+ Hits         2158     2180      +22     
+ Misses        335      318      -17     
Files Coverage Δ
lib/postgres_v3_experimental.dart 51.16% <ø> (ø)
lib/src/text_codec.dart 93.85% <100.00%> (+13.15%) ⬆️
lib/src/v3/connection.dart 85.75% <100.00%> (+0.76%) ⬆️
lib/src/v3/pool.dart 87.30% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

///
/// When [useSimpleQueryProtocol] is set to true, the implementation will use
/// the Simple Query Protocol. Please note, a query with [parameters] cannot
/// be used with this protocol.
Future<PgResult> execute(
Object /* String | PgSql */ query, {
Object? /* List<Object?|PgTypedParameter> | Map<String, Object?|PgTypedParameter> */
parameters,
bool ignoreRows = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we'll receive the rows anyway and just chose to ignore them, perhaps the ignoreRows parameter doesn't make much sense anymore when we have a useSimpleQueryProtocol parameter as well.

So maybe the ignoreRows parameter should just be removed in favor of useSimpleQueryProtocol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure what is the use of ignoreRows so I left it as is.. I will take a closer look later today and update the PR accordingly.

I will also expand on the docs for useSimpleQueryProtocol and mention some of its use cases.

Copy link
Owner

Choose a reason for hiding this comment

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

If possible, I would like to keep ignoreRows, because if the caller needs only a non-error execution, we can spare a bit of processing. I don't mind having useSimpleQueryProtocol but only if that's the only way to do it (see my main comment about having it at connection level or with a callback).

@isoos
Copy link
Owner

isoos commented Aug 20, 2023

The Simple Query Protocol is the only protocol that can be used when the connection is in Replication Mode in order to query system information such as IDENTIFY_SYSTEM; query.

I have no idea about this, but can a connection switch between replication mode and normal mode? If it can't, it would seem to be better to make it a connection-level attribute to always use the simple query protocol.

And even if it can, I would assume that it is something that can be turned on, and from that onward, every query must be sent like that. Having to use an explicit useSimpleQueryProtocol for each query seems to be error-prone, it would be better to force everything inside a callback. Similar to the PgPool.withConnection, or the run and runTx method, it could be a callback-style API that gets a PgSession which is then forced to use the simple query protocol at every query.

wdyt?

@osaxma
Copy link
Contributor Author

osaxma commented Aug 21, 2023

can a connection switch between replication mode and normal mode?

No, a connection must be established as either one since it's defined in the StartUpMessage.

But within a session, either Query protocol can be used. Definitely, the default one should be as it is which is the Extended Query Protocol.

I am not sure about the other cases where the Simple Query Protocol is preferable or a must over the Extended one, but it is obviously an execution option.

Having to use an explicit useSimpleQueryProtocol for each query seems to be error-prone

I agree with you having done the mistake few times already but, in my case, the server returns a clear error message (e.g. extended query protocol cannot be used in logical replication...etc). I am not sure about the cases where one would use both protocols from the same connection and how sporadic that would look.

Another option is to have a dedicated method (e.g. PgConnection.executeSQP(...)) as a straightforward and simple option. Also, in case we come across additional options for how the simple protocol could be executed, we can add it as optional parameters for that method.

Please let me know what do you prefer. I am fine with any option as long as it's available.

for reference, I came across this nicely written article that goes into details about the differences between the two protocols: Wire Protocol of PostgreSQL Queries in a Nutshell.


About ignoreRows, I don't have an opinion there and I think keeping it won't harm.

@osaxma
Copy link
Contributor Author

osaxma commented Aug 25, 2023

I just looked at pgx implementation for reference, and they do have two options available for users to set the Query protocol:

  • a connection configuration for setting the default query protocol
  • a per query parameter to override the connection configuration

... as explained in their docs:

DefaultQueryExecMode controls the default mode for executing queries. By default pgx uses the extended protocol
and automatically prepares and caches prepared statements. However, this may be incompatible with proxies such as
PGBouncer. In this case it may be preferrable to use QueryExecModeExec or QueryExecModeSimpleProtocol. The same
functionality can be controlled on a per query basis by passing a QueryExecMode as the first query argument.

from: https://pkg.go.dev/github.com/jackc/pgx/[email protected]#ConnConfig

If that's a preferable approach, we can create an enum e.g. QueryExecMode, and have it as a member of PgSettings as a connection default in addition to having it as an optional parameter in PgConnection.execute for overriding it.

@isoos
Copy link
Owner

isoos commented Aug 25, 2023

Maybe the name can be as short as QueryMode with values of simple and extended? Thanks for the links, reading a bit more upon it, having it as connection-level default value + query-level override seems to be reasonable.

@isoos
Copy link
Owner

isoos commented Oct 1, 2023

@osaxma: I think this is close - only a single test started to fail. Could you please check it?
I am undecided to change QueryMode into [Pg]QueryProtocol instead, but that may come later, as we make sure everything in v3 is uniformly named.

@osaxma osaxma force-pushed the fix_simple_query_protocol branch from e82d7e0 to 703c909 Compare October 1, 2023 11:26
@osaxma
Copy link
Contributor Author

osaxma commented Oct 1, 2023

I think this is close - only a single test started to fail. Could you please check it?

@isoos there were some merge conflicts and my branch wasn't up to date. I am not sure why when you merged with master, the conflicts didn't show up.

I am undecided to change QueryMode into [Pg]QueryProtocol instead, but that may come later, as we make sure everything in v3 is uniformly named.

I think it's a good idea to have an umbrella issue mentioning all these stuff that we would like to cleanup later so we don't forget.

@isoos isoos merged commit 73dd0d7 into isoos:master Oct 1, 2023
@osaxma osaxma deleted the fix_simple_query_protocol branch October 1, 2023 14:38
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.

4 participants