On Mon, Sep 9, 2024 at 11:44 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Fri, Sep 6, 2024 at 4:48 PM vignesh C <vignes...@gmail.com> wrote: > > > > On Mon, 2 Sept 2024 at 18:22, Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > > > On Mon, Sep 2, 2024 at 3:32 PM Amit Kapila <amit.kapil...@gmail.com> > > > wrote: > > > > > > > > On Mon, Sep 2, 2024 at 11:21 AM Dilip Kumar <dilipbal...@gmail.com> > > > > wrote: > > > > > > > > > > While working on some other code I noticed that in > > > > > FindReplTupleInLocalRel() there is an assert [1] that seems to be > > > > > passing IndexRelation to GetRelationIdentityOrPK() whereas it should > > > > > be passing normal relation. > > > > > > > > > > > > > Agreed. But this should lead to assertion failure. Did you try testing > > > > it? > > > > > > No, I did not test this particular case, it impacted me with my other > > > addition of the code where I got Index Relation as input to the > > > RelationGetIndexList() function, and my local changes were impacted by > > > that. I will write a test for this stand-alone case so that it hits > > > the assert. Thanks for looking into this. > > > > The FindReplTupleInLocalRel function can be triggered by both update > > and delete operations, but this only occurs if the relation has been > > marked as updatable by the logicalrep_rel_mark_updatable function. If > > the relation is marked as non-updatable, an error will be thrown by > > check_relation_updatable. Given this, if a relation is updatable, the > > IsIndexUsableForReplicaIdentityFull condition might always evaluate to > > true due to the previous checks in logicalrep_rel_mark_updatable. > > Therefore, it’s possible that we might not encounter the Assert > > statement, as IsIndexUsableForReplicaIdentityFull may consistently be > > true. > > Thoughts? > > With that it seems that the first Assert condition is useless isn't it? >
The second part of the assertion is incomplete. The IsIndexUsableForReplicaIdentityFull() should be used only when the remote relation has REPLICA_IDENTITY_FULL set. I haven't tested all possible cases yet but I think the attached should be a better way to write this assertion. -- With Regards, Amit Kapila.
v1_improve_assert_worker.patch
Description: Binary data
