Horton

Thread

Commits

  1. Fix misbehavior of EvalPlanQual checks with multiple result relations.

  2. Rework planning and execution of UPDATE and DELETE.

  1. The documentation for READ COMMITTED may be incomplete or wrong

    Aleksander Alekseev <aleksander@timescale.com> — 2023-05-18T14:23:08Z

    Hi hackers,

A colleague of mine, Ante Krešić, got puzzled by the following behavior:

Setup:

postgres=# create table inh_test (id serial, value float);
CREATE TABLE
postgres=# create table inh_child_1 () INHERITS ( inh_test);
CREATE TABLE
postgres=# create table inh_child_2 () INHERITS ( inh_test);
CREATE TABLE
postgres=# insert into inh_child_1 values (1,1);
INSERT 0 1
postgres=# insert into inh_child_2 values (1,1);
INSERT 0 1

Update tuples in first transaction:

postgres=# begin;
BEGIN
postgres=*# update inh_test set value = 2 where value = 1;
UPDATE 2

Delete in second transaction while the first is still active:

postgres=# delete from inh_test where value = 1;

Commit in the first transaction and we get a delete in the second one
even though committed values do not qualify after update.

postgres=# COMMIT;

postgres=# delete from inh_test where value = 1;
DELETE 1

The same happens for declarative partitioned tables as well. When
working on a table without inheritance / partitioning the result is
different, DELETE 0.

So what's the problem?

According to the documentation [1]:

"""
UPDATE, DELETE [..] commands behave the same as SELECT in terms of
searching for target rows: they will only find target rows that were
committed as of the command start time. However, such a target row
might have already been updated (or deleted or locked) by another
concurrent transaction by the time it is found. In this case, the
would-be updater will wait for the first updating transaction to
commit or roll back (if it is still in progress). If the first updater
rolls back, then its effects are negated and the second updater can
proceed with updating the originally found row. If the first updater
commits, the second updater will ignore the row if the first updater
deleted it, otherwise it will attempt to apply its operation to the
updated version of the row. The search condition of the command (the
WHERE clause) is re-evaluated to see if the updated version of the row
still matches the search condition. If so, the second updater proceeds
with its operation using the updated version of the row.
"""

It looks like the observed behaviour contradicts the documentation. If
we read it literally the second transaction should delete 0 rows, as
it does for non-partitioned and non-inherited tables. From what I can
tell the observed behavior doesn't contradict the general guarantees
promised by READ COMMITTED.

Perhaps we should update the documentation for this case, or maybe
remove the quoted part of it.

Thoughts?

[1]: https://www.postgresql.org/docs/current/transaction-iso.html

-- 
Best regards,
Aleksander Alekseev

  2. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-18T14:53:35Z

    Aleksander Alekseev <aleksander@timescale.com> writes:
> A colleague of mine, Ante Krešić, got puzzled by the following behavior:

That's not a documentation problem.  That's a bug, and an extremely
nasty one.  A quick check shows that it works as expected up through
v13, but fails as described in v14 and later.  Needs bisecting ...

			regards, tom lane

  3. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Nathan Bossart <nathandbossart@gmail.com> — 2023-05-18T15:20:18Z

    On Thu, May 18, 2023 at 10:53:35AM -0400, Tom Lane wrote:
> That's not a documentation problem.  That's a bug, and an extremely
> nasty one.  A quick check shows that it works as expected up through
> v13, but fails as described in v14 and later.  Needs bisecting ...

git-bisect points me to 86dc900.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  4. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-18T15:22:54Z

    I wrote:
> Aleksander Alekseev <aleksander@timescale.com> writes:
>> A colleague of mine, Ante Krešić, got puzzled by the following behavior:

> That's not a documentation problem.  That's a bug, and an extremely
> nasty one.  A quick check shows that it works as expected up through
> v13, but fails as described in v14 and later.  Needs bisecting ...

Ugh.  Bisecting says it broke at

86dc90056dfdbd9d1b891718d2e5614e3e432f35 is the first bad commit
commit 86dc90056dfdbd9d1b891718d2e5614e3e432f35
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Wed Mar 31 11:52:34 2021 -0400

    Rework planning and execution of UPDATE and DELETE.

