Feature: Add reloption support for table access method

Started by 吴昊over 2 years ago4 messages

wuhao@hashdata.cn

over 2 years ago

1 attachment(s)

Hi pg-hackers,

When I wrote an extension to implement a new storage by table access method. I found some issues
that the existing code has strong assumptions for heap tables now. Here are 3 issues that I currently have:

1. Index access method has a callback to handle reloptions, but table access method hasn't. We can't
add storage-specific parameters by `WITH` clause when creating a table.
2. Existing code strongly assumes that the data file of a table structures by a serial physical files named
in a hard coded rule: <relfilenode>[.<segno>]. It may only fit for heap like tables. A new storage may have its
owner structure on how the data files are organized. The problem happens when dropping a table.

3. The existing code also assumes that the data file consists of a series of fix-sized block. It may not
be desired by other storage. Is there any suggestions on this situation?

The rest of this mail is to talk about the first issue. It looks reasonable to add a similar callback in
struct TableAmRoutine, and parse reloptions by the callback. This patch is in the attachment file.

Another thing about reloption is that the current API passes a parameter `validate` to tell the parse
functioin to check and whether to raise an error. It doesn't have enough context when these reloptioins
are used:
1. CREATE TABLE ... WITH(...)
2. ALTER TABLE ... SET ...
3. ALTER TABLE ... RESET ...
The reason why the context matters is that some reloptions are disallowed to change after creating
the table, while some reloptions are allowed.

I wonder if this change makes sense for you.

The attached patch only supports callback for TAM-specific implementation, not include the change
about usage context.

Regards,
Hao Wu

Attachments:

reloptions.diffsapplication/octet-stream; charset=utf-8; name="=?UTF-8?B?cmVsb3B0aW9ucy5kaWZmcw==?="Download

commit 5c5c472e3fe40f8bb71e2727327bf3a070dba031
Author: Hao Wu <gfphoenix78@gmail.com>
Date:   Fri May 5 09:13:22 2023 +0800

    Add reloption support for table

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 41bf950a4a..c7f130acd3 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -973,7 +973,7 @@ brinvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
  * reloptions processor for BRIN indexes
  */
 bytea *
-brinoptions(Datum reloptions, bool validate)
+brinoptions(Datum reloptions, char relkind, bool validate)
 {
 	static const relopt_parse_elt tab[] = {
 		{"pages_per_range", RELOPT_TYPE_INT, offsetof(BrinOptions, pagesPerRange)},
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 90cb3951fc..eed9521827 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1401,7 +1401,7 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
 		case RELKIND_RELATION:
 		case RELKIND_TOASTVALUE:
 		case RELKIND_MATVIEW:
-			options = heap_reloptions(classForm->relkind, datum, false);
+			options = table_reloptions(amoptions, datum, classForm->relkind, false);
 			break;
 		case RELKIND_PARTITIONED_TABLE:
 			options = partitioned_table_reloptions(datum, false);
@@ -1411,7 +1411,7 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
 			break;
 		case RELKIND_INDEX:
 		case RELKIND_PARTITIONED_INDEX:
-			options = index_reloptions(amoptions, datum, false);
+			options = index_reloptions(amoptions, datum, classForm->relkind, false);
 			break;
 		case RELKIND_FOREIGN_TABLE:
 			options = NULL;
@@ -2014,37 +2014,43 @@ view_reloptions(Datum reloptions, bool validate)
 									  tab, lengthof(tab));
 }
 
-/*
- * Parse options for heaps, views and toast tables.
- */
 bytea *
