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

Álvaro Herrera
On 2021-Jan-21, Zhihong Yu wrote:

> 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) ?

No -- process_backslash_command reads the word and sets it as argv[0].

> +               appendPQExpBufferStr(&conn->errorMessage,
> +                                 libpq_gettext("cannot queue commands
> during COPY\n"));
> +               return false;
> +               break;
>
> Is the break necessary ? Similar comment for pqBatchProcessQueue().

Not necessary.  I removed them.

> +int
> +PQexitBatchMode(PGconn *conn)
>
> Since either 0 or 1 is returned, maybe change the return type to bool.

I was kinda tempted to do that, but in reality a lot of libpq's API is
defined like that -- to return 0 (failure)/1 (success) as ints, not
bools.  For example see PQsendQuery().  I'm not inclined to do different
for these new functions.  (One curious case is PQsetvalue, which returns
int, and is documented as "returns zero for failure" and then uses
"return false").

> Also, the function operates on PGconn - should the function be static
> (pqBatchProcessQueue is) ?

I don't understand this suggestion.  How would an application call it,
if we make it static?

Thanks

--
Álvaro Herrera       Valdivia, Chile
"Cómo ponemos nuestros dedos en la arcilla del otro. Eso es la amistad; jugar
al alfarero y ver qué formas se pueden sacar del otro" (C. Halloway en
La Feria de las Tinieblas, R. Bradbury)


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Zhihong Yu
Hi,
+static int pqBatchProcessQueue(PGconn *conn);

I was suggesting changing:

+int
+PQexitBatchMode(PGconn *conn)

to:

+static int
+PQexitBatchMode(PGconn *conn)

Cheers

On Fri, Feb 19, 2021 at 10:43 AM Alvaro Herrera <[hidden email]> wrote:
On 2021-Jan-21, Zhihong Yu wrote:

> 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) ?

No -- process_backslash_command reads the word and sets it as argv[0].

