Обсуждение: [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