-heap_reloptions(char relkind, Datum reloptions, bool validate)
+table_reloptions(amoptions_function amoptions, Datum reloptions, char relkind, bool validate)
+{
+	Assert(relkind == RELKIND_RELATION ||
+			relkind == RELKIND_TOASTVALUE ||
+			relkind == RELKIND_MATVIEW);
+	if (amoptions == NULL)
+	{
+		if (PointerIsValid(DatumGetPointer(reloptions)))
+			elog(ERROR, "table access method doesn't supported reloptions");
+		return NULL;
+	}
+	return amoptions(reloptions, relkind, validate);
+}
+
+bytea *
+table_reloptions_am(Oid accessMethodId, Datum reloptions, char relkind, bool validate)
 {
-	StdRdOptions *rdopts;
+	const TableAmRoutine *tam;
 
-	switch (relkind)
+	Assert(relkind == RELKIND_RELATION ||
+			relkind == RELKIND_TOASTVALUE ||
+			relkind == RELKIND_MATVIEW);
+
+	switch(accessMethodId)
 	{
-		case RELKIND_TOASTVALUE:
-			rdopts = (StdRdOptions *)
-				default_reloptions(reloptions, validate, RELOPT_KIND_TOAST);
-			if (rdopts != NULL)
-			{
-				/* adjust default-only parameters for TOAST relations */
-				rdopts->fillfactor = 100;
-				rdopts->autovacuum.analyze_threshold = -1;
-				rdopts->autovacuum.analyze_scale_factor = -1;
-			}
-			return (bytea *) rdopts;
-		case RELKIND_RELATION:
-		case RELKIND_MATVIEW:
-			return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
+		/* built-in table access method put here to fetch TAM fast */
+		case HEAP_TABLE_AM_OID:
+			tam = GetHeapamTableAmRoutine();
+			break;
 		default:
-			/* other relkinds are not supported */
-			return NULL;
+			tam = GetTableAmRoutineByAmId(accessMethodId);
+			break;
 	}
+	return table_reloptions(tam->amoptions, reloptions, relkind, validate);
 }
 
-
 /*
  * Parse options for indexes.
  *
@@ -2053,7 +2059,7 @@ heap_reloptions(char relkind, Datum reloptions, bool validate)
  *	validate	error flag
  */
 bytea *
-index_reloptions(amoptions_function amoptions, Datum reloptions, bool validate)
+index_reloptions(amoptions_function amoptions, Datum reloptions, char relkind, bool validate)
 {
 	Assert(amoptions != NULL);
 
@@ -2061,7 +2067,7 @@ index_reloptions(amoptions_function amoptions, Datum reloptions, bool validate)
 	if (!PointerIsValid(DatumGetPointer(reloptions)))
 		return NULL;
 
-	return amoptions(reloptions, validate);
+	return amoptions(reloptions, relkind, validate);
 }
 
 /*
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 437f24753c..7cdaf3c6f9 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -598,7 +598,7 @@ ginExtractEntries(GinState *ginstate, OffsetNumber attnum,
 }
 
 bytea *
-ginoptions(Datum reloptions, bool validate)
+ginoptions(Datum reloptions, char relkind, bool validate)
 {
 	static const relopt_parse_elt tab[] = {
 		{"fastupdate", RELOPT_TYPE_BOOL, offsetof(GinOptions, useFastUpdate)},
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index f9f51152b8..cc21368c45 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -909,7 +909,7 @@ gistPageRecyclable(Page page)
 }
 
 bytea *
-gistoptions(Datum reloptions, bool validate)
+gistoptions(Datum reloptions, char relkind, bool validate)
 {
 	static const relopt_parse_elt tab[] = {
 		{"fillfactor", RELOPT_TYPE_INT, offsetof(GiSTOptions, fillfactor)},
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index 88089ce02b..638cb901c4 100644
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -273,7 +273,7 @@ _hash_checkpage(Relation rel, Buffer buf, int flags)
 }
 
 bytea *
-hashoptions(Datum reloptions, bool validate)
+hashoptions(Datum reloptions, char relkind, bool validate)
 {
 	static const relopt_parse_elt tab[] = {
 		{"fillfactor", RELOPT_TYPE_INT, offsetof(HashOptions, fillfactor)},
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index cbb35aa73d..d0adc674e2 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -23,6 +23,7 @@
 #include "access/heapam.h"
 #include "access/heaptoast.h"
 #include "access/multixact.h"
+#include "access/reloptions.h"
 #include "access/rewriteheap.h"
 #include "access/syncscan.h"
 #include "access/tableam.h"
@@ -2539,6 +2540,36 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
 	}
 }
 
+/*
+ * Parse options for heaps, views and toast tables.
+ */
+static bytea *
+heapam_amoptions(Datum reloptions, char relkind, bool validate)
+{
+	StdRdOptions *rdopts;
+
+	switch (relkind)
+	{
+		case RELKIND_TOASTVALUE:
+			rdopts = (StdRdOptions *)
+				default_reloptions(reloptions, validate, RELOPT_KIND_TOAST);
+			if (rdopts != NULL)
+			{
+				/* adjust default-only parameters for TOAST relations */
+				rdopts->fillfactor = 100;
+				rdopts->autovacuum.analyze_threshold = -1;
+				rdopts->autovacuum.analyze_scale_factor = -1;
+			}
+			return (bytea *) rdopts;
+		case RELKIND_RELATION:
+		case RELKIND_MATVIEW:
+			return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
+		default:
+			Assert(false);
+			return NULL;
+	}
+}
+
 
 /* ------------------------------------------------------------------------
  * Definition of the heap table access method.
@@ -2601,7 +2632,10 @@ static const TableAmRoutine heapam_methods = {
 	.scan_bitmap_next_block = heapam_scan_bitmap_next_block,
 	.scan_bitmap_next_tuple = heapam_scan_bitmap_next_tuple,
 	.scan_sample_next_block = heapam_scan_sample_next_block,
-	.scan_sample_next_tuple = heapam_scan_sample_next_tuple
+	.scan_sample_next_tuple = heapam_scan_sample_next_tuple,
+
+	.amoptions	= heapam_amoptions,
+
 };
 
 
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index 05abf36032..5ba16b6bb6 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -2109,7 +2109,7 @@ BTreeShmemInit(void)
 }
 
 bytea *
-btoptions(Datum reloptions, bool validate)
+btoptions(Datum reloptions, char relkind, bool validate)
 {
 	static const relopt_parse_elt tab[] = {
 		{"fillfactor", RELOPT_TYPE_INT, offsetof(BTOptions, fillfactor)},
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 190e4f76a9..ecf8af32ac 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -728,7 +728,7 @@ SpGistInitMetapage(Page page)
  * reloptions processing for SPGiST
  */
 bytea *
-spgoptions(Datum reloptions, bool validate)
+spgoptions(Datum reloptions, char relkind, bool validate)
 {
 	static const relopt_parse_elt tab[] = {
 		{"fillfactor", RELOPT_TYPE_INT, offsetof(SpGistOptions, fillfactor)},
diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index d7798b6afb..0392ba7842 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -107,6 +107,36 @@ GetTableAmRoutine(Oid amhandler)
 	return routine;
 }
 
+/*
+ * GetTableAmRoutineByAmId - look up the handler of the table access method
+ * with the given OID, and get its TableAmRoutine struct.
+ *
+ * If the given OID isn't a valid index access method, throws error.
+ */
+const TableAmRoutine *
+GetTableAmRoutineByAmId(Oid amoid)
+{
+	HeapTuple	tuple;
+	Form_pg_am  amform;
+	regproc     amhandler;
+
+	tuple = SearchSysCache1(AMOID, ObjectIdGetDatum(amoid));
+	if (!HeapTupleIsValid(tuple))
+		ereport(ERROR, (errmsg("cache lookup failed for access method %u", amoid)));
+
+	amform = (Form_pg_am) GETSTRUCT(tuple);
+	if (amform->amtype != AMTYPE_TABLE)
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("access method \"%s\" is not of type TABLE",
+						NameStr(amform->amname))));
+
+	amhandler = amform->amhandler;
+	ReleaseSysCache(tuple);
+
+	return GetTableAmRoutine(amhandler);
+}
+
 /* check_hook: validate new default_table_access_method */
 bool
 check_default_table_access_method(char **newval, void **extra, GucSource source)
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index ab12b0b9de..291a556c71 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -15,6 +15,7 @@
 #include "postgres.h"
 
 #include "access/heapam.h"
+#include "access/reloptions.h"
 #include "access/toast_compression.h"
 #include "access/xact.h"
 #include "catalog/binary_upgrade.h"
@@ -148,6 +149,11 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
 	ObjectAddress baseobject,
 				toastobject;
 
+	Assert(rel->rd_tableam || rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
+	if (rel->rd_tableam)
+		(void) table_reloptions_am(table_relation_toast_am(rel), reloptions,
+									RELKIND_TOASTVALUE, true);
+
 	/*
 	 * Is it already toasted?
 	 */
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index e91920ca14..9183f7258e 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -130,8 +130,6 @@ create_ctas_internal(List *attrList, IntoClause *into)
 										validnsps,
 										true, false);
 
