Thread: pgsql: Enforce foreign key correctly during cross-partition updates

pgsql: Enforce foreign key correctly during cross-partition updates

From
Alvaro Herrera
Date:
Enforce foreign key correctly during cross-partition updates

When an update on a partitioned table referenced in foreign key
constraints causes a row to move from one partition to another,
the fact that the move is implemented as a delete followed by an insert
on the target partition causes the foreign key triggers to have
surprising behavior.  For example, a given foreign key's delete trigger
which implements the ON DELETE CASCADE clause of that key will delete
any referencing rows when triggered for that internal DELETE, although
it should not, because the referenced row is simply being moved from one
partition of the referenced root partitioned table into another, not
being deleted from it.

This commit teaches trigger.c to skip queuing such delete trigger events
on the leaf partitions in favor of an UPDATE event fired on the root
target relation.  Doing so is sensible because both the old and the new
tuple "logically" belong to the root relation.

The after trigger event queuing interface now allows passing the source
and the target partitions of a particular cross-partition update when
registering the update event for the root partitioned table.  Along with
the two ctids of the old and the new tuple, the after trigger event now
also stores the OIDs of those partitions. The tuples fetched from the
source and the target partitions are converted into the root table
format, if necessary, before they are passed to the trigger function.

The implementation currently has a limitation that only the foreign keys
pointing into the query's target relation are considered, not those of
its sub-partitioned partitions.  That seems like a reasonable
limitation, because it sounds rare to have distinct foreign keys
pointing to sub-partitioned partitions instead of to the root table.

This misbehavior stems from commit f56f8f8da6af (which added support for
foreign keys to reference partitioned tables) not paying sufficient
attention to commit 2f178441044b (which had introduced cross-partition
updates a year earlier).  Even though the former commit goes back to
Postgres 12, we're not backpatching this fix at this time for fear of
destabilizing things too much, and because there are a few ABI breaks in
it that we'd have to work around in older branches.  It also depends on
commit f4566345cf40, which had its own share of backpatchability issues
as well.

Author: Amit Langote <amitlangote09@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Reported-by: Eduard Català <eduard.catala@gmail.com>
Discussion: https://postgr.es/m/CA+HiwqFvkBCmfwkQX_yBqv2Wz8ugUGiBDxum8=WvVbfU1TXaNg@mail.gmail.com
Discussion: https://postgr.es/m/CAL54xNZsLwEM1XCk5yW9EqaRzsZYHuWsHQkA2L5MOSKXAwviCQ@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/ba9a7e392171c83eb3332a757279e7088487f9a2

Modified Files
--------------
doc/src/sgml/ref/update.sgml              |   7 +
src/backend/commands/trigger.c            | 394 +++++++++++++++++++++++++-----
src/backend/executor/execMain.c           |  86 ++++++-
src/backend/executor/execReplication.c    |   5 +-
src/backend/executor/nodeModifyTable.c    | 151 +++++++++++-
src/backend/utils/adt/ri_triggers.c       |   6 +
src/include/commands/trigger.h            |   8 +-
src/include/executor/executor.h           |   4 +-
src/include/nodes/execnodes.h             |   6 +
src/test/regress/expected/foreign_key.out | 204 +++++++++++++++-
src/test/regress/sql/foreign_key.sql      | 135 +++++++++-
11 files changed, 926 insertions(+), 80 deletions(-)


Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Enforce foreign key correctly during cross-partition updates

skink is not too happy with this:

==2663594== VALGRINDERROR-BEGIN
==2663594== Conditional jump or move depends on uninitialised value(s)
==2663594==    at 0x421526: ExecUpdateAct (nodeModifyTable.c:1855)
==2663594==    by 0x4217A3: ExecUpdate (nodeModifyTable.c:2107)
==2663594==    by 0x423027: ExecModifyTable (nodeModifyTable.c:2905)
==2663594==    by 0x3F5CB8: ExecProcNodeFirst (execProcnode.c:463)
==2663594==    by 0x3EE739: ExecProcNode (executor.h:259)
==2663594==    by 0x3EE739: ExecutePlan (execMain.c:1633)
==2663594==    by 0x3EE90E: standard_ExecutorRun (execMain.c:362)
==2663594==    by 0x3EE9D4: ExecutorRun (execMain.c:306)
==2663594==    by 0x5B3628: ProcessQuery (pquery.c:160)
==2663594==    by 0x5B41E9: PortalRunMulti (pquery.c:1274)
==2663594==    by 0x5B47AD: PortalRun (pquery.c:788)
==2663594==    by 0x5B091D: exec_simple_query (postgres.c:1250)
==2663594==    by 0x5B281B: PostgresMain (postgres.c:4520)
==2663594==  Uninitialised value was created by a stack allocation
==2663594==    at 0x421433: ExecUpdateAct (nodeModifyTable.c:1773)
==2663594== 
==2663594== VALGRINDERROR-END

It reproduces easily for me under valgrind.  I guess
ExecCrossPartitionUpdate must be failing to set
inserted_tuple or insert_destrel.

            regards, tom lane



Re: pgsql: Enforce foreign key correctly during cross-partition updates

From
Alvaro Herrera
Date:
On 2022-Mar-20, Tom Lane wrote:

> ==2663594== Conditional jump or move depends on uninitialised value(s)
> ==2663594==    at 0x421526: ExecUpdateAct (nodeModifyTable.c:1855)

> It reproduces easily for me under valgrind.  I guess
> ExecCrossPartitionUpdate must be failing to set
> inserted_tuple or insert_destrel.

Yeah, I was trying to reproduce it -- but it doesn't for me.  Strange.
Anyway, I think you're right: insert_destrel is used in the test when
ExecCrossPartitionUpdate returns true, and clearly it can do that and
not set insert_destrel.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"La grandeza es una experiencia transitoria.  Nunca es consistente.
Depende en gran parte de la imaginación humana creadora de mitos"
(Irulan)