Re: patch : Allow toast tables to be moved to a different tablespace

Hi,

Here's a patch to allow TOAST tables to be moved to a different
tablespace. This item has been picked up from the TODO list.
Main idea is to consider that a TOAST table can have its own tablespace.

Regards,

--
JT

On Fri, Oct 7, 2011 at 10:10 AM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
> Hi,
>
> Here's a patch to allow TOAST tables to be moved to a different tablespace.
> This item has been picked up from the TODO list.
> Main idea is to consider that a TOAST table can have its own tablespace.
>

Hi,

This patch doesn't apply cleanly to head now... can you send a new
version against head?

about the patch itself. i don't like the fact that now the normal case
needs to include the word TABLE. IMHO, it should be optional and if
ommited TABLE should be assumed

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

2011/11/15 Jaime Casanova <jaime(at)2ndquadrant(dot)com>:
> On Fri, Oct 7, 2011 at 10:10 AM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>> Hi,
>>
>> Here's a patch to allow TOAST tables to be moved to a different tablespace.
>> This item has been picked up from the TODO list.
>> Main idea is to consider that a TOAST table can have its own tablespace.
>>
>
> Hi,
>
> This patch doesn't apply cleanly to head now... can you send a new
> version against head?

Hi Jaime,

New patch is attached.

>
> about the patch itself. i don't like the fact that now the normal case
> needs to include the word TABLE. IMHO, it should be optional and if
> ommited TABLE should be assumed
>

Maybe I'd missed something, but the normal case is :
ALTER TABLE ... SET TABLESPACE => moves Table + moves associated TOAST Table
ALTER TABLE ... SET TABLE TABLESPACE => moves Table & keeps associated
TOAST Table at its place
ALTER TABLE ... SET TOAST TABLESPACE => keeps Table at its place &
moves associated TOAST Table

Regards,

--
JT

On Tue, Nov 15, 2011 at 11:08 AM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>
> Maybe I'd missed something, but the normal case is :
> ALTER TABLE ... SET TABLESPACE => moves Table + moves associated TOAST Table
> ALTER TABLE ... SET TABLE TABLESPACE => moves Table & keeps associated
> TOAST Table at its place
> ALTER TABLE ... SET TOAST TABLESPACE => keeps Table at its place &
> moves associated TOAST Table
>

oh! i didn't test the patch just read it... and maybe i misunderstood,
will see it again.

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Tue, Nov 15, 2011 at 11:08 AM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>
> Maybe I'd missed something, but the normal case is :
> ALTER TABLE ... SET TABLESPACE => moves Table + moves associated TOAST Table
> ALTER TABLE ... SET TABLE TABLESPACE => moves Table & keeps associated
> TOAST Table at its place
> ALTER TABLE ... SET TOAST TABLESPACE => keeps Table at its place &
> moves associated TOAST Table
>

it has docs, and pg_dump support which is good.

but i found a few problems with the behaviour:
1) it accepts the sintax ALTER INDEX ... SET TOAST TABLESPACE ...;
which does nothing
2) after CLUSTER the index of the toast table gets moved to the same
tablespace as the main table
3) after ALTER TABLE ... ALTER ... TYPE ...; the toast table gets
moved to the same tablespace as the main table

now, if we are now supporting this variants
ALTER TABLE SET TABLE TABLESPACE
ALTER TABLE SET TOAST TABLESPACE

why not also support ALTER TABLE SET INDEX TABLESPACE which should
have the same behaviour as ALTER INDEX SET TABLESPACE... just an idea,
and of course not necessary for this patch

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

Hi Jaime,

Please find a new version.

2011/11/16 Jaime Casanova <jaime(at)2ndquadrant(dot)com>:
> On Tue, Nov 15, 2011 at 11:08 AM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>>
>> Maybe I'd missed something, but the normal case is :
>> ALTER TABLE ... SET TABLESPACE => moves Table + moves associated TOAST Table
>> ALTER TABLE ... SET TABLE TABLESPACE => moves Table & keeps associated
>> TOAST Table at its place
>> ALTER TABLE ... SET TOAST TABLESPACE => keeps Table at its place &
>> moves associated TOAST Table
>>
>
> it has docs, and pg_dump support which is good.
>
> but i found a few problems with the behaviour:
> 1) it accepts the sintax ALTER INDEX ... SET TOAST TABLESPACE ...;
> which does nothing

Fixed.

> 2) after CLUSTER the index of the toast table gets moved to the same
> tablespace as the main table

> 3) after ALTER TABLE ... ALTER ... TYPE ...; the toast table gets
> moved to the same tablespace as the main table

Fixed.

>
> now, if we are now supporting this variants
> ALTER TABLE SET TABLE TABLESPACE
> ALTER TABLE SET TOAST TABLESPACE
>
> why not also support ALTER TABLE SET INDEX TABLESPACE which should
> have the same behaviour as ALTER INDEX SET TABLESPACE... just an idea,
> and of course not necessary for this patch
>
> --
> Jaime Casanova         www.2ndQuadrant.com
> Professional PostgreSQL: Soporte 24x7 y capacitación

On Mon, Nov 28, 2011 at 1:32 PM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
> Hi Jaime,
>
> Please find a new version.
>

cool

>> 2) after CLUSTER the index of the toast table gets moved to the same
>> tablespace as the main table
>

there is still a variant of this one, i created 3 tablespaces (datos_tblspc):

"""
create table t1 (
i serial primary key,
t text
) tablespace datos_tblspc;

ALTER TABLE t1 SET TOAST TABLESPACE pg_default;
CLUSTER t1 USING t1_pkey;
"""

>>
>> now, if we are now supporting this variants
>> ALTER TABLE SET TABLE TABLESPACE
>> ALTER TABLE SET TOAST TABLESPACE
>>
>> why not also support ALTER TABLE SET INDEX TABLESPACE which should
>> have the same behaviour as ALTER INDEX SET TABLESPACE... just an idea,
>> and of course not necessary for this patch
>>

any opinion about this? maybe i can make a patch for that if there is
consensus that it could be good for symettry

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

Hi,

2011/12/10 Jaime Casanova <jaime(at)2ndquadrant(dot)com>:
> On Mon, Nov 28, 2011 at 1:32 PM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>
>>> 2) after CLUSTER the index of the toast table gets moved to the same
>>> tablespace as the main table
>>
>
> there is still a variant of this one, i created 3 tablespaces (datos_tblspc):
>
> """
> create table t1 (
>     i serial primary key,
>     t text
> ) tablespace datos_tblspc;
>
> ALTER TABLE t1 SET TOAST TABLESPACE pg_default;
> CLUSTER t1 USING t1_pkey;
> """

I am not able to reproduce this case, could you show me exactly how to
reproduce it ?

>
>>>
>>> now, if we are now supporting this variants
>>> ALTER TABLE SET TABLE TABLESPACE
>>> ALTER TABLE SET TOAST TABLESPACE
>>>
>>> why not also support ALTER TABLE SET INDEX TABLESPACE which should
>>> have the same behaviour as ALTER INDEX SET TABLESPACE... just an idea,
>>> and of course not necessary for this patch
>>>
>
> any opinion about this? maybe i can make a patch for that if there is
> consensus that it could be good for symettry

Thanks,

On Sat, Dec 10, 2011 at 4:16 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
>>> now, if we are now supporting this variants
>>> ALTER TABLE SET TABLE TABLESPACE
>>> ALTER TABLE SET TOAST TABLESPACE
>>>
>>> why not also support ALTER TABLE SET INDEX TABLESPACE which should
>>> have the same behaviour as ALTER INDEX SET TABLESPACE... just an idea,
>>> and of course not necessary for this patch
>
> any opinion about this? maybe i can make a patch for that if there is
> consensus that it could be good for symettry

I'm not really convinced we need it. I think it would end up just
being a shorthand for ALTER INDEX .. SET TABLESPACE for each index.
Most tables don't have more than a handful of indexes, so it doesn't
seem like we'd be gaining much (compare GRANT ... ON ALL TABLES IN
SCHEMA, which could easily be a shorthand for hundreds or perhaps even
thousands of individual GRANT statements).

Also, it seems that we haven't really discussed much why moving the
TOAST table to a different tablespace from the main table might be
useful. I'm not saying we shouldn't have it if it's good for
something, but what's the reason for wanting it?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Dec 12, 2011 at 10:54 AM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
> Hi,
>
> 2011/12/10 Jaime Casanova <jaime(at)2ndquadrant(dot)com>:
>> On Mon, Nov 28, 2011 at 1:32 PM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>>
>>>> 2) after CLUSTER the index of the toast table gets moved to the same
>>>> tablespace as the main table
>>>
>>
>> there is still a variant of this one, i created 3 tablespaces (datos_tblspc):
>>
>> """
>> create table t1 (
>>     i serial primary key,
>>     t text
>> ) tablespace datos_tblspc;
>>
>> ALTER TABLE t1 SET TOAST TABLESPACE pg_default;
>> CLUSTER t1 USING t1_pkey;
>> """
>
> I am not able to reproduce this case, could you show me exactly how to
> reproduce it ?
>

just as that...
- create a table in a certain tablespace (diferent from pg_default),
the toast table will be in the same tablespace,
- then change the tablespace to pg_default and
- then cluster the table...
the toast table will be again in the same tablespace as the main table

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

2011/12/13 Jaime Casanova <jaime(at)2ndquadrant(dot)com>:
> On Mon, Dec 12, 2011 at 10:54 AM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>> 2011/12/10 Jaime Casanova <jaime(at)2ndquadrant(dot)com>:
>>> On Mon, Nov 28, 2011 at 1:32 PM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>>>
>>>>> 2) after CLUSTER the index of the toast table gets moved to the same
>>>>> tablespace as the main table
>>>>
>>>
>>> there is still a variant of this one, i created 3 tablespaces (datos_tblspc):
>>>
>>> """
>>> create table t1 (
>>>     i serial primary key,
>>>     t text
>>> ) tablespace datos_tblspc;
>>>
>>> ALTER TABLE t1 SET TOAST TABLESPACE pg_default;
>>> CLUSTER t1 USING t1_pkey;
>>> """
>>
>> I am not able to reproduce this case, could you show me exactly how to
>> reproduce it ?
>>
>
> just as that...
> - create a table in a certain tablespace (diferent from pg_default),
> the toast table will be in the same tablespace,
> - then change the tablespace to pg_default and
> - then cluster the table...
> the toast table will be again in the same tablespace as the main table

Right, it seems to happen when the destination tablespace is the same
as the database's tbs, because, in this case, relation's tbs is set to
InvalidOid :
src/backend/commands/tablecmds.c line 8342

+ rd_rel->reltablespace = (newTableSpace == MyDatabaseTableSpace) ?
InvalidOid : newTableSpace;

Why don't just asign newTableSpace value here ?

Thanks,

On Tue, Dec 13, 2011 at 12:02 PM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
> Right, it seems to happen when the destination tablespace is the same
> as the database's tbs, because, in this case, relation's tbs is set to
> InvalidOid :
> src/backend/commands/tablecmds.c line 8342
>
> +       rd_rel->reltablespace = (newTableSpace == MyDatabaseTableSpace) ?
> InvalidOid : newTableSpace;
>
> Why don't just asign newTableSpace value here ?

When a relation is stored in the default tablespace, we always record
that in the system catalogs as InvalidOid. Otherwise, if the
database's default tablespace were changed, things would break.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/12/13 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Dec 13, 2011 at 12:02 PM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>> Right, it seems to happen when the destination tablespace is the same
>> as the database's tbs, because, in this case, relation's tbs is set to
>> InvalidOid :
>> src/backend/commands/tablecmds.c line 8342
>>
>> +       rd_rel->reltablespace = (newTableSpace == MyDatabaseTableSpace) ?
>> InvalidOid : newTableSpace;
>>
>> Why don't just asign newTableSpace value here ?
>
> When a relation is stored in the default tablespace, we always record
> that in the system catalogs as InvalidOid.  Otherwise, if the
> database's default tablespace were changed, things would break.

OK, considering that, I don't see any way to handle the case raised by Jaime :(

On 12/13/2011 12:29 PM, Julien Tachoires wrote:
> 2011/12/13 Robert Haas<robertmhaas(at)gmail(dot)com>:
>
>> On Tue, Dec 13, 2011 at 12:02 PM, Julien Tachoires<julmon(at)gmail(dot)com> wrote:
>>
>>> Right, it seems to happen when the destination tablespace is the same
>>> as the database's tbs, because, in this case, relation's tbs is set to
>>> InvalidOid :
>>> src/backend/commands/tablecmds.c line 8342
>>>
>>> + rd_rel->reltablespace = (newTableSpace == MyDatabaseTableSpace) ?
>>> InvalidOid : newTableSpace;
>>>
>>> Why don't just asign newTableSpace value here ?
>>>
>> When a relation is stored in the default tablespace, we always record
>> that in the system catalogs as InvalidOid. Otherwise, if the
>> database's default tablespace were changed, things would break.
>>
> OK, considering that, I don't see any way to handle the case raised by Jaime :(
>

So we have a problem here: there's a case that's messy to handle. And
there's really a large issue hanging over this whole patch, which is
that it needs a better explanation of what exactly it's going to get
used for. Especially if the implementation gets more complicated, we'd
want to see a clear reason to use this feature. And that's not really
clear.

If you can return with an update that perhaps finds a way to work around
this OID issue, please re-submit that. And if you can explain some more
about where you think this feature is useful, more information on that
would be helpful. Since this isn't going to get committed soon, I'm
going to mark it returned with feedback for now.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

Excerpts from Julien Tachoires's message of mar dic 13 14:29:56 -0300 2011:
> 2011/12/13 Robert Haas <robertmhaas(at)gmail(dot)com>:
> > On Tue, Dec 13, 2011 at 12:02 PM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
> >> Right, it seems to happen when the destination tablespace is the same
> >> as the database's tbs, because, in this case, relation's tbs is set to
> >> InvalidOid :
> >> src/backend/commands/tablecmds.c line 8342
> >>
> >> +       rd_rel->reltablespace = (newTableSpace == MyDatabaseTableSpace) ?
> >> InvalidOid : newTableSpace;
> >>
> >> Why don't just asign newTableSpace value here ?
> >
> > When a relation is stored in the default tablespace, we always record
> > that in the system catalogs as InvalidOid.  Otherwise, if the
> > database's default tablespace were changed, things would break.
>
> OK, considering that, I don't see any way to handle the case raised by Jaime :(

Uhm, surely you could compare the original toast tablespace to the heap
tablespace, and if they differ, handle appropriately when creating the
new toast table? Just pass down the toast tablespace into
AlterTableCreateToastTable, instead of having it assume that
rel->rd_rel->relnamespace is sufficient. This should be done in all
cases where a toast tablespace is created, which shouldn't be more than
a handful of them.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Tue, Dec 13, 2011 at 12:29 PM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>
> OK, considering that, I don't see any way to handle the case raised by Jaime :(

Did you consider what Álvaro suggested? anyway, seems is too late for
this commitfest.
are you intending to resume work on this for the next cycle?
do we consider this as a useful thing?

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

Excerpts from Jaime Casanova's message of lun ene 16 03:23:30 -0300 2012:
> On Tue, Dec 13, 2011 at 12:29 PM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
> >
> > OK, considering that, I don't see any way to handle the case raised by Jaime :(
>
> Did you consider what Álvaro suggested? anyway, seems is too late for
> this commitfest.
> are you intending to resume work on this for the next cycle?
> do we consider this as a useful thing?

The remaining bits shouldn't be too hard. In case Julien is not
interested in the task, I have added a link to this discussion in the
TODO item.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hi,

2012/1/16 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
> Excerpts from Jaime Casanova's message of lun ene 16 03:23:30 -0300 2012:
>> On Tue, Dec 13, 2011 at 12:29 PM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>> >
>> > OK, considering that, I don't see any way to handle the case raised by Jaime :(
>>
>> Did you consider what Álvaro suggested? anyway, seems is too late for
>> this commitfest.

Not yet.

>> are you intending to resume work on this for the next cycle?
>> do we consider this as a useful thing?

That's a good question.
If the answer is "yes", I'll continue on this work.

>
> The remaining bits shouldn't be too hard.  In case Julien is not
> interested in the task, I have added a link to this discussion in the
> TODO item.
>

2011/12/15 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
> Uhm, surely you could compare the original toast tablespace to the heap
> tablespace, and if they differ, handle appropriately when creating the
> new toast table?  Just pass down the toast tablespace into
> AlterTableCreateToastTable, instead of having it assume that
> rel->rd_rel->relnamespace is sufficient.  This should be done in all
> cases where a toast tablespace is created, which shouldn't be more than
> a handful of them.

Thank you, that way seems right.
Now, I distinguish before each creation of a TOAST table with
AlterTableCreateToastTable() : if it will create a new one or recreate
an existing one.
Thus, in create_toast_table() when toastTableSpace is equal to
InvalidOid, we are able :
- to fallback to the main table tablespace in case of new TOAST table creation
- to keep it previous tablespace in case of recreation.

Here's a new version rebased against HEAD.

Regards,

--
JT

On Sun, Jan 22, 2012 at 11:04 AM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
> 2011/12/15 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>>
>> Uhm, surely you could compare the original toast tablespace to the heap
>> tablespace, and if they differ, handle appropriately when creating the
>> new toast table?  Just pass down the toast tablespace into
>> AlterTableCreateToastTable, instead of having it assume that
>> rel->rd_rel->relnamespace is sufficient.  This should be done in all
>> cases where a toast tablespace is created, which shouldn't be more than
>> a handful of them.
>
> Thank you, that way seems right.
> Now, I distinguish before each creation of a TOAST table with
> AlterTableCreateToastTable() : if it will create a new one or recreate
> an existing one.
> Thus, in create_toast_table() when toastTableSpace is equal to
> InvalidOid, we are able :
> - to fallback to the main table tablespace in case of new TOAST table creation
> - to keep it previous tablespace in case of recreation.
>
> Here's a new version rebased against HEAD.

To ask more directly the question that's come up a few times upthread,
why do *you* think this is useful? What motivated you to want this
behavior, and/or how do you think it could benefit other PostgreSQL
users?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2012/1/24 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Jan 22, 2012 at 11:04 AM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>> 2011/12/15 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>>>
>>> Uhm, surely you could compare the original toast tablespace to the heap
>>> tablespace, and if they differ, handle appropriately when creating the
>>> new toast table?  Just pass down the toast tablespace into
>>> AlterTableCreateToastTable, instead of having it assume that
>>> rel->rd_rel->relnamespace is sufficient.  This should be done in all
>>> cases where a toast tablespace is created, which shouldn't be more than
>>> a handful of them.
>>
>> Thank you, that way seems right.
>> Now, I distinguish before each creation of a TOAST table with
>> AlterTableCreateToastTable() : if it will create a new one or recreate
>> an existing one.
>> Thus, in create_toast_table() when toastTableSpace is equal to
>> InvalidOid, we are able :
>> - to fallback to the main table tablespace in case of new TOAST table creation
>> - to keep it previous tablespace in case of recreation.
>>
>> Here's a new version rebased against HEAD.
>
> To ask more directly the question that's come up a few times upthread,
> why do *you* think this is useful?  What motivated you to want this
> behavior, and/or how do you think it could benefit other PostgreSQL
> users?
>

Sorry, I didn't get this question to me. I've just picked up this item
from the TODO list and then I was thinking that it could be useful. My
motivation was to learn more about PostgreSQL dev. and to work on a
concrete case. Now, I'm not sure anymore this is useful.

--
JT

On Wed, Jan 25, 2012 at 7:13 AM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
> 2012/1/24 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Sun, Jan 22, 2012 at 11:04 AM, Julien Tachoires <julmon(at)gmail(dot)com> wrote:
>>> 2011/12/15 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>>>>
>>>> Uhm, surely you could compare the original toast tablespace to the heap
>>>> tablespace, and if they differ, handle appropriately when creating the
>>>> new toast table?  Just pass down the toast tablespace into
>>>> AlterTableCreateToastTable, instead of having it assume that
>>>> rel->rd_rel->relnamespace is sufficient.  This should be done in all
>>>> cases where a toast tablespace is created, which shouldn't be more than
>>>> a handful of them.
>>>
>>> Thank you, that way seems right.
>>> Now, I distinguish before each creation of a TOAST table with
>>> AlterTableCreateToastTable() : if it will create a new one or recreate
>>> an existing one.
>>> Thus, in create_toast_table() when toastTableSpace is equal to
>>> InvalidOid, we are able :
>>> - to fallback to the main table tablespace in case of new TOAST table creation
>>> - to keep it previous tablespace in case of recreation.
>>>
>>> Here's a new version rebased against HEAD.
>>
>> To ask more directly the question that's come up a few times upthread,
>> why do *you* think this is useful?  What motivated you to want this
>> behavior, and/or how do you think it could benefit other PostgreSQL
>> users?
>
> Sorry, I didn't get this question to me. I've just picked up this item
> from the TODO list and then I was thinking that it could be useful. My
> motivation was to learn more about PostgreSQL dev. and to work on a
> concrete case. Now, I'm not sure anymore this is useful.

OK. In that case, I don't think it makes sense to add this right now.
I share the feeling that it could possibly be useful under some set
of circumstances, but it doesn't seem prudent to add features because
there might hypothetically be a use case. I suggest that we mark this
patch Returned with Feedback and add links to your latest version of
this patch and one of the emails expressing concerns about the utility
of this patch to the Todo list. If we later have a clearer idea in
mind why this might be useful, we can add it then - and document the
use case.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi

We have some tables with documents and their metadata (filename,
filetype, etc).
Most of the time we just want to get the metadata (filename, filetype,
etc) and not the document itself.
In this case it would be nice to have the metadata (which wouldn't end
up on the TOAST) on a fast array (SSD-based),
and put the filedata on a slow array (HDD-based). It's probably possible
to have two tables one for metadata and one
for the extra file, but this is a workaround.

--
Mvh
Christian Nicolaisen
Deltasoft AS
Tlf +47 938 38 596

On 2/8/12 6:17 AM, Christian Nicolaisen wrote:
> Hi
>
> We have some tables with documents and their metadata (filename, filetype, etc).
> Most of the time we just want to get the metadata (filename, filetype, etc) and not the document itself.
> In this case it would be nice to have the metadata (which wouldn't end up on the TOAST) on a fast array (SSD-based),
> and put the filedata on a slow array (HDD-based). It's probably possible to have two tables one for metadata and one
> for the extra file, but this is a workaround.
Did you intend to post a patch? Because nothing was attached. Also, if you're submitting a patch you should add it to the next commitfest.

On Fri, Feb 10, 2012 at 2:23 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
> On 2/8/12 6:17 AM, Christian Nicolaisen wrote:
>> We have some tables with documents and their metadata (filename, filetype,
>> etc).
>> Most of the time we just want to get the metadata (filename, filetype,
>> etc) and not the document itself.
>> In this case it would be nice to have the metadata (which wouldn't end up
>> on the TOAST) on a fast array (SSD-based),
>> and put the filedata on a slow array (HDD-based). It's probably possible
>> to have two tables one for metadata and one
>> for the extra file, but this is a workaround.
>
> Did you intend to post a patch? Because nothing was attached. Also, if
> you're submitting a patch you should add it to the next commitfest.

He was commenting on the possible utility of a previously-submitted
patch, which got pushed off because we weren't sure it was good for
anything.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Julien Tachoires <julmon(at)gmail(dot)com> writes:
>
> 2011/12/15 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>>
>> Uhm, surely you could compare the original toast tablespace to the heap
>> tablespace, and if they differ, handle appropriately when creating the
>> new toast table? =A0Just pass down the toast tablespace into
>> AlterTableCreateToastTable, instead of having it assume that
>> rel->rd_rel->relnamespace is sufficient. =A0This should be done in all
>> cases where a toast tablespace is created, which shouldn't be more than
>> a handful of them.
>
> Thank you, that way seems right.
> Now, I distinguish before each creation of a TOAST table with
> AlterTableCreateToastTable() : if it will create a new one or recreate
> an existing one.
> Thus, in create_toast_table() when toastTableSpace is equal to
> InvalidOid, we are able :
> - to fallback to the main table tablespace in case of new TOAST table creat=
> ion
> - to keep it previous tablespace in case of recreation.
>
> Here's a new version rebased against HEAD.

Almost 3 years later, here's a version rebased against current HEAD. :-)

It compiles and even passes the included regression test.

There were doubts originally if this is actually a useful feature, but
there is at least one plausible use case (main table on SSD, TOAST on
HDD):

http://www.postgresql.org/message-id/4F3267EE.80405@deltasoft.no

I didn't add anything on top of the latest version, but I notice we've
added mat. views since then, so it looks like we now also need this:

ALTER MATERIALIZED VIEW SET [VIEW | TOAST] TABLESPACE

Should I add this patch to the next CommitFest?

--
Alex

On Fri, Nov 28, 2014 at 11:38 AM, Alex Shulgin <ash(at)commandprompt(dot)com> wrote:
> Should I add this patch to the next CommitFest?

If you don't want it to get forgotten about, yes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

Here is my review of the feature.

- I have not worked enough with tablespaces to see if this patch would
be useful. There was some uncertainty about this upstream.

- Would it not be enough to simply allow running ALTER TABLE SET
TABLESPACE on the TOAST tables? Or is manually modifying those too ugly?

- I like that it adds tab completion for the new commands.

- The feature seems to work as described, other than the ALTER INDEX
issue noted below and that it broke pg_dump. The broken pg_dump means I
have not tested how this changes database dumps.

- A test fails in create_view.out. I looked some into it and did not see
how this could happen.

***
/home/andreas/dev/postgresql/src/test/regress/expected/create_view.out
2014-08-09 01:35:50.008886776 +0200
---
/home/andreas/dev/postgresql/src/test/regress/results/create_view.out
2014-12-30 00:41:17.966525339 +0100
***************
*** 283,289 ***
relname | relkind | reloptions
------------+---------+--------------------------
mysecview1 | v |
! mysecview2 | v |
mysecview3 | v | {security_barrier=true}
mysecview4 | v | {security_barrier=false}
(4 rows)
--- 283,289 ----
relname | relkind | reloptions
------------+---------+--------------------------
mysecview1 | v |
! mysecview2 | v | {security_barrier=true}
mysecview3 | v | {security_barrier=true}
mysecview4 | v | {security_barrier=false}
(4 rows)

- pg_dump is broken

pg_dump: [archiver (db)] query failed: ERROR: syntax error at or
near "("
LINE 1: ...nest(tc.reloptions) x), ', ') AS toast_reloptions
(SELECT sp...

- "ALTER INDEX foo_idx SET TABLE TABLESPACE ..." should not be allowed,
currently it changes the tablespace of the index. I do not think "ALTER
INDEX foo_idx SET (TABLE|TOAST) TABLESPACE ..." should even be allowed
in the grammar.

- You should add a link to
http://www.postgresql.org/docs/current/interactive/storage-toast.html to
the manual page of ALTER TABLE.

- I do not like how \d handles the toast tablespace. Having TOAST in
pg_default and the table in another space looks the same as if there was
no TOAST table at all. I think we should always print both tablespaces
if either of them are not pg_default.

- Would it be interesting to add syntax for moving the toast index to a
separate tablespace?

- There is no warning if you set the toast table space of a table
lacking toast. I think there should be one.

- No support for materialized views as pointed out by Alex.

- I think the code would be cleaner if ATPrepSetTableSpace and
ATPrepSetToastTableSpace were merged into one function or split into
two, one setting the toast and one setting the tablespace of the table
and one setting it on the TOAST table.

- I think a couple of tests for the error cases would be nice.

= Minor style issue

- Missing periods on the ALTER TABLE manual page after "See also CREATE
TABLESPACE" (in two places).

- Missing period last in the new paragraph added to the storage manual page.

- Double spaces in src/backend/catalog/toasting.c after "if (new_toast".

- The comment "and if this is not a TOAST re-creation case (through
CLUSTER)." incorrectly implies that CLUSTER is the only case where the
TOAST table is recreated.

- The local variables ToastTableSpace and ToastRel do not follow the
naming conventions.

- Remove the parentheses around "(is_system_catalog)".

- Why was the "Save info for Phase 3 to do the real work" comment
changed to "XXX: Save info for Phase 3 to do the real work"?

- Incorrect indentation for new code added last in ATExecSetTableSpace.

- The patch adds commented out code in src/include/commands/tablecmds.h.

--
Andreas Karlsson

On Tue, Dec 30, 2014 at 11:48 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> Here is my review of the feature.
Marked as returned with feedback.
--
Michael

