On Sun, Jan 24, 2021 at 6:51 AM Amit Langote <amitlangot...@gmail.com>
wrote:
> On Sun, Jan 24, 2021 at 11:26 AM Corey Huinker <corey.huin...@gmail.com>
> wrote:
> > On Sat, Jan 23, 2021 at 12:52 PM Zhihong Yu <z...@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?
