On Fri, Nov 15, 2019 at 10:34:55AM +0900, Michael Paquier wrote: > It seems to me that if the plan is to have one option structure for > each index AM, which has actually the advantage to reduce the bloat of > each relcache entry currently relying on StdRdOptions, then we could > have those extra assertion checks in the same patch, because the new > macros are introduced.
I have looked at this patch, and did not like much having the calculations of the page free space around, so I have moved that into each AM's dedicated header. > There is rd_rel->relam. You can for example refer to pgstatindex.c > which has AM-related checks to make sure that the correct index AM is > being used. We can do something similar for GIN and BRIN on top of the rest, so updated the patch with that. Nikolay, I would be fine to commit this patch as-is. Thanks for your patience on this stuff. -- Michael
diff --git a/src/include/access/brin.h b/src/include/access/brin.h
index fb351b36e0..794088867a 100644
--- a/src/include/access/brin.h
+++ b/src/include/access/brin.h
@@ -37,11 +37,15 @@ typedef struct BrinStatsData
#define BRIN_DEFAULT_PAGES_PER_RANGE 128
#define BrinGetPagesPerRange(relation) \
- ((relation)->rd_options ? \
+ (AssertMacro(relation->rd_rel->relkind == RELKIND_INDEX && \
+ relation->rd_rel->relam == BRIN_AM_OID), \
+ (relation)->rd_options ? \
((BrinOptions *) (relation)->rd_options)->pagesPerRange : \
BRIN_DEFAULT_PAGES_PER_RANGE)
#define BrinGetAutoSummarize(relation) \
- ((relation)->rd_options ? \
+ (AssertMacro(relation->rd_rel->relkind == RELKIND_INDEX && \
+ relation->rd_rel->relam == BRIN_AM_OID), \
+ (relation)->rd_options ? \
((BrinOptions *) (relation)->rd_options)->autosummarize : \
false)
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 78fcd826f1..4e45552b3f 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -14,6 +14,7 @@
#include "access/gin.h"
#include "access/ginblock.h"
#include "access/itup.h"
+#include "catalog/pg_am_d.h"
#include "fmgr.h"
#include "storage/bufmgr.h"
#include "lib/rbtree.h"
@@ -30,10 +31,14 @@ typedef struct GinOptions
#define GIN_DEFAULT_USE_FASTUPDATE true
#define GinGetUseFastUpdate(relation) \
- ((relation)->rd_options ? \
+ (AssertMacro(relation->rd_rel->relkind == RELKIND_INDEX && \
+ relation->rd_rel->relam == GIN_AM_OID), \
+ (relation)->rd_options ? \
((GinOptions *) (relation)->rd_options)->useFastUpdate : GIN_DEFAULT_USE_FASTUPDATE)
#define GinGetPendingListCleanupSize(relation) \
- ((relation)->rd_options && \
+ (AssertMacro(relation->rd_rel->relkind == RELKIND_INDEX && \
+ relation->rd_rel->relam == GIN_AM_OID), \
+ (relation)->rd_options && \
((GinOptions *) (relation)->rd_options)->pendingListCleanupSize != -1 ? \
((GinOptions *) (relation)->rd_options)->pendingListCleanupSize : \
gin_pending_list_limit)
diff --git a/src/include/access/hash.h b/src/include/access/hash.h
index 24af778fdf..7c530a99a0 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -20,6 +20,7 @@
#include "access/amapi.h"
#include "access/itup.h"
#include "access/sdir.h"
+#include "catalog/pg_am_d.h"
#include "lib/stringinfo.h"
#include "storage/bufmgr.h"
#include "storage/lockdefs.h"
@@ -263,6 +264,21 @@ typedef struct HashMetaPageData
typedef HashMetaPageData *HashMetaPage;
+typedef struct HashOptions
+{
+ int32 varlena_header_; /* varlena header (do not touch directly!) */
+ int fillfactor; /* page fill factor in percent (0..100) */
+} HashOptions;
+
+#define HashGetFillFactor(relation) \
+ (AssertMacro(relation->rd_rel->relkind == RELKIND_INDEX && \
+ relation->rd_rel->relam == HASH_AM_OID), \
+ (relation)->rd_options ? \
+ ((HashOptions *) (relation)->rd_options)->fillfactor : \
+ HASH_DEFAULT_FILLFACTOR)
+#define HashGetTargetPageUsage(relation) \
+ (BLCKSZ * HashGetFillFactor(relation) / 100)
+
/*
* Maximum size of a hash index item (it's okay to have only one per page)
*/
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 4a80e84aa7..ca24dfdc4d 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -18,6 +18,7 @@
#include "access/itup.h"
#include "access/sdir.h"
#include "access/xlogreader.h"
+#include "catalog/pg_am_d.h"
#include "catalog/pg_index.h"
#include "lib/stringinfo.h"
#include "storage/bufmgr.h"
@@ -680,6 +681,23 @@ typedef BTScanOpaqueData *BTScanOpaque;
#define SK_BT_DESC (INDOPTION_DESC << SK_BT_INDOPTION_SHIFT)
#define SK_BT_NULLS_FIRST (INDOPTION_NULLS_FIRST << SK_BT_INDOPTION_SHIFT)
+typedef struct BTOptions
+{
+ int32 varlena_header_; /* varlena header (do not touch directly!) */
+ int fillfactor; /* page fill factor in percent (0..100) */
+ /* fraction of newly inserted tuples prior to trigger index cleanup */
+ float8 vacuum_cleanup_index_scale_factor;
+} BTOptions;
+
+#define BTGetFillFactor(relation) \
+ (AssertMacro(relation->rd_rel->relkind == RELKIND_INDEX && \
+ relation->rd_rel->relam == BTREE_AM_OID), \
+ (relation)->rd_options ? \
+ ((BTOptions *) (relation)->rd_options)->fillfactor : \
+ BTREE_DEFAULT_FILLFACTOR)
+#define BTGetTargetPageFreeSpace(relation) \
+ (BLCKSZ * (100 - BTGetFillFactor(relation)) / 100)
+
/*
* Constant definition for progress reporting. Phase numbers must match
* btbuildphasename.
diff --git a/src/include/access/spgist.h b/src/include/access/spgist.h
index d787ab213d..d5fd7bcc02 100644
--- a/src/include/access/spgist.h
+++ b/src/include/access/spgist.h
@@ -19,10 +19,6 @@
#include "lib/stringinfo.h"
-/* reloption parameters */
-#define SPGIST_MIN_FILLFACTOR 10
-#define SPGIST_DEFAULT_FILLFACTOR 80
-
/* SPGiST opclass support function numbers */
#define SPGIST_CONFIG_PROC 1
#define SPGIST_CHOOSE_PROC 2
diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h
index 428e54bfbd..193ca105c7 100644
--- a/src/include/access/spgist_private.h
+++ b/src/include/access/spgist_private.h
@@ -16,12 +16,29 @@
#include "access/itup.h"
#include "access/spgist.h"
+#include "catalog/pg_am_d.h"
#include "nodes/tidbitmap.h"
#include "storage/buf.h"
#include "utils/geo_decls.h"
#include "utils/relcache.h"
+typedef struct SpGistOptions
+{
+ int32 varlena_header_; /* varlena header (do not touch directly!) */
+ int fillfactor; /* page fill factor in percent (0..100) */
+} SpGistOptions;
+
+#define SpGistGetFillFactor(relation) \
+ (AssertMacro(relation->rd_rel->relkind == RELKIND_INDEX && \
+ relation->rd_rel->relam == SPGIST_AM_OID), \
+ (relation)->rd_options ? \
+ ((SpGistOptions *) (relation)->rd_options)->fillfactor : \
+ SPGIST_DEFAULT_FILLFACTOR)
+#define SpGistGetTargetPageFreeSpace(relation) \
+ (BLCKSZ * (100 - SpGistGetFillFactor(relation)) / 100)
+
+
/* Page numbers of fixed-location pages */
#define SPGIST_METAPAGE_BLKNO (0) /* metapage */
#define SPGIST_ROOT_BLKNO (1) /* root for normal entries */
@@ -423,6 +440,11 @@ typedef SpGistDeadTupleData *SpGistDeadTuple;
#define GBUF_REQ_NULLS(flags) ((flags) & GBUF_NULLS)
/* spgutils.c */
+
+/* reloption parameters */
+#define SPGIST_MIN_FILLFACTOR 10
+#define SPGIST_DEFAULT_FILLFACTOR 80
+
extern SpGistCache *spgGetCache(Relation index);
extern void initSpGistState(SpGistState *state, Relation index);
extern Buffer SpGistNewBuffer(Relation index);
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 8b8b237f0d..3fc12bcd51 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -265,7 +265,6 @@ typedef struct StdRdOptions
int32 vl_len_; /* varlena header (do not touch directly!) */
int fillfactor; /* page fill factor in percent (0..100) */
/* fraction of newly inserted tuples prior to trigger index cleanup */
- float8 vacuum_cleanup_index_scale_factor;
int toast_tuple_target; /* target for tuple toasting */
AutoVacOpts autovacuum; /* autovacuum-related options */
bool user_catalog_table; /* use as an additional catalog relation */
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 3f22a6c354..48377ace24 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -23,7 +23,7 @@
#include "access/htup_details.h"
#include "access/nbtree.h"
#include "access/reloptions.h"
-#include "access/spgist.h"
+#include "access/spgist_private.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
#include "commands/tablespace.h"
@@ -1521,8 +1521,6 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
offsetof(StdRdOptions, user_catalog_table)},
{"parallel_workers", RELOPT_TYPE_INT,
offsetof(StdRdOptions, parallel_workers)},
- {"vacuum_cleanup_index_scale_factor", RELOPT_TYPE_REAL,
- offsetof(StdRdOptions, vacuum_cleanup_index_scale_factor)},
{"vacuum_index_cleanup", RELOPT_TYPE_BOOL,
offsetof(StdRdOptions, vacuum_index_cleanup)},
{"vacuum_truncate", RELOPT_TYPE_BOOL,
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index 63697bfeb5..f84cee8935 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -358,7 +358,7 @@ _hash_init(Relation rel, double num_tuples, ForkNumber forkNum)
data_width = sizeof(uint32);
item_width = MAXALIGN(sizeof(IndexTupleData)) + MAXALIGN(data_width) +
sizeof(ItemIdData); /* include the line pointer */
- ffactor = RelationGetTargetPageUsage(rel, HASH_DEFAULT_FILLFACTOR) / item_width;
+ ffactor = HashGetTargetPageUsage(rel) / item_width;
/* keep to a sane range */
if (ffactor < 10)
ffactor = 10;
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index c5005f4754..669dccc492 100644
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -289,7 +289,14 @@ _hash_checkpage(Relation rel, Buffer buf, int flags)
bytea *
hashoptions(Datum reloptions, bool validate)
{
- return default_reloptions(reloptions, validate, RELOPT_KIND_HASH);
+ static const relopt_parse_elt tab[] = {
+ {"fillfactor", RELOPT_TYPE_INT, offsetof(HashOptions, fillfactor)},
+ };
+
+ return (bytea *) build_reloptions(reloptions, validate,
+ RELOPT_KIND_HASH,
+ sizeof(HashOptions),
+ tab, lengthof(tab));
}
/*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4cfd5289ad..c67235ab80 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -816,7 +816,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
}
else
{
- StdRdOptions *relopts;
+ BTOptions *relopts;
float8 cleanup_scale_factor;
float8 prev_num_heap_tuples;
@@ -827,7 +827,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
* tuples exceeds vacuum_cleanup_index_scale_factor fraction of
* original tuples count.
*/
- relopts = (StdRdOptions *) info->index->rd_options;
+ relopts = (BTOptions *) info->index->rd_options;
cleanup_scale_factor = (relopts &&
relopts->vacuum_cleanup_index_scale_factor >= 0)
? relopts->vacuum_cleanup_index_scale_factor
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index fc7d43a0f3..1dd39a9535 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -716,8 +716,8 @@ _bt_pagestate(BTWriteState *wstate, uint32 level)
if (level > 0)
state->btps_full = (BLCKSZ * (100 - BTREE_NONLEAF_FILLFACTOR) / 100);
else
- state->btps_full = RelationGetTargetPageFreeSpace(wstate->index,
- BTREE_DEFAULT_FILLFACTOR);
+ state->btps_full = BTGetTargetPageFreeSpace(wstate->index);
+
/* no parent level, yet */
state->btps_next = NULL;
diff --git a/src/backend/access/nbtree/nbtsplitloc.c b/src/backend/access/nbtree/nbtsplitloc.c
index a04d4e25d6..29167f1ef5 100644
--- a/src/backend/access/nbtree/nbtsplitloc.c
+++ b/src/backend/access/nbtree/nbtsplitloc.c
@@ -167,7 +167,7 @@ _bt_findsplitloc(Relation rel,
/* Count up total space in data items before actually scanning 'em */
olddataitemstotal = rightspace - (int) PageGetExactFreeSpace(page);
- leaffillfactor = RelationGetFillFactor(rel, BTREE_DEFAULT_FILLFACTOR);
+ leaffillfactor = BTGetFillFactor(rel);
/* Passed-in newitemsz is MAXALIGNED but does not include line pointer */
newitemsz += sizeof(ItemIdData);
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index 7669a1a66f..5faade637c 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -2014,7 +2014,17 @@ BTreeShmemInit(void)
bytea *
btoptions(Datum reloptions, bool validate)
{
- return default_reloptions(reloptions, validate, RELOPT_KIND_BTREE);
+ static const relopt_parse_elt tab[] = {
+ {"fillfactor", RELOPT_TYPE_INT, offsetof(BTOptions, fillfactor)},
+ {"vacuum_cleanup_index_scale_factor", RELOPT_TYPE_REAL,
+ offsetof(BTOptions, vacuum_cleanup_index_scale_factor)}
+
+ };
+ return (bytea *) build_reloptions(reloptions, validate,
+ RELOPT_KIND_BTREE,
+ sizeof(BTOptions),
+ tab, lengthof(tab));
+
}
/*
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 45472db147..3331c5f162 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -408,8 +408,7 @@ SpGistGetBuffer(Relation index, int flags, int needSpace, bool *isNew)
* related to the ones already on it. But fillfactor mustn't cause an
* error for requests that would otherwise be legal.
*/
- needSpace += RelationGetTargetPageFreeSpace(index,
- SPGIST_DEFAULT_FILLFACTOR);
+ needSpace += SpGistGetTargetPageFreeSpace(index);
needSpace = Min(needSpace, SPGIST_PAGE_CAPACITY);
/* Get the cache entry for this flags setting */
@@ -586,7 +585,14 @@ SpGistInitMetapage(Page page)
bytea *
spgoptions(Datum reloptions, bool validate)
{
- return default_reloptions(reloptions, validate, RELOPT_KIND_SPGIST);
+ static const relopt_parse_elt tab[] = {
+ {"fillfactor", RELOPT_TYPE_INT, offsetof(SpGistOptions, fillfactor)},
+ };
+ return (bytea *) build_reloptions(reloptions, validate,
+ RELOPT_KIND_SPGIST,
+ sizeof(SpGistOptions),
+ tab, lengthof(tab));
+
}
/*
signature.asc
Description: PGP signature
