Re: 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