Re: [PATCH] session_replication_role = replica with TRUNCATE

[Peter Eisentraut]

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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Peter Eisentraut]

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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Petr Jelinek]

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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Marco Nenciarini]

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.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
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;
  


signature.asc
Description: OpenPGP digital signature

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Peter Eisentraut]

On 1/18/18 12:41, Tom Lane wrote:
> Peter Eisentraut  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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Peter Eisentraut]

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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Petr Jelinek]

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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Marco Nenciarini]

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.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
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


signature.asc
Description: OpenPGP digital signature

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Tom Lane]

Peter Eisentraut  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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Peter Eisentraut]

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
From aa22bdd6428ae3df1dbf0f0e31f9431dd374d8c5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
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;
+   chartgenabled;
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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Peter Eisentraut]

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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Petr Jelinek]

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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Marco Nenciarini]

Hi,

Il 30/12/17 08:42, Craig Ringer ha scritto:
> On 30 December 2017 at 03:32, Petr Jelinek  > 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.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
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;

 
  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.
--- 6502,6510 

 
  Controls firing of replication-related triggers and rules for the
! current session. When set to replica 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 origin (the 
default),
  replica and local.
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


signature.asc
Description: OpenPGP digital signature

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Craig Ringer]

On 30 December 2017 at 03:32, Petr Jelinek 
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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Petr Jelinek]

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

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Marco Nenciarini]

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.nenciar...@2ndquadrant.it | www.2ndQuadrant.it



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Craig Ringer]

On 29 December 2017 at 22:14, Petr Jelinek 
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