Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT - Mailing list pgsql-hackers
| From | Matheus de Oliveira |
|---|---|
| Subject | Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT |
| Date | |
| Msg-id | CAJghg4K7+J0RTfB8h-FgsUCFTuc5Jggf9w39LWqQp4AQLu7JdQ@mail.gmail.com Whole thread Raw |
| In response to | Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT (Tom Lane <tgl@sss.pgh.pa.us>) |
| Responses |
Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT
(Tom Lane <tgl@sss.pgh.pa.us>)
|
| List | pgsql-hackers |
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@sss.pgh.pa.us> wrote:
Matheus de Oliveira <matioli.matheus@gmail.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?)
--
Matheus de Oliveira
Attachment
pgsql-hackers by date: