On Thursday, March 2, 2023 11:23 PM Önder Kalacı <[email protected]> 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
