On Wed, May 13, 2020 at 6:15 AM Euler Taveira
<euler.taveira@2ndquadrant.com> wrote:
>
> On Tue, 12 May 2020 at 06:36, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote:
>>
>> On Mon, 11 May 2020 at 16:28, Michael Paquier <michael@paquier.xyz> wrote:
>> >
>> > On Sun, May 10, 2020 at 07:08:03PM -0300, Euler Taveira wrote:
>> > > I attached a patch with the described solution. I also included a test that
>> > > covers this scenario.
>> >
>> > - Assert(RelationGetReplicaIndex(rel) == RelationGetRelid(idxrel));
>> > + Assert(GetRelationIdentityOrPK(rel) == RelationGetRelid(idxrel));
>> >
>> > Not much a fan of adding a routine to relcache.c to do the work of two
>> > routines already present, so I think that we had better add an extra
>> > condition based on RelationGetPrimaryKeyIndex, and give up on
>> > GetRelationIdentityOrPK() in execReplication.c.
>>
> Although, I think this solution is fragile, I updated the patch accordingly.
> (When/If someone changed GetRelationIdentityOrPK() it will break this assert)
>
>>
>> In any case, it seems to me that the comment of
>> build_replindex_scan_key needs to be updated.
>>
>> * This is not generic routine, it expects the idxrel to be replication
>> * identity of a rel and meet all limitations associated with that.
>>
> It is implicit that a primary key can be a replica identity so I think this
> comment is fine.
I like your idea of modifying the assert instead of completely removing.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com