Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints
On Mon, Nov 06, 2017 at 05:50:21PM +1300, Thomas Munro wrote: > On Fri, Oct 20, 2017 at 9:05 AM, Nico Williams wrote: > > Rebased (there were conflicts in the SGML files). > > Hi Nico > > FYI that version has some stray absolute paths in constraints.source: > > -COPY COPY_TBL FROM '@abs_srcdir@/data/constro.data'; > +COPY COPY_TBL FROM > '/home/nico/ws/postgres/src/test/regress/data/constro.data'; > > -COPY COPY_TBL FROM '@abs_srcdir@/data/constrf.data'; > +COPY COPY_TBL FROM > '/home/nico/ws/postgres/src/test/regress/data/constrf.data'; Oops! Thanks for catching that! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints
On Fri, Oct 20, 2017 at 9:05 AM, Nico Williams wrote: > Rebased (there were conflicts in the SGML files). Hi Nico FYI that version has some stray absolute paths in constraints.source: -COPY COPY_TBL FROM '@abs_srcdir@/data/constro.data'; +COPY COPY_TBL FROM '/home/nico/ws/postgres/src/test/regress/data/constro.data'; -COPY COPY_TBL FROM '@abs_srcdir@/data/constrf.data'; +COPY COPY_TBL FROM '/home/nico/ws/postgres/src/test/regress/data/constrf.data'; -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints
On Fri, Nov 03, 2017 at 01:41:45PM -0400, Peter Eisentraut wrote: > On 11/2/17 16:54, Nico Williams wrote: > > Replacing condeferred and condeferrable with a char columns also > > occurred to me, and though I assume backwards-incompatible changes to > > pg_catalog tables are fair game, I assumed everyone would prefer > > avoiding such changes where possible. > > I don't think there is an overriding mandate to avoid such catalog > changes. Consider old clients that don't know about your new column. > They might look at the catalog entries and derive information about a > constraint, not being aware that there is additional information in > another column that overrides that. So in such cases it's arguably > better to make a break. Makes sense. > (In any case, it might be worth waiting for a review of the rest of the > patch before taking on a significant rewrite of the catalog structures.) I'll wait then :) When you're done with that I'll make this change (replacing those three bool columns with a single char column). > > Hmmm, must I do anything special about _downgrades_? Does PostgreSQL > > support downgrades? > > no Oh good. Thanks for clarifying that. Nico -- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints
On 11/2/17 16:54, Nico Williams wrote: > Replacing condeferred and condeferrable with a char columns also > occurred to me, and though I assume backwards-incompatible changes to > pg_catalog tables are fair game, I assumed everyone would prefer > avoiding such changes where possible. I don't think there is an overriding mandate to avoid such catalog changes. Consider old clients that don't know about your new column. They might look at the catalog entries and derive information about a constraint, not being aware that there is additional information in another column that overrides that. So in such cases it's arguably better to make a break. (In any case, it might be worth waiting for a review of the rest of the patch before taking on a significant rewrite of the catalog structures.) > Hmmm, must I do anything special about _downgrades_? Does PostgreSQL > support downgrades? no -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints
On Thu, Nov 02, 2017 at 04:20:19PM -0400, Peter Eisentraut wrote: > I haven't really thought about this feature too hard; I just want to > give you a couple of code comments. Thanks! > I think the catalog structure, and relatedly also the parser structures, > could be made more compact. We currently have condeferrable and > condeferred to represent three valid states (NOT DEFERRABLE, DEFERRABLE > INITIALLY IMMEDIATE, DEFERRABLE INITIALLY DEFERRED). You are adding > conalwaysdeferred, but you are adding only additional state (ALWAYS > DEFERRED). So we end up with three bool fields to represent four > states. I think this should all be rolled into one char field with four > states. I thought about this. I couldn't see a way to make the two existing boolean columns have a combination meaning "ALWAYS DEFERRED" that might not break something else. Since (condeferred AND NOT condeferrable) is an impossible combination today, I could use it to mean ALWAYS DEFERRED. I'm not sure how safe that would be. And it does seem like a weird way to express ALWAYS DEFERRED, though it would work. Replacing condeferred and condeferrable with a char columns also occurred to me, and though I assume backwards-incompatible changes to pg_catalog tables are fair game, I assumed everyone would prefer avoiding such changes where possible. Also, a backwards-incompatible change to the table would significantly enlarge the patch, as more version checks would be needed, particularly regarding upgrades (which are otherwise trivial). I felt adding a new column was probably safest. I'll make a backwards- incompatible change if requested, naturally, but I guess I'd want to get wider consensus on that, as I fear others may not agree. That fear may just be due to my ignorance of the community's preference as to pg_catalog backwards-compatibility vs. cleanliness. Hmmm, must I do anything special about _downgrades_? Does PostgreSQL support downgrades? > In psql and pg_dump, if you are query new catalog fields, you need to > have a version check to have a different query for >=PG11. (This would > likely apply whether you adopt my suggestion above or not.) Ah, yes, of course. I will add such a check. > Maybe a test case in pg_dump would be useful. Will do. > Other than that, this looks like a pretty complete patch. Thanks for the review! It's a small-ish patch, and my very first for PG. It was fun writing it. I greatly appreciate that PG source is easy to read. Nico -- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints
I haven't really thought about this feature too hard; I just want to give you a couple of code comments. I think the catalog structure, and relatedly also the parser structures, could be made more compact. We currently have condeferrable and condeferred to represent three valid states (NOT DEFERRABLE, DEFERRABLE INITIALLY IMMEDIATE, DEFERRABLE INITIALLY DEFERRED). You are adding conalwaysdeferred, but you are adding only additional state (ALWAYS DEFERRED). So we end up with three bool fields to represent four states. I think this should all be rolled into one char field with four states. In psql and pg_dump, if you are query new catalog fields, you need to have a version check to have a different query for >=PG11. (This would likely apply whether you adopt my suggestion above or not.) Maybe a test case in pg_dump would be useful. Other than that, this looks like a pretty complete patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints
Rebased (there were conflicts in the SGML files). Nico -- >From 80d284ecefa22945d507d2822f1f1a195e2af751 Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Tue, 3 Oct 2017 00:33:09 -0500 Subject: [PATCH] Add ALWAYS DEFERRED option for CONSTRAINTs and CONSTRAINT TRIGGERs. This is important so that one can have triggers and constraints that must run after all of the user/client's statements in a transaction (i.e., at COMMIT time), so that the user/client may make no further changes (triggers, of course, still can). --- doc/src/sgml/catalogs.sgml | 17 - doc/src/sgml/ref/alter_table.sgml | 4 +- doc/src/sgml/ref/create_table.sgml | 10 ++- doc/src/sgml/ref/create_trigger.sgml | 2 +- doc/src/sgml/trigger.sgml | 1 + src/backend/bootstrap/bootparse.y | 2 + src/backend/catalog/heap.c | 1 + src/backend/catalog/index.c| 8 +++ src/backend/catalog/information_schema.sql | 8 +++ src/backend/catalog/pg_constraint.c| 2 + src/backend/catalog/toasting.c | 2 +- src/backend/commands/indexcmds.c | 2 +- src/backend/commands/tablecmds.c | 20 +- src/backend/commands/trigger.c | 28 +++-- src/backend/commands/typecmds.c| 3 + src/backend/nodes/copyfuncs.c | 3 + src/backend/nodes/outfuncs.c | 4 ++ src/backend/parser/gram.y | 99 ++ src/backend/parser/parse_utilcmd.c | 46 +- src/backend/utils/adt/ruleutils.c | 4 ++ src/bin/pg_dump/pg_dump.c | 31 -- src/bin/pg_dump/pg_dump.h | 2 + src/bin/psql/describe.c| 34 +++--- src/bin/psql/tab-complete.c| 4 +- src/include/catalog/index.h| 2 + src/include/catalog/pg_constraint.h| 42 +++-- src/include/catalog/pg_constraint_fn.h | 1 + src/include/catalog/pg_trigger.h | 16 ++--- src/include/commands/trigger.h | 1 + src/include/nodes/parsenodes.h | 6 +- src/include/utils/reltrigger.h | 1 + src/test/regress/input/constraints.source | 51 +++ src/test/regress/output/constraints.source | 54 +++- 33 files changed, 418 insertions(+), 93 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index ef60a58..1bc35dc 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -,6 +,13 @@ SCRAM-SHA-256$:&l + conalwaysdeferred + bool + + Is the constraint always deferred? + + + convalidated bool @@ -6968,6 +6975,13 @@ SCRAM-SHA-256$ :&l + tgalwaysdeferred + bool + + True if constraint trigger is always deferred + + + tgnargs int2 @@ -7029,7 +7043,8 @@ SCRAM-SHA-256$ :&l When tgconstraint is nonzero, tgconstrrelid, tgconstrindid, -tgdeferrable, and tginitdeferred are +tgdeferrable, tginitdeferred, and +tgalwaysdeferred are largely redundant with the referenced pg_constraint entry. However, it is possible for a non-deferrable trigger to be associated with a deferrable constraint: foreign key constraints can have some diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index b4b8dab..fe24521 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -55,7 +55,7 @@ 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 [ DEFERRABLE | NOT DEFERRABLE | ALWAYS DEFERRED ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ] VALIDATE CONSTRAINT constraint_name DROP CONSTRAINT [ IF EXISTS ] constraint_name [ RESTRICT | CASCADE ] DISABLE TRIGGER [ trigger_name | ALL | USER ] @@ -89,7 +89,7 @@ ALTER TABLE [ IF EXISTS ] name [ CONSTRAINT constraint_name ] { UNIQUE | PRIMARY KEY } USING INDEX index_name -[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ] +[ DEFERRABLE | NOT DEFERRABLE | ALWAYS DEFERRED ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ] diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml index 2db2e9f..cf1ba1c 100644 --- a/doc/src/sgml/ref/create_table.sgml +++ b/doc/src/sgml/ref/create_table.sgml @@ -67,7 +67,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI PRIMARY KEY index_parameters | REFERENCES reftable [ ( refcolumn ) ] [ MATCH FULL | MATC
Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints
FYI, I've added my patch to the commitfest. https://commitfest.postgresql.org/15/1319/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints
I accidentally typoed when saving a file. Here's the new patch with that typo corrected, changes to information_schema dropped, and with the addition of tab completion of ALWAYS DEFERRED in psql. Nico -- >From 97d3db0be9307eff5919821db7fc437da52ef7e3 Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Tue, 3 Oct 2017 00:33:09 -0500 Subject: [PATCH] Add ALWAYS DEFERRED option for CONSTRAINTs and CONSTRAINT TRIGGERs. This is important so that one can have triggers and constraints that must run after all of the user/client's statements in a transaction (i.e., at COMMIT time), so that the user/client may make no further changes (triggers, of course, still can). --- doc/src/sgml/catalogs.sgml | 17 - doc/src/sgml/ref/alter_table.sgml | 4 +- doc/src/sgml/ref/create_table.sgml | 10 ++- doc/src/sgml/ref/create_trigger.sgml | 2 +- doc/src/sgml/trigger.sgml | 1 + src/backend/bootstrap/bootparse.y | 2 + src/backend/catalog/heap.c | 1 + src/backend/catalog/index.c| 8 +++ src/backend/catalog/information_schema.sql | 8 +++ src/backend/catalog/pg_constraint.c| 2 + src/backend/catalog/toasting.c | 2 +- src/backend/commands/indexcmds.c | 2 +- src/backend/commands/tablecmds.c | 20 +- src/backend/commands/trigger.c | 28 +++-- src/backend/commands/typecmds.c| 3 + src/backend/nodes/copyfuncs.c | 3 + src/backend/nodes/outfuncs.c | 4 ++ src/backend/parser/gram.y | 99 ++ src/backend/parser/parse_utilcmd.c | 46 +- src/backend/utils/adt/ruleutils.c | 4 ++ src/bin/pg_dump/pg_dump.c | 31 -- src/bin/pg_dump/pg_dump.h | 2 + src/bin/psql/describe.c| 34 +++--- src/bin/psql/tab-complete.c| 4 +- src/include/catalog/index.h| 2 + src/include/catalog/pg_constraint.h| 42 +++-- src/include/catalog/pg_constraint_fn.h | 1 + src/include/catalog/pg_trigger.h | 16 ++--- src/include/commands/trigger.h | 1 + src/include/nodes/parsenodes.h | 6 +- src/include/utils/reltrigger.h | 1 + src/test/regress/input/constraints.source | 51 +++ src/test/regress/output/constraints.source | 54 +++- 33 files changed, 418 insertions(+), 93 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 9af77c1..2c3ed23 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -2202,6 +2202,13 @@ SCRAM-SHA-256$: < + conalwaysdeferred + bool + + Is the constraint always deferred? + + + convalidated bool @@ -6948,6 +6955,13 @@ SCRAM-SHA-256$ : < + tgalwaysdeferred + bool + + True if constraint trigger is always deferred + + + tgnargs int2 @@ -7009,7 +7023,8 @@ SCRAM-SHA-256$ : < When tgconstraint is nonzero, tgconstrrelid, tgconstrindid, -tgdeferrable, and tginitdeferred are +tgdeferrable, tginitdeferred, and +tgalwaysdeferred are largely redundant with the referenced pg_constraint entry. However, it is possible for a non-deferrable trigger to be associated with a deferrable constraint: foreign key constraints can have some diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 0fb385e..e81d1fa 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -55,7 +55,7 @@ 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 [ DEFERRABLE | NOT DEFERRABLE | ALWAYS DEFERRED ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ] VALIDATE CONSTRAINT constraint_name DROP CONSTRAINT [ IF EXISTS ] constraint_name [ RESTRICT | CASCADE ] DISABLE TRIGGER [ trigger_name | ALL | USER ] @@ -89,7 +89,7 @@ ALTER TABLE [ IF EXISTS ] name [ CONSTRAINT constraint_name ] { UNIQUE | PRIMARY KEY } USING INDEX index_name -[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ] +[ DEFERRABLE | NOT DEFERRABLE | ALWAYS DEFERRED ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ] diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml index 1477288..38c88b8 100644 --- a/doc/src/sgml/ref/create_table.sgml +++ b/doc/src/sgml/ref/create_table.sgml @@ -67,7 +67,7 @@ CREATE [ [ GL
Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints
Ah, David Fetter points out that I should also update tabe completion for psql. I'll do that at some point. I notice there's no table completion for column constraint attributes... If it's obvious enough I'll try to fix that too. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints
Ay, NOT WIP -- I left that in the Subject: by accident. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
