Thread: [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
Attachment
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.
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
Attachment
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.
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
Attachment
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, > 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. > 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
Attachment
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
Attachment
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
Attachment
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