Horton

Thread

Commits

  1. psql: Rename meta-command \close to \close_prepared

  2. psql: Add tests for repeated calls of \bind[_named]

  3. psql: Fix memory leak with repeated calls of \bind

  4. psql: Clean up more aggressively state of \bind[_named], \parse and \close

  5. psql: Add more meta-commands able to use the extended protocol

  6. psql: Add ignore_slash_options in bind's inactive branch

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

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2023-11-02T09:52:36Z

    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

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-01-13T14:37:31Z

    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"

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

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2024-01-16T08:13:17Z

    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"

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-01-16T09:37:22Z

    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.

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-01-16T12:51:59Z

    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.

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-01-16T12:57:44Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2024-01-17T07:28:52Z

    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

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

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2024-01-17T09:05:33Z

    > 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.

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

    Michael Paquier <michael@paquier.xyz> — 2024-01-18T01:29:11Z

    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

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

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2024-01-18T08:25:16Z

    > 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.

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

    Michael Paquier <michael@paquier.xyz> — 2024-01-19T05:20:31Z

    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

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

    vignesh C <vignesh21@gmail.com> — 2024-01-27T03:26:53Z

    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

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2024-07-18T22:17:44Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2024-07-19T02:23:08Z

    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

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2024-07-19T13:28:44Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2024-07-24T05:04:05Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2024-07-24T05:07:21Z

    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

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

    Aleksander Alekseev <aleksander@timescale.com> — 2024-07-24T12:19:52Z

    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

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

    Peter Eisentraut <peter@eisentraut.org> — 2024-07-24T15:33:07Z

    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?

  20. Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

    Michael Paquier <michael@paquier.xyz> — 2024-07-25T02:19:08Z

    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

  21. Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2024-07-25T06:45:28Z

    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.

  22. Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-08-20T22:00:06Z

    On Thu, 25 Jul 2024 at 08:45, Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
> +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.

+1 on naming it \bind_named

@Anthonin are you planning to update the patch accordingly?

  23. Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2024-08-21T07:29:04Z

    On Wed, Aug 21, 2024 at 12:00 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> @Anthonin are you planning to update the patch accordingly?

Here's the patch with \bindx renamed to \bind_named.

I've also made a small change to Michael's refactoring in 0002 by
initialising success to false in ExecQueryAndProcessResults. There was
a compiler warning about success possibly used uninitialized[1].

[1] https://cirrus-ci.com/task/6207675187331072

  24. Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

    Michael Paquier <michael@paquier.xyz> — 2024-08-22T07:33:54Z

    On Wed, Aug 21, 2024 at 09:29:04AM +0200, Anthonin Bonnefoy wrote:
> Here's the patch with \bindx renamed to \bind_named.

Looks OK to me.  I have spent more time double-checking the whole, and
it looks like we're there, so applied.  Now let's play with it in more
regression tests.  Note that the refactoring patch has been merged
with the original one in a single commit.

> I've also made a small change to Michael's refactoring in 0002 by
> initialising success to false in ExecQueryAndProcessResults. There was
> a compiler warning about success possibly used uninitialized[1].
> 
> [1] https://cirrus-ci.com/task/6207675187331072

Ah, thanks!  I've missed this one.  I see where my mistake was.
--
Michael

  25. Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

    Alexander Law <exclusion@gmail.com> — 2024-09-17T15:00:00Z

    Hello Michael and Anthonin,

22.08.2024 10:33, Michael Paquier wrote:
> Looks OK to me.  I have spent more time double-checking the whole, and
> it looks like we're there, so applied.  Now let's play with it in more
> regression tests.  Note that the refactoring patch has been merged
> with the original one in a single commit.

Please look at an assertion failure, caused by \bind_named:
regression=# SELECT $1 \parse s
\bind_named s

regression=# \bind_named
\bind_named: missing required argument
regression=# 1 \g
psql: common.c:1501: ExecQueryAndProcessResults: Assertion `pset.stmtName != ((void *)0)' failed.

Best regards,
Alexander

  26. Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2024-09-18T07:42:43Z

    On Tue, Sep 17, 2024 at 5:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:
>
> Please look at an assertion failure, caused by \bind_named:
> regression=# SELECT $1 \parse s
> \bind_named s
>
> regression=# \bind_named
> \bind_named: missing required argument
> regression=# 1 \g
> psql: common.c:1501: ExecQueryAndProcessResults: Assertion `pset.stmtName != ((void *)0)' failed.

