Thread: [BUG?] check_exclusion_or_unique_constraint false negative

Dear Michail,

Thanks for pointing out the issue!

>* RelationFindReplTupleByIndex
>
>Amit, this is why I've included you in this previously solo thread :)
>RelationFindReplTupleByIndex uses DirtySnapshot and may not find some records
>if they are updated by a parallel transaction. This could lead to lost
>deletes/updates, especially in the case of streaming=parallel mode. 
>I'm not familiar with how parallel workers apply transactions, so maybe this
>isn't possible.

IIUC, the issue can happen when two concurrent transactions using DirtySnapshot access
the same tuples, which is not specific to the parallel apply. Consider that two
subscriptions exist and publishers modify the same tuple of the same table.
In this case, two workers access the tuple, so one of the changes may be missed
by the scenario you said. I feel we do not need special treatments for parallel
apply.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Aug 1, 2024 at 2:55 PM Michail Nikolaev
<michail.nikolaev@gmail.com> wrote:
>
> > Thanks for pointing out the issue!
>
> Thanks for your attention!
>
> > IIUC, the issue can happen when two concurrent transactions using DirtySnapshot access
> > the same tuples, which is not specific to the parallel apply
>
> Not exactly, it happens for any DirtySnapshot scan over a B-tree index with some other transaction updating the same
indexpage (even using the MVCC snapshot). 
>
> So, logical replication related scenario looks like this:
>
> * subscriber worker receives a tuple update\delete from the publisher
> * it calls RelationFindReplTupleByIndex to find the tuple in the local table
> * some other transaction updates the tuple in the local table (on subscriber side) in parallel
> * RelationFindReplTupleByIndex may not find the tuple because it uses DirtySnapshot
> * update\delete is lost
>
> Parallel apply mode looks like more dangerous because it uses multiple workers on the subscriber side, so the
probabilityof the issue is higher. 
> In that case, "some other transaction" is just another worker applying changes of different transaction in parallel.
>

I think it is rather less likely or not possible in a parallel apply
case because such conflicting updates (updates on the same tuple)
should be serialized at the publisher itself. So one of the updates
will be after the commit that has the second update.

I haven't tried the test based on your description of the general
problem with DirtySnapshot scan. In case of logical replication, we
will LOG update_missing type of conflict and the user may need to take
some manual action based on that. I have not tried a test so I could
be wrong as well. I am not sure we can do anything specific to logical
replication for this but feel free to suggest if you have ideas to
solve this problem in general or specific to logical replication.

--
With Regards,
Amit Kapila.

On Fri, Aug 2, 2024 at 10:38 PM Michail Nikolaev
<michail.nikolaev@gmail.com> wrote:
>
> > I think it is rather less likely or not possible in a parallel apply
> > case because such conflicting updates (updates on the same tuple)
> > should be serialized at the publisher itself. So one of the updates
> > will be after the commit that has the second update.
>
> Glad to hear! But anyway, such logic looks very fragile to me.
>
> > I haven't tried the test based on your description of the general
> > problem with DirtySnapshot scan. In case of logical replication, we
> > will LOG update_missing type of conflict and the user may need to take
> > some manual action based on that.
>
> Current it is just DEBUG1, so it will be probably missed by the user.
>
> > * XXX should this be promoted to ereport(LOG) perhaps?
> > */
> > elog(DEBUG1,
> > "logical replication did not find row to be updated "
> > "in replication target relation \"%s\"",
> > RelationGetRelationName(localrel));
> > }
>

Right, but we are extending this functionality to detect and resolve
such conflicts [1][2]. I am hoping after that such updates won't be
missed.

[1] - https://commitfest.postgresql.org/49/5064/
[2] - https://commitfest.postgresql.org/49/5021/


--
With Regards,
Amit Kapila.

Hi,

Thanks for reporting the issue !

I tried to reproduce this in logical replication but failed. If possible,
could you please share some steps to reproduce it in logicalrep context ?

In my test, if the tuple is updated and new tuple is in the same page,
heapam_index_fetch_tuple should find the new tuple using HOT chain. So, it's a
bit unclear to me how the updated tuple is missing. Maybe I missed some other
conditions for this issue.

It would be better if we can reproduce this by adding some breakpoints using
gdb, which may help us to write a tap test using injection point to reproduce
this reliably. I see the tap test you shared used pgbench to reproduce this,
it works, but It would be great if we can analyze the issue more deeply by
debugging the code.

And I have few questions related the steps you shared:

> * Session 1 reads a B-tree page using SnapshotDirty and copies item X to the buffer.
> * Session 2 updates item X, inserting a new TID Y into the same page.
> * Session 2 commits its transaction.
> * Session 1 starts to fetch from the heap and tries to fetch X, but it was
>   already deleted by session 2. So, it goes to the B-tree for the next TID.
> * The B-tree goes to the next page, skipping Y.
> * Therefore, the search finds nothing, but tuple Y is still alive.

I am wondering at which point should the update happen ? should it happen after
calling index_getnext_tid and before index_fetch_heap ? It would be great if
you could give more details in above steps. Thanks !

Best Regards,
Hou zj