Hi, + 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'. + 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). Cheers On Fri, Jan 22, 2021 at 11:10 PM Amit Langote <amitlangot...@gmail.com> wrote: > On Fri, Jan 22, 2021 at 3:22 PM Corey Huinker <corey.huin...@gmail.com> > wrote: > >> I decided not to deviate from pk_ terminology so that the new code > >> doesn't look too different from the other code in the file. Although, > >> I guess we can at least call the main function > >> ri_ReferencedKeyExists() instead of ri_PrimaryKeyExists(), so I've > >> changed that. > > > > I think that's a nice compromise, it makes the reader aware of the > concept. > >> > >> I've attached the updated patch. > > > > Missing "break" added. Check. > > Comment updated. Check. > > Function renamed. Check. > > Attribute mapping matching test (and assertion) added. Check. > > Patch applies to an as-of-today master, passes make check and check > world. > > No additional regression tests required, as no new functionality is > introduced. > > No docs required, as there is nothing user-facing. > > Thanks for the review. > > > Questions: > > 1. There's a palloc for mapped_partkey_attnums, which is never freed, is > the prevailing memory context short lived enough that we don't care? > > 2. Same question for the AtrrMap map, should there be a free_attrmap(). > > I hadn't checked, but yes, the prevailing context is > AfterTriggerTupleContext, a short-lived one that is reset for every > trigger event tuple. I'm still inclined to explicitly free those > objects, so changed like that. While at it, I also changed > mapped_partkey_attnums to root_partattrs for readability. > > Attached v4. > -- > Amit Langote > EDB: http://www.enterprisedb.com >