Re: simplifying foreign key/RI checks

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 <amitlangote09(at)gmail(dot)com>
wrote:

> On Fri, Jan 22, 2021 at 3:22 PM Corey Huinker <corey(dot)huinker(at)gmail(dot)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
>