which was absolutely not supposed to be breaking any concurrent-execution
guarantees.  I wonder what we got wrong.

			regards, tom lane

  5. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Aleksander Alekseev <aleksander@timescale.com> — 2023-05-18T15:27:22Z

    Hi,

> I wonder what we got wrong.

One thing we noticed is that the description for EvalPlanQual may be wrong [1]:

"""
In UPDATE/DELETE, only the target relation needs to be handled this way.
In SELECT FOR UPDATE, there may be multiple relations flagged FOR UPDATE,
so we obtain lock on the current tuple version in each such relation before
executing the recheck.
"""

[1]: https://github.com/postgres/postgres/blob/master/src/backend/executor/README#L381

-- 
Best regards,
Aleksander Alekseev

  6. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Nathan Bossart <nathandbossart@gmail.com> — 2023-05-18T19:51:23Z

    On Thu, May 18, 2023 at 11:22:54AM -0400, Tom Lane wrote:
> Ugh.  Bisecting says it broke at
> 
> 86dc90056dfdbd9d1b891718d2e5614e3e432f35 is the first bad commit
> commit 86dc90056dfdbd9d1b891718d2e5614e3e432f35
> Author: Tom Lane <tgl@sss.pgh.pa.us>
> Date:   Wed Mar 31 11:52:34 2021 -0400
> 
>     Rework planning and execution of UPDATE and DELETE.
> 
> which was absolutely not supposed to be breaking any concurrent-execution
> guarantees.  I wonder what we got wrong.

With the reproduction steps listed upthread, I see that XMAX for both
tuples is set to the deleting transaction, but the one in inh_child_2 has
two additional infomask flags: HEAP_XMAX_EXCL_LOCK and HEAP_XMAX_LOCK_ONLY.
If I add a third table (i.e., inh_child_3), XMAX for all three tuples is
set to the deleting transaction, and only the one in inh_child_3 has the
lock bits set.  Also, in the three-table case, the DELETE statement reports
"DELETE 2".

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  7. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-18T20:03:36Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> On Thu, May 18, 2023 at 11:22:54AM -0400, Tom Lane wrote:
>> Ugh.  Bisecting says it broke at
>> commit 86dc90056dfdbd9d1b891718d2e5614e3e432f35
>> which was absolutely not supposed to be breaking any concurrent-execution
>> guarantees.  I wonder what we got wrong.

> With the reproduction steps listed upthread, I see that XMAX for both
> tuples is set to the deleting transaction, but the one in inh_child_2 has
> two additional infomask flags: HEAP_XMAX_EXCL_LOCK and HEAP_XMAX_LOCK_ONLY.
> If I add a third table (i.e., inh_child_3), XMAX for all three tuples is
> set to the deleting transaction, and only the one in inh_child_3 has the
> lock bits set.  Also, in the three-table case, the DELETE statement reports
> "DELETE 2".

Yeah.  I see the problem: when starting up an EPQ recheck, we stuff
the tuple-to-test into the epqstate->relsubs_slot[] entry for the
relation it came from, but we do nothing to the EPQ state for the
other target relations, which allows the EPQ plan to fetch rows
from those relations as usual.  If it finds a (non-updated) row
passing the qual, kaboom!  We decide the EPQ check passed.

What we need to do, I think, is set epqstate->relsubs_done[] for
all target relations except the one we are stuffing a tuple into.

While nodeModifyTable can certainly be made to do that, things are
complicated by the fact that currently ExecScanReScan thinks it ought
to clear all the relsubs_done flags, which would break things again.
I wonder if we can simply delete that code.  Dropping the
FDW/Custom-specific code there is a bit scary, but on the whole that
looks like code that got cargo-culted in rather than anything we
actually need.

The reason this wasn't a bug before 86dc90056 is that any given
plan tree could have only one target relation, so there was not
anything else to suppress.

			regards, tom lane

  8. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Nathan Bossart <nathandbossart@gmail.com> — 2023-05-18T21:34:42Z

    On Thu, May 18, 2023 at 04:03:36PM -0400, Tom Lane wrote:
> Yeah.  I see the problem: when starting up an EPQ recheck, we stuff
> the tuple-to-test into the epqstate->relsubs_slot[] entry for the
> relation it came from, but we do nothing to the EPQ state for the
> other target relations, which allows the EPQ plan to fetch rows
> from those relations as usual.  If it finds a (non-updated) row
> passing the qual, kaboom!  We decide the EPQ check passed.

