Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. psql: Fix assertion failures with pipeline mode

  2. psql: Rework TAP routine psql_fails_like() to define WAL sender context

  3. psql: Split extended query protocol meta-commands in --help=commands

  4. psql: Improve descriptions of \\flush[request] in --help

  5. psql: Fix incorrect status code returned by \getresults

  6. psql: Allow queries terminated by semicolons while in pipeline mode

  7. psql: Add \sendpipeline to send query buffers while in a pipeline

  8. psql: Fix memory leak with \gx used within a pipeline

  9. psql: Add pipeline status to prompt and some state variables

  10. Add more tests for utility commands in pipelines

  11. psql: Add support for pipelines

  12. Add braces for if block with large comment in psql's common.c

  1. Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2024-11-27T09:50:25Z

    Hi,

With \bind, \parse, \bind_named and \close, it is possible to issue
queries from psql using the extended protocol. However, it wasn't
possible to send those queries using pipelining and the only way to
test pipelined queries was through pgbench's tap tests.

The attached patch adds pipelining support to psql with 3 new
meta-commands, mirroring what's already done in pgbench:
- \startpipeline starts a new pipeline. All extended queries will be
queued until the end of the pipeline is reached.
- \endpipeline ends an ongoing pipeline. All queued commands will be
sent to the server and all responses will be processed by the psql.
- \syncpipeline queue a synchronisation point without flushing the
commands to the server

Those meta-commands will allow testing pipelined query behaviour using
psql regression tests.

Regards,
Anthonin

  2. Re: Add Pipelining support in psql

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-27T10:46:36Z

    On Wed, 27 Nov 2024 at 14:50, Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
>
> Hi,
>
> With \bind, \parse, \bind_named and \close, it is possible to issue
> queries from psql using the extended protocol. However, it wasn't
> possible to send those queries using pipelining and the only way to
> test pipelined queries was through pgbench's tap tests.

Hello, good concept. Our connection pooler testing will greatly
benefit from this feature. At the moment, our tests are golang-based
and build pipelines in extended proto and verify the outcome.
Regression tests based on SQL will be far more thorough and organic.

> The attached patch adds pipelining support to psql with 3 new
> meta-commands, mirroring what's already done in pgbench:
> - \startpipeline starts a new pipeline. All extended queries will be
> queued until the end of the pipeline is reached.
> - \endpipeline ends an ongoing pipeline. All queued commands will be
> sent to the server and all responses will be processed by the psql.
> - \syncpipeline queue a synchronisation point without flushing the
> commands to the server

I'm very doubtful about the \syncpipeline . Maybe we should instead
support \sync meta-command in psql? This will be a useful contribution
itself.

> Those meta-commands will allow testing pipelined query behaviour using
> psql regression tests.
>
> Regards,
> Anthonin

I haven't looked into the patch in detail yet.

-- 
Best regards,
Kirill Reshke

  3. Re: Add Pipelining support in psql

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-27T12:04:58Z

    On Wed, 27 Nov 2024 at 14:50, Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
>
> Hi,
>
> With \bind, \parse, \bind_named and \close, it is possible to issue
> queries from psql using the extended protocol. However, it wasn't
> possible to send those queries using pipelining and the only way to
> test pipelined queries was through pgbench's tap tests.
>
> The attached patch adds pipelining support to psql with 3 new
> meta-commands, mirroring what's already done in pgbench:
> - \startpipeline starts a new pipeline. All extended queries will be
> queued until the end of the pipeline is reached.
> - \endpipeline ends an ongoing pipeline. All queued commands will be
> sent to the server and all responses will be processed by the psql.
> - \syncpipeline queue a synchronisation point without flushing the
> commands to the server
>
> Those meta-commands will allow testing pipelined query behaviour using
> psql regression tests.
>
> Regards,
> Anthonin

Hi! I stopped this:

```
db1=# \startpipeline
db1=# begin \parse p1
db1=*#
```
Notice the asterisks that appeared after parse the message. This
typically indicates we are in the tx block. this is however untrue
before the bind+exec message for p1 will be sent (\bind_name
metacommand). Am I correct?


-- 
Best regards,
Kirill Reshke

  4. Re: Add Pipelining support in psql

    Jelte Fennema <postgres@jeltef.nl> — 2024-11-27T12:45:32Z

    On Wed, 27 Nov 2024 at 10:50, Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
> With \bind, \parse, \bind_named and \close, it is possible to issue
> queries from psql using the extended protocol. However, it wasn't
> possible to send those queries using pipelining and the only way to
> test pipelined queries was through pgbench's tap tests.

Big +1. Not being able to use psql for even the most basic pipeline
tests has definitely been an annoyance of mine.

I played around quickly with this patch and it works quite well. A few
things that would be nice improvements I think. Feel free to change
the command names:
1. Add a \flush command that calls PQflush
2. Add a \flushrequest command that calls PQsendFlushRequest
3. Add a \getresult command so you can get a result from a query
without having to close the pipeline

To be clear, not having those additional commands isn't a blocker for
this patch imho, but I'd definitely miss them if they weren't there
when I would be using it.

  5. Re: Add Pipelining support in psql

    Jelte Fennema <postgres@jeltef.nl> — 2024-11-27T12:54:45Z

    On Wed, 27 Nov 2024 at 13:05, Kirill Reshke <reshkekirill@gmail.com> wrote:
> ```
> db1=# \startpipeline
> db1=# begin \parse p1
> db1=*#
> ```
> Notice the asterisks that appeared after parse the message. This
> typically indicates we are in the tx block. this is however untrue
> before the bind+exec message for p1 will be sent (\bind_name
> metacommand). Am I correct?

This behaviour is expected, it also happens if you send "SELECT 1"
instead of "begin" in the parse message:
db1=# \startpipeline
db1=# SELECT 1 \parse p1
db1=*#

The reason this happens is that the first command in a pipeline sent
to the server opens an implicit transaction. See the "implicit COMMIT"
wording here[1], or look at this code in exec_parse_message:

    /*
     * Start up a transaction command so we can run parse analysis etc. (Note
     * that this will normally change current memory context.) Nothing happens
     * if we are already in one.  This also arms the statement timeout if
     * necessary.
     */
    start_xact_command();


[1]: https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-FLOW-PIPELINING

  6. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2024-11-28T06:43:04Z

    On Wed, Nov 27, 2024 at 01:45:32PM +0100, Jelte Fennema-Nio wrote:
> I played around quickly with this patch and it works quite well. A few
> things that would be nice improvements I think. Feel free to change
> the command names:
> 1. Add a \flush command that calls PQflush
> 2. Add a \flushrequest command that calls PQsendFlushRequest
> 3. Add a \getresult command so you can get a result from a query
> without having to close the pipeline
> 
> To be clear, not having those additional commands isn't a blocker for
> this patch imho, but I'd definitely miss them if they weren't there
> when I would be using it.

Hmm.  The start, end and sync meta-commands are useful for testing.  I
find the flush control a bit less interesting, TBH.

What would you use these for?  Perhaps these ones would be better if
just applied to pgbench rather than psql where it is possible to pass
down custom SQL sequences with input files?
--
Michael

  7. Re: Add Pipelining support in psql

    Jelte Fennema <postgres@jeltef.nl> — 2024-11-28T09:06:28Z

    On Thu, 28 Nov 2024 at 07:43, Michael Paquier <michael@paquier.xyz> wrote:
> Hmm.  The start, end and sync meta-commands are useful for testing.  I
> find the flush control a bit less interesting, TBH.
>
> What would you use these for?

I guess mostly for interactively playing around with pipelining from psql.

But I think \getresult would be useful for testing too. This would
allow us to test that we can read part of the pipeline, without
sending a sync and waiting for everything.

To be clear \flushrequest and \flush would be necessary to make
\getresult work reliably.

  8. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2024-11-28T09:22:49Z

    On Wed, Nov 27, 2024 at 11:46 AM Kirill Reshke <reshkekirill@gmail.com> wrote:
> I'm very doubtful about the \syncpipeline . Maybe we should instead
> support \sync meta-command in psql? This will be a useful contribution
> itself.

\syncpipeline is useful to cover regression tests involving implicit
transaction blocks within a pipeline where a sync acts as an implicit
COMMIT. What would be the use of sending a \sync outside of a
pipeline? All extended queries meta-commands sent by psql already send
a sync if you're not in a pipeline, so the only use of a \sync
meta-command would be to send a single sync (which could be achieved
with \startpipeline followed by a \endpipeline).

On Wed, Nov 27, 2024 at 1:54 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> This behaviour is expected, it also happens if you send "SELECT 1"
> instead of "begin" in the parse message:
> db1=# \startpipeline
> db1=# SELECT 1 \parse p1
> db1=*#
>
> The reason this happens is that the first command in a pipeline sent
> to the server opens an implicit transaction. See the "implicit COMMIT"
> wording here[1], or look at this code in exec_parse_message:

I don't think that's the case here. Nothing was sent to the server so
there's no active transaction yet. From prompt.c, psql will show a '*'
when PQtransactionStatus is either PQTRANS_ACTIVE or PQTRANS_INTRANS.

Looking at the state of the PGConn, after a \startpipeline, we have:
db->xactStatus: (PGTransactionStatusType) PQTRANS_IDLE
db->asyncStatus: (PGAsyncStatusType) PGASYNC_IDLE
db->pipelineStatus: (PGpipelineStatus) PQ_PIPELINE_ON

After the first command is sent to the pipeline:
db->asyncStatus: (PGAsyncStatusType) PGASYNC_BUSY
db->xactStatus: (PGTransactionStatusType) PQTRANS_IDLE
db->pipelineStatus: (PGpipelineStatus) PQ_PIPELINE_ON

The xactStatus is idle, however, since asyncStatus reports busy, we
fall in the "if (conn->asyncStatus != PGASYNC_IDLE) return
PQTRANS_ACTIVE;" and psql display '*' in the prompt. This might be a
bit misleading since there's no ongoing transaction block on the
server and maybe it's worth having a new state? I've updated the patch
to display a tentative '|' when there's an ongoing pipeline. On the
other hand, a pipeline will start an implicit transaction block even
without BEGIN so leaving the '*' may be a good way to reflect that?

On Wed, Nov 27, 2024 at 1:45 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> I played around quickly with this patch and it works quite well. A few
> things that would be nice improvements I think. Feel free to change
> the command names:
> 1. Add a \flush command that calls PQflush
> 2. Add a \flushrequest command that calls PQsendFlushRequest
> 3. Add a \getresult command so you can get a result from a query
> without having to close the pipeline
>
> To be clear, not having those additional commands isn't a blocker for
> this patch imho, but I'd definitely miss them if they weren't there
> when I would be using it.

