Thread: 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
Attachment
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