On Tue, Mar 7, 2023 at 9:16 AM shiy.f...@fujitsu.com <shiy.f...@fujitsu.com> wrote: > > On Monay, Mar 6, 2023 7:19 PM Önder Kalacı <onderkal...@gmail.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.
use_index_sub_amit_1.patch
Description: Binary data