Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT

Sorry about the long delay in answering that, I hope to get to a consensus
on how to do that feature, which I think it is really valuable. Sending few
options and observations bellow...

On Sun, Jul 28, 2019 at 2:37 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Matheus de Oliveira <matioli(dot)matheus(at)gmail(dot)com> writes:
> > [ postgresql-alter-constraint.v5.patch ]
>
> Somebody seems to have marked this Ready For Committer without posting
> any review, which is not very kosher,

Sorry. I know Lucas, will talk to him for a better review ;D

> but I took a quick look at it
> anyway.
>
>
Thank you so much by that.

* It's failing to apply, as noted by the cfbot, because somebody added
> an unrelated test to the same spot in foreign_key.sql. I fixed that
> in the attached rebase.
>
>
That was a mistake on rebase, sorry.

> * It also doesn't pass "git diff --check" whitespace checks, so
> I ran it through pgindent.
>
>
Still learning here, will take more care.

> * Grepping around for other references to struct Constraint,
> I noticed that you'd missed updating equalfuncs.c. I did not
> fix that.
>
>
Certainly true, I fixed that just to keep it OK for now.

> The main issue I've got though is a definitional one --- I'm not at all
> sold on your premise that constraint deferrability syntax should mean
> different things depending on the previous state of the constraint.
>

I see the point, but I really believe we should have a simpler way to
change just specific properties
of the constraint without touching the others, and I do believe it is
valuable. So I'd like to check with
you all what would be a good option to have that.

Just as a demonstration, and a PoC, I have changed the patch to accept two
different syntaxes:
1. The one we have today with ALTER CONSTRAINT, and it change every
constraint property
2. A similar one with SET keyword in the middle, to force changing only the
given properties, e.g.:
ALTER TABLE table_name ALTER CONSTRAINT constr_name *SET* ON UPDATE
CASCADE;

I'm not at all happy with the syntax, doens't seem very clear. But I
proceeded this way nonetheless
just to verify the code on tablecmds.c would work. Please, does NOT
consider the patch as "ready",
it is more like a WIP and demonstration now (specially the test part, which
is no longer complete,
and gram.y that I changed the lazy way - both easy to fix if the syntax is
good).

I would really appreciate opinions on that, and I'm glad to work on a
better patch after we decide
the best syntax and approach.

> We don't generally act that way in other ALTER commands

That is true. I think one exception is ALTER COLUMN, which just acts on the
changes explicitly provided.
And I truly believe most people would expect changes on only provided
information on ALTER CONSTRAINT
as well. But I have no real research on that, more like a feeling :P

> and I don't see
> a strong argument to start doing so here. If you didn't do this then
> you wouldn't (I think) need the extra struct Constraint fields in the
> first place, which is why I didn't run off and change equalfuncs.c.
>
>
Indeed true, changes on `Constraint` struct were only necessary due to
that, the patch would in fact
be way simpler without it (that is why I still insist on finding some way
to make it happen, perhaps
with a better syntax).

> In short, I'm inclined to argue that this variant of ALTER TABLE
> should replace *all* the fields of the constraint with the same
> properties it'd have if you'd created it fresh using the same syntax.
> This is by analogy to CREATE OR REPLACE commands, which don't
> preserve any of the old properties of the replaced object.

I agree for CREATE OR REPLACE, but in my POV REPLACE makes it clearer to
the user that
*everything* is changed, ALTER not so much. Again, this is just *my
opinion*, not a very strong
one though, but following first messages on that thread current behaviour
can be easily confused
with a bug (although it is not, the code clear shows it is expected,
specially on tests).

> Given
> the interactions between these fields, I think you're going to end up
> with a surprising mess of ad-hoc choices if you do differently.
> Indeed, you already have, but I think it'll get worse if anyone
> tries to extend the feature set further.
>
>
Certainly agree with that, the code is harder that way, as I said above.
Still thinking that
having the option is valuable though, we should be able to find a better
syntax/approach
for that.

> Perhaps the right way to attack it, given that, is to go ahead and
> invent "ALTER TABLE t ADD OR REPLACE CONSTRAINT c ...". At least
> in the case at hand with FK constraints, we could apply suitable
> optimizations (ie skip revalidation) when the new definition shares
> the right properties with the old, and otherwise treat it like a
> drop-and-add.
>

I believe this path is quite easy for me to do now, if you all agree it is
a good approach.
What worries me is that we already have ALTER CONSTRAINT syntax, so what
would
we do with that? I see a few options:
1. Leave ALTER CONSTRAINT to only change given properties (as I proposed at
first), and let
ADD OR REPLACE to do a full change
2. Have only ADD OR REPLACE and deprecate ALTER CONSTRAINT (I think it is
too harsh
for users already using it, a big compatibility change)
3. Just have both syntaxes and add a syntax similar to the SET I'm sending
here to keep
current properties (works well, but the syntax seems ugly to me, better
ideas?)

Best regards,
--
Matheus de Oliveira