Foreign keys and partitioned tables

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

Alvaro Herrera wrote:

This patch enables foreign key constraints to and from partitioned
tables.

This version is rebased on current master.

0001: fix for a get_relation_info bug in current master.
Posted in <20180124174134.ma4ui2kczmqwb4um@alvherre.pgsql>
0002: Allows local partitioned index to be unique;
Posted in <20180122225559.7pbzomvgp5iwmath@alvherre.pgsql>
0003: Allows FOR EACH ROW triggers on partitioned tables;
Posted in <20180123221027.2qenwwpvgplrrx3d@alvherre.pgsql>

0004: the actual matter of this thread.
0005: bugfix for 0004, after recent changes I introduced in 0004.
It's separate because I am undecided about it being the best
approach; maybe further changes in 0003 are a better approach.

No further changes from the version I posted upthread. Tests pass. I'm
going to review this code now to see what further changes are needed (at
the very least, I think some dependency changes are in order; plus need
to add a few more tests for various ri_triggers.c code paths.)

Thanks

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

On Sun, Dec 31, 2017 at 2:43 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

This patch removes all the ONLY markers from queries in ri_triggers.c.
That makes the queries work for the new use case, but I haven't figured
if it breaks things for other use cases. I suppose not, since regular
inheritance isn't supposed to allow foreign keys in the first place, but
I haven't dug any further.

