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 comment*s*? 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 [1]: https://www.postgresql.org/message-id/CAA4eK1KP-sV4aER51J-2mELjNzq_zVSLf1%2BW90Vu0feo-thVNA%40mail.gmail.com
