Re: contrib modules and relkind check

On Fri, Feb 10, 2017 at 4:29 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> 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.

You did not get completely what I wrote upthread. This check is
repeated 3 times in pg_visibility.c:
+ /* Other relkinds don't have visibility info */
+ if (rel->rd_rel->relkind != RELKIND_RELATION &&
+ rel->rd_rel->relkind != RELKIND_MATVIEW &&
+ rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("\"%s\" is not a table, materialized view, or
TOAST table",
+ RelationGetRelationName(rel))));

And this one is present 4 times in pgstatindex.c:
+ /* only the following relkinds have storage */
+ if (rel->rd_rel->relkind != RELKIND_RELATION &&
+ rel->rd_rel->relkind != RELKIND_INDEX &&
+ rel->rd_rel->relkind != RELKIND_MATVIEW &&
+ rel->rd_rel->relkind != RELKIND_SEQUENCE &&
+ rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("\"%s\" is not a table, index, materialized
view, sequence, or TOAST table",
+ RelationGetRelationName(rel))));

So the suggestion is simply to encapsulate such blocks into their own
function like check_relation_relkind, each one locally in their
respective contrib's file. And each function includes the error
message, which should use ERRCODE_WRONG_OBJECT_TYPE as errcode by the
way.

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

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.

> 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

Would it be a problem to mention this relkind as just "partitioned
table" in the error message.

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

Well, perhaps I am playing with the words in my last paragraph, but
the documentation clearly states that any relation defined with
PARTITION BY is a "partitioned table".
--
Michael