Date: Tue, 29 Dec 2020 17:08:05 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org, 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>
Subject: Re: create table like: ACCESS METHOD
Message-ID: <20201229230805.GE25152@telsasoft.com>
References: <20201209201328.GT24052@telsasoft.com>
 <X+WJqu4X4EniF6xT@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="3V7upXqbjpZ4EhLz"
Content-Disposition: inline
In-Reply-To: <X+WJqu4X4EniF6xT@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


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

On Fri, Dec 25, 2020 at 03:41:46PM +0900, Michael Paquier wrote:
> On Wed, Dec 09, 2020 at 02:13:29PM -0600, Justin Pryzby wrote:
> > I thought this was a good idea, but didn't hear back when I raised it before.
> > 
> > Failing to preserve access method is arguably a bug, reminiscent of CREATE
> > STATISTICS and 5564c1181.  But maybe it's not important to backpatch a fix in
> > this case, since access methods are still evolving.
> 
> Interesting.  Access methods for tables are released for more than one
> year now, so my take about a backpatch is that this boat has already
> sailed.  This may give a reason to actually not introduce this
> feature.

Are you saying that since v12/13 didn't preserve the access method, it might be
preferred to never do it ?  I think it's reasonable to to not change v12/13 but
the behavior seems like an omission going forward.  It's not so important right
now, since AMs aren't widely used.

This might be important for a few cases I can think of easily:

If an readonly AM doesn't support DDL, and table needs to be rebuilt, we'd
handle that by creating a new table LIKE the existing table, preserving its AM,
and then INSERT into it.  Like for column type promotion.  That's much better
than querying amname FROM pg_class JOIN relam.

ALTER TABLE..ATTACH PARTITION requires a less strong lock than CREATE
TABLE..PARTITION OF, so it's nice to be able to CREATE TABLE LIKE.

To use an alternate AM for historic data, we'd CREATE TABLE LIKE an existing,
populated table before inserting into it.  This would support re-creating on a
new AM, or re-creating on the same AM, say, to get rid of dropped columns, or
to re-arrange columns.  

>  -- fail, as partitioned tables don't allow NO INHERIT constraints
> -CREATE TABLE noinh_con_copy1_parted (LIKE noinh_con_copy INCLUDING ALL)
> +CREATE TABLE noinh_con_copy1_parted (LIKE noinh_con_copy INCLUDING ALL EXCLUDING ACCESS METHOD)
>    PARTITION BY LIST (a);
> This diff means that you are introducing an incompatible change by
> forcing any application using CREATE TABLE LIKE for a partitioned
> table to exclude access methods.  This is not acceptable, and it may
> be better to just ignore this clause instead in this context.

Ok.  This means that 
CREATE TABLE (LIKE x INCLUDING ACCESS METHOD) PARTITION BY ...
silently ignores the INCLUDING AM.  Is that ok ?  I think the alternative is
for INCLUDING to be "ternary" options, defaulting to UNSET=0, when it's ok to
ignore options in contexts where they're not useful.
Maybe we'd need to specially handle INCLUDING ALL, to make options
"soft"/implied rather than explicit.

> This patch should have more tests.  Something could be added in
> create_am.sql where there is a fake heap2 as table AM.

Yes, I had already done that locally.

-- 
Justin

--3V7upXqbjpZ4EhLz
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v2-0001-create-table-like-.-including-ACCESS-METHOD.patch"

From c4c4570f90623aa8508a2749f5ca5e2780ab10df Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 15 Nov 2020 16:54:53 -0600
Subject: [PATCH v2] create table (like .. including ACCESS METHOD)

