Re: simplifying foreign key/RI checks

On Sun, Apr 4, 2021 at 1:51 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Fri, Apr 2, 2021 at 11:55 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> >
> > Hi,
> >
> > +       skip = !ExecLockTableTuple(erm->relation, &tid, markSlot,
> > +                                  estate->es_snapshot, estate->es_output_cid,
> > +                                  lockmode, erm->waitPolicy, &epq_needed);
> > +       if (skip)
> >
> > It seems the variable skip is only used above. The variable is not needed - if statement can directly check the
returnvalue.
 
> >
> > + *         Locks tuple with given TID with given lockmode following given wait
> >
> > given appears three times in the above sentence. Maybe the following is bit easier to read:
> >
> > Locks tuple with the specified TID, lockmode following given wait policy
> >
> > + * Checks whether a tuple containing the same unique key as extracted from the
> > + * tuple provided in 'slot' exists in 'pk_rel'.
> >
> > I think 'same' is not needed here since the remaining part of the sentence has adequately identified the key.
> >
> > +       if (leaf_pk_rel == NULL)
> > +           goto done;
> >
> > It would be better to avoid goto by including the cleanup statements in the if block and return.
> >
> > +   if (index_getnext_slot(scan, ForwardScanDirection, outslot))
> > +       found = true;
> > +
> > +   /* Found tuple, try to lock it in key share mode. */
> > +   if (found)
> >
> > Since found is only assigned in one place, the two if statements can be combined into one.
>
> Thanks for taking a look.  I agree with most of your suggestions and
> have incorporated them in the v8 just posted.

The 2nd patch does not apply on Head, please post a rebased version:
error: patch failed: src/backend/utils/adt/ri_triggers.c:337
error: src/backend/utils/adt/ri_triggers.c: patch does not apply

Regards,
Vignesh