[PATCH] WIP Add ALWAYS DEFERRED option for constraints

På tirsdag 03. oktober 2017 kl. 21:51:30, skrev Nico Williams <
nico@cryptonector.com <mailto:nico@cryptonector.com>>:
Attached are patches to add an ALWAYS DEFERRED option to CONSTRAINTs and
CONSTRAINT TRIGGERs, meaning: SET CONSTRAINTS .. IMMEDIATE will not make
immediate any constraint/trigger that is declared as ALWAYS DEFERRED.

I.e., the opposite of NOT DEFERRED.  Perhaps I should make this NOT
IMMEDIATE?  Making it NOT IMMEDIATE has the benefit of not having to
change the precedence of ALWAYS to avoid a shift/reduce conflict...  It
may also be more in keeping with NOT DEFERRED.

Motivation:

 - I have trigger procedures that must run at the end of the transaction
   (after the last statement prior to COMMIT sent by the client/user),
   which I make DEFERRABLE, INITIALLY DEFERRED CONSTRAINT TRIGGERs out
   of, but SET CONSTRAINTS can be used to foil my triggers.  I have
   written SQL code to detect that constraint triggers have fired too
   soon, but I'd rather not need it.

 - Symmetry.  If we can have NOT DEFERRABLE constraints, why not also
   NOT IMMEDIABLE?  :)  Naturally "immediable" is not a word, but you
   get the point.

 - To learn my way around PostgreSQL source code in preparation for
   other contributions.

Anyways, this patch is NOT passing tests at the moment, and I'm not sure
why.  I'm sure I can figure it out, but first I need to understand the
failures.  E.g., I see this sort of difference:

   \d testschema.test_index1
   Index "testschema.test_index1"
    Column |  Type  | Definition
   --------+--------+------------
    id     | bigint | id
  -btree, for table "testschema.test_default_tab"
  +f, for table "testschema.btree", predicate (test_default_tab)

which means, I think, that I've screwed up in src/bin/psql/describe.c,
don't it's not obvious to me yet how.

Some questions for experienced PostgreSQL developers:

Q0: Is this sort of patch welcomed?

Q1: Should new columns for pg_catalog.pg_constraint go at the end, or may
    they be added in the middle?

Q2: Can I add new columns to information_schema tables, or are there
    standards-compliance issues with that?

Q3: Is the C-style for PG documented somewhere?  (sorry if I missed this)

Q4: Any ideas what I'm doing wrong in this patch series?

Nico
 
 
+1.
 
While we're in deferrable constraints land...;  I even more often need
deferrable conditional unique-indexes.
In PG you now may have:
ALTER TABLE email_folder ADD CONSTRAINT some_uk UNIQUE (owner_id, folder_type,
name) DEFERRABLE INITIALLY DEFERRED;
 
But this isn't supported:
CREATE UNIQUE INDEX some_uk ON email_folder(owner_id, folder_type, name) WHERE
parent_idIS NULL DEFERRABLE INITIALLY DEFERRED;  

Are there any plans to support this?
 
Thanks.

 
--
Andreas Joseph Krogh
 

On Tue, Oct 03, 2017 at 10:10:59PM +0200, Andreas Joseph Krogh wrote:

+1.

While we're in deferrable constraints land...; I even more often need
deferrable conditional unique-indexes.
In PG you now may have:
ALTER TABLE email_folder ADD CONSTRAINT some_uk UNIQUE (owner_id, folder_type,
name) DEFERRABLE INITIALLY DEFERRED;

But this isn't supported:
CREATE UNIQUE INDEX some_uk ON email_folder(owner_id, folder_type, name) WHERE
parent_id IS NULL DEFERRABLE INITIALLY DEFERRED;

Are there any plans to support this?

Not by me, but I can take a look and, if it is trivial, do it. At a
quick glance it does look like it should be easy enough to do it, at
least to get started on a patch.

If I can get some help with my current patch, I'll take a look :)

But yes, I'd like to have full consistency between CREATE and ALTER.
Everything that one can do with CREATE should be possible to do with
ALTER, including IF NOT EXISTS.

Nico
--

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Oct 03, 2017 at 02:51:30PM -0500, Nico Williams wrote:

Anyways, this patch is NOT passing tests at the moment, and I'm not sure
why. I'm sure I can figure it out, but first I need to understand the
failures. E.g., I see this sort of difference:

\d testschema.test_index1
Index "testschema.test_index1"
Column |  Type  | Definition
--------+--------+------------
id     | bigint | id
-btree, for table "testschema.test_default_tab"
+f, for table "testschema.btree", predicate (test_default_tab)

which means, I think, that I've screwed up in src/bin/psql/describe.c,
don't it's not obvious to me yet how.

Ah, I needed to adjust references to results columns. I suspect that
something similar relates to other remaining failures.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/03/2017 10:10 PM, Andreas Joseph Krogh wrote:

While we're in deferrable constraints land...;
I even more often need deferrable /conditional /unique-indexes.
In PG you now may have:

