[PATCH] Add additional extended protocol commands to psql: \parse and \bindx

On Thu, 2 Nov 2023 at 10:52, Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:

The main goal is to provide more ways to test extended protocol in
regression tests
similarly to what \bind is doing.

I think this is a great addition. One thing that I think should be
added for completeness though is the ability to deallocate the
prepared statement using PQsendClosePrepared. Other than that the
changes look great.

Also a tiny nitpick: stmt_name should be replaced with STMT_NAME in
this line of the help message.

Hi,

Thanks for the review and comments.

One thing that I think should be added for completeness though is the
ability to deallocate the prepared statement using
PQsendClosePrepared. Other than that the changes look great.

Good point, I've added the \close command.

Also a tiny nitpick: stmt_name should be replaced with STMT_NAME in
this line of the help message.

Fixed

Looks really good now. One thing I noticed is that \bindx doesn't call
ignore_slash_options if it's not in an active branch. Afaict it
should. I do realize the same is true for plain \bind, but it seems
like a bug there too.

One more usability thing. I think \parse and \close should not require
a \g to send the message. You can do that by returning PSQL_CMD_SEND
instead of PSQL_CMD_SKIP_LIN.
I feel like the main point of requiring \g for \bind and \bindx is so
you can also use \gset or \gexec. But since \parse and \close don't
return any rows that argument does not apply to them.

And regarding the docs. I think the examples for \bindx and \close
should use \parse instead of PREPARE. ISTM that people will likely
want to use the extended query protocol for preparing and executing,
not a mix of them. I know that it's possible to do that, but I think
the examples should cover the most common use case.

On Tue, 16 Jan 2024 at 10:37, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:

Looks really good now. One thing I noticed is that \bindx doesn't call
ignore_slash_options if it's not in an active branch. Afaict it
should. I do realize the same is true for plain \bind, but it seems
like a bug there too.

To cover this case with tests you add your net commands to the big
list of meta commands in the "\if false" block on around line 1000 of
src/test/regress/sql/psql.sql

On Tue, Jan 16, 2024 at 10:37:22AM +0100, Jelte Fennema-Nio wrote:

I do realize the same is true for plain \bind, but it seems
like a bug there too.

Hmm. ignore_slash_options() is used to make the difference between
active and inactive branches with \if. I was playing a bit with
psql.sql but I don't really see a difference if for example adding
some \bind commands (say a valid SELECT $1 \bind 4) in the big "\if
false" that all the command types (see "vars and backticks").

Perhaps I am missing a trick?
--
Michael

I do realize the same is true for plain \bind, but it seems
like a bug there too.

The unscanned bind's parameters are discarded later in the
HandleSlashCmds functions. So adding the ignore_slash_options() for
inactive branches scans and discards them earlier. I will add it to
match what's done in the other commands but I don't think it's
testable as the behaviour is the same unless I miss something.

I did add the \bind, \bindx, \close and \parse to the inactive branch
tests to complete the list.

One more usability thing. I think \parse and \close should not require
a \g to send the message. You can do that by returning PSQL_CMD_SEND
instead of PSQL_CMD_SKIP_LIN

Changed.

I think the examples for \bindx and \close
should use \parse instead of PREPARE

Done. I had to rely on manual PREPARE for my first tests and it leaked
in the docs.

On Wed, Jan 17, 2024 at 10:05:33AM +0100, Anthonin Bonnefoy wrote:

I do realize the same is true for plain \bind, but it seems
like a bug there too.

The unscanned bind's parameters are discarded later in the
HandleSlashCmds functions. So adding the ignore_slash_options() for
inactive branches scans and discards them earlier. I will add it to
match what's done in the other commands but I don't think it's
testable as the behaviour is the same unless I miss something.

Hmm. So it does not lead to any user-visible changes, right? I can
get your argument about being consistent in the code across the board
for all the backslash commands, though.

I did add the \bind, \bindx, \close and \parse to the inactive branch
tests to complete the list.

Could you split the bits for \bind into a separate patch, please?
This requires a separate evaluation, especially if this had better be
backpatched.
--
Michael

Hmm. So it does not lead to any user-visible changes, right?

From what I can tell, there's no change in the behaviour. All paths
would eventually go through HandleSlashCmds's cleaning logic. This is
also mentioned in ignore_slash_options's comment.

* Read and discard "normal" slash command options.
*
* This should be used for inactive-branch processing of any slash command
* that eats one or more OT_NORMAL, OT_SQLID, or OT_SQLIDHACK parameters.
* We don't need to worry about exactly how many it would eat, since the
* cleanup logic in HandleSlashCmds would silently discard any extras anyway.

Could you split the bits for \bind into a separate patch, please?
This requires a separate evaluation, especially if this had better be
backpatched.

Done. patch 1 adds ignore_slash_options to bind. patch 2 adds the new
\bindx, \close and \parse commands.

On Thu, Jan 18, 2024 at 09:25:16AM +0100, Anthonin Bonnefoy wrote:

From what I can tell, there's no change in the behaviour. All paths
would eventually go through HandleSlashCmds's cleaning logic. This is
also mentioned in ignore_slash_options's comment.

Yeah, I can confirm that. I would be really tempted to backpatch that
because that's a bug: we have to call ignore_slash_options() for
inactive branches when a command parses options with OT_NORMAL. Now,
I cannot break things, either.

Done. patch 1 adds ignore_slash_options to bind. patch 2 adds the new
\bindx, \close and \parse commands.

0001 has been applied on HEAD.
--
Michael

On Fri, 19 Jan 2024 at 10:50, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Jan 18, 2024 at 09:25:16AM +0100, Anthonin Bonnefoy wrote:

From what I can tell, there's no change in the behaviour. All paths
would eventually go through HandleSlashCmds's cleaning logic. This is
also mentioned in ignore_slash_options's comment.

Yeah, I can confirm that. I would be really tempted to backpatch that
because that's a bug: we have to call ignore_slash_options() for
inactive branches when a command parses options with OT_NORMAL. Now,
I cannot break things, either.

Done. patch 1 adds ignore_slash_options to bind. patch 2 adds the new
\bindx, \close and \parse commands.

0001 has been applied on HEAD.

Since the 0001 patch has been applied, sending only 0002 as v5-0001 so
that CFBot can apply and run.

Regards,
Vignesh

Hi,

shall we do something about this patch? It seems to be in a pretty good
shape (pretty much RFC, based on quick review), the cfbot is still
happy, and there seems to be agreement this is a nice feature.

Michael, I see you've reviewed the patch in January. Do you agree / plan
to get it committed, or should I take a look?

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jul 19, 2024 at 12:17:44AM +0200, Tomas Vondra wrote:

shall we do something about this patch? It seems to be in a pretty good
shape (pretty much RFC, based on quick review), the cfbot is still
happy, and there seems to be agreement this is a nice feature.

Michael, I see you've reviewed the patch in January. Do you agree / plan
to get it committed, or should I take a look?

This feel off my radar a bit, thanks for the reminder :)

I have a local branch dating back from January where this patch is
sitting, with something like 50% of the code reviewed. I'd still need
to look at the test coverage, but I did like the proposed patch a lot
based on my notes.

I may be able to come back to that if not next week, then the week
after that. If you want to handle it yourself before that, that's
fine by me.
--
Michael

On 7/19/24 04:23, Michael Paquier wrote:

On Fri, Jul 19, 2024 at 12:17:44AM +0200, Tomas Vondra wrote:

shall we do something about this patch? It seems to be in a pretty good
shape (pretty much RFC, based on quick review), the cfbot is still
happy, and there seems to be agreement this is a nice feature.

Michael, I see you've reviewed the patch in January. Do you agree / plan
to get it committed, or should I take a look?

This feel off my radar a bit, thanks for the reminder :)

I have a local branch dating back from January where this patch is
sitting, with something like 50% of the code reviewed. I'd still need
to look at the test coverage, but I did like the proposed patch a lot
based on my notes.

I may be able to come back to that if not next week, then the week
after that. If you want to handle it yourself before that, that's
fine by me.

OK, if you're already half-way through the review, I'll leave it up to
you. I don't think we need to rush, and I'd have to learn about all the
psql stuff first anyway.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jul 19, 2024 at 03:28:44PM +0200, Tomas Vondra wrote:

OK, if you're already half-way through the review, I'll leave it up to
you. I don't think we need to rush, and I'd have to learn about all the
psql stuff first anyway.