---
 doc/src/sgml/ref/create_table.sgml              | 12 +++++++++++-
 src/backend/parser/gram.y                       |  3 ++-
 src/backend/parser/parse_utilcmd.c              | 10 ++++++++++
 src/include/nodes/parsenodes.h                  | 17 +++++++++--------
 src/test/regress/expected/create_table_like.out | 12 ++++++++++++
 src/test/regress/sql/create_table_like.sql      |  8 ++++++++
 6 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 569f4c9da7..cb95177e92 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -87,7 +87,7 @@ class="parameter">referential_action</replaceable> ] [ ON UPDATE <replaceable cl
 
 <phrase>and <replaceable class="parameter">like_option</replaceable> is:</phrase>
 
-{ INCLUDING | EXCLUDING } { COMMENTS | CONSTRAINTS | DEFAULTS | GENERATED | IDENTITY | INDEXES | STATISTICS | STORAGE | ALL }
+{ INCLUDING | EXCLUDING } { ACCESS METHOD | COMMENTS | CONSTRAINTS | DEFAULTS | GENERATED | IDENTITY | INDEXES | STATISTICS | STORAGE | ALL }
 
 <phrase>and <replaceable class="parameter">partition_bound_spec</replaceable> is:</phrase>
 
@@ -593,6 +593,16 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
       available options are:
 
       <variablelist>
+       <varlistentry>
+        <term><literal>INCLUDING ACCESS METHOD</literal></term>
+        <listitem>
+         <para>
+          The table's access method will be copied.  By default, the
+          <literal>default_table_access_method</literal> is used.
+         </para>
+        </listitem>
+       </varlistentry>
+
        <varlistentry>
         <term><literal>INCLUDING COMMENTS</literal></term>
         <listitem>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 8f341ac006..fee0aceb65 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3651,7 +3651,8 @@ TableLikeOptionList:
 		;
 
 TableLikeOption:
-				COMMENTS			{ $$ = CREATE_TABLE_LIKE_COMMENTS; }
+				ACCESS METHOD			{ $$ = CREATE_TABLE_LIKE_ACCESS_METHOD; }
+				| COMMENTS			{ $$ = CREATE_TABLE_LIKE_COMMENTS; }
 				| CONSTRAINTS		{ $$ = CREATE_TABLE_LIKE_CONSTRAINTS; }
 				| DEFAULTS			{ $$ = CREATE_TABLE_LIKE_DEFAULTS; }
 				| IDENTITY_P		{ $$ = CREATE_TABLE_LIKE_IDENTITY; }
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 89ee990599..4eed7e3ea2 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -96,6 +96,7 @@ typedef struct
 	bool		ispartitioned;	/* true if table is partitioned */
 	PartitionBoundSpec *partbound;	/* transformed FOR VALUES */
 	bool		ofType;			/* true if statement contains OF typename */
+	char		*accessMethod;	/* table access method */
 } CreateStmtContext;
 
 /* State shared by transformCreateSchemaStmt and its subroutines */
@@ -252,6 +253,7 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
 	cxt.ispartitioned = stmt->partspec != NULL;
 	cxt.partbound = stmt->partbound;
 	cxt.ofType = (stmt->ofTypename != NULL);
+	cxt.accessMethod = NULL;
 
 	Assert(!stmt->ofTypename || !stmt->inhRelations);	/* grammar enforces */
 
@@ -346,6 +348,9 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
 	stmt->tableElts = cxt.columns;
 	stmt->constraints = cxt.ckconstraints;
 
+	if (cxt.accessMethod != NULL)
+		stmt->accessMethod = cxt.accessMethod;
+
 	result = lappend(cxt.blist, stmt);
 	result = list_concat(result, cxt.alist);
 	result = list_concat(result, save_alist);
@@ -1118,6 +1123,11 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
 		cxt->likeclauses = lappend(cxt->likeclauses, table_like_clause);
 	}
 
