Re: PATCH: Batch/pipelining support for libpq

Michael Paquier wrote:

>Could you as well update src/tools/msvc/vcregress.pl, aka the routine
>modulescheck so as this new test is skipped. I am sure that nobody
>will scream if this test is not run on Windows, but the buildfarm will
>complain if the patch is let in its current state.

Thanks for the finding. Hmm, modulescheck will not pick up test_libpq as
"subdircheck" will fail for this module(because it does not have
"sql"/"expected" folders in it) and hence it will be skipped.
But, Modified "Mkvcbuild.pm" to exclude "test_libpq" module, as this test
anyways will not be run in Windows and also because it uses linux specific
APIs(gettimeofday , timersub) .

On Wed, Mar 29, 2017 at 12:28 AM, Daniel Verite <daniel(at)manitou-mail(dot)org>
wrote:

> Michael Paquier wrote:
>
> > # Running: testlibpqbatch dbname=postgres 10000 copyfailure
> > dispatching SELECT query failed: cannot queue commands during COPY
> >
> > COPYBUF: 5
> >
> > Error status and message got from server due to COPY command failure
> > are : PGRES_FATAL_ERROR ERROR: unexpected message type 0x50 during
> > COPY from stdin
> > CONTEXT: COPY batch_demo, line 1
>
> Same result here.
>
> BTW the doc says:
> "In batch mode only asynchronous operations are permitted, and COPY is
> not recommended as it most likely will trigger failure in batch
> processing"
> Yet it seems that the test assumes that it should work nonetheless.
> I don't quite understand what we expect of this test, given what's
> documented. Or what am I missing?
>

copyfailure test is basically tests the error scenarios arising in batch
mode on using copy command. And, test expects error(not that it should
work) and the test is made to pass if expected error message is received.
So, it is error case testing behaves like:
expects error -> receives error = test pass
expects error -> no error = test fail

There are 2 cases tested here:

1. Example for the case - Using COPY command in batch mode will trigger
failure. (Specified in documentation)
Failure can be seen only when processing the copy command's results. That
is, after dispatching COPY command, server goes into COPY state , but the
client dispatched another query in batch mode.

Below error messages belongs to this case :
Error status and message got from server due to COPY command failure are :
PGRES_FATAL_ERROR ERROR: unexpected message type 0x50 during COPY from
stdin
CONTEXT: COPY batch_demo, line 1

message type 0x5a arrived from server while idle

2. When connection is is copy state, that is, while processing copy
command's result, Cannot queue any query in batch mode.

Below error message belongs to this case:
dispatching SELECT query failed: cannot queue commands during COPY

I added this test following Michael's review comment - "It may be a good
idea to add a test for COPY and trigger a failure."
Hope this clarifies the presence of error message in log file and why the
test is made to pass.

>
> While looking at the regress log, I noticed multiple spurious
> test_singlerowmode tests among the others. Should be fixed with:
>
> --- a/src/test/modules/test_libpq/testlibpqbatch.c
> +++ b/src/test/modules/test_libpq/testlibpqbatch.c
> @@ -1578,6 +1578,7 @@ main(int argc, char **argv)
> run_batch_abort = 0;
> run_timings = 0;
> run_copyfailure = 0;
> + run_singlerowmode = 0;
> if (strcmp(argv[3], "disallowed_in_batch") == 0)
> run_disallowed_in_batch = 1;
> else if (strcmp(argv[3], "simple_batch") == 0)
>

Thanks for finding it out and yes corrected.

> There's also the fact that this test doesn't care whether it fails
> (compare with all the "goto fail" of the other tests).
>

Modified the test to check for failure condition.

>
> And it happens that PQsetSingleRowMode() fails after the call to
> PQbatchQueueProcess() that instantiates a PGresult
> of status PGRES_BATCH_END to indicate the end of the batch,
>
> To me this shows how this way of setting the single row mode is not ideal.
> In order to avoid the failure, the loop should know in advance
> what kind of result it's going to dequeue before calling PQgetResult(),
> which doesn't feel right.
>
>
Yes, it is one of the requirement that the batch mode application should
know the order of queries it sent and the expected results. (Specified in
"Processing results" section of batch mode documentation)

Attached the latest code and test patches.

Thanks & Regards,
Vaishnavi
Fujitusu Australia.