Date: Sun, 28 Feb 2021 16:25:30 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: pgsql-hackers@postgresql.org
Cc: Jacob Champion <pchampion@vmware.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Ashwin Agrawal <aagrawal@pivotal.io>,
	Ashutosh Sharma <ashu.coek88@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Robert Haas <robertmhaas@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Dilip Kumar <dilipbalaut@gmail.com>
Subject: alter table set TABLE ACCESS METHOD
Message-ID: <20210228222530.GD20769@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="utPK4TBebyzZxMrE"
Content-Disposition: inline
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--utPK4TBebyzZxMrE
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

I thought this was a good idea, but didn't hear back when I raised it before.
I was motivated to finally look into it by Dilip's toast compression patch,
which also (can do) a table rewrite when changing a column's toast compression.

I called this "set TABLE access method" rather than just "set access method"
for the reasons given on the LIKE thread:
https://www.postgresql.org/message-id/20210119210331.GN8560@telsasoft.com

I've tested this with zedstore, but the lack of 2nd, in-core table AM limits
testing possibilities.  It also limits at least my own ability to reason about
the AM API.  For example, I was surprised to hear that toast is a concept
that's intended to be applied to AMs other than heap.
https://www.postgresql.org/message-id/flat/CA%2BTgmoYTuT4sRtviMLOOO%2B79VnDCpCNyy9rK6UZFb7KEAVt21w%40mail.gmail.com

I plan to add to CF - it seems like a minor addition, but may not be for v14.

https://www.postgresql.org/message-id/20190818193533.GL11185@telsasoft.com
On Sun, Aug 18, 2019 at 02:35:33PM -0500, Justin Pryzby wrote:
>  . What do you think about pg_restore --no-tableam; similar to
>    --no-tablespaces, it would allow restoring a table to a different AM:
>    PGOPTIONS='-c default_table_access_method=zedstore' pg_restore --no-tableam ./pg_dump.dat -d postgres
>    Otherwise, the dump says "SET default_table_access_method=heap", which
>    overrides any value from PGOPTIONS and precludes restoring to new AM.
...
>  . it'd be nice if there was an ALTER TABLE SET ACCESS METHOD, to allow
>    migrating data.  Otherwise I think the alternative is:
>       begin; lock t;
>       CREATE TABLE new_t LIKE (t INCLUDING ALL) USING (zedstore);
>       INSERT INTO new_t SELECT * FROM t;
>       for index; do CREATE INDEX...; done
>       DROP t; RENAME new_t (and all its indices). attach/inherit, etc.
>       commit;
>
>  . Speaking of which, I think LIKE needs a new option for ACCESS METHOD, which
>    is otherwise lost.

--utPK4TBebyzZxMrE
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-ALTER-TABLE-SET-ACCESS-METHOD.patch"

From b13dac0ffbe108c45c97095b69218e76bf02799f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 27 Feb 2021 20:40:54 -0600
Subject: [PATCH] ALTER TABLE SET ACCESS METHOD

This does a tuple-level rewrite to the new AM
---
 src/backend/catalog/heap.c              |  1 +
 src/backend/commands/cluster.c          | 17 +++--
 src/backend/commands/matview.c          |  1 +
 src/backend/commands/tablecmds.c        | 86 ++++++++++++++++++++++---
 src/backend/parser/gram.y               | 10 +++
 src/include/commands/cluster.h          |  2 +-
 src/include/commands/event_trigger.h    |  1 +
 src/include/nodes/parsenodes.h          |  1 +
 src/test/regress/expected/create_am.out | 10 +++
 src/test/regress/sql/create_am.sql      |  5 ++
 10 files changed, 119 insertions(+), 15 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 9abc4a1f55..9d5a9240e9 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1117,6 +1117,7 @@ AddNewRelationType(const char *typeName,
  *	reltypeid: OID to assign to rel's rowtype, or InvalidOid to select one
  *	reloftypeid: if a typed table, OID of underlying type; else InvalidOid
  *	ownerid: OID of new rel's owner
+ *	accessmtd: OID of new rel's access method
  *	tupdesc: tuple descriptor (source of column definitions)
  *	cooked_constraints: list of precooked check constraints and defaults
  *	relkind: relkind for new rel
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 096a06f7b3..426a5b83ba 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -577,6 +577,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
 {
 	Oid			tableOid = RelationGetRelid(OldHeap);
 	Oid			tableSpace = OldHeap->rd_rel->reltablespace;
+	Oid			accessMethod = OldHeap->rd_rel->relam;
 	Oid			OIDNewHeap;
 	char		relpersistence;
 	bool		is_system_catalog;
@@ -598,6 +599,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
 	/* Create the transient table that will receive the re-ordered data */
 	OIDNewHeap = make_new_heap(tableOid, tableSpace,
 							   relpersistence,
+							   accessMethod,
 							   AccessExclusiveLock);
 
 	/* Copy the heap data into the new table in the desired order */
@@ -619,15 +621,15 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
  * Create the transient table that will be filled with new data during
  * CLUSTER, ALTER TABLE, and similar operations.  The transient table
  * duplicates the logical structure of the OldHeap, but is placed in
- * NewTableSpace which might be different from OldHeap's.  Also, it's built
- * with the specified persistence, which might differ from the original's.
+ * NewTableSpace/accessMethod/persistence, which might differ from those
+ * of the OldHeap.
  *
  * After this, the caller should load the new heap with transferred/modified
  * data, then call finish_heap_swap to complete the operation.
  */
 Oid
 make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
-			  LOCKMODE lockmode)
+			  Oid relam, LOCKMODE lockmode)
 {
 	TupleDesc	OldHeapDesc;
 	char		NewHeapName[NAMEDATALEN];
@@ -686,7 +688,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
 										  InvalidOid,
 										  InvalidOid,
 										  OldHeap->rd_rel->relowner,
-										  OldHeap->rd_rel->relam,
+										  relam,
 										  OldHeapDesc,
 										  NIL,
 										  RELKIND_RELATION,
@@ -1036,6 +1038,10 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
 		relform1->reltablespace = relform2->reltablespace;
 		relform2->reltablespace = swaptemp;
 
+		swaptemp = relform1->relam;
+		relform1->relam = relform2->relam;
+		relform2->relam = swaptemp;
+
 		swptmpchr = relform1->relpersistence;
 		relform1->relpersistence = relform2->relpersistence;
 		relform2->relpersistence = swptmpchr;
@@ -1071,6 +1077,9 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
 		if (relform1->relpersistence != relform2->relpersistence)
 			elog(ERROR, "cannot change persistence of mapped relation \"%s\"",
 				 NameStr(relform1->relname));
+		if (relform1->relam != relform2->relam)
+			elog(ERROR, "cannot change access method of mapped relation \"%s\"",
+				 NameStr(relform1->relname));
 		if (!swap_toast_by_content &&
 			(relform1->reltoastrelid || relform2->reltoastrelid))
 			elog(ERROR, "cannot swap toast by links for mapped relation \"%s\"",
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index c5c25ce11d..c206eaab7c 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -299,6 +299,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 	 * will be gone).
 	 */
 	OIDNewHeap = make_new_heap(matviewOid, tableSpace, relpersistence,
+							   matviewRel->rd_rel->relam,
 							   ExclusiveLock);
 	LockRelationOid(OIDNewHeap, AccessExclusiveLock);
 	dest = CreateTransientRelDestReceiver(OIDNewHeap);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b2457a6924..6384e7f715 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -18,6 +18,7 @@
 #include "access/genam.h"
 #include "access/heapam.h"
 #include "access/heapam_xlog.h"
+#include "access/heaptoast.h"
 #include "access/multixact.h"
 #include "access/reloptions.h"
 #include "access/relscan.h"
@@ -165,6 +166,7 @@ typedef struct AlteredTableInfo
 	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
+	Oid			newAccessMethod;	/* new table access method; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
 	char		newrelpersistence;	/* if above is true */
 	Expr	   *partition_constraint;	/* for attach partition validation */
@@ -508,6 +510,7 @@ static bool ATPrepChangePersistence(Relation rel, bool toLogged);
 static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel,
 								const char *tablespacename, LOCKMODE lockmode);
 static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode);
