Subject: Re: wrong relkind error messages
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <dc35a398-37d0-75ce-07ea-1dd71d98f8ec@2ndquadrant.com>
 <01d4fd55-d4fe-5afc-446c-a7f99e043f3d@enterprisedb.com>
 <YN6xchlgmG5/zGab@paquier.xyz>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Message-ID: <84b376ef-573f-47bf-fff9-797078b7146b@enterprisedb.com>
Date: Fri, 2 Jul 2021 12:53:08 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <YN6xchlgmG5/zGab@paquier.xyz>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/84b376ef-573f-47bf-fff9-797078b7146b%40enterprisedb.com>
Precedence: bulk

On 02.07.21 08:25, Michael Paquier wrote:
> +           ereport(ERROR,
> +                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                    errmsg("ALTER action %s cannot be performed on relation \"%s\"",
> +                           action_str, RelationGetRelationName(rel)),
> +                    errdetail_relkind_not_supported(rel->rd_rel->relkind)));
> Perhaps the result of alter_table_type_to_string() is worth a note for
> translators?

ok

> +       case AT_DetachPartitionFinalize:
> +           return "DETACH PARTITION FINALIZE";
> To be exact, I think that this one should be "DETACH PARTITION
> ... FINALIZE".

ok

> +       if (relkind == RELKIND_INDEX || relkind == RELKIND_PARTITIONED_INDEX)
> +           ereport(ERROR,
> +                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                    errmsg("cannot change schema of index \"%s\"",
> +                           rv->relname),
> +                    errhint("Change the schema of the table instead.")));
> +       else if (relkind == RELKIND_COMPOSITE_TYPE)
> +           ereport(ERROR,
> +                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                    errmsg("cannot change schema of composite type
> \"%s\"",
> +                           rv->relname),
> +                    errhint("Use ALTER TYPE instead.")));
> I would simplify this part by removing the errhint(), and use "cannot
> change schema of relation .." as error string, with a dose of
> errdetail_relkind_not_supported().

I aimed for parity with the error reporting in ATExecChangeOwner() here.

> +                errmsg("relation \"%s\" cannot have triggers",
> +                       RelationGetRelationName(rel)),
> Better as "cannot create/rename/remove triggers on relation \"%s\""
> for the three code paths of trigger.c?
> 
> +                errmsg("relation \"%s\" cannot have rules",
> [...]
> +                errmsg("relation \"%s\" cannot have rules",
> For rewriteDefine.c, this could be "cannot create/rename rules on
> relation".

I had it like that, but in previous reviews some people liked it better 
this way. ;-)  I tend to agree with that, since the error condition 
isn't that you can't create a rule/etc. (like, due to incorrect 
prerequisite state) but that there cannot be one ever.