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

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