On 10 August 2016 at 14:44, Michael Paquier <michael.paqu...@gmail.com> wrote:
> On Fri, Jun 3, 2016 at 8:51 PM, Dmitry Igrishin <dmit...@gmail.com> wrote: > >> BTW, I've publushed the HTML-ified SGML docs to > >> http://2ndquadrant.github.io/postgres/libpq-batch-mode.html as a > preview. > > Typo detected: "Returns 1 if the batch curently being received" -- > "curently". > > I am looking a bit more seriously at this patch and assigned myself as > a reviewer. > Much appreciated. > testlibpqbatch.c:1239:73: warning: format specifies type 'long' but > the argument has type '__darwin_suseconds_t' (aka 'int') [-Wformat] > printf("batch insert elapsed: %ld.%06lds\n", > elapsed_time.tv_sec, elapsed_time.tv_usec); > 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. > 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. defaultNoticeProcessor can be > overridden to allow applications to have error messages sent > elsewhere. 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). > > 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 didn't think it added much complexity to fe-exec.c personally. A lot of the appeal is that it has very minor impact on anything that isn't using it. I think it makes sense to (ab)use the recovery module tests for this, invoking the test program from there. Ideally I'd like to teach pgsql and pg_restore how to use async mode, but that's a whole separate patch. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
