Thread: ALTER TABLE ALTER CONSTRAINT misleading error message
hi.
create table t(a int, constraint cc check(a = 1));
ALTER TABLE t ALTER CONSTRAINT cc not valid;
ERROR: FOREIGN KEY constraints cannot be marked NOT VALID
LINE 1: ALTER TABLE t ALTER CONSTRAINT cc not valid;
^
the error message seems misleading,
should we consider it as a bug for pg18?
the entry point is in gram.y, following part:
| ALTER CONSTRAINT name ConstraintAttributeSpec
{
AlterTableCmd *n = makeNode(AlterTableCmd);
ATAlterConstraint *c = makeNode(ATAlterConstraint);
n->subtype = AT_AlterConstraint;
n->def = (Node *) c;
c->conname = $3;
if ($4 & (CAS_NOT_ENFORCED | CAS_ENFORCED))
c->alterEnforceability = true;
if ($4 & (CAS_DEFERRABLE | CAS_NOT_DEFERRABLE |
CAS_INITIALLY_DEFERRED | CAS_INITIALLY_IMMEDIATE))
c->alterDeferrability = true;
if ($4 & CAS_NO_INHERIT)
c->alterInheritability = true;
processCASbits($4, @4, "FOREIGN KEY",
&c->deferrable,
&c->initdeferred,
&c->is_enforced,
NULL,
&c->noinherit,
yyscanner);
$$ = (Node *) n;
}
On Wed, May 28, 2025 at 7:59 PM Álvaro Herrera <alvherre@kurilemu.de> wrote: > > On 2025-May-28, jian he wrote: > > > hi. > > > > create table t(a int, constraint cc check(a = 1)); > > ALTER TABLE t ALTER CONSTRAINT cc not valid; > > ERROR: FOREIGN KEY constraints cannot be marked NOT VALID > > LINE 1: ALTER TABLE t ALTER CONSTRAINT cc not valid; > > ^ > > > > the error message seems misleading, > > 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.
Attachment
On 2025/06/02 12:13, jian he wrote: > On Wed, May 28, 2025 at 7:59 PM Álvaro Herrera <alvherre@kurilemu.de> wrote: >> >> On 2025-May-28, jian he wrote: >> >>> hi. >>> >>> create table t(a int, constraint cc check(a = 1)); >>> ALTER TABLE t ALTER CONSTRAINT cc not valid; >>> ERROR: FOREIGN KEY constraints cannot be marked NOT VALID >>> LINE 1: ALTER TABLE t ALTER CONSTRAINT cc not valid; >>> ^ >>> >>> the error message seems misleading, I also ran into this issue while testing constraints with NOT VALID. >> 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. Regards, -- Fujii Masao NTT DATA Japan Corporation
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")
Attachment
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