I suspect that this leads to bugs under concurrency, something to do
with crosscheck_snapshot, but I couldn't say exactly what the problem
is off the top of my head. My hope is that partitioning might be
immune on the strength of knowing that any given tuple could only be
present in one particular partition, but that might be wishful
thinking.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[ Resending an email from yesterday. Something is going very wrong with
my outgoing mail provider :-( ]

Rebase of the prior code, on top of the improved row triggers posted
elsewhere. I added some more tests too, and fixed a couple of small
bugs.

(This includes the patches I just posted in the row triggers patch)

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

On 3/11/18 22:40, Alvaro Herrera wrote:

[ Resending an email from yesterday. Something is going very wrong with
my outgoing mail provider :-( ]

Rebase of the prior code, on top of the improved row triggers posted
elsewhere. I added some more tests too, and fixed a couple of small
bugs.

(This includes the patches I just posted in the row triggers patch)

Since the row triggers patch has been committed, do you plan to send an
update on this patch?

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

Peter Eisentraut wrote:

Since the row triggers patch has been committed, do you plan to send an
update on this patch?

Yes, I'll do that shortly.

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

Here's an updated version. After wasting some time trying to resolve
"minor last minute issues", I decided to reduce the scope for now: in
the current patch, it's allowed to have foreign keys in partitioned
tables, but it is not possible to have foreign keys that point to
partitioned tables. I have split out some preliminary changes that
intended to support FKs referencing partitioned tables; I intend to
propose that for early v12, to avoid spending any more time this
commitfest on that. Yes, I'm pretty disappointed about that.

0001 prohibits having foreign keys pointing to partitioned tables, as
discussed above.

0002 is a fixup for a bug in the row triggers patch: I had a restriction
earlier that triggers declared internal were not cloned, and I seem to
have lost it in rebase. Reinstate it.

0003 is the matter of interest. This is essentially the same code I
posted earlier, rebased to the committed row triggers patch, with a few
minor bug fixes and some changes in the regression tests to try and make
them more comprehensive, including leaving a partitioned table with an
FK to test that the whole pg_dump thing works via the pg_upgrade test.
An important change is that when a table containing data is attached as
a partition, the data is verified to satisfy the constraint via the
regular alter table phase 3 code.

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

On 3/29/18 23:19, Alvaro Herrera wrote:

0001 prohibits having foreign keys pointing to partitioned tables, as
discussed above.

This is already prohibited. You get an error

ERROR: cannot reference partitioned table "fk_partitioned_pk"

Your patch 0001 just adds the same error check a few lines above the
existing one, while 0003 removes the existing one.

I think you can squash 0001 and 0003 together.

0002 is a fixup for a bug in the row triggers patch: I had a restriction
earlier that triggers declared internal were not cloned, and I seem to
have lost it in rebase. Reinstate it.

Hmm, doesn't cause any test changes?

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

On 3/29/18 23:19, Alvaro Herrera wrote:

0003 is the matter of interest. This is essentially the same code I
posted earlier, rebased to the committed row triggers patch, with a few
minor bug fixes and some changes in the regression tests to try and make
them more comprehensive, including leaving a partitioned table with an
FK to test that the whole pg_dump thing works via the pg_upgrade test.

I've only read the tests so far. The functionality appears to work
correctly. It's a bit strange how the tests are split up between
alter_table.sql and foreign_key.sql, especially since the latter also
contains ALTER TABLE checks and vice versa.

Some tests are a bit redundant, e.g., this in alter_table.sql:

+-- these fail:
+INSERT INTO at_partitioned VALUES (1000, 42);
+ERROR:  insert or update on table "at_partitioned_0" violates foreign
key constraint "at_partitioned_reg1_col1_fkey"
+DETAIL:  Key (reg1_col1)=(42) is not present in table "at_regular1".

and

+INSERT INTO at_partitioned VALUES (5000, 42);
+ERROR:  insert or update on table "at_partitioned_0" violates foreign
key constraint "at_partitioned_reg1_col1_fkey"
+DETAIL:  Key (reg1_col1)=(42) is not present in table "at_regular1".

There are no documentation changes. The foreign key section in CREATE
TABLE does not contain anything about partitioned tables, which is
probably an existing omission, but it might be good to fix this up now.

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

Peter Eisentraut wrote:

On 3/29/18 23:19, Alvaro Herrera wrote:

0003 is the matter of interest. This is essentially the same code I
posted earlier, rebased to the committed row triggers patch, with a few
minor bug fixes and some changes in the regression tests to try and make
them more comprehensive, including leaving a partitioned table with an
FK to test that the whole pg_dump thing works via the pg_upgrade test.

I've only read the tests so far. The functionality appears to work
correctly. It's a bit strange how the tests are split up between
alter_table.sql and foreign_key.sql, especially since the latter also
contains ALTER TABLE checks and vice versa.

Yeah, I started by putting what I thought was going to be just ALTER
TABLE in that test, then moved to the other file and added what I
thought were more complete tests there and failed to move stuff to
alter_table. Honestly, I think these should mostly all belong in
foreign_key, but of course the line is pretty blurry as to what to put
in which file.

Some tests are a bit redundant, e.g., this in alter_table.sql:

+-- these fail:
+INSERT INTO at_partitioned VALUES (1000, 42);
+ERROR:  insert or update on table "at_partitioned_0" violates foreign
key constraint "at_partitioned_reg1_col1_fkey"
+DETAIL:  Key (reg1_col1)=(42) is not present in table "at_regular1".

and

+INSERT INTO at_partitioned VALUES (5000, 42);
+ERROR:  insert or update on table "at_partitioned_0" violates foreign
key constraint "at_partitioned_reg1_col1_fkey"
+DETAIL:  Key (reg1_col1)=(42) is not present in table "at_regular1".

Oh, right. I had some of these to support the case of a FK pointing to
a partitioned PK, but then deleted the other partitioned table that this
referred to, so the test looks kinda silly without the stuff that was
previously interspersed there.

I think I'll remove everything from alter_table and just add what's
missing to foreign_key.

There are no documentation changes. The foreign key section in CREATE
TABLE does not contain anything about partitioned tables, which is
probably an existing omission, but it might be good to fix this up now.

Good catch. I propose this in the PARTITIONED BY section:

      <para>
-      Partitioned tables do not support <literal>EXCLUDE</literal> or
-      <literal>FOREIGN KEY</literal> constraints; however, you can define
-      these constraints on individual partitions.
+      Partitioned tables do not support <literal>EXCLUDE</literal> constraints;
+      however, you can define these constraints on individual partitions.
+      Also, while it's possible to define <literal>PRIMARY KEY</literal>
+      constraints on partitioned tables, it is not supported to create foreign
+      keys cannot that reference them.  This restriction will be lifted in a
+      future release.
      </para>

I propose this under the REFERENCES clause:

      <para>
       These clauses specify a foreign key constraint, which requires
       that a group of one or more columns of the new table must only
       contain values that match values in the referenced
       column(s) of some row of the referenced table.  If the <replaceable
       class="parameter">refcolumn</replaceable> list is omitted, the
       primary key of the <replaceable class="parameter">reftable</replaceable>
       is used.  The referenced columns must be the columns of a non-deferrable
       unique or primary key constraint in the referenced table.  The user
       must have <literal>REFERENCES</literal> permission on the referenced table
       (either the whole table, or the specific referenced columns).
       Note that foreign key constraints cannot be defined between temporary
-      tables and permanent tables.
+      tables and permanent tables.  Also note that while it is permitted to
+      define a foreign key on a partitioned table, declaring a foreign key
+      that references a partitioned table is not allowed.
      <para>

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

On 3/31/18 18:21, Alvaro Herrera wrote:

Yeah, I started by putting what I thought was going to be just ALTER
TABLE in that test, then moved to the other file and added what I
thought were more complete tests there and failed to move stuff to
alter_table. Honestly, I think these should mostly all belong in
foreign_key,

right

<para>
-      Partitioned tables do not support <literal>EXCLUDE</literal> or
-      <literal>FOREIGN KEY</literal> constraints; however, you can define
-      these constraints on individual partitions.
+      Partitioned tables do not support <literal>EXCLUDE</literal> constraints;
+      however, you can define these constraints on individual partitions.
+      Also, while it's possible to define <literal>PRIMARY KEY</literal>
+      constraints on partitioned tables, it is not supported to create foreign
+      keys cannot that reference them.  This restriction will be lifted in a

This doesn't read correctly.

+ future release.
</para>

-      tables and permanent tables.
+      tables and permanent tables.  Also note that while it is permitted to
+      define a foreign key on a partitioned table, declaring a foreign key
+      that references a partitioned table is not allowed.
<para>

Maybe use "possible" or "supported" instead of "allowed" and "permitted"
in this and similar cases.

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

Comments on the code:

@@ -7226,12 +7235,23 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab,
Relation rel,
     * numbers)
     */
    if (pkrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+   {
+       /* fix recursion in ATExecValidateConstraint to enable this case */
+       if (fkconstraint->skip_validation && !fkconstraint->initially_valid)
+           ereport(ERROR,
+                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                    errmsg("cannot add NOT VALID foreign key to
relation \"%s\"",
+                           RelationGetRelationName(pkrel))));
+   }

Maybe this error message should be more along the lines of "is not
supported yet". Also, this restriction should perhaps be mentioned in
the documentation additions that we have been discussing.

The first few hunks in ri_triggers.c (the ones that refer to the
pktable) are apparently for the postponed part of the feature, foreign
keys referencing partitioned tables. So I think those hunks should be
dropped from this patch.

The removal of the ONLY for the foreign key queries also affects
inherited tables, in undesirable ways. For example, consider this
script:

create table test1 (a int primary key);
insert into test1 values (1), (2), (3);

create table test2 (x int primary key, y int references test1 on delete
cascade);
insert into test2 values (11, 1), (22, 2), (33, 3);

create table test2a () inherits (test2);
insert into test2a values (111, 1), (222, 2);

delete from test1 where a = 1;

select * from test1 order by 1;
select * from test2 order by 1, 2;

With the patch, this will have deleted the (111, 1) record in test2a,
which it did not do before.

I think the trigger functions need to look up whether the table is a
partitioned table and decide whether the use ONLY based on that.
(This will probably also apply to the PK side, when that is
implemented.)

In pg_dump, maybe this can be refined:

+       /*
+        * For partitioned tables, it doesn't work to emit constraints
as not
+        * inherited.
+        */
+       if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
+           only = "";
+       else
+           only = "ONLY ";

We use ONLY for foreign keys on inherited tables, but foreign keys are
not inherited anyway, so this is at best future-proofing. We could
just drop the ONLY unconditionally. Or at least explain this more.

Other than that, it looks OK.

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

While adding some more tests for the "action" part (i.e. updates and
deletes on the referenced table) I came across a bug that was causing
the server to crash ... but it's actually a preexisting bug in an
assert. The fix is in 0001.

0002 I already posted: don't clone row triggers that are declared
internal. As you noted, there is no test that changes because of this.
I haven't tried yet; the only case that comes to mind is doing something
with a deferred unique constraint. Anyway, it was clear to me from the
beginning that cloning internal triggers was not going to work for the
FK constraints.

0003 is the main patch, which is a bit changed from v4, notably to cover
your review comments:

Peter Eisentraut wrote:

-      tables and permanent tables.
+      tables and permanent tables.  Also note that while it is permitted to
+      define a foreign key on a partitioned table, declaring a foreign key
+      that references a partitioned table is not allowed.
<para>

Maybe use "possible" or "supported" instead of "allowed" and "permitted"
in this and similar cases.

Fixed.

@@ -7226,12 +7235,23 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab,
Relation rel,
* numbers)
*/
if (pkrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+   {
+       /* fix recursion in ATExecValidateConstraint to enable this case */
+       if (fkconstraint->skip_validation && !fkconstraint->initially_valid)
+           ereport(ERROR,
+                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                    errmsg("cannot add NOT VALID foreign key to
relation \"%s\"",
+                           RelationGetRelationName(pkrel))));
+   }

