Re: ALTER TABLE ALTER CONSTRAINT misleading error message

On 2025/06/27 22:30, Álvaro Herrera wrote:
> On 2025-06-27, Fujii Masao wrote:
> 
>> To make this distinction, I just started thinking it's better to raise
>> the error
>> in ATExecAlterConstraint() rather than in gram.y. I've attached a draft
>> patch that
>> follows this approach.
> 
> Hmm I don't like this very much, it feels very kludgy. I think if we want to consider this in scope for pg18 I would
muchprefer to use the patch I mentioned near the beginning of the thread.
 

Are you referring to v2-0001-trial.patch proposed at [1]?

Regarding this patch:

-                    c->alterDeferrability = true;
-                    processCASbits($4, @4, "FOREIGN KEY",
+                    processCASbits($4, @4, NULL,
                                      &c->deferrable,
                                      &c->initdeferred,
                                      NULL, NULL, NULL, yyscanner);
+                    c->alterDeferrability = ALTER_DEFERRABILITY($4);
+                    /* cannot (currently) be changed by this syntax: */
+                    if (ALTER_ENFORCEABILITY($4))
+                        ereport(ERROR,
+                                errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("cannot alter constraint enforceability"),
+                                parser_errposition(@4));
+                    if (ALTER_VALID($4))
+                        ereport(ERROR,
+                                errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("cannot alter constraint validity"),
+                                parser_errposition(@4));
+                    if (ALTER_INHERIT($4))
+                        ereport(ERROR,
+                                errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("cannot alter constraint inheritability"),
+                                parser_errposition(@4));

This patch raises an error if ENFORCED, NOT ENFORCED, INHERIT, or NO INHERIT
is specified. But those options are currently accepted, so these checks seem
unnecessary for now.

Also, the patch raises an error for NOT VALID after calling processCASbits(),
there's no need to call processCASbits() in the first place. It would be
cleaner to check for NOT VALID and raise an error before calling it.

Jian He already proposed this approach in a patch at [2].

      {
          if (deferrable)
              *deferrable = true;
-        else
+        else if (constrType)
              ereport(ERROR,
                      (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
              /* translator: %s is CHECK, UNIQUE, or similar */

If the NOT VALID check is moved before processCASbits(), the above changes
inside processCASbits() also become unnecessary. Jian's patch doesn't change
processCASbits() this way.

It looks like Jian's patch at [2] is an updated version of the one you referred to,
so you may agree with that approach. Thought?

Just one note: Jian's patch doesn't handle the same issue for TRIGGER case,
so that part might still need to be addressed.

Regards,

[1] https://www.postgresql.org/message-id/CAAJ_b97hd-jMTS7AjgU6TDBCzDx_KyuKxG+K-DtYmOieg+giyQ@mail.gmail.com
[2] https://postgr.es/m/CACJufxH9krMV-rJkC1J=Jvy_FAO_NRVXGMV+DSNm2saHjbuw8Q@mail.gmail.com

-- 
Fujii Masao
NTT DATA Japan Corporation