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

On 23/10/2023 09:00, Alexander Lakhin wrote:
> Third, with this change applied I immediately got a failure of the next
> assert in heap_delete():
> [added more context to show the code better]
> if (crosscheck != InvalidSnapshot && result == TM_Ok)
> {
> /* 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));
> }
> }

Yeah that Assert is wrong and should be removed. I think it's trying to
assert that if we are deleting a tuple and the visibility check fails,
it must be because it was concurrently updated, not because the
inserting XID is not visible. But that doesn't hold for this additional
check with RI updates, the inserting XID might well be invisible to the
crosscheck snapshot.

>> which was introduced by 5db6df0c0.
> Sorry for my mistake here. I had cited a wrong line. It should be:
>         Assert(result != TM_Updated ||
>                !ItemPointerEquals(&tp.t_self, &tp.t_data->t_ctid));
> As I still can't see, which cases those asserts intended for, but see cases
> when they are falsified, I propose removing them.
> Please look at the complete patch attached.

I propose to attached slight variation, which moves the assertions
before the crosscheck snapshot check. The assertions are correct as they
are, if you don't need to take the crosscheck into account.

I'm a little worried if the callers of tuple_update() might also get
confused when tuple_update() returns TM_Updated but xmax is invalid. As
you said:

> So in the latter case the tuple's xmin is not committed according to the
> crosscheck snapshot. Meanwhile, a comment in nodeModifyTable.c says:
> * Note: if es_crosscheck_snapshot isn't InvalidSnapshot, we check that
> * the row to be updated is visible to that snapshot, and throw a
> * can't-serialize error if not. ...
>
>
> And with a non-assert build I really get:
> ERROR: could not serialize access due to concurrent update
> in these cases.

I think you only get that error with REPEATABLE READ or SERIALIZABLE
isolation level. I don't quite understand how this works. In READ
COMMITTED level, ISTM that ExecUpdate() or ExecDelete() will proceed
with EvalPlanQualSlot() and locking the row with table_tuple_lock(). Or
do we never use the cross-check snapshot in READ COMMITTED mode?

--
Heikki Linnakangas
Neon (https://neon.tech)