Re: PATCH: Batch/pipelining support for libpq - Mailing list pgsql-hackers
| From | Vaishnavi Prabakaran |
|---|---|
| Subject | Re: PATCH: Batch/pipelining support for libpq |
| Date | |
| Msg-id | CAOoUkxRe_dMc7WPTfnEbcHPU0rh6rs-JmdEOZiKvUYvG5v8_yw@mail.gmail.com Whole thread Raw |
| In response to | PATCH: Batch/pipelining support for libpq (Craig Ringer <craig@2ndquadrant.com>) |
| Responses |
Re: PATCH: Batch/pipelining support for libpq
(Michael Paquier <michael.paquier@gmail.com>)
|
| List | pgsql-hackers |
Michael Paquier wrote:
>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@manitou-mail.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.
Attachment
pgsql-hackers by date: