v32 attached.
On 3/21/24 07:57, Peter Eisentraut wrote:
> Two more questions:
>
> 1. In ri_triggers.c ri_KeysEqual, you swap the order of arguments to ri_AttributesEqual():
>
> - if (!ri_AttributesEqual(riinfo->ff_eq_oprs[i], RIAttType(rel, attnums[i]),
> - oldvalue, newvalue))
> + if (!ri_AttributesEqual(eq_opr, RIAttType(rel, attnums[i]),
> + newvalue, oldvalue))
>
> But the declared arguments of ri_AttributesEqual() are oldvalue and newvalue, so passing them
> backwards is really confusing. And the change does matter in the tests.
>
> Can we organize this better?
I renamed the params and actually the whole function. All it's doing is execute `oldvalue op
newvalue`, casting if necessary. So I changed it to ri_CompareWithCast and added some documentation.
In an earlier version of this patch I had a separate function for the PERIOD comparison, but it's
just doing the same thing, so I think the best thing is to give the function a more accurate name
and use it.
> 2. There are some tests that error with
>
> ERROR: only b-tree indexes are supported for non-PERIOD foreign keys
>
> But this is an elog() error, so should not normally be visible. I suspect some other error should
> really show here, and the order of checks is a bit wrong or something?
At first I thought I should just make this ereport, because it is reachable now, but I didn't like
the error message or where we were reaching it. The high-level problem is defining a non-temporal FK
against a temporal PK, and we should check for that in those terms, not when looking at individual
attribute opclasses. So I added a check prior to this and gave it a more descriptive error message.
On 3/21/24 01:25, jian he wrote:
> with foreign key "no action",
> in a transaction, we can first insert foreign key data, then primary key data.
> also the update/delete can fail at the end of transaction.
>
> based on [1] explanation about the difference between "no action" and
> "restrict".
> I only refactor the v31-0002-Support-multiranges-in-temporal-FKs.patch test.
I added some tests for deferred NO ACTION checks. I added them for all of range/multirange/PERIOD. I
also adopted your change ALTERing the constraint for NO ACTION (even though it's already that), to
make each test section more independent. Your patch had a lot of other noisy changes, e.g.
whitespace and reordering lines. If there are other things you intended to add to the tests, can you
describe them?
Rebased to 7e65ad197f.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
