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

On Fri, Feb 17, 2017 at 2:17 PM, Prabakaran, Vaishnavi
<VaishnaviP@fast.au.fujitsu.com> wrote:
> On 22 November 2016 at 18:32, Craig Ringer<craig@2ndquadrant.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.

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>.

+   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.

+/* 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.

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.

+   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.

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

-                       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.

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/

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.