Re: additional foreign key test coverage

On 2018-Dec-07, Peter Eisentraut wrote:

> On 04/12/2018 14:23, Alvaro Herrera wrote:

> > Hmm. One of the things I did for FKs on partitioned tables was remove
> > all the cases involving only unpartitioned tables, then run just the
> > foreign_key test and see what the coverage looked like -- in the first
> > versions, there were large swaths of uncovered code. That guided me to
> > add a few more tests to increase coverage in later versions. This is
> > all to say that I think it would be useful to include the case of
> > partitioned tables in the tests you add, where relevant.
>
> I'm not sure I understand where partitioned tables come in here. In
> ri_triggers.c, it's all dealing with single base tables. Certainly
> other code elsewhere needs to know about partitions.

Well, certain features (say, referential actions) needed some specific
code changes when FKs appeared in partitioned tables. I didn't notice
those at first, and only noticed when I added tests involving
partitioned tables. I'm just saying if you add for the simple case, you
might miss bugs when whatever feature you're covering is used with
partitioned tables.

I see one example right in your 0001 patch, where your code calls
ri_restrict. That one needs to add ONLY or not depending on
partitionedness. I think you don't need to do anything here because
the !is_no_action case is already covered for partitioned tables.

Another potential example in 0002 (and 0003): in the covered function we
do this,
if (ri_NullCheck(RelationGetDescr(pk_rel), old_row, riinfo, true) != RI_KEYS_NONE_NULL)
are we using the correct tuple descriptor? Keep in mind that partition
can have different column layout than parent. (In this case it's not a
problem, because the pk_rel is not yet allowed to be partitioned, so if
you commit this soon, it will be my problem not yours).

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services