Thread

Commits

Improve assertion in FindReplTupleInLocalRel().
- 3beb945da9d7 18.0 landed

Invalid Assert while validating REPLICA IDENTITY?

Dilip Kumar <dilipbalaut@gmail.com> — 2024-09-02T05:51:13Z

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.

[1]
if (OidIsValid(localidxoid))
{
#ifdef USE_ASSERT_CHECKING
    Relation idxrel = index_open(localidxoid, AccessShareLock);

    /* Index must be PK, RI, or usable for REPLICA IDENTITY FULL tables */
    Assert(GetRelationIdentityOrPK(idxrel) == localidxoid ||
    IsIndexUsableForReplicaIdentityFull(BuildIndexInfo(idxrel),
    edata->targetRel->attrmap));
    index_close(idxrel, AccessShareLock);
#endif

In the above code, we are passing idxrel to GetRelationIdentityOrPK(),
whereas we should be passing localrel

Fix should be


@@ -2929,7 +2929,7 @@ FindReplTupleInLocalRel(ApplyExecutionData
*edata, Relation localrel,

                Relation        idxrel = index_open(localidxoid,
AccessShareLock);

                /* Index must be PK, RI, or usable for REPLICA
IDENTITY FULL tables */
-               Assert(GetRelationIdentityOrPK(idxrel) == localidxoid ||
+               Assert(GetRelationIdentityOrPK(localrel) == localidxoid ||

IsIndexUsableForReplicaIdentityFull(BuildIndexInfo(idxrel),

                            edata->targetRel->attrmap));

                index_close(idxrel, AccessShareLock);


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Invalid Assert while validating REPLICA IDENTITY?

Amit Kapila <amit.kapila16@gmail.com> — 2024-09-02T10:02:24Z

On Mon, Sep 2, 2024 at 11:21 AM Dilip Kumar <dilipbalaut@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?

-- 
With Regards,
Amit Kapila.

Re: Invalid Assert while validating REPLICA IDENTITY?

Dilip Kumar <dilipbalaut@gmail.com> — 2024-09-02T12:52:09Z

On Mon, Sep 2, 2024 at 3:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Sep 2, 2024 at 11:21 AM Dilip Kumar <dilipbalaut@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.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Invalid Assert while validating REPLICA IDENTITY?

vignesh C <vignesh21@gmail.com> — 2024-09-06T11:18:51Z

On Mon, 2 Sept 2024 at 18:22, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Sep 2, 2024 at 3:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Sep 2, 2024 at 11:21 AM Dilip Kumar <dilipbalaut@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?

Regards,
Vignesh

Re: Invalid Assert while validating REPLICA IDENTITY?

Dilip Kumar <dilipbalaut@gmail.com> — 2024-09-09T06:14:42Z

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

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Invalid Assert while validating REPLICA IDENTITY?

Amit Kapila <amit.kapila16@gmail.com> — 2024-09-09T07:42:41Z

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

Re: Invalid Assert while validating REPLICA IDENTITY?

vignesh C <vignesh21@gmail.com> — 2024-09-10T06:06:02Z

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

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

Re: Invalid Assert while validating REPLICA IDENTITY?

Amit Kapila <amit.kapila16@gmail.com> — 2024-09-10T08:46:27Z

On Tue, Sep 10, 2024 at 11:36 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 9 Sept 2024 at 13:12, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > 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.
>

Thanks, I'll push this tomorrow unless Dilip or anyone else has any
comments. BTW, as the current code doesn't lead to any bug or
assertion failure, I am planning to push this change to HEAD only, let
me know if you think otherwise.

With Regards,
Amit Kapila.

Re: Invalid Assert while validating REPLICA IDENTITY?

Amit Kapila <amit.kapila16@gmail.com> — 2024-09-11T04:35:42Z

On Tue, Sep 10, 2024 at 2:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Sep 10, 2024 at 11:36 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Mon, 9 Sept 2024 at 13:12, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > 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.
> >
>
> Thanks, I'll push this tomorrow unless Dilip or anyone else has any
> comments. BTW, as the current code doesn't lead to any bug or
> assertion failure, I am planning to push this change to HEAD only, let
> me know if you think otherwise.
>

Pushed.

-- 
With Regards,
Amit Kapila.

Re: Invalid Assert while validating REPLICA IDENTITY?

vignesh C <vignesh21@gmail.com> — 2024-09-11T16:59:56Z

On Wed, 11 Sept 2024 at 10:05, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Sep 10, 2024 at 2:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Sep 10, 2024 at 11:36 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Mon, 9 Sept 2024 at 13:12, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > 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.
> > >
> >
> > Thanks, I'll push this tomorrow unless Dilip or anyone else has any
> > comments. BTW, as the current code doesn't lead to any bug or
> > assertion failure, I am planning to push this change to HEAD only, let
> > me know if you think otherwise.
> >
>
> Pushed.

I noticed that Drongo failed the subscription-check for the 015_stream
test, as reported at [1].

In publisher log I could see the following statement executed
015_stream.pl line 253:
$node_publisher->safe_psql('postgres', "TRUNCATE TABLE test_tab_2");

With the following log content from 015_stream_publisher.log:
2024-09-11 09:13:36.886 UTC [512:4] 015_stream.pl LOG:  statement:
TRUNCATE TABLE test_tab_2

and lastly executed wait_for_catchup 015_stream.pl line 254:
2024-09-11 09:13:38.811 UTC [4320:4] 015_stream.pl LOG:  statement:
SELECT '0/20E3BC8' <= replay_lsn AND state = 'streaming'
         FROM pg_catalog.pg_stat_replication
         WHERE application_name IN ('tap_sub', 'walreceiver')

In subscriber log I could see the following statement executed
015_stream.pl line 255:
$node_subscriber->safe_psql('postgres',
"CREATE UNIQUE INDEX idx_tab on test_tab_2(a)");

With the following log content from 015_stream_subscriber.log:
2024-09-11 09:13:39.610 UTC [3096:4] 015_stream.pl LOG:  statement:
CREATE UNIQUE INDEX idx_tab on test_tab_2(a)

However, there is no clear indication of what transpired afterward or
why the remaining statements were not executed. It appears this issue
is unrelated to the recent patch, as the changes introduced by the
patch affect only update and delete operations.
I will keep monitoring the buildfarm for additional runs to determine
if the issue recurs and draw conclusions based on the observations.

[1] - https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2024-09-11%2007%3A24%3A53

Regards,
Vignesh