Re: BUG #17893: Assert failed in heap_update()/_delete() when FK modiified by RI trigger in non-read-committed xact

12.04.2023 15:00, PG Bug reporting form wrote:
> The following bug has been logged on the website:
>
> Bug reference: 17893
>
> The following script:
> ...
>
> triggers an assertion failure:

I've come to this assertion failure again, this time from another side, so I
have one reason more to move forward to the issue fix.

First, I've found that two instances of "additional check for
transaction-snapshot mode RI updates" in head_update() and heap_delete()
are not covered by the existing regression tests at all, so I would like to
propose an addition for the fk-snapshot test (see attached). It increases
coverage and causes the assertion failures in both functions.

Second, it looks like Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID))
was added to make sure that tmfd->xmax = HeapTupleHeaderGetUpdateXid(tp.t_data);
(which ends up with just returning t_xmax) returns valid xmax. Really, in the
problematic case we get zero in tmfd->xmax. But ExecDelete(), ExecUpdate() do:
...
        switch (result)
...
            case TM_Updated:
...
                    if (IsolationUsesXactSnapshot())
                        ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
                                 errmsg("could not serialize access due to concurrent update")));

before any processing of tmfd. So it seems like the invalid xmax is not
an issue in this case.
Thus, maybe
        Assert((!(tp.t_data->t_infomask & HEAP_XMAX_INVALID))
should be changed to
        Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID) ||
               (result == TM_Updated && crosscheck != InvalidSnapshot));

Third, with this change applied I immediately got a failure of the next
assert in heap_delete():
            Assert(!ItemPointerEquals(&oldtup.t_self, &oldtup.t_data->t_ctid));
which was introduced by 5db6df0c0.

(By the way, I've noticed a typo brought by that commit:
t_xmax, and, if possible, and, if possible, t_cmax
)

It's not clear to me yet, which anomalous condition that Assert should
detect, specifically in heap_update(), inside this branch:
        /* Perform additional check for transaction-snapshot mode RI updates */
        if (!HeapTupleSatisfiesVisibility(&oldtup, crosscheck, buffer))
        {
            result = TM_Updated;
            Assert(!ItemPointerEquals(&oldtup.t_self, &oldtup.t_data->t_ctid));
        }

Andres, could you shed some light on this, please?

Best regards,
Alexander