Re: ALTER INDEX fails on partitioned index
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
> 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_INDEX) &&
> + !(allowed_targets_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
Re: ALTER INDEX fails on partitioned index
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
signature.asc
Description: PGP signature
Re: ALTER INDEX fails on partitioned index
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: ALTER INDEX fails on partitioned index
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
>From e5bb363f514d768a4f540d9c82ad5745944b1486 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
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_INDEX) &&
+ !(allowed_targets_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
>From 5f7f79c4c8e85528e88ffa0b749faad1da4ab4d5 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
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,
Re: ALTER INDEX fails on partitioned index
On Thu, Dec 26, 2019 at 10:52 PM Justin Pryzby 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
Re: ALTER INDEX fails on partitioned index
On Mon, Jan 07, 2019 at 04:23:30PM -0300, Alvaro Herrera wrote: > On 2019-Jan-05, Justin Pryzby wrote: > > postgres=# CREATE TABLE t(i int)PARTITION BY RANGE(i); > > postgres=# CREATE INDEX ON t(i) WITH(fillfactor=11); > > postgres=# ALTER INDEX t_i_idx SET (fillfactor=12); > > ERROR: 42809: "t_i_idx" is not a table, view, materialized view, or index > > LOCATION: ATWrongRelkindError, tablecmds.c:5031 > > > > I can't see that's deliberate, > > Well, I deliberately ignored that aspect of the report at the time as it > seemed to me (per discussion in thread [1]) that this behavior was > intentional. However, if I think in terms of things like > pages_per_range in BRIN indexes, this decision seems to be a mistake, > because surely we should propagate that value to children. > > [1] > https://www.postgresql.org/message-id/flat/CAH2-WzkOKptQiE51Bh4_xeEHhaBwHkZkGtKizrFMgEkfUuRRQg%40mail.gmail.com 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; -- Justin Pryzby System Administrator Telsasoft +1-952-707-8581 >From fb05137c2c8a59dcab8fbd25fdee80e976588261 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Thu, 26 Dec 2019 21:40:06 -0600 Subject: [PATCH v1] Allow ALTER INDEX SET () on partitioned indexes --- src/backend/commands/tablecmds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d9f13da..aea84d5 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -4067,7 +4067,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, 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; -- 2.7.4
Re: ALTER INDEX fails on partitioned index
On 2019-Jan-05, Justin Pryzby wrote: > 12dev and 11.1: > > postgres=# CREATE TABLE t(i int)PARTITION BY RANGE(i); > postgres=# CREATE INDEX ON t(i) WITH(fillfactor=11); > postgres=# ALTER INDEX t_i_idx SET (fillfactor=12); > ERROR: 42809: "t_i_idx" is not a table, view, materialized view, or index > LOCATION: ATWrongRelkindError, tablecmds.c:5031 > > I can't see that's deliberate, So do you have a proposed patch? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: ALTER INDEX fails on partitioned index
On Mon, Jan 07, 2019 at 01:34:08PM -0600, Justin Pryzby wrote: > I don't see any discussion regarding ALTER (?) > > Actually, I ran into this while trying to set pages_per_range. > But shouldn't it also work for fillfactor ? Like ALTER TABLE, the take for ALTER INDEX is that we are still lacking a ALTER INDEX ONLY flavor which would apply only to single partitioned indexes instead of applying it down to a full set of partitions below the partitioned entry on which the DDL is defined. That would be useful for SET STATISTICS as well. So Alvaro's decision looks right to me as of what has been done in v11. -- Michael signature.asc Description: PGP signature
Re: ALTER INDEX fails on partitioned index
On Mon, Jan 07, 2019 at 04:23:30PM -0300, Alvaro Herrera wrote: > On 2019-Jan-05, Justin Pryzby wrote: > > > 12dev and 11.1: > > > > postgres=# CREATE TABLE t(i int)PARTITION BY RANGE(i); > > postgres=# CREATE INDEX ON t(i) WITH(fillfactor=11); > > postgres=# ALTER INDEX t_i_idx SET (fillfactor=12); > > ERROR: 42809: "t_i_idx" is not a table, view, materialized view, or index > > LOCATION: ATWrongRelkindError, tablecmds.c:5031 > > > > I can't see that's deliberate, > > Well, I deliberately ignored that aspect of the report at the time as it > seemed to me (per discussion in thread [1]) that this behavior was > intentional. However, if I think in terms of things like > pages_per_range in BRIN indexes, this decision seems to be a mistake, > because surely we should propagate that value to children. > > [1] > https://www.postgresql.org/message-id/flat/CAH2-WzkOKptQiE51Bh4_xeEHhaBwHkZkGtKizrFMgEkfUuRRQg%40mail.gmail.com I don't see any discussion regarding ALTER (?) Actually, I ran into this while trying to set pages_per_range. But shouldn't it also work for fillfactor ? Thanks, Justin
Re: ALTER INDEX fails on partitioned index
On 2019-Jan-05, Justin Pryzby wrote: > 12dev and 11.1: > > postgres=# CREATE TABLE t(i int)PARTITION BY RANGE(i); > postgres=# CREATE INDEX ON t(i) WITH(fillfactor=11); > postgres=# ALTER INDEX t_i_idx SET (fillfactor=12); > ERROR: 42809: "t_i_idx" is not a table, view, materialized view, or index > LOCATION: ATWrongRelkindError, tablecmds.c:5031 > > I can't see that's deliberate, Well, I deliberately ignored that aspect of the report at the time as it seemed to me (per discussion in thread [1]) that this behavior was intentional. However, if I think in terms of things like pages_per_range in BRIN indexes, this decision seems to be a mistake, because surely we should propagate that value to children. [1] https://www.postgresql.org/message-id/flat/CAH2-WzkOKptQiE51Bh4_xeEHhaBwHkZkGtKizrFMgEkfUuRRQg%40mail.gmail.com -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
ALTER INDEX fails on partitioned index
12dev and 11.1: postgres=# CREATE TABLE t(i int)PARTITION BY RANGE(i); postgres=# CREATE INDEX ON t(i) WITH(fillfactor=11); postgres=# ALTER INDEX t_i_idx SET (fillfactor=12); ERROR: 42809: "t_i_idx" is not a table, view, materialized view, or index LOCATION: ATWrongRelkindError, tablecmds.c:5031 I can't see that's deliberate, but I found an earlier problem report; however, discussion regarding the ALTER behavior seems to have been eclipsed due to 2nd, separate issue with pageinspect. https://www.postgresql.org/message-id/flat/CAKcux6mb6AZjMVyohnta6M%2BfdkUB720Gq8Wb6KPZ24FPDs7qzg%40mail.gmail.com Justin