+static void ATPrepSeTabletAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname, LOCKMODE lockmode);
 static void ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace);
 static void ATExecSetRelOptions(Relation rel, List *defList,
 								AlterTableType operation,
@@ -3876,6 +3879,7 @@ AlterTableGetLockLevel(List *cmds)
 			case AT_AddColumn:	/* may rewrite heap, in some cases and visible
 								 * to SELECT */
 			case AT_SetTableSpace:	/* must rewrite heap */
+			case AT_SetTableAccessMethod:	/* must rewrite heap */
 			case AT_AlterColumnType:	/* must rewrite heap */
 				cmd_lockmode = AccessExclusiveLock;
 				break;
@@ -4392,6 +4396,15 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			ATPrepSetTableSpace(tab, rel, cmd->name, lockmode);
 			pass = AT_PASS_MISC;	/* doesn't actually matter */
 			break;
+
+		case AT_SetTableAccessMethod:	/* SET ACCESS METHOD */
+			ATSimplePermissions(rel, ATT_TABLE);
+			/* This command never recurses */
+			tab->rewrite |= AT_REWRITE_ACCESS_METHOD;
+			ATPrepSeTabletAccessMethod(tab, rel, cmd->name, lockmode);
+			pass = AT_PASS_MISC;	/* doesn't actually matter */
+			break;
+
 		case AT_SetRelOptions:	/* SET (...) */
 		case AT_ResetRelOptions:	/* RESET (...) */
 		case AT_ReplaceRelOptions:	/* reset them all, then set just these */
@@ -4752,6 +4765,10 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
 				ATExecSetTableSpaceNoStorage(rel, tab->newTableSpace);
 
 			break;
+		case AT_SetTableAccessMethod:	/* SET TABLE ACCESS METHOD */
+			/* Handled specially in Phase 3 */
+			break;
+
 		case AT_SetRelOptions:	/* SET (...) */
 		case AT_ResetRelOptions:	/* RESET (...) */
 		case AT_ReplaceRelOptions:	/* replace entire option list */
@@ -5067,7 +5084,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
 
 		/*
 		 * We only need to rewrite the table if at least one column needs to
-		 * be recomputed, or we are changing its persistence.
+		 * be recomputed, or we are changing its persistence or access method.
 		 *
 		 * There are two reasons for requiring a rewrite when changing
 		 * persistence: on one hand, we need to ensure that the buffers
@@ -5082,6 +5099,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
 			Relation	OldHeap;
 			Oid			OIDNewHeap;
 			Oid			NewTableSpace;
+			Oid			NewAccessMethod;
 			char		persistence;
 
 			OldHeap = table_open(tab->relid, NoLock);
@@ -5116,11 +5134,16 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
 			 * Select destination tablespace (same as original unless user
 			 * requested a change)
 			 */
-			if (tab->newTableSpace)
+			if (OidIsValid(tab->newTableSpace))
 				NewTableSpace = tab->newTableSpace;
 			else
 				NewTableSpace = OldHeap->rd_rel->reltablespace;
 
+			if (OidIsValid(tab->newAccessMethod))
+				NewAccessMethod = tab->newAccessMethod;
+			else
+				NewAccessMethod = OldHeap->rd_rel->relam;
+
 			/*
 			 * Select persistence of transient table (same as original unless
 			 * user requested a change)
@@ -5161,7 +5184,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
 			 * unlogged anyway.
 			 */
 			OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, persistence,
-									   lockmode);
+									   NewAccessMethod, lockmode);
 
 			/*
 			 * Copy the heap data into the new table with the desired
@@ -5483,13 +5506,35 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 			{
 				/* Extract data from old tuple */
 				slot_getallattrs(oldslot);
