Re: Reviewing freeze map code

Hi,

some of the review items here are mere matters of style/preference. Feel
entirely free to discard them, but I thought if I'm going through the
change anyway...


On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
> a892234 Change the format of the VM fork to add a second bit per page.

TL;DR: fairly minor stuff.


+ * 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?


+    /*
+     * 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?).


+ *
+ * Clearing both visibility map bits is not separately WAL-logged.  The callers * must make sure that whenever a bit
iscleared, 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.
 * * VACUUM will normally skip pages for which the visibility map bit is set; * such pages can't contain any dead
tuplesand 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...).


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


#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?


+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?


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


 * 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
onthe 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?

    /*
-     * 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
wemake 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.


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



@@ -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?


+        /*
+         * 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.


Greetings,

Andres Freund