Re: wal_consistency_checking reports an inconsistency on master branch

On 2018-04-23 13:22:21 +0300, Heikki Linnakangas wrote:
> On 13/04/18 13:08, Michael Paquier wrote:
> > On Fri, Apr 13, 2018 at 02:15:35PM +0530, amul sul wrote:
> > > I have looked into this and found that the issue is in heap_xlog_delete -- we
> > > have missed to set the correct offset number from the target_tid when
> > > XLH_DELETE_IS_PARTITION_MOVE flag is set.
> >
> > Oh, this looks good to me. So when a row was moved across partitions
> > this could have caused incorrect tuple references on a standby, which
> > could have caused corruptions.
>
> Hmm. So, the problem was that HeapTupleHeaderSetMovedPartitions() only sets
> the block number to InvalidBlockNumber, and leaves the offset number
> unchanged. WAL replay didn't preserve the offset number, so the master and
> the standby had a different offset number in the ctid.

Right.

> Why does HeapTupleHeaderSetMovedPartitions() leave the offset number
> unchanged? The old offset number is meaningless without the block number.
> Also, bits and magic values in the tuple header are scarce. We're
> squandering a whole range of values in the ctid, everything with
> ip_blkid==InvalidBlockNumber, to mean "moved to different partition", when a
> single value would suffice.

Yes, I agree on that.

> I kept using InvalidBlockNumber there, so ItemPointerIsValid() still
> considers those item pointers as invalid. But my gut feeling is actually
> that it would be better to use e.g. 0 as the block number, so that these
> item pointers would appear valid. Again, to follow the precedent of
> speculative insertion tokens. But I'm not sure if there was some
> well-thought-out reason to make them appear invalid. A comment on that would
> be nice, at least.

That seems risky to me. We want something that stops EPQ style chasing
without running into asserts for invalid offsets...

Greetings,

Andres Freund