Thread: Invalid Assert while validating REPLICA IDENTITY?
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
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.
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
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
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
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
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.
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.
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