Thanks for the report.

Looking at the failure, it seems like the issue was already present
with \bind, though there was no assertion failure: repeatedly calling
\bind would allocate new stmtName/bind_params and leak them at the
start of exec_command_bind.

I've joined a patch to clean the psql extended state at the start of
every extended protocol backslash command, freeing the allocated
variables and resetting the send_mode. Another possible approach would
be to return an error when there's already an existing state instead
of overwriting it.

  27. Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

    Michael Paquier <michael@paquier.xyz> — 2024-09-18T09:08:54Z

    On Wed, Sep 18, 2024 at 09:42:43AM +0200, Anthonin Bonnefoy wrote:
> Looking at the failure, it seems like the issue was already present
> with \bind, though there was no assertion failure: repeatedly calling
> \bind would allocate new stmtName/bind_params and leak them at the
> start of exec_command_bind.

Indeed.  That's a bad idea to do that in the client.  We'd better
back-patch that.

> I've joined a patch to clean the psql extended state at the start of
> every extended protocol backslash command, freeing the allocated
> variables and resetting the send_mode. Another possible approach would
> be to return an error when there's already an existing state instead
> of overwriting it.

I'll double-check all that tomorrow, but you have looks like it is
going in the right direction.
--
Michael

  28. Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

    Michael Paquier <michael@paquier.xyz> — 2024-09-19T07:30:11Z

    On Wed, Sep 18, 2024 at 06:08:54PM +0900, Michael Paquier wrote:
> On Wed, Sep 18, 2024 at 09:42:43AM +0200, Anthonin Bonnefoy wrote:
>> I've joined a patch to clean the psql extended state at the start of
>> every extended protocol backslash command, freeing the allocated
>> variables and resetting the send_mode. Another possible approach would
>> be to return an error when there's already an existing state instead
>> of overwriting it.
> 
> I'll double-check all that tomorrow, but you have looks like it is
> going in the right direction.

And done down to v16, with one logic for HEAD and something simpler
for \bind in v16 and v17.

Issuing an error if there is a state does not sound like a good idea
at this stage because it would suddenly break scripts that expect
multiple commands of \bind to prioritize the last one.  If that was
something only on HEAD, I would have considered that as a serious
option, but not with v16 in mind for \bind.
--
Michael

  29. Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-09-19T08:53:07Z

    On Thu, 19 Sept 2024 at 09:30, Michael Paquier <michael@paquier.xyz> wrote:
> Issuing an error if there is a state does not sound like a good idea
> at this stage because it would suddenly break scripts that expect
> multiple commands of \bind to prioritize the last one.

Seems like a good idea to add a simple test for that behaviour then.
See attached.

On Thu, 19 Sept 2024 at 09:30, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Sep 18, 2024 at 06:08:54PM +0900, Michael Paquier wrote:
> > On Wed, Sep 18, 2024 at 09:42:43AM +0200, Anthonin Bonnefoy wrote:
> >> I've joined a patch to clean the psql extended state at the start of
> >> every extended protocol backslash command, freeing the allocated
> >> variables and resetting the send_mode. Another possible approach would
> >> be to return an error when there's already an existing state instead
> >> of overwriting it.
> >
> > I'll double-check all that tomorrow, but you have looks like it is
> > going in the right direction.
>
> And done down to v16, with one logic for HEAD and something simpler
> for \bind in v16 and v17.
>
> Issuing an error if there is a state does not sound like a good idea
> at this stage because it would suddenly break scripts that expect
> multiple commands of \bind to prioritize the last one.  If that was
> something only on HEAD, I would have considered that as a serious
> option, but not with v16 in mind for \bind.
> --
> Michael

  30. Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bind

    Michael Paquier <michael@paquier.xyz> — 2024-09-19T23:59:51Z

    On Thu, Sep 19, 2024 at 10:53:07AM +0200, Jelte Fennema-Nio wrote:
> Seems like a good idea to add a simple test for that behaviour then.
> See attached.

Thanks.  The same can be said for \bind_named, so I have added
something for both \bind and \bind_named.
--
Michael

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

    Peter Eisentraut <peter@eisentraut.org> — 2025-06-12T13:14:22Z

    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.

