General development

Tom Lane | 2 Aug 2012 07:19

Re: [patch] libpq one-row-at-a-time API

Marko Kreen <markokr <at> gmail.com> writes:
> On Wed, Aug 1, 2012 at 6:18 PM, Tom Lane <tgl <at> sss.pgh.pa.us> wrote:
>> So I'm working from the first set of patches in your message
>> <20120721194907.GA28021 <at> gmail.com>.

> Great!

Here's an updated draft patch.  I've not looked at the docs yet so
this doesn't include that, but I'm reasonably happy with the code
changes now.  The main difference from what you had is that I pushed
the creation of the single-row PGresults into pqRowProcessor, so that
they're formed immediately while scanning the input message.  That
other method with postponing examination of the rowBuf does not work,
any more than it did with the original patch, because you can't assume
that the network input buffer is going to hold still.  For example,
calling PQconsumeInput after parseInput has absorbed a row-data message
but before calling PQgetResult would likely break things.

In principle I suppose we could hack PQconsumeInput enough that it
didn't damage the row buffer while still meeting its API contract of
clearing the read-ready condition on the socket; but it wouldn't be
simple or cheap to do so.

			regards, tom lane

Attachment: text/x-patch, 48 KiB


--

--

(Continue reading)

headers

Marko Kreen | 2 Aug 2012 22:24

Re: [patch] libpq one-row-at-a-time API

On Thu, Aug 2, 2012 at 8:19 AM, Tom Lane <tgl <at> sss.pgh.pa.us> wrote:
> Marko Kreen <markokr <at> gmail.com> writes:
>> On Wed, Aug 1, 2012 at 6:18 PM, Tom Lane <tgl <at> sss.pgh.pa.us> wrote:
>>> So I'm working from the first set of patches in your message
>>> <20120721194907.GA28021 <at> gmail.com>.
>
>> Great!
>
> Here's an updated draft patch.  I've not looked at the docs yet so
> this doesn't include that, but I'm reasonably happy with the code
> changes now.  The main difference from what you had is that I pushed
> the creation of the single-row PGresults into pqRowProcessor, so that
> they're formed immediately while scanning the input message.  That
> other method with postponing examination of the rowBuf does not work,
> any more than it did with the original patch, because you can't assume
> that the network input buffer is going to hold still.  For example,
> calling PQconsumeInput after parseInput has absorbed a row-data message
> but before calling PQgetResult would likely break things.
>
> In principle I suppose we could hack PQconsumeInput enough that it
> didn't damage the row buffer while still meeting its API contract of
> clearing the read-ready condition on the socket; but it wouldn't be
> simple or cheap to do so.

Any code using single-row-mode is new.  Any code calling consumeInput
before fully draining rows from buffer is buggy, as it allows unlimited growth
of network buffer, which breaks whole reason to use single-row mode.

It was indeed bug in previous patch that it did not handle this situation,
but IMHO it should be handled with error and not with allowing such code

(Continue reading)

headers

Tom Lane | 2 Aug 2012 23:01

Re: [patch] libpq one-row-at-a-time API

Marko Kreen <markokr <at> gmail.com> writes:
> On Thu, Aug 2, 2012 at 8:19 AM, Tom Lane <tgl <at> sss.pgh.pa.us> wrote:
>> In principle I suppose we could hack PQconsumeInput enough that it
>> didn't damage the row buffer while still meeting its API contract of
>> clearing the read-ready condition on the socket; but it wouldn't be
>> simple or cheap to do so.

> Any code using single-row-mode is new.  Any code calling consumeInput
> before fully draining rows from buffer is buggy, as it allows unlimited growth
> of network buffer, which breaks whole reason to use single-row mode.

Sorry, that argument will not fly.  The API specification for
PQconsumeInput is that it can be called at any time to drain the kernel
input buffer, without changing the logical state of the PGconn.  It's
not acceptable to insert a parenthetical "oh, but you'd better not try
it in single-row mode" caveat.

The reason I find this of importance is that PQconsumeInput is meant to
be used in an application's main event loop for the sole purpose of
clearing the read-ready condition on the connection's socket, so that
it can wait for other conditions.  This could very well be decoupled
from the logic that is involved in testing PQisBusy and
fetching/processing actual results.  (If we had not intended to allow
those activities to be decoupled, we'd not have separated PQconsumeInput
and PQisBusy in the first place.)  Introducing a dependency between
these things could lead to non-reproducible, timing-dependent, very
hard-to-find bugs.

By the same token, if somebody were trying to put single-row-mode
support into an existing async application, they'd be fooling with the

(Continue reading)

headers

Marko Kreen | 2 Aug 2012 23:44

Re: [patch] libpq one-row-at-a-time API

On Fri, Aug 3, 2012 at 12:01 AM, Tom Lane <tgl <at> sss.pgh.pa.us> wrote:
> Marko Kreen <markokr <at> gmail.com> writes:
>> On Thu, Aug 2, 2012 at 8:19 AM, Tom Lane <tgl <at> sss.pgh.pa.us> wrote:
>>> In principle I suppose we could hack PQconsumeInput enough that it
>>> didn't damage the row buffer while still meeting its API contract of
>>> clearing the read-ready condition on the socket; but it wouldn't be
>>> simple or cheap to do so.
>
>> Any code using single-row-mode is new.  Any code calling consumeInput
>> before fully draining rows from buffer is buggy, as it allows unlimited growth
>> of network buffer, which breaks whole reason to use single-row mode.
>
> Sorry, that argument will not fly.  The API specification for
> PQconsumeInput is that it can be called at any time to drain the kernel
> input buffer, without changing the logical state of the PGconn.  It's
> not acceptable to insert a parenthetical "oh, but you'd better not try
> it in single-row mode" caveat.

Well, if the old API contract must be kept, then so be it.

> The reason I find this of importance is that PQconsumeInput is meant to
> be used in an application's main event loop for the sole purpose of
> clearing the read-ready condition on the connection's socket, so that
> it can wait for other conditions.  This could very well be decoupled
> from the logic that is involved in testing PQisBusy and
> fetching/processing actual results.  (If we had not intended to allow
> those activities to be decoupled, we'd not have separated PQconsumeInput
> and PQisBusy in the first place.)  Introducing a dependency between
> these things could lead to non-reproducible, timing-dependent, very
> hard-to-find bugs.

(Continue reading)