Hi,

Sorry for the delay, I missed this thread.
Here is a new version of this patch considering Andreas' comments.

On 30/12/2014 03:48, Andreas Karlsson wrote:
> - A test fails in create_view.out. I looked some into it and did not see
> how this could happen.
>
> ***
> /home/andreas/dev/postgresql/src/test/regress/expected/create_view.out
> 2014-08-09 01:35:50.008886776 +0200
> ---
> /home/andreas/dev/postgresql/src/test/regress/results/create_view.out
> 2014-12-30 00:41:17.966525339 +0100
> ***************
> *** 283,289 ***
> relname | relkind | reloptions
> ------------+---------+--------------------------
> mysecview1 | v |
> ! mysecview2 | v |
> mysecview3 | v | {security_barrier=true}
> mysecview4 | v | {security_barrier=false}
> (4 rows)
> --- 283,289 ----
> relname | relkind | reloptions
> ------------+---------+--------------------------
> mysecview1 | v |
> ! mysecview2 | v | {security_barrier=true}
> mysecview3 | v | {security_barrier=true}
> mysecview4 | v | {security_barrier=false}
> (4 rows)

I can't reproduce this issue.

> - pg_dump is broken
>
> pg_dump: [archiver (db)] query failed: ERROR: syntax error at or
> near "("
> LINE 1: ...nest(tc.reloptions) x), ', ') AS toast_reloptions
> (SELECT sp...

Fixed.

> - "ALTER INDEX foo_idx SET TABLE TABLESPACE ..." should not be allowed,
> currently it changes the tablespace of the index. I do not think "ALTER
> INDEX foo_idx SET (TABLE|TOAST) TABLESPACE ..." should even be allowed
> in the grammar.

Fixed.

> - You should add a link to
> http://www.postgresql.org/docs/current/interactive/storage-toast.html to
> the manual page of ALTER TABLE.

Added.

> - I do not like how \d handles the toast tablespace. Having TOAST in
> pg_default and the table in another space looks the same as if there was
> no TOAST table at all. I think we should always print both tablespaces
> if either of them are not pg_default.

If we do it that way, we should always show the tablespace even if it's
pg_default in any case to be consistent. I'm pretty sure that we don't
want that.

> - Would it be interesting to add syntax for moving the toast index to a
> separate tablespace?

-1, we just want to be able to move TOAST heap, which is supposed to
contain a lot of data and we want to move on a different storage device
/ filesystem.

> - There is no warning if you set the toast table space of a table
> lacking toast. I think there should be one.

A notice is raised now in that case.

> - No support for materialized views as pointed out by Alex.

Support, documentation and regression tests added.

> - I think the code would be cleaner if ATPrepSetTableSpace and
> ATPrepSetToastTableSpace were merged into one function or split into
> two, one setting the toast and one setting the tablespace of the table
> and one setting it on the TOAST table.

Done.

> - I think a couple of tests for the error cases would be nice.

2 more tests added.

> - Missing periods on the ALTER TABLE manual page after "See also CREATE
> TABLESPACE" (in two places).
>
> - Missing period last in the new paragraph added to the storage manual page.

I don't understand those 2 last points ?

> - Double spaces in src/backend/catalog/toasting.c after "if (new_toast".

Fixed.

> - The comment "and if this is not a TOAST re-creation case (through
> CLUSTER)." incorrectly implies that CLUSTER is the only case where the
> TOAST table is recreated.

Fixed.

> - The local variables ToastTableSpace and ToastRel do not follow the
> naming conventions.

Fixed (I hope so).

> - Remove the parentheses around "(is_system_catalog)".

Fixed.

> - Why was the "Save info for Phase 3 to do the real work" comment
> changed to "XXX: Save info for Phase 3 to do the real work"?

Fixed.

> - Incorrect indentation for new code added last in ATExecSetTableSpace.

Fixed.

> - The patch adds commented out code in src/include/commands/tablecmds.h.

Fixed.

Thank you for your review.

--
Julien

On 03/03/2015 04:14 PM, Julien Tachoires wrote:
> On 30/12/2014 03:48, Andreas Karlsson wrote:
>> - A test fails in create_view.out. I looked some into it and did not see
>> how this could happen.
>>
>>[...]
>
> I can't reproduce this issue.

Neither can I anymore.

