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

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