Re: Duplicated LSN in ReorderBuffer

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