Re: Reviewing freeze map code

On Mon, May 2, 2016 at 8:25 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> + * heap_tuple_needs_eventual_freeze
> + *
> + * Check to see whether any of the XID fields of a tuple (xmin, xmax, xvac)
> + * will eventually require freezing. Similar to heap_tuple_needs_freeze,
> + * but there's no cutoff, since we're trying to figure out whether freezing
> + * will ever be needed, not whether it's needed now.
> + */
> +bool
> +heap_tuple_needs_eventual_freeze(HeapTupleHeader tuple)
>
> Wouldn't redefining this to heap_tuple_is_frozen() and then inverting the
> checks be easier to understand?

I thought it much safer to keep this as close to a copy of
heap_tuple_needs_freeze() as possible. Copying a function and
inverting all of the return values is much more likely to introduce
bugs, IME.

> + /*
> + * If xmax is a valid xact or multixact, this tuple is also not frozen.
> + */
> + if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
> + {
> + MultiXactId multi;
> +
> + multi = HeapTupleHeaderGetRawXmax(tuple);
> + if (MultiXactIdIsValid(multi))
> + return true;
> + }
>
> Hm. What's the test inside the if() for? There shouldn't be any case
> where xmax is invalid if HEAP_XMAX_IS_MULTI is set. Now there's a
> check like that outside of this commit, but it seems strange to me
> (Alvaro, perhaps you could comment on this?).

Here again I was copying existing code, with appropriate simplifications.

> + *
> + * Clearing both visibility map bits is not separately WAL-logged. The callers
> * must make sure that whenever a bit is cleared, the bit is cleared on WAL
> * replay of the updating operation as well.
>
> I think including "both" here makes things less clear, because it
> differentiates clearing one bit from clearing both. There's no practical
> differentce atm, but still.

I agree.

> *
> * VACUUM will normally skip pages for which the visibility map bit is set;
> * such pages can't contain any dead tuples and therefore don't need vacuuming.
> - * The visibility map is not used for anti-wraparound vacuums, because
> - * an anti-wraparound vacuum needs to freeze tuples and observe the latest xid
> - * present in the table, even on pages that don't have any dead tuples.
> *
>
> I think the remaining sentence isn't entirely accurate, there's now more
> than one bit, and they're different with regard to scan_all/!scan_all
> vacuums (or will be - maybe this updated further in a later commit? But
> if so, that sentence shouldn't yet be removed...).

We can adjust the language, but I don't really see a big problem here.

> -/* Number of heap blocks we can represent in one byte. */
> -#define HEAPBLOCKS_PER_BYTE 8
> -
> Hm, why was this moved to the header? Sounds like something the outside
> shouldn't care about.

Oh... yeah. Let's undo that.

> #define HEAPBLK_TO_MAPBIT(x) (((x) % HEAPBLOCKS_PER_BYTE) * BITS_PER_HEAPBLOCK)
>
> Hm. This isn't really a mapping to an individual bit anymore - but I
> don't really have a better name in mind. Maybe TO_OFFSET?

Well, it sorta is... but we could change it, I suppose.

> +static const uint8 number_of_ones_for_visible[256] = {
> ...
> +};
> +static const uint8 number_of_ones_for_frozen[256] = {
> ...
> };
>
> Did somebody verify the new contents are correct?

I admit that I didn't. It seemed like an unlikely place for a goof,
but I guess we should verify.

> /*
> - * visibilitymap_clear - clear a bit in visibility map
> + * visibilitymap_clear - clear all bits in visibility map
> *
>
> This seems rather easy to misunderstand, as this really only clears all
> the bits for one page, not actually all the bits.

We could change "in" to "for one page in the".

> * the bit for heapBlk, or InvalidBuffer. The caller is responsible for
> - * releasing *buf after it's done testing and setting bits.
> + * releasing *buf after it's done testing and setting bits, and must pass flags
> + * for which it needs to check the value in visibility map.
> *
> * NOTE: This function is typically called without a lock on the heap page,
> * so somebody else could change the bit just after we look at it. In fact,
> @@ -327,17 +351,16 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
>
> I'm not seing what flags the above comment change is referring to?

Ugh. I think that's leftover cruft from an earlier patch version that
should have been excised from what got committed.

> /*
> - * A single-bit read is atomic. There could be memory-ordering effects
> + * A single byte read is atomic. There could be memory-ordering effects
> * here, but for performance reasons we make it the caller's job to worry
> * about that.
> */
> - result = (map[mapByte] & (1 << mapBit)) ? true : false;
> -
> - return result;
> + return ((map[mapByte] >> mapBit) & VISIBILITYMAP_VALID_BITS);
> }
>
> Not a new issue, and *very* likely to be irrelevant in practice (given
> the value is only referenced once): But there's really no guarantee
> map[mapByte] is only read once here.

Meh. But we can fix if you want to.

> -BlockNumber
> -visibilitymap_count(Relation rel)
> +void
> +visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_frozen)
>
> Not really a new issue again: The parameter types (previously return
> type) to this function seem wrong to me.

Not this patch's job to tinker.

> @@ -1934,5 +1992,14 @@ heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId *visibility_cut
> }
> + /*
> + * We don't bother clearing *all_frozen when the page is discovered not
> + * to be all-visible, so do that now if necessary. The page might fail
> + * to be all-frozen for other reasons anyway, but if it's not all-visible,
> + * then it definitely isn't all-frozen.
> + */
> + if (!all_visible)
> + *all_frozen = false;
> +
>
> Why don't we just set *all_frozen to false when appropriate? It'd be
> just as many lines and probably easier to understand?

I thought that looked really easy to mess up, either now or down the
road. This way seemed more solid to me. That's a judgement call, of
course.

> + /*
> + * If the page is marked as all-visible but not all-frozen, we should
> + * so mark it. Note that all_frozen is only valid if all_visible is
> + * true, so we must check both.
> + */
>
> This kinda seems to imply that all-visible implies all_frozen. Also, why
> has that block been added to the end of the if/else if chain? Seems like
> it belongs below the (all_visible && !all_visible_according_to_vm) block.

We can adjust the comment a bit to make it more clear, if you like,
but I doubt it's going to cause serious misunderstanding. As for the
placement, the reason I put it at the end is because I figured that we
did not want to mark it all-frozen if any of the "oh crap, emit a
warning" cases applied.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company