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.fujii@oss.nttdata.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