-	(void) heap_reloptions(RELKIND_TOASTVALUE, toast_options, true);
-
 	NewRelationCreateToastTable(intoRelationAddr.objectId, toast_options);
 
 	/* Create the "view" part of a materialized view. */
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index e6ee99e51f..5b8c4b1358 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -887,7 +887,7 @@ DefineIndex(Oid relationId,
 	reloptions = transformRelOptions((Datum) 0, stmt->options,
 									 NULL, NULL, false, false);
 
-	(void) index_reloptions(amoptions, reloptions, true);
+	(void) index_reloptions(amoptions, reloptions, RELKIND_INDEX, true);
 
 	/*
 	 * Prepare arguments for index_create, primarily an IndexInfo structure.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c7a8a689b7..e874ba9778 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -822,6 +822,26 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	if (!OidIsValid(ownerId))
 		ownerId = GetUserId();
 
+	/*
+	 * If the statement hasn't specified an access method, but we're defining
+	 * a type of relation that needs one, use the default.
+	 */
+	if (stmt->accessMethod != NULL)
+	{
+		accessMethod = stmt->accessMethod;
+
+		if (partitioned)
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("specifying a table access method is not supported on a partitioned table")));
+	}
+	else if (RELKIND_HAS_TABLE_AM(relkind))
+		accessMethod = default_table_access_method;
+
+	/* look up the access method, verify it is for a table */
+	if (accessMethod != NULL)
+		accessMethodId = get_table_am_oid(accessMethod, false);
+
 	/*
 	 * Parse and validate reloptions, if any.
 	 */
@@ -836,8 +856,13 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		case RELKIND_PARTITIONED_TABLE:
 			(void) partitioned_table_reloptions(reloptions, true);
 			break;
+		case RELKIND_RELATION:
+		case RELKIND_MATVIEW:
+		case RELKIND_TOASTVALUE:
+			(void) table_reloptions_am(accessMethodId, reloptions, relkind, true);
+			break;
 		default:
-			(void) heap_reloptions(relkind, reloptions, true);
+			break;
 	}
 
 	if (stmt->ofTypename)
@@ -941,26 +966,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 			attr->attstorage = GetAttributeStorage(attr->atttypid, colDef->storage_name);
 	}
 
-	/*
-	 * If the statement hasn't specified an access method, but we're defining
-	 * a type of relation that needs one, use the default.
-	 */
-	if (stmt->accessMethod != NULL)
-	{
-		accessMethod = stmt->accessMethod;
-
-		if (partitioned)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("specifying a table access method is not supported on a partitioned table")));
-	}
-	else if (RELKIND_HAS_TABLE_AM(relkind))
-		accessMethod = default_table_access_method;
-
-	/* look up the access method, verify it is for a table */
-	if (accessMethod != NULL)
-		accessMethodId = get_table_am_oid(accessMethod, false);
-
 	/*
 	 * Create the relation.  Inherited defaults and constraints are passed in
 	 * for immediate handling --- since they don't need parsing, they can be
@@ -14333,7 +14338,7 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
 		case RELKIND_RELATION:
 		case RELKIND_TOASTVALUE:
 		case RELKIND_MATVIEW:
-			(void) heap_reloptions(rel->rd_rel->relkind, newOptions, true);
+			(void) table_reloptions(rel->rd_tableam->amoptions, newOptions, rel->rd_rel->relkind, true);
 			break;
 		case RELKIND_PARTITIONED_TABLE:
 			(void) partitioned_table_reloptions(newOptions, true);
@@ -14343,7 +14348,7 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
 			break;
 		case RELKIND_INDEX:
 		case RELKIND_PARTITIONED_INDEX:
-			(void) index_reloptions(rel->rd_indam->amoptions, newOptions, true);
+			(void) index_reloptions(rel->rd_indam->amoptions, newOptions, rel->rd_rel->relkind, true);
 			break;
 		default:
 			ereport(ERROR,
@@ -14446,7 +14451,7 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
 										 defList, "toast", validnsps, false,
 										 operation == AT_ResetRelOptions);
 
-		(void) heap_reloptions(RELKIND_TOASTVALUE, newOptions, true);
+		(void) table_reloptions(toastrel->rd_tableam->amoptions, newOptions, RELKIND_TOASTVALUE, true);
 
 		memset(repl_val, 0, sizeof(repl_val));
 		memset(repl_null, false, sizeof(repl_null));
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 30b51bf4d3..35989558ad 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1195,9 +1195,6 @@ ProcessUtilitySlow(ParseState *pstate,
 																validnsps,
 																true,
 																false);
-							(void) heap_reloptions(RELKIND_TOASTVALUE,
-												   toast_options,
-												   true);
 
 							NewRelationCreateToastTable(address.objectId,
 														toast_options);
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 40140de958..20e80c24e7 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -474,8 +474,10 @@ RelationParseRelOptions(Relation relation, HeapTuple tuple)
 	{
 		case RELKIND_RELATION:
 		case RELKIND_TOASTVALUE:
-		case RELKIND_VIEW:
 		case RELKIND_MATVIEW:
+			amoptsfn = relation->rd_tableam->amoptions;
+			break;
+		case RELKIND_VIEW:
 		case RELKIND_PARTITIONED_TABLE:
 			amoptsfn = NULL;
 			break;
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 281039ef67..389a74fbc3 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -138,6 +138,7 @@ typedef void (*amcostestimate_function) (struct PlannerInfo *root,
 
 /* parse index reloptions */
 typedef bytea *(*amoptions_function) (Datum reloptions,
+									  char relkind,
 									  bool validate);
 
 /* report AM, index, or index column property */
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 97ddc925b2..1caf7c73c7 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -107,7 +107,7 @@ extern IndexBulkDeleteResult *brinbulkdelete(IndexVacuumInfo *info,
 											 void *callback_state);
 extern IndexBulkDeleteResult *brinvacuumcleanup(IndexVacuumInfo *info,
 												IndexBulkDeleteResult *stats);
-extern bytea *brinoptions(Datum reloptions, bool validate);
+extern bytea *brinoptions(Datum reloptions, char relkind, bool validate);
 
 /* brin_validate.c */
 extern bool brinvalidate(Oid opclassoid);
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 6da64928b6..bb68423abe 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -89,7 +89,7 @@ typedef struct GinState
 
 
 /* ginutil.c */
-extern bytea *ginoptions(Datum reloptions, bool validate);
+extern bytea *ginoptions(Datum reloptions, char relkind, bool validate);
 extern void initGinState(GinState *state, Relation index);
 extern Buffer GinNewBuffer(Relation index);
 extern void GinInitBuffer(Buffer b, uint32 f);
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index ee275650bd..658da14b33 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -479,7 +479,7 @@ extern void gistadjustmembers(Oid opfamilyoid,
 #define GIST_MIN_FILLFACTOR			10
 #define GIST_DEFAULT_FILLFACTOR		90
 
-extern bytea *gistoptions(Datum reloptions, bool validate);
+extern bytea *gistoptions(Datum reloptions, char relkind, bool validate);
 extern bool gistproperty(Oid index_oid, int attno,
 						 IndexAMProperty prop, const char *propname,
 						 bool *res, bool *isnull);
diff --git a/src/include/access/hash.h b/src/include/access/hash.h
index 9e035270a1..d71441ff25 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -380,7 +380,7 @@ extern IndexBulkDeleteResult *hashbulkdelete(IndexVacuumInfo *info,
 											 void *callback_state);
 extern IndexBulkDeleteResult *hashvacuumcleanup(IndexVacuumInfo *info,
 												IndexBulkDeleteResult *stats);
-extern bytea *hashoptions(Datum reloptions, bool validate);
+extern bytea *hashoptions(Datum reloptions, char relkind, bool validate);
 extern bool hashvalidate(Oid opclassoid);
 extern void hashadjustmembers(Oid opfamilyoid,
 							  Oid opclassoid,
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index d684786095..8547c6a806 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -1260,7 +1260,7 @@ extern void _bt_end_vacuum(Relation rel);
 extern void _bt_end_vacuum_callback(int code, Datum arg);
 extern Size BTreeShmemSize(void);
 extern void BTreeShmemInit(void);
-extern bytea *btoptions(Datum reloptions, bool validate);
+extern bytea *btoptions(Datum reloptions, char relkind, bool validate);
 extern bool btproperty(Oid index_oid, int attno,
 					   IndexAMProperty prop, const char *propname,
 					   bool *res, bool *isnull);
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 1d5bfa62ff..9ba57cb5d5 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -235,11 +235,15 @@ extern void *build_local_reloptions(local_relopts *relopts, Datum options,
 
 extern bytea *default_reloptions(Datum reloptions, bool validate,
 								 relopt_kind kind);
-extern bytea *heap_reloptions(char relkind, Datum reloptions, bool validate);
+
+extern bytea *table_reloptions(amoptions_function amoptions, Datum reloptions,
+								  char relkind, bool validate);
+extern bytea *table_reloptions_am(Oid accessMethodId, Datum reloptions,
+								  char relkind, bool validate);
 extern bytea *view_reloptions(Datum reloptions, bool validate);
 extern bytea *partitioned_table_reloptions(Datum reloptions, bool validate);
 extern bytea *index_reloptions(amoptions_function amoptions, Datum reloptions,
-							   bool validate);
+							   char relkind, bool validate);
 extern bytea *attribute_reloptions(Datum reloptions, bool validate);
 extern bytea *tablespace_reloptions(Datum reloptions, bool validate);
 extern LOCKMODE AlterTableGetRelOptionsLockLevel(List *defList);
diff --git a/src/include/access/spgist.h b/src/include/access/spgist.h
index fe31d32dbe..9ceba400c8 100644
--- a/src/include/access/spgist.h
+++ b/src/include/access/spgist.h
@@ -190,7 +190,7 @@ typedef struct spgLeafConsistentOut
 
 
 /* spgutils.c */
-extern bytea *spgoptions(Datum reloptions, bool validate);
+extern bytea *spgoptions(Datum reloptions, char relkind, bool validate);
 
 /* spginsert.c */
 extern IndexBuildResult *spgbuild(Relation heap, Relation index,
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b19d50ecc2..681d872cb8 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -866,6 +866,11 @@ typedef struct TableAmRoutine
 										   struct SampleScanState *scanstate,
 										   TupleTableSlot *slot);
 
+	/*
+	 * This callback is used to parse reloptions for relation/matview/toast.
+	 */
+	bytea       *(*amoptions)(Datum reloptions, char relkind, bool validate);
+
 } TableAmRoutine;
 
 
@@ -2095,6 +2100,7 @@ extern void table_block_relation_estimate_size(Relation rel,
  */
 
 extern const TableAmRoutine *GetTableAmRoutine(Oid amhandler);
+extern const TableAmRoutine *GetTableAmRoutineByAmId(Oid amoid);
 extern const TableAmRoutine *GetHeapamTableAmRoutine(void);
 
 #endif							/* TABLEAM_H */

Andres Freund

andres@anarazel.de

over 2 years ago

In reply to: 吴昊 (#1)

Re: Feature: Add reloption support for table access method

Hi,

On 2023-05-05 16:44:39 +0800, 吴昊 wrote:

When I wrote an extension to implement a new storage by table access method. I found some issues
that the existing code has strong assumptions for heap tables now. Here are 3 issues that I currently have:

1. Index access method has a callback to handle reloptions, but table access method hasn't. We can't
add storage-specific parameters by `WITH` clause when creating a table.

Makes sense to add that.

2. Existing code strongly assumes that the data file of a table structures by a serial physical files named
in a hard coded rule: <relfilenode>[.<segno>]. It may only fit for heap like tables. A new storage may have its
owner structure on how the data files are organized. The problem happens when dropping a table.

I agree that it's not great, but I don't think it's particularly easy to fix
(because things like transactional DDL require fairly tight integration). Most
of the time it should be possible can also work around the limitations though.

3. The existing code also assumes that the data file consists of a series of fix-sized block. It may not
be desired by other storage. Is there any suggestions on this situation?

That's a requirement of using the buffer manager, but if you don't want to
rely on that, you can use a different pattern. There's some limitations
(format of TIDs, most prominently), but you should be able to deal with that.

I don't think it would make sense to support other block sizes in the buffer
manager.

The rest of this mail is to talk about the first issue. It looks reasonable to add a similar callback in
struct TableAmRoutine, and parse reloptions by the callback. This patch is in the attachment file.

Why did you add relkind to the callbacks? The callbacks are specific to a
certain relkind, so I don't think that makes sense.

I don't think we really need GetTableAmRoutineByAmId() that raises nice
errors etc - as the AM has already been converted to an oid, we shouldn't need
to recheck?

+bytea *
+table_reloptions_am(Oid accessMethodId, Datum reloptions, char relkind, bool validate)
{
...

+		/* built-in table access method put here to fetch TAM fast */
+		case HEAP_TABLE_AM_OID:
+			tam = GetHeapamTableAmRoutine();
+			break;
default:
-			/* other relkinds are not supported */
-			return NULL;
+			tam = GetTableAmRoutineByAmId(accessMethodId);
+			break;

Why do we need this fastpath? This shouldn't be something called at a
meaningful frequency?

}
+ return table_reloptions(tam->amoptions, reloptions, relkind, validate);
}

I'd just pass the tam, instead of an individual function.

@@ -866,6 +866,11 @@ typedef struct TableAmRoutine
struct SampleScanState *scanstate,
TupleTableSlot *slot);
+	/*
+	 * This callback is used to parse reloptions for relation/matview/toast.
+	 */
+	bytea       *(*amoptions)(Datum reloptions, char relkind, bool validate);
+
} TableAmRoutine;

Did you mean table instead of relation in the comment?

Another thing about reloption is that the current API passes a parameter `validate` to tell the parse
functioin to check and whether to raise an error. It doesn't have enough context when these reloptioins
are used:
1. CREATE TABLE ... WITH(...)
2. ALTER TABLE ... SET ...
3. ALTER TABLE ... RESET ...
The reason why the context matters is that some reloptions are disallowed to change after creating
the table, while some reloptions are allowed.

What kind of reloption are you thinking of here?

Greetings,

Andres Freund

吴昊

wuhao@hashdata.cn

over 2 years ago

In reply to: Andres Freund (#2)

Re:Re: Feature: Add reloption support for table access method

The rest of this mail is to talk about the first issue. It looks reasonable to add a similar callback in
struct TableAmRoutine, and parse reloptions by the callback. This patch is in the attachment file.

Why did you add relkind to the callbacks? The callbacks are specific to a
certain relkind, so I don't think that makes sense.

An implementation of table access method may be used for table/toast/matview, different relkinds
may define different set of reloptions. If they have the same reloption set, just ignore the relkind
parameter.

I don't think we really need GetTableAmRoutineByAmId() that raises nice
errors etc - as the AM has already been converted to an oid, we shouldn't need
to recheck?

When defining a relation, the function knows only the access method name, not the AM routine struct.
The AmRoutine needs to be looked-up by its access method name or oid. The existing function
calculates AmRoutine by the handler oid, not by am oid.

+bytea *
+table_reloptions_am(Oid accessMethodId, Datum reloptions, char relkind, bool validate)
{
...

+		/* built-in table access method put here to fetch TAM fast */
+		case HEAP_TABLE_AM_OID:
+			tam = GetHeapamTableAmRoutine();
+			break;
default:
-			/* other relkinds are not supported */
-			return NULL;
+			tam = GetTableAmRoutineByAmId(accessMethodId);
+			break;

Why do we need this fastpath? This shouldn't be something called at a
meaningful frequency?

OK, it make sense.

}
+ return table_reloptions(tam->amoptions, reloptions, relkind, validate);
}