+	/* CREATE TABLE LIKE ACCESS METHOD is not copied for partitioned table */
+	if ((table_like_clause->options & CREATE_TABLE_LIKE_ACCESS_METHOD) != 0 &&
+		!cxt->ispartitioned)
+		cxt->accessMethod = get_am_name(relation->rd_rel->relam);
+
 	/*
 	 * We may copy extended statistics if requested, since the representation
 	 * of CreateStatsStmt doesn't depend on column numbers.
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 48a79a7657..2d5dfe487e 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -677,14 +677,15 @@ typedef struct TableLikeClause
 
 typedef enum TableLikeOption
 {
-	CREATE_TABLE_LIKE_COMMENTS = 1 << 0,
-	CREATE_TABLE_LIKE_CONSTRAINTS = 1 << 1,
-	CREATE_TABLE_LIKE_DEFAULTS = 1 << 2,
-	CREATE_TABLE_LIKE_GENERATED = 1 << 3,
-	CREATE_TABLE_LIKE_IDENTITY = 1 << 4,
-	CREATE_TABLE_LIKE_INDEXES = 1 << 5,
-	CREATE_TABLE_LIKE_STATISTICS = 1 << 6,
-	CREATE_TABLE_LIKE_STORAGE = 1 << 7,
+	CREATE_TABLE_LIKE_ACCESS_METHOD = 1 << 0,
+	CREATE_TABLE_LIKE_COMMENTS = 1 << 1,
+	CREATE_TABLE_LIKE_CONSTRAINTS = 1 << 2,
+	CREATE_TABLE_LIKE_DEFAULTS = 1 << 3,
+	CREATE_TABLE_LIKE_GENERATED = 1 << 4,
+	CREATE_TABLE_LIKE_IDENTITY = 1 << 5,
+	CREATE_TABLE_LIKE_INDEXES = 1 << 6,
+	CREATE_TABLE_LIKE_STATISTICS = 1 << 7,
+	CREATE_TABLE_LIKE_STORAGE = 1 << 8,
 	CREATE_TABLE_LIKE_ALL = PG_INT32_MAX
 } TableLikeOption;
 
diff --git a/src/test/regress/expected/create_table_like.out b/src/test/regress/expected/create_table_like.out
index 10d17be23c..a09cd48ed0 100644
--- a/src/test/regress/expected/create_table_like.out
+++ b/src/test/regress/expected/create_table_like.out
@@ -429,6 +429,18 @@ SELECT s.stxname, objsubid, description FROM pg_description, pg_statistic_ext s
  ctlt_all_a_b_stat |        0 | ab stats
 (1 row)
 
+CREATE ACCESS METHOD heapdup TYPE TABLE HANDLER heap_tableam_handler;
+CREATE TABLE likeam() USING heapdup;
+CREATE TABLE likeamlike(LIKE likeam INCLUDING ALL);
+-- pg_regress helpfully hides the Access Method output, which we need:
+SELECT a.amname FROM pg_class c JOIN pg_am a ON c.relam=a.oid WHERE c.oid='likeamlike'::regclass;
+ amname  
+---------
+ heapdup
+(1 row)
+
+DROP TABLE likeam, likeamlike;
+DROP ACCESS METHOD heapdup;
 CREATE TABLE inh_error1 () INHERITS (ctlt1, ctlt4);
 NOTICE:  merging multiple inherited definitions of column "a"
 ERROR:  inherited column "a" has a storage parameter conflict
diff --git a/src/test/regress/sql/create_table_like.sql b/src/test/regress/sql/create_table_like.sql
index 06b76f949d..ee6c464851 100644
--- a/src/test/regress/sql/create_table_like.sql
+++ b/src/test/regress/sql/create_table_like.sql
@@ -165,6 +165,14 @@ CREATE TABLE ctlt_all (LIKE ctlt1 INCLUDING ALL);
 SELECT c.relname, objsubid, description FROM pg_description, pg_index i, pg_class c WHERE classoid = 'pg_class'::regclass AND objoid = i.indexrelid AND c.oid = i.indexrelid AND i.indrelid = 'ctlt_all'::regclass ORDER BY c.relname, objsubid;
 SELECT s.stxname, objsubid, description FROM pg_description, pg_statistic_ext s WHERE classoid = 'pg_statistic_ext'::regclass AND objoid = s.oid AND s.stxrelid = 'ctlt_all'::regclass ORDER BY s.stxname, objsubid;
 
+CREATE ACCESS METHOD heapdup TYPE TABLE HANDLER heap_tableam_handler;
+CREATE TABLE likeam() USING heapdup;
+CREATE TABLE likeamlike(LIKE likeam INCLUDING ALL);
+-- pg_regress helpfully hides the Access Method output, which we need:
+SELECT a.amname FROM pg_class c JOIN pg_am a ON c.relam=a.oid WHERE c.oid='likeamlike'::regclass;
+DROP TABLE likeam, likeamlike;
+DROP ACCESS METHOD heapdup;
+
 CREATE TABLE inh_error1 () INHERITS (ctlt1, ctlt4);
 CREATE TABLE inh_error2 (LIKE ctlt4 INCLUDING STORAGE) INHERITS (ctlt1);
 
-- 
2.17.0


--3V7upXqbjpZ4EhLz--