I will need a bit of time to check those and how they can be included
in the regression tests.

  9. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2024-12-04T12:52:05Z

    An updated version of the patch, still a bit rough around the edges.

I've added \flushrequest, \flush and \getrequests meta-commands.
\flushrequest and \getresults look interesting as they add an
additional protocol message to test, but it also introduces the
possibility to be in the middle of an aborted pipeline which adds some
complexity to the patch. I'm a bit skeptical of \flush as \getresults
already automatically flush through PQgetResult and I haven't thought
of interesting tests that could rely on \flush.

I've added a new prompt interpolation instead of trying to hijack the
existing %x: %P reports the pipeline status with either
'|num_syncs,num_queries,pending_results|' for an ongoing pipeline or
'+num_syncs,num_queries,pending_results+' for an aborted pipeline.
Those are definitely open to debate for those but I found those
informations useful while debugging.

With the new \flushrequest and \getresults, it's visible that %x
doesn't reflect the transaction status when pipelines are involved. If
I do:

postgres=# \startpipeline
postgres=|0,0,0|# BEGIN \bind \g
postgres=|0,1,0|*# \flushrequest
postgres=|0,1,1|*# \getresults
BEGIN
postgres=|0,0,0|#

There's an ongoing transaction block running on the server but %x will
be empty as there's no queued commands and the server results are
consumed. I'm tempted to change %x to not display anything when within
a pipeline.

Calling PQgetResult while a command is piped and not followed by a
flushrequest or a sync would block since the asyncstatus is busy. So
\getresults has to keep track of the number of pending results to
avoid calling PQgetResult if there's no pending results. This is to
cover cases like:

postgres=# \startpipeline
postgres=|0,0,0|# select 1 \bind \g
postgres=|0,1,0|*# \getresults
No pending results to get
postgres=|0,1,0|*#

There are some interesting behaviors with pipeline + copy. Running the
following:

\startpipeline
SELECT $1 \bind 'val1' \g
COPY psql_pipeline FROM STDIN \bind \g
\flushrequest
\getresults
3   test3
\.
\endpipeline

Will complain with a "message type 0x5a arrived from server while
idle" (but still work). It's also possible to trigger a protocol error
with a query error after a piped copy:

truncate pgbench_accounts;
\startpipeline
copy pgbench_accounts FROM stdin \bind \g
SELECT 1 \bind \g
\endpipeline
1 2 3 asd
\.

Copy will fail with "ERROR:  unexpected message type 0x50 during COPY
from stdin" and connection will be terminated.

  10. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2024-12-10T10:43:50Z

    An improved version with simplifications and refinements.

num_queries (2nd element in the pipeline status prompt) is now used to
track queued queries that were not flushed (with a flush request or
sync) to the server. It used to count both unflushed queries and
flushed queries.

Code in ExecQueryAndProcessResults should be simpler now.
- DiscardAbortedPipelineResults function handles both discarding of
results until a synchronisation point is reached or discarding of
results until there's no more pending results.
- The logic to process the pipeline's results and getting the next
results fit more with the existing flow.
- Tests didn't cover chunk results so I've added additional tests to
cover use of pipelining + FETCH_COUNT

  11. Re: Add Pipelining support in psql

    Jelte Fennema <postgres@jeltef.nl> — 2024-12-11T23:53:27Z

    On Tue, 10 Dec 2024 at 11:44, Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
> num_queries (2nd element in the pipeline status prompt) is now used to
> track queued queries that were not flushed (with a flush request or
> sync) to the server. It used to count both unflushed queries and
> flushed queries.

I skimmed the code a bit, but haven't looked closely at it yet. I did
try the patch out though. My thoughts below:

I think that new prompt is super useful, so useful in fact that I'd
suggest linking to it from the \startpipeline docs. I do think that
the wording in the docs could be a bit more precise:
1. The columns are not necessarily queries, they are messages or
commands. i.e. \parse and \bind_named both count as 1, even though
they form one query together.
2. messages not followed by \sync and \flushrequest, could very well
already "be sent to the server" (if the client buffer got full, or in
case of manual \flush). The main thing that \sync and \flushrequest do
is make sure that the server actually sends its own result back, even
if its buffer is not full yet.

The main feedback I have after playing around with this version is
that I'd like to have a \getresult (without the s), to only get a
single result. So that you can get results one by one, possibly
interleaved with some other queries again.

One thing I'm wondering is how useful the num_syncs count is in the
pipeline prompt, since you never really wait for a sync.

Regarding the usefulness of \flush. I agree it's not as useful as I
thought, because indeed \getresults already flushes everything. But
it's not completely useless either. The main way I was able to use it
interactively in a somewhat interesting way was to send it after a
long running query, and then while that's processing type up the next
query after it. Something like the following:

localhost jelte@postgres:5432-26274=
#> \startpipeline
Time: 0.000 ms
localhost jelte@postgres:5432-26274=
#|0,0,0|> select pg_sleep(5) \bind \g
Time: 0.000 ms
localhost jelte@postgres:5432-26274=
#|0,1,0|*> \flush
Time: 0.000 ms
localhost jelte@postgres:5432-26274=
#|0,1,0|*> select 1 \bind \g
Time: 0.000 ms
localhost jelte@postgres:5432-26274=
#|0,2,0|*> \syncpipeline
Time: 0.000 ms
localhost jelte@postgres:5432-26274=
#|1,0,2|*> \getresults
 pg_sleep
──────────

(1 row)

 ?column?
──────────
        1
(1 row)

Time: 0.348 ms

  12. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2024-12-12T10:38:48Z

    Thanks for the review!

On Thu, Dec 12, 2024 at 12:53 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> I think that new prompt is super useful, so useful in fact that I'd
> suggest linking to it from the \startpipeline docs.

Good point, I've added a paragraph with the link to the %P prompt.

> I do think that
> the wording in the docs could be a bit more precise:
> 1. The columns are not necessarily queries, they are messages or
> commands. i.e. \parse and \bind_named both count as 1, even though
> they form one query together.

Yeah, I wasn't super happy with the num_queries and wording. I've
renamed the prompt columns to piped_syncs, piped_commands and
pending_results and added more precision on what counts as a command.

> 2. messages not followed by \sync and \flushrequest, could very well
> already "be sent to the server" (if the client buffer got full, or in
> case of manual \flush). The main thing that \sync and \flushrequest do
> is make sure that the server actually sends its own result back, even
> if its buffer is not full yet.

I had the wrong assumptions on what was happening. I've changed the
definition of pending_results to: "pending_results is the number of
commands that were followed by either a \flushrequest or a
\syncpipeline, forcing the server to send the results that can be
retrieved with \getresults."

> The main feedback I have after playing around with this version is
> that I'd like to have a \getresult (without the s), to only get a
> single result. So that you can get results one by one, possibly
> interleaved with some other queries again.

\getresults was easier to implement since it was more or less the
current behaviour :). I didn't want to add a new meta-command just for
that (I feel like I'm already adding a lot of them) so I've added a
num_results parameter to \getresults. You should be able to use
\getresults 1 to get a single result. A sync response count as a
result.

> One thing I'm wondering is how useful the num_syncs count is in the
> pipeline prompt, since you never really wait for a sync.

With the addition of the num_results parameter for \getresults,
knowing the number of syncs becomes more useful when results are
consumed one by one.

> Regarding the usefulness of \flush. I agree it's not as useful as I
> thought, because indeed \getresults already flushes everything. But
> it's not completely useless either. The main way I was able to use it
> interactively in a somewhat interesting way was to send it after a
> long running query, and then while that's processing type up the next
> query after it.

I feel there's a large overlap between \flush and \flushrequest. On
the other hand, if people want to test the behaviour of pushing data
with and without a flush request message, then \flush can be useful.

  13. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-01-10T10:59:44Z

    > I feel there's a large overlap between \flush and \flushrequest. On
> the other hand, if people want to test the behaviour of pushing data
> with and without a flush request message, then \flush can be useful.

I've been looking into some issues related to a backend process stuck
in ClientWrite state blocking the WAL replay and it turns out that
pipelining + \flush was extremely useful to reproduce the issue. It
makes it possible to saturate the server->client socket buffer since
psql doesn't consume the results, easily triggering the state where
the process is stuck in ClientWrite. So I'm amending my position on
the usefulness of \flush and definitely see interesting usages.

I've also found out that I didn't correctly manage connection reset.
I've fixed this in v6 by resetting the pipeline counters on connection
reset and only calling discardAbortedPipelineResults if we're inside a
pipeline.

  14. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-01-14T08:49:02Z

    During my tests, I've noticed I didn't handle query cancellation, this
is now fixed. I've also added additional comments related to
available_results to make it clearer that it depends on what the
server has flushed to the client.

  15. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-02-18T07:22:53Z

    On Tue, Jan 14, 2025 at 09:49:02AM +0100, Anthonin Bonnefoy wrote:
> During my tests, I've noticed I didn't handle query cancellation, this
> is now fixed. I've also added additional comments related to
> available_results to make it clearer that it depends on what the
> server has flushed to the client.

This is a pretty cool patch.  I like the structure you have used for
the integration with the tracking of the number of commands, the
number of syncs (like pgbench) put in a pipeline, the number of
results requested and the number of results available.  That makes the
whole easier to look at with a state in pset.

+	PSQL_SEND_PIPELINE_SYNC,
+	PSQL_START_PIPELINE_MODE,
+	PSQL_END_PIPELINE_MODE,
+	PSQL_FLUSH,
+	PSQL_SEND_FLUSH_REQUEST,
+	PSQL_GET_RESULTS,

These new values are inconsistent, let's use some more PSQL_SEND_*
here.  That makes the whole set of values more greppable.

The tests in psql.sql are becoming really long.  Perhaps it would be
better to split that into its own file, say psql_pipeline.sql?  The
input file is already 2k lines, you are adding 15% more lines to that.

+ * otherwise, calling PQgetResults will block. 

Likely PQgetResults => PQgetResult().

Wondering if the cancellation in ExecQueryAndProcessResults() is
sound, I've not been able to break it, still..

I can also get behind the additions of \flush and \flushrequest to
query different parts of libpq.

+	if (pset.requested_results == 0)
+		/* We've read all requested results, exit */
+		return res;

Set of nits with the style of the code, but I'd suggest to use
braces {} here, to outline that the comment is in a block.  There's a
second, larger one in discardAbortedPipelineResults().

+	if (strcmp(cmd, "gx") == 0 && PQpipelineStatus(pset.db) != PQ_PIPELINE_OFF)
+	{
+		pg_log_error("\\gx not allowed in pipeline mode");
+		clean_extended_state();
+		return PSQL_CMD_ERROR;
+	}

What is the reasoning here behind this restriction?  \gx is a wrapper
of \g with expanded mode on, but it is also possible to call \g with
expanded=on, bypassing this restriction.

Let's split the prompt patch with the support of %P into its own
patch.

-#define DEFAULT_PROMPT1 "%/%R%x%# "
-#define DEFAULT_PROMPT2 "%/%R%x%# "
+#define DEFAULT_PROMPT1 "%/%R%P%x%# "
+#define DEFAULT_PROMPT2 "%/%R%P%x%# "
 #define DEFAULT_PROMPT3 ">> "

I don't think that changing this default is a good idea.  Everybody
can do that in their own .psqlrc (spoiler: I do).

The idea to use three fields with a hardcoded format does not look
like a good idea to me.  I think that this should be done in a
different and more extensible way:
- Use %P to track if we are in pipeline mode on, off or abort.
- Define three new variables that behave like ROW_COUNT, but for the
fields you want to track here.  These could then be passed down to a
PROMPT with %:name:, grammar already supported.

That would make the whole much more flexible.  At it seems to me that
we could also add requested_results to this set?  These could be named
with the same prefix, like PIPELINE_SYNC_COUNT, etc.
--
Michael

  16. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-02-18T17:34:20Z

    On Tue, Feb 18, 2025 at 8:23 AM Michael Paquier <michael@paquier.xyz> wrote:
> This is a pretty cool patch.  I like the structure you have used for
> the integration with the tracking of the number of commands, the
> number of syncs (like pgbench) put in a pipeline, the number of
> results requested and the number of results available.  That makes the
> whole easier to look at with a state in pset.

Thanks!

> +       PSQL_SEND_PIPELINE_SYNC,
> +       PSQL_START_PIPELINE_MODE,
> +       PSQL_END_PIPELINE_MODE,
> +       PSQL_FLUSH,
> +       PSQL_SEND_FLUSH_REQUEST,
> +       PSQL_GET_RESULTS,
>
> These new values are inconsistent, let's use some more PSQL_SEND_*
> here.  That makes the whole set of values more greppable.

Changed.

> The tests in psql.sql are becoming really long.  Perhaps it would be
> better to split that into its own file, say psql_pipeline.sql?  The
> input file is already 2k lines, you are adding 15% more lines to that.

Agreed, I wasn't sure if this was enough to warrant a dedicated test
file. This is now separated in psql_pipeline.sql.

> + * otherwise, calling PQgetResults will block.
>
> Likely PQgetResults => PQgetResult().

Indeed, this is fixed.

> Set of nits with the style of the code, but I'd suggest to use
> braces {} here, to outline that the comment is in a block.  There's a
> second, larger one in discardAbortedPipelineResults().
>
> +       if (strcmp(cmd, "gx") == 0 && PQpipelineStatus(pset.db) != PQ_PIPELINE_OFF)
> +       {
> +               pg_log_error("\\gx not allowed in pipeline mode");
> +               clean_extended_state();
> +               return PSQL_CMD_ERROR;
> +       }

Changed.

> What is the reasoning here behind this restriction?  \gx is a wrapper
> of \g with expanded mode on, but it is also possible to call \g with
> expanded=on, bypassing this restriction.

The issue is that \gx enables expanded mode for the duration of the
query and immediately reset it in sendquery_cleanup. With pipelining,
the command is piped and displaying is done by either \endpipeline or
\getresults, so the flag change has no impact. Forbidding it was a way
to make it clearer that it won't have the expected effect. If we
wanted a similar feature, this would need to be done with something
like \endpipelinex or \getresultsx.

> Let's split the prompt patch with the support of %P into its own
> patch.
>
> -#define DEFAULT_PROMPT1 "%/%R%x%# "
> -#define DEFAULT_PROMPT2 "%/%R%x%# "
> +#define DEFAULT_PROMPT1 "%/%R%P%x%# "
> +#define DEFAULT_PROMPT2 "%/%R%P%x%# "
>  #define DEFAULT_PROMPT3 ">> "
>
> I don't think that changing this default is a good idea.  Everybody
> can do that in their own .psqlrc (spoiler: I do).
>
> The idea to use three fields with a hardcoded format does not look
> like a good idea to me.  I think that this should be done in a
> different and more extensible way:
> - Use %P to track if we are in pipeline mode on, off or abort.
> - Define three new variables that behave like ROW_COUNT, but for the
> fields you want to track here.  These could then be passed down to a
> PROMPT with %:name:, grammar already supported.
>
> That would make the whole much more flexible.  At it seems to me that
> we could also add requested_results to this set?  These could be named
> with the same prefix, like PIPELINE_SYNC_COUNT, etc.

I've split the patch and created the 3 special variables:
PIPELINE_SYNC_COUNT, PIPELINE_COMMAND_COUNT, PIPELINE_RESULT_COUNT.

For requested_results, I don't think there's value in exposing it
since it is used as an exit condition and thus will always be 0
outside of ExecQueryAndProcessResults.

  17. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-02-20T08:02:42Z

    On Tue, Feb 18, 2025 at 06:34:20PM +0100, Anthonin Bonnefoy wrote:
> On Tue, Feb 18, 2025 at 8:23 AM Michael Paquier <michael@paquier.xyz> wrote:
>> The tests in psql.sql are becoming really long.  Perhaps it would be
>> better to split that into its own file, say psql_pipeline.sql?  The
>> input file is already 2k lines, you are adding 15% more lines to that.
> 
> Agreed, I wasn't sure if this was enough to warrant a dedicated test
> file. This is now separated in psql_pipeline.sql.

You have forgotten the expected output.  Not a big issue as the input
was sent.

>> What is the reasoning here behind this restriction?  \gx is a wrapper
>> of \g with expanded mode on, but it is also possible to call \g with
>> expanded=on, bypassing this restriction.
> 
> The issue is that \gx enables expanded mode for the duration of the
> query and immediately reset it in sendquery_cleanup. With pipelining,
> the command is piped and displaying is done by either \endpipeline or
> \getresults, so the flag change has no impact. Forbidding it was a way
> to make it clearer that it won't have the expected effect. If we
> wanted a similar feature, this would need to be done with something
> like \endpipelinex or \getresultsx.

Hmm, okay.  If one wants one mode or the other it is always possible
to force one with \pset expanded when getting the results.  Not sure
if there is any need for new specific commands for these two printing
the results.  Another option would be to authorize the command to run,
but perhaps your option is just better as per the enforced behavior in
the output.  So fine by me.  There is coverage so we'll know if there
are arguments in favor of authorizing the command, if need be.

> I've split the patch and created the 3 special variables:
> PIPELINE_SYNC_COUNT, PIPELINE_COMMAND_COUNT, PIPELINE_RESULT_COUNT.

Thanks.  Looks sensible now.

> For requested_results, I don't think there's value in exposing it
> since it is used as an exit condition and thus will always be 0
> outside of ExecQueryAndProcessResults.

I've been playing with this patch and this configuration:
\set PROMPT1 '=(pipeline=%P,sync=%:PIPELINE_SYNC_COUNT:,cmd=%:PIPELINE_COMMAND_COUNT:,res=%:PIPELINE_RESULT_COUNT:)%#' 

That's long, but seeing the evolution of the pipeline status is pretty
cool depending on the meta-commands used.

While testing, I have been able to run into an assertion failure by
adding some tests in psql.sql to check for the case of inactive
branches for \if.  For example:
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1047,11 +1047,15 @@ select \if false \\ (bogus \else \\ 42 \endif \\ forty_two;
    \echo arg1 arg2 arg3 arg4 arg5
    \echo arg1
    \encoding arg1
+   \endpipeline
    \errverbose

And the report:
+psql: mainloop.c:513: MainLoop: Assertion `conditional_active(cond_stack)' failed.

We should have tests for all new six meta-commands in psql.sql.
MainLoop() is wrong when in pipeline mode for inactive branches.
--
Michael

  18. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-02-20T09:29:33Z

    On Thu, Feb 20, 2025 at 9:02 AM Michael Paquier <michael@paquier.xyz> wrote:
> You have forgotten the expected output.  Not a big issue as the input
> was sent.

I was writing the mail with the missing file when you sent this mail.
This is fixed.

> While testing, I have been able to run into an assertion failure by
> adding some tests in psql.sql to check for the case of inactive
> branches for \if.  For example:
> --- a/src/test/regress/sql/psql.sql
> +++ b/src/test/regress/sql/psql.sql
> @@ -1047,11 +1047,15 @@ select \if false \\ (bogus \else \\ 42 \endif \\ forty_two;
>     \echo arg1 arg2 arg3 arg4 arg5
>     \echo arg1
>     \encoding arg1
> +   \endpipeline
>     \errverbose
>
> And the report:
> +psql: mainloop.c:513: MainLoop: Assertion `conditional_active(cond_stack)' failed.
>
> We should have tests for all new six meta-commands in psql.sql.
> MainLoop() is wrong when in pipeline mode for inactive branches.

Ha yeah, I forgot about the inactive branches. I've added the new
commands and fixed the behaviour.

A small issue I've noticed while testing: When a pipeline has at least
one queue command, pqClearConnErrorState isn't called in
PQsendQueryStart and errors are appended. For example:

\startpipeline
select 1 \bind \g
select 1;
PQsendQuery not allowed in pipeline mode
select 1;
PQsendQuery not allowed in pipeline mode
PQsendQuery not allowed in pipeline mode

This looks more like an issue on libpq's side as there's no way to
reset or advance the errorReported from ExecQueryAndProcessResults
(plus PQerrorMessage seems to ignore errorReported). I've added an
additional test to track this behaviour for now as this would probably
be better discussed in a dedicated thread.

  19. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-02-21T02:33:41Z

    On Thu, Feb 20, 2025 at 10:29:33AM +0100, Anthonin Bonnefoy wrote:
> Ha yeah, I forgot about the inactive branches. I've added the new
> commands and fixed the behaviour.

And I did not notice that it was as simple as forcing the status in
the routines for the new meta-commands, as we do for the existing
ones.  Noted.

> This looks more like an issue on libpq's side as there's no way to
> reset or advance the errorReported from ExecQueryAndProcessResults
> (plus PQerrorMessage seems to ignore errorReported). I've added an
> additional test to track this behaviour for now as this would probably
> be better discussed in a dedicated thread.

I am not sure if we should change that, actually, as it does not feel
completely wrong to stack these errors.  That's a bit confusing,
sure.  Perhaps a new libpq API to retrieve stacked errors when we are
in pipeline mode would be more adapted?  The design would be
different.

Anyway, I've stared at the result processing code for a couple of
hours, and the branches we're taking for the pipeline modes seem to be
rather right the way you have implemented them.  The docs, comments
and tests needed quite a few tweaks and edits to be more consistent.
There were some grammar mistakes, some frenchisms.

I'm hoping that there won't be any issues, but let's be honest, I am
definitely sure there will be some more tuning required.  It comes
down to if we want this set of features, and I do to be able to expand
tests in core with the extended query protocol and pipelines, knowing
that there is an ask for out-of-core projects.  This one being
reachable with a COPY gave me a huge smile:
+message type 0x5a arrived from server while idle

So let's take one step here, I have applied the main patch.  I am
really excited by the possibilities all this stuff offers.

Attached are the remaining pieces, split here because they are
different bullet points:
- Tests for implicit transactions with various commands, with some
edits. 
- Prompt support, with more edits.

I'm putting these on standby for a few days, to let the buildfarm
digest the main change.
--
Michael

  20. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-02-25T01:11:13Z

    On Fri, Feb 21, 2025 at 11:33:41AM +0900, Michael Paquier wrote:
> Attached are the remaining pieces, split here because they are
> different bullet points:
> - Tests for implicit transactions with various commands, with some
> edits. 
> - Prompt support, with more edits.
> 
> I'm putting these on standby for a few days, to let the buildfarm
> digest the main change.

Initial digestion has gone well.  The remaining pieces have been done
as 3ce357584e79 and a4e986ef5a46.  For the prompt part, I have added a
couple of tests with \echo and the variables.  The patch felt
incomplete without these.  Perhaps we could extend them more, at least
we have a start point.
--
Michael

  21. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-02-25T07:30:16Z

    On Tue, Feb 25, 2025 at 2:11 AM Michael Paquier <michael@paquier.xyz> wrote:
> For the prompt part, I have added a
> couple of tests with \echo and the variables.  The patch felt
> incomplete without these.  Perhaps we could extend them more, at least
> we have a start point.

Good catch. I guess that's also another benefit of having special
variables, as it makes it easier to check the pipeline state.

  22. Re: Add Pipelining support in psql

    Daniel Verite <daniel@manitou-mail.org> — 2025-02-28T16:31:00Z

    	Anthonin Bonnefoy wrote:

> > What is the reasoning here behind this restriction?  \gx is a wrapper
> > of \g with expanded mode on, but it is also possible to call \g with
> > expanded=on, bypassing this restriction.
> 
> The issue is that \gx enables expanded mode for the duration of the
> query and immediately reset it in sendquery_cleanup. With pipelining,
> the command is piped and displaying is done by either \endpipeline or
> \getresults, so the flag change has no impact. Forbidding it was a way
> to make it clearer that it won't have the expected effect

But it's not just \gx

The following invocations don't respect the desired output destination
and formats (ignoring them), when a pipeline is active:

select ... \bind \g filename
select ... \bind \g |program
select ... \bind \g (format=unaligned tuples_only=on)

Just like for \gx the problem is that in a pipeline, sending the query
is not followed by getting the results, and the output properties
of a query are lost in between.


Best regards,
-- 
Daniel Vérité 
https://postgresql.verite.pro/

  23. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-03-04T00:56:55Z

    (My previous message did not reach the lists, so re-sending with some
edits.)

On Fri, Feb 28, 2025 at 05:31:00PM +0100, Daniel Verite wrote:
> The following invocations don't respect the desired output destination
> and formats (ignoring them), when a pipeline is active:
> 
> select ... \bind \g filename
> select ... \bind \g |program
> select ... \bind \g (format=unaligned tuples_only=on)
> 
> Just like for \gx the problem is that in a pipeline, sending the query
> is not followed by getting the results, and the output properties
> of a query are lost in between.

Right.  I completely forgot that these options could be applied with a
simple \g.  With the results being decoupled from the execution, one
can argue that the options defined at the moment when the query is
sent and executed should be the moment commanding how the result are
shaped when retrieving a batch with \getresult during a pipeline.
However, this means that we would need to save the set of options from
printQueryOpt when running the query depending on the number of
results we expect, and reapply them in order of the results
expected. We have the APIs to do that, with savePsetInfo() and
restorePsetInfo().

Anyway, can we really say that the set of printQueryOpt saved at
execution is the correct set to use?  It is possible to have the
opposite argument and say that we should just apply the printQueryOpt
at the moment where \getresult is run.  A benefit of that it to keep
the loop retrieving results simpler in ExecQueryAndProcessResults(),
because we pass down to this call *one* printQueryOpt as "opt". 

There's of course the simplicity argument that I do like a lot here,
but applying the printing options at the time of \getresult feels also
more natural to me.

FWIW, I agree that HEAD is unbalanced with its handling of \gx, so we
could do one of the following two things:
1) Ignore any formatting options given to \g, but also allow \gx to
run, documenting that during a pipeline the formatting options are
ignored, and that the set of options defined when doing a \getresult
is the only thing that matters.
2) Invent a new meta-command (suggested by Daniel Verite off-list, and
not published to the lists because I don't know how to do a
reply-all), like a \send, a \push, forbiding entirely \gx and \g when
in a pipeline.  If we were to integrate options into this new
meta-command, a split with \g would make an integration easier to
think about.  One name suggestion I can come up is \sendpipeline.

I'd consider option 2, based on Daniel's concerns.  One line I'm going
to draw here is that we should not go down to manipulations of
printQueryOpt while retrieving batch of results depending on the style
of output that was defined when sending a query in a pipeline.

Anthonin, as the primary author, any thoughts?
--
Michael

  24. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-03-04T09:31:46Z

    On Tue, Mar 4, 2025 at 1:57 AM Michael Paquier <michael@paquier.xyz> wrote:
> Anyway, can we really say that the set of printQueryOpt saved at
> execution is the correct set to use?  It is possible to have the
> opposite argument and say that we should just apply the printQueryOpt
> at the moment where \getresult is run.  A benefit of that it to keep
> the loop retrieving results simpler in ExecQueryAndProcessResults(),
> because we pass down to this call *one* printQueryOpt as "opt".
>
> There's of course the simplicity argument that I do like a lot here,
> but applying the printing options at the time of \getresult feels also
> more natural to me.

Saving the printQueryOpt when a command is pushed was an option I had
in mind if that was straightforward to implement. However, even with
savePsetInfo, you will need to save values like gfname and gset_prefix
since it impacts the output (it may make sense to move those in
printQueryOpt). This would also need to be saved for all commands,
like \close or \parse since we don't distinguish if a piped command
generates an output or not. So that definitely looks like it would add
a lot of complexity for limited benefit.

> FWIW, I agree that HEAD is unbalanced with its handling of \gx, so we
> could do one of the following two things:
> 1) Ignore any formatting options given to \g, but also allow \gx to
> run, documenting that during a pipeline the formatting options are
> ignored, and that the set of options defined when doing a \getresult
> is the only thing that matters.
> 2) Invent a new meta-command (suggested by Daniel Verite off-list, and
> not published to the lists because I don't know how to do a
> reply-all), like a \send, a \push, forbiding entirely \gx and \g when
> in a pipeline.  If we were to integrate options into this new
> meta-command, a split with \g would make an integration easier to
> think about.  One name suggestion I can come up is \sendpipeline.
>
> I'd consider option 2, based on Daniel's concerns.  One line I'm going
> to draw here is that we should not go down to manipulations of
> printQueryOpt while retrieving batch of results depending on the style
> of output that was defined when sending a query in a pipeline.

Another possible option would be to directly send the command without
requiring an additional meta-command, like "SELECT 1 \bind". However,
this would make it more painful to introduce new parameters, plus it
makes the \bind and \bind_named inconsistent as it is normally
required to send the result with a separate meta-command.

I like the \sendpipeline option. It makes it clearer that formatting
options are not applicable within a pipeline (at least, in the current
implementation) and I think it would make more sense to have those
formatting options in \getresults or \endpipeline.

I took a stab at creating the \sendpipeline meta-command. I've also
realised there's a small leak where fname is currently not freed on
queries like 'select ... \bind \gx filename' when within a pipeline,
which is fixed in patch 0001.

  25. Re: Add Pipelining support in psql

    Daniel Verite <daniel@manitou-mail.org> — 2025-03-04T12:32:38Z

    	Anthonin Bonnefoy wrote:

> Another possible option would be to directly send the command without
> requiring an additional meta-command, like "SELECT 1 \bind". However,
> this would make it more painful to introduce new parameters, plus it
> makes the \bind and \bind_named inconsistent as it is normally
> required to send the result with a separate meta-command.

AFAIU the reason why \bind is required (even when there are no $N
parameters in the query) is to trigger the use of the extended query
protocol. This pre-dates the pipeline feature.

But when in a pipeline, we can't send queries with the simple query
protocol anyway, so a possible usability improvement would be to make
\bind optional in that case.

Concretely it's not possible currently to issue:
  \startpipeline
  select 1;
it causes the error:  "PQsendQuery not allowed in pipeline mode"

whereas this sequence does works:
  \startpipeline
  \bind
  select 1;
  \flushrequest
  \getresults

But if the code triggered the use of the extended query protocol
if \bind is in effect *or* a pipeline is active, then the first sequence
would  just push "select 1" into the pipeline.

This would have the advantage that, to submit into a pipeline
a pre-existing file with SQL commands separated with ";"  you don't have
to pre-process it to inject metacommands. Adding a \startpipeline at
the beginning and an \endpipeline at the end would be sufficient in the
cases that the user does not need the results before the end.

The \sendpipeline is not mandatory when ";" can be used to terminate
the queries. But it makes it clearer that the script wants
specifically to push into a pipeline, and it might accept specific
options in the future, whereas obviously ";" cannot.


Best regards,
-- 
Daniel Vérité 
https://postgresql.verite.pro/

  26. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-03-04T17:37:09Z

    On Tue, Mar 4, 2025 at 1:32 PM Daniel Verite <daniel@manitou-mail.org> wrote:
> But if the code triggered the use of the extended query protocol
> if \bind is in effect *or* a pipeline is active, then the first sequence
> would  just push "select 1" into the pipeline.
>
> This would have the advantage that, to submit into a pipeline
> a pre-existing file with SQL commands separated with ";"  you don't have
> to pre-process it to inject metacommands. Adding a \startpipeline at
> the beginning and an \endpipeline at the end would be sufficient in the
> cases that the user does not need the results before the end.
>
> The \sendpipeline is not mandatory when ";" can be used to terminate
> the queries. But it makes it clearer that the script wants
> specifically to push into a pipeline, and it might accept specific
> options in the future, whereas obviously ";" cannot.

So if I understand correctly, you want to automatically convert a
simple query into an extended query when we're within a pipeline. That
would be doable with:

--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1668,7 +1668,16 @@ ExecQueryAndProcessResults(const char *query,
                        }
                        break;
                case PSQL_SEND_QUERY:
-                       success = PQsendQuery(pset.db, query);
+                       if (PQpipelineStatus(pset.db) != PQ_PIPELINE_OFF) {
+                               success = PQsendQueryParams(pset.db, query,
+
pset.bind_nparams, NULL,
+                                                               (const
char *const *) pset.bind_params,
+                                                               NULL, NULL, 0);
+                               if (success)
+                                       pset.piped_commands++;
+                       }
+                       else
+                               success = PQsendQuery(pset.db, query);
                        break;
        }

I do see the idea to make it easier to convert existing scripts into
using pipelining. The main focus of the initial implementation was
more on protocol regression tests with psql, so that's not necessarily
something I had in mind. I have some reservation as it will push all
parameters in the query string which may not be the desired behaviour.
But on the other hand, if it is to convert existing psql scripts, then
everything was already pushed as simple queries. Plus, this is similar
to what pgbench is doing when using -Mextended or -Mprepared.

  27. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-03-04T23:11:36Z

    On Tue, Mar 04, 2025 at 10:31:46AM +0100, Anthonin Bonnefoy wrote:
> Saving the printQueryOpt when a command is pushed was an option I had
> in mind if that was straightforward to implement. However, even with
> savePsetInfo, you will need to save values like gfname and gset_prefix
> since it impacts the output (it may make sense to move those in
> printQueryOpt). This would also need to be saved for all commands,
> like \close or \parse since we don't distinguish if a piped command
> generates an output or not. So that definitely looks like it would add
> a lot of complexity for limited benefit.

Yeah, same opinion here.  I don't want this level of complexity with
extra manipulations of printQueryOpt when fetching the results,
either.  I'm all for making these meta-commands to what we think is
more natural, but not at the cost of a more complex logic in the
result printing depending on what's been given by a meta-command when
a query is pushed to a pipeline.

> I took a stab at creating the \sendpipeline meta-command. I've also
> realised there's a small leak where fname is currently not freed on
> queries like 'select ... \bind \gx filename' when within a pipeline,
> which is fixed in patch 0001.

Indeed.  Fixed this one for now.
--
Michael

  28. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-03-04T23:25:34Z

    On Tue, Mar 04, 2025 at 06:37:09PM +0100, Anthonin Bonnefoy wrote:
> I do see the idea to make it easier to convert existing scripts into
> using pipelining. The main focus of the initial implementation was
> more on protocol regression tests with psql, so that's not necessarily
> something I had in mind. I have some reservation as it will push all
> parameters in the query string which may not be the desired behaviour.
> But on the other hand, if it is to convert existing psql scripts, then
> everything was already pushed as simple queries. Plus, this is similar
> to what pgbench is doing when using -Mextended or -Mprepared.

Hmm.  Simplicity is tempting here because we know the status of the
pipeline when sending the query.  If we do something like what you are
suggesting here, do we actually need the \sendpipeline at all?  We
should still prevent \g, \gx and others from running in a pipeline
because of the format argument raised by Daniel so as we restrict the
use of meta-commands that can manipulate the output format, right?
--
Michael

  29. Re: Add Pipelining support in psql

    Daniel Verite <daniel@manitou-mail.org> — 2025-03-05T14:25:12Z

    	Anthonin Bonnefoy wrote:

> So if I understand correctly, you want to automatically convert a
> simple query into an extended query when we're within a pipeline. That
> would be doable with:
> 
> --- a/src/bin/psql/common.c
> +++ b/src/bin/psql/common.c
> @@ -1668,7 +1668,16 @@ ExecQueryAndProcessResults(const char *query,
>                        }
>                        break;
>                case PSQL_SEND_QUERY:
> -                       success = PQsendQuery(pset.db, query);
> +                       if (PQpipelineStatus(pset.db) != PQ_PIPELINE_OFF) {
> +                               success = PQsendQueryParams(pset.db, query,
> +
> pset.bind_nparams, NULL,
> +                                                               (const
> char *const *) pset.bind_params,
> +                                                               NULL, NULL,
> 0);
> +                               if (success)
> +                                       pset.piped_commands++;
> +                       }
> +                       else
> +                               success = PQsendQuery(pset.db, query);
>                        break;
>        }

Yes, except that the bind parameters need to be cleared after this,
as done in clean_extended_state()

> I do see the idea to make it easier to convert existing scripts into
> using pipelining. The main focus of the initial implementation was
> more on protocol regression tests with psql, so that's not necessarily
> something I had in mind.

Understood. Yet pipelining can accelerate considerably certain scripts
when client-server latency is an issue. We should expect end users to
benefit from it too.

> I have some reservation as it will push all
> parameters in the query string which may not be the desired
> behaviour.

I don't follow. For me the change discussed here is about simplifying
the syntax when there is no out-of-query $N-style parameters, it does
not change anything for queries that actually use them, nor does
it forbid a \bind without parameters.


Best regards,
-- 
Daniel Vérité 
https://postgresql.verite.pro/

  30. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-03-06T04:20:37Z

    On Wed, Mar 05, 2025 at 03:25:12PM +0100, Daniel Verite wrote:
> 	Anthonin Bonnefoy wrote:
>> I do see the idea to make it easier to convert existing scripts into
>> using pipelining. The main focus of the initial implementation was
>> more on protocol regression tests with psql, so that's not necessarily
>> something I had in mind.
> 
> Understood. Yet pipelining can accelerate considerably certain scripts
> when client-server latency is an issue. We should expect end users to
> benefit from it too.

That was not a test case we had in mind originally here, but if it is
possible to keep the implementation simple while supporting your
demand, well, let's do it.  If it's not that straight-forward, let's
use the new meta-command, forbidding \g and \gx based on your
arguments from upthread.
--
Michael

  31. Re: Add Pipelining support in psql

    Jelte Fennema <postgres@jeltef.nl> — 2025-03-07T00:05:13Z

    On Tue, 25 Feb 2025 at 02:11, Michael Paquier <michael@paquier.xyz> wrote:
> Initial digestion has gone well.

One thing I've noticed is that \startpipeline throws warnings when
copy pasting multiple lines. It seems to still execute everything as
expected though. As an example you can copy paste this tiny script:

\startpipeline
select pg_sleep(5) \bind \g
\endpipeline

And then it will show these "extra argument ... ignored" warnings

\startpipeline: extra argument "select" ignored
\startpipeline: extra argument "pg_sleep(5)" ignored

  32. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-03-07T08:08:51Z

    On Thu, Mar 6, 2025 at 5:20 AM Michael Paquier <michael@paquier.xyz> wrote:
> That was not a test case we had in mind originally here, but if it is
> possible to keep the implementation simple while supporting your
> demand, well, let's do it.  If it's not that straight-forward, let's
> use the new meta-command, forbidding \g and \gx based on your
> arguments from upthread.

I think the new meta-command is a separate issue from allowing ';' to
push in a pipeline. Any time there's a change or an additional format
option added to \g, it will need to be forbidden for pipelining. The
\sendpipeline meta-command will help keep those exceptions low since
the whole \g will be forbidden.

Another possible option would be to allow both \g and \gx, but send a
warning like "printing options within a pipeline will be ignored" if
those options are used, similar to "SET LOCAL" warning when done
outside of a transaction block. That would have the benefit of making
existing scripts using \g and \gx compatible.

For using ';' to push commands in a pipeline, I think it should be
fairly straightforward. I can try to work on that next week (I'm
currently chasing a weird memory context bug that I need to finish
first).

On Fri, Mar 7, 2025 at 1:05 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> One thing I've noticed is that \startpipeline throws warnings when
> copy pasting multiple lines. It seems to still execute everything as
> expected though. As an example you can copy paste this tiny script:
>
> \startpipeline
> select pg_sleep(5) \bind \g
> \endpipeline
>
> And then it will show these "extra argument ... ignored" warnings
>
> \startpipeline: extra argument "select" ignored
> \startpipeline: extra argument "pg_sleep(5)" ignored

It looks like an issue with libreadline. At least, I've been able to
reproduce the warnings and 'readline(prompt);' returns everything as a
single line, with the \n inside the string. This explains why what is
after \startpipeline is processed as arguments. This can also be done
with:
select 1 \bind \g
select 2 \bind \g
And somehow, I couldn't reproduce the issue anymore once I've compiled
and installed libreadline with debug symbols.

  33. Re: Add Pipelining support in psql

    Daniel Verite <daniel@manitou-mail.org> — 2025-03-07T22:31:12Z

    	Jelte Fennema-Nio wrote:

> As an example you can copy paste this tiny script:
> 
> \startpipeline
> select pg_sleep(5) \bind \g
> \endpipeline
> 
> And then it will show these "extra argument ... ignored" warnings
> 
> \startpipeline: extra argument "select" ignored
> \startpipeline: extra argument "pg_sleep(5)" ignored

It happens with other metacommands as well, and appears
to depend on a readline option that is "on" by default since
readline-8.1 [1]

 enable-bracketed-paste

    When set to ‘On’, Readline configures the terminal to insert each
    paste into the editing buffer as a single string of characters,
    instead of treating each character as if it had been read from the
    keyboard. This is called putting the terminal into bracketed paste
    mode; it prevents Readline from executing any editing commands
    bound to key sequences appearing in the pasted text. The default
    is ‘On’.


This behavior of the metacommand complaining about arguments 
on the next line also happens if using \e and typing this sequence
of commands  in the editor. In that case readline is not involved.

There might be something to improve here, because
a metacommand cannot take its argument from the next line,
and yet that's what the error messages somewhat imply.
But that issue is not related to the new pipeline metacommands.


[1]
https://tiswww.case.edu/php/chet/readline/readline.html#index-enable_002dbracketed_002dpaste


Best regards,
-- 
Daniel Vérité 
https://postgresql.verite.pro/

  34. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-03-17T09:50:50Z

    Here is a new patch set:

0001: This introduces the \sendpipeline meta-command and forbid \g in
a pipeline. This is to fix the formatting options of \g that are not
supported in a pipeline.