I'd just pass the tam, instead of an individual function.

It's aligned to index_reloptions, and the function extractRelOptions also uses
an individual function other a pointer to AmRoutine struct.

Did you mean table instead of relation in the comment?

Yes, the comment doesn't update.

Another thing about reloption is that the current API passes a parameter `validate` to tell the parse
functioin to check and whether to raise an error. It doesn't have enough context when these reloptioins
are used:
1. CREATE TABLE ... WITH(...)
2. ALTER TABLE ... SET ...
3. ALTER TABLE ... RESET ...
The reason why the context matters is that some reloptions are disallowed to change after creating
the table, while some reloptions are allowed.

What kind of reloption are you thinking of here?

DRAFT: The amoptions in TableAmRoutine may change to
```
bytea *(*amoptions)(Datum reloptions, char relkind, ReloptionContext context);
enum ReloptionContext {
RELOPTION_INIT, // CREATE TABLE ... WITH(...)
RELOPTION_SET, // ALTER TABLE ... SET ...
RELOPTION_RESET, // ALTER TABLE ... RESET ...
RELOPTION_EXTRACT, // build reloptions from pg_class.reloptions
}
```
The callback always validates the reloptions if the context is not RELOPTION_EXTRACT.
If the TAM disallows to update some reloptions, it may throw an error when the context is
one of (RELOPTION_SET, RELOPTION_RESET).
The similar callback `amoptions` in IndexRoutine also applies this change.
BTW, it's hard to find an appropriate header file to define the ReloptionContext, which is
used by index/table AM.

