Re: wrong relkind error messages

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.