On Wed, Jul 15, 2020 at 12:18 AM Andres Freund <and...@anarazel.de> wrote:
> 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
>
>
>
The build is failing for this patch, can you please take a look at this?
https://cirrus-ci.com/task/4568547922804736
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.129221
I am marking the patch "Waiting on Author"
--
Ibrar Ahmed
