Обсуждение: 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
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 Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
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
Вложения
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 Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
Вложения
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 > > 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 > -- Justin