Thread: [Logical Replication] TRAP: FailedAssertion("rel->rd_rel->relreplident== REPLICA_IDENTITY_DEFAULT || rel->rd_rel->relreplident ==REPLICA_IDENTITY_FULL || rel->rd_rel->relreplident == REPLICA_IDENTITY_INDEX"

On Tue, Jan 07, 2020 at 05:38:49PM +0530, Neha Sharma wrote:
> I am getting a server crash on publication server on HEAD for the below
> test case.
>
> Test case:
> Publication server:
> create table test(a int);
> create publication test_pub for all tables;
> alter table test replica identity NOTHING ;
>
> Subscription server:
> create table test(a int);
> create subscription test_sub CONNECTION 'host=172.16.208.32 port=5432
> dbname=postgres user=centos' PUBLICATION test_pub WITH ( slot_name =
> test_slot_sub);
>
> Publication server:
> insert into test values(generate_series(1,5),'aa');

This would not work as your relation has only one column.  There are
some TAP tests for logical replication (none actually stressing
NOTHING as replica identity), which do not fail, and I cannot
reproduce the failure myself.

> After executing the DML in publication server ,it crashed with the
> mentioned assert.

Do you have other objects defined on your schema on the publication or
the subscription side?  Like, er, triggers?
--
Michael

On Thu, Jan 9, 2020 at 12:50 PM Neha Sharma
<neha.sharma@enterprisedb.com> wrote:
>
> Hi Michael,
> Thanks for looking into the issue. Sorry by mistake I had mentioned the incorrect DML query,please use the query as
mentionedbelow.
 
>
> On Thu, Jan 9, 2020 at 11:38 AM Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Tue, Jan 07, 2020 at 05:38:49PM +0530, Neha Sharma wrote:
>> > I am getting a server crash on publication server on HEAD for the below
>> > test case.
>> >
>> > Test case:
>> > Publication server:
>> > create table test(a int);
>> > create publication test_pub for all tables;
>> > alter table test replica identity NOTHING ;
>> >
>> > Subscription server:
>> > create table test(a int);
>> > create subscription test_sub CONNECTION 'host=172.16.208.32 port=5432
>> > dbname=postgres user=centos' PUBLICATION test_pub WITH ( slot_name =
>> > test_slot_sub);
>> >
>> > Publication server:
>> > insert into test values(generate_series(1,5),'aa');
>
>  insert into test values(generate_series(1,5));
>>
>>
>> This would not work as your relation has only one column.  There are
>> some TAP tests for logical replication (none actually stressing
>> NOTHING as replica identity), which do not fail, and I cannot
>> reproduce the failure myself.

I am able to reproduce the failure,  I think the assert in the
'logicalrep_write_insert' is not correct.  IMHO even if the replica
identity is set to NOTHING we should be able to replicate INSERT?

This will fix the issue.

diff --git a/src/backend/replication/logical/proto.c
b/src/backend/replication/logical/proto.c
index dcf7c08..471461c 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -145,7 +145,8 @@ logicalrep_write_insert(StringInfo out, Relation
rel, HeapTuple newtuple)

        Assert(rel->rd_rel->relreplident == REPLICA_IDENTITY_DEFAULT ||
                   rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL ||
-                  rel->rd_rel->relreplident == REPLICA_IDENTITY_INDEX);
+                  rel->rd_rel->relreplident == REPLICA_IDENTITY_INDEX ||
+                  rel->rd_rel->relreplident == REPLICA_IDENTITY_NOTHING);

        /* use Oid as relation identifier */
        pq_sendint32(out, RelationGetRelid(rel));

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

On Thu, 9 Jan 2020 at 08:48, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jan 9, 2020 at 12:50 PM Neha Sharma
> <neha.sharma@enterprisedb.com> wrote:
> >
> > Hi Michael,
> > Thanks for looking into the issue. Sorry by mistake I had mentioned the incorrect DML query,please use the query as
mentionedbelow.
 
