On Tue, 7 Mar 2023 at 19:17, Önder Kalacı <onderkal...@gmail.com> wrote: > > Hi Amit, Peter > >> >> > > Let me give an example to demonstrate why I thought something is fishy >> > > here: >> > > >> > > Imagine rel has a (non-default) REPLICA IDENTITY with Oid=1111. >> > > Imagine the same rel has a PRIMARY KEY with Oid=2222. >> > > > > > Hmm, alright, this is syntactically possible, but not sure if any user > would do this. Still thanks for catching this. > > And, you are right, if a user has created such a schema, > IdxIsRelationIdentityOrPK() > would return the wrong result and we'd use sequential scan instead of index > scan. > This would be a regression. I think we should change the function. > > > Here is the example: > DROP TABLE tab1; > CREATE TABLE tab1 (a int NOT NULL); > CREATE UNIQUE INDEX replica_unique ON tab1(a); > ALTER TABLE tab1 REPLICA IDENTITY USING INDEX replica_unique; > ALTER TABLE tab1 ADD CONSTRAINT pkey PRIMARY KEY (a); > > > I'm attaching v35.
Few comments 1) Maybe this change is not required: fallback if no other solution is possible. If a replica identity other than <quote>full</quote> is set on the publisher side, a replica identity - comprising the same or fewer columns must also be set on the subscriber - side. See <xref linkend="sql-altertable-replica-identity"/> for details on + comprising the same or fewer columns must also be set on the subscriber side. + See <xref linkend="sql-altertable-replica-identity"/> for details on 2) Variable declaration and the assignment can be split so that the readability is better: + + bool isUsableIndex = + IsIndexUsableForReplicaIdentityFull(indexInfo); + + index_close(indexRelation, AccessShareLock); + 3) Since there is only one statement within the if condition, the braces can be removed + if (is_btree && !is_partial && !is_only_on_expression) + { + return true; + } 4) There is minor indentation issue in this, we could run pgindent to fix it: +static Oid FindLogicalRepLocalIndex(Relation localrel, + LogicalRepRelation *remoterel); + Regards, Vignesh