0002: Allows ';' to send a query using extended protocol when within a
pipeline by using PQsendQueryParams with 0 parameters. It is not
possible to send parameters with extended protocol this way and
everything will be propagated through the query string, similar to a
simple query.

  35. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-03-17T10:19:30Z

    On Mon, Mar 17, 2025 at 10:50:50AM +0100, Anthonin Bonnefoy wrote:
> 0001: This introduces the \sendpipeline meta-command and forbid \g in
> a pipeline. This is to fix the formatting options of \g that are not
> supported in a pipeline.
> 
> 0002: Allows ';' to send a query using extended protocol when within a
> pipeline by using PQsendQueryParams with 0 parameters. It is not
> possible to send parameters with extended protocol this way and
> everything will be propagated through the query string, similar to a
> simple query.

Thanks for sending a new patch set.  I was planning to look at the
situation tomorrow, and you have beaten me to it.

The split makes sense, and I'm OK with 0001.  0002 is going to require
a much closer lookup.
--
Michael

  36. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-03-18T00:50:27Z

    On Mon, Mar 17, 2025 at 10:50:50AM +0100, Anthonin Bonnefoy wrote:
> 0001: This introduces the \sendpipeline meta-command and forbid \g in
> a pipeline. This is to fix the formatting options of \g that are not
> supported in a pipeline.

- count 
--------
-     4
-(1 row)

This removal done in the regression tests was not intentional.

I have done some reordering of the code around the new meta-command so
as things are ordered alphabetically, and applied the result.

> 0002: Allows ';' to send a query using extended protocol when within a
> pipeline by using PQsendQueryParams with 0 parameters. It is not
> possible to send parameters with extended protocol this way and
> everything will be propagated through the query string, similar to a
> simple query.

I like the simplicity of what you are doing here, relying on
PSQL_SEND_QUERY being the default so as we use PQsendQueryParams()
with no parameters rather than PQsendQuery() when the pipeline mode is
not off.

How about adding a check on PIPELINE_COMMAND_COUNT when sending a
query through this path?  Should we check for more scenarios with
syncs and flushes as well when sending these queries?
--
Michael

  37. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-03-18T08:55:21Z

    On Tue, Mar 18, 2025 at 1:50 AM Michael Paquier <michael@paquier.xyz> wrote:
> - count
> --------
> -     4
> -(1 row)
>
> This removal done in the regression tests was not intentional.

Yes, thanks for fixing that.

> How about adding a check on PIPELINE_COMMAND_COUNT when sending a
> query through this path?  Should we check for more scenarios with
> syncs and flushes as well when sending these queries?

I've added additional tests when piping queries with ';':
- I've reused the same scenario with \sendpipeline: single query,
multiple queries, flushes, syncs, using COPY...
- Using ';' will replace the unnamed prepared statement. It's a bit
different from expected as a simple query will delete the unnamed
prepared statement.
- Sending an extended query prepared with \bind using a ';' on a
newline, though this is not specific to pipelining. The scanned
semicolon triggers the call to SendQuery, processing the buffered
extended query. It's a bit unusual but that's the current behaviour.

  38. Re: Add Pipelining support in psql

    Jelte Fennema <postgres@jeltef.nl> — 2025-03-18T09:27:38Z

    On Tue, 18 Mar 2025 at 09:55, Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
> I've added additional tests when piping queries with ';':
> - I've reused the same scenario with \sendpipeline: single query,
> multiple queries, flushes, syncs, using COPY...
> - Using ';' will replace the unnamed prepared statement. It's a bit
> different from expected as a simple query will delete the unnamed
> prepared statement.
> - Sending an extended query prepared with \bind using a ';' on a
> newline, though this is not specific to pipelining. The scanned
> semicolon triggers the call to SendQuery, processing the buffered
> extended query. It's a bit unusual but that's the current behaviour.

One thing that comes to mind that I think would be quite useful and
pretty easy to implement if we have this functionality within a
pipeline: An \extended command. That puts psql in "extended protocol
mode" (without enabling pipelining). In "extended protocol mode" all
queries would automatically be sent using PQsendQueryParams. That
would remove the need to use \bind anymore outside of a pipeline
either.

  39. Re: Add Pipelining support in psql

    Daniel Verite <daniel@manitou-mail.org> — 2025-03-18T09:36:28Z

    	Anthonin Bonnefoy wrote:

> 0002: Allows ';' to send a query using extended protocol when within a
> pipeline by using PQsendQueryParams

It's a nice improvement!

> with 0 parameters. It is not
> possible to send parameters with extended protocol this way and
> everything will be propagated through the query string, similar to a
> simple query.

It's actually possible to use parameters

\startpipeline 
\bind 'foo'
select $1;
\endpipeline

 ?column? 
----------
 foo
(1 row)

I suspect there's a misunderstanding that \bind can only be placed
after the query, because it's always written like that in the regression
tests, and in the documentation.
But that's just a notational preference.


Best regards,
-- 
Daniel Vérité 
https://postgresql.verite.pro/

  40. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-03-18T23:56:54Z

    On Tue, Mar 18, 2025 at 10:36:28AM +0100, Daniel Verite wrote:
> It's actually possible to use parameters
> 
> \startpipeline 
> \bind 'foo'
> select $1;
> \endpipeline
> 
>  ?column? 
> ----------
>  foo
> (1 row)
> 
> I suspect there's a misunderstanding that \bind can only be placed
> after the query, because it's always written like that in the regression
> tests, and in the documentation.
> But that's just a notational preference.

Nice trick, unrelated to pipelines.  I don't think that we have
anything testing this specific pattern for \bind.  At quick glance all
our test use \bind at the end of a query string.  Perhaps we should?
--
Michael

  41. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-03-19T02:28:54Z

    On Tue, Mar 18, 2025 at 10:27:38AM +0100, Jelte Fennema-Nio wrote:
> One thing that comes to mind that I think would be quite useful and
> pretty easy to implement if we have this functionality within a
> pipeline: An \extended command. That puts psql in "extended protocol
> mode" (without enabling pipelining). In "extended protocol mode" all
> queries would automatically be sent using PQsendQueryParams. That
> would remove the need to use \bind anymore outside of a pipeline
> either.

How does that help when passing parameter values?  \bind is here to be
able to pass down parameter values to queries that are prepared, so we
cannot bypass it as the parameter values need to be passed to the
\bind meta-command itself.

Perhaps an \extended command that behaves outside a pipeline makes
sense to force the use of queries without parameters to use the
extended mode, but I cannot get much excited about the concept knowing
all the meta-commands we have now (not talking about the pipeline
part, which is different, as we can treat queries in batches).
--
Michael

  42. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-03-19T04:49:23Z

    On Tue, Mar 18, 2025 at 09:55:21AM +0100, Anthonin Bonnefoy wrote:
> I've added additional tests when piping queries with ';':
> - I've reused the same scenario with \sendpipeline: single query,
> multiple queries, flushes, syncs, using COPY...
> - Using ';' will replace the unnamed prepared statement. It's a bit
> different from expected as a simple query will delete the unnamed
> prepared statement.
> - Sending an extended query prepared with \bind using a ';' on a
> newline, though this is not specific to pipelining. The scanned
> semicolon triggers the call to SendQuery, processing the buffered
> extended query. It's a bit unusual but that's the current behaviour.

The tests could be much more organized, particularly for the "sinple"
and "multiple" and COPY cases, rather than being treated as two
different groups at different locations of psql_pipeline.sql.  I've
spent some time reorganizing all that.

A second thing that was a bit itchy is the use of ";" for what's a
semicolon, and we use this term in the psql docs to refer to queries
terminated by that.  The whole paragraph could be simplified a bit
more, mentioning that everything in a pipeline uses the extended
protocol, while \bind & co are more like options.  The description of
PIPELINE_COMMAND_COUNT could be simpler, and the part about the
pending results can be more general now so I've removed it.

With all that set, I've applied the patch.  If you have more
suggestions, please feel free to mention them.
--
Michael

  43. Re: Add Pipelining support in psql

    Daniel Verite <daniel@manitou-mail.org> — 2025-03-19T13:05:34Z

    	Michael Paquier wrote:

> Perhaps an \extended command that behaves outside a pipeline makes
> sense to force the use of queries without parameters to use the
> extended mode, but I cannot get much excited about the concept knowing
> all the meta-commands we have now (not talking about the pipeline
> part, which is different, as we can treat queries in batches).

When psql started supporting the extended query protocol, the question
of enabling it more globally rather than query-by-query was discussed
a bit [1]. The idea was to switch to it with a setting or a variable
rather than a metacommand.
Some pros and cons were mentioned in the thread, but on the whole
it was not convincing enough to get implemented.

[1]
https://www.postgresql.org/message-id/e8dd1cd5-0e04-3598-0518-a605159fe314%40enterprisedb.com


Best regards,
-- 
Daniel Vérité 
https://postgresql.verite.pro/

  44. pipelining in psql, commit 41625ab

    Noah Misch <noah@leadboat.com> — 2025-04-15T21:34:50Z

    On Fri, Feb 21, 2025 at 11:33:41AM +0900, Michael Paquier wrote:
> So let's take one step here, I have applied the main patch.

commit 41625ab wrote:
>     * \syncpipeline queues a synchronisation request, without flushing the
>     commands to the server, equivalent of PQsendPipelineSync().

libpq has both PQpipelineSync() and PQsendPipelineSync(), so I find it odd
that the psql command for PQsendPipelineSync() is \syncpipeline.  I would have
expected the word "send" somewhere in its name.  That said, maybe having
PQpipelineSync() was a mistake, since I think it's just PQsendPipelineSync() +
PQflush().  In that light, it's reasonable not to spread the extra "send" word
into psql.  \syncpipeline is fine with me, but it's worth others taking a
second look.

> +				pg_log_error("\\getresults: invalid number of requested results");
> +				return PSQL_CMD_SKIP_LINE;

This should be PSQL_CMD_ERROR.  That matters under ON_ERROR_STOP=1.

> --- a/src/bin/psql/help.c
> +++ b/src/bin/psql/help.c
> @@ -167,15 +167,22 @@ slashUsage(unsigned short int pager)
>  	HELP0("  \\close STMT_NAME       close an existing prepared statement\n");
>  	HELP0("  \\copyright             show PostgreSQL usage and distribution terms\n");
>  	HELP0("  \\crosstabview [COLUMNS] execute query and display result in crosstab\n");
> +	HELP0("  \\endpipeline           exit pipeline mode\n");
>  	HELP0("  \\errverbose            show most recent error message at maximum verbosity\n");
> +	HELP0("  \\flush                 push unsent data to the server\n");
> +	HELP0("  \\flushrequest          send a flushrequest command\n");

