On Tue, 7 Mar 2023 at 19:17, Önder Kalacı <onderkalaci@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
