Re: simplifying foreign key/RI checks

On Sun, Jan 24, 2021 at 11:26 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
> On Sat, Jan 23, 2021 at 12:52 PM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
>>
>> Hi,

Thanks for the review.

>> + for (i = 0; i < riinfo->nkeys; i++)
>> + {
>> + Oid eq_opr = eq_oprs[i];
>> + Oid typeid = RIAttType(fk_rel, riinfo->fk_attnums[i]);
>> + RI_CompareHashEntry *entry = ri_HashCompareOp(eq_opr, typeid);
>> +
>> + if (pk_nulls[i] != 'n' && OidIsValid(entry->cast_func_finfo.fn_oid))
>>
>> It seems the pk_nulls[i] != 'n' check can be lifted ahead of the assignment to the three local variables. That way, ri_HashCompareOp wouldn't be called when pk_nulls[i] == 'n'.

Good idea, so done. Although, there can't be nulls right now.

>> + case TM_Updated:
>> + if (IsolationUsesXactSnapshot())
>> ...
>> + case TM_Deleted:
>> + if (IsolationUsesXactSnapshot())
>>
>> It seems the handling for TM_Updated and TM_Deleted is the same. The cases for these two values can be put next to each other (saving one block of code).

Ah, yes. The TM_Updated case used to be handled a bit differently in
earlier unposted versions of the patch, though at some point I
concluded that the special handling was unnecessary, but didn't
realize what you just pointed out. Fixed.

> I'll pause on reviewing v4 until you've addressed the suggestions above.

Here's v5.

--
Amit Langote
EDB: http://www.enterprisedb.com