Re: BUG #18238: Cross-partitition MERGE/UPDATE with delete-preventing trigger leads to incorrect memory access

On Sun, 10 Dec 2023 at 12:20, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> > On Sun, Dec 10, 2023 at 1:10 AM PG Bug reporting form
> > <noreply(at)postgresql(dot)org> wrote:
> > >
> > > CREATE TABLE t (a int) PARTITION BY LIST (a);
> > > CREATE TABLE tp1 PARTITION OF t FOR VALUES IN (1);
> > > CREATE TABLE tp2 PARTITION OF t FOR VALUES IN (2);
> > > INSERT INTO t VALUES (1);
> > >
> > > CREATE FUNCTION tf() RETURNS TRIGGER LANGUAGE plpgsql AS
> > > $$ BEGIN RETURN NULL; END; $$;
> > >
> > > CREATE TRIGGER tr BEFORE DELETE ON t
> > > FOR EACH ROW EXECUTE PROCEDURE tf();
> > >
> > > MERGE INTO t USING t st ON TRUE WHEN MATCHED THEN UPDATE SET a = 2;
> > >

Ah, yes. Nice catch! The cross-partition MERGE code is evidently still
not right.

> > --- a/src/backend/executor/nodeModifyTable.c
> > +++ b/src/backend/executor/nodeModifyTable.c
> > @@ -1828,10 +1828,10 @@ ExecCrossPartitionUpdate(ModifyTableContext *context,
> > * additional rechecking, and might end up executing a different
> > * action entirely).
> > */
> > - if (context->relaction != NULL)
> > - return false;
> > - else if (TupIsNull(epqslot))
> > + if (TupIsNull(epqslot))
> > return true;
> > + else if (context->relaction != NULL)
> > + return false;
> > else
> > {
> > /* Fetch the most recent version of old tuple. */
> >
> > seems to work.

No, that's not right. Doing that breaks the isolation tests, because
it causes it to fail to spot concurrent updates. What this code needs
to do is differentiate between a delete blocked by a concurrent
update, and a delete blocked by triggers.

The easiest way to do that is to have ExecDelete() return the
TM_Result status to ExecCrossPartitionUpdate(). I think
ExecCrossPartitionUpdate() should then return the TM_Result status to
ExecUpdateAct(), so that it can return the correct status, rather than
just assuming TM_Updated on failure, though possibly that's not
strictly necessary.

> > but now the new command tag is MERGE 1. should be MERGE 0.
>
> @@ -2909,9 +2909,12 @@ lmerge_matched:
> */
> if (updateCxt.crossPartUpdate)
> {
> - mtstate->mt_merge_updated += 1;
> - if (canSetTag)
> - (estate->es_processed)++;
> + if (context->cpUpdateReturningSlot)
> + {
> + mtstate->mt_merge_updated += 1;
> + if (canSetTag)
> +
> (estate->es_processed)++;
> + }
> return true;
> }
>

No, that's not right. cpUpdateReturningSlot will currently always be
NULL in a MERGE, so that'll cause it to never update the command tag.

The simplest fix is for ExecMergeMatched() to pass canSetTag to
ExecUpdateAct(), so that it updates the command tag, if it does a
cross-partition update. That makes that code path in
ExecMergeMatched() more consistent with ExecUpdate().

So I think we need something like the attached.

Regards,
Dean