Re: PATCH: Batch/pipelining support for libpq

On 23 August 2016 at 08:27, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> On 10 August 2016 at 14:44, Michael Paquier <michael(dot)paquier(at)gmail(dot)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.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services