protocol.sgml calls it a "Flush command".

>  	HELP0("  \\g [(OPTIONS)] [FILE]  execute query (and send result to file or |pipe);\n"
>  		  "                         \\g with no arguments is equivalent to a semicolon\n");
>  	HELP0("  \\gdesc                 describe result of query, without executing it\n");
> +	HELP0("  \\getresults [NUM_RES]  read NUM_RES pending results. All pending results are\n"
> +		  "                         read if no argument is provided\n");
>  	HELP0("  \\gexec                 execute query, then execute each value in its result\n");
>  	HELP0("  \\gset [PREFIX]         execute query and store result in psql variables\n");
>  	HELP0("  \\gx [(OPTIONS)] [FILE] as \\g, but forces expanded output mode\n");
>  	HELP0("  \\parse STMT_NAME       create a prepared statement\n");
>  	HELP0("  \\q                     quit psql\n");
> +	HELP0("  \\startpipeline         enter pipeline mode\n");
> +	HELP0("  \\syncpipeline          add a synchronisation point to an ongoing pipeline\n");

v17 "\?" has a 14-line "General" section:

General
  \bind [PARAM]...       set query parameters
  \copyright             show PostgreSQL usage and distribution terms
  \crosstabview [COLUMNS] execute query and display result in crosstab
  \errverbose            show most recent error message at maximum verbosity
  \g [(OPTIONS)] [FILE]  execute query (and send result to file or |pipe);
                         \g with no arguments is equivalent to a semicolon
  \gdesc                 describe result of query, without executing it
  \gexec                 execute query, then execute each value in its result
  \gset [PREFIX]         execute query and store result in psql variables
  \gx [(OPTIONS)] [FILE] as \g, but forces expanded output mode
  \q                     quit psql
  \watch [[i=]SEC] [c=N] [m=MIN]
                         execute query every SEC seconds, up to N times,
                         stop if less than MIN rows are returned

v18 has raised that to 26 lines via $SUBJECT and other additions.  I think a
new "Extended Query Protocol" section should house \bind and all the v18
additions.  Beginners should ignore the new section.  The section may as well
appear last.  What do you think?

  45. Re: pipelining in psql, commit 41625ab

    Jelte Fennema <postgres@jeltef.nl> — 2025-04-15T22:00:19Z

    On Tue, 15 Apr 2025 at 23:34, Noah Misch <noah@leadboat.com> wrote:
> That said, maybe having
> PQpipelineSync() was a mistake, since I think it's just PQsendPipelineSync() +
> PQflush().

Yes, IMO that's pretty much the case. But we cannot remove that
function because of backwards compatibility.

Note that for all the other commands (e.g \bind, \close, \parse) we
use the send variant of the libpq function too without including send
in the command. However, send means a different thing for those: If
you use the non-send one you get a PGresult back.

  46. Re: Add Pipelining support in psql

    a.kozhemyakin <a.kozhemyakin@postgrespro.ru> — 2025-04-16T14:31:59Z

    Hello,

After commit 2cce0fe on master

When executing query:
psql postgres <<EOF
CREATE TABLE psql_pipeline();
\startpipeline
COPY psql_pipeline FROM STDIN;
SELECT 'val1';
\syncpipeline
\getresults
EOF


ERROR:  unexpected message type 0x50 during COPY from stdin
CONTEXT:  COPY psql_pipeline, line 1
Pipeline aborted, command did not run
psql: common.c:1510: discardAbortedPipelineResults: Assertion `res == 
((void *)0) || result_status == PGRES_PIPELINE_ABORTED' failed.
Aborted (core dumped)


The psql crashes with the stack trace:
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, 
threadid=<optimized out>) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=<optimized out>) at 
./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) 
at ./nptl/pthread_kill.c:89
#3  0x0000760edd24527e in __GI_raise (sig=sig@entry=6) at 
../sysdeps/posix/raise.c:26
#4  0x0000760edd2288ff in __GI_abort () at ./stdlib/abort.c:79
#5  0x0000760edd22881b in __assert_fail_base (fmt=0x760edd3d01e8 
"%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
     assertion=assertion@entry=0x5ba46ab79850 "res == ((void *)0) || 
result_status == PGRES_PIPELINE_ABORTED", file=file@entry=0x5ba46ab6fcad 
"common.c",
     line=line@entry=1510, function=function@entry=0x5ba46ab9c780 
<__PRETTY_FUNCTION__.3> "discardAbortedPipelineResults") at 
./assert/assert.c:96
#6  0x0000760edd23b517 in __assert_fail 
(assertion=assertion@entry=0x5ba46ab79850 "res == ((void *)0) || 
result_status == PGRES_PIPELINE_ABORTED",
     file=file@entry=0x5ba46ab6fcad "common.c", line=line@entry=1510,
     function=function@entry=0x5ba46ab9c780 <__PRETTY_FUNCTION__.3> 
"discardAbortedPipelineResults") at ./assert/assert.c:105
#7  0x00005ba46ab2bd40 in discardAbortedPipelineResults () at common.c:1510
#8  ExecQueryAndProcessResults (query=query@entry=0x5ba4a2ec1e10 "SELECT 
'val1';", elapsed_msec=elapsed_msec@entry=0x7ffeb07262a8,
     svpt_gone_p=svpt_gone_p@entry=0x7ffeb07262a7, 
is_watch=is_watch@entry=false, min_rows=min_rows@entry=0, 
opt=opt@entry=0x0, printQueryFout=0x0)
     at common.c:1811
#9  0x00005ba46ab2983f in SendQuery (query=0x5ba4a2ec1e10 "SELECT 
'val1';") at common.c:1212
#10 0x00005ba46ab3f66a in MainLoop (source=source@entry=0x760edd4038e0 
<_IO_2_1_stdin_>) at mainloop.c:515
#11 0x00005ba46ab23f2a in process_file (filename=0x0, 
use_relative_path=use_relative_path@entry=false) at command.c:4870
#12 0x00005ba46ab1e9d9 in main (argc=<optimized out>, 
argv=0x7ffeb07269d8) at startup.c:420




06.03.2025 11:20, Michael Paquier пишет:
> On Wed, Mar 05, 2025 at 03:25:12PM +0100, Daniel Verite wrote:
>> 	Anthonin Bonnefoy wrote:
>>> I do see the idea to make it easier to convert existing scripts into
>>> using pipelining. The main focus of the initial implementation was
>>> more on protocol regression tests with psql, so that's not necessarily
>>> something I had in mind.
>> Understood. Yet pipelining can accelerate considerably certain scripts
>> when client-server latency is an issue. We should expect end users to
>> benefit from it too.
> That was not a test case we had in mind originally here, but if it is
> possible to keep the implementation simple while supporting your
> demand, well, let's do it.  If it's not that straight-forward, let's
> use the new meta-command, forbidding \g and \gx based on your
> arguments from upthread.
> --
> Michael

  47. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-04-16T16:18:01Z

    On Wed, Apr 16, 2025 at 09:31:59PM +0700, a.kozhemyakin wrote:
> After commit 2cce0fe on master
> 
> ERROR:  unexpected message type 0x50 during COPY from stdin
> CONTEXT:  COPY psql_pipeline, line 1
> Pipeline aborted, command did not run
> psql: common.c:1510: discardAbortedPipelineResults: Assertion `res == ((void
> *)0) || result_status == PGRES_PIPELINE_ABORTED' failed.
> Aborted (core dumped)

Reproduced here, thanks for the report.  I'll look at that at the
beginning of next week, adding an open item for now.
--
Michael

  48. Re: pipelining in psql, commit 41625ab

    Michael Paquier <michael@paquier.xyz> — 2025-04-16T16:46:42Z

    On Tue, Apr 15, 2025 at 02:34:50PM -0700, Noah Misch wrote:
> On Fri, Feb 21, 2025 at 11:33:41AM +0900, Michael Paquier wrote:
> commit 41625ab wrote:
>>     * \syncpipeline queues a synchronisation request, without flushing the
>>     commands to the server, equivalent of PQsendPipelineSync().
>> 
> libpq has both PQpipelineSync() and PQsendPipelineSync(), so I find it odd
> that the psql command for PQsendPipelineSync() is \syncpipeline.  I would have
> expected the word "send" somewhere in its name.  That said, maybe having
> PQpipelineSync() was a mistake, since I think it's just PQsendPipelineSync() +
> PQflush().  In that light, it's reasonable not to spread the extra "send" word
> into psql.  \syncpipeline is fine with me, but it's worth others taking a
> second look.

At this stage, PQpipelineSync() is just around for compatibility
purposes, we cannot remove it and I don't see recommending it either.
It is true that \syncpipeline does not map clearly with the fact that
we're just calling PQsendPipelineSync() under the hood.

> 
>> +				pg_log_error("\\getresults: invalid number of requested results");
>> +				return PSQL_CMD_SKIP_LINE;
> 
> This should be PSQL_CMD_ERROR.  That matters under ON_ERROR_STOP=1.

Yes, good catch.  Will fix.

>> --- a/src/bin/psql/help.c
>> +++ b/src/bin/psql/help.c
>> @@ -167,15 +167,22 @@ slashUsage(unsigned short int pager)
>>  	HELP0("  \\close STMT_NAME       close an existing prepared statement\n");
>>  	HELP0("  \\copyright             show PostgreSQL usage and distribution terms\n");
>>  	HELP0("  \\crosstabview [COLUMNS] execute query and display result in crosstab\n");
>> +	HELP0("  \\endpipeline           exit pipeline mode\n");
>>  	HELP0("  \\errverbose            show most recent error message at maximum verbosity\n");
>> +	HELP0("  \\flush                 push unsent data to the server\n");
>> +	HELP0("  \\flushrequest          send a flushrequest command\n");
> 
> protocol.sgml calls it a "Flush command".

For \flushrequest, how about "send a request to the server to flush
its output buffer" and for \flush "flush output data to the server",
mapping more with the libpq desctiptions?

