On Fri, Sep 20, 2019 at 09:16:58AM +0900, Michael Paquier wrote: > On Thu, Sep 19, 2019 at 02:13:23PM +0300, Nikolay Shaplov wrote: >> What a good catch! dummy_index already proved to be useful ;-) > > Yes, the testing around custom reloptions is rather poor now, so this > module has value. I still don't like much its shape though, so I > began hacking on it for integration, and I wanted to get that part > done in this CF :)
So... I have looked at the patch of upthread in details, and as I suspected the module is over-designed. First, on HEAD the coverage of reloptions.c is 86.6%, with your patch we get at 94.1%, and with the attached I reach 95.1% thanks to the addition of a string parameter with a NULL default value and a NULL description, for roughly half the code size. The GUCs are also basically not necessary, as you can just replace the various WARNING calls (please don't call elog on anything which can be reached by the user!) by lookups at reloptions in pg_class. Once this is removed, the whole code gets more simple, and there is no point in having neither a separate header nor a different set of files and the size of the whole module gets really reduced. I still need to do an extra pass on the code (particularly the AM part), but I think that we could commit that. Please note that I included the fix for the lockmode I sent today so as the patch can be tested: https://www.postgresql.org/message-id/20190920013831.gd1...@paquier.xyz Thoughts? -- Michael
diff --git a/contrib/bloom/expected/bloom.out b/contrib/bloom/expected/bloom.out
index 5ab9e34f82..dae12a7d3e 100644
--- a/contrib/bloom/expected/bloom.out
+++ b/contrib/bloom/expected/bloom.out
@@ -5,6 +5,7 @@ CREATE TABLE tst (
);
INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM generate_series(1,2000) i;
CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 = 3);
+ALTER INDEX bloomidx SET (length=80);
SET enable_seqscan=on;
SET enable_bitmapscan=off;
SET enable_indexscan=off;
diff --git a/contrib/bloom/sql/bloom.sql b/contrib/bloom/sql/bloom.sql
index 32755f2b1a..4733e1e705 100644
--- a/contrib/bloom/sql/bloom.sql
+++ b/contrib/bloom/sql/bloom.sql
@@ -7,6 +7,7 @@ CREATE TABLE tst (
INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM generate_series(1,2000) i;
CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 = 3);
+ALTER INDEX bloomidx SET (length=80);
SET enable_seqscan=on;
SET enable_bitmapscan=off;
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 20f4ed3c38..b59e606771 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -659,6 +659,13 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc)
newoption->namelen = strlen(name);
newoption->type = type;
+ /*
+ * Set the default lock mode for this option. There is no actual way
+ * for a module to enforce it when declaring a custom relation option,
+ * so just use the highest level, which is safe for all cases.
+ */
+ newoption->lockmode = AccessExclusiveLock;
+
MemoryContextSwitchTo(oldcxt);
return newoption;
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 0e4e53d63e..b2eaef3bff 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -7,6 +7,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = \
brin \
commit_ts \
+ dummy_index_am \
dummy_seclabel \
snapshot_too_old \
test_bloomfilter \
diff --git a/src/test/modules/dummy_index_am/.gitignore b/src/test/modules/dummy_index_am/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/dummy_index_am/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/dummy_index_am/Makefile b/src/test/modules/dummy_index_am/Makefile
new file mode 100644
index 0000000000..b7e09b1469
--- /dev/null
+++ b/src/test/modules/dummy_index_am/Makefile
@@ -0,0 +1,20 @@
+# src/test/modules/dummy_index_am/Makefile
+
+MODULES = dummy_index_am
+
+EXTENSION = dummy_index_am
+DATA = dummy_index_am--1.0.sql
+PGFILEDESC = "dummy_index_am - minimized index access method"
+
+REGRESS = reloptions
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/dummy_index_am
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/dummy_index_am/README b/src/test/modules/dummy_index_am/README
new file mode 100644
index 0000000000..5563d060ac
--- /dev/null
+++ b/src/test/modules/dummy_index_am/README
@@ -0,0 +1,11 @@
+Dummy Index AM
+==============
+
+Dummy index is an index module for testing any facility usable by an index
+access method, whose code is kept a maximum simple.
+
+This includes tests for all relation option types:
+- boolean
+- integer
+- real
+- strings
diff --git a/src/test/modules/dummy_index_am/dummy_index_am--1.0.sql b/src/test/modules/dummy_index_am/dummy_index_am--1.0.sql
new file mode 100644
index 0000000000..005863de87
--- /dev/null
+++ b/src/test/modules/dummy_index_am/dummy_index_am--1.0.sql
@@ -0,0 +1,19 @@
+/* src/test/modules/dummy_index_am/dummy_index_am--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION dummy_index_am" to load this file. \quit
+
+CREATE FUNCTION dihandler(internal)
+RETURNS index_am_handler
+AS 'MODULE_PATHNAME'
+LANGUAGE C;
+
+-- Access method
+CREATE ACCESS METHOD dummy_index_am TYPE INDEX HANDLER dihandler;
+COMMENT ON ACCESS METHOD dummy_index_am IS 'dummy index access method';
+
+-- Operator classes
+CREATE OPERATOR CLASS int4_ops
+DEFAULT FOR TYPE int4 USING dummy_index_am AS
+ OPERATOR 1 = (int4, int4),
+ FUNCTION 1 hashint4(int4);
diff --git a/src/test/modules/dummy_index_am/dummy_index_am.c b/src/test/modules/dummy_index_am/dummy_index_am.c
new file mode 100644
index 0000000000..0cee0f62a0
--- /dev/null
+++ b/src/test/modules/dummy_index_am/dummy_index_am.c
@@ -0,0 +1,271 @@
+/*-------------------------------------------------------------------------
+ *
+ * dummy_index.c
+ * Dummy index AM main file.
+ *
+ * Copyright (c) 2019, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/test/modules/dummy_index_am/dummy_index.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "access/amapi.h"
+#include "access/reloptions.h"
+#include "catalog/index.h"
+#include "nodes/pathnodes.h"
+#include "utils/guc.h"
+#include "utils/rel.h"
+
+PG_MODULE_MAGIC;
+
+void _PG_init(void);
+
+/* parse table for fillRelOptions */
+relopt_parse_elt di_relopt_tab[5];
+
+/* Kind of relation options for dummy index */
+relopt_kind di_relopt_kind;
+
+/* Dummy index options */
+typedef struct DummyIndexOptions
+{
+ int32 vl_len_; /* varlena header (do not touch directly!) */
+ int option_int;
+ double option_real;
+ bool option_bool;
+ char *option_string_val;
+ char *option_option_null;
+} DummyIndexOptions;
+
+/*
+ * Validation function for string relation options.
+ */
+static void
+validate_string_option(const char *value)
+{
+ ereport(NOTICE,
+ (errmsg("new option value for string parameter %s",
+ value ? value : "NULL")));
+}
+
+/*
+ * This function creates a full set of relation option types,
+ * with various patterns.
+ */
+static void
+create_reloptions_table(void)
+{
+ di_relopt_kind = add_reloption_kind();
+
+ add_int_reloption(di_relopt_kind, "option_int",
+ "Integer option for dummy_index_am",
+ 10, -10, 100);
+ di_relopt_tab[0].optname = "option_int";
+ di_relopt_tab[0].opttype = RELOPT_TYPE_INT;
+ di_relopt_tab[0].offset = offsetof(DummyIndexOptions, option_int);
+
+ add_real_reloption(di_relopt_kind, "option_real",
+ "Real option for dummy_index_am",
+ 3.1415, -10, 100);
+ di_relopt_tab[1].optname = "option_real";
+ di_relopt_tab[1].opttype = RELOPT_TYPE_REAL;
+ di_relopt_tab[1].offset = offsetof(DummyIndexOptions, option_real);
+
+ add_bool_reloption(di_relopt_kind, "option_bool",
+ "Boolean option for dummy_index_am",
+ true);
+ di_relopt_tab[2].optname = "option_bool";
+ di_relopt_tab[2].opttype = RELOPT_TYPE_BOOL;
+ di_relopt_tab[2].offset = offsetof(DummyIndexOptions, option_bool);
+
+ add_string_reloption(di_relopt_kind, "option_string_val",
+ "String option for dummy_index_am with non-NULL default",
+ "DefaultValue", &validate_string_option);
+ di_relopt_tab[3].optname = "option_string_val";
+ di_relopt_tab[3].opttype = RELOPT_TYPE_STRING;
+ di_relopt_tab[3].offset = offsetof(DummyIndexOptions, option_string_val);
+
+ /*
+ * String option for dummy_index_am with NULL default, and without
+ * description.
+ */
+ add_string_reloption(di_relopt_kind, "option_string_null",
+ NULL, /* description */
+ NULL, &validate_string_option);
+ di_relopt_tab[4].optname = "option_string_null";
+ di_relopt_tab[4].opttype = RELOPT_TYPE_STRING;
+ di_relopt_tab[4].offset = offsetof(DummyIndexOptions, option_option_null);
+}
+
+
+PG_FUNCTION_INFO_V1(dihandler);
+
+static IndexBuildResult *
+dibuild(Relation heap, Relation index, IndexInfo *indexInfo)
+{
+ IndexBuildResult *result;
+
+ result = (IndexBuildResult *) palloc(sizeof(IndexBuildResult));
+
+ /* let's pretend that no tuples were scanned */
+ result->heap_tuples = 0;
+ /* and no index tuples were created (that is true) */
+ result->index_tuples = 0;
+
+ return result;
+}
+
+static void
+dibuildempty(Relation index)
+{
+ /* No need to build an init fork for a dummy index */
+}
+
+static bool
+diinsert(Relation index, Datum *values, bool *isnull,
+ ItemPointer ht_ctid, Relation heapRel,
+ IndexUniqueCheck checkUnique,
+ IndexInfo *indexInfo)
+{
+ /* nothing to do */
+ return false;
+}
+
+static IndexBulkDeleteResult *
+dibulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
+ IndexBulkDeleteCallback callback, void *callback_state)
+{
+ /*
+ * There is nothing to delete. Return NULL as there is nothing to
+ * pass to amvacuumcleanup.
+ */
+ return NULL;
+}
+
+static IndexBulkDeleteResult *
+divacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
+{
+ /* Index has not been modified, so returning NULL is fine */
+ return NULL;
+}
+
+static void
+dicostestimate(PlannerInfo *root, IndexPath *path, double loop_count,
+ Cost *indexStartupCost, Cost *indexTotalCost,
+ Selectivity *indexSelectivity, double *indexCorrelation,
+ double *indexPages)
+{
+ /* Tell planner to never use this index! */
+ *indexStartupCost = 1.0e10; /* AKA disable_cost */
+ *indexTotalCost = 1.0e10; /* AKA disable_cost */
+
+ /* Do not care about the rest */
+ *indexSelectivity = 1;
+ *indexCorrelation = 0;
+ *indexPages = 1;
+}
+
+static bytea *
+dioptions(Datum reloptions, bool validate)
+{
+ relopt_value *options;
+ int numoptions;
+ DummyIndexOptions *rdopts;
+
+ /* Parse the user-given reloptions */
+ options = parseRelOptions(reloptions, validate, di_relopt_kind, &numoptions);
+ rdopts = allocateReloptStruct(sizeof(DummyIndexOptions), options, numoptions);
+ fillRelOptions((void *) rdopts, sizeof(DummyIndexOptions), options, numoptions,
+ validate, di_relopt_tab, lengthof(di_relopt_tab));
+
+ return (bytea *) rdopts;
+}
+
+static bool
+divalidate(Oid opclassoid)
+{
+ /* Index is dummy so we are happy with any opclass */
+ return true;
+}
+
+static IndexScanDesc
+dibeginscan(Relation r, int nkeys, int norderbys)
+{
+ IndexScanDesc scan;
+
+ /* Let's pretend we are doing something */
+ scan = RelationGetIndexScan(r, nkeys, norderbys);
+ return scan;
+}
+
+static void
+direscan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
+ ScanKey orderbys, int norderbys)
+{
+ /* nothing to do */
+}
+
+static void
+diendscan(IndexScanDesc scan)
+{
+ /* nothing to do */
+}
+
+/*
+ * Dummy index handler function: return IndexAmRoutine with access method
+ * parameters and callbacks.
+ */
+Datum
+dihandler(PG_FUNCTION_ARGS)
+{
+ IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
+
+ amroutine->amstrategies = 0;
+ amroutine->amsupport = 1;
+ amroutine->amcanorder = false;
+ amroutine->amcanorderbyop = false;
+ amroutine->amcanbackward = false;
+ amroutine->amcanunique = false;
+ amroutine->amcanmulticol = false;
+ amroutine->amoptionalkey = false;
+ amroutine->amsearcharray = false;
+ amroutine->amsearchnulls = false;
+ amroutine->amstorage = false;
+ amroutine->amclusterable = false;
+ amroutine->ampredlocks = false;
+ amroutine->amcanparallel = false;
+ amroutine->amcaninclude = false;
+ amroutine->amkeytype = InvalidOid;
+
+ amroutine->ambuild = dibuild;
+ amroutine->ambuildempty = dibuildempty;
+ amroutine->aminsert = diinsert;
+ amroutine->ambulkdelete = dibulkdelete;
+ amroutine->amvacuumcleanup = divacuumcleanup;
+ amroutine->amcanreturn = NULL;
+ amroutine->amcostestimate = dicostestimate;
+ amroutine->amoptions = dioptions;
+ amroutine->amproperty = NULL;
+ amroutine->amvalidate = divalidate;
+ amroutine->ambeginscan = dibeginscan;
+ amroutine->amrescan = direscan;
+ amroutine->amgettuple = NULL;
+ amroutine->amgetbitmap = NULL;
+ amroutine->amendscan = diendscan;
+ amroutine->ammarkpos = NULL;
+ amroutine->amrestrpos = NULL;
+ amroutine->amestimateparallelscan = NULL;
+ amroutine->aminitparallelscan = NULL;
+ amroutine->amparallelrescan = NULL;
+
+ PG_RETURN_POINTER(amroutine);
+}
+
+void
+_PG_init(void)
+{
+ create_reloptions_table();
+}
diff --git a/src/test/modules/dummy_index_am/dummy_index_am.control b/src/test/modules/dummy_index_am/dummy_index_am.control
new file mode 100644
index 0000000000..7ea62a0c1c
--- /dev/null
+++ b/src/test/modules/dummy_index_am/dummy_index_am.control
@@ -0,0 +1,5 @@
+# dummy_index extension
+comment = 'dummy_index_am - minimized index access method'
+default_version = '1.0'
+module_pathname = '$libdir/dummy_index_am'
+relocatable = true
diff --git a/src/test/modules/dummy_index_am/expected/reloptions.out b/src/test/modules/dummy_index_am/expected/reloptions.out
new file mode 100644
index 0000000000..04a120a0e0
--- /dev/null
+++ b/src/test/modules/dummy_index_am/expected/reloptions.out
@@ -0,0 +1,77 @@
+-- Tests for relation options
+CREATE EXTENSION dummy_index_am;
+CREATE TABLE dummy_test_tab (i int4);
+-- Test with default values.
+CREATE INDEX dummy_test_idx ON dummy_test_tab
+ USING dummy_index_am (i);
+SELECT unnest(reloptions) FROM pg_class WHERE relname = 'dummy_test_idx';
+ unnest
+--------
+(0 rows)
+
+DROP INDEX dummy_test_idx;
+-- Test with full set of options.
+CREATE INDEX dummy_test_idx ON dummy_test_tab
+ USING dummy_index_am (i) WITH (
+ option_bool = false,
+ option_int = 5,
+ option_real = 3.1,
+ option_string_val = NULL,
+ option_string_null = 'val');
+NOTICE: new option value for string parameter null
+NOTICE: new option value for string parameter val
+SELECT unnest(reloptions) FROM pg_class WHERE relname = 'dummy_test_idx';
+ unnest
+------------------------
+ option_bool=false
+ option_int=5
+ option_real=3.1
+ option_string_val=null
+ option_string_null=val
+(5 rows)
+
+-- ALTER INDEX .. SET
+ALTER INDEX dummy_test_idx SET (option_int = 10);
+NOTICE: new option value for string parameter null
+NOTICE: new option value for string parameter val
+ALTER INDEX dummy_test_idx SET (option_bool = true);
+NOTICE: new option value for string parameter null
+NOTICE: new option value for string parameter val
+ALTER INDEX dummy_test_idx SET (option_real = 3.2);
+NOTICE: new option value for string parameter null
+NOTICE: new option value for string parameter val
+ALTER INDEX dummy_test_idx SET (option_string_val = 'val2');
+NOTICE: new option value for string parameter val
+NOTICE: new option value for string parameter val2
+ALTER INDEX dummy_test_idx SET (option_string_null = NULL);
+NOTICE: new option value for string parameter val2
+NOTICE: new option value for string parameter null
+SELECT unnest(reloptions) FROM pg_class WHERE relname = 'dummy_test_idx';
+ unnest
+-------------------------
+ option_int=10
+ option_bool=true
+ option_real=3.2
+ option_string_val=val2
+ option_string_null=null
+(5 rows)
+
+-- ALTER INDEX .. RESET
+ALTER INDEX dummy_test_idx RESET (option_int);
+NOTICE: new option value for string parameter val2
+NOTICE: new option value for string parameter null
+ALTER INDEX dummy_test_idx RESET (option_bool);
+NOTICE: new option value for string parameter val2
+NOTICE: new option value for string parameter null
+ALTER INDEX dummy_test_idx RESET (option_real);
+NOTICE: new option value for string parameter val2
+NOTICE: new option value for string parameter null
+ALTER INDEX dummy_test_idx RESET (option_string_val);
+NOTICE: new option value for string parameter null
+ALTER INDEX dummy_test_idx RESET (option_string_null);
+SELECT unnest(reloptions) FROM pg_class WHERE relname = 'dummy_test_idx';
+ unnest
+--------
+(0 rows)
+
+DROP INDEX dummy_test_idx;
diff --git a/src/test/modules/dummy_index_am/sql/reloptions.sql b/src/test/modules/dummy_index_am/sql/reloptions.sql
new file mode 100644
index 0000000000..10ca1a6d03
--- /dev/null
+++ b/src/test/modules/dummy_index_am/sql/reloptions.sql
@@ -0,0 +1,38 @@
+-- Tests for relation options
+CREATE EXTENSION dummy_index_am;
+
+CREATE TABLE dummy_test_tab (i int4);
+
+-- Test with default values.
+CREATE INDEX dummy_test_idx ON dummy_test_tab
+ USING dummy_index_am (i);
+SELECT unnest(reloptions) FROM pg_class WHERE relname = 'dummy_test_idx';
+DROP INDEX dummy_test_idx;
+
+-- Test with full set of options.
+CREATE INDEX dummy_test_idx ON dummy_test_tab
+ USING dummy_index_am (i) WITH (
+ option_bool = false,
+ option_int = 5,
+ option_real = 3.1,
+ option_string_val = NULL,
+ option_string_null = 'val');
+SELECT unnest(reloptions) FROM pg_class WHERE relname = 'dummy_test_idx';
+
+-- ALTER INDEX .. SET
+ALTER INDEX dummy_test_idx SET (option_int = 10);
+ALTER INDEX dummy_test_idx SET (option_bool = true);
+ALTER INDEX dummy_test_idx SET (option_real = 3.2);
+ALTER INDEX dummy_test_idx SET (option_string_val = 'val2');
+ALTER INDEX dummy_test_idx SET (option_string_null = NULL);
+SELECT unnest(reloptions) FROM pg_class WHERE relname = 'dummy_test_idx';
+
+-- ALTER INDEX .. RESET
+ALTER INDEX dummy_test_idx RESET (option_int);
+ALTER INDEX dummy_test_idx RESET (option_bool);
+ALTER INDEX dummy_test_idx RESET (option_real);
+ALTER INDEX dummy_test_idx RESET (option_string_val);
+ALTER INDEX dummy_test_idx RESET (option_string_null);
+SELECT unnest(reloptions) FROM pg_class WHERE relname = 'dummy_test_idx';
+
+DROP INDEX dummy_test_idx;
signature.asc
Description: PGP signature