Maybe this error message should be more along the lines of "is not
supported yet".

I added errdetail("This feature is not yet supported on partitioned tables.")))

Also, this restriction should perhaps be mentioned in
the documentation additions that we have been discussing.

Added a note in ALTER TABLE.

The first few hunks in ri_triggers.c (the ones that refer to the
pktable) are apparently for the postponed part of the feature, foreign
keys referencing partitioned tables. So I think those hunks should be
dropped from this patch.

Yeah, reverted.

The removal of the ONLY for the foreign key queries also affects
inherited tables, in undesirable ways. For example, consider this
script: [...]

With the patch, this will have deleted the (111, 1) record in test2a,
which it did not do before.

I think the trigger functions need to look up whether the table is a
partitioned table and decide whether the use ONLY based on that.
(This will probably also apply to the PK side, when that is
implemented.)

Updated this. I added you test script to inherit.sql.

In pg_dump, maybe this can be refined:

+       /*
+        * For partitioned tables, it doesn't work to emit constraints
as not
+        * inherited.
+        */
+       if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
+           only = "";
+       else
+           only = "ONLY ";

We use ONLY for foreign keys on inherited tables, but foreign keys are
not inherited anyway, so this is at best future-proofing. We could
just drop the ONLY unconditionally. Or at least explain this more.