> General
>   \bind [PARAM]...       set query parameters
>   \copyright             show PostgreSQL usage and distribution terms
>   \crosstabview [COLUMNS] execute query and display result in crosstab
>   \errverbose            show most recent error message at maximum verbosity
>   \g [(OPTIONS)] [FILE]  execute query (and send result to file or |pipe);
>                          \g with no arguments is equivalent to a semicolon
>   \gdesc                 describe result of query, without executing it
>   \gexec                 execute query, then execute each value in its result
>   \gset [PREFIX]         execute query and store result in psql variables
>   \gx [(OPTIONS)] [FILE] as \g, but forces expanded output mode
>   \q                     quit psql
>   \watch [[i=]SEC] [c=N] [m=MIN]
>                          execute query every SEC seconds, up to N times,
>                          stop if less than MIN rows are returned
> 
> v18 has raised that to 26 lines via $SUBJECT and other additions.  I think a
> new "Extended Query Protocol" section should house \bind and all the v18
> additions.  Beginners should ignore the new section.  The section may as well
> appear last.  What do you think?

At this stage, most of the bloat of the general section is caused by
the extended protocol commands, so I like your suggestion of a new
section with a split from General.
--
Michael

  49. Re: pipelining in psql, commit 41625ab

    Noah Misch <noah@leadboat.com> — 2025-04-16T23:13:10Z

    On Wed, Apr 16, 2025 at 09:46:42AM -0700, Michael Paquier wrote:
> On Tue, Apr 15, 2025 at 02:34:50PM -0700, Noah Misch wrote:
> > On Fri, Feb 21, 2025 at 11:33:41AM +0900, Michael Paquier wrote:
> > commit 41625ab wrote:
> >> --- a/src/bin/psql/help.c
> >> +++ b/src/bin/psql/help.c
> >> @@ -167,15 +167,22 @@ slashUsage(unsigned short int pager)
> >>  	HELP0("  \\close STMT_NAME       close an existing prepared statement\n");
> >>  	HELP0("  \\copyright             show PostgreSQL usage and distribution terms\n");
> >>  	HELP0("  \\crosstabview [COLUMNS] execute query and display result in crosstab\n");
> >> +	HELP0("  \\endpipeline           exit pipeline mode\n");
> >>  	HELP0("  \\errverbose            show most recent error message at maximum verbosity\n");
> >> +	HELP0("  \\flush                 push unsent data to the server\n");
> >> +	HELP0("  \\flushrequest          send a flushrequest command\n");
> > 
> > protocol.sgml calls it a "Flush command".
> 
> For \flushrequest, how about "send a request to the server to flush
> its output buffer" and for \flush "flush output data to the server",
> mapping more with the libpq desctiptions?

Works for me.

  50. Re: pipelining in psql, commit 41625ab

    Michael Paquier <michael@paquier.xyz> — 2025-04-21T03:13:22Z

    On Wed, Apr 16, 2025 at 04:13:10PM -0700, Noah Misch wrote:
> Works for me.

I have fixed the three defects you have reported with 5ee7bd944ee8,
5743d122fcf3 and 78231baaf967 for respectively the error code of
\getresults, the description of \flush[request] and the output of
--help=commands with the new sub-section for the extended protocol
versions.

There is still one pending item for the assertion failure, registered
as an open item..  I'll catch up with that soon.
--
Michael

  51. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-04-21T06:22:28Z

    On Wed, Apr 16, 2025 at 09:18:01AM -0700, Michael Paquier wrote:
> On Wed, Apr 16, 2025 at 09:31:59PM +0700, a.kozhemyakin wrote:
>> After commit 2cce0fe on master
>> 
>> ERROR:  unexpected message type 0x50 during COPY from stdin
>> CONTEXT:  COPY psql_pipeline, line 1
>> Pipeline aborted, command did not run
>> psql: common.c:1510: discardAbortedPipelineResults: Assertion `res == ((void
>> *)0) || result_status == PGRES_PIPELINE_ABORTED' failed.
>> Aborted (core dumped)
> 
> Reproduced here, thanks for the report.  I'll look at that at the
> beginning of next week, adding an open item for now.

The failure is not related to 2cce0fe.  The following sequence fails
as well, as long as we have one SELECT after the COPY to mess up with
the \getresults that causes a PGRES_FATAL_ERROR combined with a
"terminating connection because protocol synchronization was lost" on
the backend side, because the server expects some data while the
client does not send it but psql is not able to cope with this state:
\startpipeline
COPY psql_pipeline FROM STDIN \bind \sendpipeline
SELECT $1 \bind 'val1' \sendpipeline
\syncpipeline
\getresults
\endpipeline

It's actually nice that we are able to emulate such query patterns
with psql using all these meta-commands, I don't think we have
any coverage for the backend synchronization loss case yet like this
one?

2cce0fe makes that easier to reach by allowing more command patterns,
but it's the mix of COPY followed by a SELECT that causes psql to be
confused.  All the tests that we have don't check this kind of
scenarios, for COPY TO/FROM, with always use a flush or a sync
followed quickly by \getresults, but we don't have tests where we mix
things.

Anyway, I don't think that there is much we can do under a
PGRES_FATAL_ERROR in this code path when discarding the pipe results.
As far as I can tell, the server has failed the query suddenly and the
whole pipeline flow is borked.  The best thing that I can think of is
to discard all the results while decrementing the counters, then let
psql complain about that like in the attached.  I've added two tests
in TAP, as these trigger a FATAL in the backend so we cannot use the
normal SQL route, so as we have some coverage.

@Anthonin: Any thoughts or comments, perhaps?  A second opinion would
be welcome here.
--
Michael

  52. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-04-22T00:06:20Z

    On Mon, Apr 21, 2025 at 03:22:28PM +0900, Michael Paquier wrote:
> Anyway, I don't think that there is much we can do under a
> PGRES_FATAL_ERROR in this code path when discarding the pipe results.
> As far as I can tell, the server has failed the query suddenly and the
> whole pipeline flow is borked.  The best thing that I can think of is
> to discard all the results while decrementing the counters, then let
> psql complain about that like in the attached.  I've added two tests
> in TAP, as these trigger a FATAL in the backend so we cannot use the
> normal SQL route, so as we have some coverage.
> 
> @Anthonin: Any thoughts or comments, perhaps?  A second opinion would
> be welcome here.

While considering more ways to test this patch, I've recalled that
injection points that issue a FATAL in the backend to emulate the
original failure with more query patterns can provide more coverage,
and the discard cleanup is showing stable enough as presented in the
patch.  I am wondering if we could not be smarter with the handling of
the counters, but I really doubt that there is much more we can do
under a PGRES_FATAL_ERROR.
--
Michael

  53. Re: Add Pipelining support in psql

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-04-22T12:37:19Z

    On Tue, Apr 22, 2025 at 2:06 AM Michael Paquier <michael@paquier.xyz> wrote:
> I am wondering if we could not be smarter with the handling of
> the counters, but I really doubt that there is much more we can do
> under a PGRES_FATAL_ERROR.

One thing that bothers me is that the reported error is silently
discarded within discardAbortedPipelineResults.

psql -f bug_assertion.sql
psql:bug_assertion.sql:7: ERROR:  unexpected message type 0x50 during
COPY from stdin
CONTEXT:  COPY psql_pipeline, line 1
psql:bug_assertion.sql:7: Pipeline aborted, command did not run

This should ideally report the "FATAL:  terminating connection because
protocol synchronization was lost" sent by the backend process.

Also, we still have a triggered assertion failure with the following:
CREATE TABLE psql_pipeline(a text);
\startpipeline
COPY psql_pipeline FROM STDIN;
SELECT 'val1';
\syncpipeline
\endpipeline
...
Assertion failed: (pset.piped_syncs == 0), function
ExecQueryAndProcessResults, file common.c, line 2153.

A possible alternative could be to abort discardAbortedPipelineResults
when we encounter a res != NULL + FATAL error and let the outer loop
handle it. As you said, the pipeline flow is borked so there's not
much to salvage. The outer loop would read and print all error
messages until the closed connection is detected. Then,
CheckConnection will reset the connection which will reset the
pipeline state.

While testing this change, I was initially looking for the "FATAL:
terminating connection because protocol synchronization was lost"
message in the tests. However, this was failing on Windows[1] as the
FATAL message wasn't reported on stderr. I'm not sure why yet.

[1]: https://cirrus-ci.com/task/5051031505076224?logs=check_world#L240-L246

  54. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-04-23T07:13:14Z

    On Tue, Apr 22, 2025 at 02:37:19PM +0200, Anthonin Bonnefoy wrote:
> Also, we still have a triggered assertion failure with the following:
> CREATE TABLE psql_pipeline(a text);
> \startpipeline
> COPY psql_pipeline FROM STDIN;
> SELECT 'val1';
> \syncpipeline
> \endpipeline
> ...
> Assertion failed: (pset.piped_syncs == 0), function
> ExecQueryAndProcessResults, file common.c, line 2153.

Right.  I didn't think about the case of a \endpipeline that fetches
all the results by itself.

> A possible alternative could be to abort discardAbortedPipelineResults
> when we encounter a res != NULL + FATAL error and let the outer loop
> handle it. As you said, the pipeline flow is borked so there's not
> much to salvage. The outer loop would read and print all error
> messages until the closed connection is detected. Then,
> CheckConnection will reset the connection which will reset the
> pipeline state.

Sounds like a better idea seen from here, yes.

> While testing this change, I was initially looking for the "FATAL:
> terminating connection because protocol synchronization was lost"
> message in the tests. However, this was failing on Windows[1] as the
> FATAL message wasn't reported on stderr. I'm not sure why yet.

Hmm.  I vaguely recall that there could be some race condition here
with the attempt to catch up the FATAL message once the server tries
to shut down the connection..

Anyway, I agree that it would be nice to track that this specific
error message is generated in the server.  How about checking the
server logs instead, using a slurp_file() with an offset of the log
file before running each pipeline sequence?  We should use a few
wait_for_log() calls, I think, to be extra careful with the timings
where psql_fails_like() gives up, and I'm worried that this could be
unstable on slow machines.  Something like the attached seems stable
enough here.  What do you think?

The tweak for psql_fails_like() was kind of independent of the rest of
the fix, so I have applied that as a commit of its own.  I am not
convinced about the addition of a 4th test where we use the queries
with semicolons without a \getresults between the sync and the end.
--
Michael

  55. Re: Add Pipelining support in psql

    Michael Paquier <michael@paquier.xyz> — 2025-04-24T03:26:33Z

    On Wed, Apr 23, 2025 at 04:13:14PM +0900, Michael Paquier wrote:
> The tweak for psql_fails_like() was kind of independent of the rest of
> the fix, so I have applied that as a commit of its own.  I am not
> convinced about the addition of a 4th test where we use the queries
> with semicolons without a \getresults between the sync and the end.

I've had some room to look again at all that this morning as this is
an open item, and I have applied the fix.  One tweak was in the tests,
where I have added only one wait_for_log() which should offer enough
coverage.

If you notice anything else, please let me know.
--
Michael