diff options
author | Tom Lane <tgl@sss.pgh.pa.us> | 2017-04-14 17:04:25 -0400 |
---|---|---|
committer | Tom Lane <tgl@sss.pgh.pa.us> | 2017-04-14 17:04:25 -0400 |
commit | 2040bb4a0b50ef0434a1a723f00d040ab4f1c06f (patch) | |
tree | cc6feca5b13b82793004108cedc54972891b40ea | |
parent | 1dffabed49054a81b6005a363ab2da4aee0aab9e (diff) | |
download | postgresql-2040bb4a0b50ef0434a1a723f00d040ab4f1c06f.tar.gz |
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.
-rw-r--r-- | contrib/pageinspect/hashfuncs.c | 12 | ||||
-rw-r--r-- | contrib/pgstattuple/pgstattuple.c | 2 | ||||
-rw-r--r-- | src/backend/access/hash/hash_xlog.c | 8 | ||||
-rw-r--r-- | src/backend/access/hash/hashovfl.c | 2 | ||||
-rw-r--r-- | src/backend/access/hash/hashutil.c | 4 | ||||
-rw-r--r-- | src/include/access/hash.h | 26 |
6 files changed, 29 insertions, 25 deletions
diff --git a/contrib/pageinspect/hashfuncs.c b/contrib/pageinspect/hashfuncs.c index dd00aaa81a..6e52969fd3 100644 --- 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 44f90cd0d3..eb02ec5b89 100644 --- 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 2ccaf469e7..d1c0e6904f 100644 --- 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 d5f5023068..b5133e3945 100644 --- 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 037582bd79..9f832f2544 100644 --- 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 fcc3957036..adba224008 100644 --- 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, |