On Mon, Feb 6, 2017 at 4:01 AM, Amit Langote <langote_amit...@lab.ntt.co.jp>
wrote:
> 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...@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.
>
> https://commitfest.postgresql.org/13/988/
>
> Thanks,
> Amit
>
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.
