Re: PATCH: Batch/pipelining support for libpq

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
54 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Andres Freund
Hi,

On 2020-11-03 10:42:34 -0300, Alvaro Herrera wrote:
> It would definitely help if you (and others) could think about the API
> being added: Does it fulfill the promises being made?  Does it offer the
> guarantees that real-world apps want to have?  I'm not much of an
> application writer myself -- particularly high-traffic apps that would
> want to use this.

Somewhere earlier in this thread there was a patch with support for
batching in pgbench. I think it'd be good to refresh that. Both because
it shows at least some real-world-lite usage of the feature and because
we need a way to stress it to see whether it has unnecessary
bottlenecks.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

matt-42
In reply to this post by David G Johnston
Hi David,

Thanks for the feedback. I did rework a bit the doc based on your
remarks. Here is the v24 patch.

Matthieu Garrigues

On Tue, Nov 3, 2020 at 6:21 PM David G. Johnston
<[hidden email]> wrote:

>
> On Mon, Nov 2, 2020 at 8:58 AM Alvaro Herrera <[hidden email]> wrote:
>>
>> On 2020-Nov-02, Alvaro Herrera wrote:
>>
>> > In v23 I've gone over docs; discovered that PQgetResults docs were
>> > missing the new values.  Added those.  No significant other changes yet.
>>
>
> Just reading the documentation of this patch, haven't been following the longer thread:
>
> Given the caveats around blocking mode connections why not just require non-blocking mode, in a similar fashion to how synchronous functions are disallowed?
>
> "Batched operations will be executed by the server in the order the client
> sends them. The server will send the results in the order the statements
> executed."
>
> Maybe:
>
> "The server executes statements, and returns results, in the order the client sends them."
>
> Using two sentences and relying on the user to mentally link the two "in the order" descriptions together seems to add unnecessary cognitive load.
>
> +     The client <link linkend="libpq-batch-interleave">interleaves result
> +     processing</link> with sending batch queries, or for small batches may
> +     process all results after sending the whole batch.
>
> Suggest: "The client may choose to interleave result processing with sending batch queries, or wait until the complete batch has been sent."
>
> I would expect to process the results of a batch only after sending the entire batch to the server.  That I don't have to is informative but knowing when I should avoid doing so, and why, is informative as well.  To the extreme while you can use batch mode and interleave if you just poll getResult after every command you will make the whole batch thing pointless.  Directing the reader from here to the section "Interleaving Result Processing and Query Dispatch" seems worth considering.  The dynamics of small sizes and sockets remains a bit unclear as to what will break (if anything, or is it just process memory on the server) if interleaving it not performed and sizes are large.
>
> I would suggest placing commentary about "all transactions subsequent to a failed transaction in a batch are ignored while previous completed transactions are retained" in the "When to Use Batching".  Something like "Batching is less useful, and more complex, when a single batch contains multiple transactions (see Error Handling)."
>
> My imagined use case would be to open a batch, start a transaction, send all of its components, end the transaction, end the batch, check for batch failure and if it doesn't fail have the option to easily continue without processing individual pgResults (or if it does fail, have the option to extract the first error pgResult and continue, ignoring the rest, knowing that the transaction as a whole was reverted and the batch unapplied).  I've never interfaced with libpq directly.  Though given how the existing C API works what is implemented here seems consistent.
>
> The "queueing up queries into a pipeline to be executed as a batch on the server" can be read as a client-side behavior where nothing is sent to the server until the batch has been completed.  Reading further it becomes clear that all it basically is is a sever-side toggle that instructs the server to continue processing incoming commands even while prior commands have their results waiting to be ingested by the client.
>
> Batch seems like the user-visible term to describe this feature.  Pipeline seems like an implementation detail that doesn't need to be mentioned in the documentation - especially given that pipeline doesn't get a mentioned beyond the first two paragraphs of the chapter and never without being linked directly to "batch".  I would probably leave the indexterm and have a paragraph describing that batching is implemented using a query pipeline so that people with the implementation detail on their mind can find this chapter, but the prose for the user should just stick to batching.
>
> Sorry, that all is a bit unfocused, but the documentation for the user of the API could be cleaned up a bit and some more words spent on what trade-offs are being made when using batching versus normal command-response processing.  That said, while I don't see all of this purely a matter of style I'm also not seeing anything demonstrably wrong with the documentation at the moment.  Hopefully my perspective helps though, and depending on what happens next I may try and make my thoughts more concrete with an actual patch.
>
> David J.
>

