Re: PATCH: Batch/pipelining support for libpq

On Tue, Feb 21, 2017 at 6:51 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Fri, Feb 17, 2017 at 2:17 PM, Prabakaran, Vaishnavi
> <VaishnaviP(at)fast(dot)au(dot)fujitsu(dot)com> wrote:
> > On 22 November 2016 at 18:32, Craig Ringer<craig(at)2ndquadrant(dot)com> wrote:
> > I am interested in this patch and addressed various below comments from
> reviewers. And, I have separated out code and test module into 2 patches.
> So, If needed, test patch can be enhanced more, meanwhile code patch can be
> committed.
>
> Cool. Nice to see a new version of this patch.
> (You may want to your replies breath a bit, for example by adding an
> extra newline to the paragraph you are answering to.)
>
> >>Renaming and refactoring new APIs
> >> +PQisInBatchMode 172
> >>+PQqueriesInBatch 173
> >>+PQbeginBatchMode 174
> >>+PQendBatchMode 175
> >>+PQsendEndBatch 176
> >>+PQgetNextQuery 177
> >>+PQbatchIsAborted 178
> >>This set of routines is a bit inconsistent. Why not just prefixing them
> with PQbatch? Like that for example:
> >> PQbatchStatus(): consists of disabled/inactive/none, active, error.
> This covers both PQbatchIsAborted() and PQisInBatchMode().
> >>PQbatchBegin()
> >>PQbatchEnd()
> >>PQbatchQueueSync() or PQbatchQueueFlush, same as PQsendEndBatch() to add
> and process a sync message into the queue.
> >
> > Renamed and modified batch status APIs as below
> > PQisInBatchMode & PQbatchIsAborted ==> PQbatchStatus
> > PQqueriesInBatch ==> PQbatchQueueCount
> > PQbeginBatchMode ==> PQbatchBegin
> > PQendBatchMode ==> PQbatchEnd
> > PQsendEndBatch ==> PQbatchQueueSync
> > PQgetNextQuery ==> PQbatchQueueProcess
>
> Thanks. This seems a way cleaner interface to me. Others may have a
> different opinion so let's see if there are some.

>
>>PQbatchQueueCount(): returns N>0 if there are N entries, 0 if empty,-1 on
> failure
> >>PQbatchQueueProcess(): returns 1 if process can begin, 0 if not, -1 on
> failure (OOM)
> >
> > I think it is still ok to keep the current behaviour like other ones
> present in the same file. E.g:"PQsendPrepare" "PQsendQueryGuts"
> >
> >>PQqueriesInBatch() (Newname(NN):PQbatchQueueCount)doesn't work as
> documented.
> >>It says:
> >>"Returns the number of queries still in the queue for this batch"
> >>but in fact it's implemented as a Boolean.
> >
> > Modified the logic to count number of entries in pending queue and
> return the count
> >
> >>The changes in src/test/examples/ are not necessary anymore. You moved
> all the tests to test_libpq (for the best actually).
> > Removed these unnecessary changes from src/test/examples folder and
> corrected the path mentioned in comments section of testlibpqbatch.c
>
> >>But with the libpq batch API, maybe this could be modernized
> >>with meta-commands like this:
> >>\startbatch
> >>...
> >>\endbatch
> >
> > I think it is a separate patch candidate.
>
> Definitely agreed on that. Let's not complicate things further more.
>
> >> It may be a good idea to check for PG_PROTOCOL_MAJOR < 3 and issue an
> error for the new routines.
> >
> > All the APIs which supports asynchronous command processing can be
> executed only if PG_PROTOCOL_MAJOR >= 3. So, adding it to new routines are
> not really needed.
>
> OK. Let's not complicate the patch more than necessary.
>
> After an extra lookup at the patch, here are some comments.
>

Thanks for reviewing the patch.

>
> In the docs when listing any of the PQBATCH_MODE_* variables, you
> should have a markup <literal> around them. It would be cleaner to
> make a list of the potential values that can be returned by
> PQbatchStatus() using <variablelist>.
>

Modified the format of PQbatchStatus() and other batch APIs too in
documentation along with addition of <literal> tags wherever required.

>
> + while (queue != NULL)
> + {
> + PGcommandQueueEntry *prev = queue;
> +
> + queue = queue->next;
> + if (prev->query)
> + free(prev->query);
> + free(prev);
> + }
> + conn->cmd_queue_recycle = NULL
> This could be useful as a separate routine.
>

Ok, Moved this code to new function - PQfreeCommandQueue().

>
> +/* PQmakePipelinedCommand
> + * Get a new command queue entry, allocating it if required. Doesn't add
> it to
> + * the tail of the queue yet, use PQappendPipelinedCommand once the
> command has
> + * been written for that. If a command fails once it's called this, it
> should
> + * use PQrecyclePipelinedCommand to put it on the freelist or release it.
> + *
> This comment block is not project-like. Please use an empty line as
> first line with only "/*". The same comment applies to a bunch of the
> routines introduced by this patch.
>

Corrected this.

>
> Not sure there is much point in having PQstartProcessingNewQuery. It
> only does two things in two places, so that's not worth the loss in
> readability.
>

Yes, removed this function and placed those two things in line.

>
> + if (conn->batch_status != PQBATCH_MODE_OFF)
> + {
> + pipeCmd = PQmakePipelinedCommand(conn);
> +
> + if (pipeCmd == NULL)
> + return 0; /* error msg already set */
> +
> + last_query = &pipeCmd->query;
> + queryclass = &pipeCmd->queryclass;
> + }
> + else
> + {
> + last_query = &conn->last_query;
> + queryclass = &conn->queryclass;
> + }
> This setup should happen further down.
>

Moved it down post other validations in this function.

>
> conn->in_batch is undefined, causing the patch to fail to compile. And
> actually you should not need it.
>

That is an update missed in my previous patch, corrected in the new patch.

>
> - conn->asyncStatus = PGASYNC_READY;
> + conn->asyncStatus = PGASYNC_READY_MORE;
> return;
> This should really not be changed, or it should be changed to a status
> dedicated to batching only if batch mode is activated.
>

Since pqParseInput3() reads all the input data post "Row description"
message, yes, this change is not needed here.

>
> If you are planning for integrating this patch into Postres 10, please
> be sure that this is registered in the last commit fest that will
> begin next week:
> https://commitfest.postgresql.org/13/

Yes, I have created a new patch entry into the commitfest 2017-03 and
attached the latest patch with this e-mail.

>
> I'll try to book a couple of cycles to look at it if you are able to
> register it into the CF and provide a new version.
>

Thanks again.

Regards,
Vaishnavi,
Fujitsu Australia.