>> - pg_dump is broken
>>
>> pg_dump: [archiver (db)] query failed: ERROR: syntax error at or
>> near "("
>> LINE 1: ...nest(tc.reloptions) x), ', ') AS toast_reloptions
>> (SELECT sp...
>
> Fixed.

I still get a failing pg_dump test though when running make check-world.
And it does not work when run manually either.

+ pg_dumpall -f
/home/andreas/dev/postgresql/contrib/pg_upgrade/tmp_check/dump1.sql
pg_dump: column number -1 is out of range 0..28
cannot duplicate null pointer (internal error)
pg_dumpall: pg_dump failed on database "postgres", exiting
+ pg_dumpall1_status=1
+ [ /home/andreas/dev/postgresql != /home/andreas/dev/postgresql ]
+
/home/andreas/dev/postgresql/contrib/pg_upgrade/tmp_check/install//home/andreas/dev/postgresql-inst/bin/pg_ctl
-m fast stop
waiting for server to shut down.... done
server stopped
+ [ -n ]
+ [ -n ]
+ [ -n 1 ]
+ echo pg_dumpall of pre-upgrade database cluster failed
pg_dumpall of pre-upgrade database cluster failed
+ exit 1
+ rm -rf /tmp/pg_upgrade_check-5A3wsI

>> - I do not like how \d handles the toast tablespace. Having TOAST in
>> pg_default and the table in another space looks the same as if there was
>> no TOAST table at all. I think we should always print both tablespaces
>> if either of them are not pg_default.
>
> If we do it that way, we should always show the tablespace even if it's
> pg_default in any case to be consistent. I'm pretty sure that we don't
> want that.

I think we will have to agree to disagree here. I think it should be
obvious when the toast table is in the default tablespace for tables
outside it.

>> - Would it be interesting to add syntax for moving the toast index to a
>> separate tablespace?
>
> -1, we just want to be able to move TOAST heap, which is supposed to
> contain a lot of data and we want to move on a different storage device
> / filesystem.

I think we should allow moving the indexes for consistency. With this
patch we can move everything except for TOAST indexes.

>> - There is no warning if you set the toast table space of a table
>> lacking toast. I think there should be one.
>
> A notice is raised now in that case.

Excellent, also add a test case for this.

>> - Missing periods on the ALTER TABLE manual page after "See also CREATE
>> TABLESPACE" (in two places).
>>
>> - Missing period last in the new paragraph added to the storage manual page.
>
> I don't understand those 2 last points ?

I mean that "TOAST table can be moved to a different tablespace with
<command>ALTER TABLE SET TOAST TABLESPACE</>" should be changed to
"TOAST table can be moved to a different tablespace with <command>ALTER
TABLE SET TOAST TABLESPACE</>." since a sentece should always end in ".".

= New comments

- The patch does not apply cleanly anymore, I had to solve to minor
conflicts.

- Rewrap the documentation for SET TABLESPACE.

- You have added a tab in alter_table.sgml.

- Merge "case AT_SetTableTableSpace:" and "case AT_SetToastTableSpace:"
where they both do the same thing, i.e. nothing.

- Should it not be foo_mv in the query from the test suite below?

SELECT spcname FROM pg_class c JOIN pg_class d ON
(c.reltoastrelid=d.oid) JOIN pg_tablespace ON (d.reltablespace =
pg_tablespace.oid) WHERE c.relname = 'foo2';

--
Andreas Karlsson

On 03/03/2015 04:14 PM, Julien Tachoires wrote:
> Sorry for the delay, I missed this thread.
> Here is a new version of this patch considering Andreas' comments.

Please also add it to the next open commitfest so we do not lose the patch.

--
Andreas Karlsson

On Mon, Mar 9, 2015 at 7:26 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>>> - I do not like how \d handles the toast tablespace. Having TOAST in
>>> pg_default and the table in another space looks the same as if there was
>>> no TOAST table at all. I think we should always print both tablespaces
>>> if either of them are not pg_default.
>>
>> If we do it that way, we should always show the tablespace even if it's
>> pg_default in any case to be consistent. I'm pretty sure that we don't
>> want that.
>
> I think we will have to agree to disagree here. I think it should be obvious
> when the toast table is in the default tablespace for tables outside it.

I'm not sure about the details here, but that seems like a pretty
sound principle.

>>> - Would it be interesting to add syntax for moving the toast index to a
>>> separate tablespace?
>>
>> -1, we just want to be able to move TOAST heap, which is supposed to
>> contain a lot of data and we want to move on a different storage device
>> / filesystem.
>
> I think we should allow moving the indexes for consistency. With this patch
> we can move everything except for TOAST indexes.

It might make sense to always put the TOAST index with the TOAST
table, but it seems strange to put the TOAST index with the heap and
the TOAST table someplace else. Or at least, that's how it seems to
me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Mon, Mar 9, 2015 at 7:26 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:

> > I think we should allow moving the indexes for consistency. With this patch
> > we can move everything except for TOAST indexes.
>
> It might make sense to always put the TOAST index with the TOAST
> table, but it seems strange to put the TOAST index with the heap and
> the TOAST table someplace else. Or at least, that's how it seems to
> me.

Agreed. It doesn't seem necessary to allow moving the toast index to a
tablespace other than the one containing the toast table. In other
words, if you move the toast table, the index always follows it, and
there's no option to move it independently.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 03/10/2015 01:23 PM, Robert Haas wrote:
> On Mon, Mar 9, 2015 at 7:26 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>>>> - I do not like how \d handles the toast tablespace. Having TOAST in
>>>> pg_default and the table in another space looks the same as if there was
>>>> no TOAST table at all. I think we should always print both tablespaces
>>>> if either of them are not pg_default.
>>>
>>> If we do it that way, we should always show the tablespace even if it's
>>> pg_default in any case to be consistent. I'm pretty sure that we don't
>>> want that.
>>
>> I think we will have to agree to disagree here. I think it should be obvious
>> when the toast table is in the default tablespace for tables outside it.
>
> I'm not sure about the details here, but that seems like a pretty
> sound principle.

What I am talking about are the 6 cases below of \d with the current
code. I think that case 4) and 5) look the same, which may confuse users
skimming the \d into thinking that we have no TOAST table in the case
where the TOAST table is in pg_default while the table tablespace is
somewhere else.

1. Table in pg_default, no TOAST

Column | Type | Modifiers
--------+---------+-----------
x | integer |

2. Table in pg_default, TOAST in pg_default

Column | Type | Modifiers
--------+------+-----------
x | text |

3. Table in pg_default, TOAST in ts1

Column | Type | Modifiers
--------+------+-----------
x | text |
TOAST Tablespace: "ts1"

4. Table in ts1, no TOAST

Column | Type | Modifiers
--------+---------+-----------
x | integer |
Tablespace: "ts1"

5. Table in ts1, TOAST in pg_default

Column | Type | Modifiers
--------+------+-----------
x | text |
Tablespace: "ts1"

6. Table in ts1, TOAST in ts1

Column | Type | Modifiers
--------+------+-----------
x | text |
Tablespace: "ts1"
TOAST Tablespace: "ts1"

>> I think we should allow moving the indexes for consistency. With this patch
>> we can move everything except for TOAST indexes.
>
> It might make sense to always put the TOAST index with the TOAST
> table, but it seems strange to put the TOAST index with the heap and
> the TOAST table someplace else. Or at least, that's how it seems to
> me.

Ok, my idea was that one might want to put all indexes in a third table
space, including TOAST indexes. Not sure how realistic this use case is.

--
Andreas Karlsson

On 10/03/2015 13:27, Alvaro Herrera wrote:
> Robert Haas wrote:
>> On Mon, Mar 9, 2015 at 7:26 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>
>>> I think we should allow moving the indexes for consistency. With this patch
>>> we can move everything except for TOAST indexes.
>>
>> It might make sense to always put the TOAST index with the TOAST
>> table, but it seems strange to put the TOAST index with the heap and
>> the TOAST table someplace else. Or at least, that's how it seems to
>> me.
>
> Agreed. It doesn't seem necessary to allow moving the toast index to a
> tablespace other than the one containing the toast table. In other
> words, if you move the toast table, the index always follows it, and
> there's no option to move it independently.

This behaviour is already implemented, the TOAST index always follows
the TOAST table. I'll add a couple of regression tests about it.

--
Julien

Hi,

On 10/03/2015 00:31, Andreas Karlsson wrote:
> On 03/03/2015 04:14 PM, Julien Tachoires wrote:
>> Sorry for the delay, I missed this thread.
>> Here is a new version of this patch considering Andreas' comments.
>
> Please also add it to the next open commitfest so we do not lose the patch.

Here is a new version rebased against head and considering Andreas' last
comments:

- New regression tests about the fact that a notice is raised when
ALTER TABLE SET TOAST TABLESPACE is done for a non-TOASTed table.
- New regression tests to check that a TOAST index follows the TOAST
table when it's moved.
- Some fixes in the documentation.
- psql's '\d' command shows now both table and TOAST table tablespaces
when they are different, even if one of them is pg_default.
- patch added to the next open commitfest:
https://commitfest.postgresql.org/5/188/

Regards,

--
Julien

On 03/13/2015 11:48 AM, Julien Tachoires wrote:
> Here is a new version rebased against head and considering Andreas' last
> comments:
>
> - New regression tests about the fact that a notice is raised when
> ALTER TABLE SET TOAST TABLESPACE is done for a non-TOASTed table.
> - New regression tests to check that a TOAST index follows the TOAST
> table when it's moved.
> - Some fixes in the documentation.
> - psql's '\d' command shows now both table and TOAST table tablespaces
> when they are different, even if one of them is pg_default.
> - patch added to the next open commitfest:
> https://commitfest.postgresql.org/5/188/

Nice, I have no remaining comments on this patch other than some
incorrect mixing of whitespace in the indentation of the "Case when
TOAST and table tablespaces are different and als" comment.

Once that has been fixed I would say this patch is technically ready for
committer, but since it is in the open commitfest I do not think it will
get committed any time soon.

--
Andreas Karlsson

Noticed a bug when playing round some more with pg_dump. It does not
seem to dump custom table space for the table and default table space
for the toast correctly. It forgets about the toast table being in
pg_default.

