Re: contrib modules and relkind check

On Tue, Feb 14, 2017 at 3:18 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2017/02/13 14:59, Michael Paquier wrote:
> I see, thanks for explaining. Implemented that in the attached, also
> fixing the errcode. Please see if that's what you had in mind.

Yes. That's it, the overall patch footprint is reduced.

> I was tempted for a moment to name the functions
> check_relation_has_storage() with the message "relation %s has no storage"
> and check_relation_has_visibility_info() with a similar message, but soon
> got over it. ;)

I like the format of your patch: simple and it goes to the point.

>>>>> 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?
>>
>> Checking those error code paths would be nice. It would be nice as
>> well that the relation types that are expected to be supported and
>> unsupported are checked. For pageinspect the check range is correct.
>> There are some relkinds missing in pgstatindex though.
>
> Added more tests in pgstattuple and the new ones for pg_visibility,
> although I may have overdone the latter.

A bonus idea is also to add tests for relkinds that work, with for
example the creation of a table, inserting some data in it, vacuum it,
and look at "SELECT count(*) > 0 FROM pg_visibility('foo'::regclass)".

> In certain contexts where a subset of relkinds are allowed and others are
> not or vice versa, partitioned tables are still referred to as "tables".
> That's because we still use CREATE/DROP TABLE to create/drop them and
> perhaps more to the point, their being partitioned is irrelevant.
>
> Examples of where partitioned tables are referred to as tables:
>
> [...]
>
> In other contexts, where a table's being partitioned is relevant, the
> message is shown as "relation is/is not partitioned table". Examples:
>
> [...]

Hm... It may be a good idea to be consistent on the whole system and
refer to "partitioned table" as a table without storage and used as an
entry point for partitions. The docs use this term in CREATE TABLE,
and we would finish with messages like "not a table or a partitioned
table". Extra thoughts are welcome here, the current inconsistencies
would be confusing for users.
--
Michael