> +               appendPQExpBufferStr(&conn->errorMessage,
> +                                 libpq_gettext("cannot queue commands
> during COPY\n"));
> +               return false;
> +               break;
>
> Is the break necessary ? Similar comment for pqBatchProcessQueue().

Not necessary.  I removed them.

> +int
> +PQexitBatchMode(PGconn *conn)
>
> Since either 0 or 1 is returned, maybe change the return type to bool.

I was kinda tempted to do that, but in reality a lot of libpq's API is
defined like that -- to return 0 (failure)/1 (success) as ints, not
bools.  For example see PQsendQuery().  I'm not inclined to do different
for these new functions.  (One curious case is PQsetvalue, which returns
int, and is documented as "returns zero for failure" and then uses
"return false").

> Also, the function operates on PGconn - should the function be static
> (pqBatchProcessQueue is) ?

I don't understand this suggestion.  How would an application call it,
if we make it static?

Thanks

--
Álvaro Herrera       Valdivia, Chile
"Cómo ponemos nuestros dedos en la arcilla del otro. Eso es la amistad; jugar
al alfarero y ver qué formas se pueden sacar del otro" (C. Halloway en
La Feria de las Tinieblas, R. Bradbury)
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
Hi,

On 2021-Feb-19, Zhihong Yu wrote:

> Hi,
> +static int pqBatchProcessQueue(PGconn *conn);
>
> I was suggesting changing:
>
> +int
> +PQexitBatchMode(PGconn *conn)
>
> to:
>
> +static int
> +PQexitBatchMode(PGconn *conn)

I understand that, but what I'm saying is that it doesn't work.
pqBatchProcessQueue can be static because it's only used internally in
libpq, but PQexitBatchMode is supposed to be called by the client
application.

--
Álvaro Herrera       Valdivia, Chile
"No necesitamos banderas
 No reconocemos fronteras"                  (Jorge González)


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Zhihong Yu
Hi,
Thanks for the response.

On Fri, Feb 19, 2021 at 12:46 PM Alvaro Herrera <[hidden email]> wrote:
Hi,

On 2021-Feb-19, Zhihong Yu wrote:

> Hi,
> +static int pqBatchProcessQueue(PGconn *conn);
>
> I was suggesting changing:
>
> +int
> +PQexitBatchMode(PGconn *conn)
>
> to:
>
> +static int
> +PQexitBatchMode(PGconn *conn)

I understand that, but what I'm saying is that it doesn't work.
pqBatchProcessQueue can be static because it's only used internally in
libpq, but PQexitBatchMode is supposed to be called by the client
application.

--
Álvaro Herrera       Valdivia, Chile
"No necesitamos banderas
 No reconocemos fronteras"                  (Jorge González)
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Craig Ringer-5
In reply to this post by Álvaro Herrera


On Wed, 17 Feb 2021, 07:13 Alvaro Herrera, <[hidden email]> wrote:
Here's a new version, where I've renamed everything to "pipeline". 

Wow. Thanks.

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.

I'll do that soon and send an update.

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

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

> On Wed, 17 Feb 2021, 07:13 Alvaro Herrera, <[hidden email]> wrote:

> > 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.
>
> I'll do that soon and send an update.

I started to do that, but was sidetracked having a look at error
handling while checking one of the claims.

So now it starts with this:

   <sect3 id="libpq-pipeline-sending">
    <title>Issuing Queries</title>

    <para>
     After entering pipeline mode, the application dispatches requests using
     <xref linkend="libpq-PQsendQueryParams"/>,
     or its prepared-query sibling
     <xref linkend="libpq-PQsendQueryPrepared"/>.
     These requests are queued on the client-side until flushed to the server;
     this occurs when <xref linkend="libpq-PQsendPipeline"/> is used to
     establish a synchronization point in the pipeline,
     or when <xref linkend="libpq-PQflush"/> is called.
     The functions <xref linkend="libpq-PQsendPrepare"/>,
     <xref linkend="libpq-PQsendDescribePrepared"/>, and
     <xref linkend="libpq-PQsendDescribePortal"/> also work in pipeline mode.
     Result processing is described below.
    </para>

    <para>
     The server executes statements, and returns results, in the order the
     client sends them.  The server will begin executing the commands in the
     pipeline immediately, not waiting for the end of the pipeline.
     If any statement encounters an error, the server aborts the current
     transaction and skips processing commands in the pipeline until the
     next synchronization point established by <function>PQsendPipeline</function>.
     (This remains true even if the commands in the pipeline would rollback
     the transaction.)
     Query processing resumes after the synchronization point.
    </para>

(Note I changed the wording that "the pipeline is ended by
PQsendPipeline" to "a synchronization point is established".  Is this
easily understandable?  On re-reading it, I'm not sure it's really an
improvement.)

BTW we don't explain why doesn't PQsendQuery work (to wit: because it
uses "simple" query protocol and thus doesn't require the Sync message).
I think we should either document that, or change things so that
PQsendQuery no longer uses a 'Q' message when in pipeline mode; instead
use extended query protocol.  I don't see why we'd force people to use
PQsendQueryParams when not necessary.

BTW, in my experimentation, the sequence of PGresult that you get when
handling a pipeline with errors don't make a lot of sense.  I'll spend
some more time on it.

While at it, as for the PGresult sequence and NULL returns: I think for
PQexec and maybe PQsendQuery, it makes sense to loop until PQgetResult
returns NULL, because you never know how many results are you going to
get.  But for PQsendQueryParams, this is no longer true, because you
can't send multiple queries in one query string.  So the only way to get
multiple results, is by using single-row mode.  But that already has its
own protocol for multiple results, namely to get a stream of
PGRES_SINGLE_TUPLE terminated by a zero-rows PGRES_TUPLES_OK.  So I'm
not sure there is a strong need for the mandatory NULL result.

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


Reply | Threaded
Open this post in threaded view
|

RE: PATCH: Batch/pipelining support for libpq

k.jamison@fujitsu.com
In reply to this post by Álvaro Herrera
On Wed, Feb 17, 2021 8:14 AM (JST), Alvaro Herrera wrote:

Hi Alvaro,

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

I tried applying this patch to test it on top of Iwata-san's libpq trace log [1].
In my environment, the compiler complains.
It seems that in libpqwalreceiver.c: libpqrcv_exec()
the switch for PQresultStatus needs to handle the
cases for PGRES_PIPELINE_END and PGRES_PIPELINE_ABORTED too.

     switch (PQresultStatus(pgres))
     {
         ...
         Case PGRES_PIPELINE_ABORTED:
      ...
         Case PGRES_PIPELINE_ END:

Regards,
Kirk Jamison

[1] https://www.postgresql.org/message-id/flat/1d650278-552a-4bd2-8411-a907fd54446c%40www.fastmail.com#32df02a4e2a08185508a2126e6e0caf1


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
On 2021-Mar-03, [hidden email] wrote:

> I tried applying this patch to test it on top of Iwata-san's libpq trace log [1].
> In my environment, the compiler complains.
> It seems that in libpqwalreceiver.c: libpqrcv_exec()
> the switch for PQresultStatus needs to handle the
> cases for PGRES_PIPELINE_END and PGRES_PIPELINE_ABORTED too.

Yeah, I noticed this too and decided to handle these cases as errors.

Here's v28, which includes that fix, and also gets rid of the
PGASYNC_QUEUED state; I instead made the rest of the code work using the
existing PGASYNC_IDLE state.  This allows us get rid of a few cases
where we had to report "internal error: IDLE state unexpected" in a few
cases, and was rather pointless.  With the current formulation, both
pipeline mode and normal mode share pretty much all state.

Also, I've renamed PGRES_PIPELINE_END to PGRES_PIPELINE_SYNC, because
that's closer to what it means: that result state by no means that
pipeline mode has ended.  It only means that you called PQsendPipeline()
so the server gives you a Sync message.

I'm much more comfortable with this version, so I'm marking the patch as
Ready for Committer in case anybody else wants to look at this before I
push it.

However: on errors, I think it's a bit odd that you get a NULL from
PQgetResult after getting PGRES_PIPELINE_ABORTED.  Maybe I should
suppress that.  I'm no longer wrestling with the NULL after
PGRES_PIPELINE_END however, because that's not critical and you can not
ask for it and things work fine.

(Also, the test pipeline.c program in src/test/modules/test_libpq has a
new "transaction" mode that is not exercised by the TAP program; I'll
plug that in before commit.)

--
Álvaro Herrera       Valdivia, Chile

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

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
On 2021-Mar-03, 'Alvaro Herrera' wrote:

> I'm much more comfortable with this version, so I'm marking the patch as
> Ready for Committer in case anybody else wants to look at this before I
> push it.

Actually, I just noticed a pretty serious problem, which is that when we
report an error, we don't set "conn->errQuery" to the current query but
to the *previous* query, leading to non-sensical error reports like

$ ./pipeline pipeline_abort
aborted pipeline... ERROR:  no existe la función no_such_function(integer)
LINE 1: INSERT INTO pq_pipeline_demo(itemno) VALUES ($1);
               ^
HINT:  Ninguna función coincide en el nombre y tipos de argumentos. Puede ser necesario agregar conversión explícita de tipos.
ok


(Sorry about the Spanish there, but the gist of the problem is that
we're positioning the error cursor on a query that succeeded prior to
the query that raised the error.)

This should obviously not occur.  I'm trying to figure out how to repair
it and not break everything again ...

--
Álvaro Herrera       Valdivia, Chile


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Álvaro Herrera
On 2021-Mar-03, 'Alvaro Herrera' wrote:

> This should obviously not occur.  I'm trying to figure out how to repair
> it and not break everything again ...

I think trying to set up the connection state so that the next query
appears in conn->last_query prior to PQgetResult being called again
leads to worse breakage.  The simplest fix seems to make fe-protocol3.c
aware that in this case, the query is in conn->cmd_queue_head instead,
as in the attached patch.

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

error-fix.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

Justin Pryzby
In reply to this post by Álvaro Herrera
I'm proposing some minor changes.

--
Justin

0002-doc-review.notapxtch (12K) 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 Álvaro Herrera
On 2021-Mar-03, 'Alvaro Herrera' wrote:

> On 2021-Mar-03, 'Alvaro Herrera' wrote:
>
> > This should obviously not occur.  I'm trying to figure out how to repair
> > it and not break everything again ...
>
> I think trying to set up the connection state so that the next query
> appears in conn->last_query prior to PQgetResult being called again
> leads to worse breakage.  The simplest fix seems to make fe-protocol3.c
> aware that in this case, the query is in conn->cmd_queue_head instead,
> as in the attached patch.

I wonder if it would make sense to get rid of conn->last_query
completely and just rely always on conn->cmd_queue_head, where the
normal (non-pipeline) would use the first entry of the command queue.
That might end up being simpler than pipeline mode "pretending" to take
over ->last_query.

--
Álvaro Herrera                            39°49'30"S 73°17'W
"La verdad no siempre es bonita, pero el hambre de ella sí"


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Batch/pipelining support for libpq

David G Johnston
In reply to this post by Justin Pryzby
On Wed, Mar 3, 2021 at 5:45 PM Justin Pryzby <[hidden email]> wrote:
I'm proposing some minor changes.


Some additional tweaks/comments for the documentation with the edit proposed edits:

(edit) +     <function>PQresultStatus</function>, will report a

Remove the comma

(orig) +     the failed operation are skipped entirely. The same behaviour holds if the

We seem to use "behavior" more frequently

(edit) +     From the client perspective, after <function>PQresultStatus</function>

Possessive "client's perspective"

(edit) +     its expected results queue.  Based on available memory, results from the

"its corresponding results queue" - to go along with this change:
-     of the order in which it sent queries and the expected results.
+     of the order in which it sent queries, to associate them with their
+     corresponding results.

(orig)
+       pipeline mode. If the current statement isn't finished processing
+       or there are results pending for collection with

Needs a comma after processing.
"results pending for collection" reads oddly to me...not sure what would be better though...

(edit)
+   <note>
+    <para>
+     The pipeline API was introduced in <productname>PostgreSQL</productname> 14.
+     Pipeline mode is a client-side feature which doesn't require server
+     support, and works on any server that supports the v3 extended query
+     protocol.
+     </para>
+   </note>

This note seems like it should be placed either near the very beginning of the feature or incorporated into the feature introduction.
(orig)
+     If any statement encounters an error, the server aborts the current
+(-)     transaction and skips processing commands in the pipeline until the
+     next synchronization point established by <function>PQsendPipeline</function>.

I dislike "skips" here - even if it doesn't execute a command it still will place a result on the socket so that the client can have something to match up with the queries it sent, correct?

+ transaction and creates a PGRES_PIPELINE_ABORTED result for all commands in the pipeline until the

David J.


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-Mar-03, 'Alvaro Herrera' wrote:

> I wonder if it would make sense to get rid of conn->last_query
> completely and just rely always on conn->cmd_queue_head, where the
> normal (non-pipeline) would use the first entry of the command queue.
> That might end up being simpler than pipeline mode "pretending" to take
> over ->last_query.

I experimented with this today and it appears to be a good change,
though I haven't been able to make everything work correctly yet.

--
Álvaro Herrera       Valdivia, Chile
"Entristecido, Wutra                     (canción de Las Barreras)
echa a Freyr a rodar
y a nosotros al mar"


123