It took me a couple of days to get back to it, but attached is what I
have finished with. This was mostly OK, except for a few things:
- \close was inconsistent with the other two commands, where no
argument was treated as the unnamed prepared statement. I think that
this should be made consistent with \parse and \bindx, requiring an
argument, where '' is the unnamed statement.
- The docs did not mention the case of the unnamed statement, so added
some notes about that.
- Some free() calls were not needed in the command executions, where
psql_scan_slash_option() returns NULL.
- Tests missing when no argument is provided for the new commands.

One last thing I have found really confusing is that this leads to the
addition of two more status flags in pset for the close and parse
parts, with \bind and \bindx sharing the third one while deciding
which path to use depending on if the statement name is provided.
That's fragile. I think that it would be much cleaner to put all that
behind an enum, falling back to PQsendQuery() by default. I am
attaching that as 0002, for clarity, but my plan is to merge both 0001
and 0002 together.
--
Michael

On Fri, Jul 19, 2024 at 03:28:44PM +0200, Tomas Vondra wrote:

OK, if you're already half-way through the review, I'll leave it up to
you. I don't think we need to rush, and I'd have to learn about all the
psql stuff first anyway.

It took me a couple of days to get back to it, but attached is what I
have finished with. This was mostly OK, except for a few things:
- \close was inconsistent with the other two commands, where no
argument was treated as the unnamed prepared statement. I think that
this should be made consistent with \parse and \bindx, requiring an
argument, where '' is the unnamed statement.
- The docs did not mention the case of the unnamed statement, so added
some notes about that.
- Some free() calls were not needed in the command executions, where
psql_scan_slash_option() returns NULL.
- Tests missing when no argument is provided for the new commands.

One last thing I have found really confusing is that this leads to the
addition of two more status flags in pset for the close and parse
parts, with \bind and \bindx sharing the third one while deciding
which path to use depending on if the statement name is provided.
That's fragile. I think that it would be much cleaner to put all that
behind an enum, falling back to PQsendQuery() by default. I am
attaching that as 0002, for clarity, but my plan is to merge both 0001
and 0002 together.
--
Michael

Hi,

It took me a couple of days to get back to it, but attached is what I
have finished with. This was mostly OK, except for a few things:
- \close was inconsistent with the other two commands, where no
argument was treated as the unnamed prepared statement. I think that
this should be made consistent with \parse and \bindx, requiring an
argument, where '' is the unnamed statement.
- The docs did not mention the case of the unnamed statement, so added
some notes about that.
- Some free() calls were not needed in the command executions, where
psql_scan_slash_option() returns NULL.
- Tests missing when no argument is provided for the new commands.

One last thing I have found really confusing is that this leads to the
addition of two more status flags in pset for the close and parse
parts, with \bind and \bindx sharing the third one while deciding
which path to use depending on if the statement name is provided.
That's fragile. I think that it would be much cleaner to put all that
behind an enum, falling back to PQsendQuery() by default. I am
attaching that as 0002, for clarity, but my plan is to merge both 0001
and 0002 together.

I reviewed and tested v6. I believe it's ready to be merged.

--
Best regards,
Aleksander Alekseev

On 24.07.24 07:04, Michael Paquier wrote:

This commit introduces three additional commands: \parse, \bindx and
\close.
\parse creates a prepared statement using extended protocol.
\bindx binds and execute an existing prepared statement using extended
protocol.
\close closes an existing prepared statement using extended protocol.

This commit message confused me, because I don't think this is what the
\bindx command actually does. AFAICT, it only binds, it does not
execute. At least that is what the documentation in the content of the
patch appears to indicate.

I'm not sure \bindx is such a great name. The "x" stands for "I ran out
of ideas". ;-) Maybe \bind_named or \bindn or something like that. Or
use the existing \bind with a -name argument?

On Wed, Jul 24, 2024 at 05:33:07PM +0200, Peter Eisentraut wrote:

This commit message confused me, because I don't think this is what the
\bindx command actually does. AFAICT, it only binds, it does not execute.
At least that is what the documentation in the content of the patch appears
to indicate.

Yep. FWIW, I always edit these before commit, and noticed that it was
incorrect. Just took the original message for now.

I'm not sure \bindx is such a great name. The "x" stands for "I ran out of
ideas". ;-) Maybe \bind_named or \bindn or something like that. Or use the
existing \bind with a -name argument?

Not sure that I like much the additional option embedded in the
existing command; I'd rather keep a separate command for each libpq
call, that seems cleaner. So I would be OK with your suggested
\bind_named. Fine by me to be outvoted, of course.
--
Michael