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 | CAOoUkxRvpT7bT5HCs5saXTdAjq-rrnhucfxqxZyCeQstSX_bzg@mail.gmail.com Whole thread Raw |
| In response to | Re: PATCH: Batch/pipelining support for libpq (Michael Paquier <michael.paquier@gmail.com>) |
| List | pgsql-hackers |
On Thu, Mar 30, 2017 at 12:08 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Wed, Mar 29, 2017 at 12:40 PM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:
> 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).
src/port/gettimeofday.c extends things on Windows, and we could
consider to just drop the timing portion for simplicity (except
Windows I am not seeing any platform missing timersub). But that's
just a point of detail. Or we could just drop those tests from the
final patch, and re-implement them after having some psql-level
subcommands. That would far better for portability.
Yes agree, having tests with psql meta commands will be more easy to understand also. And, that is one of the reason I separated the patch into two - code and test . So, yes, we can drop the test patch for now.
> 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
Such messages are really confusing to the user as they refer to the
protocol going out of sync. I would think that something like "cannot
process COPY results during a batch processing" would be cleaner.
Isn't some more error handling necessary in GetCopyStart()?
Hmm, With batch mode, after sending COPY command to server(and server started processing the query and goes into COPY state) , client does not immediately read the result , but it keeps sending other queries to the server. By this time, server already encountered the error scenario(Receiving different message during COPY state) and sent error messages. So, when client starts reading the result, already error messages sent by server are present in socket. I think trying to handle during result processing is more like masking the server's error message with new error message and I think it is not appropriate.
> Attached the latest code and test patches.
For me the test is still broken:
ok 1 - testlibpqbatch disallowed_in_batch
ok 2 - testlibpqbatch simple_batch
ok 3 - testlibpqbatch multi_batch
ok 4 - testlibpqbatch batch_abort
ok 5 - testlibpqbatch timings
not ok 6 - testlibpqbatch copyfailure
Still broken here. I can see that this patch is having enough
safeguards in PQbatchBegin() and PQsendQueryStart(), but this issue is
really pointing out at something...
I will check this one with fresh setup again and I guess that this should not block the code patch.
Thanks & Regards,
Vaishnavi
Fujitsu Australia.
pgsql-hackers by date: