Re: Invalid Assert while validating REPLICA IDENTITY?

On Mon, 9 Sept 2024 at 13:12, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Sep 9, 2024 at 11:44 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Fri, Sep 6, 2024 at 4:48 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Mon, 2 Sept 2024 at 18:22, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > >
> > > > On Mon, Sep 2, 2024 at 3:32 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Mon, Sep 2, 2024 at 11:21 AM Dilip Kumar <dilipbalaut(at)gmail(dot)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.

The changes look good to me.
I have verified the following test which hits the Assert condition:
Check the first condition in the assert statement with the following test:
-- pub
create table test_update_assert(c1 int primary key, c2 int);
create publication pub1 for table test_update_assert;
--sub
create table test_update_assert(c1 int primary key, c2 int);
create subscription ...;

--pub
insert into test_update_assert values(1,1);
update test_update_assert set c1 = 2;

I also verified that if we replace localrel with idxrel, the assert
will fail for the above test. This is the original issue reported by
Dilip.

Check the 2nd condition in assert with the following test:
-- pub
create table test_update_assert1(c1 int, c2 int);
alter table test_update_assert1 replica identity full;
create publication pub1 for table test_update_assert1;

-- sub
create table test_update_assert1(c1 int, c2 int);
create unique index idx1 on test_update_assert1(c1,c2);
create subscription ...;

--pub
insert into test_update_assert1 values(1,1);
update test_update_assert1 set c1 = 2;

Regards,
Vignesh