Re: ALTER INDEX fails on partitioned index

On Thu, Dec 26, 2019 at 10:52 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

Possibly attached should be backpatched through v11 ?

This allows SET on the parent index, which is used for newly created child
indexes, but doesn't itself recurse to children.

I noticed recursive "*" doesn't seem to be allowed for "alter INDEX":
postgres=# ALTER INDEX p_i2* SET (fillfactor = 22);
ERROR: syntax error at or near "*"
LINE 1: ALTER INDEX p_i2* SET (fillfactor = 22);

Also, I noticed this "doesn't fail", but setting is neither recursively applied
nor used for new partitions.

postgres=# ALTER INDEX p_i_idx ALTER COLUMN 1 SET STATISTICS 123;

Seems a little hard to believe that this needs no other code changes.
And what about documentation updates?

BTW, if we don't do this, we should at least try to improve the error
message. Telling somebody that something they created using CREATE
INDEX is not an index will not win us any friends. A more specific
error message, saying that the operation is not supported for
partitioned indexes, seems better.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 2020-Feb-27, Justin Pryzby wrote:

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.

I think ALTER not acting recursively is a bug that we would do well not
to propagate any further. Enough effort we have to spend trying to fix
that already. Let's add ALTER .. ONLY if needed.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Feb 27, 2020 at 05:25:13PM -0600, Justin Pryzby wrote:

/*
- * 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);
-}

Please don't undo that. You can look at 1bbd608 for all the details.
--
Michael

On Thu, Feb 27, 2020 at 09:11:14PM -0300, Alvaro Herrera wrote:

On 2020-Feb-27, Justin Pryzby wrote:

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.

I think ALTER not acting recursively is a bug that we would do well not
to propagate any further. Enough effort we have to spend trying to fix
that already. Let's add ALTER .. ONLY if needed.

I was modeling after the behavior for tablespaces, and didn't realize that
non-recursive alter was considered discouraged.

On Thu, Feb 27, 2020 at 05:25:13PM -0600, Justin Pryzby wrote:

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

Is there any interest in this one ?

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

/messages/by-id/CA+Tgmobq8_-DS7qDEmMi-4ARP1_0bkgFEjYfiK97L2eXq+Q+nw@mail.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

--
Justin