On 2019-Jul-26, Andres Freund wrote: > Petr, Simon, see the potential issue related to fast forward at the > bottom.
I think we neglected this bit. I looked at the patch Simon submitted downthread, and while I vaguely understand that we need to process NEW_CID records during fast-forwarding, I don't quite understand why we still can skip XLOG_INVALIDATION messages. I *think* we should process those too. Here's a patch that also contains that change; I also reworded Simon's proposed comment. I appreciate reviews. Thoughts? Relevant extracts from Andres' message below. > Thinking about it, it was not immediately clear to me that it is > necessary to process XLOG_HEAP2_NEW_CID at that stage. We only need the > cid mapping when decoding content of the transaction that the > XLOG_HEAP2_NEW_CID record was about - which will not happen if it > started before SNAPBUILD_FULL. > > Except that they also are responsible for signalling that a transaction > performed catalog modifications (cf ReorderBufferXidSetCatalogChanges() > call), which in turn is important for SnapBuildCommitTxn() to know > whether to include that transaction needs to be included in historic > snapshots. > > So unless I am missing something - which is entirely possible, I've had > this code thoroughly swapped out - that means that we only need to > process XLOG_HEAP2_NEW_CID < SNAPBUILD_FULL if there can be transactions > with relevant catalog changes, that don't have any invalidations > messages. > > After thinking about it for a bit, that's not guaranteed however. For > one, even for system catalog tables, looking at > CacheInvalidateHeapTuple() et al there can be catalog modifications that > create neither a snapshot invalidation message, nor a catcache > one. There's also the remote scenario that we possibly might be only > modifying a toast relation. > > But more importantly, the only modified table could be a user defined > catalog table (i.e. WITH (user_catalog_table = true)). Which in all > likelihood won't cause invalidation messages. So I think currently it is > required to process NEW_ID records - although we don't need to actually > execute the ReorderBufferAddNewTupleCids() etc calls. [...] > And related to the point of the theorizing above, I don't think skipping > XLOG_HEAP2_NEW_CID entirely when forwarding is correct. As a NEW_CID > record does not imply an invalidation message as discussed above, we'll > afaict compute wrong snapshots when such transactions are encountered > during forwarding. And we'll then log those snapshots to disk. Which > then means the slot cannot safely be used for actual decoding anymore - > as we'll use that snapshot when starting up decoding without fast > forwarding. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c index 5e1dc8a651..dcb6873f12 100644 --- a/src/backend/replication/logical/decode.c +++ b/src/backend/replication/logical/decode.c @@ -336,10 +336,9 @@ DecodeStandbyOp(LogicalDecodingContext *ctx, XLogRecordBuffer *buf) xl_invalidations *invalidations = (xl_invalidations *) XLogRecGetData(r); - if (!ctx->fast_forward) - ReorderBufferImmediateInvalidation(ctx->reorder, - invalidations->nmsgs, - invalidations->msgs); + ReorderBufferImmediateInvalidation(ctx->reorder, + invalidations->nmsgs, + invalidations->msgs); } break; default: @@ -360,11 +359,9 @@ DecodeHeap2Op(LogicalDecodingContext *ctx, XLogRecordBuffer *buf) ReorderBufferProcessXid(ctx->reorder, xid, buf->origptr); /* - * If we don't have snapshot or we are just fast-forwarding, there is no - * point in decoding changes. + * If we don't have snapshot, we have no reason to decode changes. */ - if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT || - ctx->fast_forward) + if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT) return; switch (info) @@ -379,6 +376,13 @@ DecodeHeap2Op(LogicalDecodingContext *ctx, XLogRecordBuffer *buf) xl_heap_new_cid *xlrec; xlrec = (xl_heap_new_cid *) XLogRecGetData(buf->record); + + /* + * Must process this one even when fast-forwarding. (If we + * were to skip it, an incorrect snapshout would be built + * for catalog changes that don't carry invalidations, such + * as for user-marked catalog tables.) + */ SnapBuildProcessNewCid(builder, xid, buf->origptr, xlrec); break;
