Re: Fix a typo in walsender.c

[atorikoshi]

On 2018/03/29 7:23, Bruce Momjian wrote:


Thanks, patch applied.


Thank you for committing!

Fix a typo in walsender.c

[atorikoshi]

Hi,

Attached a minor patch for a typo: s/log/lag

Regards,

--
Atsushi Torikoshi
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
diff --git a/src/backend/replication/walsender.c 
b/src/backend/replication/walsender.c
index d46374d..8bb1919 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1245,7 +1245,7 @@ WalSndWriteData(LogicalDecodingContext *ctx, XLogRecPtr 
lsn, TransactionId xid,
 /*
  * LogicalDecodingContext 'update_progress' callback.
  *
- * Write the current position to the log tracker (see XLogSendPhysical).
+ * Write the current position to the lag tracker (see XLogSendPhysical).
  */
 static void
 WalSndUpdateProgress(LogicalDecodingContext *ctx, XLogRecPtr lsn, 
TransactionId xid)

Re: Fix a typo in walsender.c

[atorikoshi]

On 2018/02/09 4:40, Robert Haas wrote:

On Thu, Feb 8, 2018 at 1:32 AM, atorikoshi
<torikoshi_atsushi...@lab.ntt.co.jp> wrote:

Attached a minor patch for variable name in comment:


Committed.



Thank you!

Fix a typo in walsender.c

[atorikoshi]

Hi,

Attached a minor patch for variable name in comment:  
s/progress_update/update_progress


  ---include/server/replication/logical.h
  ...
  35 typedef struct LogicalDecodingContext
  36 {
  ...
  68 LogicalOutputPluginWriterUpdateProgress update_progress;

Regards,

--
Atsushi Torikoshi
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
diff --git a/src/backend/replication/walsender.c 
b/src/backend/replication/walsender.c
index 130ecd5..d46374d 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1243,7 +1243,7 @@ WalSndWriteData(LogicalDecodingContext *ctx, XLogRecPtr 
lsn, TransactionId xid,
 }
 
 /*
- * LogicalDecodingContext 'progress_update' callback.
+ * LogicalDecodingContext 'update_progress' callback.
  *
  * Write the current position to the log tracker (see XLogSendPhysical).
  */

Re: Failed to delete old ReorderBuffer spilled files

[atorikoshi]

On 2018/01/06 0:30, Alvaro Herrera wrote:

Ahh, so the reason I didn't see these crashes is that Atsushi had
already fixed them.  Nice.  It would be *very* good to trim the quoted
material when you reply --- don't leave everything, just enough lines
for context.  I would have seen this comment if it didn't require me to
scroll pages and pages and pages of useless material.


Sorry for the confusion.



I have pushed this patch from 9.4 to master.  I reformulated the
comments.


Thanks for modifications and committing.

Regards,

--
Atsushi Torikoshi
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Failed to delete old ReorderBuffer spilled files

[atorikoshi]

On 2017/11/24 10:57, Craig Ringer wrote:
> On 24 November 2017 at 09:20, atorikoshi 
<torikoshi_atsushi...@lab.ntt.co.jp

>> wrote:
>
>>
>> Thanks for letting me know.
>> I think I only tested running "make check" at top directory, sorry
>> for my insufficient test.
>>
>> The test failure happened at the beginning of replication(creating
>> slot), so there are no changes yet and getting the tail of changes
>> failed.
>>
>> Because the bug we are fixing only happens after creating files,
>> I've added "txn->serialized" to the if statement condition.
>
>
> Thanks.
>
> Re-reading the patch I see
>
>  * The final_lsn of which transaction that hasn't produced an abort
>  * record is invalid.
>
> which I find very hard to parse. I suggest:
>
>  We set final_lsn when we decode the commit or abort record for a
> transaction,
>  but transactions implicitly aborted by a crash never write such a 
record.

>
> then continue from there with the same text as in the patch.
>
> Otherwise I'm happy. It passes make check, test decoding and the recovery
> TAP tests too.
>

Thanks for your kind suggestion and running test.
I've added your comment at the beginning.

--
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..ee28d26 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1724,6 +1724,22 @@ ReorderBufferAbortOld(ReorderBuffer *rb, TransactionId 
oldestRunningXid)
 
if (TransactionIdPrecedes(txn->xid, oldestRunningXid))
{
+   /*
+* We set final_lsn when we decode the commit or abort 
record for
+* a transaction, but transactions implicitly aborted 
by a crash
+* never write such a record.
+* The final_lsn of which transaction that hasn't 
produced an abort
+* record is invalid. To ensure cleanup the serialized 
changes of
+* such transaction we set the LSN of the last change 
action to
+* final_lsn.
+*/
+   if (txn->serialized && txn->final_lsn == 0)
+   {
+   ReorderBufferChange *last_change =
+   dlist_tail_element(ReorderBufferChange, node, 
>changes);
+   txn->final_lsn = last_change->lsn;
+   }
+
elog(DEBUG2, "aborting old transaction %u", txn->xid);
 
/* remove potential on-disk data, and deallocate this 
tx */
diff --git a/src/include/replication/reorderbuffer.h 
b/src/include/replication/reorderbuffer.h
index 86effe1..7931757 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
+* Note that this can also be a LSN of the last change action of this 
xact
+* if it is an implicitly aborted transaction.
 * 
 */
XLogRecPtr  final_lsn;

Re: Failed to delete old ReorderBuffer spilled files

[atorikoshi]

On 2017/11/22 16:49, Masahiko Sawada wrote:

On Wed, Nov 22, 2017 at 2:56 PM, Craig Ringer  wrote:

On 22 November 2017 at 12:15, Kyotaro HORIGUCHI
 wrote:


At Wed, 22 Nov 2017 12:57:34 +0900, Michael Paquier
 wrote in

Failed to delete old ReorderBuffer spilled files

[atorikoshi]

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,
  'aa'
FROM
generate_series(1, 100) as i;

4. Then we can see spilled files.

  @pub $ ls -1 ${PGDATA}/pg_replslot/sub/
state
xid-561-lsn-0-100.snap
xid-561-lsn-0-200.snap
xid-561-lsn-0-300.snap
xid-561-lsn-0-400.snap
xid-561-lsn-0-500.snap
xid-561-lsn-0-600.snap
xid-561-lsn-0-700.snap
xid-561-lsn-0-800.snap
xid-561-lsn-0-900.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.

--
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..51d5b71 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1093,6 +1093,20 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, 
ReorderBufferTXN *txn)
ReorderBufferCleanupTXN(rb, subtxn);
}
 
+   /*
+* 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 we set final_lsn to the lsn of the last ReorderBufferChange.
+*/
+   if (txn->final_lsn == 0)
+   {
+   ReorderBufferChange *last_change =
+   dlist_tail_element(ReorderBufferChange, node, >changes);
+   txn->final_lsn = last_change->lsn;
+   }
+
/* cleanup changes in the toplevel txn */
dlist_foreach_modify(iter, >changes)
{