On Wed, Mar 8, 2023 at 3:03 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> 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.
That suggestion was because IMO the 'optype' and 'opfamily' belong
together. TBH, really think the assignment of 'opttype' should happen
later with the 'opfamilly' assignment too because then it will be
*after* the (!AttributeNumberIsValid(table_attno)) check.
>
> > ======
> > 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.
>
OK. I failed to notice that was same as the existing code.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
