additional foreign key test coverage

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 reasonable or 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 reasonable or 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