Date: Thu, 27 Feb 2020 17:25:13 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, pgsql-hackers@postgresql.org
Subject: Re: ALTER INDEX fails on partitioned index
Message-ID: <20200227232513.GE29456@telsasoft.com>
References: <20190206173212.GA23770@alvherre.pgsql>
 <201901071923.7r2xflxvncly@alvherre.pgsql>
 <20191227035157.GD12890@telsasoft.com>
 <CA+Tgmobq8_-DS7qDEmMi-4ARP1_0bkgFEjYfiK97L2eXq+Q+nw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="l76fUT7nc3MelDdI"
Content-Disposition: inline
In-Reply-To: 
 <CA+Tgmobq8_-DS7qDEmMi-4ARP1_0bkgFEjYfiK97L2eXq+Q+nw@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


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

The attached allows CREATE/ALTER to specify reloptions on a partitioned table
which are used as defaults for future children.

I think that's a desirable behavior, same as for tablespaces.  Michael
mentioned that ALTER INDEX ONLY doesn't exist, but that's only an issue if
ALTER acts recursively, which isn't the case here.

The current behavior seems unreasonable: CREATE allows specifying fillfactor,
which does nothing, and unable to alter it, either:

postgres=# CREATE TABLE tt(i int)PARTITION BY RANGE (i);;
CREATE TABLE
postgres=# CREATE INDEX ON tt(i)WITH(fillfactor=11);
CREATE INDEX
postgres=# \d tt
...
    "tt_i_idx" btree (i) WITH (fillfactor='11')
postgres=# ALTER INDEX tt_i_idx SET (fillfactor=12);
ERROR:  "tt_i_idx" is not a table, view, materialized view, or index

Maybe there are other ALTER commands to handle (UNLOGGED currently does nothing
on a partitioned table?, STATISTICS, ...).

The first patch makes a prettier message, per Robert's suggestion.

-- 
Justin

--l76fUT7nc3MelDdI
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v2-0001-More-specific-error-message-when-failing-to-alter.patch"

From e5bb363f514d768a4f540d9c82ad5745944b1486 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 30 Dec 2019 09:31:03 -0600
Subject: [PATCH v2 1/2] More specific error message when failing to alter a
 partitioned index..

"..is not an index" is deemed to be unfriendly

https://www.postgresql.org/message-id/CA%2BTgmobq8_-DS7qDEmMi-4ARP1_0bkgFEjYfiK97L2eXq%2BQ%2Bnw%40mail.gmail.com
---
 src/backend/commands/tablecmds.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b7c8d66..1b271af 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -366,7 +366,7 @@ static void ATRewriteTables(AlterTableStmt *parsetree,
 static void ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode);
 static AlteredTableInfo *ATGetQueueEntry(List **wqueue, Relation rel);
 static void ATSimplePermissions(Relation rel, int allowed_targets);
-static void ATWrongRelkindError(Relation rel, int allowed_targets);
+static void ATWrongRelkindError(Relation rel, int allowed_targets, int actual_target);
 static void ATSimpleRecursion(List **wqueue, Relation rel,
 							  AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode,
 							  AlterTableUtilityContext *context);
@@ -5458,7 +5458,7 @@ ATSimplePermissions(Relation rel, int allowed_targets)
 
 	/* Wrong target type? */
 	if ((actual_target & allowed_targets) == 0)
-		ATWrongRelkindError(rel, allowed_targets);
+		ATWrongRelkindError(rel, allowed_targets, actual_target);
 
 	/* Permissions checks */
 	if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
@@ -5479,7 +5479,7 @@ ATSimplePermissions(Relation rel, int allowed_targets)
  * type.
  */
 static void
-ATWrongRelkindError(Relation rel, int allowed_targets)
+ATWrongRelkindError(Relation rel, int allowed_targets, int actual_target)
 {
 	char	   *msg;
 
@@ -5527,9 +5527,20 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 			break;
 	}
 
