On Fri, Jun 27, 2025 at 2:11 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> I had this concern because other commands, like ALTER SEQUENCE ALTER CONSTRAINT NOT VALID,
> can also hit this error, and seeing an error message that starts with ALTER TABLE ...
> in that context can be confusing. That's why I thought a message like:
>
> ERROR: ALTER action ALTER CONSTRAINT ... NOT VALID is not supported
>
> would be clearer.
>
> However, most ALTER ... commands (other than ALTER TABLE) don't support ALTER CONSTRAINT
> at all, not just the NOT VALID clause. So in those cases, I think we should use
> the existing error handling for other commands and emit an error like:
>
> ERROR: ALTER action ALTER CONSTRAINT cannot be performed on relation ...
>
> Only in the case of ALTER TABLE should we produce a more specific message, like:
>
> ERROR: ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported
>
> 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.
>
I was thinking
ALTER SEQUENCE ALTER CONSTRAINT NOT VALID
is a corner case, maybe not worth addressing.
With your patch, the error handling in ATExecAlterConstraint.
ALTER SEQUENCE ALTER CONSTRAINT NOT VALID behave the same as the master.
The only loss is that the error position would not print out, I guess
that is fine.
one typo:
+ /*
+ * Mark whether NOT VALID was specified. If true, an error
+ * will be raised later in ATExecAlterConstraint().
+ * processCASbits() is not used to set skip_validation,
+ * as it may set it to true for unrelated reasons, even if
+ * NOT VALID wan't specified.
+ */
+ if ($4 & CAS_NOT_VALID)
+ c->skip_validation = true;
"wan't" should be "wasn't".
Other than that, it looks good to me.
