partitioned indexes and tablespaces
Hello
A customer reported to us that partitioned indexes are not working
consistently with tablespaces:
1. When a CREATE INDEX specifies a tablespace, existing partitions get
the index in the correct tablespace; however, the parent index itself
does not record the tablespace. So when new partitions are created
later, they get the index in the default tablespace instead of the
specified tablespace. Fix by saving the tablespace in the pg_class row
for the parent index.
2. ALTER TABLE SET TABLESPACE, applied to the partitioned index, would
raise an error indicating that it's not the correct relation kind. In
order for this to actually work, we need bespoke code for ATExecCmd();
the code for all other relation kinds wants to move storage (and runs in
Phase 3, later), but these indexes do not have that. Therefore, write a
cut-down version which is invoked directly in ATExecCmd instead.
3. ALTER INDEX ALL IN TABLESPACE, identical problem, is also fixed by
the above change.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Fix-tablespace-handling-for-partitioned-indexes.patchtext/x-diff; charset=us-asciiDownload
From 819dd64baf04d39f02785cde72b6a4c33fd10f3b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 1 Nov 2018 21:29:56 -0300
Subject: [PATCH] Fix tablespace handling for partitioned indexes
When creating partitioned indexes, the tablespace was not being saved
for the parent index. This meant that subsequently created partitions
would not use the right tablespace for their indexes.
ALTER INDEX SET TABLESPACE and ALTER INDEX ALL IN TABLESPACE also raised
errors when tried; fix them too.
---
src/backend/catalog/heap.c | 10 ++++-
src/backend/commands/tablecmds.c | 61 +++++++++++++++++++++++++++++--
src/test/regress/input/tablespace.source | 10 +++++
src/test/regress/output/tablespace.source | 19 +++++++++-
4 files changed, 95 insertions(+), 5 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 8c52a1543d..61ce44830b 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -297,7 +297,6 @@ heap_create(const char *relname,
case RELKIND_COMPOSITE_TYPE:
case RELKIND_FOREIGN_TABLE:
case RELKIND_PARTITIONED_TABLE:
- case RELKIND_PARTITIONED_INDEX:
create_storage = false;
/*
@@ -306,6 +305,15 @@ heap_create(const char *relname,
*/
reltablespace = InvalidOid;
break;
+
+ case RELKIND_PARTITIONED_INDEX:
+ /*
+ * Preserve tablespace so that it's used as tablespace for indexes
+ * on future partitions.
+ */
+ create_storage = false;
+ break;
+
case RELKIND_SEQUENCE:
create_storage = true;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 357c73073d..514c1c9d49 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -446,6 +446,8 @@ static bool ATPrepChangePersistence(Relation rel, bool toLogged);
static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel,
const char *tablespacename, LOCKMODE lockmode);
static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode);
+static void ATExecPartedIdxSetTableSpace(Relation rel, Oid newTableSpace);
+
static void ATExecSetRelOptions(Relation rel, List *defList,
AlterTableType operation,
LOCKMODE lockmode);
@@ -3845,7 +3847,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_DROP;
break;
case AT_SetTableSpace: /* SET TABLESPACE */
- ATSimplePermissions(rel, ATT_TABLE | ATT_MATVIEW | ATT_INDEX);
+ ATSimplePermissions(rel, ATT_TABLE | ATT_MATVIEW | ATT_INDEX |
+ ATT_PARTITIONED_INDEX);
/* This command never recurses */
ATPrepSetTableSpace(tab, rel, cmd->name, lockmode);
pass = AT_PASS_MISC; /* doesn't actually matter */
@@ -4181,10 +4184,13 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
*/
break;
case AT_SetTableSpace: /* SET TABLESPACE */
-
/*
- * Nothing to do here; Phase 3 does the work
+ * Only do this for partitioned indexes, for which this is just
+ * a catalog change. Other relation types are handled by Phase 3.
*/
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+ ATExecPartedIdxSetTableSpace(rel, tab->newTableSpace);
+
break;
case AT_SetRelOptions: /* SET (...) */
case AT_ResetRelOptions: /* RESET (...) */
@@ -11050,6 +11056,55 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
}
/*
+ * Special handling of ALTER TABLE SET TABLESPACE for partitioned indexes,
+ * which have no storage (so not handled in Phase 3 like other relation types)
+ */
+static void
+ATExecPartedIdxSetTableSpace(Relation rel, Oid newTableSpace)
+{
+ HeapTuple tuple;
+ Oid oldTableSpace;
+ Relation pg_class;
+ Form_pg_class rd_rel;
+ Oid indexOid = RelationGetRelid(rel);
+
+ Assert(rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX);
+
+ /*
+ * No work if no change in tablespace.
+ */
+ oldTableSpace = rel->rd_rel->reltablespace;
+ if (newTableSpace == oldTableSpace ||
+ (newTableSpace == MyDatabaseTableSpace && oldTableSpace == 0))
+ {
+ InvokeObjectPostAlterHook(RelationRelationId,
+ indexOid, 0);
+ return;
+ }
+
+ /* Get a modifiable copy of the relation's pg_class row */
+ pg_class = heap_open(RelationRelationId, RowExclusiveLock);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", indexOid);
+ rd_rel = (Form_pg_class) GETSTRUCT(tuple);
+
+ /* update the pg_class row */
+ rd_rel->reltablespace = (newTableSpace == MyDatabaseTableSpace) ? InvalidOid : newTableSpace;
+ CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+ InvokeObjectPostAlterHook(RelationRelationId, indexOid, 0);
+
+ heap_freetuple(tuple);
+
+ heap_close(pg_class, RowExclusiveLock);
+
+ /* Make sure the reltablespace change is visible */
+ CommandCounterIncrement();
+}
+
+/*
* Alter Table ALL ... SET TABLESPACE
*
* Allows a user to move all objects of some type in a given tablespace in the
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index 7f7934b745..60c87261db 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -44,6 +44,14 @@ CREATE INDEX foo_idx on testschema.foo(i) TABLESPACE regress_tblspace;
SELECT relname, spcname FROM pg_catalog.pg_tablespace t, pg_catalog.pg_class c
where c.reltablespace = t.oid AND c.relname = 'foo_idx';
+-- partitioned index
+CREATE TABLE testschema.part (a int) PARTITION BY LIST (a);
+CREATE TABLE testschema.part1 PARTITION OF testschema.part FOR VALUES IN (1);
+CREATE INDEX part_a_idx ON testschema.part (a) TABLESPACE regress_tblspace;
+CREATE TABLE testschema.part2 PARTITION OF testschema.part FOR VALUES IN (2);
+SELECT relname, spcname FROM pg_catalog.pg_tablespace t, pg_catalog.pg_class c
+ where c.reltablespace = t.oid AND c.relname LIKE 'part%_idx';
+
-- check that default_tablespace doesn't affect ALTER TABLE index rebuilds
CREATE TABLE testschema.test_default_tab(id bigint) TABLESPACE regress_tblspace;
INSERT INTO testschema.test_default_tab VALUES (1);
@@ -93,6 +101,8 @@ CREATE UNIQUE INDEX anindex ON testschema.atable(column1);
ALTER TABLE testschema.atable SET TABLESPACE regress_tblspace;
ALTER INDEX testschema.anindex SET TABLESPACE regress_tblspace;
+ALTER INDEX testschema.part_a_idx SET TABLESPACE pg_default;
+ALTER INDEX testschema.part_a_idx SET TABLESPACE regress_tblspace;
INSERT INTO testschema.atable VALUES(3); -- ok
INSERT INTO testschema.atable VALUES(1); -- fail (checks index)
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index fe3614cd76..43962e6f01 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -61,6 +61,20 @@ SELECT relname, spcname FROM pg_catalog.pg_tablespace t, pg_catalog.pg_class c
foo_idx | regress_tblspace
(1 row)
+-- partitioned index
+CREATE TABLE testschema.part (a int) PARTITION BY LIST (a);
+CREATE TABLE testschema.part1 PARTITION OF testschema.part FOR VALUES IN (1);
+CREATE INDEX part_a_idx ON testschema.part (a) TABLESPACE regress_tblspace;
+CREATE TABLE testschema.part2 PARTITION OF testschema.part FOR VALUES IN (2);
+SELECT relname, spcname FROM pg_catalog.pg_tablespace t, pg_catalog.pg_class c
+ where c.reltablespace = t.oid AND c.relname LIKE 'part%_idx';
+ relname | spcname
+-------------+------------------
+ part1_a_idx | regress_tblspace
+ part2_a_idx | regress_tblspace
+ part_a_idx | regress_tblspace
+(3 rows)
+
-- check that default_tablespace doesn't affect ALTER TABLE index rebuilds
CREATE TABLE testschema.test_default_tab(id bigint) TABLESPACE regress_tblspace;
INSERT INTO testschema.test_default_tab VALUES (1);
@@ -200,6 +214,8 @@ CREATE TABLE testschema.atable AS VALUES (1), (2);
CREATE UNIQUE INDEX anindex ON testschema.atable(column1);
ALTER TABLE testschema.atable SET TABLESPACE regress_tblspace;
ALTER INDEX testschema.anindex SET TABLESPACE regress_tblspace;
+ALTER INDEX testschema.part_a_idx SET TABLESPACE pg_default;
+ALTER INDEX testschema.part_a_idx SET TABLESPACE regress_tblspace;
INSERT INTO testschema.atable VALUES(3); -- ok
INSERT INTO testschema.atable VALUES(1); -- fail (checks index)
ERROR: duplicate key value violates unique constraint "anindex"
@@ -241,10 +257,11 @@ NOTICE: no matching relations in tablespace "regress_tblspace_renamed" found
-- Should succeed
DROP TABLESPACE regress_tblspace_renamed;
DROP SCHEMA testschema CASCADE;
-NOTICE: drop cascades to 5 other objects
+NOTICE: drop cascades to 6 other objects
DETAIL: drop cascades to table testschema.foo
drop cascades to table testschema.asselect
drop cascades to table testschema.asexecute
+drop cascades to table testschema.part
drop cascades to table testschema.atable
drop cascades to table testschema.tablespace_acl
DROP ROLE regress_tablespace_user1;
--
2.11.0
On Thu, Nov 01, 2018 at 09:31:38PM -0300, Alvaro Herrera wrote:
A customer reported to us that partitioned indexes are not working
consistently with tablespaces:
Let's see...
1. When a CREATE INDEX specifies a tablespace, existing partitions get
the index in the correct tablespace; however, the parent index itself
does not record the tablespace. So when new partitions are created
later, they get the index in the default tablespace instead of the
specified tablespace. Fix by saving the tablespace in the pg_class row
for the parent index.
I may be missing something of course... But partitioned tables don't
register the tablespace they are on either so as it cannot be used by
any partitions created on it:
=# create tablespace popo location '/home/ioltas/data/tbspace';
CREATE TABLESPACE
=# create table aa (a int) partition by list (a) tablespace popo;
CREATE TABLE
=# create table aa_1 partition of aa for values in (1) tablespace popo;
CREATE TABLE
=# create table aa_2 partition of aa for values in (2);
CREATE TABLE
=# select t.spcname, c.relname from pg_class c, pg_tablespace t
where c.oid > 16000 and c.reltablespace = t.oid;
spcname | relname
---------+---------
popo | aa_1
(1 row)
It seems to me that the current behavior is wanted in this case, because
partitioned tables and partitioned indexes have no physical storage.
2. ALTER TABLE SET TABLESPACE, applied to the partitioned index, would
raise an error indicating that it's not the correct relation kind. In
order for this to actually work, we need bespoke code for ATExecCmd();
the code for all other relation kinds wants to move storage (and runs in
Phase 3, later), but these indexes do not have that. Therefore, write a
cut-down version which is invoked directly in ATExecCmd instead.
Using the previous example, this does not raise an error:
alter table aa set tablespace popo;
However the reference to reltablespace in pg_class is not changed. So I
would agree with your point to not raise an error and back-patch that,
but I don't agree with the point of changing reltablespace for a
partitioned index if that's what you mean.
3. ALTER INDEX ALL IN TABLESPACE, identical problem, is also fixed by
the above change.
Reproducible with just the following stuff on top of the previous
example:
create index aai on aa(a);
alter index all in tablespace pg_default set tablespace popo;
In this case also raising an error is a bug, it seems to me that
partitioned indexes should just be ignored.
Could you add an entry in the next CF to not lose track of what is
discussed here?
--
Michael
Hi,
On 2018/11/02 10:27, Michael Paquier wrote:
On Thu, Nov 01, 2018 at 09:31:38PM -0300, Alvaro Herrera wrote:
A customer reported to us that partitioned indexes are not working
consistently with tablespaces:Let's see...
1. When a CREATE INDEX specifies a tablespace, existing partitions get
the index in the correct tablespace; however, the parent index itself
does not record the tablespace. So when new partitions are created
later, they get the index in the default tablespace instead of the
specified tablespace. Fix by saving the tablespace in the pg_class row
for the parent index.I may be missing something of course... But partitioned tables don't
register the tablespace they are on either so as it cannot be used by
any partitions created on it:
=# create tablespace popo location '/home/ioltas/data/tbspace';
CREATE TABLESPACE
=# create table aa (a int) partition by list (a) tablespace popo;
CREATE TABLE
=# create table aa_1 partition of aa for values in (1) tablespace popo;
CREATE TABLE
=# create table aa_2 partition of aa for values in (2);
CREATE TABLE
=# select t.spcname, c.relname from pg_class c, pg_tablespace t
where c.oid > 16000 and c.reltablespace = t.oid;
spcname | relname
---------+---------
popo | aa_1
(1 row)It seems to me that the current behavior is wanted in this case, because
partitioned tables and partitioned indexes have no physical storage.
Keith Fiske complained about this behavior for partitioned *tables* a few
months ago, which led to the following discussion:
/messages/by-id/CAKJS1f9PXYcT+j=oyL-Lquz=ScNwpRtmD7u9svLASUygBdbN8w@mail.gmail.com
It's Michael's message that was the last one on that thread. :)
/messages/by-id/20180413224007.GB27295@paquier.xyz
By the way, if we decide to do something about this, I think we do the
same for partitioned tables. There are more than one interesting
behaviors possible that are mentioned in the above thread for when
parent's reltablespace is set/changed. For example, when it's set, the
existing partitions are not moved, but the new value only applies to
subsequently created partitions. Considering various such behaviors, this
would seem like new feature work, which I'd suppose would only be
considered for 12.
2. ALTER TABLE SET TABLESPACE, applied to the partitioned index, would
raise an error indicating that it's not the correct relation kind. In
order for this to actually work, we need bespoke code for ATExecCmd();
the code for all other relation kinds wants to move storage (and runs in
Phase 3, later), but these indexes do not have that. Therefore, write a
cut-down version which is invoked directly in ATExecCmd instead.Using the previous example, this does not raise an error:
alter table aa set tablespace popo;
However the reference to reltablespace in pg_class is not changed. So I
would agree with your point to not raise an error and back-patch that,
but I don't agree with the point of changing reltablespace for a
partitioned index if that's what you mean.3. ALTER INDEX ALL IN TABLESPACE, identical problem, is also fixed by
the above change.Reproducible with just the following stuff on top of the previous
example:
create index aai on aa(a);
alter index all in tablespace pg_default set tablespace popo;In this case also raising an error is a bug, it seems to me that
partitioned indexes should just be ignored.
Same argument here too.
IOW, I agree with Michael that if something will be back-patched to 11, it
should be a small patch to make the unsupported relkind error go away.
Thanks,
Amit
On 2018-Nov-02, Michael Paquier wrote:
On Thu, Nov 01, 2018 at 09:31:38PM -0300, Alvaro Herrera wrote:
1. When a CREATE INDEX specifies a tablespace, existing partitions get
the index in the correct tablespace; however, the parent index itself
does not record the tablespace. So when new partitions are created
later, they get the index in the default tablespace instead of the
specified tablespace. Fix by saving the tablespace in the pg_class row
for the parent index.I may be missing something of course... But partitioned tables don't
register the tablespace they are on either so as it cannot be used by
any partitions created on it:
This is not relevant to my case, IMO. Partitioned tables are explicitly
created each time, with their own parameters; if you want to specify the
tablespace in which it is created, you can do so at that point. This is
not the case with partitioned indexes, because they are created
automatically at CREATE TABLE PARTITION OF time, without an option to
specify where each index goes.
It seems to me that the current behavior is wanted in this case, because
partitioned tables and partitioned indexes have no physical storage.
Well, I designed the partitioned indexes feature and I can say for
certain that this behavior was not explicitly designed in, but was just
a oversight.
2. ALTER TABLE SET TABLESPACE, applied to the partitioned index, would
raise an error indicating that it's not the correct relation kind.Using the previous example, this does not raise an error:
alter table aa set tablespace popo;
However the reference to reltablespace in pg_class is not changed. So I
would agree with your point to not raise an error and back-patch that,
but I don't agree with the point of changing reltablespace for a
partitioned index if that's what you mean.
Same argument here. The pg_class record for the partitioned index
serves to guide the storage of indexes on future partitions, so it is
valuable to have it. Not recording the tablespace (and not allowing it
to be changed afterwards) is a usability fail.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-Nov-02, Amit Langote wrote:
On 2018/11/02 10:27, Michael Paquier wrote:
It seems to me that the current behavior is wanted in this case, because
partitioned tables and partitioned indexes have no physical storage.Keith Fiske complained about this behavior for partitioned *tables* a few
months ago, which led to the following discussion:/messages/by-id/CAKJS1f9PXYcT+j=oyL-Lquz=ScNwpRtmD7u9svLASUygBdbN8w@mail.gmail.com
It's Michael's message that was the last one on that thread. :)
I agree with Fiske, FWIW. I think the current behavior results because
people (including me) overlooked things, not because it was designed
explicitly that way.
By the way, if we decide to do something about this, I think we do the
same for partitioned tables.
I'm up for changing the behavior of partitioned tables in pg12 (please
send a patch), but I'm up for changing the behavior of partitioned
tables in pg11.
There are more than one interesting
behaviors possible that are mentioned in the above thread for when
parent's reltablespace is set/changed.
I'm *NOT* proposing to move existing partitions to another tablespace,
in any case.
IOW, I agree with Michael that if something will be back-patched to 11, it
should be a small patch to make the unsupported relkind error go away.
I don't.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Nov 2, 2018 at 11:02 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
By the way, if we decide to do something about this, I think we do the
same for partitioned tables.I'm up for changing the behavior of partitioned tables in pg12 (please
send a patch), but I'm up for changing the behavior of partitioned
tables in pg11.
Uh, what?
I strongly object to inserting behavior changes into released branches
on the grounds that the behavior wasn't considered carefully enough
before feature freeze. If we accept that as a justification, then
anybody can claim that any behavior change should be back-patched at
any time as long as they were the author of the original patch. But
that's not a recipe for a stable product. There's got to be a point
at which we say, yeah, you know, this is maybe not the perfect set of
design decisions, but we're going to support the decisions we made for
the next 5 years. And maybe we'll make better ones in the next
release.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2018-Nov-02, Robert Haas wrote:
I strongly object to inserting behavior changes into released branches
on the grounds that the behavior wasn't considered carefully enough
before feature freeze.
I'm not proposing to change any stable behavior. The thing I'm
proposing to change clearly cannot be used by anyone, because it just
throws errors.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-Nov-02, Robert Haas wrote:
On Fri, Nov 2, 2018 at 11:02 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
By the way, if we decide to do something about this, I think we do the
same for partitioned tables.I'm up for changing the behavior of partitioned tables in pg12 (please
send a patch), but I'm up for changing the behavior of partitioned
tables in pg11.Uh, what?
Sorry, I just realized the typo in the above. The behavior I propose to
change in pg11 is that of partitioned *indexes*, not tables.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Nov 2, 2018 at 12:05 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2018-Nov-02, Robert Haas wrote:
I strongly object to inserting behavior changes into released branches
on the grounds that the behavior wasn't considered carefully enough
before feature freeze.I'm not proposing to change any stable behavior. The thing I'm
proposing to change clearly cannot be used by anyone, because it just
throws errors.
I don't get it. If the default tablespace for partition is set as for
a regular table, that is a usable behavior. If it is taken from the
parent table, that is a different and also usable behavior. Isn't
this part of what you are proposing to change?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2018-Nov-02, Robert Haas wrote:
On Fri, Nov 2, 2018 at 12:05 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2018-Nov-02, Robert Haas wrote:
I strongly object to inserting behavior changes into released branches
on the grounds that the behavior wasn't considered carefully enough
before feature freeze.I'm not proposing to change any stable behavior. The thing I'm
proposing to change clearly cannot be used by anyone, because it just
throws errors.I don't get it. If the default tablespace for partition is set as for
a regular table, that is a usable behavior. If it is taken from the
parent table, that is a different and also usable behavior. Isn't
this part of what you are proposing to change?
In this thread I'm not proposing to change the behavior for tables, only
for indexes. If people want to change behavior for tables (and I agree
with doing so), they can start their own threads.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Nov 02, 2018 at 03:53:51PM -0300, Alvaro Herrera wrote:
In this thread I'm not proposing to change the behavior for tables, only
for indexes. If people want to change behavior for tables (and I agree
with doing so), they can start their own threads.
Changing this behavior on back-branches is not a good idea I think
because that changes the representation of the pg_class entries in the
catalogs by adding the reltablespace reference for a partitioned index
which does not exist now. We are long past the time of doing such
changes on v11, but that can perfectly be done for v12.
Making the tablespace inherited from the parent if the parent has no
information on disk is sensible in my opinion, so please let's consider
that as a feature but not a bug, and also let's do the change for both
partitioned tables *and* partitioned indexes for consistency. The
current behavior does not prevent the features to work.
So what I would suggest is to fix the commands which are failing by not
ignoring partitioned indexes for v11, then raise a new thread which
implements the new tablespace behavior for all partitioned objects. Per
my recent lookups at partition-related features, making a maximum
consistency between how partitioned tables and partitioned indexes
behave is actually a good approach.
--
Michael
On 2018-Nov-03, Michael Paquier wrote:
On Fri, Nov 02, 2018 at 03:53:51PM -0300, Alvaro Herrera wrote:
In this thread I'm not proposing to change the behavior for tables, only
for indexes. If people want to change behavior for tables (and I agree
with doing so), they can start their own threads.Changing this behavior on back-branches is not a good idea I think
because that changes the representation of the pg_class entries in the
catalogs by adding the reltablespace reference for a partitioned index
which does not exist now. We are long past the time of doing such
changes on v11, but that can perfectly be done for v12.
With all due respect, this argument makes no sense. All partitioned
indexes that exist today have a null reltablespace (all pg_class rows
already have a reltablespace column; I'm not changing that). If users
want to keep the current behavior (i.e. that indexes on future
partitions are created in the default tablespace), all they have to do
is not send any ALTER INDEX changing the index's tablespace.
You're saying that people currently running Postgres 11 that are
doing "CREATE INDEX ... TABLESPACE foo" will be surprised because the
index ends up in tablespace foo for partitions they create afterwards.
Additionally, you're saying that all people currently doing
ALTER INDEX ... SET TABLESPACE foo
expect that
1. the parent partitioned index silently does not change at all
2. the indexes on future partitions end up in the default tablespace.
I cannot see how that's for real.
Furthermore, I think delaying this change to pg12 serves nobody, because
it just means that there will be a behavior difference for no reason.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 11/02/2018 07:12 PM, Alvaro Herrera wrote:
On 2018-Nov-03, Michael Paquier wrote:
On Fri, Nov 02, 2018 at 03:53:51PM -0300, Alvaro Herrera wrote:
In this thread I'm not proposing to change the behavior for tables, only
for indexes. If people want to change behavior for tables (and I agree
with doing so), they can start their own threads.Changing this behavior on back-branches is not a good idea I think
because that changes the representation of the pg_class entries in the
catalogs by adding the reltablespace reference for a partitioned index
which does not exist now. We are long past the time of doing such
changes on v11, but that can perfectly be done for v12.With all due respect, this argument makes no sense. All partitioned
indexes that exist today have a null reltablespace (all pg_class rows
already have a reltablespace column; I'm not changing that). If users
want to keep the current behavior (i.e. that indexes on future
partitions are created in the default tablespace), all they have to do
is not send any ALTER INDEX changing the index's tablespace.You're saying that people currently running Postgres 11 that are
doing "CREATE INDEX ... TABLESPACE foo" will be surprised because the
index ends up in tablespace foo for partitions they create afterwards.Additionally, you're saying that all people currently doing
ALTER INDEX ... SET TABLESPACE foo
expect that
1. the parent partitioned index silently does not change at all
2. the indexes on future partitions end up in the default tablespace.I cannot see how that's for real.
Furthermore, I think delaying this change to pg12 serves nobody, because
it just means that there will be a behavior difference for no reason.
+1. This is unquestionably a POLA violation that should be fixed, IMNSHO.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-Nov-03, Andrew Dunstan wrote:
+1. This is unquestionably a POLA violation that should be fixed, IMNSHO.
Yeah, that's my view on it too.
Pushed.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Nov 2, 2018 at 7:12 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
With all due respect, this argument makes no sense. All partitioned
indexes that exist today have a null reltablespace (all pg_class rows
already have a reltablespace column; I'm not changing that). If users
I hope not, because that column isn't nullable.
want to keep the current behavior (i.e. that indexes on future
partitions are created in the default tablespace), all they have to do
is not send any ALTER INDEX changing the index's tablespace.You're saying that people currently running Postgres 11 that are
doing "CREATE INDEX ... TABLESPACE foo" will be surprised because the
index ends up in tablespace foo for partitions they create afterwards.Additionally, you're saying that all people currently doing
ALTER INDEX ... SET TABLESPACE foo
expect that
1. the parent partitioned index silently does not change at all
2. the indexes on future partitions end up in the default tablespace.I cannot see how that's for real.
Furthermore, I think delaying this change to pg12 serves nobody, because
it just means that there will be a behavior difference for no reason.
Well, you've guaranteed that already. Now 11 will be different from
11.1, and tables will be different from indexes until somebody goes
and makes that consistent again.
I now wish I hadn't objected to changing the behavior in April. If
I'd know that the alternative was going to be to change it in
November, back-patched, two days before a minor release, with more
people voting against the change than for it, I would have kept my
mouth shut.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2018-Nov-03, Robert Haas wrote:
Well, you've guaranteed that already. Now 11 will be different from
11.1, and tables will be different from indexes until somebody goes
and makes that consistent again.
Nobody is running 11.0 in production. The people using 11.0 are just
testing, and those that run into this behavior have already reported as
an inconvenience, not something to rely on.
I now wish I hadn't objected to changing the behavior in April. If
I'd know that the alternative was going to be to change it in
November, back-patched, two days before a minor release, with more
people voting against the change than for it, I would have kept my
mouth shut.
Well, you didn't object to changing index behavior in April. You
objected to a change related to tables. Nobody had noticed this
behavior for indexes.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-11-03 16:24:28 -0300, Alvaro Herrera wrote:
On 2018-Nov-03, Robert Haas wrote:
Well, you've guaranteed that already. Now 11 will be different from
11.1, and tables will be different from indexes until somebody goes
and makes that consistent again.Nobody is running 11.0 in production.
Uh, I know of people doing so.
Greetings,
Andres Freund
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2018-Nov-03, Andrew Dunstan wrote:
+1. This is unquestionably a POLA violation that should be fixed, IMNSHO.
Yeah, that's my view on it too.
Pushed.
Hmm ... in the April thread, one of the main concerns that prevented hasty
action was fear of breaking dump/restore behavior. Have you checked that
with this change, a dump/restore will restore the same state (same
actual tablespace assignments) that existed in the source DB? How about
if the parent partitioned index's tablespace assignment has been changed
since a child index was made? What happens with the --no-tablespaces
option?
I think I'm okay with this change if the answers to all those questions
are sane, but I didn't see them discussed in this thread.
regards, tom lane
On 2018-Nov-03, Tom Lane wrote:
Hmm ... in the April thread, one of the main concerns that prevented hasty
action was fear of breaking dump/restore behavior. Have you checked that
with this change, a dump/restore will restore the same state (same
actual tablespace assignments) that existed in the source DB?
I just did, and it does. The tablespaces are changed with individual
"SET default_tablespace" lines whenever it changes between dumping two
indexes.
How about if the parent partitioned index's tablespace assignment has
been changed since a child index was made?
Each index is created independently, with the correct default
tablespace, and then they are all attached together to the parent index
using ALTER INDEX ATTTACH PARTITION. The tablespace assignments are
identical to the source database.
What happens with the --no-tablespaces option?
No "SET default_tablespace" lines are emitted in this case, so
everything ends up in the default tablespace, as expected.
I think I'm okay with this change if the answers to all those questions
are sane, but I didn't see them discussed in this thread.
I think they are.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Nov 03, 2018 at 12:30:15PM -0700, Andres Freund wrote:
On 2018-11-03 16:24:28 -0300, Alvaro Herrera wrote:
On 2018-Nov-03, Robert Haas wrote:
Well, you've guaranteed that already. Now 11 will be different from
11.1, and tables will be different from indexes until somebody goes
and makes that consistent again.Nobody is running 11.0 in production.
Uh, I know of people doing so.
My understanding of the term GA is that anything marked as GA is
considered enough stable for production use, and that beta releases are
here for testing. So I don't get the argument. And you have made
partitioned indexes now inconsistent with partitioned tables.
Seeing this stuff discussed and committed in haste gives me the sad
face.
:(
--
Michael
On 2018-Nov-04, Michael Paquier wrote:
On Sat, Nov 03, 2018 at 12:30:15PM -0700, Andres Freund wrote:
On 2018-11-03 16:24:28 -0300, Alvaro Herrera wrote:
On 2018-Nov-03, Robert Haas wrote:
Well, you've guaranteed that already. Now 11 will be different from
11.1, and tables will be different from indexes until somebody goes
and makes that consistent again.Nobody is running 11.0 in production.
Uh, I know of people doing so.
My understanding of the term GA is that anything marked as GA is
considered enough stable for production use, and that beta releases are
here for testing. So I don't get the argument.
I think 11.0 is ready for testing that a migration from a production
running 10.x, but not for just blindly migrating. If you wanted to take
such a leap of faith, surely you'd wait for 11.1 at the very least.
And you have made partitioned indexes now inconsistent with
partitioned tables.
I don't buy this consistency argument. Partitions on partitioned tables
are not created automatically. They are for indexes. We just got bug
report #15490 about the problem I fixed.
Seeing this stuff discussed and committed in haste gives me the sad
face.
Nah.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Nov 7, 2018 at 10:46 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
I think 11.0 is ready for testing that a migration from a production
running 10.x, but not for just blindly migrating. If you wanted to take
such a leap of faith, surely you'd wait for 11.1 at the very least.
I think that's an irresponsible attitude for a committer to take. In
practice, you are probably right, but we shouldn't treat our
supposedly-stable releases as if they don't really need to be kept
stable. That's a recipe for anybody back-patching anything they feel
like whenever they like, which is a recipe for disaster.
But maybe you've adopted that policy already. You back-patched a
behavior change 2 days before a minor release when the vote was 2-3
against the change. If it matters, the three people opposing it all
work for different companies, wheres your only concurring vote came
from someone with whom you share an employer. I though the principle
here was that we operate by consensus; that is not a consensus to
proceed at all, let alone to back-patch on short notice.
Seeing this stuff discussed and committed in haste gives me the sad
face.Nah.
Sorry, but you don't get to decide what makes other people sad. I'm
with Michael on this one.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2018-11-07 11:19:53 -0500, Robert Haas wrote:
On Wed, Nov 7, 2018 at 10:46 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
I think 11.0 is ready for testing that a migration from a production
running 10.x, but not for just blindly migrating. If you wanted to take
such a leap of faith, surely you'd wait for 11.1 at the very least.I think that's an irresponsible attitude for a committer to take. In
practice, you are probably right, but we shouldn't treat our
supposedly-stable releases as if they don't really need to be kept
stable. That's a recipe for anybody back-patching anything they feel
like whenever they like, which is a recipe for disaster.
+1
Greetings,
Andres Freund
On 2018-Nov-07, Robert Haas wrote:
On Wed, Nov 7, 2018 at 10:46 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
I think 11.0 is ready for testing that a migration from a production
running 10.x, but not for just blindly migrating. If you wanted to take
such a leap of faith, surely you'd wait for 11.1 at the very least.I think that's an irresponsible attitude for a committer to take. In
practice, you are probably right, but we shouldn't treat our
supposedly-stable releases as if they don't really need to be kept
stable.
I take back that part actually, in the sense that I certainly wouldn't
push patches that I didn't think were good reasonable bugfixes the week
before a release.
But, again, I don't think I was changing any behavior that anybody was
relying on. Had anybody tried this case, they would have immediately
complained that it didn't work the way they would expect, like #14590.
But maybe you've adopted that policy already. You back-patched a
behavior change 2 days before a minor release when the vote was 2-3
against the change.
It was? This is my count:
For: Alvaro, Andrew, Tom
Against: Michael, Robert, Andres
Also, I contested every point that was raised about this patch. I don't
think there were any remaining technical objections.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Nov 7, 2018 at 1:22 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
But maybe you've adopted that policy already. You back-patched a
behavior change 2 days before a minor release when the vote was 2-3
against the change.It was? This is my count:
For: Alvaro, Andrew, Tom
Against: Michael, Robert, Andres
Tom's message was posted after you had already committed it. His vote
counts from the point of view of determining retrospectively whether
your action had support, but you can't use it to justify the decision
to push the commit when you did, unless you used a time machine to
foresee his message before he posted it.
Also, I contested every point that was raised about this patch. I don't
think there were any remaining technical objections.
Sure, but I don't think the standard is whether you told people that
they were wrong. I think the standard is whether you convinced them
to revise their position. You sure haven't convinced me.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 11/7/18 7:41 PM, Robert Haas wrote:
On Wed, Nov 7, 2018 at 1:22 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
But maybe you've adopted that policy already. You back-patched a
behavior change 2 days before a minor release when the vote was 2-3
against the change.It was? This is my count:
For: Alvaro, Andrew, Tom
Against: Michael, Robert, AndresTom's message was posted after you had already committed it. His
vote counts from the point of view of determining retrospectively
whether your action had support, but you can't use it to justify the
decision to push the commit when you did, unless you used a time
machine to foresee his message before he posted it.
Yeah. I think the change is OK from technical POV - it essentially fixes
behavior that is useless/surprising and would just result in raised
eyebrows and bogus bug reports. No problems here.
But the consensus probably was not there when it got pushed ...
Also, I contested every point that was raised about this patch. I
don't think there were any remaining technical objections.Sure, but I don't think the standard is whether you told people that
they were wrong. I think the standard is whether you convinced them
to revise their position. You sure haven't convinced me.
Yeah. While I think the objections were wrong, and Alvaro explained that
pretty well in his response, there should have been more time for the
OPs to respond before pushing the change. That's not great.
FWIW, it was mentioned that "your only concurring vote came from someone
with whom you share an employer" which kind suggests opinions/votes from
people working for the same company are somehow less honest/valuable. I
find that annoying and even insulting, because it kinda hints the
company (or companies) are pushing people to respond differently than
they would otherwise. Which I find rather insulting.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Nov 7, 2018 at 2:33 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
FWIW, it was mentioned that "your only concurring vote came from someone
with whom you share an employer" which kind suggests opinions/votes from
people working for the same company are somehow less honest/valuable. I
find that annoying and even insulting, because it kinda hints the
company (or companies) are pushing people to respond differently than
they would otherwise. Which I find rather insulting.
It doesn't have to be companies exerting overt pressure on their
employees. It's natural that people are going to have a sympathetic
view of patches written by people with whom they work on a day-to-day
basis. And it's not even a bad thing; having friends and colleagues
whom we trust is good. Still, it's impossible to be be sure that
you're reacting in exactly the same way to a patch by someone you know
and trust as you would to a patch written by a stranger, which is why
when Amit and I recently posted some reviews of zheap-related patches,
we inserted disclaimers into the first paragraph that we are not
independent of those patches and that independent review is desirable.
We did that because *we know* that we may be biased and we want to be
fair about that and on guard against it. I don't think asking other
people to be similarly aware should be annoying or insulting. If your
Mom decided to take up PostgreSQL hacking and posted a patch here, can
you really say you'd evaluate that patch with the exact same
objectivity that you'd apply to a patch written by somebody you'd
never met before? Whether you have a good relationship with your Mom
or a terrible one, that seems like an impossible standard for any
normal human being to meet.
With regard to this patch, I think the new behavior is fine in and of
itself, but I *do not* think it should have been back-patched and I
*do not* think it should work one way for tables and another for
indexes. And, regardless of the technical merits, I strongly object
to things to which multiple people are objecting being jammed into a
back-branch just before a release wraps. That's just not cool.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company