[PATCH] session_replication_role = replica with TRUNCATE
Hi,
The current behavior of session_replication_role = replica with TRUNCATE
is not the same of with the other commands.
It does not check FKs for INSERT/UPDATE/DELETE but it does for TRUNCATE,
so one cannot execute TRUNCATE on a table when it is possible to DELETE
from table without WHERE clause.
I'm attaching a simple patch to make TRUNCATE match behavior of DELETE
for session_replication_role = replica.
Regards,
Marco
--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
Attachments:
truncate_ignore_fks_role_replica.v1.patchtext/plain; charset=UTF-8; name=truncate_ignore_fks_role_replica.v1.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d979ce266d..296807849f 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 1341,1356 **** ExecuteTruncate(TruncateStmt *stmt)
}
/*
! * Check foreign key references. In CASCADE mode, this should be
! * unnecessary since we just pulled in all the references; but as a
! * cross-check, do it anyway if in an Assert-enabled build.
*/
#ifdef USE_ASSERT_CHECKING
- heap_truncate_check_FKs(rels, false);
- #else
- if (stmt->behavior == DROP_RESTRICT)
heap_truncate_check_FKs(rels, false);
#endif
/*
* If we are asked to restart sequences, find all the sequences, lock them
--- 1341,1364 ----
}
/*
! * Suppress foreign key references check if session replication role is
! * set to REPLICA.
*/
+ if (SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA)
+ {
+
+ /*
+ * Check foreign key references. In CASCADE mode, this should be
+ * unnecessary since we just pulled in all the references; but as a
+ * cross-check, do it anyway if in an Assert-enabled build.
+ */
#ifdef USE_ASSERT_CHECKING
heap_truncate_check_FKs(rels, false);
+ #else
+ if (stmt->behavior == DROP_RESTRICT)
+ heap_truncate_check_FKs(rels, false);
#endif
+ }
/*
* If we are asked to restart sequences, find all the sequences, lock them
Hi,
On 29/12/17 13:01, Marco Nenciarini wrote:
Hi,
The current behavior of session_replication_role = replica with TRUNCATE
is not the same of with the other commands.
It does not check FKs for INSERT/UPDATE/DELETE but it does for TRUNCATE,
so one cannot execute TRUNCATE on a table when it is possible to DELETE
from table without WHERE clause.
Yes please, I never understood why 'DELETE FROM foo;' works fine with
'session_replication_role = replica' and FKs while 'TRUNCATE foo;' would
throw error.
I'm attaching a simple patch to make TRUNCATE match behavior of DELETE
for session_replication_role = replica.
May be worth documenting that the session_replication_role also affects
TRUNCATE's interaction with FKs in config.sgml.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 29 December 2017 at 22:14, Petr Jelinek <petr.jelinek@2ndquadrant.com>
wrote:
Hi,
On 29/12/17 13:01, Marco Nenciarini wrote:
Hi,
The current behavior of session_replication_role = replica with TRUNCATE
is not the same of with the other commands.
It does not check FKs for INSERT/UPDATE/DELETE but it does for TRUNCATE,
so one cannot execute TRUNCATE on a table when it is possible to DELETE
from table without WHERE clause.Yes please, I never understood why 'DELETE FROM foo;' works fine with
'session_replication_role = replica' and FKs while 'TRUNCATE foo;' would
throw error.
I've spent ages scratching my head about various ways to handle TRUNCATE
and FKs on the downstream, and it never once occurred to me that
session_replication_role should ignore FK cascades for replicated truncate.
But it's really sensible to do just that. Sure, you can have dangling FKs,
but the same is true if you create FKs from non-replicated tables pointing
to replicated tables and do DELETEs from the replicated table, if your
replication tool doesn't have some trick to stop you creating the FK.
I'd still like to know if it was a cascade when applying it, so I can
possibly make some client-determined behaviour choice, but that's for the
other patch really.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Il 29/12/17 15:14, Petr Jelinek ha scritto:
May be worth documenting that the session_replication_role also affects
TRUNCATE's interaction with FKs in config.sgml.
The current documentation of session_replication_role GUC is:
Controls firing of replication-related triggers and rules for the
current session. Setting this variable requires superuser privilege
and results in discarding any previously cached query plans.
Possible values are origin (the default), replica and local.
See ALTER TABLE for more information.
It doesn't speak about foreign keys or referential integrity, but only
about triggers and rules. I don't think that it's worth to add a special
case for truncate, unless we want to expand/rewrite the documentation to
specify all the effects in the details.
Regards,
Marco
--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
On 29/12/17 16:53, Marco Nenciarini wrote:
Il 29/12/17 15:14, Petr Jelinek ha scritto:
May be worth documenting that the session_replication_role also affects
TRUNCATE's interaction with FKs in config.sgml.The current documentation of session_replication_role GUC is:
Controls firing of replication-related triggers and rules for the
current session. Setting this variable requires superuser privilege
and results in discarding any previously cached query plans.
Possible values are origin (the default), replica and local.
See ALTER TABLE for more information.It doesn't speak about foreign keys or referential integrity, but only
about triggers and rules. I don't think that it's worth to add a special
case for truncate, unless we want to expand/rewrite the documentation to
specify all the effects in the details.
The effects on foreign keys is implied by the fact that for DML it's
implemented using triggers, but that's not true for TRUNCATE. In any
case it does not hurt to mention the FKs explicitly rather than
implicitly here.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 30 December 2017 at 03:32, Petr Jelinek <petr.jelinek@2ndquadrant.com>
wrote:
On 29/12/17 16:53, Marco Nenciarini wrote:
Il 29/12/17 15:14, Petr Jelinek ha scritto:
May be worth documenting that the session_replication_role also affects
TRUNCATE's interaction with FKs in config.sgml.The current documentation of session_replication_role GUC is:
Controls firing of replication-related triggers and rules for the
current session. Setting this variable requires superuser privilege
and results in discarding any previously cached query plans.
Possible values are origin (the default), replica and local.
See ALTER TABLE for more information.It doesn't speak about foreign keys or referential integrity, but only
about triggers and rules. I don't think that it's worth to add a special
case for truncate, unless we want to expand/rewrite the documentation to
specify all the effects in the details.The effects on foreign keys is implied by the fact that for DML it's
implemented using triggers, but that's not true for TRUNCATE. In any
case it does not hurt to mention the FKs explicitly rather than
implicitly here.
Yeah. I'd argue that's an oversight in the current docs that "can cause FK
violations" isn't mentioned. That's kind of important, and FKs being
triggers is implementation detail we shouldn't be relying on users to know.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Hi,
Il 30/12/17 08:42, Craig Ringer ha scritto:
On 30 December 2017 at 03:32, Petr Jelinek <petr.jelinek@2ndquadrant.com
<mailto:petr.jelinek@2ndquadrant.com>> wrote:On 29/12/17 16:53, Marco Nenciarini wrote:
Il 29/12/17 15:14, Petr Jelinek ha scritto:
May be worth documenting that the session_replication_role also affects
TRUNCATE's interaction with FKs in config.sgml.The current documentation of session_replication_role GUC is:
Controls firing of replication-related triggers and rules for the
current session. Setting this variable requires superuser privilege
and results in discarding any previously cached query plans.
Possible values are origin (the default), replica and local.
See ALTER TABLE for more information.It doesn't speak about foreign keys or referential integrity, but only
about triggers and rules. I don't think that it's worth to add a special
case for truncate, unless we want to expand/rewrite the documentation to
specify all the effects in the details.The effects on foreign keys is implied by the fact that for DML it's
implemented using triggers, but that's not true for TRUNCATE. In any
case it does not hurt to mention the FKs explicitly rather than
implicitly here.Yeah. I'd argue that's an oversight in the current docs that "can cause
FK violations" isn't mentioned. That's kind of important, and FKs being
triggers is implementation detail we shouldn't be relying on users to know.
I've tried to amend the documentation to be more clear. Feel free to
suggest further editing. Patch v2 attached.
Regards,
Marco
P.S: I'm struggling to understand why we have two possible values of
session_replication_role settings that behave identically (origin and
local). I'm unable to see any difference according to the code or the
documentation, so I'm wondering if we should document that they are the
same.
--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
Attachments:
truncate_ignore_fks_role_replica.v2.patchtext/plain; charset=UTF-8; name=truncate_ignore_fks_role_replica.v2.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e4a01699e4..aff56891e6 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 6502,6508 **** COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
<listitem>
<para>
Controls firing of replication-related triggers and rules for the
! current session. Setting this variable requires
superuser privilege and results in discarding any previously cached
query plans. Possible values are <literal>origin</literal> (the default),
<literal>replica</literal> and <literal>local</literal>.
--- 6502,6510 ----
<listitem>
<para>
Controls firing of replication-related triggers and rules for the
! current session. When set to <literal>replica</literal> it also
! disables all the foreign key checks, which can leave the data in an
! inconsistent state if improperly used. Setting this variable requires
superuser privilege and results in discarding any previously cached
query plans. Possible values are <literal>origin</literal> (the default),
<literal>replica</literal> and <literal>local</literal>.
diff --git a/src/backend/commanindex d979ce266d..296807849f 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 1341,1356 **** ExecuteTruncate(TruncateStmt *stmt)
}
/*
! * Check foreign key references. In CASCADE mode, this should be
! * unnecessary since we just pulled in all the references; but as a
! * cross-check, do it anyway if in an Assert-enabled build.
*/
#ifdef USE_ASSERT_CHECKING
- heap_truncate_check_FKs(rels, false);
- #else
- if (stmt->behavior == DROP_RESTRICT)
heap_truncate_check_FKs(rels, false);
#endif
/*
* If we are asked to restart sequences, find all the sequences, lock them
--- 1341,1364 ----
}
/*
! * Suppress foreign key references check if session replication role is
! * set to REPLICA.
*/
+ if (SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA)
+ {
+
+ /*
+ * Check foreign key references. In CASCADE mode, this should be
+ * unnecessary since we just pulled in all the references; but as a
+ * cross-check, do it anyway if in an Assert-enabled build.
+ */
#ifdef USE_ASSERT_CHECKING
heap_truncate_check_FKs(rels, false);
+ #else
+ if (stmt->behavior == DROP_RESTRICT)
+ heap_truncate_check_FKs(rels, false);
#endif
+ }
/*
* If we are asked to restart sequences, find all the sequences, lock them
Hi,
On 02/01/18 17:16, Marco Nenciarini wrote:
Hi,
I've tried to amend the documentation to be more clear. Feel free to
suggest further editing. Patch v2 attached.
I think the patch as is now looks okay. So marking as ready for committer.
This is noteworthy for the release notes though as it's behavior change
compared to old versions.
Regards,
MarcoP.S: I'm struggling to understand why we have two possible values of
session_replication_role settings that behave identically (origin and
local). I'm unable to see any difference according to the code or the
documentation, so I'm wondering if we should document that they are the
same.
It's for use by 3rd party tools (afaik both londiste and slony use it)
to differentiate between replicated and not replicated changes.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 1/17/18 11:33, Petr Jelinek wrote:
P.S: I'm struggling to understand why we have two possible values of
session_replication_role settings that behave identically (origin and
local). I'm unable to see any difference according to the code or the
documentation, so I'm wondering if we should document that they are the
same.It's for use by 3rd party tools (afaik both londiste and slony use it)
to differentiate between replicated and not replicated changes.
I have committed some documentation updates and tests to cover this a
bit better.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 12/29/17 07:01, Marco Nenciarini wrote:
The current behavior of session_replication_role = replica with TRUNCATE
is not the same of with the other commands.
It does not check FKs for INSERT/UPDATE/DELETE but it does for TRUNCATE,
so one cannot execute TRUNCATE on a table when it is possible to DELETE
from table without WHERE clause.I'm attaching a simple patch to make TRUNCATE match behavior of DELETE
for session_replication_role = replica.
I'm wondering whether this shouldn't be fixed the other way around,
namely have foreign keys always enforced on replicas.
I'm not aware of an explanation why it currently works the way it does,
other than that FKs happen to be implemented by triggers and triggers
happen to work that way. But I think it's pretty bogus that logical
replication subscriptions can insert data that violates constraints.
It's also weird that you can violate deferred unique constraints but not
immediate ones (I think, not tested).
So I'm proposing the attached alternative patch, which creates
constraint triggers to be TRIGGER_FIRES_ALWAYS by default.
Thoughts?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Create-constraint-triggers-to-fire-ALWAYS.patchtext/plain; charset=UTF-8; name=0001-Create-constraint-triggers-to-fire-ALWAYS.patch; x-mac-creator=0; x-mac-type=0Download
From aa22bdd6428ae3df1dbf0f0e31f9431dd374d8c5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Thu, 18 Jan 2018 12:06:45 -0500
Subject: [PATCH] Create constraint triggers to fire ALWAYS
This ensures that all constraints are also enforced on logical replicas.
---
src/backend/commands/trigger.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 1c488c338a..e67f981029 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -143,6 +143,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
bool isInternal)
{
int16 tgtype;
+ char tgenabled;
int ncolumns;
int16 *columns;
int2vector *tgattr;
@@ -327,6 +328,12 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
errmsg("INSTEAD OF triggers cannot have column lists")));
}
+ /*
+ * Constraint triggers fire always, normal triggers only on origin by
+ * default.
+ */
+ tgenabled = stmt->isconstraint ? TRIGGER_FIRES_ALWAYS : TRIGGER_FIRES_ON_ORIGIN;
+
/*
* We don't yet support naming ROW transition variables, but the parser
* recognizes the syntax so we can give a nicer message here.
@@ -741,7 +748,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
CStringGetDatum(trigname));
values[Anum_pg_trigger_tgfoid - 1] = ObjectIdGetDatum(funcoid);
values[Anum_pg_trigger_tgtype - 1] = Int16GetDatum(tgtype);
- values[Anum_pg_trigger_tgenabled - 1] = CharGetDatum(TRIGGER_FIRES_ON_ORIGIN);
+ values[Anum_pg_trigger_tgenabled - 1] = CharGetDatum(tgenabled);
values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal);
values[Anum_pg_trigger_tgconstrrelid - 1] = ObjectIdGetDatum(constrrelid);
values[Anum_pg_trigger_tgconstrindid - 1] = ObjectIdGetDatum(indexOid);
--
2.15.1
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
So I'm proposing the attached alternative patch, which creates
constraint triggers to be TRIGGER_FIRES_ALWAYS by default.
Thoughts?
Hm, the general idea seems attractive, but I'm not sure we want
this behavioral change for user-created triggers. Can we make it
happen like that only for RI triggers specifically? If not, there's
at least some missing doco changes here.
regards, tom lane
On 18 January 2018 at 17:14, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 12/29/17 07:01, Marco Nenciarini wrote:
The current behavior of session_replication_role = replica with TRUNCATE
is not the same of with the other commands.
It does not check FKs for INSERT/UPDATE/DELETE but it does for TRUNCATE,
so one cannot execute TRUNCATE on a table when it is possible to DELETE
from table without WHERE clause.I'm attaching a simple patch to make TRUNCATE match behavior of DELETE
for session_replication_role = replica.I'm wondering whether this shouldn't be fixed the other way around,
namely have foreign keys always enforced on replicas.I'm not aware of an explanation why it currently works the way it does,
other than that FKs happen to be implemented by triggers and triggers
happen to work that way. But I think it's pretty bogus that logical
replication subscriptions can insert data that violates constraints.
It's also weird that you can violate deferred unique constraints but not
immediate ones (I think, not tested).
The explanation is two-fold.
1. If we pass valid data from the publisher, it should still be valid
data when it gets there. If we cannot apply changes then the
subscriber is broken and the subscription cannot continue, so
executing the a constraint trigger does not solve the problem. (That
is why we will eventually be sending patches to handle breakage, which
is what you need for multi-master/BDR). It's not the role of this
patch to solve that problem, this patch just completes the set of
operations that can be successfully published.
2. If we know it already passes, then executing the constraint trigger
is just wasted cycles.
The subscriber side already has various requirements, such as manually
created DDL, so requiring sensible FKs is not so bad. This situation
will improve over next few years.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Peter,
Il 18/01/18 17:30, Peter Eisentraut ha scritto:
On 1/17/18 11:33, Petr Jelinek wrote:
P.S: I'm struggling to understand why we have two possible values of
session_replication_role settings that behave identically (origin and
local). I'm unable to see any difference according to the code or the
documentation, so I'm wondering if we should document that they are the
same.It's for use by 3rd party tools (afaik both londiste and slony use it)
to differentiate between replicated and not replicated changes.I have committed some documentation updates and tests to cover this a
bit better.
Thanks, the documentation is a lot clearer now.
This superseded the documentation change that was in the patch, so I've
removed it from the v3 version.
Regards,
Marco
--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
Attachments:
0001-truncate_ignore_fks_role_replica.v3.patchtext/plain; charset=UTF-8; name=0001-truncate_ignore_fks_role_replica.v3.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f2a928b823..180ebd0717 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 1340,1355 **** ExecuteTruncate(TruncateStmt *stmt)
}
/*
! * Check foreign key references. In CASCADE mode, this should be
! * unnecessary since we just pulled in all the references; but as a
! * cross-check, do it anyway if in an Assert-enabled build.
*/
#ifdef USE_ASSERT_CHECKING
- heap_truncate_check_FKs(rels, false);
- #else
- if (stmt->behavior == DROP_RESTRICT)
heap_truncate_check_FKs(rels, false);
#endif
/*
* If we are asked to restart sequences, find all the sequences, lock them
--- 1340,1363 ----
}
/*
! * Suppress foreign key references check if session replication role is
! * set to REPLICA.
*/
+ if (SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA)
+ {
+
+ /*
+ * Check foreign key references. In CASCADE mode, this should be
+ * unnecessary since we just pulled in all the references; but as a
+ * cross-check, do it anyway if in an Assert-enabled build.
+ */
#ifdef USE_ASSERT_CHECKING
heap_truncate_check_FKs(rels, false);
+ #else
+ if (stmt->behavior == DROP_RESTRICT)
+ heap_truncate_check_FKs(rels, false);
#endif
+ }
/*
* If we are asked to restart sequences, find all the sequences, lock them
On 19/01/18 12:41, Marco Nenciarini wrote:
Hi Peter,
Il 18/01/18 17:30, Peter Eisentraut ha scritto:
On 1/17/18 11:33, Petr Jelinek wrote:
P.S: I'm struggling to understand why we have two possible values of
session_replication_role settings that behave identically (origin and
local). I'm unable to see any difference according to the code or the
documentation, so I'm wondering if we should document that they are the
same.It's for use by 3rd party tools (afaik both londiste and slony use it)
to differentiate between replicated and not replicated changes.I have committed some documentation updates and tests to cover this a
bit better.Thanks, the documentation is a lot clearer now.
This superseded the documentation change that was in the patch, so I've
removed it from the v3 version.
Now that we have tests for this, I think it would be good idea to expand
them to cover the new behavior of TRUNCATE in this patch.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 1/19/18 05:31, Simon Riggs wrote:
I'm not aware of an explanation why it currently works the way it does,
other than that FKs happen to be implemented by triggers and triggers
happen to work that way. But I think it's pretty bogus that logical
replication subscriptions can insert data that violates constraints.
It's also weird that you can violate deferred unique constraints but not
immediate ones (I think, not tested).The explanation is two-fold.
1. If we pass valid data from the publisher, it should still be valid
data when it gets there.
That is not necessarily the case, because we allow independent writes on
the subscriber. The current test suite contains an example that you can
create invalid data this way.
If we cannot apply changes then the
subscriber is broken and the subscription cannot continue, so
executing the a constraint trigger does not solve the problem.
But that is already the case for any other kind of constraint or type
violation. Just that certain kinds of constraints are apparently
ignored for no clear reason.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/18/18 12:41, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
So I'm proposing the attached alternative patch, which creates
constraint triggers to be TRIGGER_FIRES_ALWAYS by default.
Thoughts?Hm, the general idea seems attractive, but I'm not sure we want
this behavioral change for user-created triggers. Can we make it
happen like that only for RI triggers specifically? If not, there's
at least some missing doco changes here.
I never quite understood the difference between a normal trigger and a
constraint trigger. But perhaps this should be it. If the purpose of a
constraint trigger is to enforce a constraint, then this should be the
default behavior, I think. You could always manually ALTER TABLE things.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Il 22/01/18 19:41, Petr Jelinek ha scritto:
On 19/01/18 12:41, Marco Nenciarini wrote:
Hi Peter,
Il 18/01/18 17:30, Peter Eisentraut ha scritto:
On 1/17/18 11:33, Petr Jelinek wrote:
P.S: I'm struggling to understand why we have two possible values of
session_replication_role settings that behave identically (origin and
local). I'm unable to see any difference according to the code or the
documentation, so I'm wondering if we should document that they are the
same.It's for use by 3rd party tools (afaik both londiste and slony use it)
to differentiate between replicated and not replicated changes.I have committed some documentation updates and tests to cover this a
bit better.Thanks, the documentation is a lot clearer now.
This superseded the documentation change that was in the patch, so I've
removed it from the v3 version.Now that we have tests for this, I think it would be good idea to expand
them to cover the new behavior of TRUNCATE in this patch.
You are right. Attached the new v4 version.
Regards,
Marco
--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
Attachments:
0001-truncate_ignore_fks_role_replica.v4.patchtext/plain; charset=UTF-8; name=0001-truncate_ignore_fks_role_replica.v4.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2e768dd5e4..bdce4164d6 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 1404,1419 **** ExecuteTruncate(TruncateStmt *stmt)
}
/*
! * Check foreign key references. In CASCADE mode, this should be
! * unnecessary since we just pulled in all the references; but as a
! * cross-check, do it anyway if in an Assert-enabled build.
*/
#ifdef USE_ASSERT_CHECKING
- heap_truncate_check_FKs(rels, false);
- #else
- if (stmt->behavior == DROP_RESTRICT)
heap_truncate_check_FKs(rels, false);
#endif
/*
* If we are asked to restart sequences, find all the sequences, lock them
--- 1404,1427 ----
}
/*
! * Suppress foreign key references check if session replication role is
! * set to REPLICA.
*/
+ if (SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA)
+ {
+
+ /*
+ * Check foreign key references. In CASCADE mode, this should be
+ * unnecessary since we just pulled in all the references; but as a
+ * cross-check, do it anyway if in an Assert-enabled build.
+ */
#ifdef USE_ASSERT_CHECKING
heap_truncate_check_FKs(rels, false);
+ #else
+ if (stmt->behavior == DROP_RESTRICT)
+ heap_truncate_check_FKs(rels, false);
#endif
+ }
/*
* If we are asked to restart sequences, find all the sequences, lock them
diff --git a/src/test/regress/expected/index d967e8dd21..86748430c5 100644
*** a/src/test/regress/expected/truncate.out
--- b/src/test/regress/expected/truncate.out
***************
*** 68,73 **** HINT: Truncate table "trunc_b" at the same time, or use TRUNCATE ... CASCADE.
--- 68,77 ----
TRUNCATE TABLE truncate_a CASCADE; -- ok
NOTICE: truncate cascades to table "trunc_b"
NOTICE: truncate cascades to table "trunc_e"
+ -- Ignore foreign-key checks with session_replication_role = replica
+ SET session_replication_role = replica;
+ TRUNCATE TABLE truncate_a; -- ok
+ RESET session_replication_role;
-- circular references
ALTER TABLE truncate_a ADD FOREIGN KEY (col1) REFERENCES trunc_c;
-- Add some data to verify that truncating actually works ...
diff --git a/src/test/regress/sql/truncate.sqindex fbd1d1a8a5..0d0a3705d2 100644
*** a/src/test/regress/sql/truncate.sql
--- b/src/test/regress/sql/truncate.sql
***************
*** 33,38 **** TRUNCATE TABLE trunc_c,trunc_d,trunc_e,truncate_a,trunc_b; -- ok
--- 33,43 ----
TRUNCATE TABLE truncate_a RESTRICT; -- fail
TRUNCATE TABLE truncate_a CASCADE; -- ok
+ -- Ignore foreign-key checks with session_replication_role = replica
+ SET session_replication_role = replica;
+ TRUNCATE TABLE truncate_a; -- ok
+ RESET session_replication_role;
+
-- circular references
ALTER TABLE truncate_a ADD FOREIGN KEY (col1) REFERENCES trunc_c;
On 23/01/18 13:34, Marco Nenciarini wrote:
Il 22/01/18 19:41, Petr Jelinek ha scritto:
On 19/01/18 12:41, Marco Nenciarini wrote:
Hi Peter,
Il 18/01/18 17:30, Peter Eisentraut ha scritto:
On 1/17/18 11:33, Petr Jelinek wrote:
P.S: I'm struggling to understand why we have two possible values of
session_replication_role settings that behave identically (origin and
local). I'm unable to see any difference according to the code or the
documentation, so I'm wondering if we should document that they are the
same.It's for use by 3rd party tools (afaik both londiste and slony use it)
to differentiate between replicated and not replicated changes.I have committed some documentation updates and tests to cover this a
bit better.Thanks, the documentation is a lot clearer now.
This superseded the documentation change that was in the patch, so I've
removed it from the v3 version.Now that we have tests for this, I think it would be good idea to expand
them to cover the new behavior of TRUNCATE in this patch.You are right. Attached the new v4 version.
Thanks, looks good to me, marking as ready for committer.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
So, what should we do here? The argument of the patch is that it makes
the behavior of TRUNCATE consistent with DELETE, which would be good.
But my argument is that the behavior of DELETE is kind of bad, and we
should create more stuff like that. I also haven't seen an argument why
this change is actually necessary.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
This part of the patch didn't end up being necessary, since the updated
implementation of logical decoding of TRUNCATE could do without it.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services