Re: wal_consistency_checking reports an inconsistency on master branch

On Mon, Apr 23, 2018 at 07:58:30AM -0700, Andres Freund wrote:
> On 2018-04-23 13:22:21 +0300, Heikki Linnakangas wrote:
>> 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.

True that the spculative inserts and the partition move are handled in
inconsistent ways now.

>> 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...

-/*
- * Special value used in t_ctid.ip_posid, to indicate that it holds a
- * speculative insertion token rather than a real TID. This must be
-higher
- * than MaxOffsetNumber, so that it can be distinguished from a valid
- * offset number in a regular item pointer.
- */
-#define SpecTokenOffsetNumber 0xfff
Moving those definitions from htup_details.h to itemptr.h seems
confusing for me. As those are heap-related operations, I would
recommend to keep them where they are, and also move those two ones
to htup_details.h, renaming them on the way so as they are more
-consistent:
- ItemPointerIndicatesMovedPartitions
- ItemPointerSetMovedPartitions
--
Michael