Re: Foreign keys and partitioned tables
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Foreign keys and partitioned tables
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 : */
26463026 : 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 */
26553026 : test_snapshot = InvalidSnapshot;
26563026 : crosscheck_snapshot = InvalidSnapshot;
2657 : }
https://coverage.postgresql.org/src/backend/utils/adt/ri_triggers.c.gcov.html
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Foreign keys and partitioned tables
Tom Lane wrote: > Alvaro Herrera 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Foreign keys and partitioned tables
Alvaro Herrera 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
Re: Foreign keys and partitioned tables
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Foreign keys and partitioned tables
Robert Haas wrote: > On Sun, Dec 31, 2017 at 2:43 PM, Alvaro Herrera > 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Foreign keys and partitioned tables
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
Re: Foreign keys and partitioned tables
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Foreign keys and partitioned tables
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Foreign keys and partitioned tables
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.
> >
>
> 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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From d5313eea2e0196631d3269f453eb3bad86e5eca5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 3 Apr 2018 13:58:49 -0300
Subject: [PATCH v5 1/3] Ancient bug fix
---
src/backend/utils/adt/ri_triggers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/ri_triggers.c
b/src/backend/utils/adt/ri_triggers.c
index 3bb708f863..d0fe65cea9 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -514,7 +514,7 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel,
boolresult;
/* Only called for non-null rows */
- Assert(ri_NullCheck(RelationGetDescr(fk_rel), old_row, riinfo, true) ==
RI_KEYS_NONE_NULL);
+ Assert(ri_NullCheck(RelationGetDescr(pk_rel), old_row, riinfo, true) ==
RI_KEYS_NONE_NULL);
if (SPI_connect() != SPI_OK_CONNECT)
elog(ERROR, "SPI_connect failed");
--
2.11.0
>From ed65fe82a8395e086ee0ceeea6243da10c26ac2b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Mon, 26 Mar 2018 16:01:34 -0300
Subject: [PATCH v5
Re: Foreign keys and partitioned tables
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
Re: Foreign keys and partitioned tables
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 > > - Partitioned tables do not support EXCLUDE or > - FOREIGN KEY constraints; however, you can define > - these constraints on individual partitions. > + Partitioned tables do not support EXCLUDE > constraints; > + however, you can define these constraints on individual partitions. > + Also, while it's possible to define PRIMARY KEY > + 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. > > - 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. > 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
Re: Foreign keys and partitioned tables
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: - Partitioned tables do not support EXCLUDE or - FOREIGN KEY constraints; however, you can define - these constraints on individual partitions. + Partitioned tables do not support EXCLUDE constraints; + however, you can define these constraints on individual partitions. + Also, while it's possible to define PRIMARY KEY + 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. I propose this under the REFERENCES clause: 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 refcolumn list is omitted, the primary key of the reftable 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 REFERENCES 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. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Foreign keys and partitioned tables
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
Re: Foreign keys and partitioned tables
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
Re: Foreign keys and partitioned tables
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 0a9cff0ceb4f0e72c09551bcd2d40f31e245267c Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Thu, 29 Mar 2018 15:49:17 -0300
Subject: [PATCH v4 1/3] Refuse a FK pointing to a PK in a partitioned table
---
src/backend/commands/tablecmds.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 83a881eff3..d0545495ae 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7211,6 +7211,17 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab,
Relation rel,
pkrel = heap_openrv(fkconstraint->pktable,
ShareRowExclusiveLock);
/*
+* Disallow a foreign key referencing a partitioned table; supporting
this
+* case requires more work.
+*/
+ if (pkrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("\"%s\" is a partitioned table",
+ RelationGetRelationName(pkrel)),
+errdetail("Foreign keys cannot reference
partitioned tables.")));
+
+ /*
* Validity checks (permission checks wait till we have the column
* numbers)
*/
--
2.11.0
>From 7c59061ef6752cae5e96d0c0e6515a43a1f1d5e1 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Mon, 26 Mar 2018 16:01:34 -0300
Subject: [PATCH v4 2/3] don't clone internal triggers
---
src/backend/commands/tablecmds.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d0545495ae..f67eefc74c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -14347,6 +14347,10 @@ CloneRowTriggersToPartition(Relation parent, Relation
partition)
if (!TRIGGER_FOR_ROW(trigForm->tgtype))
continue;
+ /* We don't clone internal triggers, either */
+ if (trigForm->tgisinternal)
+ continue;
+
/*
* Complain if we find an unexpected trigger type.
*/
--
2.11.0
>From 0a3a3034222167c71cea8d2d5d14f6915b20e8cf Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Thu, 22 Mar 2018 14:47:12 -0300
Subject: [PATCH v4 3/3] Allow foreign key triggers on partitioned tables
---
src/backend/catalog/pg_constraint.c| 224 +
src/backend/commands/tablecmds.c | 138 +++---
src/backend/parser/parse_utilcmd.c | 12 --
src/backend/utils/adt/ri_triggers.c| 50 +++
src/bin/pg_dump/pg_dump.c | 43 --
src/include/catalog/pg_constraint_fn.h | 14 ++
src/include/commands/tablecmds.h | 4 +
src/test/regress/expected/alter_table.out | 71 -
src/test/regress/expected/create_table.out | 10 --
src/test/regress/expected/foreign_key.out | 107 ++
src/test/regress/sql/alter_table.sql | 45 +-
src/test/regress/sql/create_table.sql | 8 --
src/test/regress/sql/foreign_key.sql | 86 +++
13 files changed, 713 insertions(+), 99 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index 4f1a27a7d3..749529bf39 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -26,6
Re: Foreign keys and partitioned tables
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Foreign keys and partitioned tables
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
Re: Foreign keys and partitioned tables
[ 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >From a34e786924b54d94dbf28c182aed27d0e92dba06 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Thu, 8 Mar 2018 14:01:39 -0300 Subject: [PATCH v3 1/4] add missing CommandCounterIncrement in StorePartitionBound --- src/backend/catalog/heap.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index cf36ce4add..2b5377bdf2 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -3299,6 +3299,9 @@ StorePartitionBound(Relation rel, Relation parent, PartitionBoundSpec *bound) heap_freetuple(newtuple); heap_close(classRel, RowExclusiveLock); + /* Make update visible */ + CommandCounterIncrement(); + /* * The partition constraint for the default partition depends on the * partition bounds of every other partition, so we must invalidate the -- 2.11.0 >From 1165c0438c627ea214de9ee4cffa83d89b0aa485 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Thu, 8 Mar 2018 14:04:13 -0300 Subject: [PATCH v3 2/4] Add missing CommandCounterIncrement() in partitioned index code --- src/backend/catalog/pg_constraint.c | 4 src/backend/commands/indexcmds.c| 6 ++ src/backend/commands/tablecmds.c| 2 -- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 731c5e4317..38fdf72877 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -18,6 +18,7 @@ #include "access/heapam.h" #include "access/htup_details.h" #include "access/sysattr.h" +#include "access/xact.h" #include "catalog/dependency.h" #include "catalog/indexing.h" #include "catalog/objectaccess.h" @@ -781,6 +782,9 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId) recordDependencyOn(&depender, &referenced, DEPENDENCY_INTERNAL_AUTO); heap_close(constrRel, RowExclusiveLock); + + /* make update visible */ + CommandCounterIncrement(); } diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 504806b25b..9ca632865b 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -1003,6 +1003,9 @@ DefineIndex(Oid relationId, ReleaseSysCache(tup); heap_close(pg_index, RowExclusiveLock); heap_freetuple(newtup); + + /* make update visible */ + CommandCounterIncrement(); } } else @@ -2512,5 +2515,8 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid) recordDependencyOn(&partIdx, &partitionTbl, DEPENDENCY_AUTO); } + + /* make our updates visible */ + CommandCounterIncrement(); } } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 74e020bffc..7ecfbc17a0 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -14571,8 +14571,6 @@ ATExecAttachPartitionIdx(List **wqueue, Relation parentIdx, RangeVar *name) pfree(attmap); - CommandCounterIncrement(); - validatePartitionedIndex(parentIdx, parentTbl); } -- 2.11.0 >From ee640078fa9bd7662a28058b8b0affe49cdd336f Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Tue, 21 Nov 2017 15:53:11 -0300 Subject: [PATCH v3 3/4] Allow FOR EACH ROW triggers on partitioned tables --- src/backend/catalog/heap.c | 1 + src/backend/catalog/index.c| 4 +- src/backend/catalog/pg_constraint.c| 3 + src/backend/commands/tablecmds.c | 92 +- src/backend/commands/trigger.c | 191 ++-- src/backend/commands/typecmds.c| 1 + src/backend/tcop/utility.c | 3 +- src/include/catalog/indexing.h | 2 + src/include/catalog/pg_constraint.h| 39 ++-- src/include/catalog/pg_constraint_fn.h | 1 + src/include/commands/trigger.h | 4 +- src/test/regress/expected/triggers.out | 277 ++--- src/test/regress/input/constraints.source | 16 ++ src/test/regress/output/constraints.source | 26 +++ src/test/regress/sql/triggers.sql | 178 -- 15 files changed, 763 insertions(+), 75 deletions(-)
Re: Foreign keys and partitioned tables
On Sun, Dec 31, 2017 at 2:43 PM, Alvaro Herrera 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
Re: Foreign keys and partitioned tables
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 <2018015559.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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 53506fd3a9f0cb81334024bf5a0e8856fd8e5e82 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Wed, 24 Jan 2018 14:40:26 -0300
Subject: [PATCH v2 1/5] Ignore partitioned indexes in get_relation_info
---
src/backend/optimizer/util/plancat.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/src/backend/optimizer/util/plancat.c
b/src/backend/optimizer/util/plancat.c
index 8c60b35068..60f21711f4 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -208,6 +208,16 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId,
bool inhparent,
}
/*
+* Ignore partitioned indexes, since they are not
usable for
+* queries.
+*/
+ if (indexRelation->rd_rel->relkind ==
RELKIND_PARTITIONED_INDEX)
+ {
+ index_close(indexRelation, NoLock);
+ continue;
+ }
+
+ /*
* If the index is valid, but cannot yet be used,
ignore it; but
* mark the plan we are generating as transient. See
* src/backend/access/heap/README.HOT for discussion.
--
2.11.0
>From f67c07baa0a211a42e42b1480c4ffd47b0a7fb06 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Mon, 6 Nov 2017 17:04:55 +0100
Subject: [PATCH v2 2/5] allow indexes on partitioned tables to be unique
---
doc/src/sgml/ref/alter_table.sgml | 9 +-
doc/src/sgml/ref/create_table.sgml| 16 +-
src/backend/bootstrap/bootparse.y | 2 +
src/backend/catalog/index.c | 45 -
src/backend/catalog/pg_constraint.c | 76
src/backend/catalog/toasting.c| 4 +-
src/backend/commands/indexcmds.c | 125 +++--
src/backend/commands/tablecmds.c | 62 ++-
src/backend/parser/analyze.c | 7 +
src/backend/parser/parse_utilcmd.c| 31 +---
src/backend/tcop/utility.c| 1 +
src/include/catalog/index.h | 5 +-
src/include/catalog/pg_constraint_fn.h| 4 +-
src/include/commands/defrem.h | 1 +
src/include/parser/parse_utilcmd.h| 3 +-
src/test/regress/expected/alter_table.out | 8 -
src/test/regress/expected/create_table.out| 12 --
src/test/regress/expected/indexing.out| 254 +-
src/test/regress/expected/insert_conflict.out | 2 +-
src/test/regress/sql/alter_table.sql | 2 -
src/test/regress/sql/create_table.sql | 8 -
src/test/regress/sql/indexing.sql | 151 ++-
22 files changed, 740 insertions(+), 88 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml
b/doc/src/sgml/ref/alter_table.sgml
index 286c7a8589..c00fd09fe1 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -804,8 +804,9 @@ ALTER TABLE [ IF EXISTS ] name
This form attaches an existing table (which might itself be partitioned)
as a partition of the target table. The table can be attached
as a partition for specific values using FOR VALUES
- or as a default partition by using DEFAULT
- . For each index in the target table, a corresponding
+ or as a default partition by using
+ DEFAULT.
+ For each index in the target table, a corresponding
one will be created in the attached table; or, if an equivalent
index already exists, will be attached to the target table's index,
as if ALTER INDEX ATTACH PARTITION had been executed.
@@ -820,8 +821,10 @@ ALTE
Re: Foreign keys and partitioned tables
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
0001-Local-partitioned-indexes.patch.gz
Description: application/gunzip
>From b7e85e873ba77509793180e9076295fae2fd88a7 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 21 Nov 2017 15:54:14 -0300
Subject: [PATCH 2/2] [WIP] Allow foreign key triggers on partitioned tables
---
src/backend/catalog/pg_constraint.c| 192 +
src/backend/commands/tablecmds.c | 105 +---
src/backend/parser/parse_utilcmd.c | 12 --
src/backend/utils/adt/ri_triggers.c| 50 +++-
src/include/catalog/pg_constraint_fn.h | 2 +
src/include/commands/tablecmds.h | 4 +
src/test/regress/expected/alter_table.out | 115 -
src/test/regress/expected/create_table.out | 10 --
src/test/regress/expected/foreign_key.out | 67 ++
src/test/regress/sql/alter_table.sql | 57 -
src/test/regress/sql/create_table.sql | 8 --
src/test/regress/sql/foreign_key.sql | 28 +
12 files changed, 566 insertions(+), 84 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index 7dee6db0eb..9dc91fb67f 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -26,6 +26,7 @@
#include "catalog/pg_operator.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
+#include "commands/tablecmds.h"
#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
@@ -375,6 +376,197 @@ CreateConstraintEntry(const char *constraintName,
return conOid;
}
+/*
+ * For each foreign key constraint in relation parentId, create a cloned
+ * copy of it for relationId.
+ *
+ * relationId is a partition of parentId, so we can be certain that it has
+ * the same columns with the same datatypes. They may be in different order,
+ * though.
+ */
+void
+CloneForeignKeyConstraints(Oid parentId, Oid relationId)
+{
+ Relationpg_constraint;
+ Relationrel;
+ ScanKeyData key;
+ SysScanDesc scan;
+ TupleDesc tupdesc;
+ HeapTuple tuple;
+
+ /* see ATAddForeignKeyConstraint about lock level */
+ rel = heap_open(relationId, AccessExclusiveLock);
+
+ pg_constraint = heap_open(ConstraintRelationId, RowShareLock);
+ tupdesc = RelationGetDescr(pg_constraint);
+
+ ScanKeyInit(&key,
+ Anum_pg_constraint_conrelid,
BTEqualStrategyNumber,
+ F_OIDEQ, ObjectIdGetDatum(parentId));
+ scan = systable_beginscan(pg_constraint, ConstraintRelidIndexId, true,
+ NULL, 1, &key);
+
+ while ((tuple = systable_getnext(scan)) != NULL)
+ {
+ Form_pg_constraint constrForm = (Form_pg_constraint)
GETSTRUCT(tuple);
+ AttrNumber conkey[INDEX_MAX_KEYS];
+ AttrNumber confkey[INDEX_MAX_KEYS];
+ Oid conpfeqop[INDEX_MAX_KEYS];
+ Oid conppeqop[INDEX_MAX_KEYS];
+ Oid conffeqop[INDEX_MAX_KEYS];
+ Constraint *fkconstraint;
+ Oid constrOid;
+ ObjectAddress parentAddr,
+ childAddr;
+ int nelem;
+ ArrayType *arr;
+ Datum datum;
+ boolisnull;
+
+ /* only foreign keys */
+ if (constrForm->contype != CONSTRAINT_FOREIGN)
+ continue;
+
+ ObjectAddressSet(parentAddr, ConstraintRelationId,
+HeapTupleGetOid(tuple));
+
+ datum = fastgetattr(tuple, Anum_pg_constraint_conkey,
+ tupdesc, &isnull);
+ if (isnull)
+ elog(ERROR, "null conkey");
+ arr = DatumGetArrayTypeP(datum);
+ nelem = ARR_DIMS(arr)[0];
+ if (ARR_NDIM(arr) != 1 ||
+ nelem < 1 ||
+ nelem > INDEX_MAX_KEYS ||
+ ARR_HASNULL(arr) ||
+ ARR_ELEMTYPE(arr) != INT2OID)
+ elog(ERROR, "conkey is not a 1-D smallint array");
+ memcpy(conkey, ARR_DATA_PTR(arr), nelem * sizeof(AttrNumber));
+
+ datum = fastgetattr(tuple, Anum_pg_constraint_confkey,
+ tupdesc, &isnull);
+ if (isnull)
+ elog(ERROR, "null confkey");
+ arr = DatumGetArrayTypeP(datum);
+ nelem = ARR_DIMS(arr)[0];
+ if (ARR_N
