Обсуждение: 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@gmail.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@2ndquadrant.com>: > On Fri, Oct 7, 2011 at 10:10 AM, Julien Tachoires <julmon@gmail.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@gmail.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@gmail.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@2ndquadrant.com>: > On Tue, Nov 15, 2011 at 11:08 AM, Julien Tachoires <julmon@gmail.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@gmail.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@2ndquadrant.com>: > On Mon, Nov 28, 2011 at 1:32 PM, Julien Tachoires <julmon@gmail.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@2ndquadrant.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@gmail.com> wrote: > Hi, > > 2011/12/10 Jaime Casanova <jaime@2ndquadrant.com>: >> On Mon, Nov 28, 2011 at 1:32 PM, Julien Tachoires <julmon@gmail.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@2ndquadrant.com>: > On Mon, Dec 12, 2011 at 10:54 AM, Julien Tachoires <julmon@gmail.com> wrote: >> 2011/12/10 Jaime Casanova <jaime@2ndquadrant.com>: >>> On Mon, Nov 28, 2011 at 1:32 PM, Julien Tachoires <julmon@gmail.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@gmail.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@gmail.com>: > On Tue, Dec 13, 2011 at 12:02 PM, Julien Tachoires <julmon@gmail.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@gmail.com>: > >> On Tue, Dec 13, 2011 at 12:02 PM, Julien Tachoires<julmon@gmail.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@2ndQuadrant.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@gmail.com>: > > On Tue, Dec 13, 2011 at 12:02 PM, Julien Tachoires <julmon@gmail.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@commandprompt.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@gmail.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@gmail.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Hi, 2012/1/16 Alvaro Herrera <alvherre@commandprompt.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@gmail.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@commandprompt.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@gmail.com> wrote: > 2011/12/15 Alvaro Herrera <alvherre@commandprompt.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@gmail.com>: > On Sun, Jan 22, 2012 at 11:04 AM, Julien Tachoires <julmon@gmail.com> wrote: >> 2011/12/15 Alvaro Herrera <alvherre@commandprompt.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@gmail.com> wrote: > 2012/1/24 Robert Haas <robertmhaas@gmail.com>: >> On Sun, Jan 22, 2012 at 11:04 AM, Julien Tachoires <julmon@gmail.com> wrote: >>> 2011/12/15 Alvaro Herrera <alvherre@commandprompt.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 usefulunder 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 thenext commitfest.
On Fri, Feb 10, 2012 at 2:23 PM, Jim Nasby <jim@nasby.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@gmail.com> writes: > > 2011/12/15 Alvaro Herrera <alvherre@commandprompt.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@commandprompt.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@proxel.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@proxel.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@proxel.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@proxel.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@proxel.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@proxel.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.Nasby@bluetreble.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.Nasby@bluetreble.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@proxel.se> wrote:
--
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.On 07/15/2015 09:30 PM, Robert Haas wrote:On Tue, Jul 14, 2015 at 5:57 PM, Jim Nasby <Jim.Nasby@bluetreble.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 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/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services