Re: contrib modules and relkind check

On Thu, Feb 9, 2017 at 7:21 AM, Corey Huinker <corey.huinker@gmail.com> wrote:
> Is this still needing a reviewer?

Useful input is always welcome.

> 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.

The visibility checks are localized in pg_visibility.c and the storage
checks in pgstatindex.c, so yes we could have macros in those files.
Or even better: just have a sanity check routine as the error messages
are the same everywhere.

> 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.

Yep, good point. That's easy enough to add.

By the way, partition tables create a file on disk but they should
not... Amit, I think you are working on a patch for that as well?
That's tweaking heap_create() to unlist partitioned tables and then be
sure that other code paths don't try to look at the parent partitioned
table on disk.
-- 
Michael