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)