--
Andreas Karlsson

On 14/03/2015 16:10, Andreas Karlsson wrote:
> Noticed a bug when playing round some more with pg_dump. It does not
> seem to dump custom table space for the table and default table space
> for the toast correctly. It forgets about the toast table being in
> pg_default.

Good catch. This is now fixed.

--
Julien

On 03/14/2015 05:59 PM, Julien Tachoires wrote:
> On 14/03/2015 16:10, Andreas Karlsson wrote:
>> Noticed a bug when playing round some more with pg_dump. It does not
>> seem to dump custom table space for the table and default table space
>> for the toast correctly. It forgets about the toast table being in
>> pg_default.
>
> Good catch. This is now fixed.

Nice. You will also want to apply the attached patch which fixes support
for the --no-tablespaces flag.

--
Andreas Karlsson

On 03/15/2015 04:25 AM, Andreas Karlsson wrote:
> Nice. You will also want to apply the attached patch which fixes support
> for the --no-tablespaces flag.

Just realized that --no-tablespaces need to be fixed for pg_restore too.

--
Andreas Karlsson

On 15/03/2015 04:34, Andreas Karlsson wrote:
> On 03/15/2015 04:25 AM, Andreas Karlsson wrote:
>> Nice. You will also want to apply the attached patch which fixes support
>> for the --no-tablespaces flag.
>
> Just realized that --no-tablespaces need to be fixed for pg_restore too.

Indeed, after taking a look at pg_restore case, I would say it won't be
so easy.
Will try to fix it.

--
Julien

On 15/03/2015 20:27, Julien Tachoires wrote:
> On 15/03/2015 04:34, Andreas Karlsson wrote:
>> On 03/15/2015 04:25 AM, Andreas Karlsson wrote:
>>> Nice. You will also want to apply the attached patch which fixes support
>>> for the --no-tablespaces flag.
>>
>> Just realized that --no-tablespaces need to be fixed for pg_restore too.
>
> Indeed, after taking a look at pg_restore case, I would say it won't be
> so easy.
> Will try to fix it.

Here is a new version fixing this issue. I've added a new kind of TOC
entry for being able to handle pg_restore --no-tablespace case.

--
Julien

On 03/17/2015 09:00 AM, Julien Tachoires wrote:
> Here is a new version fixing this issue. I've added a new kind of TOC
> entry for being able to handle pg_restore --no-tablespace case.

Looks good but I think one minor improvement could be to set the table
space of the toast entires to the same as the tablespace of the table to
reduce the amount of "SET default_tablespace". What do you think?

--
Andreas Karlsson

On 18/03/2015 19:54, Andreas Karlsson wrote:
> On 03/17/2015 09:00 AM, Julien Tachoires wrote:
>> Here is a new version fixing this issue. I've added a new kind of TOC
>> entry for being able to handle pg_restore --no-tablespace case.
>
> Looks good but I think one minor improvement could be to set the table
> space of the toast entires to the same as the tablespace of the table to
> reduce the amount of "SET default_tablespace". What do you think?

Yes, you're right, some useless "SET default_tablespace" were added for
each ALTER TABLE SET TOAST TABLESPACE statement. It's now fixed with
this new patch. Thanks.

--
Julien

On 03/19/2015 04:55 PM, Julien Tachoires wrote:
> On 18/03/2015 19:54, Andreas Karlsson wrote:
>> Looks good but I think one minor improvement could be to set the table
>> space of the toast entires to the same as the tablespace of the table to
>> reduce the amount of "SET default_tablespace". What do you think?
>
> Yes, you're right, some useless "SET default_tablespace" were added for
> each ALTER TABLE SET TOAST TABLESPACE statement. It's now fixed with
> this new patch. Thanks.

I am confused by your fix. Wouldn't cleaner fix be to use
tbinfo->reltablespace rather than tbinfo->reltoasttablespace when
calling ArchiveEntry()?

I tried the attached path and it seemed to work just fine.

--
Andreas Karlsson

On 20/03/2015 00:33, Andreas Karlsson wrote:
> On 03/19/2015 04:55 PM, Julien Tachoires wrote:
>> On 18/03/2015 19:54, Andreas Karlsson wrote:
>>> Looks good but I think one minor improvement could be to set the table
>>> space of the toast entires to the same as the tablespace of the table to
>>> reduce the amount of "SET default_tablespace". What do you think?
>>
>> Yes, you're right, some useless "SET default_tablespace" were added for
>> each ALTER TABLE SET TOAST TABLESPACE statement. It's now fixed with
>> this new patch. Thanks.
>
> I am confused by your fix. Wouldn't cleaner fix be to use
> tbinfo->reltablespace rather than tbinfo->reltoasttablespace when
> calling ArchiveEntry()?

Yes, doing this that way is cleaner. Here is a new version including
your fix. Thanks.

--
Julien

On 03/21/2015 01:19 PM, Julien Tachoires wrote:
>> I am confused by your fix. Wouldn't cleaner fix be to use
>> tbinfo->reltablespace rather than tbinfo->reltoasttablespace when
>> calling ArchiveEntry()?
>
> Yes, doing this that way is cleaner. Here is a new version including
> your fix. Thanks.

I am now satisfied with how the patch looks. Thanks for your work. I
will mark this as ready for committeer now but not expect any committer
to look at it until the commitfest starts.

--
Andreas Karlsson

Andreas Karlsson <andreas(at)proxel(dot)se> writes:
> On 03/21/2015 01:19 PM, Julien Tachoires wrote:
>>> I am confused by your fix. Wouldn't cleaner fix be to use
>>> tbinfo->reltablespace rather than tbinfo->reltoasttablespace when
>>> calling ArchiveEntry()?

>> Yes, doing this that way is cleaner. Here is a new version including
>> your fix. Thanks.

> I am now satisfied with how the patch looks. Thanks for your work. I
> will mark this as ready for committeer now but not expect any committer
> to look at it until the commitfest starts.

I have just looked through this thread, and TBH I think we should reject
this patch altogether --- not RWF, but "no we don't want this". The
use-case remains hypothetical: no performance numbers showing a real-world
benefit have been exhibited AFAICS. And allowing a toast table to be in a
different tablespace from its parent opens up a host of corner cases that,
despite the many revisions of the patch so far, probably haven't all been
addressed yet. (For instance, I wonder whether pg_upgrade copes with
toast tables in non-parent tablespaces.)

I regret the idea of wasting all the work that's been poured into this,
but I think pushing this patch forward will just waste even more time,
now and in the future, for benefit that will be at most marginal.

If any committers nonetheless want to take this up, be advised that it's
far from committable as-is. Here are some notes just from looking at
the pg_dump part of the patch:

* Addition in pg_backup_archiver.c seems pretty dubious; we don't
handle --no-tablespace that way for other cases.

* Why is getTables() collecting toast tablespace only from latest-model
servers? This will likely lead to doing the wrong thing (ie, dumping
incorrect "ALTER SET TOAST TABLESPACE pg_default" commands) when dumping
from an older server.

* dumpTOASTTablespace (man, that's an ugly choice of name ... camel
case combined with all-upper-case-words is a bad idea) neglects the
possibility that it needs to quote the tablespace name.

* Assorted random whitespace inconsistencies, only some of which would
be cleaned up by pgindent.

