partitioned tables referenced by FKs

classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|

partitioned tables referenced by FKs

Alvaro Herrera-9
Here's a patch to allow partitioned tables to be referenced by foreign
keys.  Current state is WIP, but everything should work; see below for
the expected exception.

The design is very simple: have one pg_constraint row for each partition
on each side, each row pointing to the topmost table on the other side;
triggers appear on each leaf partition (and naturally they don't appear
on any intermediate partitioned table).

There are tests that should cover all the basic features.  pg_upgrade
tests work (containing relevant tables, as regress/foreign_key.sql
leaves them behind for this reason: partitioned-references-partitioned).

There is one requisite feature still missing from this patch: when a
partition on the referenced side is detached or dropped, we must ensure
no referencing row remains on the other side.  For this, I have an
(unmerged and thus unsubmitted here) new ri_triggers.c routine
RI_Inverted_Initial_Check (need to come up with better name, heh) that
verifies this, invoked at DETACH/DROP time.

Also, some general code cleanup and documentation patching is needed.

I'm posting this now so that I can take my hands off it for a few days;
will return to post an updated version at some point before next
commitfest.  I wanted to have this ready for this commitfest, but RL
dictated otherwise.  This patch took a *very long time* to write ... I
wrote three different recursion models for this.


One thing I realized while writing this, is that my commit
3de241dba86f ("Foreign keys on partitioned tables") put function
CloneForeignKeyConstraints() in catalog/pg_constraint.c that should
really have been in tablecmds.c.  In this patch I produced some siblings
of that function still in pg_constraint.c, but I intend to move the
whole lot to tablecmds.c before commit.

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

partitioned-foreign-keys.patch (54K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Alvaro Herrera-9
Oh, I forgot to mention one thing.  When creating a constraint, an index
OID is normally given.  I'm not sure what is this for.  In the patch
it's a little convoluted to get the correct index OID, so I'm just
passing InvalidOid.  Things work nonetheless.  I wonder if we shouldn't
just do away with the index OID.  There are several /*FIXME*/ notes in
the code due to this.

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

Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Corey Huinker
In reply to this post by Alvaro Herrera-9
On Fri, Nov 2, 2018 at 7:42 PM Alvaro Herrera <[hidden email]> wrote:
Here's a patch to allow partitioned tables to be referenced by foreign
keys.  Current state is WIP, but everything should work; see below for
the expected exception.

The design is very simple: have one pg_constraint row for each partition
on each side, each row pointing to the topmost table on the other side;
triggers appear on each leaf partition (and naturally they don't appear
on any intermediate partitioned table).

This is an important and much needed feature!

Based on my extremely naive reading of this code, I have two perhaps equally naive questions:

1. it seems that we will continue to to per-row RI checks for inserts and updates. However, there already exists a bulk check in RI_Initial_Check(). Could we modify this bulk check to do RI checks on a per-statement basis rather than a per-row basis?

2. If #1 is possible, is the overhead of transitions tables too great for the single-row case?
Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Alvaro Herrera-9
On 2018-Nov-05, Corey Huinker wrote:

> This is an important and much needed feature!

I agree :-)

> Based on my extremely naive reading of this code, I have two perhaps
> equally naive questions:
>
> 1. it seems that we will continue to to per-row RI checks for inserts and
> updates. However, there already exists a bulk check in RI_Initial_Check().
> Could we modify this bulk check to do RI checks on a per-statement basis
> rather than a per-row basis?

One of the goals when implementing trigger transition tables was to
supplant the current per-row implementation of RI triggers with
per-statement.  I haven't done that, but AFAIK it remains possible :-)

Changing that is definitely not a goal of this patch.

> 2. If #1 is possible, is the overhead of transitions tables too great for
> the single-row case?

Without an implementation, I can't say, but if I had to guess, I would
assume so.  Or maybe there are clever optimizations for that particular
case.

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

Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Corey Huinker

> 1. it seems that we will continue to to per-row RI checks for inserts and
> updates. However, there already exists a bulk check in RI_Initial_Check().
> Could we modify this bulk check to do RI checks on a per-statement basis
> rather than a per-row basis?

One of the goals when implementing trigger transition tables was to
supplant the current per-row implementation of RI triggers with
per-statement.  I haven't done that, but AFAIK it remains possible :-)

Changing that is definitely not a goal of this patch.

Then I may try to tackle it myself in a separate thread.

Without an implementation, I can't say, but if I had to guess, I would
assume so.  Or maybe there are clever optimizations for that particular
case.

But in this case there is no actual defined trigger, it's internal code making an SPI call...is there an indicator that tells us whether this change was multi-row or not?

 
Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Alvaro Herrera-9
In reply to this post by Alvaro Herrera-9
Here's a more credible version of this patch series.

0001 refactors some duplicative code that interprets a pg_constraint row
for a foreign key back into a Constraint node.  Only what is necessary
for current features is read.

0002 moves some code that I added in 3de241dba86f to
src/backend/catalog/pg_constraint.c so that it appears in tablecmds.c
instead.  After developing the current patch I realized that the latter
is its natural home.

0003 changes the shape of a loop in deleteObjectsInList, so that I can
add object type-specific functions to be called before deleting an
object.  This is needed by 0004 and makes no sense on its own; I would
probably push both together, but splitting it like this makes it very
obvious what it is I'm doing.

0004 adds the feature itself

In 0004 there's also a new function "index_get_partition" which allows
to simplify some code I added in 8b08f7d4820f.  I will probably split it
up and commit separately because it's an obvious code beautify.

0005 modifies psql to show the constraint in a different way, which
makes more sense overall (rather than show the "internal" constraint
that concerns the partition, show the top-level contraint that concerns
the ancestor table on which it is defined.  This includes naming the
table in which the constraint is defined).

There's one half of 0005 that I think should be applied to pg11, because
when partitions have foreign keys, the display looks a bit odd
currently.  The other half I would probably merge into 0004 instead of
committing separately.  Even if not backpatched, it makes sense to apply
ahead of the rest because it changes expected output for a regression
test.

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

0001-Refactor-DeconstructConstraintRow.patch (16K) Download Attachment
0002-move-CloneForeignKeyConstraints-to-tablecmds.c.patch (21K) Download Attachment
0003-Rework-deleteObjectsInList-to-allow-objtype-specific.patch (2K) Download Attachment
0004-Support-foreign-key-referencing-partitioned-tables.patch (71K) Download Attachment
0005-Have-psql-not-display-redundant-FKs.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Jesper Pedersen
Hi,

On 11/30/18 2:12 PM, Alvaro Herrera wrote:
> Here's a more credible version of this patch series.
>

The patch series applies with hunks, and passes check-world.

No comments for 0001, 0002, 0003 and 0005.

While I'm still looking at 0004 - should we have a test that updates one
of the constraints of fk to another partition in pk ?

I'll didn't try this yet with [1], so sending you a flamegraph of
INSERTs off-list, since data loading takes a while during an OLTP based
work load.

Thanks for working on this !

[1] https://commitfest.postgresql.org/21/1897/

Best regards,
  Jesper

Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Jesper Pedersen
Hi,

On 1/7/19 1:07 PM, Jesper Pedersen wrote:
> While I'm still looking at 0004 - should we have a test that updates one
> of the constraints of fk to another partition in pk ?
>

In addition:

* Document pre_drop_class_check()
* I think index_get_partition() would be more visible in partition.c
* Remove code in #if 0 block

I have tested this with a hash based setup.

Thanks again !

Best regards,
  Jesper

Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Amit Langote-2
In reply to this post by Alvaro Herrera-9
Hi Alvaro,

On 2018/12/01 4:12, Alvaro Herrera wrote:
> Here's a more credible version of this patch series.

Are you going to post rebased patches soon?

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Alvaro Herrera-9
Hello,

On 2019-Jan-21, Amit Langote wrote:

> On 2018/12/01 4:12, Alvaro Herrera wrote:
> > Here's a more credible version of this patch series.
>
> Are you going to post rebased patches soon?

Yes, very soon -- right now, in fact :-)

Two preliminary patches in this series are already pushed, per a nearby
bugfix.  I also split out the new index_get_partition routine to
catalog/partition.c, per comment from Jesper, and put it on its own
patch.  0003 is the interesting bits in this submission.