I'm wondering now if \close is a good name of this command.  I hadn't 
seen any discussion around the name choice in this thread.

On the SQL level, the CLOSE command operates on cursors.  I think it 
would be useful to keep SQL and psql aligned (e.g., COPY, \copy).  On 
the protocol level, the "Close" message operates on both cursors and 
statements, but those are actually two different message (sub-)types. 
And this is not something users ever see, so the connection would not be 
obvious.  Maybe this should be called something more specific like 
\close_stmt.

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

    Greg Sabino Mullane <htamfids@gmail.com> — 2025-06-13T01:53:13Z

    On Thu, Jun 12, 2025 at 9:14 AM Peter Eisentraut <peter@eisentraut.org>
wrote:

> And this is not something users ever see, so the connection would not be
> obvious.  Maybe this should be called something more specific like
> \close_stmt.
>

Maybe just \closeprepared ?

Cheers,
Greg

--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support

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

    Michael Paquier <michael@paquier.xyz> — 2025-06-13T02:56:37Z

    On Thu, Jun 12, 2025 at 09:53:13PM -0400, Greg Sabino Mullane wrote:
> On Thu, Jun 12, 2025 at 9:14 AM Peter Eisentraut <peter@eisentraut.org>
> wrote:
>> And this is not something users ever see, so the connection would not be
>> obvious.  Maybe this should be called something more specific like
>> \close_stmt.
> 
> Maybe just \closeprepared ?

I'm OK with a rename if people feel strongly about it and we still
have the time to do tweaks like that, but I don't like the suggestions
\close_stmt and \closeprepared, because that's inconsistent with the
other new meta-commands.

What about \close_named to be consistent with \bind_named?  We always
require a statement name when closing a prepared statement.
--
Michael

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

    Peter Eisentraut <peter@eisentraut.org> — 2025-06-13T13:57:42Z

    On 13.06.25 04:56, Michael Paquier wrote:
> On Thu, Jun 12, 2025 at 09:53:13PM -0400, Greg Sabino Mullane wrote:
>> On Thu, Jun 12, 2025 at 9:14 AM Peter Eisentraut <peter@eisentraut.org>
>> wrote:
>>> And this is not something users ever see, so the connection would not be
>>> obvious.  Maybe this should be called something more specific like
>>> \close_stmt.
>>
>> Maybe just \closeprepared ?
> 
> I'm OK with a rename if people feel strongly about it and we still
> have the time to do tweaks like that, but I don't like the suggestions
> \close_stmt and \closeprepared, because that's inconsistent with the
> other new meta-commands.
> 
> What about \close_named to be consistent with \bind_named?  We always
> require a statement name when closing a prepared statement.

That doesn't address the concern that it's confusing what kind of object 
\close operates on.  There are named and unnamed cursors (= portals), 
after all.

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-06-17T14:05:45Z

    On Fri, 13 Jun 2025 at 15:57, Peter Eisentraut <peter@eisentraut.org> wrote:
> That doesn't address the concern that it's confusing what kind of object
> \close operates on.  There are named and unnamed cursors (= portals),
> after all.

Agreed. My vote is for \closeprepared as that aligns with the libpq function.

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

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-06-18T06:23:13Z

    On Tue, Jun 17, 2025 at 4:05 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> Agreed. My vote is for \closeprepared as that aligns with the libpq function.

Since \bind_named is also new, we can also rename it to make it
consistent with close meta-command. So what about renaming \bind_named
to \bindprepared and \close to \closeprepared?

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-06-18T07:15:07Z

    On Wed, 18 Jun 2025 at 08:23, Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
>
> On Tue, Jun 17, 2025 at 4:05 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> >
> > Agreed. My vote is for \closeprepared as that aligns with the libpq function.
>
> Since \bind_named is also new, we can also rename it to make it
> consistent with close meta-command. So what about renaming \bind_named
> to \bindprepared and \close to \closeprepared?

I think I still prefer \bind_named or maybe \bindnamed (depending on
what our policy for underscores in \ commands is). For that command it
should differentiate from the already existing \bind command, which is
also for prepared statements, just not for "named" prepared
statements. While close needs to differentiate from close for portals
vs close for prepared statements.

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

    Michael Paquier <michael@paquier.xyz> — 2025-06-19T07:09:21Z

    On Wed, Jun 18, 2025 at 09:15:07AM +0200, Jelte Fennema-Nio wrote:
> On Wed, 18 Jun 2025 at 08:23, Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> wrote:
>> Since \bind_named is also new, we can also rename it to make it
>> consistent with close meta-command. So what about renaming \bind_named
>> to \bindprepared and \close to \closeprepared?
> 
> I think I still prefer \bind_named or maybe \bindnamed (depending on
> what our policy for underscores in \ commands is).

Not sure that there is such a policy in place.  I find names with
underscores easier to parse.

> For that command it
> should differentiate from the already existing \bind command, which is
> also for prepared statements, just not for "named" prepared
> statements. While close needs to differentiate from close for portals
> vs close for prepared statements.

Good point.  I would be on board with a \close_prepared then, if
that's the consensus we reach, without touching at \bind_named.  We
still have time to decide on the name until the release, just let's
make sure to not do a rename multiple times.
--
Michael

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

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-06-23T07:04:15Z

    On Thu, Jun 19, 2025 at 9:09 AM Michael Paquier <michael@paquier.xyz> wrote:
> Good point.  I would be on board with a \close_prepared then, if
> that's the consensus we reach, without touching at \bind_named.  We
> still have time to decide on the name until the release, just let's
> make sure to not do a rename multiple times.

True. That would also allow adding a \close_cursor meta-command
without name ambiguity.

Since the consensus seems to lean toward \close_prepared, I've
prepared the patch to rename the command. I haven't modified the
traduction files though, I think they're updated through a different
process?

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-06-23T07:21:46Z

    On Thu, 19 Jun 2025 at 09:09, Michael Paquier <michael@paquier.xyz> wrote:
> > I think I still prefer \bind_named or maybe \bindnamed (depending on
> > what our policy for underscores in \ commands is).
>
> Not sure that there is such a policy in place.  I find names with
> underscores easier to parse.

I just double checked and there are already psql meta-commands with
underscores. So it seems fine to have underscores in our new commands
too.

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

    Michael Paquier <michael@paquier.xyz> — 2025-06-23T07:40:47Z

    On Mon, Jun 23, 2025 at 09:04:15AM +0200, Anthonin Bonnefoy wrote:
> Since the consensus seems to lean toward \close_prepared, I've
> prepared the patch to rename the command.

This renaming patch looks correct to me.  I am not seeing any missing
references of \close remaining, including APIs, comments and docs.

> I haven't modified the
> traduction files though, I think they're updated through a different
> process?

Traduction files are all updated in a single batch on a regular basis,
using the contents of the translation git repo.  There is no need to
touch them when it comes to hacking the core tree.
--
Michael

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-06-23T07:44:29Z

    On Mon, 23 Jun 2025 at 09:40, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Jun 23, 2025 at 09:04:15AM +0200, Anthonin Bonnefoy wrote:
> > Since the consensus seems to lean toward \close_prepared, I've
> > prepared the patch to rename the command.
>
> This renaming patch looks correct to me.  I am not seeing any missing
> references of \close remaining, including APIs, comments and docs.

Agreed

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

    Peter Eisentraut <peter@eisentraut.org> — 2025-06-23T13:25:32Z

    On 23.06.25 09:04, Anthonin Bonnefoy wrote:
> On Thu, Jun 19, 2025 at 9:09 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Good point.  I would be on board with a \close_prepared then, if
>> that's the consensus we reach, without touching at \bind_named.  We
>> still have time to decide on the name until the release, just let's
>> make sure to not do a rename multiple times.
> 
> True. That would also allow adding a \close_cursor meta-command
> without name ambiguity.
> 
> Since the consensus seems to lean toward \close_prepared, I've
> prepared the patch to rename the command.

Looks good to me, thanks.

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

    Michael Paquier <michael@paquier.xyz> — 2025-06-24T04:14:55Z

    On Mon, Jun 23, 2025 at 09:44:29AM +0200, Jelte Fennema-Nio wrote:
> On Mon, 23 Jun 2025 at 09:40, Michael Paquier <michael@paquier.xyz> wrote:
>> This renaming patch looks correct to me.  I am not seeing any missing
>> references of \close remaining, including APIs, comments and docs.
> 
> Agreed

Applied.
--
Michael