From fe7337f2dc3177da19b647fcda3a33b73da82e14 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Thu, 5 Jun 2014 18:27:11 +0200 Subject: [PATCH] Fix off-by-one in decoding causing one-record events to be skipped. A ReorderBufferTransaction's end_lsn, the sentPtr advocated by walsender keepalive messages, and the end location remembered by the decoding get_*changes* SQL functions all use the location of the last read record + 1. I.e. the LSN points to the beginning of the next record. That cannot realistically be changed without changing the replication protocol because that's how keepalive messages have worked since 9.0. The bug is that the logic inside the snapshot builder, which decides whether a transaction's contents should be decoded, assumed the start location would point towards the last byte of the last record. The reason this didn't actually cause visible problems is that currently that decision is only made for commit records. Since interesting transactions always have at least one additional record - containing actual data - we'd never skip a transaction. But if there ever were transactions, or other events, with just one record containing important information, we'd skip them after stopping and restarting logical decoding. --- src/backend/replication/logical/snapbuild.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index f00fd7d422..196c188033 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -153,11 +153,11 @@ struct SnapBuild TransactionId xmax; /* - * Don't replay commits from an LSN <= this LSN. This can be set + * Don't replay commits from an LSN < this LSN. This can be set * externally but it will also be advanced (never retreat) from within * snapbuild.c. */ - XLogRecPtr transactions_after; + XLogRecPtr start_decoding_at; /* * Don't start decoding WAL until the "xl_running_xacts" information @@ -309,7 +309,7 @@ AllocateSnapshotBuilder(ReorderBuffer *reorder, builder->committed.includes_all_transactions = true; builder->initial_xmin_horizon = xmin_horizon; - builder->transactions_after = start_lsn; + builder->start_decoding_at = start_lsn; MemoryContextSwitchTo(oldcontext); @@ -375,7 +375,7 @@ SnapBuildCurrentState(SnapBuild *builder) bool SnapBuildXactNeedsSkip(SnapBuild *builder, XLogRecPtr ptr) { - return ptr <= builder->transactions_after; + return ptr < builder->start_decoding_at; } /* @@ -955,8 +955,8 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid, if (builder->state < SNAPBUILD_CONSISTENT) { /* ensure that only commits after this are getting replayed */ - if (builder->transactions_after < lsn) - builder->transactions_after = lsn; + if (builder->start_decoding_at <= lsn) + builder->start_decoding_at = lsn + 1; /* * We could avoid treating !SnapBuildTxnIsRunning transactions as @@ -1243,9 +1243,10 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn */ if (running->xcnt == 0) { - if (builder->transactions_after == InvalidXLogRecPtr || - builder->transactions_after < lsn) - builder->transactions_after = lsn; + if (builder->start_decoding_at == InvalidXLogRecPtr || + builder->start_decoding_at <= lsn) + /* can decode everything after this */ + builder->start_decoding_at = lsn + 1; builder->xmin = running->oldestRunningXid; builder->xmax = running->latestCompletedXid;