RE: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher - Mailing list pgsql-hackers

On Thursday, March 2, 2023 11:23 PM Önder Kalacı <onderkalaci@gmail.com>  wrote:

> Both the patches are numbered 0001. It would be better to number them
> as 0001 and 0002.
> 
> Alright, attached v27_0001_use_index_on_subs_when_pub_rep_ident_full.patch and 
> v27_0002_use_index_on_subs_when_pub_rep_ident_full.patch.
> 
> I also added one more test which Andres asked me on a private chat
> (Testcase start: SUBSCRIPTION USES INDEX WITH PUB/SUB different data).

Thanks for updating the patch. I think this patch can bring noticeable
performance improvements in some use cases.

And here are few comments after reading the patch.

1.
+    usableIndexContext = AllocSetContextCreate(CurrentMemoryContext,
+                                               "usableIndexContext",
+                                               ALLOCSET_DEFAULT_SIZES);
+    oldctx = MemoryContextSwitchTo(usableIndexContext);
+
+    /* Get index list of the local relation */
+    indexlist = RelationGetIndexList(localrel);
+    Assert(indexlist != NIL);
+
+    foreach(lc, indexlist)

Is it necessary to create a memory context here ? I thought the memory will be
freed after this apply action soon.

2.

+            /*
+             * Furthermore, because primary key and unique key indexes can't
+             * include expressions we also sanity check the index is neither
+             * of those kinds.
+             */
+            Assert(!IdxIsRelationIdentityOrPK(rel, idxrel->rd_id));

It seems you mean "replica identity key" instead of "unique key" in the comments.


3.
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
...
+extern bool IsIndexOnlyOnExpression(IndexInfo *indexInfo);

The definition function seems better to be placed in execReplication.c

4.

+extern Oid GetRelationIdentityOrPK(Relation rel);

The function is only used in relation.c, so we can make it a static
function.


5.

+    /*
+     * If index scans are disabled, use a sequential scan.
+     *
+     * Note that we do not use index scans below when enable_indexscan is
+     * false. Allowing primary key or replica identity even when index scan is
+     * disabled is the legacy behaviour. So we hesitate to move the below
+     * enable_indexscan check to be done earlier in this function.
+     */
+    if (!enable_indexscan)
+        return InvalidOid;

Since the document of enable_indexscan says "Enables or disables the query
planner's use of index-scan plan types. The default is on.", and we don't use
planner here, so I am not sure should we allow/disallow index scan in apply
worker based on this GUC.

Best Regards,
Hou zj