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

On Tue, Feb 20, 2018 at 12:10 PM, Matheus de Oliveira <
matioli.matheus@gmail.com> wrote:

Hi all.

I attached a patch to add support for changing ON UPDATE/DELETE actions

of a constraint using ALTER TABLE ... ALTER CONSTRAINT.

Besides that, there is a another change in this patch on current ALTER

CONSTRAINT about deferrability options. Previously, if the user did ALTER
CONSTRAINT without specifying an option on deferrable or initdeferred, it
was implied the default options, so this:

ALTER TABLE tbl
ALTER CONSTRAINT con_name;

Was equivalent to:

ALTER TABLE tbl
ALTER CONSTRAINT con_name NOT DEFERRABLE INITIALLY IMMEDIATE;

If I kept it that way, it means that changing only ON UPDATE or ON DELETE

would cause deferrability options to be changed to the default. Now, I keep
an information of which options has actually been changed, so only the
actual changes are persisted.

But there are two exceptions (which I think that make sense):
1. If the user does only `ALTER CONSTRAINT ... INITIALLY DEFERRED`, no

matter the previous value of deferrable, it will be set to true.

2. If the user does only `ALTER CONSTRAINT ... NOT DEFERRABLE`, no matter

the previous value of initdeferred, it will be set to false.

I have pondered to raise an exception in the above cases instead of

forcing deferrable/initdeferred to valid values, but since the same
behavior happens on ADD CONSTRAINT, I think this way is simpler.

Since I'm a newbie on PG source code, this patch seems to be a bit big

for me. So please, let me know what you think about it. Specially the
change on Constraint struct on parsenode.h (and support to it on
copyfuncs.c and outfuncs.c), I'm not 100% sure that is the best way to
track if deferrability options were changed.

Thanks a lot.

Great!

I didn't read your patch yet but make sure to register it to the next open
commitfest.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

On Tue, Feb 20, 2018 at 12:38 PM, Fabrízio de Royes Mello <
fabriziomello@gmail.com> wrote:

...

I didn't read your patch yet but make sure to register it to the next open
commitfest.

Thanks a lot Fabrízio, I've done that already [1]https://commitfest.postgresql.org/17/1533/.

Please let me know if I did something wrong, and if you see improvements on
the patch ;)

[1]: https://commitfest.postgresql.org/17/1533/

Regards,
--
Matheus de Oliveira

Hi,

On 2018-02-20 12:10:22 -0300, Matheus de Oliveira wrote:

I attached a patch to add support for changing ON UPDATE/DELETE actions of
a constraint using ALTER TABLE ... ALTER CONSTRAINT.

This patch has been submitted to the last commitfest for v11 and is not
a trivial patch. As we don't accept such patches this late, it should be
moved to the next fest. Any arguments against?

Greetings,

Andres Freund

On 2/20/18 10:10, Matheus de Oliveira wrote:

Besides that, there is a another change in this patch on current ALTER
CONSTRAINT about deferrability options. Previously, if the user did
ALTER CONSTRAINT without specifying an option on deferrable or
initdeferred, it was implied the default options, so this:

    ALTER TABLE tbl
    ALTER CONSTRAINT con_name;

Was equivalent to:

    ALTER TABLE tbl
    ALTER CONSTRAINT con_name NOT DEFERRABLE INITIALLY IMMEDIATE;

Oh, that seems wrong. Probably, it shouldn't even accept that syntax
with an empty options list, let alone reset options that are not
mentioned. Can you prepare a separate patch for this issue?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Matheus,

On 3/3/18 1:32 PM, Peter Eisentraut wrote:

On 2/20/18 10:10, Matheus de Oliveira wrote:

Besides that, there is a another change in this patch on current ALTER
CONSTRAINT about deferrability options. Previously, if the user did
ALTER CONSTRAINT without specifying an option on deferrable or
initdeferred, it was implied the default options, so this:

    ALTER TABLE tbl
    ALTER CONSTRAINT con_name;

Was equivalent to:

    ALTER TABLE tbl
    ALTER CONSTRAINT con_name NOT DEFERRABLE INITIALLY IMMEDIATE;

Oh, that seems wrong. Probably, it shouldn't even accept that syntax
with an empty options list, let alone reset options that are not
mentioned. Can you prepare a separate patch for this issue?

Can you prepare the patch that Peter has requested and post on a new
thread? Please respond here with the reference (or email me directly)
and I will add to the CF.

Meanwhile, I'll push this patch to the next CF as Andres has
recommended, hearing no arguments to the contrary.

Thanks,
--
-David
david@pgmasters.net

Em 2 de mar de 2018 08:15, "Andres Freund" <andres@anarazel.de> escreveu:

Hi,

On 2018-02-20 12:10:22 -0300, Matheus de Oliveira wrote:

I attached a patch to add support for changing ON UPDATE/DELETE actions of
a constraint using ALTER TABLE ... ALTER CONSTRAINT.

This patch has been submitted to the last commitfest for v11 and is not
a trivial patch. As we don't accept such patches this late, it should be
moved to the next fest. Any arguments against?

Sorry. My bad.

I'm OK with sending this to the next one.

Best regards,

Em 3 de mar de 2018 19:32, "Peter Eisentraut" <
peter.eisentraut@2ndquadrant.com> escreveu:

On 2/20/18 10:10, Matheus de Oliveira wrote:

Besides that, there is a another change in this patch on current ALTER
CONSTRAINT about deferrability options. Previously, if the user did
ALTER CONSTRAINT without specifying an option on deferrable or
initdeferred, it was implied the default options, so this:

ALTER TABLE tbl
ALTER CONSTRAINT con_name;

Was equivalent to:

ALTER TABLE tbl
ALTER CONSTRAINT con_name NOT DEFERRABLE INITIALLY IMMEDIATE;

Oh, that seems wrong. Probably, it shouldn't even accept that syntax
with an empty options list, let alone reset options that are not
mentioned.

Yeah, it felt really weird when I noticed it. And I just noticed while
reading the source.

Can

you prepare a separate patch for this issue?

I can do that, no problem. It'll take awhile though, I'm on a trip and will
be home around March 20th.

You think this should be applied to all versions that support ALTER
CONSTRAINT, right?

Thanks.

Best regards,

Matheus de Oliveira wrote:

You think this should be applied to all versions that support ALTER
CONSTRAINT, right?

This seems a bug fix to me, so yes.

I can do that, no problem. It'll take awhile though, I'm on a trip and will
be home around March 20th.

Please do send at your earliest convenient time.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Mar 7, 2018 at 11:49 PM, Matheus de Oliveira
<matioli.matheus@gmail.com> wrote:

Em 3 de mar de 2018 19:32, "Peter Eisentraut"
<peter.eisentraut@2ndquadrant.com> escreveu:

On 2/20/18 10:10, Matheus de Oliveira wrote:

Besides that, there is a another change in this patch on current ALTER
CONSTRAINT about deferrability options. Previously, if the user did
ALTER CONSTRAINT without specifying an option on deferrable or
initdeferred, it was implied the default options, so this:

ALTER TABLE tbl
ALTER CONSTRAINT con_name;

Was equivalent to:

ALTER TABLE tbl
ALTER CONSTRAINT con_name NOT DEFERRABLE INITIALLY IMMEDIATE;

Oh, that seems wrong. Probably, it shouldn't even accept that syntax
with an empty options list, let alone reset options that are not
mentioned.

Yeah, it felt really weird when I noticed it. And I just noticed while
reading the source.

Can

you prepare a separate patch for this issue?

I can do that, no problem. It'll take awhile though, I'm on a trip and will
be home around March 20th.

Matheus,
When do you think you can provide the patch for bug fix?

Also, the patch you originally posted doesn't apply cleanly. Can you
please post a rebased version?

The patch contains 70 odd lines of test SQL and 3600 odd lines of
output. The total patch is 4200 odd lines. I don't think that it will
be acceptable to add that huge an output to the regression test. You
will need to provide a patch with much smaller output addition and may
be a smaller test as well.

You think this should be applied to all versions that support ALTER
CONSTRAINT, right?

I think so.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Hi all.

Sorry about the long delay.

On Tue, Jul 10, 2018 at 10:17 AM Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

On Wed, Mar 7, 2018 at 11:49 PM, Matheus de Oliveira
<matioli.matheus@gmail.com> wrote:

Em 3 de mar de 2018 19:32, "Peter Eisentraut"
<peter.eisentraut@2ndquadrant.com> escreveu:

On 2/20/18 10:10, Matheus de Oliveira wrote:

Besides that, there is a another change in this patch on current ALTER
CONSTRAINT about deferrability options. Previously, if the user did
ALTER CONSTRAINT without specifying an option on deferrable or
initdeferred, it was implied the default options, so this:

ALTER TABLE tbl
ALTER CONSTRAINT con_name;

Was equivalent to:

ALTER TABLE tbl
ALTER CONSTRAINT con_name NOT DEFERRABLE INITIALLY IMMEDIATE;

Oh, that seems wrong. Probably, it shouldn't even accept that syntax
with an empty options list, let alone reset options that are not
mentioned.

Yeah, it felt really weird when I noticed it. And I just noticed while
reading the source.

Can

you prepare a separate patch for this issue?

I can do that, no problem. It'll take awhile though, I'm on a trip and

will

be home around March 20th.

Matheus,
When do you think you can provide the patch for bug fix?

Attached the patch for the bug fix, all files with naming
`postgresql-fix-alter-constraint-${version}.patch`, with `${version}` since
9.4, where ALTER CONSTRAINT was introduced.

Not sure if you want to apply to master/12 as well (since the other patch
applies that as well), but I've attached it anyway, so you can decide.

Also, the patch you originally posted doesn't apply cleanly. Can you
please post a rebased version?

Attached the rebased version that applies to current master,
`postgresql-alter-constraint.v3.patch`.

