Re: ALTER TABLE ALTER CONSTRAINT misleading error message

On 2025-Jun-27, Fujii Masao wrote:

> 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
wouldmuch prefer to use the patch I mentioned near the beginning of the thread.
 
> 
> Are you referring to v2-0001-trial.patch proposed at [1]?

Yeah.

> 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.

Sure, the patch was developed before those options were added, so I
meant to reference the general approach rather than the specifics.

> 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.

Yeah, I remember having this in mind when I read Amul's patch back in
the day, too.

> Jian He already proposed this approach in a patch at [2].
>
> 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?

Ah, okay, yes I like this approach better.

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

Okay, here's my take on this, wherein I reworded the proposed error
message.  I also handled the NOT VALID case of a constraint trigger; maybe my
patch is too focused on that specific bit and instead we should handle
also NO INHERIT and NOT ENFORCED cases, not really sure (it's certainly
not an important part of this patch).

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Entristecido, Wutra                     (canción de Las Barreras)
echa a Freyr a rodar
y a nosotros al mar"