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

[David Steele]

On 3/4/21 6:28 AM, Ibrar Ahmed wrote:


This patch set no longer applies
http://cfbot.cputube.org/patch_32_1533.log 



It has been over a year since the last update to this patch and it no 
longer applies, so marking Returned with Feedback.


Please resubmit to the next CF when you have a new patch.

Regards,
--
-David
da...@pgmasters.net

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

[Ibrar Ahmed]

On Mon, Aug 3, 2020 at 9:31 PM Wolfgang Walther 
wrote:

> Tom Lane:
> > We don't generally act that way in other ALTER commands and I don't see
> > a strong argument to start doing so here.  [...]
> >
> > 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.
>
> I don't think the analogy to CREATE OR REPLACE holds. Semantically
> REPLACE and ALTER are very different. Using ALTER the expectation is to
> change something, keeping everything else unchanged. Looking at all the
> other ALTER TABLE actions, especially ALTER COLUMN, it looks like every
> command does exactly one thing and not more. I don't think deferrability
> and ON UPDATE / ON CASCADE should be changed together at all, neither
> implicitly nor explicitly.
>
> There seems to be a fundamental difference between deferrability and the
> ON UPDATE/ON DELETE clauses as well - the latter only apply to FOREIGN
> KEYs, while the former apply to multiple types of constraints.
>
> Matheus de Oliveira:
> > I'm still not sure if the chosen path is the best way. But I'd be glad
> > to follow any directions we all see fit.
> >
> > For now, this patch applies two methods:
> > 1. Changes full constraint definition (which keeps compatibility with
> > current ALTER CONSTRAINT):
> >  ALTER CONSTRAINT [] [] []
> > 2. Changes only the subset explicit seem in the command (a new way, I've
> > chosen to just add SET in the middle, similar to `ALTER COLUMN ... SET
> > {DEFAULT | NOT NULL}` ):
> >  ALTER CONSTRAINT SET [] [] []
> >
> > I'm OK with changing the approach, we just need to chose the color :D
>
> The `ALTER CONSTRAINT SET [] [] []`
> has the same problem about implied changes: What happens if you only do
> e.g. ALTER CONSTRAINT SET ON UPDATE xy - will the ON DELETE part be kept
> as-is or set to the default?
>
> Also, since the ON UPDATE/ON DELETE just applies to FOREIGN KEYs and no
> other constraints, there's one level of "nesting" missing in your SET
> variant, I think.
>
> I suggest to:
>
> - keep `ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ]
> [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]` exactly as-is
>
> - add both:
>   + `ALTER CONSTRAINT constraint_name [ALTER] FOREIGN KEY ON UPDATE
> referential_action`
>   + `ALTER CONSTRAINT constraint_name [ALTER] FOREIGN KEY ON DELETE
> referential_action`
>
> This does not imply any changes, that are not in the command - very much
> analog to the ALTER COLUMN variants.
>
> This could also be extended in the future with stuff like `ALTER
> CONSTRAINT constraint_name [ALTER] FOREIGN KEY MATCH [ FULL | PARTIAL |
> SIMPLE ]`.
>
>
>

This patch set no longer applies
http://cfbot.cputube.org/patch_32_1533.log

Can we get a rebase?

I am marking the patch "Waiting on Author"

-- 
Ibrar Ahmed

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

[Wolfgang Walther]

Tom Lane:

We don't generally act that way in other ALTER commands and I don't see
a strong argument to start doing so here.  [...]

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.


I don't think the analogy to CREATE OR REPLACE holds. Semantically 
REPLACE and ALTER are very different. Using ALTER the expectation is to 
change something, keeping everything else unchanged. Looking at all the 
other ALTER TABLE actions, especially ALTER COLUMN, it looks like every 
command does exactly one thing and not more. I don't think deferrability 
and ON UPDATE / ON CASCADE should be changed together at all, neither 
implicitly nor explicitly.


