Re: PATCH: Batch/pipelining support for libpq - Mailing list pgsql-hackers
| From | Craig Ringer |
|---|---|
| Subject | Re: PATCH: Batch/pipelining support for libpq |
| Date | |
| Msg-id | CAMsr+YF5KKoEQOfqGdhvX0XLRaXz8vtS=yV7xoPFw+=YmDTwoA@mail.gmail.com Whole thread Raw |
| In response to | Re: PATCH: Batch/pipelining support for libpq (Craig Ringer <craig@2ndquadrant.com>) |
| Responses |
Re: PATCH: Batch/pipelining support for libpq
("Daniel Verite" <daniel@manitou-mail.org>)
|
| List | pgsql-hackers |
On 23 August 2016 at 08:27, Craig Ringer <craig@2ndquadrant.com> wrote:
On 10 August 2016 at 14:44, Michael Paquier <michael.paquier@gmail.com> wrote:
I am looking a bit more seriously at this patch and assigned myself as
a reviewer.Much appreciated.
macos complains here. You may want to replace %06lds by just %06d.Yeah, or cast to a type known to be big enough. Will amend.
I used an upcast to (long), because on Linux it's a long. I don't see the point of messing about adding a configure test for something this trivial.
This patch generates a core dump, use for example pg_ctl start -w and
you'll bump into the trace above. There is something wrong with the
queue handling.Huh. I didn't see that here (Fedora 23). I'll look more closely.Do you have plans for a more generic structure for the command queue list?No plans, no. This was a weekend experiment that turned into a useful patch and I'm having to scrape up time for it amongst much more important things like logical failover / sequence decoding and various other replication work.Thanks for the docs review too, will amend.+ fprintf(stderr, "internal error, COPY in batch mode");
+ abort();
I don't think that's a good idea.
My thinking there was that it's a "shouldn't happen" case. It's a problem in library logic. I'd use an Assert() here in the backend.
I could printfPQExpBuffer(...) an error and return failure instead if you think that's more appropriate. I'm not sure how the app would handle it correctly, but OTOH it's generally better for libraries not to call abort(). So I'll do that, but since it's an internal error that's not meant to happen I'll skip the gettext calls.
Error messages should also use libpq_gettext, and perhaps
be stored in conn->errorMessage as we do so for OOMs happening on
client-side and reporting them back even if they are not expected
(those are blocked PQsendQueryStart in your patch).
I didn't get that last part, re PQsendQueryStart.
src/test/examples is a good idea to show people what this new API can
do, but this is never getting compiled. It could as well be possible
to include tests in src/test/modules/, in the same shape as what
postgres_fdw is doing by connecting to itself and link it to libpq. As
this patch complicates quote a lot fe-exec.c, I think that this would
be worth it. Thoughts?
I think it makes sense to use the TAP framework. Added src/test/modules/test_libpq/ with a test for async mode. Others can be added/migrated based on that. I thought it made more sense for the tests to live there than in src/interfaces//libpq/ since they need test client programs and shouldn't pollute the library directory.
I've made the docs changes too. Thanks.
I fixed the list handling error. I'm amazed it appears to run fine, and without complaint from valgrind, here, since it was an accidentally _deleted_ line.
Re lists, I looked at simple_list.c and it's exceedingly primitive. Using it would mean more malloc()ing since we'll have a list cell and then a struct pointed to it, and won't recycle members, but... whatever. It's not going to matter a great deal. The reason I did it with an embedded list originally was because that's how it's done for PGnotify, but that's not exactly new code
The bigger problem is that simple_list also uses pg_malloc, which won't set conn->errorMessage, it'll just fprintf() and exit(1). I'm not convinced it's appropriate to use that for libpq.
For now I've left list handling unchanged. If it's to move to a generic list, it should probably be one that knows how to use pg_malloc_extended(size, MCXT_ALLOC_NO_OOM) and emit its own libpq-error-handling-aware error. I'm not sure whether that list should use cell heads embedded in the structures it manages or pointing to them, either.
Updated patch attached.
Attachment
pgsql-hackers by date: