Horton

Thread

Commits

  1. Restore erroneously removed ONLY from PK check

  2. Foreign keys on partitioned tables

  3. Don't clone internal triggers to partitions

  4. Pass correct TupDesc to ri_NullCheck() in Assert

  1. Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-12-31T19:43:59Z

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

Naturally, FKs that reference a partitioned table require unique
constraints, and therefore they shares the restrictions of those: in my
proposed patch, it is only possible if the partition keys are part of
the unique constraint.  That's not explicitly checked by the FK code,
but rather just an property emergent of previous patches.

As far as I can tell, no documentation changes are needed, since AFAICS
we don't claim anywhere that FKs are not supported for partitioned
tables.

pg_dump support is not yet correct here, but otherwise this feature
should work as intended, and all tests pass for me.

I haven't gone exhaustively over things such as partitions created in
odd ways, dropped columns, match partial, etc, so bugs, holes and
non-working corner cases are still expected, but please do report any
you find.

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.

Patch 0001 attached here corresponds to a squashed version of patches in
other threads; it's here just for convenience.  The patch to be reviewed
for this thread is just 0002 and corresponding functionality.

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

  2. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-12-31T19:49:08Z

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

  3. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-01-24T17:52:45Z

    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

  4. Re: Foreign keys and partitioned tables

    Robert Haas <robertmhaas@gmail.com> — 2018-01-25T14:35:26Z

    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

  5. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-03-12T02:40:23Z

    [ 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

  6. Re: Foreign keys and partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-27T14:45:17Z

    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

  7. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-03-27T14:54:53Z

    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

  8. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-03-30T03:19:53Z

    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

  9. Re: Foreign keys and partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-31T13:09:01Z

    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

  10. Re: Foreign keys and partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-31T17:01:39Z

    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

  11. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-03-31T22:21:09Z

    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

  12. Re: Foreign keys and partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-04-02T12:35:29Z

    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

  13. Re: Foreign keys and partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-04-02T14:16:01Z

    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

  14. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-03T19:11:35Z

    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

  15. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-03T20:47:23Z

    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

  16. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-03T21:49:03Z

    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

  17. Re: Foreign keys and partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-04-04T13:37:14Z

    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

  18. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-04T16:56:23Z

    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

  19. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-04T17:21:14Z

    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

  20. Re: Foreign keys and partitioned tables

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-04T18:04:10Z

    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

  21. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-04T19:42:35Z

    Tom Lane wrote:
> 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.

Yeah, there ain't one, because this was a reversal mistake.  I restored
that ONLY.  (There were two ONLYs in the original query; I initially
removed both, and then went over the file and included them
conditionally on the table not being a partitioned one, based on review
comments.  In this line I restored one conditionally but failed to
realize I should have been restoring the other unconditionally.)

Pushed a fix blind.  Let's see if it appeases rhinoceros.

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

  22. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-04T21:09:09Z

    Robert Haas wrote:

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

Speaking of crosscheck_snapshot, I just noticed that the case of FKs
with repeatable read or serializable snapshot seems not to be covered by
tests at all, judging from the coverage report:

    2635             :     /*
    2636             :      * In READ COMMITTED mode, we just need to use an up-to-date regular
    2637             :      * snapshot, and we will see all rows that could be interesting. But in
    2638             :      * transaction-snapshot mode, we can't change the transaction snapshot. If
    2639             :      * the caller passes detectNewRows == false then it's okay to do the query
    2640             :      * with the transaction snapshot; otherwise we use a current snapshot, and
    2641             :      * tell the executor to error out if it finds any rows under the current
    2642             :      * snapshot that wouldn't be visible per the transaction snapshot.  Note
    2643             :      * that SPI_execute_snapshot will register the snapshots, so we don't need
    2644             :      * to bother here.
    2645             :      */
    2646        3026 :     if (IsolationUsesXactSnapshot() && detectNewRows)
    2647             :     {
    2648           0 :         CommandCounterIncrement();  /* be sure all my own work is visible */
    2649           0 :         test_snapshot = GetLatestSnapshot();
    2650           0 :         crosscheck_snapshot = GetTransactionSnapshot();
    2651             :     }
    2652             :     else
    2653             :     {
    2654             :         /* the default SPI behavior is okay */
    2655        3026 :         test_snapshot = InvalidSnapshot;
    2656        3026 :         crosscheck_snapshot = InvalidSnapshot;
    2657             :     }
https://coverage.postgresql.org/src/backend/utils/adt/ri_triggers.c.gcov.html

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

  23. Re: Foreign keys and partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-04-20T17:39:17Z

    Alvaro Herrera wrote:
> 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.

Hello,

I won't be able to work on foreign keys pointing to partitioned tables
for the next commitfest, so if somebody is interested in seeing it
supported, I applaud they working on it and I offer a bit of time to
discuss it, if they're so inclined:

CREATE TABLE pktab (a int PRIMARY KEY) PARTITION BY RANGE (a);
... create some partitions ...

CREATE TABLE fktab (a int REFERENCES pktab);


Thanks,

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