There seems to be a fundamental difference between deferrability and the 
ON UPDATE/ON DELETE clauses as well - the latter only apply to FOREIGN 
KEYs, while the former apply to multiple types of constraints.


Matheus de Oliveira:
I'm still not sure if the chosen path is the best way. But I'd be glad 
to follow any directions we all see fit.


For now, this patch applies two methods:
1. Changes full constraint definition (which keeps compatibility with 
current ALTER CONSTRAINT):

     ALTER CONSTRAINT [] [] []
2. Changes only the subset explicit seem in the command (a new way, I've 
chosen to just add SET in the middle, similar to `ALTER COLUMN ... SET 
{DEFAULT | NOT NULL}` ):

     ALTER CONSTRAINT SET [] [] []

I'm OK with changing the approach, we just need to chose the color :D


The `ALTER CONSTRAINT SET [] [] []` 
has the same problem about implied changes: What happens if you only do 
e.g. ALTER CONSTRAINT SET ON UPDATE xy - will the ON DELETE part be kept 
as-is or set to the default?


Also, since the ON UPDATE/ON DELETE just applies to FOREIGN KEYs and no 
other constraints, there's one level of "nesting" missing in your SET 
variant, I think.


I suggest to:

- keep `ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ] 
[ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]` exactly as-is


- add both:
 + `ALTER CONSTRAINT constraint_name [ALTER] FOREIGN KEY ON UPDATE 
referential_action`
 + `ALTER CONSTRAINT constraint_name [ALTER] FOREIGN KEY ON DELETE 
referential_action`


This does not imply any changes, that are not in the command - very much 
analog to the ALTER COLUMN variants.


This could also be extended in the future with stuff like `ALTER 
CONSTRAINT constraint_name [ALTER] FOREIGN KEY MATCH [ FULL | PARTIAL | 
SIMPLE ]`.

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

[Matheus de Oliveira]

Hi All,

I've took some time today to rebase the patch with master. Follows attached.

I'm still not sure if the chosen path is the best way. But I'd be glad to
follow any directions we all see fit.

For now, this patch applies two methods:
1. Changes full constraint definition (which keeps compatibility with
current ALTER CONSTRAINT):
ALTER CONSTRAINT [] [] []
2. Changes only the subset explicit seem in the command (a new way, I've
chosen to just add SET in the middle, similar to `ALTER COLUMN ... SET
{DEFAULT | NOT NULL}` ):
ALTER CONSTRAINT SET [] [] []

I'm OK with changing the approach, we just need to chose the color :D

I believe this is a small change in source code, but with huge impact for
users with big tables. Would be great if it could go in PG 13.

Best regards,
-- 
Matheus de Oliveira
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index e486196477..6a51014b6f 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -56,7 +56,9 @@ ALTER TABLE [ IF EXISTS ] name
 ALTER [ COLUMN ] column_name SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
 ADD table_constraint [ NOT VALID ]
 ADD table_constraint_using_index
-ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
+ALTER CONSTRAINT constraint_name
+  [SET] [ ON DELETE action ] [ ON UPDATE action ]
+  [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
 VALIDATE CONSTRAINT constraint_name
 DROP CONSTRAINT [ IF EXISTS ]  constraint_name [ RESTRICT | CASCADE ]
 DISABLE TRIGGER [ trigger_name | ALL | USER ]
@@ -488,6 +490,14 @@ WITH ( MODULUS numeric_literal, REM
   This form alters the attributes of a constraint that was previously
   created. Currently only foreign key constraints may be altered.
  
+ 
+  If SET keyword is ommitted, the full constraint
+  definition is changed, meaning that every option mentioned is set
+  accordingly and unmentioned options are set as default built-in values,
+  just like ADD CONSTRAINT would do, see definition of
+  default values on . With
+  SET keyword, only mentioned attributes are changed.
+ 
 

 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8e35c5bd1a..70fdea680e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9509,8 +9509,43 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
  errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint",
 		cmdcon->conname, RelationGetRelationName(rel;
 
+	/*
+	 * Verify for FKCONSTR_ACTION_UNKNOWN, if found, replace by current
+	 * action. We could handle FKCONSTR_ACTION_UNKNOWN bellow, but since we
+	 * already have to handle the case of changing to the same action, seems
+	 * simpler to simple replace FKCONSTR_ACTION_UNKNOWN by the current action
+	 * here.
+	 */
+	if (cmdcon->fk_del_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_del_action = currcon->confdeltype;
+
+	if (cmdcon->fk_upd_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_upd_action = currcon->confupdtype;
+
+	/*
+	 * Do the same for deferrable attributes. But consider that if changed
+	 * only initdeferred attribute and to true, force deferrable to be also
+	 * true. On the other hand, if changed only deferrable attribute and to
+	 * false, force initdeferred to be also false.
+	 */
+	if (!cmdcon->was_deferrable_set)
+		cmdcon->deferrable = cmdcon->initdeferred ? true : currcon->condeferrable;
+
+	if (!cmdcon->was_initdeferred_set)
+		cmdcon->initdeferred = !cmdcon->deferrable ? false : currcon->condeferred;
+
+	/*
+	 * This is a safe check only, should never happen here.
+	 */
+	if (cmdcon->initdeferred && !cmdcon->deferrable)
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("constraint declared INITIALLY DEFERRED must be DEFERRABLE")));
+
 	if (currcon->condeferrable != cmdcon->deferrable ||
-		currcon->condeferred != cmdcon->initdeferred)
+		currcon->condeferred != cmdcon->initdeferred ||
+		currcon->confdeltype != cmdcon->fk_del_action ||
+		currcon->confupdtype != cmdcon->fk_upd_action)
 	{
 		HeapTuple	copyTuple;
 		HeapTuple	tgtuple;
@@ -9528,6 +9563,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 		copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple);
 		copy_con->condeferrable = cmdcon->deferrable;
 		copy_con->condeferred = cmdcon->initdeferred;
+		copy_con->confdeltype = cmdcon->fk_del_action;
+		copy_con->confupdtype = cmdcon->fk_upd_action;
 		CatalogTupleUpdate(conrel, >t_self, copyTuple);
 
 		InvokeObjectPostAlterHook(ConstraintRelationId,
@@ -9564,23 +9601,106 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 otherrelids = list_append_unique_oid(otherrelids,
 	 tgform->tgrelid);
 
-			/*
-			 * Update deferrability of RI_FKey_noaction_del,
-			 * 

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

[Tom Lane]

Matheus de Oliveira  writes:
> [ postgresql-alter-constraint.v7.patch ]

cfbot says this isn't applying --- looks like a minor conflict in
the regression test file.  Please rebase.

regards, tom lane

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

[Matheus de Oliveira]

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  wrote:

> Matheus de Oliveira  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 

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

[Alvaro Herrera from 2ndQuadrant]

Matheus, any replies to this?   I've marked the patch as Waiting on
Author for now.

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

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

[Tom Lane]

Matheus de Oliveira  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

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 90bf195..198c640 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -55,7 +55,9 @@ ALTER TABLE [ IF EXISTS ] name
 ALTER [ COLUMN ] column_name SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
 ADD table_constraint [ NOT VALID ]
 ADD table_constraint_using_index
-ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
+ALTER CONSTRAINT constraint_name
+  [ ON DELETE action ] [ ON UPDATE action ]
+  [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
 VALIDATE CONSTRAINT constraint_name
 DROP CONSTRAINT [ IF EXISTS ]  constraint_name [ RESTRICT | CASCADE ]
 DISABLE TRIGGER [ trigger_name | ALL | USER ]
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fb2be10..f897986 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9012,8 +9012,43 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
  errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint",
 		cmdcon->conname, RelationGetRelationName(rel;
 
+	/*
+	 * Verify for FKCONSTR_ACTION_UNKNOWN, if found, replace by current
+	 * action. We could handle FKCONSTR_ACTION_UNKNOWN bellow, but since we
+	 * already have to handle the case of changing to the same action, seems
+	 * simpler to simple replace FKCONSTR_ACTION_UNKNOWN by the current action
+	 * here.
+	 */
+	if (cmdcon->fk_del_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_del_action = currcon->confdeltype;
+
+	if (cmdcon->fk_upd_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_upd_action = currcon->confupdtype;
+
+	/*
+	 * Do the same for deferrable attributes. But consider that if changed
+	 * only initdeferred attribute and to true, force deferrable to be also
+	 * true. On the other hand, if changed only deferrable attribute and to
+	 * false, force initdeferred to be also false.
+	 */
+	if (!cmdcon->was_deferrable_set)
+		cmdcon->deferrable = cmdcon->initdeferred ? true : currcon->condeferrable;
+
+	if (!cmdcon->was_initdeferred_set)
+		cmdcon->initdeferred = !cmdcon->deferrable ? false : currcon->condeferred;
+
+	/*
+	 * This is a safe check only, should never happen here.
+	 */
+	if (cmdcon->initdeferred && !cmdcon->deferrable)
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("constraint declared INITIALLY DEFERRED must be DEFERRABLE")));
+
 	if (currcon->condeferrable != cmdcon->deferrable ||
-		currcon->condeferred != cmdcon->initdeferred)
+		currcon->condeferred != cmdcon->initdeferred ||
+		currcon->confdeltype != cmdcon->fk_del_action ||
+		currcon->confupdtype != cmdcon->fk_upd_action)
 	{
 		HeapTuple	copyTuple;
 		HeapTuple	tgtuple;
@@ -9031,6 +9066,8 @@ ATExecAlterConstraint(Relation rel, 

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

[Matheus de Oliveira]

On Mon, Jul 1, 2019 at 6:21 AM Thomas Munro  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
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 90bf19564c..198c640b98 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -55,7 +55,9 @@ ALTER TABLE [ IF EXISTS ] name
 ALTER [ COLUMN ] column_name SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
 ADD table_constraint [ NOT VALID ]
 ADD table_constraint_using_index
-ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
+ALTER CONSTRAINT constraint_name
+  [ ON DELETE action ] [ ON UPDATE action ]
+  [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
 VALIDATE CONSTRAINT constraint_name
 DROP CONSTRAINT [ IF EXISTS ]  constraint_name [ RESTRICT | CASCADE ]
 DISABLE TRIGGER [ trigger_name | ALL | USER ]
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 0f1a9f0e54..b54c3d67c5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8979,8 +8979,43 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
  errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint",
 		cmdcon->conname, RelationGetRelationName(rel;
 
+	/*
+	 * Verify for FKCONSTR_ACTION_UNKNOWN, if found, replace by current
+	 * action. We could handle FKCONSTR_ACTION_UNKNOWN bellow, but since
+	 * we already have to handle the case of changing to the same action,
+	 * seems simpler to simple replace FKCONSTR_ACTION_UNKNOWN by the
+	 * current action here.
+	 */
+	if (cmdcon->fk_del_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_del_action = currcon->confdeltype;
+
+	if (cmdcon->fk_upd_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_upd_action = currcon->confupdtype;
+
+	/*
+	 * Do the same for deferrable attributes. But consider that if changed
+	 * only initdeferred attribute and to true, force deferrable to be also
+	 * true. On the other hand, if changed only deferrable attribute and to
+	 * false, force initdeferred to be also false.
+	 */
+	if (!cmdcon->was_deferrable_set)
+		cmdcon->deferrable = cmdcon->initdeferred ? true : currcon->condeferrable;
+
+	if (!cmdcon->was_initdeferred_set)
+		cmdcon->initdeferred = !cmdcon->deferrable ? false : currcon->condeferred;
+
+	/*
+	 * This is a safe check only, should never happen here.
+	 */
+	if (cmdcon->initdeferred && !cmdcon->deferrable)
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("constraint declared INITIALLY DEFERRED must be DEFERRABLE")));
+
 	if (currcon->condeferrable != cmdcon->deferrable ||
-		currcon->condeferred != cmdcon->initdeferred)
+		currcon->condeferred != cmdcon->initdeferred ||
+		currcon->confdeltype != cmdcon->fk_del_action ||
+		currcon->confupdtype != cmdcon->fk_upd_action)
 	{
 		HeapTuple	copyTuple;
 		HeapTuple	tgtuple;
@@ -8998,6 +9033,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 		copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple);
 		copy_con->condeferrable = cmdcon->deferrable;
 		copy_con->condeferred = cmdcon->initdeferred;
+		copy_con->confdeltype = cmdcon->fk_del_action;
+		copy_con->confupdtype = cmdcon->fk_upd_action;
 		CatalogTupleUpdate(conrel, >t_self, copyTuple);
 
 		InvokeObjectPostAlterHook(ConstraintRelationId,
@@ -9034,23 +9071,106 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 otherrelids = list_append_unique_oid(otherrelids,
 	 tgform->tgrelid);
 
-			/*
-			 * Update deferrability of RI_FKey_noaction_del,
-			 * RI_FKey_noaction_upd, RI_FKey_check_ins and RI_FKey_check_upd
-			 * triggers, but not others; see createForeignKeyActionTriggers
-			 * and CreateFKCheckTrigger.
-			 */
-			if (tgform->tgfoid != F_RI_FKEY_NOACTION_DEL &&
-tgform->tgfoid != F_RI_FKEY_NOACTION_UPD &&
-tgform->tgfoid != F_RI_FKEY_CHECK_INS &&
-tgform->tgfoid != F_RI_FKEY_CHECK_UPD)
-continue;
-
 			copyTuple = heap_copytuple(tgtuple);
 			copy_tg = (Form_pg_trigger) GETSTRUCT(copyTuple);
 
+			/*
+			 * Set deferrability here, but note that it may be overridden bellow
+			 * if the pg_trigger entry is on the referencing table and depending
+			 * on the action used for ON UPDATE/DELETE. But for check triggers
+			 * (in the referenced table) it is kept as is (since ON
+			 * UPDATE/DELETE actions makes no difference for the check
+			 * triggers).
+			 */
 			copy_tg->tgdeferrable = cmdcon->deferrable;
 			copy_tg->tginitdeferred = cmdcon->initdeferred;
+
+			/*
+			 * Set ON DELETE action
+			 */
+			if (tgform->tgfoid == F_RI_FKEY_NOACTION_DEL ||
+tgform->tgfoid == F_RI_FKEY_RESTRICT_DEL ||
+tgform->tgfoid == F_RI_FKEY_CASCADE_DEL 

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

[Thomas Munro]

On Tue, Mar 19, 2019 at 1:04 PM Matheus de Oliveira
 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

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

[Matheus de Oliveira]

On Sun, Feb 3, 2019 at 8:28 AM Andres Freund  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
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index e360728c02..b2866f467a 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -55,7 +55,9 @@ ALTER TABLE [ IF EXISTS ] name
 ALTER [ COLUMN ] column_name SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
 ADD table_constraint [ NOT VALID ]
 ADD table_constraint_using_index
-ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
+ALTER CONSTRAINT constraint_name
+  [ ON DELETE action ] [ ON UPDATE action ]
+  [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
 VALIDATE CONSTRAINT constraint_name
 DROP CONSTRAINT [ IF EXISTS ]  constraint_name [ RESTRICT | CASCADE ]
 DISABLE TRIGGER [ trigger_name | ALL | USER ]
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 515c29072c..56847b90f9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8305,8 +8305,43 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
  errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint",
 		cmdcon->conname, RelationGetRelationName(rel;
 
+	/*
+	 * Verify for FKCONSTR_ACTION_UNKNOWN, if found, replace by current
+	 * action. We could handle FKCONSTR_ACTION_UNKNOWN bellow, but since
+	 * we already have to handle the case of changing to the same action,
+	 * seems simpler to simple replace FKCONSTR_ACTION_UNKNOWN by the
+	 * current action here.
+	 */
+	if (cmdcon->fk_del_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_del_action = currcon->confdeltype;
+
+	if (cmdcon->fk_upd_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_upd_action = currcon->confupdtype;
+
+	/*
+	 * Do the same for deferrable attributes. But consider that if changed
+	 * only initdeferred attribute and to true, force deferrable to be also
+	 * true. On the other hand, if changed only deferrable attribute and to
+	 * false, force initdeferred to be also false.
+	 */
+	if (!cmdcon->was_deferrable_set)
+		cmdcon->deferrable = cmdcon->initdeferred ? true : currcon->condeferrable;
+
+	if (!cmdcon->was_initdeferred_set)
+		cmdcon->initdeferred = !cmdcon->deferrable ? false : currcon->condeferred;
+
+	/*
+	 * This is a safe check only, should never happen here.
+	 */
+	if (cmdcon->initdeferred && !cmdcon->deferrable)
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("constraint declared INITIALLY DEFERRED must be DEFERRABLE")));
+
 	if (currcon->condeferrable != cmdcon->deferrable ||
-		currcon->condeferred != cmdcon->initdeferred)
+		currcon->condeferred != cmdcon->initdeferred ||
+		currcon->confdeltype != cmdcon->fk_del_action ||
+		currcon->confupdtype != cmdcon->fk_upd_action)
 	{
 		HeapTuple	copyTuple;
 		HeapTuple	tgtuple;
@@ -8324,6 +8359,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 		copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple);
 		copy_con->condeferrable = cmdcon->deferrable;
 		copy_con->condeferred = cmdcon->initdeferred;
+		copy_con->confdeltype = cmdcon->fk_del_action;
+		copy_con->confupdtype = cmdcon->fk_upd_action;
 		CatalogTupleUpdate(conrel, >t_self, copyTuple);
 
 		InvokeObjectPostAlterHook(ConstraintRelationId,
@@ -8360,23 +8397,106 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 otherrelids = list_append_unique_oid(otherrelids,
 	 tgform->tgrelid);
 
-			/*
-			 * Update deferrability of RI_FKey_noaction_del,
-			 * RI_FKey_noaction_upd, RI_FKey_check_ins and RI_FKey_check_upd
-			 * triggers, but not others; see createForeignKeyTriggers and
-			 * CreateFKCheckTrigger.
-			 */
-			if (tgform->tgfoid != F_RI_FKEY_NOACTION_DEL &&
-tgform->tgfoid != F_RI_FKEY_NOACTION_UPD &&
-tgform->tgfoid != F_RI_FKEY_CHECK_INS &&
-tgform->tgfoid != F_RI_FKEY_CHECK_UPD)
-continue;
-
 			copyTuple = heap_copytuple(tgtuple);
 			copy_tg = (Form_pg_trigger) GETSTRUCT(copyTuple);
 
+			/*
+			 * Set deferrability here, but note that it may be overridden bellow
+			 * if the pg_trigger entry is on the referencing table and 

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

[José Arthur Benetasso Villanova]

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

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

[Dmitry Dolgov]

> On Sun, Oct 14, 2018 at 8:30 PM Matheus de Oliveira 
>  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.

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

[Matheus de Oliveira]

On Tue, Oct 2, 2018 at 3:40 AM Michael Paquier  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
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index f13a6cd944..5910680cf3 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -55,7 +55,9 @@ ALTER TABLE [ IF EXISTS ] name
 ALTER [ COLUMN ] column_name SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
 ADD table_constraint [ NOT VALID ]
 ADD table_constraint_using_index
-ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
+ALTER CONSTRAINT constraint_name
+  [ ON DELETE action ] [ ON UPDATE action ]
+  [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
 VALIDATE CONSTRAINT constraint_name
 DROP CONSTRAINT [ IF EXISTS ]  constraint_name [ RESTRICT | CASCADE ]
 DISABLE TRIGGER [ trigger_name | ALL | USER ]
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3e112b4ef4..86dabc9bbc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7843,8 +7843,43 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
  errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint",
 		cmdcon->conname, RelationGetRelationName(rel;
 
+	/*
+	 * Verify for FKCONSTR_ACTION_UNKNOWN, if found, replace by current
+	 * action. We could handle FKCONSTR_ACTION_UNKNOWN bellow, but since
+	 * we already have to handle the case of changing to the same action,
+	 * seems simpler to simple replace FKCONSTR_ACTION_UNKNOWN by the
+	 * current action here.
+	 */
+	if (cmdcon->fk_del_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_del_action = currcon->confdeltype;
+
+	if (cmdcon->fk_upd_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_upd_action = currcon->confupdtype;
+
+	/*
+	 * Do the same for deferrable attributes. But consider that if changed
+	 * only initdeferred attribute and to true, force deferrable to be also
+	 * true. On the other hand, if changed only deferrable attribute and to
+	 * false, force initdeferred to be also false.
+	 */
+	if (!cmdcon->was_deferrable_set)
+		cmdcon->deferrable = cmdcon->initdeferred ? true : currcon->condeferrable;
+
+	if (!cmdcon->was_initdeferred_set)
+		cmdcon->initdeferred = !cmdcon->deferrable ? false : currcon->condeferred;
+
+	/*
+	 * This is a safe check only, should never happen here.
+	 */
+	if (cmdcon->initdeferred && !cmdcon->deferrable)
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("constraint declared INITIALLY DEFERRED must be DEFERRABLE")));
+
 	if (currcon->condeferrable != cmdcon->deferrable ||
-		currcon->condeferred != cmdcon->initdeferred)
+		currcon->condeferred != cmdcon->initdeferred ||
+		currcon->confdeltype != cmdcon->fk_del_action ||
+		currcon->confupdtype != cmdcon->fk_upd_action)
 	{
 		HeapTuple	copyTuple;
 		HeapTuple	tgtuple;
@@ -7862,6 +7897,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 		copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple);
 		copy_con->condeferrable = cmdcon->deferrable;
 		copy_con->condeferred = cmdcon->initdeferred;
+		copy_con->confdeltype = cmdcon->fk_del_action;
+		copy_con->confupdtype = cmdcon->fk_upd_action;
 		CatalogTupleUpdate(conrel, >t_self, copyTuple);
 
 		InvokeObjectPostAlterHook(ConstraintRelationId,
@@ -7898,23 +7935,106 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 otherrelids = list_append_unique_oid(otherrelids,
 	 tgform->tgrelid);
 
-			/*
-			 * Update deferrability of RI_FKey_noaction_del,
-			 * RI_FKey_noaction_upd, RI_FKey_check_ins and RI_FKey_check_upd
-			 * triggers, but not others; see createForeignKeyTriggers and
-			 * CreateFKCheckTrigger.
-			 */
-			if (tgform->tgfoid != F_RI_FKEY_NOACTION_DEL &&
-tgform->tgfoid != F_RI_FKEY_NOACTION_UPD &&
-tgform->tgfoid != F_RI_FKEY_CHECK_INS &&
-tgform->tgfoid != F_RI_FKEY_CHECK_UPD)
-continue;
-
 			copyTuple = heap_copytuple(tgtuple);
 			copy_tg = (Form_pg_trigger) GETSTRUCT(copyTuple);
 
+			/*
+			 * Set deferrability here, but note that it may be overridden bellow
+			 * if the pg_trigger entry is on the referencing table and depending
+			 * on the action used for ON UPDATE/DELETE. But for check triggers
+			 * (in the referenced table) it is kept as is (since ON
+			 * UPDATE/DELETE actions makes no difference for the check
+			 * triggers).
+			 */
 			copy_tg->tgdeferrable = cmdcon->deferrable;
 			copy_tg->tginitdeferred = cmdcon->initdeferred;
+
+			/*
+			 * Set ON DELETE action
+			 */
+			if (tgform->tgfoid == F_RI_FKEY_NOACTION_DEL ||
+tgform->tgfoid == F_RI_FKEY_RESTRICT_DEL ||
+

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

[Michael Paquier]

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


signature.asc
Description: PGP signature

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

[Matheus de Oliveira]

Hi all.

Sorry about the long delay.

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

> On Wed, Mar 7, 2018 at 11:49 PM, Matheus de Oliveira
>  wrote:
> >
> >
> > Em 3 de mar de 2018 19:32, "Peter Eisentraut"
> >  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
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 424a426..958f7d3 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6681,6 +6681,26 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
  errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint",
 		cmdcon->conname, RelationGetRelationName(rel;
 
+	/*
+	 * Check which deferrable attributes changed. But consider that if changed
+	 * only initdeferred attribute and to true, force deferrable to be also
+	 * true. On the other hand, if changed only deferrable attribute and to
+	 * false, force initdeferred to be also false.
+	 */
+	if (!cmdcon->was_deferrable_set)
+		cmdcon->deferrable = cmdcon->initdeferred ? true : currcon->condeferrable;
+
+	if (!cmdcon->was_initdeferred_set)
+		cmdcon->initdeferred = !cmdcon->deferrable ? false : currcon->condeferred;
+
+	/*
+	 * This is a safe check only, should never happen here.
+	 */
+	if (cmdcon->initdeferred && !cmdcon->deferrable)
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("constraint declared INITIALLY DEFERRED must be DEFERRABLE")));
+
 	if (currcon->condeferrable != cmdcon->deferrable ||
 		currcon->condeferred != cmdcon->initdeferred)
 	{
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index db492a7..2e0d68b 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2592,6 +2592,8 @@ _copyConstraint(const Constraint *from)
 	COPY_SCALAR_FIELD(deferrable);
 	COPY_SCALAR_FIELD(initdeferred);
 	COPY_LOCATION_FIELD(location);
+	COPY_SCALAR_FIELD(was_deferrable_set);
+	COPY_SCALAR_FIELD(was_initdeferred_set);
 	COPY_SCALAR_FIELD(is_no_inherit);
 	COPY_NODE_FIELD(raw_expr);
 	COPY_STRING_FIELD(cooked_expr);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 5c03e9f..78fa6fa 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2843,6 +2843,8 @@ _outConstraint(StringInfo str, const Constraint *node)
 	WRITE_BOOL_FIELD(deferrable);
 	WRITE_BOOL_FIELD(initdeferred);
 	WRITE_LOCATION_FIELD(location);
+	WRITE_BOOL_FIELD(was_deferrable_set);
+	WRITE_BOOL_FIELD(was_initdeferred_set);
 
 	appendStringInfoString(str, " :contype ");
 	

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

[Ashutosh Bapat]

On Wed, Mar 7, 2018 at 11:49 PM, Matheus de Oliveira
 wrote:
>
>
> Em 3 de mar de 2018 19:32, "Peter Eisentraut"
>  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

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

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

[Matheus de Oliveira]

Em 3 de mar de 2018 19:32, "Peter Eisentraut" <
peter.eisentr...@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,

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

[Matheus de Oliveira]

Em 2 de mar de 2018 08:15, "Andres Freund"  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,

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

[David Steele]

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
da...@pgmasters.net

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

[Peter Eisentraut]

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

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

[Andres Freund]

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

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

[Matheus de Oliveira]

On Tue, Feb 20, 2018 at 12:38 PM, Fabrízio de Royes Mello <
fabriziome...@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].

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

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

[Fabrízio de Royes Mello]

On Tue, Feb 20, 2018 at 12:10 PM, Matheus de Oliveira <
matioli.math...@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
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello