Re: ALTER TABLE ALTER CONSTRAINT misleading error message

On 2025/06/13 16:10, jian he wrote:
> On Wed, Jun 11, 2025 at 10:20 PM Fujii Masao
> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>
>>>> We discussed this already, didn't we? There's a thread with IIRC three
>>>> proposed patches for this. I think I liked this one the most:
>>>>
>>>> https://postgr.es/m/CAAJ_b97hd-jMTS7AjgU6TDBCzDx_KyuKxG+K-DtYmOieg+giyQ@mail.gmail.com
>>>>
>>>
>>> for ALTER CONSTRAINT,
>>> we already handled most error cases in ATExecAlterConstraint.
>>>
>>> if (cmdcon->alterDeferrability && currcon->contype != CONSTRAINT_FOREIGN)
>>> ereport(ERROR,
>>> (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>>> errmsg("constraint \"%s\" of relation \"%s\" is not a
>>> foreign key constraint",
>>> cmdcon->conname, RelationGetRelationName(rel))));
>>> if (cmdcon->alterEnforceability && currcon->contype != CONSTRAINT_FOREIGN)
>>> ereport(ERROR,
>>> (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>>> errmsg("cannot alter enforceability of constraint
>>> \"%s\" of relation \"%s\"",
>>> cmdcon->conname, RelationGetRelationName(rel))));
>>> if (cmdcon->alterInheritability &&
>>> currcon->contype != CONSTRAINT_NOTNULL)
>>> ereport(ERROR,
>>> errcode(ERRCODE_WRONG_OBJECT_TYPE),
>>> errmsg("constraint \"%s\" of relation \"%s\" is not a
>>> not-null constraint",
>>> cmdcon->conname, RelationGetRelationName(rel)));
>>>
>>> but ATExecAlterConstraint didn't handle "ALTER CONSTRAINT NOT VALID",
>>> it was handled in processCASbits.
>>>
>>> so the attached minimum patch (extract from v2-0001-trial.patch)
>>> is fine for PG18, IMHO.
>>
>> + ereport(ERROR,
>> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> + errmsg("cannot alter constraint validity"),
>>
>> Since ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID isn't supported,
>> how about making the error message more specific? For example:
>>
>> "ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported"
>>
>> This would make it clearer to users what exactly isn't allowed.
>>
>
> errmsg("cannot alter constraint validity")
> can mean
> ALTER TABLE ... ALTER CONSTRAINT ... VALID
> ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID
>
> even though ALTER TABLE ... ALTER CONSTRAINT ... VALID would yield
> syntax errors.
> message saying ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID
> would make it more explicit.
>
> Hence changed to:
> errmsg("ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported")

Thanks for updating the patch!

I had overlooked that commands other than ALTER TABLE can also trigger
this error. So mentioning just ALTER TABLE ... might be a bit misleading.
Would it be better to use a message like
"ALTER action ALTER CONSTRAINT ... NOT VALID is not supported",
similar to what we already do in tablecmd.c?

Regards,

--
Fujii Masao
NTT DATA Japan Corporation