Re: [HACKERS] contrib modules and relkind check

On 2017/02/13 14:59, Michael Paquier wrote:
> On Fri, Feb 10, 2017 at 4:29 PM, Amit Langote wrote:
>> On 2017/02/10 14:32, Michael Paquier wrote:
>>> 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.

I see, thanks for explaining.  Implemented that in the attached, also
fixing the errcode.  Please see if that's what you had in mind.

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

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

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

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:

1. The following check in get_relation_by_qualified_name():

case OBJECT_TABLE:
    if (relation->rd_rel->relkind != RELKIND_RELATION &&
        relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
        ereport(ERROR,
                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                 errmsg("\"%s\" is not a table",
                        RelationGetRelationName(relation))));

2. The following in DefineQueryRewrite() (from the rewriter's point of view):

/*
 * Verify relation is of a type that rules can sensibly be applied to.
 * Internal callers can target materialized views, but transformRuleStmt()
 * blocks them for users.  Don't mention them in the error message.
 */
if (event_relation->rd_rel->relkind != RELKIND_RELATION &&
    event_relation->rd_rel->relkind != RELKIND_MATVIEW &&
    event_relation->rd_rel->relkind != RELKIND_VIEW &&
    event_relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
    ereport(ERROR,
            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
             errmsg("\"%s\" is not a table or view",
                    RelationGetRelationName(event_relation))));


In other contexts, where a table's being partitioned is relevant, the
message is shown as "relation is/is not partitioned table".  Examples:

1. The following check in DefineIndex():

else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
    ereport(ERROR,
            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
             errmsg("cannot create index on partitioned table \"%s\"",
                    RelationGetRelationName(rel))));

2. One in get_raw_page_internal():

if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
    ereport(ERROR,
            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
             errmsg("cannot get raw page from partitioned table \"%s\"",
                    RelationGetRelationName(rel))));

3. On the other hand, the following check in transformAttachPartition()
prevents an attempt to attach a partition to a a regular table:

if (parentRel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
    ereport(ERROR,
            (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
             errmsg("\"%s\" is not partitioned",
                    RelationGetRelationName(parentRel))));

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

Yes, however as I tried to describe above, the term used in error messages
differs from context to context.  I can see that a more consistent
user-facing terminology is important irrespective of the internal
implementation details.

Updated patch attached.

Thanks,
Amit

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers