Horton

Thread

Commits

  1. Fix tablespace handling for partitioned indexes

  1. partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-02T00:31:38Z

    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

  2. Re: partitioned indexes and tablespaces

    Michael Paquier <michael@paquier.xyz> — 2018-11-02T01:27:52Z

    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

  3. Re: partitioned indexes and tablespaces

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-11-02T02:46:59Z

    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:

https://www.postgresql.org/message-id/flat/CAKJS1f9PXYcT%2Bj%3DoyL-Lquz%3DScNwpRtmD7u9svLASUygBdbN8w%40mail.gmail.com

It's Michael's message that was the last one on that thread. :)

https://www.postgresql.org/message-id/20180413224007.GB27295%40paquier.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

  4. Re: partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-02T14:55:03Z

    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

  5. Re: partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-02T15:02:38Z

    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:
> 
> https://www.postgresql.org/message-id/flat/CAKJS1f9PXYcT%2Bj%3DoyL-Lquz%3DScNwpRtmD7u9svLASUygBdbN8w%40mail.gmail.com
> 
> It's Michael's message that was the last one on that thread. :)
> 
> https://www.postgresql.org/message-id/20180413224007.GB27295%40paquier.xyz

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

  6. Re: partitioned indexes and tablespaces

    Robert Haas <robertmhaas@gmail.com> — 2018-11-02T15:57:47Z

    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

  7. Re: partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-02T16:05:07Z

    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

  8. Re: partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-02T16:15:32Z

    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

  9. Re: partitioned indexes and tablespaces

    Robert Haas <robertmhaas@gmail.com> — 2018-11-02T18:49:13Z

    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

  10. Re: partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-02T18:53:51Z

    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

  11. Re: partitioned indexes and tablespaces

    Michael Paquier <michael@paquier.xyz> — 2018-11-02T22:59:37Z

    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

  12. Re: partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-02T23:12:15Z

    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

  13. Re: partitioned indexes and tablespaces

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-11-03T12:12:27Z

    
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

  14. Re: partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-03T16:34:44Z

    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

  15. Re: partitioned indexes and tablespaces

    Robert Haas <robertmhaas@gmail.com> — 2018-11-03T18:22:00Z

    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

  16. Re: partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-03T19:24:28Z

    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

  17. Re: partitioned indexes and tablespaces

    Andres Freund <andres@anarazel.de> — 2018-11-03T19:30:15Z

    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

  18. Re: partitioned indexes and tablespaces

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-03T19:32:03Z

    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

  19. Re: partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-03T20:50:13Z

    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

  20. Re: partitioned indexes and tablespaces

    Michael Paquier <michael@paquier.xyz> — 2018-11-04T00:09:06Z

    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

  21. Re: partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-07T15:45:59Z

    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

  22. Re: partitioned indexes and tablespaces

    Robert Haas <robertmhaas@gmail.com> — 2018-11-07T16:19:53Z

    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

  23. Re: partitioned indexes and tablespaces

    Andres Freund <andres@anarazel.de> — 2018-11-07T16:40:02Z

    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

  24. Re: partitioned indexes and tablespaces

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-07T18:21:59Z

    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

  25. Re: partitioned indexes and tablespaces

    Robert Haas <robertmhaas@gmail.com> — 2018-11-07T18:41:25Z

    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

  26. Re: partitioned indexes and tablespaces

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2018-11-07T19:33:27Z

    
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, 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.
> 

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

  27. Re: partitioned indexes and tablespaces

    Robert Haas <robertmhaas@gmail.com> — 2018-11-07T20:00:45Z

    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