On Sun, Jan 24, 2021 at 11:26 AM Corey Huinker <corey.huinker@gmail.com> wrote: > On Sat, Jan 23, 2021 at 12:52 PM Zhihong Yu <zyu@yugabyte.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.
v5 patches apply to master.
Suggested If/then optimization is implemented.
Suggested case merging is implemented. Passes make check and make check-world yet again. Just to confirm, we don't free the RI_CompareHashEntry because it points to an entry in a hash table which is TopMemoryContext aka lifetime of the session, correct?
Anybody else want to look this patch over before I mark it Ready For Committer?
