Thread: Odd query execution behavior with extended protocol

On Sat, Oct 3, 2015 at 5:03 AM, Shay Rojansky <roji@roji.org> wrote:
> Hi hackers, some odd behavior has been reported with Npgsql and I wanted to
> get your help.
>
> Npgsql supports sending multiple SQL statements in a single packet via the
> extended protocol. This works fine, but when the second query SELECTs a
> value modified by the first's UPDATE, I'm getting a result as if the UPDATE
> hasn't yet occurred.
>
> The exact messages send by Npgsql are:
>
> Parse (UPDATE data SET name='foo' WHERE id=1), statement=unnamed
> Describe (statement=unnamed)
> Bind (statement=unnamed, portal=MQ0)
> Parse (SELECT * FROM data WHERE id=1), statement=unnamed
> Describe (statement=unnamed)
> Bind (statement=unnamed, portal=MQ1)
> Execute (portal=MQ0)
> Close (portal=MQ0)
> Execute (portal=MQ1)
> Close (portal=MQ1)
> Sync
>
> Instead of returning the expected 'foo' value set in the first command's
> UPDATE, I'm getting whatever value was previously there.
> Note that this happen regardless of whether a transaction is already set and
> of the isolation level.
>
> Is this the expected behavior, have I misunderstood the protocol specs?

From looking at the code, it appears to me that if the Execute is run
to completion, then its results will be seen by future statements, but
if the portal is closed earlier, then not.  See the end of
exec_execute_message.  The handler for the Close message (inside
PostgresMain) doesn't seem to do anything that would result in a
CommandCounterIncrement() or CommitTransactionCommand().

This does seem a little strange.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> From looking at the code, it appears to me that if the Execute is run
> to completion, then its results will be seen by future statements, but
> if the portal is closed earlier, then not.  See the end of
> exec_execute_message.  The handler for the Close message (inside
> PostgresMain) doesn't seem to do anything that would result in a
> CommandCounterIncrement() or CommitTransactionCommand().

> This does seem a little strange.

I dunno, if you close a portal before you've gotten CommandComplete,
should you expect that all its actions were taken?  Arguably, that
should be more like a ROLLBACK.

Note there'd only be a difference in case of an operation with RETURNING,
else there's no way (at this level anyway) to pause a data-modifying
command mid-execution.  This logic all predates RETURNING, I think,
so maybe it does need to be revisited.  But it's not entirely clear
what should happen.
        regards, tom lane

On Tue, Oct 6, 2015 at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> From looking at the code, it appears to me that if the Execute is run
>> to completion, then its results will be seen by future statements, but
>> if the portal is closed earlier, then not.  See the end of
>> exec_execute_message.  The handler for the Close message (inside
>> PostgresMain) doesn't seem to do anything that would result in a
>> CommandCounterIncrement() or CommitTransactionCommand().
>
>> This does seem a little strange.
>
> I dunno, if you close a portal before you've gotten CommandComplete,
> should you expect that all its actions were taken?  Arguably, that
> should be more like a ROLLBACK.

I dunno either, but a rollback would undo everything, and a commit
would do everything.  Ending up in a state where we've done some of it
but not all of it is strange.  Being able to run an unbounded number
of commands without a CommandCounterIncrement is *really* strange.

I'm not very sure what to do about it, though.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Oct 6, 2015 at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I dunno, if you close a portal before you've gotten CommandComplete,
>> should you expect that all its actions were taken?  Arguably, that
>> should be more like a ROLLBACK.

> I dunno either, but a rollback would undo everything, and a commit
> would do everything.  Ending up in a state where we've done some of it
> but not all of it is strange.  Being able to run an unbounded number
> of commands without a CommandCounterIncrement is *really* strange.

I'm fairly sure that we *have* done all of it; the Portal code is careful
to execute DML commands to completion whether or not the client accepts
all the RETURNING rows.  It will become visible after you commit.  The
issue here is just whether it's visible to a subsequent Bind within the
same transaction.

> I'm not very sure what to do about it, though.

Possibly we need the close-portal message processing code to do the CCI
stuff if it observes that the Portal hasn't been run to completion.
(I think there is already enough state in the Portal struct to tell that,
though I'm too lazy to look right now.)

I'm concerned though whether this would amount to a protocol break.
It's certainly a behavioral change that we'd have to document.
        regards, tom lane

On Tue, Oct 6, 2015 at 6:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm concerned though whether this would amount to a protocol break.

Me, too.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Oct 6, 2015 at 6:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm concerned though whether this would amount to a protocol break.

> Me, too.

There are enough CCI's floating around the code that there may not
actually be any observable problem, at least not in typical cases.
That would explain the lack of complaints ...
        regards, tom lane

On Tue, Oct 6, 2015 at 6:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Oct 6, 2015 at 6:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I'm concerned though whether this would amount to a protocol break.
>
>> Me, too.
>
> There are enough CCI's floating around the code that there may not
> actually be any observable problem, at least not in typical cases.
> That would explain the lack of complaints ...

It's pretty to think so, but I've been doing this long enough to be skeptical.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company