Re: contrib modules and relkind check

On 2017/02/10 14:32, Michael Paquier wrote:
> On Thu, Feb 9, 2017 at 7:21 AM, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:

Thanks Corey and Michael for the reviews!

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

If I understand correctly what's being proposed here, tablecmds.c has
something called ATWrongRelkindError() which sounds (kind of) similar.
It's called by ATSimplePermissions() whenever it finds out that relkind of
the table specified in a given ALTER TABLE command is not in the "allowed
relkind targets" for the command. Different ALTER TABLE commands call
ATSimplePermissions() with an argument that specifies the "allowed relkind
targets" for each command. I'm not saying that it should be used
elsewhere, but if we do invent a new centralized routine for relkind
checks, it would look similar.

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

I changed the patch so that all newly added checks use an AND-ed list of
!= tests.

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

I added tests with a partitioned table to pageinspect's page.sql and
pgstattuple.sql. I don't see any tests for pg_visibility module, maybe I
should add?

Although, I felt a bit uncomfortable the way the error message looks, for
example:

+ -- check that using any of these functions with a partitioned table
would fail
+ create table test_partitioned (a int) partition by range (a);
+ select pg_relpages('test_partitioned');
+ ERROR: "test_partitioned" is not a table, index, materialized view,
sequence, or TOAST table

test_partitioned IS a table but just the kind without storage. This is
not the only place however where we do not separate the check for
partitioned tables from other unsupported relkinds in respective contexts.
Not sure if that would be a better idea.

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

Yep, I just sent a message titled "Partitioned tables and relfilenode".

Thanks,
Amit