On Wed, May 18, 2022 at 6:26 PM Zhihong Yu <z...@yugabyte.com> wrote:
> + accessMethodId = ((Form_pg_class) GETSTRUCT(tup))->relam;
>
> - /* look up the access method, verify it is for a table */
> - if (accessMethod != NULL)
> - accessMethodId = get_table_am_oid(accessMethod, false);
> + if (!HeapTupleIsValid(tup))
> + elog(ERROR, "cache lookup failed for relation %u", relid);
>
> The validity check of tup should be done before fetching the value of
relam field.
Thanks. Fixed and rebased.
Regards,
Soumyadeep (VMware)
From a9fc146192430aa45224fb1e64bd95e0bb64fc00 Mon Sep 17 00:00:00 2001
From: Soumyadeep Chakraborty <soumyadeep2...@gmail.com>
Date: Thu, 9 Jun 2022 10:57:35 -0700
Subject: [PATCH v3 1/2] Allow ATSETAM on partition roots
Setting the access method on partition roots was disallowed. This commit
relaxes that restriction.
Summary of changes over original patch:
* Rebased
* Minor comment updates
* Add more test coverage
Authors: Justin Pryzby <pryz...@telsasoft.com>, Soumyadeep Chakraborty
<soumyadeep2...@gmail.com>
---
src/backend/catalog/heap.c | 3 +-
src/backend/commands/tablecmds.c | 103 +++++++++++++++++++-----
src/backend/utils/cache/relcache.c | 4 +
src/test/regress/expected/create_am.out | 50 +++++++-----
src/test/regress/sql/create_am.sql | 23 +++---
5 files changed, 129 insertions(+), 54 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 1803194db94..4561f3b8c98 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1449,7 +1449,8 @@ heap_create_with_catalog(const char *relname,
* No need to add an explicit dependency for the toast table, as the
* main table depends on it.
*/
- if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE)
+ if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) ||
+ relkind == RELKIND_PARTITIONED_TABLE)
{
ObjectAddressSet(referenced, AccessMethodRelationId, accessmtd);
add_exact_object_address(&referenced, addrs);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2de0ebacec3..9a5eabcac49 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -570,6 +570,7 @@ static ObjectAddress ATExecClusterOn(Relation rel, const char *indexName,
LOCKMODE lockmode);
static void ATExecDropCluster(Relation rel, LOCKMODE lockmode);
static void ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname);
+static void ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethod);
static bool ATPrepChangePersistence(Relation rel, bool toLogged);
static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel,
const char *tablespacename, LOCKMODE lockmode);
@@ -676,7 +677,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
Oid ofTypeId;
ObjectAddress address;
LOCKMODE parentLockmode;
- const char *accessMethod = NULL;
Oid accessMethodId = InvalidOid;
/*
@@ -938,20 +938,32 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* a type of relation that needs one, use the default.
*/
if (stmt->accessMethod != NULL)
+ accessMethodId = get_table_am_oid(stmt->accessMethod, false);
+ else if (stmt->partbound && relkind == RELKIND_RELATION)
{
- accessMethod = stmt->accessMethod;
+ HeapTuple tup;
+ Oid relid;
- 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;
+ /*
+ * For partitioned tables, when no access method is specified, we
+ * default to the parent table's AM.
+ */
+ Assert(list_length(inheritOids) == 1);
+ /* XXX: should implement get_rel_relam? */
+ relid = linitial_oid(inheritOids);
+ tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for relation %u", relid);
- /* look up the access method, verify it is for a table */
- if (accessMethod != NULL)
- accessMethodId = get_table_am_oid(accessMethod, false);
+ accessMethodId = ((Form_pg_class) GETSTRUCT(tup))->relam;
+
+ ReleaseSysCache(tup);
+
+ if (!OidIsValid(accessMethodId))
+ accessMethodId = get_table_am_oid(default_table_access_method, false);
+ }
+ else if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)
+ accessMethodId = get_table_am_oid(default_table_access_method, false);
/*
* Create the relation. Inherited defaults and constraints are passed in
@@ -4693,13 +4705,6 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_SetAccessMethod: /* SET ACCESS METHOD */
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW);
-
- /* partitioned tables don't have an access method */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot change access method of a partitioned table")));
-
/* check if another access method change was already requested */
if (OidIsValid(tab->newAccessMethod))
ereport(ERROR,
@@ -5085,7 +5090,13 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
/* nothing to do here, oid columns don't exist anymore */
break;
case AT_SetAccessMethod: /* SET ACCESS METHOD */
- /* handled specially in Phase 3 */
+ /*
+ * Only do this for partitioned tables, for which this is just a
+ * catalog change. Other relation types which have storage are
+ * handled by Phase 3.
+ */
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ATExecSetAccessMethodNoStorage(rel, tab->newAccessMethod);
break;
case AT_SetTableSpace: /* SET TABLESPACE */
@@ -14421,6 +14432,58 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
list_free(reltoastidxids);
}
+/*
+ * Special handling of ALTER TABLE SET ACCESS METHOD for relations with no
+ * storage that have an interest in preserving AM.
+ *
+ * Since these relations have no storage the access method can be updated with a
+ * simple metadata only operation.
+ */
+static void
+ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethod)
+{
+ Relation pg_class;
+ Oid relid;
+ Oid oldrelam;
+ HeapTuple tuple;
+
+ /*
+ * Shouldn't be called on relations having storage; these are processed in
+ * phase 3.
+ */
+ Assert(!RELKIND_HAS_STORAGE(rel->rd_rel->relkind));
+
+ relid = RelationGetRelid(rel);
+
+ /* Pull the record for this relation and update it */
+ pg_class = table_open(RelationRelationId, RowExclusiveLock);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid));
+
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", relid);
+
+ oldrelam = ((Form_pg_class) GETSTRUCT(tuple))->relam;
+ ((Form_pg_class) GETSTRUCT(tuple))->relam = newAccessMethod;
+ CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+ /*
+ * Record dependency on AM. This is only required for relations
+ * that have no physical storage.
+ */
+ changeDependencyFor(RelationRelationId, RelationGetRelid(rel),
+ AccessMethodRelationId, oldrelam,
+ newAccessMethod);
+
+ InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(rel), 0);
+
+ heap_freetuple(tuple);
+ table_close(pg_class, RowExclusiveLock);
+
+ /* Make sure the relam change is visible */
+ CommandCounterIncrement();
+}
+
/*
* Special handling of ALTER TABLE SET TABLESPACE for relations with no
* storage that have an interest in preserving tablespace.
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 60e72f9e8bf..7e955c44173 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1206,6 +1206,10 @@ retry:
else if (RELKIND_HAS_TABLE_AM(relation->rd_rel->relkind) ||
relation->rd_rel->relkind == RELKIND_SEQUENCE)
RelationInitTableAccessMethod(relation);
+ else if (relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ /* Do nothing: the am is a catalog setting for partitions to inherit */
+ }
else
Assert(relation->rd_rel->relam == InvalidOid);
diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out
index e9a9283d7ab..a48199df9a2 100644
--- a/src/test/regress/expected/create_am.out
+++ b/src/test/regress/expected/create_am.out
@@ -176,19 +176,12 @@ SELECT f1 FROM tableam_tblmv_heap2 ORDER BY f1;
1
(1 row)
--- CREATE TABLE .. PARTITION BY doesn't not support USING
+-- CREATE TABLE .. PARTITION BY supports USING
+-- new partitions will inherit from the partition root
CREATE TABLE tableam_parted_heap2 (a text, b int) PARTITION BY list (a) USING heap2;
-ERROR: specifying a table access method is not supported on a partitioned table
-CREATE TABLE tableam_parted_heap2 (a text, b int) PARTITION BY list (a);
--- new partitions will inherit from the current default, rather the partition root
-SET default_table_access_method = 'heap';
CREATE TABLE tableam_parted_a_heap2 PARTITION OF tableam_parted_heap2 FOR VALUES IN ('a');
-SET default_table_access_method = 'heap2';
-CREATE TABLE tableam_parted_b_heap2 PARTITION OF tableam_parted_heap2 FOR VALUES IN ('b');
-RESET default_table_access_method;
-- but the method can be explicitly specified
-CREATE TABLE tableam_parted_c_heap2 PARTITION OF tableam_parted_heap2 FOR VALUES IN ('c') USING heap;
-CREATE TABLE tableam_parted_d_heap2 PARTITION OF tableam_parted_heap2 FOR VALUES IN ('d') USING heap2;
+CREATE TABLE tableam_parted_b_heap2 PARTITION OF tableam_parted_heap2 FOR VALUES IN ('b') USING heap;
-- List all objects in AM
SELECT
pc.relkind,
@@ -205,14 +198,13 @@ WHERE pa.oid = pc.relam
ORDER BY 3, 1, 2;
relkind | amname | relname
---------+--------+----------------------------------
- r | heap2 | tableam_parted_b_heap2
- r | heap2 | tableam_parted_d_heap2
+ r | heap2 | tableam_parted_a_heap2
+ p | heap2 | tableam_parted_heap2
r | heap2 | tableam_tbl_heap2
r | heap2 | tableam_tblas_heap2
m | heap2 | tableam_tblmv_heap2
- t | heap2 | toast for tableam_parted_b_heap2
- t | heap2 | toast for tableam_parted_d_heap2
-(7 rows)
+ t | heap2 | toast for tableam_parted_a_heap2
+(6 rows)
-- Show dependencies onto AM - there shouldn't be any for toast
SELECT pg_describe_object(classid,objid,objsubid) AS obj
@@ -226,8 +218,8 @@ ORDER BY classid, objid, objsubid;
table tableam_tbl_heap2
table tableam_tblas_heap2
materialized view tableam_tblmv_heap2
- table tableam_parted_b_heap2
- table tableam_parted_d_heap2
+ table tableam_parted_heap2
+ table tableam_parted_a_heap2
(5 rows)
-- ALTER TABLE SET ACCESS METHOD
@@ -284,11 +276,26 @@ ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap, SET ACCESS METHOD heap2;
ERROR: cannot have multiple SET ACCESS METHOD subcommands
DROP MATERIALIZED VIEW heapmv;
DROP TABLE heaptable;
--- No support for partitioned tables.
+-- partition hierarchies
+-- upon ALTER, new children will inherit the new am, whereas the existing
+-- children will remain untouched
CREATE TABLE am_partitioned(x INT, y INT)
PARTITION BY hash (x);
+CREATE TABLE am_partitioned_1 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 0);
+CREATE TABLE am_partitioned_2 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 1);
+ALTER TABLE am_partitioned_1 SET ACCESS METHOD heap2;
ALTER TABLE am_partitioned SET ACCESS METHOD heap2;
-ERROR: cannot change access method of a partitioned table
+CREATE TABLE am_partitioned_3 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 2);
+SELECT relname, amname FROM pg_class c, pg_am am
+ WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
+ relname | amname
+------------------+--------
+ am_partitioned | heap2
+ am_partitioned_1 | heap2
+ am_partitioned_2 | heap
+ am_partitioned_3 | heap2
+(4 rows)
+
DROP TABLE am_partitioned;
-- Second, create objects in the new AM by changing the default AM
BEGIN;
@@ -326,7 +333,7 @@ ORDER BY 3, 1, 2;
f | | tableam_fdw_heapx
r | heap2 | tableam_parted_1_heapx
r | heap | tableam_parted_2_heapx
- p | | tableam_parted_heapx
+ p | heap2 | tableam_parted_heapx
S | | tableam_seq_heapx
r | heap2 | tableam_tbl_heapx
r | heap2 | tableam_tblas_heapx
@@ -355,7 +362,6 @@ ERROR: cannot drop access method heap2 because other objects depend on it
DETAIL: table tableam_tbl_heap2 depends on access method heap2
table tableam_tblas_heap2 depends on access method heap2
materialized view tableam_tblmv_heap2 depends on access method heap2
-table tableam_parted_b_heap2 depends on access method heap2
-table tableam_parted_d_heap2 depends on access method heap2
+table tableam_parted_heap2 depends on access method heap2
HINT: Use DROP ... CASCADE to drop the dependent objects too.
-- we intentionally leave the objects created above alive, to verify pg_dump support
diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/create_am.sql
index 256884c9592..8f3db03bba2 100644
--- a/src/test/regress/sql/create_am.sql
+++ b/src/test/regress/sql/create_am.sql
@@ -124,19 +124,12 @@ CREATE SEQUENCE tableam_seq_heap2 USING heap2;
CREATE MATERIALIZED VIEW tableam_tblmv_heap2 USING heap2 AS SELECT * FROM tableam_tbl_heap2;
SELECT f1 FROM tableam_tblmv_heap2 ORDER BY f1;
--- CREATE TABLE .. PARTITION BY doesn't not support USING
+-- CREATE TABLE .. PARTITION BY supports USING
+-- new partitions will inherit from the partition root
CREATE TABLE tableam_parted_heap2 (a text, b int) PARTITION BY list (a) USING heap2;
-
-CREATE TABLE tableam_parted_heap2 (a text, b int) PARTITION BY list (a);
--- new partitions will inherit from the current default, rather the partition root
-SET default_table_access_method = 'heap';
CREATE TABLE tableam_parted_a_heap2 PARTITION OF tableam_parted_heap2 FOR VALUES IN ('a');
-SET default_table_access_method = 'heap2';
-CREATE TABLE tableam_parted_b_heap2 PARTITION OF tableam_parted_heap2 FOR VALUES IN ('b');
-RESET default_table_access_method;
-- but the method can be explicitly specified
-CREATE TABLE tableam_parted_c_heap2 PARTITION OF tableam_parted_heap2 FOR VALUES IN ('c') USING heap;
-CREATE TABLE tableam_parted_d_heap2 PARTITION OF tableam_parted_heap2 FOR VALUES IN ('d') USING heap2;
+CREATE TABLE tableam_parted_b_heap2 PARTITION OF tableam_parted_heap2 FOR VALUES IN ('b') USING heap;
-- List all objects in AM
SELECT
@@ -183,10 +176,18 @@ ALTER TABLE heaptable SET ACCESS METHOD heap, SET ACCESS METHOD heap2;
ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap, SET ACCESS METHOD heap2;
DROP MATERIALIZED VIEW heapmv;
DROP TABLE heaptable;
--- No support for partitioned tables.
+-- partition hierarchies
+-- upon ALTER, new children will inherit the new am, whereas the existing
+-- children will remain untouched
CREATE TABLE am_partitioned(x INT, y INT)
PARTITION BY hash (x);
+CREATE TABLE am_partitioned_1 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 0);
+CREATE TABLE am_partitioned_2 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 1);
+ALTER TABLE am_partitioned_1 SET ACCESS METHOD heap2;
ALTER TABLE am_partitioned SET ACCESS METHOD heap2;
+CREATE TABLE am_partitioned_3 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 2);
+SELECT relname, amname FROM pg_class c, pg_am am
+ WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
DROP TABLE am_partitioned;
-- Second, create objects in the new AM by changing the default AM
--
2.25.1
From f8d5e584a129fc49fd0877586036dac027aaa9cf Mon Sep 17 00:00:00 2001
From: Soumyadeep Chakraborty <soumyadeep2...@gmail.com>
Date: Thu, 9 Jun 2022 10:58:13 -0700
Subject: [PATCH v3 2/2] Make ATSETAM recurse by default
ATSETAM now recurses to partition children by default. To prevent
recursion, and have the new am apply to future children only, users must
specify the ONLY clause.
---
src/backend/commands/tablecmds.c | 2 ++
src/test/regress/expected/create_am.out | 13 ++++++++++++-
src/test/regress/sql/create_am.sql | 5 ++++-
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9a5eabcac49..b4f939227fd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4711,6 +4711,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot have multiple SET ACCESS METHOD subcommands")));
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
ATPrepSetAccessMethod(tab, rel, cmd->name);
pass = AT_PASS_MISC; /* does not matter; no work in Phase 2 */
break;
diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out
index a48199df9a2..e99de1ac912 100644
--- a/src/test/regress/expected/create_am.out
+++ b/src/test/regress/expected/create_am.out
@@ -284,7 +284,7 @@ CREATE TABLE am_partitioned(x INT, y INT)
CREATE TABLE am_partitioned_1 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 0);
CREATE TABLE am_partitioned_2 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 1);
ALTER TABLE am_partitioned_1 SET ACCESS METHOD heap2;
-ALTER TABLE am_partitioned SET ACCESS METHOD heap2;
+ALTER TABLE ONLY am_partitioned SET ACCESS METHOD heap2;
CREATE TABLE am_partitioned_3 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 2);
SELECT relname, amname FROM pg_class c, pg_am am
WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
@@ -296,6 +296,17 @@ SELECT relname, amname FROM pg_class c, pg_am am
am_partitioned_3 | heap2
(4 rows)
+ALTER TABLE am_partitioned SET ACCESS METHOD heap;
+SELECT relname, amname FROM pg_class c, pg_am am
+WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
+ relname | amname
+------------------+--------
+ am_partitioned | heap
+ am_partitioned_1 | heap
+ am_partitioned_2 | heap
+ am_partitioned_3 | heap
+(4 rows)
+
DROP TABLE am_partitioned;
-- Second, create objects in the new AM by changing the default AM
BEGIN;
diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/create_am.sql
index 8f3db03bba2..f4e22684782 100644
--- a/src/test/regress/sql/create_am.sql
+++ b/src/test/regress/sql/create_am.sql
@@ -184,10 +184,13 @@ CREATE TABLE am_partitioned(x INT, y INT)
CREATE TABLE am_partitioned_1 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 0);
CREATE TABLE am_partitioned_2 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 1);
ALTER TABLE am_partitioned_1 SET ACCESS METHOD heap2;
-ALTER TABLE am_partitioned SET ACCESS METHOD heap2;
+ALTER TABLE ONLY am_partitioned SET ACCESS METHOD heap2;
CREATE TABLE am_partitioned_3 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 2);
SELECT relname, amname FROM pg_class c, pg_am am
WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
+ALTER TABLE am_partitioned SET ACCESS METHOD heap;
+SELECT relname, amname FROM pg_class c, pg_am am
+WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
DROP TABLE am_partitioned;
-- Second, create objects in the new AM by changing the default AM
--
2.25.1