Note that there is a change in constraint naming on partitions.  This
affects some preexisting test output ... and I'm not liking the changes
much, anymore.  I'll have a look about how to revert to the previous
behavior.

As you noticed in the other thread, the part of the FK clone routine
that attaches to an existing constraint needed to be refactored into its
own routine.  I did that, though the split is different from what you
were proposing.

Jesper also mentioned removing the "#if 0" code.  Actually what I need
to be doing is reinstating that check in the cases where it's possible.
I haven't done that yet.

I also dropped the part that changed how psql reports constraints in \d,
since there's a separate commitfest entry for that one.

Hmm, I just noticed that there's an ereport that fails i18n by way of
using a ternary operator in the first argument of errmsg.

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

0001-Rework-deleteObjectsInList-to-allow-objtype-specific.patch (2K) Download Attachment
0002-index_get_partition.patch (4K) Download Attachment
0003-Support-FKs-referencing-partitiones-tables.patch (82K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Michael Paquier-2
On Tue, Jan 22, 2019 at 06:45:48PM -0300, Alvaro Herrera wrote:
> Yes, very soon -- right now, in fact :-)

This needs a rebase.  Moved to next CF, waiting on author.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Alvaro Herrera-9
In reply to this post by Alvaro Herrera-9
Here's another rebase.  The code is structured somewhat differently from
the previous one, mostly because of the backpatched bugfixes, but also
because of the changes in dependency handling.

I think there's also at least one more bugfix to backpatch (included in
0003 here), related to whether foreign tables are allowed as having FKs,
but AFAICS it just cosmetic: you get an error if you have a foreign
partition and add a FK, but it says "you cannot have constraint
triggers" rather than "FKs are not supported", so it's a bit odd.
0003 also includes Amit L's patch to remove the parentConstraint
argument from ATExecAddForeignKey, which I suppose I should commit
separately.

Odd bugs fixed: a) handle the case where the default partition is the
only one and it's being detached or dropped.  b) when a partition was
dropped/detached from the referenced side, the child constraint was left
in place.

I have not yet fixed the "#if 0" section.

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

