On 2017/01/24 15:35, Amit Langote wrote: > On 2017/01/24 15:11, Michael Paquier wrote: >> On Tue, Jan 24, 2017 at 2:14 PM, Amit Langote >> <Langote_Amit_f8@lab.ntt.co.jp> wrote: >>> Some contrib functions fail to fail sooner when relations of unsupported >>> relkinds are passed, resulting in error message like one below: >>> >>> create table foo (a int); >>> create view foov as select * from foo; >>> select pg_visibility('foov', 0); >>> ERROR: could not open file "base/13123/16488": No such file or directory >>> >>> Attached patch fixes that for all such functions I could find in contrib. >>> >>> It also installs RELKIND_PARTITIONED_TABLE as unsupported in a couple of >>> places (in pageinspect and pgstattuple). >> >> I have spent some time looking at your patch, and did not find any >> issues with it, nor did I notice code paths that were not treated or >> any other contrib modules sufferring from the same deficiencies that >> you may have missed. Nice work. > > Thanks for the review, Michael!
Added to the next CF, just so someone can decide to pick it up later.
Is this still needing a reviewer? If so, here it goes:
Patch applies.
make check-pgstattuple-recurse, check-pg_visibility-recurse, check-pageinspect-recurse all pass.
Code is quite clear. It does raise two questions:
1. should have these tests named in core functions, like maybe:
relation_has_visibility(Relation) relation_has_storage(Relation) and/or corresponding void functions check_relation_has_visibility() and check_relation_has_storage() which would raise the appropriate error message when the boolean test fails.
Then again, this might be overkill since we don't add new relkinds very often.
2. Is it better stylistically to have an AND-ed list of != tests, or a negated list of OR-ed equality tests, and should the one style be changed to conform to the other?
No new regression tests. I think we should create a dummy partitioned table for each contrib and show that it fails.