Yeah, I admit this is a bit weird. I expanded the comment but didn't
change the code:

/*
* Foreign keys on partitioned tables are always declared as inheriting
* to partitions; for all other cases, emit them as applying ONLY
* directly to the named table, because that's how they work for
* regular inherited tables.
*/

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

Alvaro Herrera wrote:

While adding some more tests for the "action" part (i.e. updates and
deletes on the referenced table) I came across a bug that was causing
the server to crash ... but it's actually a preexisting bug in an
assert. The fix is in 0001.

Yeah, it's a live bug that only manifests on Assert-enabled builds.
Here's an example:

create table pk (a int, b int, c int, d int primary key);
create table fk (d int references pk);
insert into pk values (1, 2, 3, 4);
insert into fk values (4);
delete from pk;

Will fix

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

Peter Eisentraut wrote:

0002 is a fixup for a bug in the row triggers patch: I had a restriction
earlier that triggers declared internal were not cloned, and I seem to
have lost it in rebase. Reinstate it.

Hmm, doesn't cause any test changes?

Here's a test case:

create table t (a int) partition by range (a);
create table t1 partition of t for values from (0) to (1000);
alter table t add constraint uniq unique (a) deferrable;
create table t2 partition of t for values from (1000) to (2000);
create table t3 partition of t for values from (2000) to (3000) partition by range (a);
create table t33 partition of t3 for values from (2000) to (2100);

Tables t and t1 have one trigger; tables t2 and t3 have two triggers;
table t33 has three triggers:

alvherre=# select tgrelid::regclass, count(*) from pg_trigger where tgrelid::regclass in ('t', 't1', 't2', 't3', 't33') group by tgrelid;
tgrelid │ count
─────────┼───────
t │ 1
t1 │ 1
t2 │ 2
t3 │ 2
t33 │ 3
(5 filas)

These triggers probably all do the same thing, so there is no
correctness issue -- only speed. I suppose it's not impossible to
construct a case that shows some actual breakage -- I just don't know
how.

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

On 4/3/18 15:11, Alvaro Herrera wrote:

0003 is the main patch, which is a bit changed from v4, notably to cover
your review comments:

Looks good now.

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

Robert Haas wrote:

On Sun, Dec 31, 2017 at 2:43 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

This patch removes all the ONLY markers from queries in ri_triggers.c.
That makes the queries work for the new use case, but I haven't figured
if it breaks things for other use cases. I suppose not, since regular
inheritance isn't supposed to allow foreign keys in the first place, but
I haven't dug any further.

I suspect that this leads to bugs under concurrency, something to do
with crosscheck_snapshot, but I couldn't say exactly what the problem
is off the top of my head. My hope is that partitioning might be
immune on the strength of knowing that any given tuple could only be
present in one particular partition, but that might be wishful
thinking.

I think you're thinking of this problem: if I insert a row in
partitioned table F, and simultaneously remove the referenced row from
table P, it is possible that we fail to reject the insertion in some
corner-case scenario. I suppose it's not completely far-fetched, if P
is partitioned. I don't see any way in which it could be a problem if
only F is partitioned.

For the record: in the patch I'm about to push, I did not implement
foreign key references to partitioned tables. So it should be safe.
More thought may be needed to implement the other direction. Offhand, I
don't see a problem, but I may well be wrong.

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

Peter Eisentraut wrote:

On 4/3/18 15:11, Alvaro Herrera wrote:

0003 is the main patch, which is a bit changed from v4, notably to cover
your review comments:

Looks good now.

Thanks, pushed.

I added a couple of test cases for ON UPDATE/DELETE and MATCH PARTIAL,
after noticing that ri_triggers.c could use some additional coverage
after deleting the parts of it that did not correspond to partitioned
tables. I think it is possible to keep adding tests, if someone really
wanted to.

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

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Thanks, pushed.

This has broken the selinux regression tests, evidently because it
removed ONLY from the emitted FK test queries. While we could change
the expected results, I would first like to hear a defense of why that
change is a good idea. It seems highly likely to be the wrong thing
for non-partitioned cases.

regards, tom lane