Hi,
On 27. 07. 19 3:15, Andres Freund wrote:
> Hi,
>
> Petr, Simon, see the potential issue related to fast forward at the
> bottom.
>
> [..snip..]
>
> This actually made me look at the nearby changes due to
>
> commit 9c7d06d60680c7f00d931233873dee81fdb311c6
> Author: Simon Riggs <simon@2ndQuadrant.com>
> Date: 2018-01-17 11:38:34 +0000
>
> Ability to advance replication slots
>
> and uhm, I'm not sure they're fully baked. Something like:
>
> /*
> * If we don't have snapshot or we are just fast-forwarding, there is no
> * point in decoding changes.
> */
> if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
> ctx->fast_forward)
> return;
>
> case XLOG_HEAP2_MULTI_INSERT:
> if (!ctx->fast_forward &&
> SnapBuildProcessChange(builder, xid, buf->origptr))
> DecodeMultiInsert(ctx, buf);
> break;
>
> is not very suggestive of that (note the second check).
>
You mean that it's redundant, yeah.., although given your next point,
see bellow.
>
> 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.
>
Hmm, I guess that's true. I think I have convinced myself that CID does
not matter outside of this transaction, but since we might actually
share the computed snapshot via file save/restore with other slots, any
non-fast-forwarding decoding that reads the same transaction could miss
the CID thanks to the shared snapshot which does not include it.
Given that we don't process any other records in this function besides
XLOG_HEAP2_MULTI_INSERT and XLOG_HEAP2_NEW_CID, it seems like simplest
fix is to just remove the first check for fast forward and keep the one
in XLOG_HEAP2_MULTI_INSERT.
--
Petr Jelinek
2ndQuadrant - PostgreSQL Solutions for the Enterprise
https://www.2ndQuadrant.com/
