On Wed, Mar 8, 2023 at 9:09 AM Peter Smith <smithpb2...@gmail.com> wrote: > > > ====== > src/backend/executor/execReplication.c > > 3. build_replindex_scan_key > > { > Oid operator; > Oid opfamily; > RegProcedure regop; > - int pkattno = attoff + 1; > - int mainattno = indkey->values[attoff]; > - Oid optype = get_opclass_input_type(opclass->values[attoff]); > + int table_attno = indkey->values[index_attoff]; > + Oid optype = get_opclass_input_type(opclass->values[index_attoff]); > > These variable declarations might look tidier if you kept all the Oids > together. >
I am not sure how much that would be an improvement over the current but that will lead to an additional churn in the patch. > ====== > src/backend/replication/logical/worker.c > > 6. FindReplTupleInLocalRel > > static bool > FindReplTupleInLocalRel(EState *estate, Relation localrel, > LogicalRepRelation *remoterel, > Oid localidxoid, > TupleTableSlot *remoteslot, > TupleTableSlot **localslot) > { > bool found; > > /* > * Regardless of the top-level operation, we're performing a read here, so > * check for SELECT privileges. > */ > TargetPrivilegesCheck(localrel, ACL_SELECT); > > *localslot = table_slot_create(localrel, &estate->es_tupleTable); > > Assert(OidIsValid(localidxoid) || > (remoterel->replident == REPLICA_IDENTITY_FULL)); > > if (OidIsValid(localidxoid)) > found = RelationFindReplTupleByIndex(localrel, localidxoid, > LockTupleExclusive, > remoteslot, *localslot); > else > found = RelationFindReplTupleSeq(localrel, LockTupleExclusive, > remoteslot, *localslot); > > return found; > } > > ~ > > Since that 'found' variable is not used, you might as well remove the > if/else and simplify the code. > Hmm, but that is an existing style/code, and this patch has done nothing which requires that change. Personally, I find the current style better for the readability purpose. -- With Regards, Amit Kapila.