Thanks for reviewing!
On 2017/11/21 18:12, Masahiko Sawada wrote:
On Tue, Nov 21, 2017 at 3:48 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote:
On Mon, Nov 20, 2017 at 7:35 PM, atorikoshi
<torikoshi_atsushi...@lab.ntt.co.jp> wrote:
Hi,
I put many queries into one transaction and made ReorderBuffer spill
data to disk, and sent SIGKILL to postgres before the end of the
transaction.
After starting up postgres again, I observed the files spilled to
data wasn't deleted.
I think these files should be deleted because its transaction was no
more valid, so no one can use these files.
Below is a reproduction instructions.
------------------------------------------------
1. Create table and publication at publiser.
@pub =# CREATE TABLE t1(
id INT PRIMARY KEY,
name TEXT);
@pub =# CREATE PUBLICATION pub FOR TABLE t1;
2. Create table and subscription at subscriber.
@sub =# CREATE TABLE t1(
id INT PRIMARY KEY,
name TEXT
);
@sub =# CREATE SUBSCRIPTION sub
CONNECTION 'host=[hostname] port=[port] dbname=[dbname]'
PUBLICATION pub;
3. Put many queries into one transaction.
@pub =# BEGIN;
INSERT INTO t1
SELECT
i,
'aaaaaaaaaa'
FROM
generate_series(1, 1000000) as i;
4. Then we can see spilled files.
@pub $ ls -1 ${PGDATA}/pg_replslot/sub/
state
xid-561-lsn-0-1000000.snap
xid-561-lsn-0-2000000.snap
xid-561-lsn-0-3000000.snap
xid-561-lsn-0-4000000.snap
xid-561-lsn-0-5000000.snap
xid-561-lsn-0-6000000.snap
xid-561-lsn-0-7000000.snap
xid-561-lsn-0-8000000.snap
xid-561-lsn-0-9000000.snap
5. Kill publisher's postgres process before COMMIT.
@pub $ kill -s SIGKILL [pid of postgres]
6. Start publisher's postgres process.
@pub $ pg_ctl start -D ${PGDATA}
7. After a while, we can see the files remaining.
(Immediately after starting publiser, we can not see these files.)
@pub $ pg_ctl start -D ${PGDATA}
When I configured with '--enable-cassert', below assertion error
was appeared.
TRAP: FailedAssertion("!(txn->final_lsn != 0)", File: "reorderbuffer.c",
Line: 2576)
------------------------------------------------
Attached patch sets final_lsn to the last ReorderBufferChange if
final_lsn == 0.
Thank you for the report. I could reproduce this issue with the above
step. My analysis is, the cause of that a serialized reorder buffer
isn't cleaned up is that the aborted transaction without an abort WAL
record has no chance to set ReorderBufferTXN->final_lsn. So if there
is such serialized transactions ReorderBufferRestoreCleanup cleanups
no files, which is cause of the assertion failure (or a file being
orphaned). What do you think?
On detail of your patch, I'm not sure it's safe if we set the lsn of
other than commit record or abort record to final_lsn. The comment in
reorderbuffer.h says,
typedef trcut ReorderBufferTXN
{
(snip)
/* ----
* LSN of the record that lead to this xact to be committed or
* aborted. This can be a
* * plain commit record
* * plain commit record, of a parent transaction
* * prepared transaction commit
* * plain abort record
* * prepared transaction abort
* * error during decoding
* ----
*/
XLogRecPtr final_lsn;
But with your patch, we could set a lsn of a record that is other than
what listed above to final_lsn. One way I came up with is to make
I added some comments on final_lsn.
ReorderBufferRestoreCleanup accept an invalid value of final_lsn and
regards it as a aborted transaction that doesn't has a abort WAL
record. So we can cleanup all serialized files if final_lsn of a
transaction is invalid.
After more thought, since there are some codes cosmetically setting
final_lsn when the fate of transaction is determined possibly we
should not accept a invalid value of final_lsn even in the case.
My new patch keeps setting final_lsn, but changed its location to the
top of ReorderBufferCleanupTXN().
I think it's a kind of preparation, so doing it at the top improves
readability.
Anyway I think you should register this patch to the next commit fest so as not
forget.
Thanks for you advice, I've registered this issue as a bug.
Regards,
--
Atsushi Torikoshi
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
diff --git a/src/backend/replication/logical/reorderbuffer.c
b/src/backend/replication/logical/reorderbuffer.c
index 0f607ba..bbeb494 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1075,6 +1075,21 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn)
bool found;
dlist_mutable_iter iter;
+ /*
+ * Before cleaning up, do a preparation.
+ * If this transaction encountered crash during transaction,
+ * txn->final_lsn remains initial value.
+ * To properly remove entries which were spilled to disk, we need valid
+ * final_lsn.
+ * So set final_lsn to the lsn of last ReorderBufferChange.
+ */
+ if (txn->final_lsn == 0)
+ {
+ ReorderBufferChange *last_change =
+ dlist_tail_element(ReorderBufferChange, node, &txn->changes);
+ txn->final_lsn = last_change->lsn;
+ }
+
/* cleanup subtransactions & their changes */
dlist_foreach_modify(iter, &txn->subtxns)
{
diff --git a/src/include/replication/reorderbuffer.h
b/src/include/replication/reorderbuffer.h
index 86effe1..cd1acbd 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -168,6 +168,8 @@ typedef struct ReorderBufferTXN
* * plain abort record
* * prepared transaction abort
* * error during decoding
+ * Only when this xact encountered server crash, this value is set to
+ * the lsn of last ReorderBufferChange for cleaning up spilled files.
* ----
*/
XLogRecPtr final_lsn;