The patch contains 70 odd lines of test SQL and 3600 odd lines of
output. The total patch is 4200 odd lines. I don't think that it will
be acceptable to add that huge an output to the regression test. You
will need to provide a patch with much smaller output addition and may
be a smaller test as well.

You are correct. I have made a test that tries all combinations of ALTER
CONSTRAINT ON UPDATE/DELETE ACTION, but it caused a really huge output. I
have changed that to a simple DO block, and still trying all possibilities
but now I just verify if the ALTER matches the same definition on
pg_trigger of a constraint that was created with the target action already,
seems simpler and work for any change.

Please, let me know if you all think any change should be made on this
patch.

Best regards,
--
Matheus de Oliveira

On Mon, Sep 17, 2018 at 12:03:17AM -0300, Matheus de Oliveira wrote:

You are correct. I have made a test that tries all combinations of ALTER
CONSTRAINT ON UPDATE/DELETE ACTION, but it caused a really huge output. I
have changed that to a simple DO block, and still trying all possibilities
but now I just verify if the ALTER matches the same definition on
pg_trigger of a constraint that was created with the target action already,
seems simpler and work for any change.

Please, let me know if you all think any change should be made on this
patch.

The last patch set does not apply, so this is moved to next CF, waiting
on author as new status.
--
Michael

On Tue, Oct 2, 2018 at 3:40 AM Michael Paquier <michael@paquier.xyz> wrote:

The last patch set does not apply, so this is moved to next CF, waiting
on author as new status.

Updated the last patch so it can apply cleanly on HEAD.

About the bugfixes, do you think it is better to move to another thread?

Best regards,
--
Matheus de Oliveira

On Sun, Oct 14, 2018 at 8:30 PM Matheus de Oliveira <matioli.matheus@gmail.com> wrote:

Updated the last patch so it can apply cleanly on HEAD.

About the bugfixes, do you think it is better to move to another thread?

I think it makes sense, this way discussions on two relatively different topics
will not interfere with each other. I would even suggest to create a new CF
item with "bugfix" type to emphasize it.

On Sun, 14 Oct 2018, Matheus de Oliveira wrote:

Updated the last patch so it can apply cleanly on HEAD.

Hi Matheus.

I applied your patch on top of bb874e30fbf9e85bdb117bad34865a5fae29dbf6.

It compiled, worked as expected, but some tests broke executing make
check:

test create_table ... FAILED
constraints ... FAILED
inherit ... FAILED
foreign_data ... FAILED
alter_table ... FAILED

I didn't test the bugfix, just the v3 patch.

--
José Arthur

Hi,

On 2019-01-13 20:22:32 -0200, Jos� Arthur Benetasso Villanova wrote:

On Sun, 14 Oct 2018, Matheus de Oliveira wrote:

Updated the last patch so it can apply cleanly on HEAD.

Hi Matheus.

I applied your patch on top of bb874e30fbf9e85bdb117bad34865a5fae29dbf6.

It compiled, worked as expected, but some tests broke executing make check:

test create_table ... FAILED
constraints ... FAILED
inherit ... FAILED
foreign_data ... FAILED
alter_table ... FAILED

I didn't test the bugfix, just the v3 patch.

Given that the patch hasn't been updated since, and the CF has ended,
I'm marking this patch as returned with feedback. Please resubmit once
that's fixed.

Greetings,

Andres Freund

On Sun, Feb 3, 2019 at 8:28 AM Andres Freund <andres@anarazel.de> wrote:

It compiled, worked as expected, but some tests broke executing make

check:

test create_table ... FAILED
constraints ... FAILED
inherit ... FAILED
foreign_data ... FAILED
alter_table ... FAILED

I didn't test the bugfix, just the v3 patch.

Given that the patch hasn't been updated since, and the CF has ended,
I'm marking this patch as returned with feedback. Please resubmit once
that's fixed.

Hi all.

Sorry for the long delay. I've rebased the patch to current master (at
f2004f19ed now), attached as postgresql-alter-constraint.v4.patch. All
tests passed cleanly.

Best regards,
--
Matheus de Oliveira

On Tue, Mar 19, 2019 at 1:04 PM Matheus de Oliveira
<matioli.matheus@gmail.com> wrote:

Sorry for the long delay. I've rebased the patch to current master (at f2004f19ed now), attached as postgresql-alter-constraint.v4.patch. All tests passed cleanly.

Hi Matheus,

As the commitfest is starting, could you please send a rebased patch?

Thanks,

--
Thomas Munro
https://enterprisedb.com

On Mon, Jul 1, 2019 at 6:21 AM Thomas Munro <thomas.munro@gmail.com> wrote:

Hi Matheus,

As the commitfest is starting, could you please send a rebased patch?

Hi all,

Glad to start working on that again... Follows the rebased version (at
5925e55498).

Thank you all.

Best regards,
--
Matheus de Oliveira

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, but I took a quick look at it
anyway.

* 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
fix that.

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
drop-and-add.

regards, tom lane