RE: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

On Thursday, March 2, 2023 11:23 PM Önder Kalacı <onderkalaci(at)gmail(dot)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