Обсуждение: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx

Hi all!

Currently, only unnamed prepared statements are supported by psql with the
\bind command and it's not possible to create or use named prepared statements
through extended protocol.

This patch introduces 2 additional commands: \parse and \bindx.
\parse allows us to issue a Parse message to create a named prepared statement
through extended protocol.
\bindx allows to bind and execute a named prepared statement through extended
protocol.

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

Regards,
Anthonin

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.

> +   HELP0("  \\bindx stmt_name [PARAM]...\n"

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


On Sat, Jan 13, 2024 at 3:37 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> 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.
>
> > +   HELP0("  \\bindx stmt_name [PARAM]...\n"

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

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.

Unless I misunderstand the remark, \bindx will call
PQsendQueryPrepared which will bind then execute the query, similar to
what \bind is doing (except \bind also parses the query).

> I'm not sure \bindx is such a great name.  The "x" stands for "I ran out of
> ideas". ;-)

That's definitely what happened :). \bind would have been a better fit
but it was already used.

On Thu, Jul 25, 2024 at 4:19 AM Michael Paquier <michael@paquier.xyz> wrote:
> 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.

+1 keeping this as a separate command and using \bind_named. \bind has
a different behaviour as it also parses the query so keeping them as
separate commands would probably avoid some confusion.