I've not studied the rest of the patch, but it would clearly need to
be gone over very carefully to get to a committable state.

regards, tom lane

On 2015-07-03 18:03:58 -0400, Tom Lane wrote:
> I have just looked through this thread, and TBH I think we should reject
> this patch altogether --- not RWF, but "no we don't want this". The
> use-case remains hypothetical: no performance numbers showing a real-world
> benefit have been exhibited AFAICS.

It's not that hard to imagine a performance benefit though? If the
toasted column is accessed infrequently/just after filtering on other
columns (not uncommon) it could very well be beneficial to put the main
table on fast storage (SSD) and the toast table on slow storage
(spinning rust).

I've seen humoungous toast tables that are barely ever read for tables
that are constantly a couple times in the field.

On 7/7/15 7:07 AM, Andres Freund wrote:
> On 2015-07-03 18:03:58 -0400, Tom Lane wrote:
>> I have just looked through this thread, and TBH I think we should reject
>> this patch altogether --- not RWF, but "no we don't want this". The
>> use-case remains hypothetical: no performance numbers showing a real-world
>> benefit have been exhibited AFAICS.
>
> It's not that hard to imagine a performance benefit though? If the
> toasted column is accessed infrequently/just after filtering on other
> columns (not uncommon) it could very well be beneficial to put the main
> table on fast storage (SSD) and the toast table on slow storage
> (spinning rust).
>
> I've seen humoungous toast tables that are barely ever read for tables
> that are constantly a couple times in the field.

+1. I know of one case where the heap was ~8GB and TOAST was over 400GB.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Tue, Jul 14, 2015 at 5:57 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> On 7/7/15 7:07 AM, Andres Freund wrote:
>> On 2015-07-03 18:03:58 -0400, Tom Lane wrote:
>>> I have just looked through this thread, and TBH I think we should reject
>>> this patch altogether --- not RWF, but "no we don't want this". The
>>> use-case remains hypothetical: no performance numbers showing a
>>> real-world
>>> benefit have been exhibited AFAICS.
>>
>>
>> It's not that hard to imagine a performance benefit though? If the
>> toasted column is accessed infrequently/just after filtering on other
>> columns (not uncommon) it could very well be beneficial to put the main
>> table on fast storage (SSD) and the toast table on slow storage
>> (spinning rust).
>>
>> I've seen humoungous toast tables that are barely ever read for tables
>> that are constantly a couple times in the field.
>
> +1. I know of one case where the heap was ~8GB and TOAST was over 400GB.

Yeah, I think that's a good argument for this. I have to admit that
I'm a bit fatigued by this thread - it started out as a "learn
PostgreSQL" project, and we iterated through a few design problems
that made me kind of worried about what sort of state the patch was in
- and now this patch is more than 4 years old. But if some committer
still has the energy to go through it in detail and make sure that all
of the problems have been fixed and that the patch is, as Andreas
says, in good shape, then I don't see why we shouldn't take it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 07/15/2015 09:30 PM, Robert Haas wrote:
> On Tue, Jul 14, 2015 at 5:57 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>> On 7/7/15 7:07 AM, Andres Freund wrote:
>>> On 2015-07-03 18:03:58 -0400, Tom Lane wrote:
>>>> I have just looked through this thread, and TBH I think we should reject
>>>> this patch altogether --- not RWF, but "no we don't want this". The
>>>> use-case remains hypothetical: no performance numbers showing a
>>>> real-world
>>>> benefit have been exhibited AFAICS.
>>>
>>>
>>> It's not that hard to imagine a performance benefit though? If the
>>> toasted column is accessed infrequently/just after filtering on other
>>> columns (not uncommon) it could very well be beneficial to put the main
>>> table on fast storage (SSD) and the toast table on slow storage
>>> (spinning rust).
>>>
>>> I've seen humoungous toast tables that are barely ever read for tables
>>> that are constantly a couple times in the field.
>>
>> +1. I know of one case where the heap was ~8GB and TOAST was over 400GB.
>
> Yeah, I think that's a good argument for this. I have to admit that
> I'm a bit fatigued by this thread - it started out as a "learn
> PostgreSQL" project, and we iterated through a few design problems
> that made me kind of worried about what sort of state the patch was in
> - and now this patch is more than 4 years old. But if some committer
> still has the energy to go through it in detail and make sure that all
> of the problems have been fixed and that the patch is, as Andreas
> says, in good shape, then I don't see why we shouldn't take it.

I personally think the patch is in a decent shape, and a worthwhile
feature. I agree though with Tom's objections about the pg_dump code.

I do not have enough time or interest right now to fix up this patch
(this is not a feature I personally have a lot of interest in), but if
someone else wishes to I do not think it would be too much work.

Andreas

On 3 August 2015 at 01:35, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:

> On 07/15/2015 09:30 PM, Robert Haas wrote:
>
>> On Tue, Jul 14, 2015 at 5:57 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>
>> wrote:
>>
>>> On 7/7/15 7:07 AM, Andres Freund wrote:
>>>
>>>> On 2015-07-03 18:03:58 -0400, Tom Lane wrote:
>>>>
>>>>> I have just looked through this thread, and TBH I think we should
>>>>> reject
>>>>> this patch altogether --- not RWF, but "no we don't want this". The
>>>>> use-case remains hypothetical: no performance numbers showing a
>>>>> real-world
>>>>> benefit have been exhibited AFAICS.
>>>>>
>>>>
>>>>
>>>> It's not that hard to imagine a performance benefit though? If the
>>>> toasted column is accessed infrequently/just after filtering on other
>>>> columns (not uncommon) it could very well be beneficial to put the main
>>>> table on fast storage (SSD) and the toast table on slow storage
>>>> (spinning rust).
>>>>
>>>> I've seen humoungous toast tables that are barely ever read for tables
>>>> that are constantly a couple times in the field.
>>>>
>>>
>>> +1. I know of one case where the heap was ~8GB and TOAST was over 400GB.
>>>
>>
>> Yeah, I think that's a good argument for this. I have to admit that
>> I'm a bit fatigued by this thread - it started out as a "learn
>> PostgreSQL" project, and we iterated through a few design problems
>> that made me kind of worried about what sort of state the patch was in
>> - and now this patch is more than 4 years old. But if some committer
>> still has the energy to go through it in detail and make sure that all
>> of the problems have been fixed and that the patch is, as Andreas
>> says, in good shape, then I don't see why we shouldn't take it.
>>
>
> I personally think the patch is in a decent shape, and a worthwhile
> feature. I agree though with Tom's objections about the pg_dump code.
>
> I do not have enough time or interest right now to fix up this patch (this
> is not a feature I personally have a lot of interest in), but if someone
> else wishes to I do not think it would be too much work.

To me the patch looks like it is in reasonable shape, caveats noted.

The patch doesn't really take us very far forwards in terms of
administration since workarounds exist which people use and understand.
Noting Tom's comments on additional complexity it seems like it could be a
source of bugs or production problems.

The deciding factor is that it brings TOAST as a keyword for us to support
forever. While reviewing this, I've come to realise that a better approach
to column stores and/or vertical partitioning is the more general way of
handling this, so creating a support legacy at this time doesn't make sense
to me personally.

On balance, the negatives seem to outweigh the positives so isn't the best
use of my time to fix it up.

I'm now returning this with feedback.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services