ALTER TABLE email_folder ADD CONSTRAINT some_uk UNIQUE (owner_id, folder_type, name) DEFERRABLE INITIALLY DEFERRED;

But this isn't supported:

CREATE UNIQUE INDEX some_uk ON email_folder(owner_id, folder_type, name) WHERE parent_id IS NULL DEFERRABLE INITIALLY DEFERRED;

Are there any plans to support this?

I don't want to hijack the thread, but you can do that with exclusion
constraints.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

På onsdag 04. oktober 2017 kl. 00:24:19, skrev Vik Fearing <
vik.fearing@2ndquadrant.com <mailto:vik.fearing@2ndquadrant.com>>:
On 10/03/2017 10:10 PM, Andreas Joseph Krogh wrote:

While we're in deferrable constraints land...;
I even more often need deferrable /conditional /unique-indexes.
In PG you now may have:

ALTER TABLE email_folder ADD CONSTRAINT some_uk UNIQUE (owner_id,

folder_type, name) DEFERRABLE INITIALLY DEFERRED;

 
But this isn't supported:

CREATE UNIQUE INDEX some_uk ON email_folder(owner_id, folder_type, name)

WHERE parent_id IS NULL DEFERRABLE INITIALLY DEFERRED;

Are there any plans to support this?

I don't want to hijack the thread, but you can do that with exclusion
constraints.
 
 
True.
 
--
Andreas Joseph Krogh
 

[make check-world passes. Tests and docs included. Should be ready for
code review.]

Attached are patches to add an ALWAYS DEFERRED option to CONSTRAINTs and
CONSTRAINT TRIGGERs, meaning: SET CONSTRAINTS .. IMMEDIATE will not make
immediate any constraint/trigger that is declared as ALWAYS DEFERRED.

I.e., the opposite of NOT DEFERRED.

Motivation:

- Security.

One may have triggers they need to always be deferred and they
cannot give direct PG access because of SET CONSTRAINTS .. IMMEDIATE.

I have such triggers that must run at the end of the transaction
(after the last statement prior to COMMIT sent by the client/user),
which I make DEFERRABLE, INITIALLY DEFERRED CONSTRAINT TRIGGERs.

I have written SQL code to detect that constraint triggers have fired
too soon, but I'd rather not need it as it does slow things down (it
uses DDL event triggers and per-table triggers).

Making it easier to write secure code DEFERRED CONSTRAINT TRIGGERs
seems like a good idea to me.

- Symmetry.

Not using NOT DEFERRABLE is not the inverse of NOT DEFERRABLE. There
is no inverse at this time.

If we can have NOT DEFERRABLE constraints, why not also the inverse,
a constraint that cannot be made IMMEDIATE with SET CONSTRAINTs?

I've *not* cleaned up C style issues in surrounding -- I'm not sure
if that's desired. Not cleaning up makes it easier to see what I
changed.

Some questions for experienced PostgreSQL developers:

Q0: Is this sort of patch welcomed?

Q1: Should new columns for pg_catalog tables go at the end, or may they
be added in the middle?

FYI, I'm adding them in the middle, so they are next to related
columns.

Q2: Can I add new columns to information_schema tables, or are there
standards-compliance issues with that?

This is done in the second patch, and it can be dropped safely.

Q3: Perhaps I should make this NOT IMMEDIATE rather than ALWAYS DEFERRED?
Making it NOT IMMEDIATE has the benefit of not having to change the
precedence of ALWAYS to avoid a shift/reduce conflict... It may
also be more in keeping with NOT DEFERRED. Thoughts?

Nico
--

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

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

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

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

Rebased (there were conflicts in the SGML files).

Nico
--

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

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

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

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

On Fri, Oct 20, 2017 at 9:05 AM, Nico Williams <nico@cryptonector.com> 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

On Mon, Nov 06, 2017 at 05:50:21PM +1300, Thomas Munro wrote:

On Fri, Oct 20, 2017 at 9:05 AM, Nico Williams <nico@cryptonector.com> 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

On Wed, Nov 8, 2017 at 8:30 AM, Nico Williams <nico@cryptonector.com> wrote:

On Mon, Nov 06, 2017 at 05:50:21PM +1300, Thomas Munro wrote:

On Fri, Oct 20, 2017 at 9:05 AM, Nico Williams <nico@cryptonector.com> 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!

Those are not fixed yet, and it has been three weeks since this
report, so I am marking this patch as returned with feedback. Of
course feel free to send an updated version if you can get into it.
--
Michael

[Re-send; first attempt appears to have hit /dev/null somewhere. My
apologies if you get two copies.]

I've finally gotten around to rebasing this patch and making the change
that was requested, which was: merge the now-would-be-three deferral-
related bool columns in various pg_catalog tables into one char column.

Instead of (deferrable, initdeferred, alwaysdeferred), now there is just
(deferral).

All tests (make check) pass.

Sorry for the delay in doing this!

Incidentally, I had to do commit-by-commit rebasing to make the rebase
easier. I have a shell function I use for this, if anyone wants a copy
of it -- sometimes it's much easier to do this than to do one huge jump.

Nico
--