Ah, so the EPQ check only fails for the last tuple because we won't fetch
rows from the other relations.  I think that explains the behavior I'm
seeing.

> What we need to do, I think, is set epqstate->relsubs_done[] for
> all target relations except the one we are stuffing a tuple into.

This seems generally reasonable to me.

> While nodeModifyTable can certainly be made to do that, things are
> complicated by the fact that currently ExecScanReScan thinks it ought
> to clear all the relsubs_done flags, which would break things again.
> I wonder if we can simply delete that code.  Dropping the
> FDW/Custom-specific code there is a bit scary, but on the whole that
> looks like code that got cargo-culted in rather than anything we
> actually need.

I see that part was added in 385f337 [0].  I haven't had a chance to
evaluate whether it seems necessary.

[0] https://postgr.es/m/9A28C8860F777E439AA12E8AEA7694F80117370C%40BPXM15GP.gisp.nec.co.jp

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  9. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-18T22:26:22Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> On Thu, May 18, 2023 at 04:03:36PM -0400, Tom Lane wrote:
>> What we need to do, I think, is set epqstate->relsubs_done[] for
>> all target relations except the one we are stuffing a tuple into.

> This seems generally reasonable to me.

Here's a draft patch for this.  I think it's OK for HEAD, but we are
going to need some different details for the back branches, because
adding a field to struct EPQState would create an ABI break.  Our
standard trick of shoving the field to the end in the back branches
won't help, because struct EPQState is actually embedded in
struct ModifyTableState, meaning that all the subsequent fields
therein will move.  Maybe we can get away with that, but I bet not.

I think what the back branches will have to do is reinitialize the
relsubs_done array every time through EvalPlanQual(), which is a bit sad
but probably doesn't amount to anything compared to the startup overhead
of the sub-executor.

Debian Code Search doesn't know of any outside code touching
relsubs_done, so I think we are safe in dropping that code in
ExecScanReScan.  It seems quite pointless anyway considering
that up to now, EvalPlanQualBegin has always zeroed the whole
array.

			regards, tom lane

  10. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-18T23:57:14Z

    I wrote:
> Debian Code Search doesn't know of any outside code touching
> relsubs_done, so I think we are safe in dropping that code in
> ExecScanReScan.  It seems quite pointless anyway considering
> that up to now, EvalPlanQualBegin has always zeroed the whole
> array.

Oh, belay that.  What I'd forgotten is that it's possible that
the target relation is on the inside of a nestloop, meaning that
we might need to fetch the EPQ substitute tuple more than once.
So there are three possible states: blocked (never return a
tuple), ready to return a tuple, and done returning a tuple
for this scan.  ExecScanReScan needs to reset "done" to "ready",
but not touch the "blocked" state.  The attached v2 mechanizes
that using two bool arrays.

What I'm thinking about doing to back-patch this is to replace
one of the pointer fields in EPQState with a pointer to a
subsidiary palloc'd structure, where we can put the new fields
along with the cannibalized old one.  We've done something
similar before, and it seems a lot safer than having basically
different logic in v16 than earlier branches.

			regards, tom lane

  11. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Aleksander Alekseev <aleksander@timescale.com> — 2023-05-19T11:02:24Z

    Hi,

> The attached v2 mechanizes  that using two bool arrays.

I tested the patch on several combinations of operating systems
(LInux, MacOS) and architectures (x64, RISC-V) available to me at the
moment, with both Meson and Autotools. Also I made sure
eval-plan-qual.spec fails when the C code is untouched.

The patch passed all the checks I could come up with.

--
Best regards,
Aleksander Alekseev

  12. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Amit Langote <amitlangote09@gmail.com> — 2023-05-19T12:53:45Z

    Thanks for the patch.

On Fri, May 19, 2023 at 8:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > Debian Code Search doesn't know of any outside code touching
> > relsubs_done, so I think we are safe in dropping that code in
> > ExecScanReScan.  It seems quite pointless anyway considering
> > that up to now, EvalPlanQualBegin has always zeroed the whole
> > array.
>
> Oh, belay that.  What I'd forgotten is that it's possible that
> the target relation is on the inside of a nestloop, meaning that
> we might need to fetch the EPQ substitute tuple more than once.
> So there are three possible states: blocked (never return a
> tuple), ready to return a tuple, and done returning a tuple
> for this scan.  ExecScanReScan needs to reset "done" to "ready",
> but not touch the "blocked" state.  The attached v2 mechanizes
> that using two bool arrays.

Aha, that's clever.  So ExecScanReScan() would only reset the
relsubs_done[] entry for the currently active ("unblocked") target
relation, because that would be the only one "unblocked" during a
given EvalPlanQual() invocation.

+    * Initialize per-relation EPQ tuple states.  Result relations, if any,
+    * get marked as blocked; others as not-fetched.

Would it be helpful to clarify that "blocked" means blocked for a
given EvalPlanQual() cycle?

+   /*
+    * relsubs_blocked[scanrelid - 1] is true if there is no EPQ tuple for
+    * this target relation.
+    */
+   bool       *relsubs_blocked;

Similarly, maybe say "no EPQ tuple for this target relation in a given
EvalPlanQual() invocation" here?

BTW, I didn't quite understand why EPQ involving resultRelations must
behave in this new way but not the EPQ during LockRows?

> What I'm thinking about doing to back-patch this is to replace
> one of the pointer fields in EPQState with a pointer to a
> subsidiary palloc'd structure, where we can put the new fields
> along with the cannibalized old one.  We've done something
> similar before, and it seems a lot safer than having basically
> different logic in v16 than earlier branches.

+1.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

  13. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-19T16:22:37Z

    Amit Langote <amitlangote09@gmail.com> writes:
> On Fri, May 19, 2023 at 8:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> +    * Initialize per-relation EPQ tuple states.  Result relations, if any,
> +    * get marked as blocked; others as not-fetched.

> Would it be helpful to clarify that "blocked" means blocked for a
> given EvalPlanQual() cycle?

Probably best to put that with the data structure's comments.  I changed
those to look like

    /*
     * relsubs_done[scanrelid - 1] is true if there is no EPQ tuple for this
     * target relation or it has already been fetched in the current scan of
     * this target relation within the current EvalPlanQual test.
     */
    bool       *relsubs_done;

    /*
     * relsubs_blocked[scanrelid - 1] is true if there is no EPQ tuple for
     * this target relation during the current EvalPlanQual test.  We keep
     * these flags set for all relids listed in resultRelations, but
     * transiently clear the one for the relation whose tuple is actually
     * passed to EvalPlanQual().
     */
    bool       *relsubs_blocked;


> BTW, I didn't quite understand why EPQ involving resultRelations must
> behave in this new way but not the EPQ during LockRows?

LockRows doesn't have a bug: it always fills all the EPQ tuple slots
it's responsible for, and it doesn't use EvalPlanQual() anyway.
In the name of simplicity I kept the behavior exactly the same for
callers other than nodeModifyTable.

Perhaps replication/logical/worker.c could use a closer look here.
It's not entirely clear to me that the EPQ state it sets up is ever
used; but if it is I think it is okay as I have it here, because it
looks like those invocations always have just one result relation in
the plan, so there aren't any "extra" result rels that need to be
blocked.

			regards, tom lane

  14. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-19T18:33:36Z

    Amit Langote <amitlangote09@gmail.com> writes:
> On Fri, May 19, 2023 at 8:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What I'm thinking about doing to back-patch this is to replace
>> one of the pointer fields in EPQState with a pointer to a
>> subsidiary palloc'd structure, where we can put the new fields
>> along with the cannibalized old one.  We've done something
>> similar before, and it seems a lot safer than having basically
>> different logic in v16 than earlier branches.

> +1.

Done that way.  I chose to replace the tuple_table field, because
it was in a convenient spot and it seemed like the field least
likely to have any outside code referencing it.

			regards, tom lane

  15. Re: The documentation for READ COMMITTED may be incomplete or wrong

    Aleksander Alekseev <aleksander@timescale.com> — 2023-05-19T18:42:50Z

    Hi Tom,

> Done that way.  I chose to replace the tuple_table field, because
> it was in a convenient spot and it seemed like the field least
> likely to have any outside code referencing it.

Many thanks!

If it's not too much trouble could you please recommend good entry
points to learn more about the internals of this part of the system
and accompanying edge cases? Perhaps there is an experiment or two an
extension author can do in order to lower the entry threshold and/or
known bugs, limitations or wanted features one could start with?

-- 
Best regards,
Aleksander Alekseev