Regards,
Hao Wu

Jelte Fennema

postgres@jeltef.nl

over 2 years ago

In reply to: Andres Freund (#2)

Re: Feature: Add reloption support for table access method

I'm definitely in favor of this general idea of supporting custom rel
options, we could use that for Citus its columnar TableAM. There have
been at least two other discussions on this topic:
1. /messages/by-id/CAFF0-CG4KZHdtYHMsonWiXNzj16gWZpduXAn8yF7pDDub+GQMg@mail.gmail.com
2. /messages/by-id/429fb58fa3218221bb17c7bf9e70e1aa6cfc6b5d.camel@j-davis.com

I haven't looked at the patch in detail, but it's probably good to
check what part of those previous discussions apply to it. And if
there's any ideas from those previous attempts that this patch could
borrow.

Show quoted text

On Tue, 9 May 2023 at 22:59, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2023-05-05 16:44:39 +0800, 吴昊 wrote:

When I wrote an extension to implement a new storage by table access method. I found some issues
that the existing code has strong assumptions for heap tables now. Here are 3 issues that I currently have:

1. Index access method has a callback to handle reloptions, but table access method hasn't. We can't
add storage-specific parameters by `WITH` clause when creating a table.

Makes sense to add that.

2. Existing code strongly assumes that the data file of a table structures by a serial physical files named
in a hard coded rule: <relfilenode>[.<segno>]. It may only fit for heap like tables. A new storage may have its
owner structure on how the data files are organized. The problem happens when dropping a table.

I agree that it's not great, but I don't think it's particularly easy to fix
(because things like transactional DDL require fairly tight integration). Most
of the time it should be possible can also work around the limitations though.

3. The existing code also assumes that the data file consists of a series of fix-sized block. It may not
be desired by other storage. Is there any suggestions on this situation?

That's a requirement of using the buffer manager, but if you don't want to
rely on that, you can use a different pattern. There's some limitations
(format of TIDs, most prominently), but you should be able to deal with that.

I don't think it would make sense to support other block sizes in the buffer
manager.

The rest of this mail is to talk about the first issue. It looks reasonable to add a similar callback in
struct TableAmRoutine, and parse reloptions by the callback. This patch is in the attachment file.

Why did you add relkind to the callbacks? The callbacks are specific to a
certain relkind, so I don't think that makes sense.

I don't think we really need GetTableAmRoutineByAmId() that raises nice
errors etc - as the AM has already been converted to an oid, we shouldn't need
to recheck?
+bytea *
+table_reloptions_am(Oid accessMethodId, Datum reloptions, char relkind, bool validate)
{
...
+             /* built-in table access method put here to fetch TAM fast */
+             case HEAP_TABLE_AM_OID:
+                     tam = GetHeapamTableAmRoutine();
+                     break;
default:
-                     /* other relkinds are not supported */
-                     return NULL;
+                     tam = GetTableAmRoutineByAmId(accessMethodId);
+                     break;
Why do we need this fastpath? This shouldn't be something called at a
meaningful frequency?

}
+ return table_reloptions(tam->amoptions, reloptions, relkind, validate);
}

I'd just pass the tam, instead of an individual function.
@@ -866,6 +866,11 @@ typedef struct TableAmRoutine
struct SampleScanState *scanstate,
TupleTableSlot *slot);
+     /*
+      * This callback is used to parse reloptions for relation/matview/toast.
+      */
+     bytea       *(*amoptions)(Datum reloptions, char relkind, bool validate);
+
} TableAmRoutine;
Did you mean table instead of relation in the comment?

Another thing about reloption is that the current API passes a parameter `validate` to tell the parse
functioin to check and whether to raise an error. It doesn't have enough context when these reloptioins
are used:
1. CREATE TABLE ... WITH(...)
2. ALTER TABLE ... SET ...
3. ALTER TABLE ... RESET ...
The reason why the context matters is that some reloptions are disallowed to change after creating
the table, while some reloptions are allowed.

What kind of reloption are you thinking of here?

Greetings,

Andres Freund