Re: [PATCH] Logical decoding of TRUNCATE
On Mon, Dec 21, 2020 at 09:42:47AM -0800, Andres Freund wrote: > On 2020-12-20 15:54:31 -0800, Peter Geoghegan wrote: > > On Sun, Dec 20, 2020 at 3:13 PM Andres Freund wrote: > > > Hm. Do I understand correctly that this problem is hit solely because > > > the parallel mode code relies on there already have been a transaction > > > snapshot set, thus avoiding the error? And that the code normally only > > > works because GetTransactionSnapshot() will already have been called > > > somewhere, before EnterParallelMode()? > > > > It seems unlikely that InitializeParallelDSM() was ever intended to be > > run in a background worker. > > IDK, the environment in a bgworker shouldn't be that different from the > normal query environment in a normal connection. And it's far from > insane to want to be able to run a paralell query in a bgworker (and I > *think* I have seen that work before). This case here seems more like > an accidental dependency than anything to me, once that could perhaps > even hint at problems in normal backends too. Yeah. SPI_execute("TRUNCATE foo", false, 0) has no trouble doing a parallel index build in a bgworker. Let's ignore intention and be pleased about that.
Re: [PATCH] Logical decoding of TRUNCATE
Hi, On 2020-12-20 15:54:31 -0800, Peter Geoghegan wrote: > On Sun, Dec 20, 2020 at 3:13 PM Andres Freund wrote: > > Hm. Do I understand correctly that this problem is hit solely because > > the parallel mode code relies on there already have been a transaction > > snapshot set, thus avoiding the error? And that the code normally only > > works because GetTransactionSnapshot() will already have been called > > somewhere, before EnterParallelMode()? > > It seems unlikely that InitializeParallelDSM() was ever intended to be > run in a background worker. IDK, the environment in a bgworker shouldn't be that different from the normal query environment in a normal connection. And it's far from insane to want to be able to run a paralell query in a bgworker (and I *think* I have seen that work before). This case here seems more like an accidental dependency than anything to me, once that could perhaps even hint at problems in normal backends too. Greetings, Andres Freund
Re: [PATCH] Logical decoding of TRUNCATE
On Sun, Dec 20, 2020 at 03:54:31PM -0800, Peter Geoghegan wrote: > On Sun, Dec 20, 2020 at 3:13 PM Andres Freund wrote: > > Hm. Do I understand correctly that this problem is hit solely because > > the parallel mode code relies on there already have been a transaction > > snapshot set, thus avoiding the error? And that the code normally only > > works because GetTransactionSnapshot() will already have been called > > somewhere, before EnterParallelMode()? I think so. > It seems unlikely that InitializeParallelDSM() was ever intended to be > run in a background worker. That wouldn't surprise me. Nonetheless, when worker_spi runs parallel queries, they work fine. The logical replication worker experiences novel scenarios, because it calls ExecuteTruncateGuts() directly, not as part of an actual TRUNCATE query. That bypasses some of the usual once-per-query setup. On Mon, Dec 21, 2020 at 12:29:37PM +0530, Amit Kapila wrote: > I think the TRUNCATE operation should not use parallelism either via > apply worker or without it because there is nothing to scan in heap. That's fair. > Additionally, we can have an Assert or elog in InitializeParallelDSM > to ensure that it is never invoked by parallel worker. I don't know whether InitializeParallelDSM() operates correctly from inside a parallel worker. That is orthogonal to the bug here.
Re: [PATCH] Logical decoding of TRUNCATE
On Sun, Dec 20, 2020 at 9:43 AM Noah Misch wrote: > > Since commit 039eb6e added logical replication support for TRUNCATE, logical > apply of the TRUNCATE fails if it chooses a parallel index build: > I think the TRUNCATE operation should not use parallelism either via apply worker or without it because there is nothing to scan in heap. Additionally, we can have an Assert or elog in InitializeParallelDSM to ensure that it is never invoked by parallel worker. -- With Regards, Amit Kapila.
Re: [PATCH] Logical decoding of TRUNCATE
On Sun, Dec 20, 2020 at 3:13 PM Andres Freund wrote: > Hm. Do I understand correctly that this problem is hit solely because > the parallel mode code relies on there already have been a transaction > snapshot set, thus avoiding the error? And that the code normally only > works because GetTransactionSnapshot() will already have been called > somewhere, before EnterParallelMode()? It seems unlikely that InitializeParallelDSM() was ever intended to be run in a background worker. -- Peter Geoghegan
Re: [PATCH] Logical decoding of TRUNCATE
Hi, On 2020-12-20 04:13:19 +, Noah Misch wrote: > postgres: subscriber: logical replication worker for subscription 16411 > (GetTransactionSnapshot+0x168) [0x951ce8] > postgres: subscriber: logical replication worker for subscription 16411 > (InitializeParallelDSM+0x16) [0x52cf86] > postgres: subscriber: logical replication worker for subscription 16411 > (btbuild+0x26a) [0x50905a] > postgres: subscriber: logical replication worker for subscription 16411 > (index_build+0x14b) [0x569c1b] > postgres: subscriber: logical replication worker for subscription 16411 > (reindex_index+0x19a) [0x56caea] > postgres: subscriber: logical replication worker for subscription 16411 > (reindex_relation+0xc0) [0x56d090] > postgres: subscriber: logical replication worker for subscription 16411 > (ExecuteTruncateGuts+0x376) [0x62f0d6] > postgres: subscriber: logical replication worker for subscription 16411 > () [0x78d592] > postgres: subscriber: logical replication worker for subscription 16411 > (ApplyWorkerMain+0x5ab) [0x78e4eb] > postgres: subscriber: logical replication worker for subscription 16411 > (StartBackgroundWorker+0x23f) [0x75522f] > postgres: subscriber: logical replication worker for subscription 16411 > () [0x762a6d] > postgres: subscriber: logical replication worker for subscription 16411 > () [0x7635ee] > /lib64/libpthread.so.0(+0xf630) [0x7fe081e97630] > /lib64/libc.so.6(__select+0x13) [0x7fe0805c0983] > postgres: subscriber: logical replication worker for subscription 16411 > () [0x4887ac] > postgres: subscriber: logical replication worker for subscription 16411 > (PostmasterMain+0x1118) [0x764c88] > postgres: subscriber: logical replication worker for subscription 16411 > (main+0x6f2) [0x48aae2] > /lib64/libc.so.6(__libc_start_main+0xf5) [0x7fe0804ed555] > postgres: subscriber: logical replication worker for subscription 16411 > () [0x48ab49] > 2020-12-19 17:54:04.683 PST [3629353:5] LOG: background worker "logical > replication worker" (PID 3629509) exited with exit code 1 Hm. Do I understand correctly that this problem is hit solely because the parallel mode code relies on there already have been a transaction snapshot set, thus avoiding the error? And that the code normally only works because GetTransactionSnapshot() will already have been called somewhere, before EnterParallelMode()? Greetings, Andres Freund
Re: [PATCH] Logical decoding of TRUNCATE
On Sat, Apr 07, 2018 at 07:40:11PM -0400, Peter Eisentraut wrote: > Committed with those changes. Since commit 039eb6e added logical replication support for TRUNCATE, logical apply of the TRUNCATE fails if it chooses a parallel index build: cat >/tmp/most_parallel.conf <
Re: [PATCH] Logical decoding of TRUNCATE
On 4/5/18 13:07, Alvaro Herrera wrote: > Note that you start the loop having the Relation; yet you go extra > length to grab the relnamespace and relname from syscache instead of > just relations[i]->rd_rel->relname etc. fixed > Maybe not a big deal, but for future pg_waldump users I'm sure it'll be > nice to have the OIDs here. done >> +void >> +ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged, >> +DropBehavior behavior, >> bool restart_seqs) >> +{ > > Please add a comment atop this function. done >> +/* >> + * Write a WAL record to allow this set of actions to be logically >> decoded. >> + * >> + * Assemble an array of relids so we can write a single WAL record for >> the >> + * whole action. >> + */ >> +if (list_length(relids_logged) > 0) >> +{ >> +xl_heap_truncate xlrec; >> +int i = 0; > > I wonder if this should happen only if logical decoding? (Maybe it > already occurs because relids_logged would be empty? Worth a comment in > that case) Added an assertion and a comment. Committed with those changes. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
This sounds like a good approach. > +static void > +pg_decode_truncate(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, > +int nrelations, Relation relations[], > ReorderBufferChange *change) > +{ > + for (i = 0; i < nrelations; i++) > + { > + Oid relid = RelationGetRelid(relations[i]); > + > + if (i > 0) > + appendStringInfoString(ctx->out, ", "); > + > + appendStringInfoString(ctx->out, > + > quote_qualified_identifier( > + > get_namespace_name(get_rel_namespace(relid)), > + > get_rel_name(relid))); Note that you start the loop having the Relation; yet you go extra length to grab the relnamespace and relname from syscache instead of just relations[i]->rd_rel->relname etc. pgoutput doesn't do it that way, so it doesn't affect logical replication, but I think it's best not to create awkward code in test_decoding, since it's so widely copied. > + else if (info == XLOG_HEAP_TRUNCATE) > + { > + xl_heap_truncate *xlrec = (xl_heap_truncate *) rec; > + > + if (xlrec->flags & XLH_TRUNCATE_CASCADE) > + appendStringInfo(buf, "cascade "); > + if (xlrec->flags & XLH_TRUNCATE_RESTART_SEQS) > + appendStringInfo(buf, "restart_seqs "); > + appendStringInfo(buf, "nrelids %u", xlrec->nrelids); > + /* skip the list of relids */ > + } Maybe not a big deal, but for future pg_waldump users I'm sure it'll be nice to have the OIDs here. > +void > +ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged, > + DropBehavior behavior, > bool restart_seqs) > +{ Please add a comment atop this function. > + /* > + * Write a WAL record to allow this set of actions to be logically > decoded. > + * > + * Assemble an array of relids so we can write a single WAL record for > the > + * whole action. > + */ > + if (list_length(relids_logged) > 0) > + { > + xl_heap_truncate xlrec; > + int i = 0; I wonder if this should happen only if logical decoding? (Maybe it already occurs because relids_logged would be empty? Worth a comment in that case) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
On 4/3/18 22:31, Peter Eisentraut wrote: > The problem I see now is that when we WAL-log a truncate, we include all > the relids in one record. That seems useful. But during decoding we > split this into one change per relid. So at the receiving end, truncate > can only process one relation at a time, which will fail if there are > foreign keys involved. The solution that had been proposed here was to > ignore foreign keys on the subscriber, which is clearly problematic. > I'm going to try to hack up an alternative approach and see how it works > out. Done here. I added a separate callback for decoding truncates, which receives all the relations at once. That information can then be shipped off together and applied together on the receiving side. So foreign keys are not a problem anymore. This ended up being a bit larger than the original patch, but I think it's sound behavior and future-proof. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services From df101e32c358ac9243285d4e8f125803988e5508 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 5 Apr 2018 11:46:41 -0400 Subject: [PATCH v19 1/2] Logical decoding of TRUNCATE --- contrib/test_decoding/Makefile| 2 +- contrib/test_decoding/expected/truncate.out | 25 ++ contrib/test_decoding/sql/truncate.sql| 10 +++ contrib/test_decoding/test_decoding.c | 61 + doc/src/sgml/logicaldecoding.sgml | 27 +- src/backend/access/heap/heapam.c | 7 ++ src/backend/access/rmgrdesc/heapdesc.c| 14 +++ src/backend/commands/tablecmds.c | 87 +-- src/backend/replication/logical/decode.c | 41 + src/backend/replication/logical/logical.c | 43 + .../replication/logical/reorderbuffer.c | 35 src/include/access/heapam_xlog.h | 23 - src/include/commands/tablecmds.h | 2 + src/include/replication/output_plugin.h | 10 +++ src/include/replication/reorderbuffer.h | 24 - 15 files changed, 398 insertions(+), 13 deletions(-) create mode 100644 contrib/test_decoding/expected/truncate.out create mode 100644 contrib/test_decoding/sql/truncate.sql diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile index 6c18189d9d..1d601d8144 100644 --- a/contrib/test_decoding/Makefile +++ b/contrib/test_decoding/Makefile @@ -39,7 +39,7 @@ submake-test_decoding: REGRESSCHECKS=ddl xact rewrite toast permissions decoding_in_xact \ decoding_into_rel binary prepared replorigin time messages \ - spill slot + spill slot truncate regresscheck: | submake-regress submake-test_decoding temp-install $(pg_regress_check) \ diff --git a/contrib/test_decoding/expected/truncate.out b/contrib/test_decoding/expected/truncate.out new file mode 100644 index 00..be85178206 --- /dev/null +++ b/contrib/test_decoding/expected/truncate.out @@ -0,0 +1,25 @@ +SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding'); + ?column? +-- + init +(1 row) + +CREATE TABLE tab1 (id serial unique, data int); +CREATE TABLE tab2 (a int primary key, b int); +TRUNCATE tab1; +TRUNCATE tab1, tab1 RESTART IDENTITY CASCADE; +TRUNCATE tab1, tab2; +SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1'); + data +-- + BEGIN + table public.tab1: TRUNCATE: (no-flags) + COMMIT + BEGIN + table public.tab1: TRUNCATE: restart_seqs cascade + COMMIT + BEGIN + table public.tab1, public.tab2: TRUNCATE: (no-flags) + COMMIT +(9 rows) + diff --git a/contrib/test_decoding/sql/truncate.sql b/contrib/test_decoding/sql/truncate.sql new file mode 100644 index 00..88f113fd5b --- /dev/null +++ b/contrib/test_decoding/sql/truncate.sql @@ -0,0 +1,10 @@ +SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding'); + +CREATE TABLE tab1 (id serial unique, data int); +CREATE TABLE tab2 (a int primary key, b int); + +TRUNCATE tab1; +TRUNCATE tab1, tab1 RESTART IDENTITY CASCADE; +TRUNCATE tab1, tab2; + +SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1'); diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c index a94aeeae29..c238f12e66 100644 --- a/contrib/test_decoding/test_decoding.c +++ b/contrib/test_decoding/test_decoding.c @@ -52,6 +52,10 @@ static void pg_decode_commit_txn(LogicalDecodingContext *ctx, static void pg_decode_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, Relation rel, ReorderBufferChange *change); +static void pg_decode_truncate(LogicalDecodingContext *ctx,
Re: [PATCH] Logical decoding of TRUNCATE
On 4 April 2018 at 03:31, Peter Eisentraut wrote: > I wonder why this approach was chosen. I looked at coding it that way and it seemed worse than the way chosen. > I'm going to try to hack up an alternative approach and see how it works > out. If you add a new filter for TRUNCATE it will mean compatibility problems and the need for pg_dump support. Need note in release notes to explain that people will need to add TRUNCATE filter to their existing publications. I was hoping to have that picked up automatically, which can't happen if we have an explicit filter for it. >> I know this has been discussed in the thread already, but it really >> strikes me as wrong to basically do some mini DDL replication feature >> via per-command WAL records. Don't really understand that comment. > I think TRUNCATE is special in some ways and it's reasonable to treat it > specially. Real DDL is being worked on in the 2PC decoding thread. > What we are discussing here isn't going to be applicable there and vice > versa, I think. In fact, one of the reasons for this effort is that in > BDR TRUNCATE is currently handled like a DDL command, which doesn't work > well. Agreed -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
Here is an updated patch that addresses some of your concerns. I've split it up into the decoding support and the pgoutput replication support. The problem I see now is that when we WAL-log a truncate, we include all the relids in one record. That seems useful. But during decoding we split this into one change per relid. So at the receiving end, truncate can only process one relation at a time, which will fail if there are foreign keys involved. The solution that had been proposed here was to ignore foreign keys on the subscriber, which is clearly problematic. I wonder why this approach was chosen. If we go through the trouble of WAL-logging all the relations together, why not present them to the output plugin as one so that they can be applied together? This will clearly make questions of filtering and replication set membership and so on more difficult, but that's just the nature of the thing. If you are connecting tables by foreign keys and only replicate some of them, then that's going to have confusing effects no matter what you do. (That's perhaps another reason why having the option of replicating truncate separately from delete could be useful.) I'm going to try to hack up an alternative approach and see how it works out. On 4/1/18 16:01, Andres Freund wrote: > On 2018-01-25 14:21:15 +0100, Marco Nenciarini wrote: >> +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 >> +} > > That *can't* be right. This is actually existing code that just looks funny in the diff after being indented. But I'd rather skip this patch altogether and find a different solution; see above. > I know this has been discussed in the thread already, but it really > strikes me as wrong to basically do some mini DDL replication feature > via per-command WAL records. I think TRUNCATE is special in some ways and it's reasonable to treat it specially. Real DDL is being worked on in the 2PC decoding thread. What we are discussing here isn't going to be applicable there and vice versa, I think. In fact, one of the reasons for this effort is that in BDR TRUNCATE is currently handled like a DDL command, which doesn't work well. >> + >> +TRUNCATE is treated as a form of >> +DELETE for the purpose of deciding whether >> +to publish, or not. >> + >> >> >> > > Why is this a good idea? I have changed this by making this a separate property. > Hm, it seems logicaldecoding.sgml hasn't been updated? I re-checked but didn't find anything suitable to update. >> + void >> + ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged, >> +DropBehavior behavior, >> bool restart_seqs) >> + { >> +List *rels = list_copy(explicit_rels); > > Why is this copied? Because it is overwritten later. I have moved it down a bit to make that a bit clearer. >> + * Write a WAL record to allow this set of actions to be logically >> decoded. >> + * We could optimize this away when !RelationIsLogicallyLogged(rel) >> + * but that doesn't save much space or time. > > What you're saying isn't that you're not logging anything, but that > you're allocating the header regardless? Because this certainly sounds > like you unconditionally log a WAL record. > I'm confused. Why do we need the resizing here, when we know the max > upfront? I have rewritten this a bit and removed the logging of the sequence relids, which isn't used anywhere after, to make the code a bit simpler. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services From 72577e567725526543e5493e235c1059610f3467 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Tue, 3 Apr 2018 22:12:00 -0400 Subject: [PATCH v18 1/2] Logical decoding of TRUNCATE --- contrib/test_decoding/expected/ddl.out| 10 ++- contrib/test_decoding/sql/ddl.sql | 3 + contrib/test_decoding/test_decoding.c | 14 +++ src/backend/access/heap/heapam.c | 7 ++ src/backend/access/rmgrdesc/heapdesc.c| 14 +++ src/backend/commands/tablecmds.c | 87 +-- src/backend/replication/logical/decode.c | 46 ++ .../replication/logical/reorderbuffer.c | 10 +++ src/include/access/heapam_xlog.h | 23 - src/include/comman
Re: [PATCH] Logical decoding of TRUNCATE
> On Apr 2, 2018, at 11:50, Andres Freund wrote: > I'm not convinced. I think it's perfectly reasonable to want to truncate > away data on the live node, but maintain it on an archival node. +1. We've had at least one specific request for this, in maintaining a data warehouse from an OLTP system. -- -- Christophe Pettus x...@thebuild.com
Re: [PATCH] Logical decoding of TRUNCATE
Hi, On 2018-04-02 07:39:57 +0100, Simon Riggs wrote: > On 1 April 2018 at 21:01, Andres Freund wrote: > > >> *** > >> *** 111,116 CREATE PUBLICATION >> class="parameter">name > >> --- 111,121 > >> and so the default value for this option is > >> 'insert, update, delete'. > >> > >> + > >> +TRUNCATE is treated as a form of > >> +DELETE for the purpose of deciding whether > >> +to publish, or not. > >> + > >> > >> > >> > > > > Why is this a good idea? > > TRUNCATE removes rows, just as DELETE does, so anybody that wants to > publish the removal of rows will be interested in this. I'm not convinced. I think it's perfectly reasonable to want to truncate away data on the live node, but maintain it on an archival node. In that case one commonly wants to continue to maintain OLTP changes (hence DELETE), but not the bulk truncation. I think there's a reasonable counter-argument in that this isn't fine grained enough. > >> + * Write a WAL record to allow this set of actions to be logically > >> decoded. > >> + * We could optimize this away when !RelationIsLogicallyLogged(rel) > >> + * but that doesn't save much space or time. > > > > What you're saying isn't that you're not logging anything, but that > > you're allocating the header regardless? Because this certainly sounds > > like you unconditionally log a WAL record. > > It says that, yes, my idea - as explained. My complaint is that the comment and the actual implementation differ. The above sounds like it's unconditionally WAL logging, but: + /* +* Write a WAL record to allow this set of actions to be logically decoded. +* We could optimize this away when !RelationIsLogicallyLogged(rel) +* but that doesn't save much space or time. +* +* Assemble an array of relids, then an array of seqrelids so we can write +* a single WAL record for the whole action. +*/ + logrelids = palloc(maxrelids * sizeof(Oid)); + foreach (cell, relids_logged) + { + nrelids++; + if (nrelids > maxrelids) + { + maxrelids *= 2; + logrelids = repalloc(logrelids, maxrelids * sizeof(Oid)); + } + logrelids[nrelids - 1] = lfirst_oid(cell); + } + + foreach (cell, seq_relids_logged) + { + nseqrelids++; + if ((nrelids + nseqrelids) > maxrelids) + { + maxrelids *= 2; + logrelids = repalloc(logrelids, maxrelids * sizeof(Oid)); + } + logrelids[nrelids + nseqrelids - 1] = lfirst_oid(cell); + } + + if ((nrelids + nseqrelids) > 0) + { + xl_heap_truncate xlrec; + + xlrec.dbId = MyDatabaseId; + xlrec.nrelids = nrelids; + xlrec.nseqrelids = nseqrelids; + xlrec.flags = 0; + if (behavior == DROP_CASCADE) + xlrec.flags |= XLH_TRUNCATE_CASCADE; + if (restart_seqs) + xlrec.flags |= XLH_TRUNCATE_RESTART_SEQS; + + XLogBeginInsert(); + XLogRegisterData((char *) &xlrec, SizeOfHeapTruncate); + XLogRegisterData((char *) logrelids, + (nrelids + nseqrelids) * sizeof(Oid)); + + XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN); + + (void) XLogInsert(RM_HEAP_ID, XLOG_HEAP_TRUNCATE); + } Note that the XLogInsert() is in an if branch that's only executed if there's either logged relids or sequence relids. I think logrelids should be allocated at the max size immediately, and the comment rewritten to explicitly only talk about the allocation, rather than sounding like it's about WAL logging as well. Greetings, Andres Freund
Re: [PATCH] Logical decoding of TRUNCATE
Hi, On 2018-04-02 14:30:50 -0400, Peter Eisentraut wrote: > On 4/1/18 16:01, Andres Freund wrote: > > On 2018-01-25 14:21:15 +0100, Marco Nenciarini wrote: > >> + 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 > >> + } > > > > That *can't* be right. > > You mean the part that ignores this in session_replication_role = > replica? I don't like that either. And it's also not clear why it's > needed for this feature. I primarily the way the code is written. For me code differing like this between USE_ASSERT_CHECKING and not seems like a recipe for disaster. And yea, I'm not sure why the session_replication_role bit is here either. > > I know this has been discussed in the thread already, but it really > > strikes me as wrong to basically do some mini DDL replication feature > > via per-command WAL records. > > The problem is that the interaction of TRUNCATE and foreign keys is a mess. > > Let's say I have a provider database with table1, table2, and table3, > all connected by foreign keys, and a replica database with table1, > table2, and table4, all connected by foreign keys. I run TRUNCATE > table1 CASCADE on the provider. What should replication do? > > The proposed patch applies the TRUNCATE table1 CASCADE on the replica, > which ends up truncating table1, table2, and table4, which is different > from what happened on the origin. I actually think that's a fairly sane behaviour because it allows you to have different schemas on both sides and still deal in a reasonably sane manner. What I'm concerned about is more that we're developing a one-of DDL replication feature w/ corresponding bespoke WAL-logging. > An alternative might be to make the replication protocol message say "I > truncated table1, table2" (let's say table3 is not replicated). (This > sounds more like logical replication rather than DDL replication.) That > will then fail to apply on the replica, because it requires that you > include all tables connected by foreign keys. And entirely fails if there's schema differences. > We could then do what we do in the DELETE case and just ignore foreign > keys altogether, which is obviously bad and not a forward-looking behavior. > > Or we could do what we *should* be doing in the DELETE case and apply > cascading actions on the replica to table4, but that kind of row-by-row > processing is what we want to avoid by using TRUNCATE in the first place. Well, you could also queue a re-check at the end of the processed message, doing a bulk re-check at the end. But that's obviously more work. > >> + > >> +TRUNCATE is treated as a form of > >> +DELETE for the purpose of deciding whether > >> +to publish, or not. > >> + > >> > >> > >> > > > > Why is this a good idea? > > I think it seemed like a good idea at the time, so to speak, but several > people have argued against it, so we should probably change this in the > final version. I think it's e.g. perfectly reasonable to want OLTP changes to be replicated, but not to truncate historical data. So for me those actions should be separate... Greetings, Andres Freund
Re: [PATCH] Logical decoding of TRUNCATE
On 2 April 2018 at 19:30, Peter Eisentraut wrote: >>> *** >>> *** 111,116 CREATE PUBLICATION >> class="parameter">name >>> --- 111,121 >>> and so the default value for this option is >>> 'insert, update, delete'. >>> >>> + >>> +TRUNCATE is treated as a form of >>> +DELETE for the purpose of deciding whether >>> +to publish, or not. >>> + >>> >>> >>> >> >> Why is this a good idea? > > I think it seemed like a good idea at the time, so to speak, but several > people have argued against it, so we should probably change this in the > final version. Who has argued against it? I see that Andres has asked twice about it and been answered twice, but expressed no opinion. Petr has said he thinks that's right. Nobody else has commented. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
On 4/1/18 16:01, Andres Freund wrote: > On 2018-01-25 14:21:15 +0100, Marco Nenciarini wrote: >> +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 >> +} > > That *can't* be right. You mean the part that ignores this in session_replication_role = replica? I don't like that either. And it's also not clear why it's needed for this feature. >> +case REORDER_BUFFER_CHANGE_TRUNCATE: >> +appendStringInfoString(ctx->out, " TRUNCATE:"); >> + >> +if (change->data.truncate_msg.restart_seqs >> +|| change->data.truncate_msg.cascade) >> +{ >> +if (change->data.truncate_msg.restart_seqs) >> +appendStringInfo(ctx->out, " >> restart_seqs"); >> +if (change->data.truncate_msg.cascade) >> +appendStringInfo(ctx->out, " cascade"); >> +} >> +else >> +appendStringInfoString(ctx->out, " (no-flags)"); >> +break; >> default: >> Assert(false); >> } > > I know this has been discussed in the thread already, but it really > strikes me as wrong to basically do some mini DDL replication feature > via per-command WAL records. The problem is that the interaction of TRUNCATE and foreign keys is a mess. Let's say I have a provider database with table1, table2, and table3, all connected by foreign keys, and a replica database with table1, table2, and table4, all connected by foreign keys. I run TRUNCATE table1 CASCADE on the provider. What should replication do? The proposed patch applies the TRUNCATE table1 CASCADE on the replica, which ends up truncating table1, table2, and table4, which is different from what happened on the origin. An alternative might be to make the replication protocol message say "I truncated table1, table2" (let's say table3 is not replicated). (This sounds more like logical replication rather than DDL replication.) That will then fail to apply on the replica, because it requires that you include all tables connected by foreign keys. We could then do what we do in the DELETE case and just ignore foreign keys altogether, which is obviously bad and not a forward-looking behavior. Or we could do what we *should* be doing in the DELETE case and apply cascading actions on the replica to table4, but that kind of row-by-row processing is what we want to avoid by using TRUNCATE in the first place. None of these solutions are good. I don't have any other ideas, though. >> *** >> *** 111,116 CREATE PUBLICATION > class="parameter">name >> --- 111,121 >> and so the default value for this option is >> 'insert, update, delete'. >> >> + >> +TRUNCATE is treated as a form of >> +DELETE for the purpose of deciding whether >> +to publish, or not. >> + >> >> >> > > Why is this a good idea? I think it seemed like a good idea at the time, so to speak, but several people have argued against it, so we should probably change this in the final version. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
On 1 April 2018 at 21:01, Andres Freund wrote: >> *** >> *** 111,116 CREATE PUBLICATION > class="parameter">name >> --- 111,121 >> and so the default value for this option is >> 'insert, update, delete'. >> >> + >> +TRUNCATE is treated as a form of >> +DELETE for the purpose of deciding whether >> +to publish, or not. >> + >> >> >> > > Why is this a good idea? TRUNCATE removes rows, just as DELETE does, so anybody that wants to publish the removal of rows will be interested in this. This avoids unnecessary overcomplication of the existing interface. >> + * Write a WAL record to allow this set of actions to be logically >> decoded. >> + * We could optimize this away when !RelationIsLogicallyLogged(rel) >> + * but that doesn't save much space or time. > > What you're saying isn't that you're not logging anything, but that > you're allocating the header regardless? Because this certainly sounds > like you unconditionally log a WAL record. It says that, yes, my idea - as explained. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
On 2018-01-25 14:21:15 +0100, Marco Nenciarini wrote: > + 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 > + } That *can't* be right. > + case REORDER_BUFFER_CHANGE_TRUNCATE: > + appendStringInfoString(ctx->out, " TRUNCATE:"); > + > + if (change->data.truncate_msg.restart_seqs > + || change->data.truncate_msg.cascade) > + { > + if (change->data.truncate_msg.restart_seqs) > + appendStringInfo(ctx->out, " > restart_seqs"); > + if (change->data.truncate_msg.cascade) > + appendStringInfo(ctx->out, " cascade"); > + } > + else > + appendStringInfoString(ctx->out, " (no-flags)"); > + break; > default: > Assert(false); > } I know this has been discussed in the thread already, but it really strikes me as wrong to basically do some mini DDL replication feature via per-command WAL records. > *** > *** 111,116 CREATE PUBLICATION class="parameter">name > --- 111,121 > and so the default value for this option is > 'insert, update, delete'. > > + > +TRUNCATE is treated as a form of > +DELETE for the purpose of deciding whether > +to publish, or not. > + > > > Why is this a good idea? Hm, it seems logicaldecoding.sgml hasn't been updated? > + void > + ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged, > + DropBehavior behavior, > bool restart_seqs) > + { > + List *rels = list_copy(explicit_rels); Why is this copied? > + * Write a WAL record to allow this set of actions to be logically > decoded. > + * We could optimize this away when !RelationIsLogicallyLogged(rel) > + * but that doesn't save much space or time. What you're saying isn't that you're not logging anything, but that you're allocating the header regardless? Because this certainly sounds like you unconditionally log a WAL record. > + * Assemble an array of relids, then an array of seqrelids so we can > write > + * a single WAL record for the whole action. > + */ > + logrelids = palloc(maxrelids * sizeof(Oid)); > + foreach (cell, relids_logged) > + { > + nrelids++; > + if (nrelids > maxrelids) > + { > + maxrelids *= 2; > + logrelids = repalloc(logrelids, maxrelids * > sizeof(Oid)); > + } > + logrelids[nrelids - 1] = lfirst_oid(cell); > + } > + > + foreach (cell, seq_relids_logged) > + { > + nseqrelids++; > + if ((nrelids + nseqrelids) > maxrelids) > + { > + maxrelids *= 2; > + logrelids = repalloc(logrelids, maxrelids * > sizeof(Oid)); > + } > + logrelids[nrelids + nseqrelids - 1] = lfirst_oid(cell); > + } I'm confused. Why do we need the resizing here, when we know the max upfront? > + /* > + * For truncate we list all truncated relids in an array, followed by all > + * sequence relids that need to be restarted, if any. > + * All rels are always within the same database, so we just list dbid once. > + */ > + typedef struct xl_heap_truncate > + { > + Oid dbId; > + uint32 nrelids; > + uint32 nseqrelids; > + uint8 flags; > + Oid relids[FLEXIBLE_ARRAY_MEMBER]; > + } xl_heap_truncate; Given that the space is used anyway due to padding, I'd just make flags 32bit. Greetings, Andres Freund
Re: [PATCH] Logical decoding of TRUNCATE
On 26/01/18 03:44, Peter Eisentraut wrote: > On 1/24/18 07:53, Petr Jelinek wrote: >> That depends on if we consider this to be part of sequence handling or >> truncate statement replication handling. It's true that if we had >> sequence replication, the restart would get propagated that way anyway. >> OTOH, if we'll want to add option in the future to propagate cascade (to >> any additional tables on downstream), it may need to reset sequences >> which are not even present upstream and as such would not be handled by >> sequence replication. > > Logical replication, as it currently is designed, replicates discrete > actions, not commands. A delete command that deletes five rows and > cascades to delete three more rows somewhere else ends up being > replicated as eight changes. So I think a TRUNCATE command that > cascades to four tables and resets six sequences should end up being > replicated as ten changes. (Since we currently don't handle sequences > at all, we'll omit the six sequence changes for now.) > Well, that depends, I think there are two separate problems a) decoding b) replication. I think it's useful for plugins to know if reset sequence or cascade was specified in the command so I think it should be decoded. Some plugins will definitely want to forward that info. But it's true that since we don't handle sequences in logical replication, the sequence reset does not need to be replicated there. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
On 1/24/18 07:53, Petr Jelinek wrote: > That depends on if we consider this to be part of sequence handling or > truncate statement replication handling. It's true that if we had > sequence replication, the restart would get propagated that way anyway. > OTOH, if we'll want to add option in the future to propagate cascade (to > any additional tables on downstream), it may need to reset sequences > which are not even present upstream and as such would not be handled by > sequence replication. Logical replication, as it currently is designed, replicates discrete actions, not commands. A delete command that deletes five rows and cascades to delete three more rows somewhere else ends up being replicated as eight changes. So I think a TRUNCATE command that cascades to four tables and resets six sequences should end up being replicated as ten changes. (Since we currently don't handle sequences at all, we'll omit the six sequence changes for now.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
On Thu, Jan 25, 2018 at 8:36 PM, Petr Jelinek wrote: > On 26/01/18 02:34, Robert Haas wrote: >> On Wed, Jan 17, 2018 at 12:07 PM, Petr Jelinek >> wrote: >>> The patch will cascade truncation on downstream if cascade was specified >>> on the upstream, that can potentially be dangerous and we either should >>> not do it and only truncate the tables which were truncated upstream >>> (but without restricting because of FKs), leaving the data inconsistent >>> on downstream (like we do already with DELETE or UPDATE). Or maybe make >>> it into either subscription or publication option so that user can chose >>> the behaviour here as I am sure some people will want it to cascade (but >>> the default should still IMHO be to not cascade as that's safer). >> >> Maybe I'm not understanding what is being proposed here, but it sounds >> like you're saying that if somebody removes a bunch of data on the >> logical master, replication will remove only some of it from the >> servers to which the change is replicated. That seems crazy. Then >> replication can't be counted on to produce a replica. >> > No, I was talking about extra tables that might be present on downstream > which weren't truncated on upstream. Oh. That's different. :-) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH] Logical decoding of TRUNCATE
On 26/01/18 02:34, Robert Haas wrote: > On Wed, Jan 17, 2018 at 12:07 PM, Petr Jelinek > wrote: >> The patch will cascade truncation on downstream if cascade was specified >> on the upstream, that can potentially be dangerous and we either should >> not do it and only truncate the tables which were truncated upstream >> (but without restricting because of FKs), leaving the data inconsistent >> on downstream (like we do already with DELETE or UPDATE). Or maybe make >> it into either subscription or publication option so that user can chose >> the behaviour here as I am sure some people will want it to cascade (but >> the default should still IMHO be to not cascade as that's safer). > > Maybe I'm not understanding what is being proposed here, but it sounds > like you're saying that if somebody removes a bunch of data on the > logical master, replication will remove only some of it from the > servers to which the change is replicated. That seems crazy. Then > replication can't be counted on to produce a replica. > No, I was talking about extra tables that might be present on downstream which weren't truncated on upstream. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
On Wed, Jan 17, 2018 at 12:07 PM, Petr Jelinek wrote: > The patch will cascade truncation on downstream if cascade was specified > on the upstream, that can potentially be dangerous and we either should > not do it and only truncate the tables which were truncated upstream > (but without restricting because of FKs), leaving the data inconsistent > on downstream (like we do already with DELETE or UPDATE). Or maybe make > it into either subscription or publication option so that user can chose > the behaviour here as I am sure some people will want it to cascade (but > the default should still IMHO be to not cascade as that's safer). Maybe I'm not understanding what is being proposed here, but it sounds like you're saying that if somebody removes a bunch of data on the logical master, replication will remove only some of it from the servers to which the change is replicated. That seems crazy. Then replication can't be counted on to produce a replica. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH] Logical decoding of TRUNCATE
Il 25/01/18 13:18, Petr Jelinek ha scritto: > On 25/01/18 08:26, Thomas Munro wrote: >> On Wed, Jan 24, 2018 at 6:47 AM, Marco Nenciarini >> wrote: >>> Version 16 attached. >> >> Hi Marco, >> >> FYI this version doesn't compile: >> >> worker.c: In function ‘apply_handle_truncate’: >> worker.c:888:39: error: ‘cascade’ undeclared (first use in this function) >> relid = logicalrep_read_truncate(s, &cascade, &restart_seqs); >>^ >> > Fixed. > Indeed. > > We also found one more issue - the truncate done by logical replication > worker is not logically logged which would break cascading. > Fixed. > I expect Marco to send new version shortly with both of these fixed. > New patch v17 attached. 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; diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out index 1e22c1eefc..d1feea4909 100644 *** a/contrib/test_decoding/expected/ddl.out --- b/contrib/test_decoding/expected/ddl.out *** *** 543,548 UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2; --- 543,550 UPDATE table_with_unique_not_null SET id = -id; UPDATE table_with_unique_not_null SET id = -id; DELETE FROM table_with_unique_not_null WHERE data = 3; + TRUNCATE table_with_unique_not_null; + TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART IDENTITY CASCADE; -- check toast support BEGIN; CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable "random" *** *** 660,665 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc --- 662,673 table public.table_with_unique_not_null: DELETE: id[integer]:4 COMMIT BEGIN + table public.table_with_unique_no
Re: [PATCH] Logical decoding of TRUNCATE
On 25/01/18 08:26, Thomas Munro wrote: > On Wed, Jan 24, 2018 at 6:47 AM, Marco Nenciarini > wrote: >> Version 16 attached. > > Hi Marco, > > FYI this version doesn't compile: > > worker.c: In function ‘apply_handle_truncate’: > worker.c:888:39: error: ‘cascade’ undeclared (first use in this function) > relid = logicalrep_read_truncate(s, &cascade, &restart_seqs); >^ > Indeed. We also found one more issue - the truncate done by logical replication worker is not logically logged which would break cascading. I expect Marco to send new version shortly with both of these fixed. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
On Wed, Jan 24, 2018 at 6:47 AM, Marco Nenciarini wrote: > Version 16 attached. Hi Marco, FYI this version doesn't compile: worker.c: In function ‘apply_handle_truncate’: worker.c:888:39: error: ‘cascade’ undeclared (first use in this function) relid = logicalrep_read_truncate(s, &cascade, &restart_seqs); ^ -- Thomas Munro http://www.enterprisedb.com
Re: [PATCH] Logical decoding of TRUNCATE
On 24/01/18 13:50, Peter Eisentraut wrote: > I wonder whether this should be dealing with sequences at all. We are > not currently publishing any information about sequences, so it seems > weird to do it only here. Also, I'd imagine that if we ever get to > publishing sequence events, then the sequence restarts would be > published as independent events. > That depends on if we consider this to be part of sequence handling or truncate statement replication handling. It's true that if we had sequence replication, the restart would get propagated that way anyway. OTOH, if we'll want to add option in the future to propagate cascade (to any additional tables on downstream), it may need to reset sequences which are not even present upstream and as such would not be handled by sequence replication. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
I wonder whether this should be dealing with sequences at all. We are not currently publishing any information about sequences, so it seems weird to do it only here. Also, I'd imagine that if we ever get to publishing sequence events, then the sequence restarts would be published as independent events. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
Il 23/01/18 18:25, Petr Jelinek ha scritto: > On 23/01/18 18:19, Marco Nenciarini wrote: >> Il 23/01/18 18:13, Petr Jelinek ha scritto: >>> Hi, >>> >>> On 23/01/18 15:38, Marco Nenciarini wrote: Il 22/01/18 23:18, Petr Jelinek ha scritto: > On 22/01/18 19:45, Petr Jelinek wrote: > > Actually on second look, I don't like the new boolean parameter much. > I'd rather we didn't touch the input list and always close only > relations opened inside the ExecuteTruncateGuts(). > > It may mean more list(s) but the current interface is still not clean. > Now ExecuteTruncateGuts unconditionally closes the relations that it opens. The caller has now always the responsibility to close the relations passed with the explicit_rels list. >>> >>> This looks good. >>> Version 15 attached. >>> >>> I see you still do CASCADE on the subscriber though. >>> >> >> No it doesn't. The following code in worker.c prevents that. >> >> >> +/* >> + * Even if we used CASCADE on the upstream master we explicitly >> + * default to replaying changes without further cascading. >> + * This might be later changeable with a user specified option. >> + */ >> +cascade = false; > > Ah, that's pretty ugly, why don't we just use DROP_RESTRICT always > instead of this (keeping the comment). Unless you plan to make it option > as part of this patch, the current coding is confusing. > Ok, Removed. Version 16 attached. 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; diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out index 1e22c1eefc..d1feea4909 100644 *** a/contrib/test_decoding/expected/ddl.out --- b/contrib/test_decoding/expected/ddl.out *** *** 543,54
Re: [PATCH] Logical decoding of TRUNCATE
Hi, On 23/01/18 18:47, Marco Nenciarini wrote: > Il 23/01/18 18:25, Petr Jelinek ha scritto: >> On 23/01/18 18:19, Marco Nenciarini wrote: >>> Il 23/01/18 18:13, Petr Jelinek ha scritto: Hi, On 23/01/18 15:38, Marco Nenciarini wrote: > Il 22/01/18 23:18, Petr Jelinek ha scritto: >> On 22/01/18 19:45, Petr Jelinek wrote: >> >> Actually on second look, I don't like the new boolean parameter much. >> I'd rather we didn't touch the input list and always close only >> relations opened inside the ExecuteTruncateGuts(). >> >> It may mean more list(s) but the current interface is still not clean. >> > > Now ExecuteTruncateGuts unconditionally closes the relations that it > opens. The caller has now always the responsibility to close the > relations passed with the explicit_rels list. This looks good. > > Version 15 attached. > I see you still do CASCADE on the subscriber though. >>> >>> No it doesn't. The following code in worker.c prevents that. >>> >>> >>> + /* >>> +* Even if we used CASCADE on the upstream master we explicitly >>> +* default to replaying changes without further cascading. >>> +* This might be later changeable with a user specified option. >>> +*/ >>> + cascade = false; >> >> Ah, that's pretty ugly, why don't we just use DROP_RESTRICT always >> instead of this (keeping the comment). Unless you plan to make it option >> as part of this patch, the current coding is confusing. >> > > Ok, Removed. > Great, thanks, I think this is ready now so marking as such. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
On 23/01/18 18:19, Marco Nenciarini wrote: > Il 23/01/18 18:13, Petr Jelinek ha scritto: >> Hi, >> >> On 23/01/18 15:38, Marco Nenciarini wrote: >>> Il 22/01/18 23:18, Petr Jelinek ha scritto: On 22/01/18 19:45, Petr Jelinek wrote: Actually on second look, I don't like the new boolean parameter much. I'd rather we didn't touch the input list and always close only relations opened inside the ExecuteTruncateGuts(). It may mean more list(s) but the current interface is still not clean. >>> >>> Now ExecuteTruncateGuts unconditionally closes the relations that it >>> opens. The caller has now always the responsibility to close the >>> relations passed with the explicit_rels list. >> >> This looks good. >> >>> >>> Version 15 attached. >>> >> >> I see you still do CASCADE on the subscriber though. >> > > No it doesn't. The following code in worker.c prevents that. > > > + /* > + * Even if we used CASCADE on the upstream master we explicitly > + * default to replaying changes without further cascading. > + * This might be later changeable with a user specified option. > + */ > + cascade = false; Ah, that's pretty ugly, why don't we just use DROP_RESTRICT always instead of this (keeping the comment). Unless you plan to make it option as part of this patch, the current coding is confusing. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
Il 23/01/18 18:13, Petr Jelinek ha scritto: > Hi, > > On 23/01/18 15:38, Marco Nenciarini wrote: >> Il 22/01/18 23:18, Petr Jelinek ha scritto: >>> On 22/01/18 19:45, Petr Jelinek wrote: >>> >>> Actually on second look, I don't like the new boolean parameter much. >>> I'd rather we didn't touch the input list and always close only >>> relations opened inside the ExecuteTruncateGuts(). >>> >>> It may mean more list(s) but the current interface is still not clean. >>> >> >> Now ExecuteTruncateGuts unconditionally closes the relations that it >> opens. The caller has now always the responsibility to close the >> relations passed with the explicit_rels list. > > This looks good. > >> >> Version 15 attached. >> > > I see you still do CASCADE on the subscriber though. > No it doesn't. The following code in worker.c prevents that. + /* +* Even if we used CASCADE on the upstream master we explicitly +* default to replaying changes without further cascading. +* This might be later changeable with a user specified option. +*/ + cascade = false; There is also a test that check it works as intended: + # should not cascade on replica + $node_publisher->safe_psql('postgres', "TRUNCATE tab_rep CASCADE"); + + $node_publisher->wait_for_catchup($appname); + + $result = $node_subscriber->safe_psql('postgres', + "SELECT count(*), min(a), max(a) FROM tab_notrep_fk"); + is($result, qq(1|-1|-1), + 'check replicated truncate does not cascade on replica'); + + $result = $node_subscriber->safe_psql('postgres', + "SELECT nextval('seq_notrep')"); + is($result, qq(102), + 'check replicated truncate does not restart identities'); + 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] Logical decoding of TRUNCATE
Hi, On 23/01/18 15:38, Marco Nenciarini wrote: > Il 22/01/18 23:18, Petr Jelinek ha scritto: >> On 22/01/18 19:45, Petr Jelinek wrote: >> >> Actually on second look, I don't like the new boolean parameter much. >> I'd rather we didn't touch the input list and always close only >> relations opened inside the ExecuteTruncateGuts(). >> >> It may mean more list(s) but the current interface is still not clean. >> > > Now ExecuteTruncateGuts unconditionally closes the relations that it > opens. The caller has now always the responsibility to close the > relations passed with the explicit_rels list. This looks good. > > Version 15 attached. > I see you still do CASCADE on the subscriber though. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
Il 22/01/18 23:18, Petr Jelinek ha scritto: > On 22/01/18 19:45, Petr Jelinek wrote: > > Actually on second look, I don't like the new boolean parameter much. > I'd rather we didn't touch the input list and always close only > relations opened inside the ExecuteTruncateGuts(). > > It may mean more list(s) but the current interface is still not clean. > Now ExecuteTruncateGuts unconditionally closes the relations that it opens. The caller has now always the responsibility to close the relations passed with the explicit_rels list. Version 15 attached. 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; diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out index 1e22c1eefc..d1feea4909 100644 *** a/contrib/test_decoding/expected/ddl.out --- b/contrib/test_decoding/expected/ddl.out *** *** 543,548 UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2; --- 543,550 UPDATE table_with_unique_not_null SET id = -id; UPDATE table_with_unique_not_null SET id = -id; DELETE FROM table_with_unique_not_null WHERE data = 3; + TRUNCATE table_with_unique_not_null; + TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART IDENTITY CASCADE; -- check toast support BEGIN; CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable "random" *** *** 660,665 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc --- 662,673 table public.table_with_unique_not_null: DELETE: id[integer]:4 COMMIT BEGIN + table public.table_with_unique_not_null: TRUNCATE: (no-flags) + COMMIT + BEGIN + table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade + COMMIT + BEGIN table public.toasttable: INSERT: id[int
Re: [PATCH] Logical decoding of TRUNCATE
On 22/01/18 19:45, Petr Jelinek wrote: > On 19/01/18 12:37, Marco Nenciarini wrote: >> Hi All, > + /* logicalrep_rel_close call not needed, because ExecuteTruncateGuts > + * already closes the relations. Setting localrel to NULL in the > map entry > + * is still needed. > + */ > + rel->localrel = NULL; This is somewhat ugly. Perhaps the ExecuteTruncateGuts should track which relations it opened and only close those and the rest should be closed by caller? That should also remove the other ugly part which is that the ExecuteTruncateGuts modifies the input list. What if caller wanted to use those relations it sent as parameter later? >>> >>> Agreed >>> >> >> Attached a new version of the patch addressing these issues. >> > > Besides the small thing I wrote for the 0001 in the other thread I am > pretty much happy with this now. > Actually on second look, I don't like the new boolean parameter much. I'd rather we didn't touch the input list and always close only relations opened inside the ExecuteTruncateGuts(). It may mean more list(s) but the current interface is still not clean. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
On 19/01/18 12:37, Marco Nenciarini wrote: > Hi All, > > Il 18/01/18 17:48, Simon Riggs ha scritto: >> On 17 January 2018 at 17:07, Petr Jelinek >> wrote: >> >>> Things I am less convinced about: >>> >>> The patch will cascade truncation on downstream if cascade was specified >>> on the upstream, that can potentially be dangerous and we either should >>> not do it and only truncate the tables which were truncated upstream >>> (but without restricting because of FKs), leaving the data inconsistent >>> on downstream (like we do already with DELETE or UPDATE). Or maybe make >>> it into either subscription or publication option so that user can chose >>> the behaviour here as I am sure some people will want it to cascade (but >>> the default should still IMHO be to not cascade as that's safer). >> >> I agree the default should be to NOT cascade. >> >> If someone wants cascading as a publication option, that can be added later. >> > > I agree that not replicating the CASCADE option is the best option > according to POLA principle. > + /* logicalrep_rel_close call not needed, because ExecuteTruncateGuts + * already closes the relations. Setting localrel to NULL in the map entry + * is still needed. + */ + rel->localrel = NULL; >>> >>> This is somewhat ugly. Perhaps the ExecuteTruncateGuts should track >>> which relations it opened and only close those and the rest should be >>> closed by caller? That should also remove the other ugly part which is >>> that the ExecuteTruncateGuts modifies the input list. What if caller >>> wanted to use those relations it sent as parameter later? >> >> Agreed >> > > Attached a new version of the patch addressing these issues. > Besides the small thing I wrote for the 0001 in the other thread I am pretty much happy with this now. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
Hi All, Il 18/01/18 17:48, Simon Riggs ha scritto: > On 17 January 2018 at 17:07, Petr Jelinek > wrote: > >> Things I am less convinced about: >> >> The patch will cascade truncation on downstream if cascade was specified >> on the upstream, that can potentially be dangerous and we either should >> not do it and only truncate the tables which were truncated upstream >> (but without restricting because of FKs), leaving the data inconsistent >> on downstream (like we do already with DELETE or UPDATE). Or maybe make >> it into either subscription or publication option so that user can chose >> the behaviour here as I am sure some people will want it to cascade (but >> the default should still IMHO be to not cascade as that's safer). > > I agree the default should be to NOT cascade. > > If someone wants cascading as a publication option, that can be added later. > I agree that not replicating the CASCADE option is the best option according to POLA principle. >>> + /* logicalrep_rel_close call not needed, because ExecuteTruncateGuts >>> + * already closes the relations. Setting localrel to NULL in the map >>> entry >>> + * is still needed. >>> + */ >>> + rel->localrel = NULL; >> >> This is somewhat ugly. Perhaps the ExecuteTruncateGuts should track >> which relations it opened and only close those and the rest should be >> closed by caller? That should also remove the other ugly part which is >> that the ExecuteTruncateGuts modifies the input list. What if caller >> wanted to use those relations it sent as parameter later? > > Agreed > Attached a new version of the patch addressing these issues. 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 diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out index 1e22c1eefc..d1feea4909 100644 *** a/contrib/test_decoding/expected/ddl.out --- b/contrib/test_decoding/expected/ddl.out *** *** 543,548 UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2; --- 543,550 UPDATE table_with_unique_not_null SET id = -id; UPDATE table_with_unique_not_null SET id = -id; DELETE FROM table_with_unique_not_null WHERE data = 3; + TRUNCATE table_with_unique_not_null; + TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART IDENTITY CASCADE; -- check toast support BEGIN; CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable "random" *** *** 660,665 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc --- 662,673 table public.table_with_unique_not_null: DELETE: id[integer]:4 COMMIT BEGIN + table public.table_with_unique_not_null: TRUNCATE: (no-flags) + COMMIT + BEGIN + table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade + COMMIT + BEGIN table public.toasttable: INSERT: id[integer]:1 toasted_col1[text]:'1234567891011121314151617181920212223242526272829303132333435363738394041424344454647484950515253545556575859606162636465666768697071727374757677787980818283848586878889909192939495969798991001011021031041051061071081091101211311411511611711811912012112212312412512612712812913013113213313413513613713813914014114214314
Re: [PATCH] Logical decoding of TRUNCATE
On 17 January 2018 at 17:07, Petr Jelinek wrote: > Things I am less convinced about: > > The patch will cascade truncation on downstream if cascade was specified > on the upstream, that can potentially be dangerous and we either should > not do it and only truncate the tables which were truncated upstream > (but without restricting because of FKs), leaving the data inconsistent > on downstream (like we do already with DELETE or UPDATE). Or maybe make > it into either subscription or publication option so that user can chose > the behaviour here as I am sure some people will want it to cascade (but > the default should still IMHO be to not cascade as that's safer). I agree the default should be to NOT cascade. If someone wants cascading as a publication option, that can be added later. >> + /* logicalrep_rel_close call not needed, because ExecuteTruncateGuts >> + * already closes the relations. Setting localrel to NULL in the map >> entry >> + * is still needed. >> + */ >> + rel->localrel = NULL; > > This is somewhat ugly. Perhaps the ExecuteTruncateGuts should track > which relations it opened and only close those and the rest should be > closed by caller? That should also remove the other ugly part which is > that the ExecuteTruncateGuts modifies the input list. What if caller > wanted to use those relations it sent as parameter later? Agreed -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
Hi, I reviewed 0001 in its own thread. So I think that we generally want this patch and I think the design decisions are right. Namely: TRUNCATE being treated as DELETE in terms of DML filtering makes sense to me as it is basically bulk delete, needs to be mentioned in release notes though. Adding special message to protocol is appropriate as truncate is more DML than DDL in sense of manipulating data so it should be replicated separately from other DDL Processing relations that were truncated when CASCADE is used separately is needed because we allow relations to be filtered by logical replication I see the patch adds new xlog record which is perhaps not ideal but the current one seems utterly unsuitable for decoding so I guess it's okay, especially when it's only added for wal_level = logical which it is. Also TRUNCATE is not exactly high tps operation. Things I am less convinced about: The patch will cascade truncation on downstream if cascade was specified on the upstream, that can potentially be dangerous and we either should not do it and only truncate the tables which were truncated upstream (but without restricting because of FKs), leaving the data inconsistent on downstream (like we do already with DELETE or UPDATE). Or maybe make it into either subscription or publication option so that user can chose the behaviour here as I am sure some people will want it to cascade (but the default should still IMHO be to not cascade as that's safer). > + /* logicalrep_rel_close call not needed, because ExecuteTruncateGuts > + * already closes the relations. Setting localrel to NULL in the map > entry > + * is still needed. > + */ > + rel->localrel = NULL; This is somewhat ugly. Perhaps the ExecuteTruncateGuts should track which relations it opened and only close those and the rest should be closed by caller? That should also remove the other ugly part which is that the ExecuteTruncateGuts modifies the input list. What if caller wanted to use those relations it sent as parameter later? -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
Hi all, After commit bbd3363e128daec0e70952c1bb2f12ab1f6f1292 that refactor subscription tests to use PostgresNode's wait_for_catchup, the patch needs to be updated to use wait_for_catchup. Attached there is the updated patch. is discussed in a separate thread https://commitfest.postgresql.org/16/1447/, but I've posted it also here to ease the review of the 0002. Regards, Marco -- 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 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 diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out index 1e22c1eefc..d1feea4909 100644 *** a/contrib/test_decoding/expected/ddl.out --- b/contrib/test_decoding/expected/ddl.out *** *** 543,548 UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2; --- 543,550 UPDATE table_with_unique_not_null SET id = -id; UPDATE table_with_unique_not_null SET id = -id; DELETE FROM table_with_unique_not_null WHERE data = 3; + TRUNCATE table_with_unique_not_null; + TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART IDENTITY CASCADE; -- check toast support BEGIN; CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable "random" *** *** 660,665 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc --- 662,673 table public.table_with_unique_not_null: DELETE: id[integer]:4 COMMIT BEGIN + table public.table_with_unique_not_null: TRUNCATE: (no-flags) + COMMIT + BEGIN + table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade + COMMIT + BEGIN table public.toasttable: INSERT: id[integer]:1 toasted_col1[text]:'12345678910111213141516171819202122232425262728293031323334353637383940414243444546474849505152535455565758596061626364656667686970717273747576777879808182838485868788899091929394959697989910010110210310410510610710810911012113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204205206207208209210211212213214215216217218219220221232242252262
Re: [PATCH] Logical decoding of TRUNCATE
Attached here there is the complete list of patches required to pass all the tests. The 0001 patch is discussed in a separate thread, but I've posted it also here to ease the review of the 0002. Regards, Marco -- 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 diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out index 1e22c1eefc..d1feea4909 100644 *** a/contrib/test_decoding/expected/ddl.out --- b/contrib/test_decoding/expected/ddl.out *** *** 543,548 UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2; --- 543,550 UPDATE table_with_unique_not_null SET id = -id; UPDATE table_with_unique_not_null SET id = -id; DELETE FROM table_with_unique_not_null WHERE data = 3; + TRUNCATE table_with_unique_not_null; + TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART IDENTITY CASCADE; -- check toast support BEGIN; CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable "random" *** *** 660,665 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc --- 662,673 table public.table_with_unique_not_null: DELETE: id[integer]:4 COMMIT BEGIN + table public.table_with_unique_not_null: TRUNCATE: (no-flags) + COMMIT + BEGIN + table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade + COMMIT + BEGIN table public.toasttable: INSERT: id[integer]:1 toasted_col1[text]:'123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110121131141151161171181191201211221231241251261271281291301311321331341351361371381391401411421431441451461471481491501511521531541551561571581591601611621631641651661671681691701711721731741751761771781791801811821831841851861871881891901911921931941951961971981992002012022032042052062072082092102112122132142152162172182192202212322422522622722822923023123223323423523623723823924024124224324424524624724824925025125225325425525625725825926026126226326426526626726826927027127227327427527627727827928028128228328
Re: [PATCH] Logical decoding of TRUNCATE
Patch rebased on the current master. Regards, Marco -- Marco Nenciarini - 2ndQuadrant Italy PostgreSQL Training, Services and Support marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out index 1e22c1eefc..d1feea4909 100644 *** a/contrib/test_decoding/expected/ddl.out --- b/contrib/test_decoding/expected/ddl.out *** *** 543,548 UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2; --- 543,550 UPDATE table_with_unique_not_null SET id = -id; UPDATE table_with_unique_not_null SET id = -id; DELETE FROM table_with_unique_not_null WHERE data = 3; + TRUNCATE table_with_unique_not_null; + TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART IDENTITY CASCADE; -- check toast support BEGIN; CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable "random" *** *** 660,665 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc --- 662,673 table public.table_with_unique_not_null: DELETE: id[integer]:4 COMMIT BEGIN + table public.table_with_unique_not_null: TRUNCATE: (no-flags) + COMMIT + BEGIN + table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade + COMMIT + BEGIN table public.toasttable: INSERT: id[integer]:1 toasted_col1[text]:'12345678910111213141516171819202122232425262728293031323334353637383940414243444546474849505152535455565758596061626364656667686970717273747576777879808182838485868788899091929394959697989910010110210310410510610710810911012113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204205206207208209210211212213214215216217218219220221232242252262272282292302312322332342352362372382392402412422432442452462472482492502512522532542552562572582592602612622632642652662672682692702712722732742752762772782792802812822832842852862872882892902912922932942952962972982993003013023033043053063073083093103113123133143153163173183193203213223233243253263273283293303313323433533633733833934034134234334434534634734834935035135235335435535635735835936036136236336436536636736836937037137237337437537637737837938038138238338438538638738838939039139239339439539639739839940040140240340440540640740840941041141241341441541641741841942042142242342442542642742842943043143243343443543643743843944044144244345446447448449450451452453454455456457458459460461462463464465466467468469470471472473474475476477478479480481482483484485486487488489490491492493494495496497498499500501502503504505506507508509510511512513514515516517518519520521522523524525526527528529530531532533534535536537538539540541542543544545546547548549550551552553554565575585595605615625635645655665675685695705715725735745755765775785795805815825835845855865875885895905915925935945955965975985996006016026036046056066076086096106116126136146156166176186196206216226236246256266276286296306316326336346356366376386396406416426436446456466476486496506516526536546556566576586596606616626636646656766866967067167267367467567667767867968068168268368468568668768868969069169269369469569669769869970070170270370470570670770870971071171271371471571671771871972072172272372472572672772872973073173273373473573673773873974074174274374474574674774874975075175275375475575675775875976076176276376476576676776876977077177277377477577678779780781782783784785786787788789790791792793794795796797798799800801802803804805806807808809810811812813814815816817818819820821822823824825826827828829830831832833834835836837838839840841842843844845846847848849850851852853854855856857858859860861862863864865866867868869870871872873874875876877878879880881882883884885886887898908918928938948958968978988999009019029039049059069079089099109119129139149159169179189199209219229239249259269279289299309319329339349359369379389399409419429439449459469479489499509519529539549559569579589599609619629639649659669679689699709719729739749759769779789799809819829839849859869879889899909919929939949959969979989991000100110021003100410051006100710081009101010111012101310141015101610171018101910201021102210231024102510261027102810291030103110321033103410351036103710381039104010411042104310441045104610471048104910501051105210531054105510561057105810591060106110621063106410651066106710681069107010711072107310741075107610771078107910801081108210831084108510861087108810891090109110921093109410951096109710981099110011011102110311041105110611071108110911101112111311141115111611171118111911201121112211231124112511261127112811291130113111321133113411351136113711381139114011411142114311441145114611471148114911501151115211531154115511561157115811591160116111621163116411651166116711681169117011711172117311741175117611771
Re: [PATCH] Logical decoding of TRUNCATE
Hi, I've found some SGML errors in the version v10 of the patch. I've fixed it in version v11 that is attached. Regards, Marco -- Marco Nenciarini - 2ndQuadrant Italy PostgreSQL Training, Services and Support marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out index 1e22c1eefc..d1feea4909 100644 *** a/contrib/test_decoding/expected/ddl.out --- b/contrib/test_decoding/expected/ddl.out *** *** 543,548 UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2; --- 543,550 UPDATE table_with_unique_not_null SET id = -id; UPDATE table_with_unique_not_null SET id = -id; DELETE FROM table_with_unique_not_null WHERE data = 3; + TRUNCATE table_with_unique_not_null; + TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART IDENTITY CASCADE; -- check toast support BEGIN; CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable "random" *** *** 660,665 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc --- 662,673 table public.table_with_unique_not_null: DELETE: id[integer]:4 COMMIT BEGIN + table public.table_with_unique_not_null: TRUNCATE: (no-flags) + COMMIT + BEGIN + table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade + COMMIT + BEGIN table public.toasttable: INSERT: id[integer]:1 toasted_col1[text]:'1234567891011121314151617181920212223242526272829303132333435363738394041424344454647484950515253545556575859606162636465666768697071727374757677787980818283848586878889909192939495969798991001011021031041051061071081091101211311411511611711811912012112212312412512612712812913013113213313413513613713813914014114214314414514614714814915015115215315415515615715815916016116216316416516616716816917017117217317417517617717817918018118218318418518618718818919019119219319419519619719819920020120220320420520620720820921021121221321421521621721821922022123224225226227228229230231232233234235236237238239240241242243244245246247248249250251252253254255256257258259260261262263264265266267268269270271272273274275276277278279280281282283284285286287288289290291292293294295296297298299300301302303304305306307308309310311312313314315316317318319320321322323324325326327328329330331332343353363373383393403413423433443453463473483493503513523533543553563573583593603613623633643653663673683693703713723733743753763773783793803813823833843853863873883893903913923933943953963973983994004014024034044054064074084094104114124134144154164174184194204214224234244254264274284294304314324334344354364374384394404414424434544644744844945045145245345445545645745845946046146246346446546646746846947047147247347447547647747847948048148248348448548648748848949049149249349449549649749849950050150250350450550650750850951051151251351451551651751851952052152252352452552652752852953053153253353453553653753853954054154254354454554654754854955055155255355456557558559560561562563564565566567568569570571572573574575576577578579580581582583584585586587588589590591592593594595596597598599600601602603604605606607608609610611612613614615616617618619620621622623624625626627628629630631632633634635636637638639640641642643644645646647648649650651652653654655656657658659660661662663664665676686696706716726736746756766776786796806816826836846856866876886896906916926936946956966976986997007017027037047057067077087097107117127137147157167177187197207217227237247257267277287297307317327337347357367377387397407417427437447457467477487497507517527537547557567577587597607617627637647657667677687697707717727737747757767877978078178278378478578678778878979079179279379479579679779879980080180280380480580680780880981081181281381481581681781881982082182282382482582682782882983083183283383483583683783883984084184284384484584684784884985085185285385485585685785885986086186286386486586686786886987087187287387487587687787887988088188288388488588688789890891892893894895896897898899900901902903904905906907908909910911912913914915916917918919920921922923924925926927928929930931932933934935936937938939940941942943944945946947948949950951952953954955956957958959960961962963964965966967968969970971972973974975976977978979980981982983984985986987988989990991992993994995996997998999100010011002100310041005100610071008100910101011101210131014101510161017101810191020102110221023102410251026102710281029103010311032103310341035103610371038103910401041104210431044104510461047104810491050105110521053105410551056105710581059106010611062106310641065106610671068106910701071107210731074107510761077107810791080108110821083108410851086108710881089109010911092109310941095109610971098109911001101110211031104110511061107110811091110111211131114111511161117111811191120112111221123112411251126112711281129113011311132113311341135113611371138113911401141114211431144114511461147114811491150115111521153115411551156115711581
Re: [PATCH] Logical decoding of TRUNCATE
On 29 December 2017 at 19:55, Andres Freund wrote: > Hi, > > On 2017-12-29 14:15:22 +0100, Marco Nenciarini wrote: >> This patch implements support for TRUNCATE statements >> in logical replication. The work has mainly done by Simon Riggs then >> finished by me. Tests are written by me. >> >> TRUNCATE is treated as a form of DELETE for the purpose of deciding >> whether to publish, or not. > > It'd be good if you explained exactly what the chosen behaviour is, and > why that's the right behaviour in comparison to other alternatives. At present the patch treats TRUNCATE as if it were a DELETE which means that CREATE PUBLICATION insert_only FOR TABLE mydata WITH (publish = 'insert'); will not publish truncates before or after this patch CREATE PUBLICATION insert_only FOR TABLE mydata; will now publish TRUNCATEs, although they were ignored in PG10 so PG10 publications will act differently I had regarded it as a missing piece, but some may view that is a behaviour change in PG11 Alternatively, we could also use WITH (publish = 'truncate') as a separate decision. That is an easy change if we wish it. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Logical decoding of TRUNCATE
Hi, Il 29/12/17 20:55, Andres Freund ha scritto: > Hi, > > On 2017-12-29 14:15:22 +0100, Marco Nenciarini wrote: >> This patch implements support for TRUNCATE statements >> in logical replication. The work has mainly done by Simon Riggs then >> finished by me. Tests are written by me. >> >> TRUNCATE is treated as a form of DELETE for the purpose of deciding >> whether to publish, or not. > > It'd be good if you explained exactly what the chosen behaviour is, and > why that's the right behaviour in comparison to other alternatives. > here is the description of how the patch works: * A new WAL record type has been added (XLOG_HEAP_TRUNCATE) to allow a precise logical decoding of the TRUNCATE operation. It contains the TRUNCATE flags and the list of the involved tables and sequences. It is treated as a NO-OP during the WAL reply as all the physical operations are logged in SMGR WAL records. * A new TRUNCATE message has been added to the logical protocol. It carries the information about the truncation of one single table specifying if CASCADE or RESTART IDENTITY has been specified. * The ExecuteTruncateGuts function has been extracted from ExecuteTruncate. This function is used by the logical apply process to replicate the TRUNCATE operations, honouring the eventual CASCADE and RESTART IDENTITY flag. 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] Logical decoding of TRUNCATE
Hi, On 2017-12-29 14:15:22 +0100, Marco Nenciarini wrote: > This patch implements support for TRUNCATE statements > in logical replication. The work has mainly done by Simon Riggs then > finished by me. Tests are written by me. > > TRUNCATE is treated as a form of DELETE for the purpose of deciding > whether to publish, or not. It'd be good if you explained exactly what the chosen behaviour is, and why that's the right behaviour in comparison to other alternatives. - Andres
[PATCH] Logical decoding of TRUNCATE
Hi, This patch implements support for TRUNCATE statements in logical replication. The work has mainly done by Simon Riggs then finished by me. Tests are written by me. TRUNCATE is treated as a form of DELETE for the purpose of deciding whether to publish, or not. The "TRUNCATE behavior when session_replication_role = replica"[1] patch is required from this patch to work correctly with tables referenced by foreign keys. [1] https://commitfest.postgresql.org/16/1447/ Regards, Marco -- Marco Nenciarini - 2ndQuadrant Italy PostgreSQL Training, Services and Support marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out index 1e22c1eefc..d1feea4909 100644 *** a/contrib/test_decoding/expected/ddl.out --- b/contrib/test_decoding/expected/ddl.out *** *** 543,548 UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2; --- 543,550 UPDATE table_with_unique_not_null SET id = -id; UPDATE table_with_unique_not_null SET id = -id; DELETE FROM table_with_unique_not_null WHERE data = 3; + TRUNCATE table_with_unique_not_null; + TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART IDENTITY CASCADE; -- check toast support BEGIN; CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable "random" *** *** 660,665 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc --- 662,673 table public.table_with_unique_not_null: DELETE: id[integer]:4 COMMIT BEGIN + table public.table_with_unique_not_null: TRUNCATE: (no-flags) + COMMIT + BEGIN + table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade + COMMIT + BEGIN table public.toasttable: INSERT: id[integer]:1 toasted_col1[text]:'1234567891011121314151617181920212223242526272829303132333435363738394041424344454647484950515253545556575859606162636465666768697071727374757677787980818283848586878889909192939495969798991001011021031041051061071081091101211311411511611711811912012112212312412512612712812913013113213313413513613713813914014114214314414514614714814915015115215315415515615715815916016116216316416516616716816917017117217317417517617717817918018118218318418518618718818919019119219319419519619719819920020120220320420520620720820921021121221321421521621721821922022123224225226227228229230231232233234235236237238239240241242243244245246247248249250251252253254255256257258259260261262263264265266267268269270271272273274275276277278279280281282283284285286287288289290291292293294295296297298299300301302303304305306307308309310311312313314315316317318319320321322323324325326327328329330331332343353363373383393403413423433443453463473483493503513523533543553563573583593603613623633643653663673683693703713723733743753763773783793803813823833843853863873883893903913923933943953963973983994004014024034044054064074084094104114124134144154164174184194204214224234244254264274284294304314324334344354364374384394404414424434544644744844945045145245345445545645745845946046146246346446546646746846947047147247347447547647747847948048148248348448548648748848949049149249349449549649749849950050150250350450550650750850951051151251351451551651751851952052152252352452552652752852953053153253353453553653753853954054154254354454554654754854955055155255355456557558559560561562563564565566567568569570571572573574575576577578579580581582583584585586587588589590591592593594595596597598599600601602603604605606607608609610611612613614615616617618619620621622623624625626627628629630631632633634635636637638639640641642643644645646647648649650651652653654655656657658659660661662663664665676686696706716726736746756766776786796806816826836846856866876886896906916926936946956966976986997007017027037047057067077087097107117127137147157167177187197207217227237247257267277287297307317327337347357367377387397407417427437447457467477487497507517527537547557567577587597607617627637647657667677687697707717727737747757767877978078178278378478578678778878979079179279379479579679779879980080180280380480580680780880981081181281381481581681781881982082182282382482582682782882983083183283383483583683783883984084184284384484584684784884985085185285385485585685785885986086186286386486586686786886987087187287387487587687787887988088188288388488588688789890891892893894895896897898899900901902903904905906907908909910911912913914915916917918919920921922923924925926927928929930931932933934935936937938939940941942943944945946947948949950951952953954955956957958959960961962963964965966967968969970971972973974975976977978979980981982983984985986987988989990991992993994995996997998999100010011002100310041005100610071008100910101011101210131014101510161017101810191020102110221023102410251026102710281029103010311032103310341035103610371038103910401041104210431044104510461047104810491050105110521053105410551056105710581059106010611062106310641065106610671068