From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
Cc: | vignesh C <vignesh21(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Invalid Assert while validating REPLICA IDENTITY? |
Date: | 2024-09-09 07:42:41 |
Message-ID: | CAA4eK1Ljfk0CQ78Ooz1XETkXFgm4kiMgAz+EmE=pgFEmueYKSQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
--
With Regards,
Amit Kapila.
Attachment | Content-Type | Size |
---|---|---|
v1_improve_assert_worker.patch | application/octet-stream | 910 bytes |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Harris | 2024-09-09 07:56:54 | Re: ANALYZE ONLY |
Previous Message | Peter Smith | 2024-09-09 07:40:50 | Re: Introduce XID age and inactive timeout based replication slot invalidation |