v24-libpq-batch.patch (117K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
In reply to this post by David G Johnston
(Adding previous reviewers to CC)

On 2020-Nov-03, David G. Johnston wrote:

> Given the caveats around blocking mode connections why not just require
> non-blocking mode, in a similar fashion to how synchronous functions are
> disallowed?

This is a very good question.  Why indeed?  Does anybody have a good
answer to this?  If not, I propose we just require that non-blocking
mode is in use in order for batch mode to be used.

I've been doing a review pass over this patch and have an updated
version, which I intend to share later today (after I fix what appears
to be a misunderstanding in the "singlerow" test in testlibpqbatch.c)


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
Here's a v25.

I made a few more changes to the docs per David's suggestions; I also
reordered the sections quite a bit.  It's now like this:
 * Batch Mode
   * Using Batch Mode
     * Issuing Queries
     * Processing Results
     * Error Handling
     * Interleaving Result Processing and Query Dispatch
     * Ending Batch Mode
   * Functions Associated with Batch Mode
   * When to Use Batching

To me as a reader, this makes more sense, but if you disagree, I think
we should discuss further changes.  (For example, maybe we should move
the "Functions" section at the end?)  The original had "When to Use
Batching" at the start, but it seemed to me that the points it's making
are not as critical as understanding what it is.

I reworked the test program to better fit the TAP model; I found that if
one test mecha failed in whatever way, the connection would be in a
weird state and cause the next test to also fail.  I changed so that it
runs one test and exits; then the t/001_libpq_async.pl file (hmm, need
to rename to 001_batch.pl I guess) calls it once for each test.
I adapted the test code to our code style.  I also removed the "timings"
stuff; I think that's something better left to pgbench.

(I haven't looked at Daniel's pgbench stuff yet, but I will do that
next.)

While looking at how the tests worked, I gave a hard stare at the new
libpq code and cleaned it up also.  There's a lot of minor changes, but
nothing truly substantial.  I moved the code around a lot, to keep
things where grouped together they belong.

I'm not 100% clear on things like PGconn->batch_status and how
PGconn->asyncStatus works.  Currently everything seems to work
correctly, but I'm worried that because we add new status values to
asyncStatus, some existing code might not be doing everything correctly
(for example when changing to/from ASYNC_BUSY in some cases, are we 100%
we shouldn't be changing to ASYNC_QUEUED?)


While looking this over I noticed a thread from 2014[1] where Matt
Newell tried to implement this stuff and apparently the main review
comment he got before abandoning the patch was that the user would like
a way to access the query that corresponded to each result.  The current
patch set does not address that need; the approach to this problem is
that it's on the application's head to keep track of this.  Honestly I
don't understand why it would be otherwise ... I'm not sure that it
makes sense to expect that the application is stupid enough that it
doesn't keep track in which order it sent things, but bright enough to
keep pointers to the queries it sent (??).  So this seems okay to me.
But added Heikki and Claudio to CC because of that old thread.


[1] https://postgr.es/m/2059418.jZQvL3lH90@obsidian


v25-libpq-batch.patch (88K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Daniel Verite
        Alvaro Herrera wrote:

> I adapted the test code to our code style.  I also removed the "timings"
> stuff; I think that's something better left to pgbench.
>
> (I haven't looked at Daniel's pgbench stuff yet, but I will do that
> next.)

The patch I posted in [1] was pretty simple, but at the time, query
results were always discarded. Now that pgbench can instantiate
variables from query results, a script can do:
  select 1 as var \gset
  select :var;
This kind of sequence wouldn't work in batch mode since it
sends queries before getting results of previous queries.

So maybe \gset should be rejected when inside a batch section.

Or alternatively pgbench should collect results before a variable is
reinjected into a query, thereby stalling the pipeline.
To do this only when necessary, it would have to track read-write
dependencies among variables, which seems overly complicated though.

[1]
https://www.postgresql.org/message-id/b4e34135-2bd9-4b8a-94ca-27d760da26d7@...

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: https://www.manitou-mail.org
Twitter: @DanielVerite


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

David G Johnston
In reply to this post by Álvaro Herrera
On Fri, Nov 13, 2020 at 5:38 PM Alvaro Herrera <[hidden email]> wrote:
Here's a v25.

I made a few more changes to the docs per David's suggestions; I also
reordered the sections quite a bit.  It's now like this:
 * Batch Mode
   * Using Batch Mode
     * Issuing Queries
     * Processing Results
     * Error Handling
     * Interleaving Result Processing and Query Dispatch
     * Ending Batch Mode
   * Functions Associated with Batch Mode
   * When to Use Batching

Thanks!

I like the new flow and changes.  I've attached a patch that covers some missing commas and typos along with a few parts that made me pause.

The impact of memory came out of nowhere under the non-blocking mode commentary.  I took a stab at incorporating it more broadly.

The "are error conditions" might be a well-known phrasing to those using libpq but that sentence reads odd to me.  I did try to make the example listing flow a bit better and added a needed comma.

Tried to clean up a few phrasings after that.  The error handling part I'm not especially happy with but I think it's closer and more useful than just "emitted during error handling" - it gets emitted upon error, handling is a client concern.

Seems odd to say the new feature was introduced in v14.0, the .0 seems ok to imply.  I didn't actually fix it in the attached but "the PostgreSQL 14 version of libpq" is going to become outdated quickly, better just to drop it.

"The batch API was introduced in PostgreSQL 14, but clients can use batches on servers supporting v3 of the extended query protocol, potentially going as far back as version 7.4."

David J.




v25-01-libpq-batch-dgj-doc-suggestions.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
In reply to this post by Daniel Verite
On 2020-Nov-14, Daniel Verite wrote:

> The patch I posted in [1] was pretty simple, but at the time, query
> results were always discarded. Now that pgbench can instantiate
> variables from query results, a script can do:
>   select 1 as var \gset
>   select :var;
> This kind of sequence wouldn't work in batch mode since it
> sends queries before getting results of previous queries.
>
> So maybe \gset should be rejected when inside a batch section.

Hah.

Hacking pgbench extensively is beyond what I'm willing to do for this
feature at this time.  Making \gset rejected in a batch section sounds
simple enough and supports \beginbatch et al sufficiently to compare
performance, so I'm OK with a patch that does that.  That'd be a small
extension to your previous patch, if I understand correctly.

If you or others want to send patches to extend batch support with
read-write tracking for variables, feel free, but I hereby declare that
I'm not taking immediate responsibility for getting them committed.



Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Andres Freund
Hi

On Wed, Nov 18, 2020, at 09:51, Alvaro Herrera wrote:

> On 2020-Nov-14, Daniel Verite wrote:
>
> > The patch I posted in [1] was pretty simple, but at the time, query
> > results were always discarded. Now that pgbench can instantiate
> > variables from query results, a script can do:
> >   select 1 as var \gset
> >   select :var;
> > This kind of sequence wouldn't work in batch mode since it
> > sends queries before getting results of previous queries.
> >
> > So maybe \gset should be rejected when inside a batch section.
>
> Hah.
>
> Hacking pgbench extensively is beyond what I'm willing to do for this
> feature at this time.  Making \gset rejected in a batch section sounds
> simple enough and supports \beginbatch et al sufficiently to compare
> performance, so I'm OK with a patch that does that.  That'd be a small
> extension to your previous patch, if I understand correctly.
>
> If you or others want to send patches to extend batch support with
> read-write tracking for variables, feel free, but I hereby declare that
> I'm not taking immediate responsibility for getting them committed.

I think minimal support is entirely sufficient initially.

Andres


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Daniel Verite
In reply to this post by Álvaro Herrera
 Hi,

Here's a new version with the pgbench support included.

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: https://www.manitou-mail.org
Twitter: @DanielVerite

v26-libpq-batch.patch (128K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
On 2020-Nov-23, Daniel Verite wrote:

>  Hi,
>
> Here's a new version with the pgbench support included.

Thanks, incorporated into my local copy.



Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
In reply to this post by Daniel Verite
Thanks David Johnston and Daniel Vérité, I have incorporated your
changes into this patch, which is now v26.  Also, it's been rebased on
current sources.

I've been using the new PQtrace() stuff to verify the behavior of the
new feature.  It's not perfect, but at least it doesn't crash
immediately as it did when I tried a few weeks ago.  There are
imperfections that I think are due to bugs in the PQtrace
implementation, not in this patch.

--
Álvaro Herrera                            39°49'30"S 73°17'W
"El conflicto es el camino real hacia la unión"

v26-libpq-batch.patch (95K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
As you can see in an XXX comment in the libpq test program, the current
implementation has the behavior that PQgetResult() returns NULL after a
batch is finished and has reported PGRES_BATCH_END.  I don't know if
there's a hard reason to do that, but I'd like to supress it because it
seems weird and out of place.

--
Álvaro Herrera                            39°49'30"S 73°17'W


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Zhihong Yu
In reply to this post by Álvaro Herrera
Hi,

+                       commandFailed(st, "SQL", "\\gset and \\aset are not allowed in a batch section");

It seems '\\gset or \\aset is not ' would correspond to the check more closely.

+       if (my_command->argc != 1)
+           syntax_error(source, lineno, my_command->first_line, my_command->argv[0],

It is possible that my_command->argc == 0 (where my_command->argv[0] shouldn't be accessed) ?

+               appendPQExpBufferStr(&conn->errorMessage,
+                                 libpq_gettext("cannot queue commands during COPY\n"));
+               return false;
+               break;

Is the break necessary ? Similar comment for pqBatchProcessQueue().

+int
+PQexitBatchMode(PGconn *conn)

Since either 0 or 1 is returned, maybe change the return type to bool.
Also, the function operates on PGconn - should the function be static (pqBatchProcessQueue is) ?

Cheers

On Thu, Jan 21, 2021 at 3:39 PM Alvaro Herrera <[hidden email]> wrote:
Thanks David Johnston and Daniel Vérité, I have incorporated your
changes into this patch, which is now v26.  Also, it's been rebased on
current sources.

I've been using the new PQtrace() stuff to verify the behavior of the
new feature.  It's not perfect, but at least it doesn't crash
immediately as it did when I tried a few weeks ago.  There are
imperfections that I think are due to bugs in the PQtrace
implementation, not in this patch.

--
Álvaro Herrera                            39°49'30"S 73°17'W
"El conflicto es el camino real hacia la unión"
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
In reply to this post by Álvaro Herrera
On 2021-Jan-21, Alvaro Herrera wrote:

> As you can see in an XXX comment in the libpq test program, the current
> implementation has the behavior that PQgetResult() returns NULL after a
> batch is finished and has reported PGRES_BATCH_END.  I don't know if
> there's a hard reason to do that, but I'd like to supress it because it
> seems weird and out of place.

Hello Craig, a question for you since this API is your devising.  I've
been looking at changing the way this works to prevent those NULL
returns from PQgetResult.  That is, instead of having what seems like a
"NULL separator" of query results, you'd just get the PGRES_BATCH_END to
signify a batch end (not a NULL result after the BATCH_END); and the
normal PGRES_COMMAND_OK or PGRES_TUPLES_OK etc when the result of a
command has been sent.  It's not working yet so I'm not sending an
updated patch, but I wanted to know if you had a rationale for including
this NULL return "separator" or was it just a convenience because of how
the code grew together.

Such a decision has obvious consequences for the test program (which
right now expects that PQgetResult() returns NULL at each step); and
naturally for libpq's internal state machine that controls how it all
works.

Thanks,

--
Álvaro Herrera                            39°49'30"S 73°17'W


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Craig Ringer-5
On Thu, 11 Feb 2021 at 07:51, Alvaro Herrera <[hidden email]> wrote:
On 2021-Jan-21, Alvaro Herrera wrote:

> As you can see in an XXX comment in the libpq test program, the current
> implementation has the behavior that PQgetResult() returns NULL after a
> batch is finished and has reported PGRES_BATCH_END.  I don't know if
> there's a hard reason to do that, but I'd like to supress it because it
> seems weird and out of place.

Hello Craig, a question for you since this API is your devising.  I've
been looking at changing the way this works to prevent those NULL
returns from PQgetResult.  That is, instead of having what seems like a
"NULL separator" of query results, you'd just get the PGRES_BATCH_END to
signify a batch end (not a NULL result after the BATCH_END); and the
normal PGRES_COMMAND_OK or PGRES_TUPLES_OK etc when the result of a
command has been sent.  It's not working yet so I'm not sending an
updated patch, but I wanted to know if you had a rationale for including
this NULL return "separator" or was it just a convenience because of how
the code grew together.

The existing API for libpq actually specifies[1] that you should call PQgetResult() until it returns NULL:

> After successfully calling PQsendQuery, call PQgetResult one or more times to obtain the results. PQsendQuery cannot be called again (on the same connection) until PQgetResult has returned a null pointer, indicating that the command is done.

Similarly, in single-row mode, the existing API specifies that you should call PQgetResult() until it returns NULL.

Also, IIRC the protocol already permits multiple result sets to be returned, and the caller cannot safely assume that a single PQsendQuery() will produce only a single result set. (I really should write a test extension that exercises that and how libpq reacts to it.)

That's why I went for the NULL return. I think. It's been a while now so it's a bit fuzzy.

I would definitely like an API that does not rely on testing for a NULL return. Especially since NULL return can be ambiguous in the context of row-at-a-time mode. New explicit enumerations for PGresult would make a lot more sense.

So +1 from me for the general idea. I need to look at the patch as it has evolved soon too.

Remember that the original patch I submitted for this was a 1-day weekend hack and proof of concept to show that libpq could be modified to support query pipelining (and thus batching too), so I could illustrate the performance benefits that could be attained by doing so. I'd been aware of the benefits and the protocol's ability to support it for some time thanks to working on PgJDBC, but couldn't get anyone interested without some C code to demonstrate it, so I wrote some. I am not going to argue that the API I added for it is ideal in any way, and happy to see improvements.

The only change I would very strongly object to would be anything that turned this into a *batch* mode without query-pipelining support. If you have to queue all the queries up in advance then send them as a batch and wait for all the results, you miss out on a lot of the potential round-trip-time optimisations and you add initial latency. So long as there is a way to "send A", "send B", "send C", "read results from A", "send D", and there's a way for the application to associate some kind of state (an application specific id or index, a pointer to an application query-queue struct, whatever)  so it can match queries to results ... then I'm happy.
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Craig Ringer-5
On Tue, 16 Feb 2021 at 09:19, Craig Ringer <[hidden email]> wrote:
So long as there is a way to "send A", "send B", "send C", "read results from A", "send D", and there's a way for the application to associate some kind of state (an application specific id or index, a pointer to an application query-queue struct, whatever)  so it can match queries to results ... then I'm happy.

FWIW I'm also thinking of revising the docs to mostly use the term "pipeline" instead of "batch". Use "pipelining and batching" in the chapter topic, and mention "batch" in the index, and add a para that explains how to run batches on top of pipelining, but otherwise use the term "pipeline" not "batch".

That's because pipelining isn't actually batching, and using it as a naïve batch interface will get you in trouble. If you PQsendQuery(...) a long list of queries without consuming results, you'll block unless you're in libpq's nonblocking-send mode. You'll then be deadlocked because the server can't send results until you consume some (tx buffer full) and can't consume queries until it can send some results, but you can't consume results because you're blocked on a send that'll never end.

An actual batch interface where you can bind and send a long list of queries might be worthwhile, but should be addressed separately, and it'd be confusing if this pipelining interface claimed the term "batch". I'm not convinced enough application developers actually code directly against libpq for it to be worth creating a pretty batch interface where you can submit (say) an array of struct PQbatchEntry { const char * query, int nparams, ... } to a PQsendBatch(...) and let libpq handle the socket I/O for you. But if someone wants to add one later it'll be easier if we don't use the term "batch" for the pipelining feature.

I'll submit a reworded patch in a bit.
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
On 2021-Feb-16, Craig Ringer wrote:

> FWIW I'm also thinking of revising the docs to mostly use the term
> "pipeline" instead of "batch". Use "pipelining and batching" in the chapter
> topic, and mention "batch" in the index, and add a para that explains how
> to run batches on top of pipelining, but otherwise use the term "pipeline"
> not "batch".

Hmm, this is a good point.  It means I have a lot of API renaming to do.
I'll get on it now and come back with a proposal.

--
Álvaro Herrera       Valdivia, Chile


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
Here's a new version, where I've renamed everything to "pipeline".  I
think the docs could use some additional tweaks now in order to make a
coherent story on pipeline mode, how it can be used in a batched
fashion, etc.

Here's the renames I applied.  It's mostly mechanical, except
PQbatchSendQueue is now PQsendPipeline:

        PQBatchStatus -> PGpipelineStatus (enum)
        PQBATCH_MODE_OFF -> PQ_PIPELINE_OFF
        PQBATCH_MODE_ON -> PQ_PIPELINE_ON
        PQBATCH_MODE_ABORTED -> PQ_PIPELINE_ABORTED
        PQbatchStatus -> PQpipelineStatus (function)
        PQenterBatchMode -> PQenterPipelineMode
        PQexitBatchMode -> PQexitPipelineMode
        PQbatchSendQueue -> PQsendPipeline
        PGRES_BATCH_END -> PGRES_PIPELINE_END
        PGRES_BATCH_ABORTED -> PGRES_PIPELINE_ABORTED

Also, PQbatchStatus(conn) returns enum PGpipelineStatus (it previously
returned int).

I'm tempted to rename PGASYNC_QUEUED to PGASYNC_PIPELINE_IDLE (not sure
if PGASYNC_PIPELINE_READY fits better with the existing one).


In pgbench, I changed the metacommands to be \startpipeline and
\endpipeline.  There's a failing Assert() there which I commented out;
needs fixed.

--
Álvaro Herrera                            39°49'30"S 73°17'W

v27-libpq-pipeline.patch (98K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Zhihong Yu
Hi,
+       if (querymode == QUERY_SIMPLE)
+       {
+           commandFailed(st, "startpipeline", "cannot use pipeline mode with the simple query protocol");
+           st->state = CSTATE_ABORTED;
+           return CSTATE_ABORTED;

I wonder why the st->state is only assigned for this if block. The state is not set for other cases where CSTATE_ABORTED is returned.

Should PQ_PIPELINE_OFF be returned for the following case ?

+PQpipelineStatus(const PGconn *conn)
+{
+   if (!conn)
+       return false;

Cheers

On Tue, Feb 16, 2021 at 3:14 PM Alvaro Herrera <[hidden email]> wrote:
Here's a new version, where I've renamed everything to "pipeline".  I
think the docs could use some additional tweaks now in order to make a
coherent story on pipeline mode, how it can be used in a batched
fashion, etc.

Here's the renames I applied.  It's mostly mechanical, except
PQbatchSendQueue is now PQsendPipeline:

        PQBatchStatus -> PGpipelineStatus (enum)
        PQBATCH_MODE_OFF -> PQ_PIPELINE_OFF
        PQBATCH_MODE_ON -> PQ_PIPELINE_ON
        PQBATCH_MODE_ABORTED -> PQ_PIPELINE_ABORTED
        PQbatchStatus -> PQpipelineStatus (function)
        PQenterBatchMode -> PQenterPipelineMode
        PQexitBatchMode -> PQexitPipelineMode
        PQbatchSendQueue -> PQsendPipeline
        PGRES_BATCH_END -> PGRES_PIPELINE_END
        PGRES_BATCH_ABORTED -> PGRES_PIPELINE_ABORTED

Also, PQbatchStatus(conn) returns enum PGpipelineStatus (it previously
returned int).

I'm tempted to rename PGASYNC_QUEUED to PGASYNC_PIPELINE_IDLE (not sure
if PGASYNC_PIPELINE_READY fits better with the existing one).


In pgbench, I changed the metacommands to be \startpipeline and
\endpipeline.  There's a failing Assert() there which I commented out;
needs fixed.

--
Álvaro Herrera                            39°49'30"S 73°17'W
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
Hello, thanks for looking at this patch.

On 2021-Feb-16, Zhihong Yu wrote:

> +       if (querymode == QUERY_SIMPLE)
> +       {
> +           commandFailed(st, "startpipeline", "cannot use pipeline mode
> with the simple query protocol");
> +           st->state = CSTATE_ABORTED;
> +           return CSTATE_ABORTED;
>
> I wonder why the st->state is only assigned for this if block. The state is
> not set for other cases where CSTATE_ABORTED is returned.

Yeah, that's a simple oversight.  We don't need to set st->state,
because the caller sets it to the value we return.

> Should PQ_PIPELINE_OFF be returned for the following case ?
>
> +PQpipelineStatus(const PGconn *conn)
> +{
> +   if (!conn)
> +       return false;

Yep.

Thanks

--
Álvaro Herrera                            39°49'30"S 73°17'W


123