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.
--
Amit Langote
EDB: http://www.enterprisedb.com