Re: [HACKERS] PATCH: Batch/pipelining support for libpq

From: Andres Freund <andres(at)anarazel(dot)de>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Nikhil Sontakke <nikhils(at)2ndquadrant(dot)com>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, vaishnaviprabakaran(at)gmail(dot)com, Craig Ringer <craig(at)2ndquadrant(dot)com>, daniel(at)manitou-mail(dot)org, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Daniel Gustafsson <daniel(at)yesql(dot)se>, David Steele <david(at)pgmasters(dot)net>, "Prabakaran, Vaishnavi" <VaishnaviP(at)fast(dot)au(dot)fujitsu(dot)com>, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>, "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>, dmitigr(at)gmail(dot)com, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, m(dot)kniep(at)web(dot)de, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>, iwata(dot)aya(at)jp(dot)fujitsu(dot)com, Michael Paquier <michael(at)paquier(dot)xyz>
Subject: Re: [HACKERS] PATCH: Batch/pipelining support for libpq
Date: 2020-07-14 19:18:01
Message-ID: 20200714191801.bhxeug2hpxs4sksi@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On 2020-07-10 19:01:49 -0400, Alvaro Herrera wrote:
> Totally unasked for, here's a rebase of this patch series. I didn't do
> anything other than rebasing to current master, solving a couple of very
> trivial conflicts, fixing some whitespace complaints by git apply, and
> running tests to verify everthing works.
>
> I don't foresee working on this at all, so if anyone is interested in
> seeing this feature in, I encourage them to read and address
> Horiguchi-san's feedback.

Nor am I planning to do so, but I do think its a pretty important
improvement.

> +/*
> + * PQrecyclePipelinedCommand
> + * Push a command queue entry onto the freelist. It must be a dangling entry
> + * with null next pointer and not referenced by any other entry's next pointer.
> + */

Dangling sounds a bit like it's already freed.

> +/*
> + * PQbatchSendQueue
> + * End a batch submission by sending a protocol sync. The connection will
> + * remain in batch mode and unavailable for new synchronous command execution
> + * functions until all results from the batch are processed by the client.

I feel like the reference to the protocol sync is a bit too low level
for an external API. It should first document what the function does
from a user's POV.

I think it'd also be good to document whether / whether not queries can
already have been sent before PQbatchSendQueue is called or not.

> +/*
> + * PQbatchProcessQueue
> + * In batch mode, start processing the next query in the queue.
> + *
> + * Returns 1 if the next query was popped from the queue and can
> + * be processed by PQconsumeInput, PQgetResult, etc.
> + *
> + * Returns 0 if the current query isn't done yet, the connection
> + * is not in a batch, or there are no more queries to process.
> + */
> +int
> +PQbatchProcessQueue(PGconn *conn)
> +{
> + PGcommandQueueEntry *next_query;
> +
> + if (!conn)
> + return 0;
> +
> + if (conn->batch_status == PQBATCH_MODE_OFF)
> + return 0;
> +
> + switch (conn->asyncStatus)
> + {
> + case PGASYNC_COPY_IN:
> + case PGASYNC_COPY_OUT:
> + case PGASYNC_COPY_BOTH:
> + printfPQExpBuffer(&conn->errorMessage,
> + libpq_gettext_noop("internal error, COPY in batch mode"));
> + break;

Shouldn't there be a return 0 here?

> + if (conn->batch_status == PQBATCH_MODE_ABORTED && conn->queryclass != PGQUERY_SYNC)
> + {
> + /*
> + * In an aborted batch we don't get anything from the server for each
> + * result; we're just discarding input until we get to the next sync
> + * from the server. The client needs to know its queries got aborted
> + * so we create a fake PGresult to return immediately from
> + * PQgetResult.
> + */
> + conn->result = PQmakeEmptyPGresult(conn,
> + PGRES_BATCH_ABORTED);
> + if (!conn->result)
> + {
> + printfPQExpBuffer(&conn->errorMessage,
> + libpq_gettext("out of memory"));
> + pqSaveErrorResult(conn);
> + return 0;

Is there any way an application can recover at this point? ISTM we'd be
stuck in the previous asyncStatus, no?

> +/* pqBatchFlush
> + * In batch mode, data will be flushed only when the out buffer reaches the threshold value.
> + * In non-batch mode, data will be flushed all the time.
> + */
> +static int
> +pqBatchFlush(PGconn *conn)
> +{
> + if ((conn->batch_status == PQBATCH_MODE_OFF)||(conn->outCount >= OUTBUFFER_THRESHOLD))
> + return(pqFlush(conn));
> + return 0; /* Just to keep compiler quiet */
> +}

unnecessarily long line.

> +/*
> + * Connection's outbuffer threshold is set to 64k as it is safe
> + * in Windows as per comments in pqSendSome() API.
> + */
> +#define OUTBUFFER_THRESHOLD 65536

I don't think the comment explains much. It's fine to send more than 64k
with pqSendSome(), they'll just be send with separate pgsecure_write()
invocations. And only on windows.

It clearly makes sense to start sending out data at a certain
granularity to avoid needing unnecessary amounts of memory, and to make
more efficient use of latency / serer side compute.

It's not implausible that 64k is the right amount for that, I just don't
think the explanation above is good.

> diff --git a/src/test/modules/test_libpq/testlibpqbatch.c b/src/test/modules/test_libpq/testlibpqbatch.c
> new file mode 100644
> index 0000000000..4d6ba266e5
> --- /dev/null
> +++ b/src/test/modules/test_libpq/testlibpqbatch.c
> @@ -0,0 +1,1456 @@
> +/*
> + * src/test/modules/test_libpq/testlibpqbatch.c
> + *
> + *
> + * testlibpqbatch.c
> + * Test of batch execution functionality
> + */
> +
> +#ifdef WIN32
> +#include <windows.h>
> +#endif

ISTM that this shouldn't be needed in a test program like this?
Shouldn't libpq abstract all of this away?

> +static void
> +simple_batch(PGconn *conn)
> +{

ISTM that all or at least several of these should include tests of
transactional behaviour with pipelining (e.g. using both implicit and
explicit transactions inside a single batch, using transactions across
batches, multiple explicit transactions inside a batch).

> + PGresult *res = NULL;
> + const char *dummy_params[1] = {"1"};
> + Oid dummy_param_oids[1] = {INT4OID};
> +
> + fprintf(stderr, "simple batch... ");
> + fflush(stderr);

Why do we need fflush()? IMO that shouldn't be needed in a use like
this? Especially not on stderr, which ought to be unbuffered?

> + /*
> + * Enter batch mode and dispatch a set of operations, which we'll then
> + * process the results of as they come in.
> + *
> + * For a simple case we should be able to do this without interim
> + * processing of results since our out buffer will give us enough slush to
> + * work with and we won't block on sending. So blocking mode is fine.
> + */
> + if (PQisnonblocking(conn))
> + {
> + fprintf(stderr, "Expected blocking connection mode\n");
> + goto fail;
> + }

Perhaps worth adding a helper for this?

#define EXPECT(condition, explanation) \
...
or such?

Greetings,

Andres Freund

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2020-07-14 19:31:26 Re: recovering from "found xmin ... from before relfrozenxid ..."
Previous Message Peter Eisentraut 2020-07-14 18:58:28 Re: warnings for invalid function casts