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