Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher - Mailing list pgsql-hackers
| From | Önder Kalacı |
|---|---|
| Subject | Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher |
| Date | |
| Msg-id | CACawEhVFE4rsNyTtQxHSshUiWE=hF-ukDfVEZBM8jSkqukgiQw@mail.gmail.com Whole thread Raw |
| In response to | RE: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher ("houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>) |
| Responses |
Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
(Amit Kapila <amit.kapila16@gmail.com>)
|
| List | pgsql-hackers |
Hi Hou zj, all
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.
Yeah, probably not useful anymore, removed.
In the earlier versions of this patch, this code block was relying on some
planner functions. In that case, it felt safer to use a memory context. Now,
it seems useless.
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.
Right, I fixed this comment. Though, are you mentioning multiple comments? I couldn't
see any other in the patch. Let me know if you see.
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
Hmm, why do you think so? IsIndexOnlyOnExpression() is used in
logical/relaton.c, and used for an assertion on execReplication.c
I think it is better suited for relaton.c, but let me know about
your perspective as well.
4.
+extern Oid GetRelationIdentityOrPK(Relation rel);
The function is only used in relation.c, so we can make it a static
function.
In the recent iteration of the patch (I think v27), we also use this
function in check_relation_updatable() in logical/worker.c.
One could argue that we can move the definition back to worker.c,
but it feels better suited for in relation.c, as the perimeter of the function
is a Rel, and the function is looking for a property of a relation.
Let me know if you think otherwise, I don't have strong opinions
on this.
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.
Given Amit's suggestion on [1], I'm planning to drop this check altogether, and
rely on table storage parameters.
(I'll incorporate these changes with a patch that I'm going to reply to Peter's e-mail).
Thanks,
Onder
Onder
pgsql-hackers by date: