Обсуждение: additional foreign key test coverage

During the development of my recent patch "unused/redundant foreign key
code" [0], I had developed a few additional test cases to increase the
coverage in ri_triggers.c.  They are in the attached patches with
explanations.  With these, coverage should be pretty complete, except
hard-to-trigger error cases.  Interested reviewers can also follow along
on coverage.postgresql.org.


[0]:
https://www.postgresql.org/message-id/flat/2fb8d28c-a4e1-f206-898b-69cd22a393a1@2ndquadrant.com/

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-Dec-04, Peter Eisentraut wrote:

> During the development of my recent patch "unused/redundant foreign key
> code" [0], I had developed a few additional test cases to increase the
> coverage in ri_triggers.c.  They are in the attached patches with
> explanations.  With these, coverage should be pretty complete, except
> hard-to-trigger error cases.  Interested reviewers can also follow along
> on coverage.postgresql.org.

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.

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

On 04/12/2018 14:23, Alvaro Herrera wrote:
> On 2018-Dec-04, Peter Eisentraut wrote:
> 
>> During the development of my recent patch "unused/redundant foreign key
>> code" [0], I had developed a few additional test cases to increase the
>> coverage in ri_triggers.c.  They are in the attached patches with
>> explanations.  With these, coverage should be pretty complete, except
>> hard-to-trigger error cases.  Interested reviewers can also follow along
>> on coverage.postgresql.org.
> 
> 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.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Hi!

I tested this patch and it applied cleanly and all tests passed. I haven't looked if the changes to tests are
reasonableor extensive to cover all aspects of what they want to cover.
 


Mitar

On 09/01/2019 09:20, Mi Tar wrote:
> I tested this patch and it applied cleanly and all tests passed. I haven't looked if the changes to tests are
reasonableor extensive to cover all aspects of what they want to cover.
 

I have committed this with additional tests for partitioned tables, as
requested by Álvaro.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services