v4-0001-Rework-deleteObjectsInList-to-allow-objtype-speci.patch (2K) Download Attachment
v4-0002-index_get_partition.patch (4K) Download Attachment
v4-0003-support-FKs-referencing-partitioned-tables.patch (80K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Amit Langote-2
Hi Alvaro,

I looked at the latest patch and most of the issues/bugs that I was going
to report based on the late January version of the patch seem to have been
taken care of; sorry that I couldn't post sooner which would've saved you
some time.   The patch needs to be rebased on top of ff11e7f4b9 which
touched ri_triggers.c.

In the following case:

create table pk (a int primary key) partition by list (a);
create table pk1 (a int primary key);
create table fk (a int references pk1);

-- adds FK referencing pk1 via ATAddForeignKeyConstraint recursion
alter table pk attach partition pk1 for values in (1);
alter table fk add foreign key (a) references pk;

or:

-- adds FK referencing pk1 via CloneForeignKeyConstraints
alter table fk add foreign key (a) references pk;
alter table pk attach partition pk1 for values in (1);

\d fk
                 Table "public.fk"
 Column │  Type   │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
 a      │ integer │           │          │
Foreign-key constraints:
    "fk_a_fkey" FOREIGN KEY (a) REFERENCES pk1(a)
    "fk_a_fkey1" FOREIGN KEY (a) REFERENCES pk(a)
    "fk_a_fkey2" FOREIGN KEY (a) REFERENCES pk1(a)

Could we have avoided creating fk_a_fkey2 which must be same as fk_a_fkey
as far as the user-visible behavior is concerned?


* Regarding 0003

I'd like to hear your thoughts on some suggestions to alter the structure
of the reorganized code around foreign key addition/cloning.  With this
patch adding support for foreign keys to reference partitioned tables, the
code now has to consider various cases due to the possibility of having
partitioned tables on both sides of a foreign key, which is reflected in
the complexity of the new code.  Following functions have been introduced
in the foreign key DDL code in the course of adding support for partitioning:

addFkRecurseReferenced
addFkRecurseReferencing
CloneForeignKeyConstraints
CloneFkReferencing
CloneFkReferenced
tryAttachPartitionForeignKey

We have addFkRecurseReferencing and CloneForeignKeyConstraints calling
CloneFkReferencing to recursively add a foreign key constraint being
defined on (or being cloned to) a partitioned table to its partitions.
The latter invokes tryAttachPartitionForeignKey to see if an existing
foreign key constraint is functionally same as one being cloned to avoid
creating a duplicate constraint.

However, we have CloneFkReferenced() calling addFkRecurseReferenced, not
the other way around.  Neither of these functions attempts to reuse an
existing, functionally equivalent, foreign key constraint referencing a
given partition that's being recursed to.  So we get the behavior in the
above example, which again, I'm not sure is intentional.

Also, it seems a bit confusing that there is a CreateConstraintEntry call
in addFkRecurseReferenced() which is creating a constraint on the
*referencing* relation, which I think should be in
ATAddForeignKeyConstraint, the caller.  I know we need to create a copy of
the constraint to reference the partitions of the referenced table, but we
might as well put it in CloneFkReferenced and reverse who calls who --
make addFkRecurseReferenced() call CloneFkReferenced and have the code to
create the cloned constraint and action triggers in the latter.  That will
make the code to handle different sides of foreign key look similar, and
imho, easier to follow.


+/*
+ * addFkRecurseReferenced
+ *      recursive subroutine for ATAddForeignKeyConstraint, referenced side

How about:

Subroutine for ATAddForeignKeyConstraint to add action trigger on
referenced relation, recursing if partitioned, in which case, both the
constraint referencing a given partition and the action trigger are cloned
in all partitions

+/*
+ * addFkRecurseReferenced
+ *      recursive subroutine for ATAddForeignKeyConstraint, referenced side

How about:

Subroutine for ATAddForeignKeyConstraint to add check trigger on
referencing relation, recursing if partitioned, in which case, both the
constraint and the check trigger are cloned in all partitions

I noticed however that this function itself is not recursively called;
CloneFkReferencing handles the recursion.

/*
 * CloneForeignKeyConstraints
 *      Clone foreign keys from a partitioned table to a newly acquired
 *      partition.

Maybe:

Clone foreign key of (or referencing) a partitioned table to a newly
acquired partition


* In 0002,

 /*
+ * Return the OID of the index, in the given partition, that is a child
of the
+ * given index or InvalidOid if there isn't one.
+ */
+Oid
+index_get_partition(Relation partition, Oid indexId)

Maybe add a comma between "...given index" and "or InvalidOid", or maybe
rewrite the sentence as:

Return the OID of index of the given partition that is a child of the
given index, or InvalidOid if there isn't one.


* Unrelated to this thread, but while reviewing, I noticed this code in
ATExecAttachPartitionIdx:

    /* no deadlock risk: RangeVarGetRelidExtended already acquired the lock */
    partIdx = relation_open(partIdxId, AccessExclusiveLock);

    /* we already hold locks on both tables, so this is safe: */
    parentTbl = relation_open(parentIdx->rd_index->indrelid, AccessShareLock);
    partTbl = relation_open(partIdx->rd_index->indrelid, NoLock);


I wonder why not NoLock in *all* of these relation_open calls?  As
the comment says, RangeVarGetRelidExtended already got the lock for
'partIdxId'.  Then there must already be a lock on 'parentTbl' as it's the
target relation of the ALTER INDEX command (actually the target index's
table).

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Alvaro Herrera-9
On 2019-Feb-28, Amit Langote wrote:

> Hi Alvaro,
>
> I looked at the latest patch and most of the issues/bugs that I was going
> to report based on the late January version of the patch seem to have been
> taken care of; sorry that I couldn't post sooner which would've saved you
> some time.   The patch needs to be rebased on top of ff11e7f4b9 which
> touched ri_triggers.c.

Rebased to current master.  I'll reply later to handle the other issues
you point out.

Given the comments on 0002 in this thread and elsewhere, I'm inclined to
push that one today with minor tweaks.

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

v5-0001-Rework-deleteObjectsInList-to-allow-objtype-speci.patch (2K) Download Attachment
v5-0002-index_get_partition.patch (4K) Download Attachment
v5-0003-support-FKs-referencing-partitioned-tables.patch (81K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Jesper Pedersen
Hi Alvaro,

On 2/28/19 1:28 PM, Alvaro Herrera wrote:
> Rebased to current master.  I'll reply later to handle the other issues
> you point out.
>

Applying with hunks.

With 0003 using

export CFLAGS="-DCOPY_PARSE_PLAN_TREES -O0 -fno-omit-frame-pointer" &&
CC="ccache gcc" ./configure --prefix /usr/local/packages/postgresql-12.x
--enable-dtrace --with-openssl --with-gssapi --with-libxml --with-llvm
--enable-debug --enable-depend --enable-tap-tests --enable-cassert

I'm getting a failure in the pg_upgrade test:

  --
+-- Name: pk5 pk5_pkey; Type: CONSTRAINT; Schema: regress_fk; Owner:
jpedersen
+--
+
+ALTER TABLE ONLY regress_fk.pk5
+    ADD CONSTRAINT pk5_pkey PRIMARY KEY (a);
+
+
+--

when running check-world.

Best regards,
  Jesper

Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Alvaro Herrera-9
In reply to this post by Amit Langote-2
On 2019-Feb-28, Amit Langote wrote:

Hello

> In the following case:
>
> create table pk (a int primary key) partition by list (a);
> create table pk1 (a int primary key);
> create table fk (a int references pk1);
>
> -- adds FK referencing pk1 via ATAddForeignKeyConstraint recursion
> alter table pk attach partition pk1 for values in (1);
> alter table fk add foreign key (a) references pk;
>
> or:
>
> -- adds FK referencing pk1 via CloneForeignKeyConstraints
> alter table fk add foreign key (a) references pk;
> alter table pk attach partition pk1 for values in (1);
>
> \d fk
>                  Table "public.fk"
>  Column │  Type   │ Collation │ Nullable │ Default
> ────────┼─────────┼───────────┼──────────┼─────────
>  a      │ integer │           │          │
> Foreign-key constraints:
>     "fk_a_fkey" FOREIGN KEY (a) REFERENCES pk1(a)
>     "fk_a_fkey1" FOREIGN KEY (a) REFERENCES pk(a)
>     "fk_a_fkey2" FOREIGN KEY (a) REFERENCES pk1(a)
>
> Could we have avoided creating fk_a_fkey2 which must be same as fk_a_fkey
> as far as the user-visible behavior is concerned?

So I wrote the code that does as you describe, and it worked
beautifully.

Once I was finished, fixed bugs and tested it, I realized that that was
a stupid thing to have done -- because THOSE ARE DIFFERENT CONSTRAINTS.
When you say "fk (a) references pk1" you're saying that all the values
in fk(a) must appear in pk1.  OTOH when you say "fk references pk" you
mean that the values might appear anywhere in pk, not just pk1.  Have a
look at the triggers in pg_trigger that appear when you do "references
pk1" vs. when you do "references pk".  The constraint itself might
appear identical, but the former adds check triggers that are not
present for the latter.

The main problem here is the way the constraints are presented to the
user in psql (fk_a_fkey2 ought not to appear at all, because it's an
implementation detail for fk_a_fkey), which as you know I have another
patch for.

> * Regarding 0003
>
> I'd like to hear your thoughts on some suggestions to alter the structure
> of the reorganized code around foreign key addition/cloning.  With this
> patch adding support for foreign keys to reference partitioned tables, the
> code now has to consider various cases due to the possibility of having
> partitioned tables on both sides of a foreign key, which is reflected in
> the complexity of the new code.  Following functions have been introduced
> in the foreign key DDL code in the course of adding support for partitioning:
>
> addFkRecurseReferenced
> addFkRecurseReferencing
> CloneForeignKeyConstraints
> CloneFkReferencing
> CloneFkReferenced
> tryAttachPartitionForeignKey

I can understand how the lack of consistency in how we handle recursion
can be confusing.  I'll see if I can move the recursion so that it's
handled similarly for both cases, but I'm not convinced that it can.

Note there's a single call of tryAttachPartitionForeignKey().  I
invented that function because in the original code pattern there were
two callers, but after some backpatched code restructuring to fix a bug,
I changed that and now that function is no longer necessary.  I might
put the code back into CloneFkReferencing.

> The latter invokes tryAttachPartitionForeignKey to see if an existing
> foreign key constraint is functionally same as one being cloned to avoid
> creating a duplicate constraint.

Do note that "functionally the same" does not mean the same thing when
applied to the referencing side than when applied to the referenced
side.

> However, we have CloneFkReferenced() calling addFkRecurseReferenced, not
> the other way around.  Neither of these functions attempts to reuse an
> existing, functionally equivalent, foreign key constraint referencing a
> given partition that's being recursed to.

You'll have to take back the assertion that those constraints are
funtionally equivalent, because they are not.

> Also, it seems a bit confusing that there is a CreateConstraintEntry call
> in addFkRecurseReferenced() which is creating a constraint on the
> *referencing* relation, which I think should be in
> ATAddForeignKeyConstraint, the caller.  I know we need to create a copy of
> the constraint to reference the partitions of the referenced table, but we
> might as well put it in CloneFkReferenced and reverse who calls who --
> make addFkRecurseReferenced() call CloneFkReferenced and have the code to
> create the cloned constraint and action triggers in the latter.  That will
> make the code to handle different sides of foreign key look similar, and
> imho, easier to follow.

I'm looking into this code restructuring you suggest.  I'm not 100% sold
on it yet ... it took me quite a while to arrive to a working
arrangement, although I admit the backpatched bugfixes dislodged things.

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

Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Robert Haas
On Thu, Mar 14, 2019 at 1:40 PM Alvaro Herrera <[hidden email]> wrote:
> Once I was finished, fixed bugs and tested it, I realized that that was
> a stupid thing to have done -- because THOSE ARE DIFFERENT CONSTRAINTS.

This made me laugh.

> When you say "fk (a) references pk1" you're saying that all the values
> in fk(a) must appear in pk1.  OTOH when you say "fk references pk" you
> mean that the values might appear anywhere in pk, not just pk1.  Have a
> look at the triggers in pg_trigger that appear when you do "references
> pk1" vs. when you do "references pk".  The constraint itself might
> appear identical, but the former adds check triggers that are not
> present for the latter.

It's probably not uncommon to have FKs between compatibly-partitioned
tables, though, and in that case they are really equivalent.  For
example, if you have an orders table and an order_lines table and the
latter has an FK pointing at the former, and both are partitioned on
the order number, then it must be that every order_line references an
order in the matching partition.  Even if it's not practical to use
the FK itself, it's a good excuse for skipping any validation scan you
otherwise might have performed.

But there are other cases in which that logic doesn't hold.  I can't
quite wrap my brain around the exact criteria at the moment.  I think
it's something like: the partitioning columns of the referring table
are a prefix of the foreign key columns, and the opfamilies match, and
likewise for the referenced table.  And the partition bounds match up
well enough.  And maybe some other things.

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

Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Alvaro Herrera-9
On 2019-Mar-14, Robert Haas wrote:

> On Thu, Mar 14, 2019 at 1:40 PM Alvaro Herrera <[hidden email]> wrote:

> > When you say "fk (a) references pk1" you're saying that all the values
> > in fk(a) must appear in pk1.  OTOH when you say "fk references pk" you
> > mean that the values might appear anywhere in pk, not just pk1.  Have a
> > look at the triggers in pg_trigger that appear when you do "references
> > pk1" vs. when you do "references pk".  The constraint itself might
> > appear identical, but the former adds check triggers that are not
> > present for the latter.
>
> It's probably not uncommon to have FKs between compatibly-partitioned
> tables, though, and in that case they are really equivalent.  For
> example, if you have an orders table and an order_lines table and the
> latter has an FK pointing at the former, and both are partitioned on
> the order number, then it must be that every order_line references an
> order in the matching partition.  Even if it's not practical to use
> the FK itself, it's a good excuse for skipping any validation scan you
> otherwise might have performed.

Well, I suppose that can be implemented as an optimization on top of
what we have, but I think that we should first get this feature right,
and later we can see about improving it.  In any case, since the RI
queries are run via SPI, any unnecessary partitions should get pruned by
partition pruning based on each partition's constraint.  So I'm not so
certain that this is a problem worth spending much time/effort/risk of
bugs on.

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

Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Robert Haas
On Thu, Mar 14, 2019 at 3:36 PM Alvaro Herrera <[hidden email]> wrote:
> Well, I suppose that can be implemented as an optimization on top of
> what we have, but I think that we should first get this feature right,
> and later we can see about improving it.

Sure, not arguing with that.

> In any case, since the RI
> queries are run via SPI, any unnecessary partitions should get pruned by
> partition pruning based on each partition's constraint.  So I'm not so
> certain that this is a problem worth spending much time/effort/risk of
> bugs on.

I doubt that partition pruning would just automatically DTRT in a case
like this, but if I'm wrong, great!

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

Reply | Threaded
Open this post in threaded view
|

Re: partitioned tables referenced by FKs

Amit Langote-2
In reply to this post by Alvaro Herrera-9
Hi,

On 2019/03/15 2:31, Alvaro Herrera wrote:
> Once I was finished, fixed bugs and tested it, I realized that that was
> a stupid thing to have done -- because THOSE ARE DIFFERENT CONSTRAINTS.
> When you say "fk (a) references pk1" you're saying that all the values
> in fk(a) must appear in pk1.  OTOH when you say "fk references pk" you
> mean that the values might appear anywhere in pk, not just pk1.

Sure, then just drop the check trigger that queries only pk1, in favor of
one that checks pk, that's already in place due to the parent constraint.
Actually, if one doesn't drop it (that is, by way of dropping the
constraint that created it), they won't be able to insert into fk anything
but the subset of rows that pk1 allows as a partition; see this:

create table pk1 (a int primary key);
insert into pk1 values (1);
create table pk (a int primary key) partition by list (a);
create table fk (a int, foreign key (a) references pk1, foreign key (a)
references pk);

insert into fk values (1);
ERROR:  insert or update on table "fk" violates foreign key constraint
"fk_a_fkey1"
DETAIL:  Key (a)=(1) is not present in table "pk".

alter table pk attach partition pk1 for values in (1);
insert into fk values (1);

create table pk2 partition of pk for values in (2);
insert into pk values (2);

insert into fk values (2);
ERROR:  insert or update on table "fk" violates foreign key constraint
"fk_a_fkey"
DETAIL:  Key (a)=(2) is not present in table "pk1".

You can no longer insert (2) into pk1 though.

Now it's true that a user can drop the constraint directly referencing pk1
themselves to make the above error go away, but it would've been nice if
they didn't have to do it.  That would be if it was internally converted
into a child constraint when attaching pk1 to pk, as I'm suggesting.

> Have a
> look at the triggers in pg_trigger that appear when you do "references
> pk1" vs. when you do "references pk".  The constraint itself might
> appear identical, but the former adds check triggers that are not
> present for the latter.

Let's consider both triggers.

1. The action trigger on the referenced table does the same thing no
matter whether it's a partition or not, which is this:

select 1 from fk x where $1 = a for key share of x;

Note that the trigger is invoked both when the referenced table is
directly mentioned in the query (delete from pk1) and when it's indirectly
mentioned via its parent table (delete from pk).  Duplication of triggers
in the latter case causes the same set of rows to be added twice for
checking in the respective triggers' contexts, which seems pointless.

2. The check trigger on the referencing table checks exactly the primary
key table that's mentioned in the foreign key constraint.  So, when the
foreign key constraint is "referencing pk1", the check query is:

select 1 from pk1 x where a = $1 for key share of x;

whereas when the foreign key constraint is "referencing pk", it's:

select 1 from pk x where a = $1 for key share of x;

The latter scans appropriate partition of pk, which may be pk1, so we can
say the first check trigger is redundant, and in fact, even annoying as
shown above.


So, when attaching pk1 to pk (where both are referenced by fk using
exactly the same columns and same other constraint attributes), we *can*
reuse the foreign key on fk referencing pk1, as the child constraint of
foreign key on fk referencing pk, whereby, we:

1. don't need to create a new action trigger on pk1, because the existing
one is enough,

2. can drop the check trigger on fk that checks pk1, because the one that
checks pk will be enough

As you know, we've done similar stuff in 123cc697a8eb + cb90de1aac18 for
the case where the partition is on the referencing side.  In that case, we
can reuse the check trigger as it checks the same table in all partitions
and drop the redundant action trigger.

Thanks,
Amit


12