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

On Tue, Mar 7, 2023 at 9:16 AM shiy(dot)fnst(at)fujitsu(dot)com
<shiy(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Monay, Mar 6, 2023 7:19 PM Önder Kalacı <onderkalaci(at)gmail(dot)com> wrote:
> >
> > Yeah, seems easier to follow to me as well. Reflected it in the comment as well.
> >

Few comments:
=============
1.
+get_usable_indexoid(ApplyExecutionData *edata, ResultRelInfo *relinfo)
{
...
+ if (targetrelkind == RELKIND_PARTITIONED_TABLE)
+ {
+ /* Target is a partitioned table, so find relmapentry of the partition */
+ TupleConversionMap *map = ExecGetRootToChildMap(relinfo, edata->estate);
+ AttrMap *attrmap = map ? map->attrMap : NULL;
+
+ relmapentry =
+ logicalrep_partition_open(relmapentry, relinfo->ri_RelationDesc,
+ attrmap);

When will we hit this part of the code? As per my understanding, for
partitioned tables, we always follow apply_handle_tuple_routing()
which should call logicalrep_partition_open(), and do the necessary
work for us.

2. In logicalrep_partition_open(), it would be better to set
localrelvalid after finding the required index. The entry should be
marked valid after initializing/updating all the required members. I
have changed this in the attached.

3.
@@ -31,6 +32,7 @@ typedef struct LogicalRepRelMapEntry
Relation localrel; /* relcache entry (NULL when closed) */
AttrMap *attrmap; /* map of local attributes to remote ones */
bool updatable; /* Can apply updates/deletes? */
+ Oid usableIndexOid; /* which index to use, or InvalidOid if none */

Would it be better to name this new variable as localindexoid to match
it with the existing variable localreloid? Also, the camel case for
this variable appears odd.

4. If you agree with the above, then we should probably change the
name of functions get_usable_indexoid() and
FindLogicalRepUsableIndex() accordingly.

5.
+ {
+ /*
+ * If we had a primary key or relation identity with a unique index,
+ * we would have already found and returned that oid. At this point,
+ * the remote relation has replica identity full.

These comments are not required as this just states what the code just
above is doing.

Apart from the above, I have made some modifications in the other
comments. If you are convinced with those, then kindly include them in
the next version.

--
With Regards,
Amit Kapila.