-	ereport(ERROR,
-			(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-			 errmsg(msg, RelationGetRelationName(rel))));
+	if (actual_target == ATT_PARTITIONED_INDEX &&
+			(allowed_targets&ATT_INDEX) &&
+			!(allowed_targets&ATT_PARTITIONED_INDEX))
+		/* Add a special errhint for this case, since "is not an index" message is unfriendly */
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg(msg, RelationGetRelationName(rel)),
+				 // errhint("\"%s\" is a partitioned index", RelationGetRelationName(rel))));
+				 errhint("operation is not supported on partitioned indexes")));
+	else
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg(msg, RelationGetRelationName(rel))));
+
 }
 
 /*
-- 
2.7.4


--l76fUT7nc3MelDdI
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v2-0002-Allow-reloptions-on-partitioned-tables-and-indexe.patch"

From 5f7f79c4c8e85528e88ffa0b749faad1da4ab4d5 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Tue, 25 Feb 2020 14:27:09 -0600
Subject: [PATCH v2 2/2] Allow reloptions on partitioned tables and indexes..

..which provides default values used for future (direct) partitions.

See also similar behavior from:
dfa60814 - Fix tablespace handling for partitioned indexes
ca410302 - Fix tablespace handling for partitioned tables
c94e6942 - Don't allocate storage for partitioned tables.
---
 src/backend/access/common/reloptions.c   | 19 +------------------
 src/backend/commands/tablecmds.c         | 32 +++++++++++++++++++++++---------
 src/test/regress/expected/reloptions.out | 29 +++++++++++++++++++++++++++++
 src/test/regress/sql/reloptions.sql      | 11 +++++++++++
 4 files changed, 64 insertions(+), 27 deletions(-)

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 79430d2..a2678e7 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1108,10 +1108,8 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
 		case RELKIND_RELATION:
 		case RELKIND_TOASTVALUE:
 		case RELKIND_MATVIEW:
-			options = heap_reloptions(classForm->relkind, datum, false);
-			break;
 		case RELKIND_PARTITIONED_TABLE:
-			options = partitioned_table_reloptions(datum, false);
+			options = heap_reloptions(classForm->relkind, datum, false);
 			break;
 		case RELKIND_VIEW:
 			options = view_reloptions(datum, false);
@@ -1581,21 +1579,6 @@ build_reloptions(Datum reloptions, bool validate,
 }
 
 /*
- * Option parser for partitioned tables
- */
-bytea *
-partitioned_table_reloptions(Datum reloptions, bool validate)
-{
-	/*
-	 * There are no options for partitioned tables yet, but this is able to do
-	 * some validation.
-	 */
-	return (bytea *) build_reloptions(reloptions, validate,
-									  RELOPT_KIND_PARTITIONED,
-									  0, NULL, 0);
-}
-
-/*
  * Option parser for views
  */
 bytea *
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1b271af..675b74d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -740,17 +740,33 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	/*
 	 * Parse and validate reloptions, if any.
 	 */
-	reloptions = transformRelOptions((Datum) 0, stmt->options, NULL, validnsps,
-									 true, false);
+
+	if (stmt->partbound)
+	{
+		/* If partitioned, also use reloptions from the immediate parent */
+		Datum oldoptions;
+		bool isnull;
+		Oid parentoid = linitial_oid(inheritOids);
+		HeapTuple tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(parentoid));
+		if (!HeapTupleIsValid(tuple))
+			elog(ERROR, "cache lookup failed for relation %u", parentoid);
+
+		oldoptions = SysCacheGetAttr(RELOID, tuple, Anum_pg_class_reloptions,
+								&isnull);
+
+		reloptions = transformRelOptions(isnull ? 0 : oldoptions,
+				stmt->options, NULL, validnsps, true, false);
+
+		ReleaseSysCache(tuple);
+	} else
+		reloptions = transformRelOptions((Datum) 0, stmt->options, NULL,
+				validnsps, true, false);
 
 	switch (relkind)
 	{
 		case RELKIND_VIEW:
 			(void) view_reloptions(reloptions, true);
 			break;
-		case RELKIND_PARTITIONED_TABLE:
-			(void) partitioned_table_reloptions(reloptions, true);
-			break;
 		default:
 			(void) heap_reloptions(relkind, reloptions, true);
 	}
@@ -4155,7 +4171,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 		case AT_SetRelOptions:	/* SET (...) */
 		case AT_ResetRelOptions:	/* RESET (...) */
 		case AT_ReplaceRelOptions:	/* reset them all, then set just these */
-			ATSimplePermissions(rel, ATT_TABLE | ATT_VIEW | ATT_MATVIEW | ATT_INDEX);
+			ATSimplePermissions(rel, ATT_TABLE | ATT_VIEW | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX);
 			/* This command never recurses */
 			/* No command-specific prep needed */
 			pass = AT_PASS_MISC;
@@ -12693,10 +12709,8 @@ 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);
-			break;
 		case RELKIND_PARTITIONED_TABLE:
-			(void) partitioned_table_reloptions(newOptions, true);
+			(void) heap_reloptions(rel->rd_rel->relkind, newOptions, true);
 			break;
 		case RELKIND_VIEW:
 			(void) view_reloptions(newOptions, true);
diff --git a/src/test/regress/expected/reloptions.out b/src/test/regress/expected/reloptions.out
index 44c1304..8fc8b62 100644
--- a/src/test/regress/expected/reloptions.out
+++ b/src/test/regress/expected/reloptions.out
@@ -222,3 +222,32 @@ SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test_idx3'::regclass;
  {fillfactor=40}
 (1 row)
 
+-- For partitioned tables/indexes, expect ALTER..SET fillfactor not to recurse,
+-- but rather to set a value in the parent table which is used for new children
+CREATE TABLE par(i int) PARTITION BY RANGE(i);
+CREATE INDEX par_i_idx ON par(i);
+CREATE TABLE par1 PARTITION OF par FOR VALUES FROM (1)TO(2);
+ALTER TABLE par SET (fillfactor=13);
+ALTER INDEX par_i_idx SET (fillfactor=13);
+CREATE TABLE par2 PARTITION OF par FOR VALUES FROM (2)TO(3);
+\d+ par1
+                                   Table "public.par1"
+ Column |  Type   | Collation | Nullable | Default | Storage | Stats target | Description 
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ i      | integer |           |          |         | plain   |              | 
+Partition of: par FOR VALUES FROM (1) TO (2)
+Partition constraint: ((i IS NOT NULL) AND (i >= 1) AND (i < 2))
+Indexes:
+    "par1_i_idx" btree (i)
+
+\d+ par2
+                                   Table "public.par2"
+ Column |  Type   | Collation | Nullable | Default | Storage | Stats target | Description 
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ i      | integer |           |          |         | plain   |              | 
+Partition of: par FOR VALUES FROM (2) TO (3)
+Partition constraint: ((i IS NOT NULL) AND (i >= 2) AND (i < 3))
+Indexes:
+    "par2_i_idx" btree (i) WITH (fillfactor='13')
+Options: fillfactor=13
+
diff --git a/src/test/regress/sql/reloptions.sql b/src/test/regress/sql/reloptions.sql
index cac5b0b..804523c 100644
--- a/src/test/regress/sql/reloptions.sql
+++ b/src/test/regress/sql/reloptions.sql
@@ -128,3 +128,14 @@ SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test_idx'::regclass;
 CREATE INDEX reloptions_test_idx3 ON reloptions_test (s);
 ALTER INDEX reloptions_test_idx3 SET (fillfactor=40);
 SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test_idx3'::regclass;
+
+-- For partitioned tables/indexes, expect ALTER..SET fillfactor not to recurse,
+-- but rather to set a value in the parent table which is used for new children
+CREATE TABLE par(i int) PARTITION BY RANGE(i);
+CREATE INDEX par_i_idx ON par(i);
+CREATE TABLE par1 PARTITION OF par FOR VALUES FROM (1)TO(2);
+ALTER TABLE par SET (fillfactor=13);
+ALTER INDEX par_i_idx SET (fillfactor=13);
+CREATE TABLE par2 PARTITION OF par FOR VALUES FROM (2)TO(3);
+\d par1
+\d par2
-- 
2.7.4


--l76fUT7nc3MelDdI--