pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedALTER INDEX fails on partitioned index
11+ messages / 4 participants
[nested] [flat]
* ALTER INDEX fails on partitioned index
@ 2019-01-05 18:59 Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 2 replies; 11+ messages in thread
From: Justin Pryzby @ 2019-01-05 18:59 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
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%40m...
Justin
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: ALTER INDEX fails on partitioned index
@ 2019-01-07 19:23 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 2 replies; 11+ messages in thread
From: Alvaro Herrera @ 2019-01-07 19:23 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers
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%40mai...
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: ALTER INDEX fails on partitioned index
@ 2019-01-07 19:34 Justin Pryzby <pryzby@telsasoft.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 11+ messages in thread
From: Justin Pryzby @ 2019-01-07 19:34 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers
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%40mai...
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: ALTER INDEX fails on partitioned index
@ 2019-01-08 01:24 Michael Paquier <michael@paquier.xyz>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 0 replies; 11+ messages in thread
From: Michael Paquier @ 2019-01-08 01:24 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-hackers
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190108012455.GG22498@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: ALTER INDEX fails on partitioned index
@ 2019-02-06 17:32 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 11+ messages in thread
From: Alvaro Herrera @ 2019-02-06 17:32 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: ALTER INDEX fails on partitioned index
@ 2019-12-27 03:51 Justin Pryzby <pryzby@telsasoft.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 11+ messages in thread
From: Justin Pryzby @ 2019-12-27 03:51 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers
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%40mai...
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
Attachments:
[text/x-diff] v1-0001-Allow-ALTER-INDEX-SET-on-partitioned-indexes.patch (999B, ../../20191227035157.GD12890@telsasoft.com/2-v1-0001-Allow-ALTER-INDEX-SET-on-partitioned-indexes.patch)
download | inline diff:
From fb05137c2c8a59dcab8fbd25fdee80e976588261 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: ALTER INDEX fails on partitioned index
@ 2019-12-29 12:43 Robert Haas <robertmhaas@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 11+ messages in thread
From: Robert Haas @ 2019-12-29 12:43 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: ALTER INDEX fails on partitioned index
@ 2020-02-27 23:25 Justin Pryzby <pryzby@telsasoft.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 3 replies; 11+ messages in thread
From: Justin Pryzby @ 2020-02-27 23:25 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-hackers
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
Attachments:
[text/x-diff] v2-0001-More-specific-error-message-when-failing-to-alter.patch (2.7K, ../../20200227232513.GE29456@telsasoft.com/2-v2-0001-More-specific-error-message-when-failing-to-alter.patch)
download | inline diff:
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
[text/x-diff] v2-0002-Allow-reloptions-on-partitioned-tables-and-indexe.patch (7.1K, ../../20200227232513.GE29456@telsasoft.com/3-v2-0002-Allow-reloptions-on-partitioned-tables-and-indexe.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: ALTER INDEX fails on partitioned index
@ 2020-02-28 00:11 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
2 siblings, 0 replies; 11+ messages in thread
From: Alvaro Herrera @ 2020-02-28 00:11 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: ALTER INDEX fails on partitioned index
@ 2020-02-28 08:09 Michael Paquier <michael@paquier.xyz>
parent: Justin Pryzby <pryzby@telsasoft.com>
2 siblings, 0 replies; 11+ messages in thread
From: Michael Paquier @ 2020-02-28 08:09 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-hackers
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200228080916.GG2688@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: ALTER INDEX fails on partitioned index
@ 2020-03-23 21:47 Justin Pryzby <pryzby@telsasoft.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
2 siblings, 0 replies; 11+ messages in thread
From: Justin Pryzby @ 2020-03-23 21:47 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-hackers
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%40ma...
> ---
> 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
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2020-03-23 21:47 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-01-05 18:59 ALTER INDEX fails on partitioned index Justin Pryzby <pryzby@telsasoft.com>
2019-01-07 19:23 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-01-07 19:34 ` Justin Pryzby <pryzby@telsasoft.com>
2019-01-08 01:24 ` Michael Paquier <michael@paquier.xyz>
2019-12-27 03:51 ` Justin Pryzby <pryzby@telsasoft.com>
2019-12-29 12:43 ` Robert Haas <robertmhaas@gmail.com>
2020-02-27 23:25 ` Justin Pryzby <pryzby@telsasoft.com>
2020-02-28 00:11 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-02-28 08:09 ` Michael Paquier <michael@paquier.xyz>
2020-03-23 21:47 ` Justin Pryzby <pryzby@telsasoft.com>
2019-02-06 17:32 ` Alvaro Herrera <alvherre@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox