On Sun, Apr 4, 2021 at 1:51 PM Amit Langote <amitlangot...@gmail.com> wrote: > > On Fri, Apr 2, 2021 at 11:55 PM Zhihong Yu <z...@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 return value. > > > > + * 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
