|From:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>|
|To:||Matheus de Oliveira <matioli(dot)matheus(at)gmail(dot)com>|
|Cc:||Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, José Arthur Benetasso Villanova <jose(dot)arthur(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Michael Paquier <michael(at)paquier(dot)xyz>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>|
|Subject:||Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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, but I took a quick look at it
* 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.
* It also doesn't pass "git diff --check" whitespace checks, so
I ran it through pgindent.
* Grepping around for other references to struct Constraint,
I noticed that you'd missed updating equalfuncs.c. I did not
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.
We don't generally act that way in other ALTER commands 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.
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. 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.
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
regards, tom lane
|Next Message||Tom Lane||2019-07-28 18:12:07||Re: how to run encoding-dependent tests by default|
|Previous Message||Tom Lane||2019-07-28 16:42:59||Re: Add parallelism and glibc dependent only options to reindexdb|