Clean up manipulations of hash indexes' hasho_flag field.

Standardize on testing a hash index page's type by doing
	(opaque->hasho_flag & LH_PAGE_TYPE) == LH_xxx_PAGE
Various places were taking shortcuts like
	opaque->hasho_flag & LH_BUCKET_PAGE
which while not actually wrong, is still bad practice because
it encourages use of
	opaque->hasho_flag & LH_UNUSED_PAGE
which *is* wrong (LH_UNUSED_PAGE == 0, so the above is constant false).
hash_xlog.c's hash_mask() contained such an incorrect test.

This also ensures that we mask out the additional flag bits that
hasho_flag has accreted since 9.6.  pgstattuple's pgstat_hash_page(),
for one, was failing to do that and was thus actively broken.

Also fix assorted comments that hadn't been updated to reflect the
extended usage of hasho_flag, and fix some macros that were testing
just "(hasho_flag & bit)" to use the less dangerous, project-approved
form "((hasho_flag & bit) != 0)".

Coverity found the bug in hash_mask(); I noted the one in
pgstat_hash_page() through code reading.

diff --git a/contrib/pageinspect/hashfuncs.c b/contrib/pageinspect/hashfuncs.c
index dd00aaa81a6941f05bf8a35f7cda3bbed24f96bc..6e52969fd3447f7674fe93aaa523ab1547ab2348 100644 (file)
--- a/contrib/pageinspect/hashfuncs.c
+++ b/contrib/pageinspect/hashfuncs.c
@@ -184,7 +184,8 @@ hash_page_type(PG_FUNCTION_ARGS)
    bytea      *raw_page = PG_GETARG_BYTEA_P(0);
    Page        page;
    HashPageOpaque opaque;