-				ExecClearTuple(newslot);
 
-				/* copy attributes */
-				memcpy(newslot->tts_values, oldslot->tts_values,
-					   sizeof(Datum) * oldslot->tts_nvalid);
-				memcpy(newslot->tts_isnull, oldslot->tts_isnull,
-					   sizeof(bool) * oldslot->tts_nvalid);
+				/* Need to detoast tuples when changing AM XXX: should check if one AM is heap and one isn't? */
+				if (newrel->rd_rel->relam != oldrel->rd_rel->relam)
+				{
+					HeapTuple htup = toast_build_flattened_tuple(oldTupDesc,
+							oldslot->tts_values,
+							oldslot->tts_isnull);
+
+					/*
+					 * Copy the value/null array to the new slot and materialize it,
+					 * before clearing the tuple from the old slot.
+					 */
+					ExecClearTuple(newslot);
+					ExecForceStoreHeapTuple(htup, oldslot, true);
+					slot_getallattrs(oldslot); /* again */
+
+					memcpy(newslot->tts_values, oldslot->tts_values, oldslot->tts_nvalid * sizeof(Datum));
+					memcpy(newslot->tts_isnull, oldslot->tts_isnull, oldslot->tts_nvalid * sizeof(bool));
+					ExecStoreVirtualTuple(newslot);
+					ExecMaterializeSlot(newslot);
+					ExecClearTuple(oldslot);
+				} else {
+					ExecClearTuple(newslot);
+					/* copy attributes */
+					memcpy(newslot->tts_values, oldslot->tts_values,
+						   sizeof(Datum) * oldslot->tts_nvalid);
+					memcpy(newslot->tts_isnull, oldslot->tts_isnull,
+						   sizeof(bool) * oldslot->tts_nvalid);
+				}
 
 				/* Set dropped attributes to null in new tuple */
 				foreach(lc, dropped_attrs)
@@ -5516,7 +5561,8 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 									   &newslot->tts_isnull[ex->attnum - 1]);
 				}
 
-				ExecStoreVirtualTuple(newslot);
+				if (newrel->rd_rel->relam == oldrel->rd_rel->relam)
+					ExecStoreVirtualTuple(newslot);
 
 				/*
 				 * Now, evaluate any expressions whose inputs come from the
@@ -13057,6 +13103,26 @@ ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, const char *tablespacen
 	tab->newTableSpace = tablespaceId;
 }
 
+/*
+ * ALTER TABLE SET ACCESS METHOD
+ */
+static void
+ATPrepSeTabletAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname, LOCKMODE lockmode)
+{
+	Oid			amoid;
+
+	/* Check that the AM exists */
+	amoid = get_table_am_oid(amname, false);
+
+	/* Save info for Phase 3 to do the real work */
+	if (OidIsValid(tab->newAccessMethod))
+		ereport(ERROR,
+				(errcode(ERRCODE_SYNTAX_ERROR),
+				 errmsg("cannot have multiple SET ACCESS METHOD subcommands")));
+
+	tab->newAccessMethod = amoid;
+}
+
 /*
  * Set, reset, or replace reloptions.
  */
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 652be0b96d..258bad6d39 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2582,6 +2582,16 @@ alter_table_cmd:
 					n->name = $3;
 					$$ = (Node *)n;
 				}
+
+			/* ALTER TABLE <name> SET TABLE ACCESS METHOD <amname> */
+			| SET TABLE ACCESS METHOD name
+				{
+					AlterTableCmd *n = makeNode(AlterTableCmd);
+					n->subtype = AT_SetTableAccessMethod;
+					n->name = $5;
+					$$ = (Node *)n;
+				}
+
 			/* ALTER TABLE <name> SET (...) */
 			| SET reloptions
 				{
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index a941f2accd..8c04466887 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -36,7 +36,7 @@ extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
 extern void mark_index_clustered(Relation rel, Oid indexOid, bool is_internal);
 
 extern Oid	make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
-						  LOCKMODE lockmode);
+						  Oid relam, LOCKMODE lockmode);
 extern void finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 							 bool is_system_catalog,
 							 bool swap_toast_by_content,
diff --git a/src/include/commands/event_trigger.h b/src/include/commands/event_trigger.h
index c11bf2d781..e765e67fd1 100644
--- a/src/include/commands/event_trigger.h
+++ b/src/include/commands/event_trigger.h
@@ -32,6 +32,7 @@ typedef struct EventTriggerData
 #define AT_REWRITE_ALTER_PERSISTENCE	0x01
 #define AT_REWRITE_DEFAULT_VAL			0x02
 #define AT_REWRITE_COLUMN_REWRITE		0x04
+#define AT_REWRITE_ACCESS_METHOD		0x08
 
 /*
  * EventTriggerData is the node type that is passed as fmgr "context" info
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 236832a2ca..81223af20c 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1869,6 +1869,7 @@ typedef enum AlterTableType
 	AT_SetUnLogged,				/* SET UNLOGGED */
 	AT_DropOids,				/* SET WITHOUT OIDS */
 	AT_SetTableSpace,			/* SET TABLESPACE */
+	AT_SetTableAccessMethod,	/* SET TABLE ACCESS METHOD */
 	AT_SetRelOptions,			/* SET (...) -- AM specific parameters */
 	AT_ResetRelOptions,			/* RESET (...) -- AM specific parameters */
 	AT_ReplaceRelOptions,		/* replace reloption list in its entirety */
diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out
index 0dfb26c301..74b53e6d63 100644
--- a/src/test/regress/expected/create_am.out
+++ b/src/test/regress/expected/create_am.out
@@ -230,6 +230,16 @@ ORDER BY classid, objid, objsubid;
  table tableam_parted_d_heap2
 (5 rows)
 
+-- ALTER TABLE SET ACCESS METHOD
+CREATE TABLE heaptable USING heap AS SELECT a, repeat(a::text,9999) FROM generate_series(1,9) AS a;
+ALTER TABLE heaptable SET TABLE ACCESS METHOD heap2;
+explain (analyze, costs off, summary off, timing off) SELECT * FROM heaptable;
+                  QUERY PLAN                   
+-----------------------------------------------
+ Seq Scan on heaptable (actual rows=9 loops=1)
+(1 row)
+
+DROP TABLE heaptable;
 -- Second, create objects in the new AM by changing the default AM
 BEGIN;
 SET LOCAL default_table_access_method = 'heap2';
diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/create_am.sql
index 9a359466ce..630df182c3 100644
--- a/src/test/regress/sql/create_am.sql
+++ b/src/test/regress/sql/create_am.sql
@@ -161,6 +161,11 @@ WHERE pg_depend.refclassid = 'pg_am'::regclass
     AND pg_am.amname = 'heap2'
 ORDER BY classid, objid, objsubid;
 
+-- ALTER TABLE SET ACCESS METHOD
+CREATE TABLE heaptable USING heap AS SELECT a, repeat(a::text,9999) FROM generate_series(1,9) AS a;
+ALTER TABLE heaptable SET TABLE ACCESS METHOD heap2;
+explain (analyze, costs off, summary off, timing off) SELECT * FROM heaptable;
+DROP TABLE heaptable;
 
 -- Second, create objects in the new AM by changing the default AM
 BEGIN;
-- 
2.17.0


--utPK4TBebyzZxMrE--