Re: PATCH: Batch/pipelining support for libpq

From: 'Alvaro Herrera' <alvherre(at)alvh(dot)no-ip(dot)org>
To: "k(dot)jamison(at)fujitsu(dot)com" <k(dot)jamison(at)fujitsu(dot)com>
Cc: Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com>, Matthieu Garrigues <matthieu(dot)garrigues(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: PATCH: Batch/pipelining support for libpq
Date: 2021-03-03 16:34:30
Message-ID: 20210303163430.GA24777@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2021-Mar-03, k(dot)jamison(at)fujitsu(dot)com wrote:

> I tried applying this patch to test it on top of Iwata-san's libpq trace log [1].
> In my environment, the compiler complains.
> It seems that in libpqwalreceiver.c: libpqrcv_exec()
> the switch for PQresultStatus needs to handle the
> cases for PGRES_PIPELINE_END and PGRES_PIPELINE_ABORTED too.

Yeah, I noticed this too and decided to handle these cases as errors.

Here's v28, which includes that fix, and also gets rid of the
PGASYNC_QUEUED state; I instead made the rest of the code work using the
existing PGASYNC_IDLE state. This allows us get rid of a few cases
where we had to report "internal error: IDLE state unexpected" in a few
cases, and was rather pointless. With the current formulation, both
pipeline mode and normal mode share pretty much all state.

Also, I've renamed PGRES_PIPELINE_END to PGRES_PIPELINE_SYNC, because
that's closer to what it means: that result state by no means that
pipeline mode has ended. It only means that you called PQsendPipeline()
so the server gives you a Sync message.

I'm much more comfortable with this version, so I'm marking the patch as
Ready for Committer in case anybody else wants to look at this before I
push it.

However: on errors, I think it's a bit odd that you get a NULL from
PQgetResult after getting PGRES_PIPELINE_ABORTED. Maybe I should
suppress that. I'm no longer wrestling with the NULL after
PGRES_PIPELINE_END however, because that's not critical and you can not
ask for it and things work fine.

(Also, the test pipeline.c program in src/test/modules/test_libpq has a
new "transaction" mode that is not exercised by the TAP program; I'll
plug that in before commit.)

--
Álvaro Herrera Valdivia, Chile

Attachment Content-Type Size
v28-libpq-pipeline.patch text/x-diff 100.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Julien Rouhaud 2021-03-03 16:36:08 Re: Shared memory size computation oversight?
Previous Message David Steele 2021-03-03 16:27:26 Re: [PATCH] Implement INSERT SET syntax