-   char       *type;
+   int         pagetype;
+   const char *type;
 
    if (!superuser())
        ereport(ERROR,
@@ -200,13 +201,14 @@ hash_page_type(PG_FUNCTION_ARGS)
        opaque = (HashPageOpaque) PageGetSpecialPointer(page);
 
        /* page type (flags) */
-       if (opaque->hasho_flag & LH_META_PAGE)
+       pagetype = opaque->hasho_flag & LH_PAGE_TYPE;
+       if (pagetype == LH_META_PAGE)
            type = "metapage";
-       else if (opaque->hasho_flag & LH_OVERFLOW_PAGE)
+       else if (pagetype == LH_OVERFLOW_PAGE)
            type = "overflow";
-       else if (opaque->hasho_flag & LH_BUCKET_PAGE)
+       else if (pagetype == LH_BUCKET_PAGE)
            type = "bucket";
-       else if (opaque->hasho_flag & LH_BITMAP_PAGE)
+       else if (pagetype == LH_BITMAP_PAGE)
            type = "bitmap";
        else
            type = "unused";

diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 44f90cd0d371c66403ff5d326e27b6f707ca61d2..eb02ec5b890e3d3e2d1ca3eb41ed245b4427cd52 100644 (file)
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -453,7 +453,7 @@ pgstat_hash_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno,
        HashPageOpaque opaque;
 
        opaque = (HashPageOpaque) PageGetSpecialPointer(page);
-       switch (opaque->hasho_flag)
+       switch (opaque->hasho_flag & LH_PAGE_TYPE)
        {
            case LH_UNUSED_PAGE:
                stat->free_space += BLCKSZ;

diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
index 2ccaf469e7850fba7beffbac31501f90f2cd56bc..d1c0e6904fcd58a8d0febf63ab70f6f6bf86faad 100644 (file)
--- a/src/backend/access/hash/hash_xlog.c
+++ b/src/backend/access/hash/hash_xlog.c
@@ -1234,6 +1234,7 @@ hash_mask(char *pagedata, BlockNumber blkno)
 {
    Page        page = (Page) pagedata;
    HashPageOpaque opaque;
+   int         pagetype;
 
    mask_page_lsn(page);
 
@@ -1242,15 +1243,16 @@ hash_mask(char *pagedata, BlockNumber blkno)
 
    opaque = (HashPageOpaque) PageGetSpecialPointer(page);
 
-   if (opaque->hasho_flag & LH_UNUSED_PAGE)
+   pagetype = opaque->hasho_flag & LH_PAGE_TYPE;
+   if (pagetype == LH_UNUSED_PAGE)
    {
        /*
         * Mask everything on a UNUSED page.
         */
        mask_page_content(page);
    }
-   else if ((opaque->hasho_flag & LH_BUCKET_PAGE) ||
-            (opaque->hasho_flag & LH_OVERFLOW_PAGE))
+   else if (pagetype == LH_BUCKET_PAGE ||
+            pagetype == LH_OVERFLOW_PAGE)
    {
        /*
         * In hash bucket and overflow pages, it is possible to modify the

diff --git a/src/backend/access/hash/hashovfl.c b/src/backend/access/hash/hashovfl.c
index d5f502306830ee8a49c9280f88a05a9728116bcd..b5133e3945cd44c467e77bd0f5810014d467ec95 100644 (file)
--- a/src/backend/access/hash/hashovfl.c
+++ b/src/backend/access/hash/hashovfl.c
@@ -168,7 +168,7 @@ _hash_addovflpage(Relation rel, Buffer metabuf, Buffer buf, bool retain_pin)
        if (retain_pin)
        {
            /* pin will be retained only for the primary bucket page */
-           Assert(pageopaque->hasho_flag & LH_BUCKET_PAGE);
+           Assert((pageopaque->hasho_flag & LH_PAGE_TYPE) == LH_BUCKET_PAGE);
            LockBuffer(buf, BUFFER_LOCK_UNLOCK);
        }
        else

diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index 037582bd7927f65d51147b7fc993c21518f5a3c8..9f832f2544fcfadfe8b3d768ee4391793369b032 100644 (file)
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -218,8 +218,8 @@ _hash_get_totalbuckets(uint32 splitpoint_phase)
 /*
  * _hash_checkpage -- sanity checks on the format of all hash pages
  *
- * If flags is not zero, it is a bitwise OR of the acceptable values of
- * hasho_flag.
+ * If flags is not zero, it is a bitwise OR of the acceptable page types
+ * (values of hasho_flag & LH_PAGE_TYPE).
  */
 void
 _hash_checkpage(Relation rel, Buffer buf, int flags)

diff --git a/src/include/access/hash.h b/src/include/access/hash.h
index fcc395703678c74a9c8b2f6a88d0f67f1417b9c0..adba224008c155fa1c110a85ad7ab13e8c68bdc8 100644 (file)
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -41,13 +41,13 @@ typedef uint32 Bucket;
 /*
  * Special space for hash index pages.
  *
- * hasho_flag tells us which type of page we're looking at.  For
- * example, knowing overflow pages from bucket pages is necessary
- * information when you're deleting tuples from a page. If all the
- * tuples are deleted from an overflow page, the overflow is made
- * available to other buckets by calling _hash_freeovflpage(). If all
- * the tuples are deleted from a bucket page, no additional action is
- * necessary.
+ * hasho_flag's LH_PAGE_TYPE bits tell us which type of page we're looking at.
+ * Additional bits in the flag word are used for more transient purposes.
+ *
+ * To test a page's type, do (hasho_flag & LH_PAGE_TYPE) == LH_xxx_PAGE.
+ * However, we ensure that each used page type has a distinct bit so that
+ * we can OR together page types for uses such as the allowable-page-types
+ * argument of _hash_checkpage().
  */
 #define LH_UNUSED_PAGE         (0)
 #define LH_OVERFLOW_PAGE       (1 << 0)
@@ -60,7 +60,7 @@ typedef uint32 Bucket;
 #define LH_PAGE_HAS_DEAD_TUPLES    (1 << 7)
 
 #define LH_PAGE_TYPE \
-   (LH_OVERFLOW_PAGE|LH_BUCKET_PAGE|LH_BITMAP_PAGE|LH_META_PAGE)
+   (LH_OVERFLOW_PAGE | LH_BUCKET_PAGE | LH_BITMAP_PAGE | LH_META_PAGE)
 
 /*
  * In an overflow page, hasho_prevblkno stores the block number of the previous
@@ -78,16 +78,16 @@ typedef struct HashPageOpaqueData
    BlockNumber hasho_prevblkno;    /* see above */
    BlockNumber hasho_nextblkno;    /* see above */
    Bucket      hasho_bucket;   /* bucket number this pg belongs to */
-   uint16      hasho_flag;     /* page type code, see above */
+   uint16      hasho_flag;     /* page type code + flag bits, see above */
    uint16      hasho_page_id;  /* for identification of hash indexes */
 } HashPageOpaqueData;
 
 typedef HashPageOpaqueData *HashPageOpaque;
 
-#define H_NEEDS_SPLIT_CLEANUP(opaque)  ((opaque)->hasho_flag & LH_BUCKET_NEEDS_SPLIT_CLEANUP)
-#define H_BUCKET_BEING_SPLIT(opaque)   ((opaque)->hasho_flag & LH_BUCKET_BEING_SPLIT)
-#define H_BUCKET_BEING_POPULATED(opaque)   ((opaque)->hasho_flag & LH_BUCKET_BEING_POPULATED)
-#define H_HAS_DEAD_TUPLES(opaque)      ((opaque)->hasho_flag & LH_PAGE_HAS_DEAD_TUPLES)
+#define H_NEEDS_SPLIT_CLEANUP(opaque)  (((opaque)->hasho_flag & LH_BUCKET_NEEDS_SPLIT_CLEANUP) != 0)
+#define H_BUCKET_BEING_SPLIT(opaque)   (((opaque)->hasho_flag & LH_BUCKET_BEING_SPLIT) != 0)
+#define H_BUCKET_BEING_POPULATED(opaque)   (((opaque)->hasho_flag & LH_BUCKET_BEING_POPULATED) != 0)
+#define H_HAS_DEAD_TUPLES(opaque)      (((opaque)->hasho_flag & LH_PAGE_HAS_DEAD_TUPLES) != 0)
 
 /*
  * The page ID is for the convenience of pg_filedump and similar utilities,