Обсуждение: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
[PATCH] Add additional extended protocol commands to psql: \parse and \bindx
От
Anthonin Bonnefoy
Дата:
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
Вложения
Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
От
Jelte Fennema-Nio
Дата:
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"
Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
От
Anthonin Bonnefoy
Дата:
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"
Вложения
Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
От
Jelte Fennema-Nio
Дата:
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.
Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
От
Jelte Fennema-Nio
Дата:
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.
Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
От
Jelte Fennema-Nio
Дата:
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
Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
От
Michael Paquier
Дата:
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
Вложения
Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
От
Anthonin Bonnefoy
Дата:
> 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.
Вложения
Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
От
Michael Paquier
Дата:
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
Вложения
Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
От
Anthonin Bonnefoy
Дата:
> 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.
Вложения
Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
От
Michael Paquier
Дата:
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