> >
> > On Thu, Jan 9, 2020 at 11:38 AM Michael Paquier <michael@paquier.xyz> wrote:
> >>
> >> On Tue, Jan 07, 2020 at 05:38:49PM +0530, Neha Sharma wrote:
> >> > I am getting a server crash on publication server on HEAD for the below
> >> > test case.
> >> >
> >> > Test case:
> >> > Publication server:
> >> > create table test(a int);
> >> > create publication test_pub for all tables;
> >> > alter table test replica identity NOTHING ;
> >> >
> >> > Subscription server:
> >> > create table test(a int);
> >> > create subscription test_sub CONNECTION 'host=172.16.208.32 port=5432
> >> > dbname=postgres user=centos' PUBLICATION test_pub WITH ( slot_name =
> >> > test_slot_sub);
> >> >
> >> > Publication server:
> >> > insert into test values(generate_series(1,5),'aa');
> >
> >  insert into test values(generate_series(1,5));
> >>
> >>
> >> This would not work as your relation has only one column.  There are
> >> some TAP tests for logical replication (none actually stressing
> >> NOTHING as replica identity), which do not fail, and I cannot
> >> reproduce the failure myself.
>
> I am able to reproduce the failure,  I think the assert in the
> 'logicalrep_write_insert' is not correct.  IMHO even if the replica
> identity is set to NOTHING we should be able to replicate INSERT?
>
True that.
> This will fix the issue.
>
> diff --git a/src/backend/replication/logical/proto.c
> b/src/backend/replication/logical/proto.c
> index dcf7c08..471461c 100644
> --- a/src/backend/replication/logical/proto.c
> +++ b/src/backend/replication/logical/proto.c
> @@ -145,7 +145,8 @@ logicalrep_write_insert(StringInfo out, Relation
> rel, HeapTuple newtuple)
>
>         Assert(rel->rd_rel->relreplident == REPLICA_IDENTITY_DEFAULT ||
>                    rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL ||
> -                  rel->rd_rel->relreplident == REPLICA_IDENTITY_INDEX);
> +                  rel->rd_rel->relreplident == REPLICA_IDENTITY_INDEX ||
> +                  rel->rd_rel->relreplident == REPLICA_IDENTITY_NOTHING);
>
>         /* use Oid as relation identifier */
>         pq_sendint32(out, RelationGetRelid(rel));
>
+1



-- 
Regards,
Rafia Sabih

Hi,

On 2020-01-09 13:17:59 +0530, Dilip Kumar wrote:
> I am able to reproduce the failure,  I think the assert in the
> 'logicalrep_write_insert' is not correct.  IMHO even if the replica
> identity is set to NOTHING we should be able to replicate INSERT?
> 
> This will fix the issue.
> 
> diff --git a/src/backend/replication/logical/proto.c
> b/src/backend/replication/logical/proto.c
> index dcf7c08..471461c 100644
> --- a/src/backend/replication/logical/proto.c
> +++ b/src/backend/replication/logical/proto.c
> @@ -145,7 +145,8 @@ logicalrep_write_insert(StringInfo out, Relation
> rel, HeapTuple newtuple)
> 
>         Assert(rel->rd_rel->relreplident == REPLICA_IDENTITY_DEFAULT ||
>                    rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL ||
> -                  rel->rd_rel->relreplident == REPLICA_IDENTITY_INDEX);
> +                  rel->rd_rel->relreplident == REPLICA_IDENTITY_INDEX ||
> +                  rel->rd_rel->relreplident == REPLICA_IDENTITY_NOTHING);
> 
>         /* use Oid as relation identifier */
>         pq_sendint32(out, RelationGetRelid(rel));

There's not much point in having this assert, right? Given that it
covers all choices? Seems better to just drop it.

Greetings,

Andres Freund

On Fri, Jan 10, 2020 at 07:30:34AM +0530, Dilip Kumar wrote:
> On Thu, 9 Jan 2020 at 10:43 PM, Andres Freund <andres@anarazel.de> wrote:
>> There's not much point in having this assert, right? Given that it
>> covers all choices? Seems better to just drop it.
>
> Yeah right!

Refreshing my mind on that...  The two remaining assertions still make
sense for update and delete changes per the restrictions in place in
CheckCmdReplicaIdentity(), and there is a gap with the regression
tests.  So combining all that I get the attached patch (origin point
is 665d1fa).  Thoughts?
--
Michael

On Fri, Jan 10, 2020 at 10:31 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jan 10, 2020 at 07:30:34AM +0530, Dilip Kumar wrote:
> > On Thu, 9 Jan 2020 at 10:43 PM, Andres Freund <andres@anarazel.de> wrote:
> >> There's not much point in having this assert, right? Given that it
> >> covers all choices? Seems better to just drop it.
> >
> > Yeah right!
>
> Refreshing my mind on that...  The two remaining assertions still make
> sense for update and delete changes per the restrictions in place in
> CheckCmdReplicaIdentity(),

Right

 and there is a gap with the regression
> tests.  So combining all that I get the attached patch (origin point
> is 665d1fa).  Thoughts?

LGTM

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

On Fri, Jan 10, 2020 at 11:01:13AM +0530, Dilip Kumar wrote:
> On Fri, Jan 10, 2020 at 10:31 AM Michael Paquier <michael@paquier.xyz> wrote:
>> and there is a gap with the regression
>> tests.  So combining all that I get the attached patch (origin point
>> is 665d1fa).  Thoughts?
>
> LGTM

Thanks for the lookup.  I'll look at that again in a couple of days
and hopefully wrap it by then.
--
Michael

On Sat, Jan 11, 2020 at 10:34:20AM +0900, Michael Paquier wrote:
> Thanks for the lookup.  I'll look at that again in a couple of days
> and hopefully wrap it by then.

And done.
--
Michael