Thread: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on thefly
Hi Hackers, I would like to propose a change, which allow CLUSTER, VACUUM FULL and REINDEX to modify relation tablespace on the fly. Actually, all these commands rebuild relation filenodes from the scratch, thus it seems natural to allow specifying them a new location. It may be helpful, when a server went out of disk, so you can attach new partition and perform e.g. VACUUM FULL, which will free some space and move data to a new location at the same time. Otherwise, you cannot complete VACUUM FULL until you have up to x2 relation disk space on a single partition. Please, find attached a patch, which extend CLUSTER, VACUUM FULL and REINDEX with additional options: REINDEX [ ( VERBOSE ) ] { INDEX | TABLE } name [ SET TABLESPACE new_tablespace ] CLUSTER [VERBOSE] table_name [ USING index_name ] [ SET TABLESPACE new_tablespace ] CLUSTER [VERBOSE] [ SET TABLESPACE new_tablespace ] VACUUM ( FULL [, ...] ) [ SET TABLESPACE new_tablespace ] [ table_and_columns [, ...] ] VACUUM FULL [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ SET TABLESPACE new_tablespace ] [ table_and_columns [, ...] ] Thereby I have a few questions: 1) What do you think about this concept in general? 2) Is SET TABLESPACE an appropriate syntax for this functionality? I thought also about a plain TABLESPACE keyword, but it seems to be misleading, and WITH (options) clause like in CREATE SUBSCRIPTION ... WITH (options). So I preferred SET TABLESPACE, since the same syntax is used currently in ALTER to change tablespace, but maybe someone will have a better idea. 3) I was not able to update the lexer for VACUUM FULL to use SET TABLESPACE after table_and_columns and completely get rid of shift/reduce conflicts. I guess it happens, since table_and_columns is optional and may be of variable length, but have no idea how to deal with it. Any thoughts? Regards -- Alexey Kondratov Postgres Professionalhttps://www.postgrespro.com Russian Postgres Company
Attachment
On Mon, Dec 24, 2018 at 6:08 AM Alexey Kondratov <a.kondratov@postgrespro.ru> wrote: > I would like to propose a change, which allow CLUSTER, VACUUM FULL and > REINDEX to modify relation tablespace on the fly. ALTER TABLE already has a lot of logic that is oriented towards being able to do multiple things at the same time. If we added CLUSTER, VACUUM FULL, and REINDEX to that set, then you could, say, change a data type, cluster, and change tablespaces all in a single SQL command. That would be cool, but probably a lot of work. :-( -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2018-Dec-26, Robert Haas wrote: > On Mon, Dec 24, 2018 at 6:08 AM Alexey Kondratov > <a.kondratov@postgrespro.ru> wrote: > > I would like to propose a change, which allow CLUSTER, VACUUM FULL and > > REINDEX to modify relation tablespace on the fly. > > ALTER TABLE already has a lot of logic that is oriented towards being > able to do multiple things at the same time. If we added CLUSTER, > VACUUM FULL, and REINDEX to that set, then you could, say, change a > data type, cluster, and change tablespaces all in a single SQL > command. That's a great observation. > That would be cool, but probably a lot of work. :-( But is it? ALTER TABLE is already doing one kind of table rewrite during phase 3, and CLUSTER is just a different kind of table rewrite (which happens to REINDEX), and VACUUM FULL is just a special case of CLUSTER. Maybe what we need is an ALTER TABLE variant that executes CLUSTER's table rewrite during phase 3 instead of its ad-hoc table rewrite. As for REINDEX, I think it's valuable to move tablespace together with the reindexing. You can already do it with the CREATE INDEX CONCURRENTLY recipe we recommend, of course; but REINDEX CONCURRENTLY is not going to provide that, and it seems worth doing. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Michael Paquier
Date:
On Wed, Dec 26, 2018 at 03:19:06PM -0300, Alvaro Herrera wrote: > As for REINDEX, I think it's valuable to move tablespace together with > the reindexing. You can already do it with the CREATE INDEX > CONCURRENTLY recipe we recommend, of course; but REINDEX CONCURRENTLY is > not going to provide that, and it seems worth doing. Even for plain REINDEX that seems useful. -- Michael
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
Hi, Thank you all for replies. >> ALTER TABLE already has a lot of logic that is oriented towards being >> able to do multiple things at the same time. If we added CLUSTER, >> VACUUM FULL, and REINDEX to that set, then you could, say, change a >> data type, cluster, and change tablespaces all in a single SQL >> command. > That's a great observation. Indeed, I thought that ALTER TABLE executes all actions sequentially one by one, e.g. in the case of ALTER TABLE test_int CLUSTER ON test_int_idx, SET TABLESPACE test_tblspc; it executes CLUSTER and THEN executes SET TABLESPACE. However, if I get it right, ALTER TABLE is rather smart, so in such a case it follows the steps: 1) Only saves new tablespace Oid during prepare phase 1 without actual work; 2) Only executes mark_index_clustered during phase 2, again without actual work done; 3) And finally rewrites relation during phase 3, where CLUSTER and SET TABLESPACE are effectively performed. >> That would be cool, but probably a lot of work. :-( > But is it? ALTER TABLE is already doing one kind of table rewrite > during phase 3, and CLUSTER is just a different kind of table rewrite > (which happens to REINDEX), and VACUUM FULL is just a special case of > CLUSTER. Maybe what we need is an ALTER TABLE variant that executes > CLUSTER's table rewrite during phase 3 instead of its ad-hoc table > rewrite. According to the ALTER TABLE example above, it is already exist for CLUSTER. > As for REINDEX, I think it's valuable to move tablespace together with > the reindexing. You can already do it with the CREATE INDEX > CONCURRENTLY recipe we recommend, of course; but REINDEX CONCURRENTLY is > not going to provide that, and it seems worth doing. Maybe I am missing something, but according to the docs REINDEX CONCURRENTLY does not exist yet, DROP then CREATE CONCURRENTLY is suggested instead. Thus, we have to add REINDEX CONCURRENTLY first, but it is a matter of different patch, I guess. >> Even for plain REINDEX that seems useful. >> -- >> Michael To summarize: 1) Alvaro and Michael agreed, that REINDEX with tablespace move may be useful. This is done in the patch attached to my initial email. Adding REINDEX to ALTER TABLE as new action seems quite questionable for me and not completely semantically correct. ALTER already looks bulky. 2) If I am correct, 'ALTER TABLE ... CLUSTER ON ..., SET TABLESPACE ...' does exactly what I wanted to add to CLUSTER in my patch. So probably no work is necessary here. 3) VACUUM FULL. It seems, that we can add special case 'ALTER TABLE ... VACUUM FULL, SET TABLESPACE ...', which will follow relatively the same path as with CLUSTER ON, but without any specific index. Relation should be rewritten in the new tablespace during phase 3. What do you think? Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
On 2018-Dec-27, Alexey Kondratov wrote: > To summarize: > > 1) Alvaro and Michael agreed, that REINDEX with tablespace move may be > useful. This is done in the patch attached to my initial email. Adding > REINDEX to ALTER TABLE as new action seems quite questionable for me and not > completely semantically correct. ALTER already looks bulky. Agreed on these points. > 2) If I am correct, 'ALTER TABLE ... CLUSTER ON ..., SET TABLESPACE ...' > does exactly what I wanted to add to CLUSTER in my patch. So probably no > work is necessary here. Well, ALTER TABLE CLUSTER ON does not really cluster the table; it only indicates which index to cluster on, for the next time you run standalone CLUSTER. I think it would be valuable to have those ALTER TABLE variants that rewrite the table do so using the cluster order, if there is one, instead of the heap order, which is what it does today. > 3) VACUUM FULL. It seems, that we can add special case 'ALTER TABLE ... > VACUUM FULL, SET TABLESPACE ...', which will follow relatively the same path > as with CLUSTER ON, but without any specific index. Relation should be > rewritten in the new tablespace during phase 3. Well, VACUUM FULL is just a table rewrite using the CLUSTER code that doesn't cluster on any index: it just uses the heap order. So in essence it's the same as a table-rewriting ALTER TABLE. In other words, if you get the index-ordered table rewriting in ALTER TABLE, I don't think this part adds anything useful; and it seems very confusing. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Masahiko Sawada
Date:
On Thu, Dec 27, 2018 at 10:24 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2018-Dec-27, Alexey Kondratov wrote: > > > To summarize: > > > > 1) Alvaro and Michael agreed, that REINDEX with tablespace move may be > > useful. This is done in the patch attached to my initial email. Adding > > REINDEX to ALTER TABLE as new action seems quite questionable for me and not > > completely semantically correct. ALTER already looks bulky. > > Agreed on these points. As an alternative idea, I think we can have a new ALTER INDEX variants that rebuilds the index while moving tablespace, something like ALTER INDEX ... REBUILD SET TABLESPACE .... Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexander Korotkov
Date:
On Fri, Dec 28, 2018 at 11:32 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > On Thu, Dec 27, 2018 at 10:24 PM Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > > > On 2018-Dec-27, Alexey Kondratov wrote: > > > > > To summarize: > > > > > > 1) Alvaro and Michael agreed, that REINDEX with tablespace move may be > > > useful. This is done in the patch attached to my initial email. Adding > > > REINDEX to ALTER TABLE as new action seems quite questionable for me and not > > > completely semantically correct. ALTER already looks bulky. > > > > Agreed on these points. > > As an alternative idea, I think we can have a new ALTER INDEX variants > that rebuilds the index while moving tablespace, something like ALTER > INDEX ... REBUILD SET TABLESPACE .... +1 It seems the easiest way to have feature-full commands. If we put functionality of CLUSTER and VACUUM FULL to ALTER TABLE, and put functionality of REINDEX to ALTER INDEX, then CLUSTER, VACUUM FULL and REINDEX would be just syntax sugar. ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
a.kondratov@postgrespro.ru
Date:
Hi hackers, On 2018-12-27 04:57, Michael Paquier wrote: > On Wed, Dec 26, 2018 at 03:19:06PM -0300, Alvaro Herrera wrote: >> As for REINDEX, I think it's valuable to move tablespace together with >> the reindexing. You can already do it with the CREATE INDEX >> CONCURRENTLY recipe we recommend, of course; but REINDEX CONCURRENTLY >> is >> not going to provide that, and it seems worth doing. > > Even for plain REINDEX that seems useful. > I've rebased the patch and put it on the closest commitfest. It is updated to allow user to do REINDEX CONCURRENTLY + SET TABLESPACE altogether, since plain REINDEX CONCURRENTLY became available this year. On 2019-06-07 21:27, Alexander Korotkov wrote: > On Fri, Dec 28, 2018 at 11:32 AM Masahiko Sawada > <sawada.mshk@gmail.com> wrote: >> On Thu, Dec 27, 2018 at 10:24 PM Alvaro Herrera >> <alvherre@2ndquadrant.com> wrote: >> > >> > On 2018-Dec-27, Alexey Kondratov wrote: >> > >> > > To summarize: >> > > >> > > 1) Alvaro and Michael agreed, that REINDEX with tablespace move may be >> > > useful. This is done in the patch attached to my initial email. Adding >> > > REINDEX to ALTER TABLE as new action seems quite questionable for me and not >> > > completely semantically correct. ALTER already looks bulky. >> > >> > Agreed on these points. >> >> As an alternative idea, I think we can have a new ALTER INDEX variants >> that rebuilds the index while moving tablespace, something like ALTER >> INDEX ... REBUILD SET TABLESPACE .... > > +1 > > It seems the easiest way to have feature-full commands. If we put > functionality of CLUSTER and VACUUM FULL to ALTER TABLE, and put > functionality of REINDEX to ALTER INDEX, then CLUSTER, VACUUM FULL and > REINDEX would be just syntax sugar. > I definitely bought into the idea of 'change a data type, cluster, and change tablespace all in a single SQL command', but stuck with some architectural questions, when it got to the code. Currently, the only one kind of table rewrite is done by ALTER TABLE. It is preformed by simply reading tuples one by one via table_scan_getnextslot and inserting into the new table via tuple_insert table access method (AM). In the same time, CLUSTER table rewrite is implemented as a separated table AM relation_copy_for_cluster, which is actually a direct link to the heap AM heapam_relation_copy_for_cluster. Basically speaking, CLUSTER table rewrite happens 2 abstraction layers lower than ALTER TABLE one. Furthermore, CLUSTER seems to be a heap-specific AM and may be meaningless for some other storages, which is even more important because of coming pluggable storages, isn't it? Maybe I overly complicate the problem, but to perform a data type change (or any other ALTER TABLE modification), cluster, and change tablespace in a row we have to bring all this high-level stuff done by ALTER TABLE to heapam_relation_copy_for_cluster. But is it even possible without leaking abstractions? I'm working toward adding REINDEX to ALTER INDEX, so it was possible to do 'ALTER INDEX ... REINDEX CONCURRENTLY SET TABLESPACE ...', but ALTER TABLE + CLUSTER/VACUUM FULL is quite questionable for me now. Anyway, new patch, which adds SET TABLESPACE to REINDEX is attached and this functionality seems really useful, so I will be very appreciate if someone will take a look on it. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com The Russian Postgres Company
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Surafel Temesgen
Date:
Hi Alexey
Here are a few commentOn Sat, Aug 31, 2019 at 11:54 PM <a.kondratov@postgrespro.ru> wrote:
Hi hackers,
Anyway, new patch, which adds SET TABLESPACE to REINDEX is attached and
this functionality seems really useful, so I will be very appreciate if
someone will take a look on it.
* There are NOWAIT option in alter index, is there a reason not to have similar option here?
* SET TABLESPACE command is not documented
* There are multiple checking for whether the relation is temporary tables of other sessions, one in check_relation_is_movable and other independently
*+ char *tablespacename;
calling it new_tablespacename will make it consistent with other places
*The patch did't applied cleanly http://cfbot.cputube.org/patch_24_2269.log
regards
Surafel
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
Hi Surafel, Thank you for looking at the patch! On 17.09.2019 14:04, Surafel Temesgen wrote: > * There are NOWAIT option in alter index, is there a reason not to > have similar option here? Currently in Postgres SET TABLESPACE always comes with [ NOWAIT ] option, so I hope it worth adding this option here for convenience. Added in the new version. > * SET TABLESPACE command is not documented Actually, new_tablespace parameter was documented, but I've added a more detailed section for SET TABLESPACE too. > * There are multiple checking for whether the relation is temporary > tables of other sessions, one in check_relation_is_movable and other > independently Yes, and there is a comment section in the code describing why. There is a repeatable bunch of checks for verification whether relation movable or not, so I put it into a separated function -- check_relation_is_movable. However, if we want to do only REINDEX, then some of them are excess, so the only one RELATION_IS_OTHER_TEMP is used. Thus, RELATION_IS_OTHER_TEMP is never executed twice, just different code paths. > *+ char *tablespacename; > > calling it new_tablespacename will make it consistent with other places > OK, changed, although I don't think it is important, since this is the only one tablespace variable there. > *The patch did't applied cleanly > http://cfbot.cputube.org/patch_24_2269.log > Patch is rebased and attached with all the fixes described above. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Michael Paquier
Date:
On Wed, Sep 18, 2019 at 03:46:20PM +0300, Alexey Kondratov wrote: > Currently in Postgres SET TABLESPACE always comes with [ NOWAIT ] option, so > I hope it worth adding this option here for convenience. Added in the new > version. It seems to me that it would be good to keep the patch as simple as possible for its first version, and split it into two if you would like to add this new option instead of bundling both together. This makes the review of one and the other more simple. Anyway, regarding the grammar, is SET TABLESPACE really our best choice here? What about: - TABLESPACE = foo, in parenthesis only? - Only using TABLESPACE, without SET at the end of the query? SET is used in ALTER TABLE per the set of subqueries available there, but that's not the case of REINDEX. +-- check that all relations moved to new tablespace +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace') +AND relname IN ('regress_tblspace_test_tbl_idx'); + relname +------------------------------- + regress_tblspace_test_tbl_idx +(1 row) Just to check one relation you could use \d with the relation (index or table) name. - if (RELATION_IS_OTHER_TEMP(iRel)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot reindex temporary tables of other - sessions"))) I would keep the order of this operation in order with CheckTableNotInUse(). -- Michael
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
Hi Michael, Thank you for your comments. On 19.09.2019 7:43, Michael Paquier wrote: > On Wed, Sep 18, 2019 at 03:46:20PM +0300, Alexey Kondratov wrote: >> Currently in Postgres SET TABLESPACE always comes with [ NOWAIT ] option, so >> I hope it worth adding this option here for convenience. Added in the new >> version. > It seems to me that it would be good to keep the patch as simple as > possible for its first version, and split it into two if you would > like to add this new option instead of bundling both together. This > makes the review of one and the other more simple. OK, it makes sense. I would also prefer first patch as simple as possible, but adding this NOWAIT option required only a few dozens of lines, so I just bundled everything together. Anyway, I will split patches if we decide to keep [ SET TABLESPACE ... [NOWAIT] ] grammar. > Anyway, regarding > the grammar, is SET TABLESPACE really our best choice here? What > about: > - TABLESPACE = foo, in parenthesis only? > - Only using TABLESPACE, without SET at the end of the query? > > SET is used in ALTER TABLE per the set of subqueries available there, > but that's not the case of REINDEX. I like SET TABLESPACE grammar, because it already exists and used both in ALTER TABLE and ALTER INDEX. Thus, if we once add 'ALTER INDEX index_name REINDEX SET TABLESPACE' (as was proposed earlier in the thread), then it will be consistent with 'REINDEX index_name SET TABLESPACE'. If we use just plain TABLESPACE, then it may be misleading in the following cases: - REINDEX TABLE table_name TABLESPACE tablespace_name - REINDEX (TABLESPACE = tablespace_name) TABLE table_name since it may mean 'Reindex all indexes of table_name, that stored in the tablespace_name', doesn't it? However, I have rather limited experience with Postgres, so I doesn't insist. > +-- check that all relations moved to new tablespace > +SELECT relname FROM pg_class > +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE > spcname='regress_tblspace') > +AND relname IN ('regress_tblspace_test_tbl_idx'); > + relname > +------------------------------- > + regress_tblspace_test_tbl_idx > +(1 row) > Just to check one relation you could use \d with the relation (index > or table) name. Yes, \d outputs tablespace name if it differs from pg_default, but it shows other information in addition, which is not necessary here. Also its output has more chances to be changed later, which may lead to the failed tests. This query output is more or less stable and new relations may be easily added to tests if we once add tablespace change to CLUSTER/VACUUM FULL. I can change test to use \d, but not sure that it would reduce test output length or will be helpful for a future tests support. > - if (RELATION_IS_OTHER_TEMP(iRel)) > - ereport(ERROR, > - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > - errmsg("cannot reindex temporary tables of other > - sessions"))) > I would keep the order of this operation in order with > CheckTableNotInUse(). Sure, I haven't noticed that reordered these operations, thanks. -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
On Thu, Sep 19, 2019 at 12:43 AM Michael Paquier <michael@paquier.xyz> wrote: > It seems to me that it would be good to keep the patch as simple as > possible for its first version, and split it into two if you would > like to add this new option instead of bundling both together. This > makes the review of one and the other more simple. Anyway, regarding > the grammar, is SET TABLESPACE really our best choice here? What > about: > - TABLESPACE = foo, in parenthesis only? > - Only using TABLESPACE, without SET at the end of the query? > > SET is used in ALTER TABLE per the set of subqueries available there, > but that's not the case of REINDEX. So, earlier in this thread, I suggested making this part of ALTER TABLE, and several people seemed to like that idea. Did we have a reason for dropping that approach? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 19.09.2019 16:21, Robert Haas wrote: > On Thu, Sep 19, 2019 at 12:43 AM Michael Paquier <michael@paquier.xyz> wrote: >> It seems to me that it would be good to keep the patch as simple as >> possible for its first version, and split it into two if you would >> like to add this new option instead of bundling both together. This >> makes the review of one and the other more simple. Anyway, regarding >> the grammar, is SET TABLESPACE really our best choice here? What >> about: >> - TABLESPACE = foo, in parenthesis only? >> - Only using TABLESPACE, without SET at the end of the query? >> >> SET is used in ALTER TABLE per the set of subqueries available there, >> but that's not the case of REINDEX. > So, earlier in this thread, I suggested making this part of ALTER > TABLE, and several people seemed to like that idea. Did we have a > reason for dropping that approach? If we add this option to REINDEX, then for 'ALTER TABLE tb_name action1, REINDEX SET TABLESPACE tbsp_name, action3' action2 will be just a direct alias to 'REINDEX TABLE tb_name SET TABLESPACE tbsp_name'. So it seems practical to do this for REINDEX first. The only one concern I have against adding REINDEX to ALTER TABLE in this context is that it will allow user to write such a chimera: ALTER TABLE tb_name REINDEX SET TABLESPACE tbsp_name, SET TABLESPACE tbsp_name; when they want to move both table and all the indexes. Because simple ALTER TABLE tb_name REINDEX, SET TABLESPACE tbsp_name; looks ambiguous. Should it change tablespace of table, indexes or both? -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Michael Paquier
Date:
On Thu, Sep 19, 2019 at 05:40:41PM +0300, Alexey Kondratov wrote: > On 19.09.2019 16:21, Robert Haas wrote: >> So, earlier in this thread, I suggested making this part of ALTER >> TABLE, and several people seemed to like that idea. Did we have a >> reason for dropping that approach? Personally, I don't find this idea very attractive as ALTER TABLE is already complicated enough with all the subqueries we already support in the command, all the logic we need to maintain to make combinations of those subqueries in a minimum number of steps, and also the number of bugs we have seen because of the amount of complication present. > If we add this option to REINDEX, then for 'ALTER TABLE tb_name action1, > REINDEX SET TABLESPACE tbsp_name, action3' action2 will be just a direct > alias to 'REINDEX TABLE tb_name SET TABLESPACE tbsp_name'. So it seems > practical to do this for REINDEX first. > > The only one concern I have against adding REINDEX to ALTER TABLE in this > context is that it will allow user to write such a chimera: > > ALTER TABLE tb_name REINDEX SET TABLESPACE tbsp_name, SET TABLESPACE > tbsp_name; > > when they want to move both table and all the indexes. Because simple > ALTER TABLE tb_name REINDEX, SET TABLESPACE tbsp_name; > looks ambiguous. Should it change tablespace of table, indexes or both? Tricky question, but we don't change the tablespace of indexes when using an ALTER TABLE, so I would say no on compatibility grounds. ALTER TABLE has never touched the tablespace of indexes, and I don't think that we should begin to do so. -- Michael
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Jose Luis Tallon
Date:
On 20/9/19 4:06, Michael Paquier wrote: > On Thu, Sep 19, 2019 at 05:40:41PM +0300, Alexey Kondratov wrote: >> On 19.09.2019 16:21, Robert Haas wrote: >>> So, earlier in this thread, I suggested making this part of ALTER >>> TABLE, and several people seemed to like that idea. Did we have a >>> reason for dropping that approach? > Personally, I don't find this idea very attractive as ALTER TABLE is > already complicated enough with all the subqueries we already support > in the command, all the logic we need to maintain to make combinations > of those subqueries in a minimum number of steps, and also the number > of bugs we have seen because of the amount of complication present. Yes, but please keep the other options: At it is, cluster, vacuum full and reindex already rewrite the table in full; Being able to write the result to a different tablespace than the original object was stored in enables a whole world of very interesting possibilities.... including a quick way out of a "so little disk space available that vacuum won't work properly" situation --- which I'm sure MANY users will appreciate, including me > If we add this option to REINDEX, then for 'ALTER TABLE tb_name action1, > REINDEX SET TABLESPACE tbsp_name, action3' action2 will be just a direct > alias to 'REINDEX TABLE tb_name SET TABLESPACE tbsp_name'. So it seems > practical to do this for REINDEX first. > > The only one concern I have against adding REINDEX to ALTER TABLE in this > context is that it will allow user to write such a chimera: > > ALTER TABLE tb_name REINDEX SET TABLESPACE tbsp_name, SET TABLESPACE > tbsp_name; > > when they want to move both table and all the indexes. Because simple > ALTER TABLE tb_name REINDEX, SET TABLESPACE tbsp_name; > looks ambiguous. Should it change tablespace of table, indexes or both? Indeed. IMHO, that form of the command should not allow that much flexibility... even on the "principle of least surprise" grounds :S That is, I'd restrict the ability to change (output) tablespace to the "direct" form --- REINDEX name, VACUUM (FULL) name, CLUSTER name --- whereas the ALTER table|index SET TABLESPACE would continue to work. Now that I come to think of it, maybe saying "output" or "move to" rather than "set tablespace" would make more sense for this variation of the commands? (clearer, less prone to confusion)? > Tricky question, but we don't change the tablespace of indexes when > using an ALTER TABLE, so I would say no on compatibility grounds. > ALTER TABLE has never touched the tablespace of indexes, and I don't > think that we should begin to do so. Indeed. I might be missing something, but is there any reason to not *require* a explicit transaction for the above multi-action commands? I mean, have it be: BEGIN; ALTER TABLE tb_name SET TABLESPACE tbsp_name; -- moves the table .... but possibly NOT the indexes? ALTER TABLE tb_name REINDEX [OUTPUT TABLESPACE tbsp_name]; -- REINDEX, placing the resulting index on tbsp_name instead of the original one COMMIT; ... and have the parser/planner combine the steps if it'd make sense (it probably wouldn't in this example)? Just my .02€ Thanks, / J.L.
On 2019-Sep-19, Robert Haas wrote: > So, earlier in this thread, I suggested making this part of ALTER > TABLE, and several people seemed to like that idea. Did we have a > reason for dropping that approach? Hmm, my own reading of that was to add tablespace changing abilities to ALTER TABLE *in addition* to this patch, not instead of it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 20.09.2019 19:38, Alvaro Herrera wrote: > On 2019-Sep-19, Robert Haas wrote: > >> So, earlier in this thread, I suggested making this part of ALTER >> TABLE, and several people seemed to like that idea. Did we have a >> reason for dropping that approach? > Hmm, my own reading of that was to add tablespace changing abilities to > ALTER TABLE *in addition* to this patch, not instead of it. That was my understanding too. On 20.09.2019 11:26, Jose Luis Tallon wrote: > On 20/9/19 4:06, Michael Paquier wrote: >> Personally, I don't find this idea very attractive as ALTER TABLE is >> already complicated enough with all the subqueries we already support >> in the command, all the logic we need to maintain to make combinations >> of those subqueries in a minimum number of steps, and also the number >> of bugs we have seen because of the amount of complication present. > > Yes, but please keep the other options: At it is, cluster, vacuum full > and reindex already rewrite the table in full; Being able to write the > result to a different tablespace than the original object was stored > in enables a whole world of very interesting possibilities.... > including a quick way out of a "so little disk space available that > vacuum won't work properly" situation --- which I'm sure MANY users > will appreciate, including me Yes, sure, that was my main motivation. The first message in the thread contains a patch, which adds SET TABLESPACE support to all of CLUSTER, VACUUM FULL and REINDEX. However, there came up an idea to integrate CLUSTER/VACUUM FULL with ALTER TABLE and do their work + all the ALTER TABLE stuff in a single table rewrite. I've dig a little bit into this and ended up with some architectural questions and concerns [1]. So I decided to start with a simple REINDEX patch. Anyway, I've followed Michael's advice and split the last patch into two: 1) Adds all the main functionality, but with simplified 'REINDEX INDEX [ CONCURRENTLY ] ... [ TABLESPACE ... ]' grammar; 2) Adds a more sophisticated syntax with '[ SET TABLESPACE ... [ NOWAIT ] ]'. Patch 1 contains all the docs and tests and may be applied/committed separately or together with 2, which is fully optional. Recent merge conflicts and reindex_index validations order are also fixed in the attached version. [1] https://www.postgresql.org/message-id/6b2a5c4de19f111ef24b63428033bb67%40postgrespro.ru Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Attachment
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, failed Spec compliant: not tested Documentation: tested, failed * I had to replace heap_open/close with table_open/close to get the patch to compile against master In the documentation + <para> + This specifies a tablespace, where all rebuilt indexes will be created. + Can be used only with <literal>REINDEX INDEX</literal> and + <literal>REINDEX TABLE</literal>, since the system indexes are not + movable, but <literal>SCHEMA</literal>, <literal>DATABASE</literal> or + <literal>SYSTEM</literal> very likely will has one. + </para> I found the "SCHEMA,DATABASE or SYSTEM very likely will has one." portion confusing and would be inclined to remove it orsomehow reword it. Consider the following ------------- create index foo_bar_idx on foo(bar) tablespace pg_default; CREATE INDEX reindex=# \d foo Table "public.foo" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- id | integer | | not null | bar | text | | | Indexes: "foo_pkey" PRIMARY KEY, btree (id) "foo_bar_idx" btree (bar) reindex=# reindex index foo_bar_idx tablespace tst1; REINDEX reindex=# reindex index foo_bar_idx tablespace pg_default; REINDEX reindex=# \d foo Table "public.foo" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- id | integer | | not null | bar | text | | | Indexes: "foo_pkey" PRIMARY KEY, btree (id) "foo_bar_idx" btree (bar), tablespace "pg_default" -------- It is a bit strange that it says "pg_default" as the tablespace. If I do this with a alter table to the table, moving the table back to pg_default makes it look as it did before. Otherwise the first patch seems fine. With the second patch(for NOWAIT) I did the following T1: begin; T1: insert into foo select generate_series(1,1000); T2: reindex index foo_bar_idx set tablespace tst1 nowait; T2 is waiting for a lock. This isn't what I would expect. The new status of this patch is: Waiting on Author
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
Hi Steve, Thank you for review. On 17.11.2019 3:53, Steve Singer wrote: > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, failed > Spec compliant: not tested > Documentation: tested, failed > > * I had to replace heap_open/close with table_open/close to get the > patch to compile against master > > In the documentation > > + <para> > + This specifies a tablespace, where all rebuilt indexes will be created. > + Can be used only with <literal>REINDEX INDEX</literal> and > + <literal>REINDEX TABLE</literal>, since the system indexes are not > + movable, but <literal>SCHEMA</literal>, <literal>DATABASE</literal> or > + <literal>SYSTEM</literal> very likely will has one. > + </para> > > I found the "SCHEMA,DATABASE or SYSTEM very likely will has one." portion confusing and would be inclined to remove itor somehow reword it. In the attached new version REINDEX with TABLESPACE and {SCHEMA, DATABASE, SYSTEM} now behaves more like with CONCURRENTLY, i.e. it skips unsuitable relations and shows warning. So this section in docs has been updated as well. Also the whole patch has been reworked. I noticed that my code in reindex_index was doing pretty much the same as inside RelationSetNewRelfilenode. So I just added a possibility to specify new tablespace for RelationSetNewRelfilenode instead. Thus, even with addition of new tests the patch becomes less complex. > Consider the following > > ------------- > create index foo_bar_idx on foo(bar) tablespace pg_default; > CREATE INDEX > reindex=# \d foo > Table "public.foo" > Column | Type | Collation | Nullable | Default > --------+---------+-----------+----------+--------- > id | integer | | not null | > bar | text | | | > Indexes: > "foo_pkey" PRIMARY KEY, btree (id) > "foo_bar_idx" btree (bar) > > reindex=# reindex index foo_bar_idx tablespace tst1; > REINDEX > reindex=# reindex index foo_bar_idx tablespace pg_default; > REINDEX > reindex=# \d foo > Table "public.foo" > Column | Type | Collation | Nullable | Default > --------+---------+-----------+----------+--------- > id | integer | | not null | > bar | text | | | > Indexes: > "foo_pkey" PRIMARY KEY, btree (id) > "foo_bar_idx" btree (bar), tablespace "pg_default" > -------- > > It is a bit strange that it says "pg_default" as the tablespace. If I do > this with a alter table to the table, moving the table back to pg_default > makes it look as it did before. > > Otherwise the first patch seems fine. Yes, I missed the fact that default tablespace of database is stored implicitly as InvalidOid, but I was setting it explicitly as specified. I have changed this behavior to stay consistent with ALTER TABLE. > With the second patch(for NOWAIT) I did the following > > T1: begin; > T1: insert into foo select generate_series(1,1000); > T2: reindex index foo_bar_idx set tablespace tst1 nowait; > > T2 is waiting for a lock. This isn't what I would expect. Indeed, I have added nowait option for RangeVarGetRelidExtended, so it should not wait if index is locked. However, for reindex we also have to put share lock on the parent table relation, which is done by opening it via table_open(heapId, ShareLock). The only one solution I can figure out right now is to wrap all such opens with ConditionalLockRelationOid(relId, ShareLock) and then do actual open with NoLock. This is how something similar is implemented in VACUUM if VACOPT_SKIP_LOCKED is specified. However, there are multiple code paths with table_open, so it becomes a bit ugly. I will leave the second patch aside for now and experiment with it. Actually, its main idea was to mimic ALTER INDEX ... SET TABLESPACE [NOWAIT] syntax, but probably it is better to stick with more brief plain TABLESPACE like in CREATE INDEX. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company P.S. I have also added all previous thread participants to CC in order to do not split the thread. Sorry if it was a badidea.
Attachment
On Wed, 20 Nov 2019, Alexey Kondratov wrote: > Hi Steve, > > Thank you for review. I've looked through the patch and tested it. I don't see any issues with this version. I think it is ready for a committer. > > Regards > > -- > Alexey Kondratov > > Postgres Professional https://www.postgrespro.com > Russian Postgres Company > > P.S. I have also added all previous thread participants to CC in order to do > not split the thread. Sorry if it was a bad idea. > Steve
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Masahiko Sawada
Date:
On Wed, 20 Nov 2019 at 19:16, Alexey Kondratov <a.kondratov@postgrespro.ru> wrote: > > Hi Steve, > > Thank you for review. > > On 17.11.2019 3:53, Steve Singer wrote: > > The following review has been posted through the commitfest application: > > make installcheck-world: tested, passed > > Implements feature: tested, failed > > Spec compliant: not tested > > Documentation: tested, failed > > > > * I had to replace heap_open/close with table_open/close to get the > > patch to compile against master > > > > In the documentation > > > > + <para> > > + This specifies a tablespace, where all rebuilt indexes will be created. > > + Can be used only with <literal>REINDEX INDEX</literal> and > > + <literal>REINDEX TABLE</literal>, since the system indexes are not > > + movable, but <literal>SCHEMA</literal>, <literal>DATABASE</literal> or > > + <literal>SYSTEM</literal> very likely will has one. > > + </para> > > > > I found the "SCHEMA,DATABASE or SYSTEM very likely will has one." portion confusing and would be inclined to remove itor somehow reword it. > > In the attached new version REINDEX with TABLESPACE and {SCHEMA, > DATABASE, SYSTEM} now behaves more like with CONCURRENTLY, i.e. it skips > unsuitable relations and shows warning. So this section in docs has been > updated as well. > > Also the whole patch has been reworked. I noticed that my code in > reindex_index was doing pretty much the same as inside > RelationSetNewRelfilenode. So I just added a possibility to specify new > tablespace for RelationSetNewRelfilenode instead. Thus, even with > addition of new tests the patch becomes less complex. > > > Consider the following > > > > ------------- > > create index foo_bar_idx on foo(bar) tablespace pg_default; > > CREATE INDEX > > reindex=# \d foo > > Table "public.foo" > > Column | Type | Collation | Nullable | Default > > --------+---------+-----------+----------+--------- > > id | integer | | not null | > > bar | text | | | > > Indexes: > > "foo_pkey" PRIMARY KEY, btree (id) > > "foo_bar_idx" btree (bar) > > > > reindex=# reindex index foo_bar_idx tablespace tst1; > > REINDEX > > reindex=# reindex index foo_bar_idx tablespace pg_default; > > REINDEX > > reindex=# \d foo > > Table "public.foo" > > Column | Type | Collation | Nullable | Default > > --------+---------+-----------+----------+--------- > > id | integer | | not null | > > bar | text | | | > > Indexes: > > "foo_pkey" PRIMARY KEY, btree (id) > > "foo_bar_idx" btree (bar), tablespace "pg_default" > > -------- > > > > It is a bit strange that it says "pg_default" as the tablespace. If I do > > this with a alter table to the table, moving the table back to pg_default > > makes it look as it did before. > > > > Otherwise the first patch seems fine. > > Yes, I missed the fact that default tablespace of database is stored > implicitly as InvalidOid, but I was setting it explicitly as specified. > I have changed this behavior to stay consistent with ALTER TABLE. > > > With the second patch(for NOWAIT) I did the following > > > > T1: begin; > > T1: insert into foo select generate_series(1,1000); > > T2: reindex index foo_bar_idx set tablespace tst1 nowait; > > > > T2 is waiting for a lock. This isn't what I would expect. > > Indeed, I have added nowait option for RangeVarGetRelidExtended, so it > should not wait if index is locked. However, for reindex we also have to > put share lock on the parent table relation, which is done by opening it > via table_open(heapId, ShareLock). > > The only one solution I can figure out right now is to wrap all such > opens with ConditionalLockRelationOid(relId, ShareLock) and then do > actual open with NoLock. This is how something similar is implemented in > VACUUM if VACOPT_SKIP_LOCKED is specified. However, there are multiple > code paths with table_open, so it becomes a bit ugly. > > I will leave the second patch aside for now and experiment with it. > Actually, its main idea was to mimic ALTER INDEX ... SET TABLESPACE > [NOWAIT] syntax, but probably it is better to stick with more brief > plain TABLESPACE like in CREATE INDEX. > Thank you for working on this. I looked at v4 patch. Here are some comments: + /* Skip all mapped relations if TABLESPACE is specified */ + if (OidIsValid(tableSpaceOid) && + classtuple->relfilenode == 0) I think we can use OidIsValid(classtuple->relfilenode) instead. --- + <para> + This specifies a tablespace, where all rebuilt indexes will be created. + Cannot be used with "mapped" and temporary relations. If <literal>SCHEMA</literal>, + <literal>DATABASE</literal> or <literal>SYSTEM</literal> is specified, then + all unsuitable relations will be skipped and a single <literal>WARNING</literal> + will be generated. + </para> This change says that temporary relation is not supported but it actually seems to work. Which is correct? postgres(1:37821)=# select relname, relpersistence from pg_class where relname like 'tmp%'; relname | relpersistence ----------+---------------- tmp | t tmp_pkey | t (2 rows) postgres(1:37821)=# reindex table tmp tablespace ts; REINDEX --- + if (newTableSpaceName) + { + tableSpaceOid = get_tablespace_oid(newTableSpaceName, false); + + /* Can't move a non-shared relation into pg_global */ + if (tableSpaceOid == GLOBALTABLESPACE_OID) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("only shared relations can be placed in pg_global tablespace"))); + } + if (OidIsValid(tablespaceOid) && RelationIsMapped(iRel)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot move system relation \"%s\"", + RelationGetRelationName(iRel)))); ISTM the kind of above errors are the same: the given tablespace exists but moving tablespace to it is not allowed since it's not supported in PostgreSQL. So I think we can use ERRCODE_FEATURE_NOT_SUPPORTED instead of ERRCODE_INVALID_PARAMETER_VALUE (which is used at 3 places) . Thoughts? Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Michael Paquier
Date:
On Tue, Nov 26, 2019 at 11:09:55PM +0100, Masahiko Sawada wrote: > Thank you for working on this. I have been looking at the latest patch as well. > I looked at v4 patch. Here are some comments: > > + /* Skip all mapped relations if TABLESPACE is specified */ > + if (OidIsValid(tableSpaceOid) && > + classtuple->relfilenode == 0) > > I think we can use OidIsValid(classtuple->relfilenode) instead. Yes, definitely. > This change says that temporary relation is not supported but it > actually seems to work. Which is correct? Yeah, I don't really see a reason why it would not work. > ISTM the kind of above errors are the same: the given tablespace > exists but moving tablespace to it is not allowed since it's not > supported in PostgreSQL. So I think we can use > ERRCODE_FEATURE_NOT_SUPPORTED instead of > ERRCODE_INVALID_PARAMETER_VALUE (which is used at 3 places) . Yes, it is also not project style to use full sentences in error messages, so I would suggest instead (note the missing quotes in the original patch): cannot move non-shared relation to tablespace \"%s\" @@ -3455,6 +3461,8 @@ RelationSetNewRelfilenode(Relation relation, char persistence) */ newrnode = relation->rd_node; newrnode.relNode = newrelfilenode; + if (OidIsValid(tablespaceOid)) + newrnode.spcNode = newTablespaceOid; The core of the patch is actually here. It seems to me that this is a very bad idea because you actually hijack a logic which happens at a much lower level which is based on the state of the tablespace stored in the relation cache entry of the relation being reindexed, then the tablespace choice actually happens in RelationInitPhysicalAddr() which for the new relfilenode once the follow-up CCI is done. So this very likely needs more thoughts, and bringing to the point: shouldn't you actually be careful that the relation tablespace is correctly updated before reindexing it and before creating its new relfilenode? This way, RelationSetNewRelfilenode() does not need any additional work, and I think that this saves from potential bugs in the choice of the tablespace used with the new relfilenode. There is no need for opt_tablespace_name as new node for the parsing grammar of gram.y as OptTableSpace is able to do the exact same job. + /* Skip all mapped relations if TABLESPACE is specified */ + if (OidIsValid(tableSpaceOid) && + classtuple->relfilenode == 0) + { + if (!system_warning) + ereport(WARNING, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot move indexes of system relations, skipping all"))); + system_warning = true; continue; It seems to me that you need to use RelationIsMapped() here, and we have no tests for it. On top of that, we should warn about *both* for catalogs reindexes and mapped relation whose tablespaces are being changed once each. Your patch has forgotten to update copyfuncs.c and equalfuncs.c with the new tablespace string field. It would be nice to add tab completion for this new clause in psql. This is not ready for committer yet in my opinion, and more work is done, so I am marking it as returned with feedback for now. -- Michael
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Michael Paquier
Date:
On Wed, Nov 27, 2019 at 12:54:16PM +0900, Michael Paquier wrote: > + /* Skip all mapped relations if TABLESPACE is specified */ > + if (OidIsValid(tableSpaceOid) && > + classtuple->relfilenode == 0) > + { > + if (!system_warning) > + ereport(WARNING, > + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + errmsg("cannot move indexes of system relations, skipping all"))); > + system_warning = true; > continue; > It seems to me that you need to use RelationIsMapped() here, and we > have no tests for it. On top of that, we should warn about *both* > for catalogs reindexes and mapped relation whose tablespaces are being > changed once each. Ditto. This has been sent too quickly. You cannot use RelationIsMapped() here because there is no Relation at hand, but I would suggest to use OidIsValid, and mention that this is the same check as RelationIsMapped(). -- Michael
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Michael Paquier
Date:
On Wed, Nov 27, 2019 at 12:54:16PM +0900, Michael Paquier wrote: > It would be nice to add tab completion for this new clause in psql. > This is not ready for committer yet in my opinion, and more work is > done, so I am marking it as returned with feedback for now. And I have somewhat missed to notice the timing of the review replies as you did not have room to reply, so fixed the CF entry to "waiting on author", and bumped it to next CF instead. -- Michael
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 27.11.2019 6:54, Michael Paquier wrote: > On Tue, Nov 26, 2019 at 11:09:55PM +0100, Masahiko Sawada wrote: >> I looked at v4 patch. Here are some comments: >> >> + /* Skip all mapped relations if TABLESPACE is specified */ >> + if (OidIsValid(tableSpaceOid) && >> + classtuple->relfilenode == 0) >> >> I think we can use OidIsValid(classtuple->relfilenode) instead. > Yes, definitely. Yes, switched to !OidIsValid(classtuple->relfilenode). Also I added a comment that it is meant to be equivalent to RelationIsMapped() and extended tests. > >> This change says that temporary relation is not supported but it >> actually seems to work. Which is correct? > Yeah, I don't really see a reason why it would not work. My bad, I was keeping in mind RELATION_IS_OTHER_TEMP validation, but it is for temp tables of other backends only, so it definitely should not be in the doc. Removed. > Your patch has forgotten to update copyfuncs.c and equalfuncs.c with > the new tablespace string field. Fixed, thanks. > It would be nice to add tab completion for this new clause in psql. Added. > There is no need for opt_tablespace_name as new node for the parsing > grammar of gram.y as OptTableSpace is able to do the exact same job. Sure, it was an artifact from the times, where I used optional SET TABLESPACE clause. Removed. > > @@ -3455,6 +3461,8 @@ RelationSetNewRelfilenode(Relation relation, > char persistence) > */ > newrnode = relation->rd_node; > newrnode.relNode = newrelfilenode; > + if (OidIsValid(tablespaceOid)) > + newrnode.spcNode = newTablespaceOid; > The core of the patch is actually here. It seems to me that this is a > very bad idea because you actually hijack a logic which happens at a > much lower level which is based on the state of the tablespace stored > in the relation cache entry of the relation being reindexed, then the > tablespace choice actually happens in RelationInitPhysicalAddr() which > for the new relfilenode once the follow-up CCI is done. So this very > likely needs more thoughts, and bringing to the point: shouldn't you > actually be careful that the relation tablespace is correctly updated > before reindexing it and before creating its new relfilenode? This > way, RelationSetNewRelfilenode() does not need any additional work, > and I think that this saves from potential bugs in the choice of the > tablespace used with the new relfilenode. When I did the first version of the patch I was looking on ATExecSetTableSpace, which implements ALTER ... SET TABLESPACE. And there is very similar pipeline there: 1) Find pg_class entry with SearchSysCacheCopy1 2) Create new relfilenode with GetNewRelFileNode 3) Set new tablespace for this relfilenode 4) Do some work with new relfilenode 5) Update pg_class entry with new tablespace 6) Do CommandCounterIncrement The only difference is that point 3) and tablespace part of 5) were missing in RelationSetNewRelfilenode, so I added them, and I do 4) after 6) in REINDEX. Thus, it seems that in my implementation of tablespace change in REINDEX I am more sure that "the relation tablespace is correctly updated before reindexing", since I do reindex after CCI (point 6), doesn't it? So why it is fine for ATExecSetTableSpace to do pretty much the same, but not for REINDEX? Or the key point is in doing actual work before CCI, but for me it seems a bit against what you have wrote? Thus, I cannot get your point correctly here. Can you, please, elaborate a little bit more your concerns? >> ISTM the kind of above errors are the same: the given tablespace >> exists but moving tablespace to it is not allowed since it's not >> supported in PostgreSQL. So I think we can use >> ERRCODE_FEATURE_NOT_SUPPORTED instead of >> ERRCODE_INVALID_PARAMETER_VALUE (which is used at 3 places) . > Yes, it is also not project style to use full sentences in error > messages, so I would suggest instead (note the missing quotes in the > original patch): > cannot move non-shared relation to tablespace \"%s\" Same here. I have taken this validation directly from tablecmds.c part for ALTER ... SET TABLESPACE. And there is exactly the same message "only shared relations can be placed in pg_global tablespace" with ERRCODE_INVALID_PARAMETER_VALUE there. However, I understand your point, but still, would it be better if I stick to the same ERRCODE/message? Or should I introduce new ERRCODE/message for the same case? > And I have somewhat missed to notice the timing of the review replies > as you did not have room to reply, so fixed the CF entry to "waiting > on author", and bumped it to next CF instead. Thank you! Attached is a patch, that addresses all the issues above, excepting the last two points (core part and error messages for pg_global), which are not clear for me right now. -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Michael Paquier
Date:
On Wed, Nov 27, 2019 at 08:47:06PM +0300, Alexey Kondratov wrote: > The only difference is that point 3) and tablespace part of 5) were missing > in RelationSetNewRelfilenode, so I added them, and I do 4) after 6) in > REINDEX. Thus, it seems that in my implementation of tablespace change in > REINDEX I am more sure that "the relation tablespace is correctly updated > before reindexing", since I do reindex after CCI (point 6), doesn't it? > > So why it is fine for ATExecSetTableSpace to do pretty much the same, but > not for REINDEX? Or the key point is in doing actual work before CCI, but > for me it seems a bit against what you have wrote? Nope, the order is not the same on what you do here, causing a duplication in the tablespace selection within RelationSetNewRelfilenode() and when flushing the relation on the new tablespace for the first time after the CCI happens, please see below. And we should avoid that. > Thus, I cannot get your point correctly here. Can you, please, elaborate a > little bit more your concerns? The case of REINDEX CONCURRENTLY is pretty simple, because a new relation which is a copy of the old relation is created before doing the reindex, so you simply need to set the tablespace OID correctly in index_concurrently_create_copy(). And actually, I think that the computation is incorrect because we need to check after MyDatabaseTableSpace as well, no? The case of REINDEX is more tricky, because you are working on a relation that already exists, hence I think that what you need to do a different thing before the actual REINDEX: 1) Update the existing relation's pg_class tuple to point to the new tablespace. 2) Do a CommandCounterIncrement. So I think that the order of the operations you are doing is incorrect, and that you have a risk of breaking the existing tablespace assignment logic done when first flushing a new relfilenode. This actually brings an extra thing: when doing a plain REINDEX you need to make sure that the past relfilenode of the relation gets away properly. The attached POC patch does that before doing the CCI which is a bit ugly, but that's enough to show my point, and there is no need to touch RelationSetNewRelfilenode() this way. -- Michael
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 02.12.2019 11:21, Michael Paquier wrote: > On Wed, Nov 27, 2019 at 08:47:06PM +0300, Alexey Kondratov wrote: >> The only difference is that point 3) and tablespace part of 5) were missing >> in RelationSetNewRelfilenode, so I added them, and I do 4) after 6) in >> REINDEX. Thus, it seems that in my implementation of tablespace change in >> REINDEX I am more sure that "the relation tablespace is correctly updated >> before reindexing", since I do reindex after CCI (point 6), doesn't it? >> >> So why it is fine for ATExecSetTableSpace to do pretty much the same, but >> not for REINDEX? Or the key point is in doing actual work before CCI, but >> for me it seems a bit against what you have wrote? > Nope, the order is not the same on what you do here, causing a > duplication in the tablespace selection within > RelationSetNewRelfilenode() and when flushing the relation on the new > tablespace for the first time after the CCI happens, please see > below. And we should avoid that. > >> Thus, I cannot get your point correctly here. Can you, please, elaborate a >> little bit more your concerns? > The case of REINDEX CONCURRENTLY is pretty simple, because a new > relation which is a copy of the old relation is created before doing > the reindex, so you simply need to set the tablespace OID correctly > in index_concurrently_create_copy(). And actually, I think that the > computation is incorrect because we need to check after > MyDatabaseTableSpace as well, no? No, the same logic already exists in heap_create: if (reltablespace == MyDatabaseTableSpace) reltablespace = InvalidOid; Which is called by index_concurrently_create_copy -> index_create -> heap_create. > The case of REINDEX is more tricky, because you are working on a > relation that already exists, hence I think that what you need to do a > different thing before the actual REINDEX: > 1) Update the existing relation's pg_class tuple to point to the new > tablespace. > 2) Do a CommandCounterIncrement. > So I think that the order of the operations you are doing is incorrect, > and that you have a risk of breaking the existing tablespace assignment > logic done when first flushing a new relfilenode. > > This actually brings an extra thing: when doing a plain REINDEX you > need to make sure that the past relfilenode of the relation gets away > properly. The attached POC patch does that before doing the CCI which > is a bit ugly, but that's enough to show my point, and there is no > need to touch RelationSetNewRelfilenode() this way. Thank you for the detailed answer and PoC patch. I will recheck everything and dig deeper into this problem, and come up with something closer to the next 01.2020 commitfest. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 2019-12-02 11:21, Michael Paquier wrote: > On Wed, Nov 27, 2019 at 08:47:06PM +0300, Alexey Kondratov wrote: > >> Thus, I cannot get your point correctly here. Can you, please, >> elaborate a >> little bit more your concerns? > > The case of REINDEX CONCURRENTLY is pretty simple, because a new > relation which is a copy of the old relation is created before doing > the reindex, so you simply need to set the tablespace OID correctly > in index_concurrently_create_copy(). And actually, I think that the > computation is incorrect because we need to check after > MyDatabaseTableSpace as well, no? > > The case of REINDEX is more tricky, because you are working on a > relation that already exists, hence I think that what you need to do a > different thing before the actual REINDEX: > 1) Update the existing relation's pg_class tuple to point to the new > tablespace. > 2) Do a CommandCounterIncrement. > So I think that the order of the operations you are doing is incorrect, > and that you have a risk of breaking the existing tablespace assignment > logic done when first flushing a new relfilenode. > > This actually brings an extra thing: when doing a plain REINDEX you > need to make sure that the past relfilenode of the relation gets away > properly. The attached POC patch does that before doing the CCI which > is a bit ugly, but that's enough to show my point, and there is no > need to touch RelationSetNewRelfilenode() this way. > OK, I hope that now I understand your concerns better. Another thing I just realised is that RelationSetNewRelfilenode is also used for mapped relations, which are not movable at all, so adding a tablespace options there seems to be not semantically correct as well. However, I still have not find a way to reproduce how to actually brake anything with my previous version of the patch. As for doing RelationDropStorage before CCI, I do not think that there is something wrong with it, this is exactly what RelationSetNewRelfilenode does. I have only moved RelationDropStorage before CatalogTupleUpdate compared to your proposal to match order inside RelationSetNewRelfilenode. > > Your patch has forgotten to update copyfuncs.c and equalfuncs.c with > the new tablespace string field. > > It would be nice to add tab completion for this new clause in psql. > This is not ready for committer yet in my opinion, and more work is > done, so I am marking it as returned with feedback for now. > Finally, I have also merged and unified all your and Masahiko's proposals with my recent changes: ereport corrections, tab-completion, docs update, copy/equalfuncs update, etc. New version is attached. Have it come any closer to a committable state now? Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Michael Paquier
Date:
On Sat, Jan 04, 2020 at 09:38:24PM +0300, Alexey Kondratov wrote: > Finally, I have also merged and unified all your and Masahiko's proposals > with my recent changes: ereport corrections, tab-completion, docs update, > copy/equalfuncs update, etc. New version is attached. Have it come any > closer to a committable state now? I have not yet reviewed this patch in details (I have that on my TODO), but at quick glance what you have here is rather close to what I'd expect to be committable as the tablespace OID assignment from your patch is consistent in the REINDEX code paths with the existing ALTER TABLE handling. -- Michael
Attachment
For your v7 patch, which handles REINDEX to a new tablespace, I have a few minor comments: + * the relation will be rebuilt. If InvalidOid is used, the default => should say "currrent", not default ? +++ b/doc/src/sgml/ref/reindex.sgml + <term><literal>TABLESPACE</literal></term> ... + <term><replaceable class="parameter">new_tablespace</replaceable></term> => I saw you split the description of TABLESPACE from new_tablespace based on comment earlier in the thread, but I suggest that the descriptions for these should be merged, like: + <varlistentry> + <term><literal>TABLESPACE</literal><replaceable class="parameter">new_tablespace</replaceable></term> + <listitem> + <para> + Allow specification of a tablespace where all rebuilt indexes will be created. + Cannot be used with "mapped" relations. If <literal>SCHEMA</literal>, + <literal>DATABASE</literal> or <literal>SYSTEM</literal> are specified, then + all unsuitable relations will be skipped and a single <literal>WARNING</literal> + will be generated. + </para> + </listitem> + </varlistentry> The existing patch is very natural, especially the parts in the original patch handling vacuum full and cluster. Those were removed to concentrate on REINDEX, and based on comments that it might be nice if ALTER handled CLUSTER and VACUUM FULL. On a separate thread, I brought up the idea of ALTER using clustered order. Tom pointed out some issues with my implementation, but didn't like the idea, either. So I suggest to re-include the CLUSTER/VAC FULL parts as a separate 0002 patch, the same way they were originally implemented. BTW, I think if "ALTER" were updated to support REINDEX (to allow multiple operations at once), it might be either: |ALTER INDEX i SET TABLESPACE , REINDEX -- to reindex a single index on a given tlbspc or |ALTER TABLE tbl REINDEX USING INDEX TABLESPACE spc; -- to reindex all inds on table inds moved to a given tblspc "USING INDEX TABLESPACE" is already used for ALTER..ADD column/table CONSTRAINT. -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 2020-02-11 19:48, Justin Pryzby wrote: > For your v7 patch, which handles REINDEX to a new tablespace, I have a > few > minor comments: > > + * the relation will be rebuilt. If InvalidOid is used, the default > > => should say "currrent", not default ? > Yes, it keeps current index tablespace in that case, thanks. > > +++ b/doc/src/sgml/ref/reindex.sgml > + <term><literal>TABLESPACE</literal></term> > ... > + <term><replaceable > class="parameter">new_tablespace</replaceable></term> > > => I saw you split the description of TABLESPACE from new_tablespace > based on > comment earlier in the thread, but I suggest that the descriptions for > these > should be merged, like: > > + <varlistentry> > + <term><literal>TABLESPACE</literal><replaceable > class="parameter">new_tablespace</replaceable></term> > + <listitem> > + <para> > + Allow specification of a tablespace where all rebuilt indexes > will be created. > + Cannot be used with "mapped" relations. If > <literal>SCHEMA</literal>, > + <literal>DATABASE</literal> or <literal>SYSTEM</literal> are > specified, then > + all unsuitable relations will be skipped and a single > <literal>WARNING</literal> > + will be generated. > + </para> > + </listitem> > + </varlistentry> > It sounds good to me, but here I just obey the structure, which is used all around. Documentation of ALTER TABLE/DATABASE, REINDEX and many others describes each literal/parameter in a separate entry, e.g. new_tablespace. So I would prefer to keep it as it is for now. > > The existing patch is very natural, especially the parts in the > original patch > handling vacuum full and cluster. Those were removed to concentrate on > REINDEX, and based on comments that it might be nice if ALTER handled > CLUSTER > and VACUUM FULL. On a separate thread, I brought up the idea of ALTER > using > clustered order. Tom pointed out some issues with my implementation, > but > didn't like the idea, either. > > So I suggest to re-include the CLUSTER/VAC FULL parts as a separate > 0002 patch, > the same way they were originally implemented. > > BTW, I think if "ALTER" were updated to support REINDEX (to allow > multiple > operations at once), it might be either: > |ALTER INDEX i SET TABLESPACE , REINDEX -- to reindex a single index > on a given tlbspc > or > |ALTER TABLE tbl REINDEX USING INDEX TABLESPACE spc; -- to reindex all > inds on table inds moved to a given tblspc > "USING INDEX TABLESPACE" is already used for ALTER..ADD column/table > CONSTRAINT. > Yes, I also think that allowing REINDEX/CLUSTER/VACUUM FULL to put resulting relation in a different tablespace is a very natural operation. However, I did a couple of attempts to integrate latter two with ALTER TABLE and failed with it, since it is already complex enough. I am still willing to proceed with it, but not sure how soon it will be. Anyway, new version is attached. It is rebased in order to resolve conflicts with a recent fix of REINDEX CONCURRENTLY + temp relations, and includes this small comment fix. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com The Russian Postgres Company
Attachment
On Sat, Feb 29, 2020 at 03:35:27PM +0300, Alexey Kondratov wrote: > Anyway, new version is attached. It is rebased in order to resolve conflicts > with a recent fix of REINDEX CONCURRENTLY + temp relations, and includes > this small comment fix. Thanks for rebasing - I actually started to do that yesterday. I extracted the bits from your original 0001 patch which handled CLUSTER and VACUUM FULL. I don't think if there's any interest in combining that with ALTER anymore. On another thread (1), I tried to implement that, and Tom pointed out problem with the implementation, but also didn't like the idea. I'm including some proposed fixes, but didn't yet update the docs, errors or tests for that. (I'm including your v8 untouched in hopes of not messing up the cfbot). My fixes avoid an issue if you try to REINDEX onto pg_default, I think due to moving system toast indexes. template1=# REINDEX DATABASE template1 TABLESPACE pg_default; 2020-02-29 08:01:41.835 CST [23382] WARNING: cannot change tablespace of indexes for mapped relations, skipping all WARNING: cannot change tablespace of indexes for mapped relations, skipping all 2020-02-29 08:01:41.894 CST [23382] ERROR: SMgrRelation hashtable corrupted 2020-02-29 08:01:41.894 CST [23382] STATEMENT: REINDEX DATABASE template1 TABLESPACE pg_default; 2020-02-29 08:01:41.894 CST [23382] WARNING: AbortTransaction while in COMMIT state 2020-02-29 08:01:41.895 CST [23382] PANIC: cannot abort transaction 491, it was already committed -- Justin (1) https://www.postgresql.org/message-id/flat/20200208150453.GV403%40telsasoft.com
Attachment
On Sat, Feb 29, 2020 at 08:53:04AM -0600, Justin Pryzby wrote: > On Sat, Feb 29, 2020 at 03:35:27PM +0300, Alexey Kondratov wrote: > > Anyway, new version is attached. It is rebased in order to resolve conflicts > > with a recent fix of REINDEX CONCURRENTLY + temp relations, and includes > > this small comment fix. > > Thanks for rebasing - I actually started to do that yesterday. > > I extracted the bits from your original 0001 patch which handled CLUSTER and > VACUUM FULL. I don't think if there's any interest in combining that with > ALTER anymore. On another thread (1), I tried to implement that, and Tom > pointed out problem with the implementation, but also didn't like the idea. > > I'm including some proposed fixes, but didn't yet update the docs, errors or > tests for that. (I'm including your v8 untouched in hopes of not messing up > the cfbot). My fixes avoid an issue if you try to REINDEX onto pg_default, I > think due to moving system toast indexes. I was able to avoid this issue by adding a call to GetNewRelFileNode, even though that's already called by RelationSetNewRelfilenode(). Not sure if there's a better way, or if it's worth Alexey's v3 patch which added a tablespace param to RelationSetNewRelfilenode. The current logic allows moving all the indexes and toast indexes, but I think we should use IsSystemRelation() unless allow_system_table_mods, like existing behavior of ALTER. template1=# ALTER TABLE pg_extension_oid_index SET tablespace pg_default; ERROR: permission denied: "pg_extension_oid_index" is a system catalog template1=# REINDEX INDEX pg_extension_oid_index TABLESPACE pg_default; REINDEX Finally, I think the CLUSTER is missing permission checks. It looks like relation_is_movable was factored out, but I don't see how that helps ? Alexey, I'm hoping to hear back if you think these changes are ok or if you'll publish a new version of the patch addressing the crash I reported. Or if you're too busy, maybe someone else can adopt the patch (I can help). -- Justin
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
Hi Justin, On 09.03.2020 23:04, Justin Pryzby wrote: > On Sat, Feb 29, 2020 at 08:53:04AM -0600, Justin Pryzby wrote: >> On Sat, Feb 29, 2020 at 03:35:27PM +0300, Alexey Kondratov wrote: >>> Anyway, new version is attached. It is rebased in order to resolve conflicts >>> with a recent fix of REINDEX CONCURRENTLY + temp relations, and includes >>> this small comment fix. >> Thanks for rebasing - I actually started to do that yesterday. >> >> I extracted the bits from your original 0001 patch which handled CLUSTER and >> VACUUM FULL. I don't think if there's any interest in combining that with >> ALTER anymore. On another thread (1), I tried to implement that, and Tom >> pointed out problem with the implementation, but also didn't like the idea. >> >> I'm including some proposed fixes, but didn't yet update the docs, errors or >> tests for that. (I'm including your v8 untouched in hopes of not messing up >> the cfbot). My fixes avoid an issue if you try to REINDEX onto pg_default, I >> think due to moving system toast indexes. > I was able to avoid this issue by adding a call to GetNewRelFileNode, even > though that's already called by RelationSetNewRelfilenode(). Not sure if > there's a better way, or if it's worth Alexey's v3 patch which added a > tablespace param to RelationSetNewRelfilenode. Do you have any understanding of what exactly causes this error? I have tried to debug it a little bit, but still cannot figure out why we need this extra GetNewRelFileNode() call and a mechanism how it helps. Probably you mean v4 patch. Yes, interestingly, if we do everything at once inside RelationSetNewRelfilenode(), then there is no issue at all with: REINDEX DATABASE template1 TABLESPACE pg_default; It feels like I am doing a monkey coding here, so I want to understand it better :) > The current logic allows moving all the indexes and toast indexes, but I think > we should use IsSystemRelation() unless allow_system_table_mods, like existing > behavior of ALTER. > > template1=# ALTER TABLE pg_extension_oid_index SET tablespace pg_default; > ERROR: permission denied: "pg_extension_oid_index" is a system catalog > template1=# REINDEX INDEX pg_extension_oid_index TABLESPACE pg_default; > REINDEX Yeah, we definitely should obey the same rules as ALTER TABLE / INDEX in my opinion. > Finally, I think the CLUSTER is missing permission checks. It looks like > relation_is_movable was factored out, but I don't see how that helps ? I did this relation_is_movable refactoring in order to share the same check between REINDEX + TABLESPACE and ALTER INDEX + SET TABLESPACE. Then I realized that REINDEX already has its own temp tables check and does mapped relations validation in multiple places, so I just added global tablespace checks instead. Thus, relation_is_movable seems to be outdated right now. Probably, we have to do another refactoring here once all proper validations will be accumulated in this patch set. > Alexey, I'm hoping to hear back if you think these changes are ok or if you'll > publish a new version of the patch addressing the crash I reported. > Or if you're too busy, maybe someone else can adopt the patch (I can help). Sorry for the late response, I was not going to abandon this patch, but was a bit busy last month. Many thanks for you review and fixups! There are some inconsistencies like mentions of SET TABLESPACE in error messages and so on. I am going to refactor and include your fixes 0003-0004 into 0001 and 0002, but keep 0005 separated for now, since this part requires more understanding IMO (and comparison with v4 implementation). That way, I am going to prepare a more clear patch set till the middle of the next week. I will be glad to receive more feedback from you then. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
On Thu, Mar 12, 2020 at 08:08:46PM +0300, Alexey Kondratov wrote: > On 09.03.2020 23:04, Justin Pryzby wrote: >> On Sat, Feb 29, 2020 at 08:53:04AM -0600, Justin Pryzby wrote: >>> On Sat, Feb 29, 2020 at 03:35:27PM +0300, Alexey Kondratov wrote: >>> tests for that. (I'm including your v8 untouched in hopes of not messing up >>> the cfbot). My fixes avoid an issue if you try to REINDEX onto pg_default, I >>> think due to moving system toast indexes. >> I was able to avoid this issue by adding a call to GetNewRelFileNode, even >> though that's already called by RelationSetNewRelfilenode(). Not sure if >> there's a better way, or if it's worth Alexey's v3 patch which added a >> tablespace param to RelationSetNewRelfilenode. > > Do you have any understanding of what exactly causes this error? I have > tried to debug it a little bit, but still cannot figure out why we need this > extra GetNewRelFileNode() call and a mechanism how it helps. The PANIC is from smgr hashtable, which couldn't find an entry it expected. My very tentative understanding is that smgr is prepared to handle a *relation* which is dropped/recreated multiple times in a transaction, but it's *not* prepared to deal with a given RelFileNode(Backend) being dropped/recreated, since that's used as a hash key. I revisited it and solved it in a somewhat nicer way. It's still not clear to me if there's an issue with your original way of adding a tablespace parameter to RelationSetNewRelfilenode(). > Probably you mean v4 patch. Yes, interestingly, if we do everything at once > inside RelationSetNewRelfilenode(), then there is no issue at all with: Yes, I meant to say "worth revisiting the v4 patch". > Many thanks for you review and fixups! There are some inconsistencies like > mentions of SET TABLESPACE in error messages and so on. I am going to > refactor and include your fixes 0003-0004 into 0001 and 0002, but keep 0005 > separated for now, since this part requires more understanding IMO (and > comparison with v4 implementation). I'd suggest to keep the CLUSTER/VACUUM FULL separate from REINDEX, in case Michael or someone else wants to progress one but cannot commit to both. But probably we should plan to finish this in July. -- Justin
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 2020-03-26 02:40, Justin Pryzby wrote: > On Thu, Mar 12, 2020 at 08:08:46PM +0300, Alexey Kondratov wrote: >> On 09.03.2020 23:04, Justin Pryzby wrote: >>> On Sat, Feb 29, 2020 at 08:53:04AM -0600, Justin Pryzby wrote: >>>> On Sat, Feb 29, 2020 at 03:35:27PM +0300, Alexey Kondratov wrote: >>>> tests for that. (I'm including your v8 untouched in hopes of not >>>> messing up >>>> the cfbot). My fixes avoid an issue if you try to REINDEX onto >>>> pg_default, I >>>> think due to moving system toast indexes. >>> I was able to avoid this issue by adding a call to GetNewRelFileNode, >>> even >>> though that's already called by RelationSetNewRelfilenode(). Not >>> sure if >>> there's a better way, or if it's worth Alexey's v3 patch which added >>> a >>> tablespace param to RelationSetNewRelfilenode. >> >> Do you have any understanding of what exactly causes this error? I >> have >> tried to debug it a little bit, but still cannot figure out why we >> need this >> extra GetNewRelFileNode() call and a mechanism how it helps. > > The PANIC is from smgr hashtable, which couldn't find an entry it > expected. My > very tentative understanding is that smgr is prepared to handle a > *relation* > which is dropped/recreated multiple times in a transaction, but it's > *not* > prepared to deal with a given RelFileNode(Backend) being > dropped/recreated, > since that's used as a hash key. > > I revisited it and solved it in a somewhat nicer way. > I included your new solution regarding this part from 0004 into 0001. It seems that at least a tip of the problem was in that we tried to change tablespace to pg_default being already there. > > It's still not clear to > me if there's an issue with your original way of adding a tablespace > parameter > to RelationSetNewRelfilenode(). > Yes, it is not clear for me too. > >> Many thanks for you review and fixups! There are some inconsistencies >> like >> mentions of SET TABLESPACE in error messages and so on. I am going to >> refactor and include your fixes 0003-0004 into 0001 and 0002, but keep >> 0005 >> separated for now, since this part requires more understanding IMO >> (and >> comparison with v4 implementation). > > I'd suggest to keep the CLUSTER/VACUUM FULL separate from REINDEX, in > case > Michael or someone else wants to progress one but cannot commit to > both. > Yes, sure, I did not have plans to melt everything into a single patch. So, it has taken much longer to understand and rework all these fixes and permission validations. Attached is the updated patch set. 0001: — It is mostly the same, but refactored — I also included your most recent fix for REINDEX DATABASE with allow_system_table_mods=1 — With this patch REINDEX + TABLESPACE simply errors out, when index on TOAST table is met and allow_system_table_mods=0 0002: — I reworked it a bit, since REINDEX CONCURRENTLY is not allowed on system catalog anyway, that is checked at the hegher levels of statement processing. So we have to care about TOAST relations — Also added the same check into the plain REINDEX — It works fine, but I am not entirely happy that with this patch errors/warnings are a bit inconsistent: template1=# REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_12773_index TABLESPACE pg_default; WARNING: skipping tablespace change of "pg_toast_12773_index" DETAIL: Cannot move system relation, only REINDEX CONCURRENTLY is performed. template1=# REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_12773 TABLESPACE pg_default; ERROR: permission denied: "pg_toast_12773" is a system catalog And REINDEX DATABASE CONCURRENTLY will generate a warning again. Maybe we should always throw a warning and do only reindex if it is not possible to change tablespace? 0003: — I have get rid of some of previous refactoring pieces like check_relation_is_movable for now. Let all these validations to settle and then think whether we could do it better — Added CLUSTER to copy/equalfuncs — Cleaned up messages and comments I hope that I did not forget anything from your proposals. -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Attachment
> I included your new solution regarding this part from 0004 into 0001. It > seems that at least a tip of the problem was in that we tried to change > tablespace to pg_default being already there. Right, causing it to try to drop that filenode twice. > +++ b/doc/src/sgml/ref/cluster.sgml > + The name of a specific tablespace to store clustered relations. Could you phrase these like you did in the comments: " the name of the tablespace where the clustered relation is to be rebuilt." > +++ b/doc/src/sgml/ref/reindex.sgml > + The name of a specific tablespace to store rebuilt indexes. " The name of a tablespace where indexes will be rebuilt" > +++ b/doc/src/sgml/ref/vacuum.sgml > + The name of a specific tablespace to write a new copy of the table. > + This specifies a tablespace, where all rebuilt indexes will be created. say "specifies the tablespace where", with no comma. > + else if (!OidIsValid(classtuple->relfilenode)) > + { > + /* > + * Skip all mapped relations. > + * relfilenode == 0 checks after that, similarly to > + * RelationIsMapped(). I would say "OidIsValid(relfilenode) checks for that, ..." > @@ -262,7 +280,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel) > * and error messages should refer to the operation as VACUUM not CLUSTER. > */ > void > -cluster_rel(Oid tableOid, Oid indexOid, int options) > +cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, int options) Add a comment here about the tablespaceOid parameter, like the other functions where it's added. The permission checking is kind of duplicitive, so I'd suggest to factor it out. Ideally we'd only have one place that checks for pg_global/system/mapped. It needs to check that it's not a system relation, or that system_table_mods are allowed, and in any case that if it's a mapped rel, that it's not being moved. I would pass a boolean indicating if the tablespace is being changed. Another issue is this: > +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> ] [ <replaceable class="parameter">table_and_columns</replaceable>[, ...] ] As you mentioned in your v1 patch, in the other cases, "tablespace [tablespace]" is added at the end of the command rather than in the middle. I wasn't able to make that work, maybe because "tablespace" isn't a fully reserved word (?). I didn't try with "SET TABLESPACE", although I understand it'd be better without "SET". -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 2020-03-26 21:01, Justin Pryzby wrote: > >> @@ -262,7 +280,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel) >> * and error messages should refer to the operation as VACUUM not >> CLUSTER. >> */ >> void >> -cluster_rel(Oid tableOid, Oid indexOid, int options) >> +cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, int >> options) > > Add a comment here about the tablespaceOid parameter, like the other > functions > where it's added. > > The permission checking is kind of duplicitive, so I'd suggest to > factor it > out. Ideally we'd only have one place that checks for > pg_global/system/mapped. > It needs to check that it's not a system relation, or that > system_table_mods > are allowed, and in any case that if it's a mapped rel, that it's not > being > moved. I would pass a boolean indicating if the tablespace is being > changed. > Yes, but I wanted to make sure first that all necessary validations are there to do not miss something as I did last time. I do not like repetitive code either, so I would like to introduce more common check after reviewing the code as a whole. > > Another issue is this: >> +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable >> class="parameter">new_tablespace</replaceable> ] [ <replaceable >> class="parameter">table_and_columns</replaceable> [, ...] ] > As you mentioned in your v1 patch, in the other cases, "tablespace > [tablespace]" is added at the end of the command rather than in the > middle. I > wasn't able to make that work, maybe because "tablespace" isn't a fully > reserved word (?). I didn't try with "SET TABLESPACE", although I > understand > it'd be better without "SET". > Initially I tried "SET TABLESPACE", but also failed to completely get rid of shift/reduce conflicts. I will try to rewrite VACUUM's part again with OptTableSpace. Maybe I will manage it this time. I will take into account all your text edits as well. Thanks -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
> Another issue is this: > > +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> ] [ <replaceable class="parameter">table_and_columns</replaceable>[, ...] ] > As you mentioned in your v1 patch, in the other cases, "tablespace > [tablespace]" is added at the end of the command rather than in the middle. I > wasn't able to make that work, maybe because "tablespace" isn't a fully > reserved word (?). I didn't try with "SET TABLESPACE", although I understand > it'd be better without "SET". I think we should use the parenthesized syntax for vacuum - it seems clear in hindsight. Possibly REINDEX should use that, too, instead of adding OptTablespace at the end. I'm not sure. CLUSTER doesn't support parenthesized syntax, but .. maybe it should? Also, perhaps VAC FULL (and CLUSTER, if it grows parenthesized syntax), should support something like this: USING INDEX TABLESPACE name I guess I would prefer just "index tablespace", without "using": |VACUUM(FULL, TABLESPACE ts, INDEX TABLESPACE its) t; |CLUSTER(VERBOSE, TABLESPACE ts, INDEX TABLESPACE its) t; -- Justin
On Thu, Mar 26, 2020 at 11:01:06PM -0500, Justin Pryzby wrote: > > Another issue is this: > > > +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> ] [ <replaceableclass="parameter">table_and_columns</replaceable> [, ...] ] > > As you mentioned in your v1 patch, in the other cases, "tablespace > > [tablespace]" is added at the end of the command rather than in the middle. I > > wasn't able to make that work, maybe because "tablespace" isn't a fully > > reserved word (?). I didn't try with "SET TABLESPACE", although I understand > > it'd be better without "SET". > > I think we should use the parenthesized syntax for vacuum - it seems clear in > hindsight. I implemented this last night but forgot to attach it. > Possibly REINDEX should use that, too, instead of adding OptTablespace at the > end. I'm not sure. > > CLUSTER doesn't support parenthesized syntax, but .. maybe it should? > > Also, perhaps VAC FULL (and CLUSTER, if it grows parenthesized syntax), should > support something like this: > > USING INDEX TABLESPACE name > > I guess I would prefer just "index tablespace", without "using": > > |VACUUM(FULL, TABLESPACE ts, INDEX TABLESPACE its) t; > |CLUSTER(VERBOSE, TABLESPACE ts, INDEX TABLESPACE its) t;
Attachment
On Thu, Mar 26, 2020 at 11:01:06PM -0500, Justin Pryzby wrote: > > Another issue is this: > > > +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> ] [ <replaceableclass="parameter">table_and_columns</replaceable> [, ...] ] > > As you mentioned in your v1 patch, in the other cases, "tablespace > > [tablespace]" is added at the end of the command rather than in the middle. I > > wasn't able to make that work, maybe because "tablespace" isn't a fully > > reserved word (?). I didn't try with "SET TABLESPACE", although I understand > > it'd be better without "SET". > > I think we should use the parenthesized syntax for vacuum - it seems clear in > hindsight. > > Possibly REINDEX should use that, too, instead of adding OptTablespace at the > end. I'm not sure. The attached mostly implements generic parenthesized options to REINDEX(...), so I'm soliciting opinions: should TABLESPACE be implemented in parenthesized syntax or non? > CLUSTER doesn't support parenthesized syntax, but .. maybe it should? And this ? -- Justin
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 2020-03-28 03:11, Justin Pryzby wrote: > On Thu, Mar 26, 2020 at 11:01:06PM -0500, Justin Pryzby wrote: >> > Another issue is this: >> > > +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> ] [ <replaceableclass="parameter">table_and_columns</replaceable> [, ...] ] >> > As you mentioned in your v1 patch, in the other cases, "tablespace >> > [tablespace]" is added at the end of the command rather than in the middle. I >> > wasn't able to make that work, maybe because "tablespace" isn't a fully >> > reserved word (?). I didn't try with "SET TABLESPACE", although I understand >> > it'd be better without "SET". > SET does not change anything in my experience. The problem is that opt_vacuum_relation_list is... optional and TABLESPACE is not a fully reserved word (why?) as you correctly noted. I have managed to put TABLESPACE to the end, but with vacuum_relation_list, like: | VACUUM opt_full opt_freeze opt_verbose opt_analyze vacuum_relation_list TABLESPACE name | VACUUM '(' vac_analyze_option_list ')' vacuum_relation_list TABLESPACE name It means that one would not be able to do VACUUM FULL of the entire database + TABLESPACE change. I do not think that it is a common scenario, but this limitation would be very annoying, wouldn't it? > >> >> I think we should use the parenthesized syntax for vacuum - it seems >> clear in >> hindsight. >> >> Possibly REINDEX should use that, too, instead of adding OptTablespace >> at the >> end. I'm not sure. > > The attached mostly implements generic parenthesized options to > REINDEX(...), > so I'm soliciting opinions: should TABLESPACE be implemented in > parenthesized > syntax or non? > >> CLUSTER doesn't support parenthesized syntax, but .. maybe it should? > > And this ? > Hmm, I went through the well known to me SQL commands in Postgres and a bit more. Parenthesized options list is mostly used in two common cases: - In the beginning for boolean options only, e.g. VACUUM - In the end for options of a various type, but accompanied by WITH, e.g. COPY, CREATE SUBSCRIPTION Moreover, TABLESPACE is already used in CREATE TABLE/INDEX in the same way I did in 0001-0002. That way, putting TABLESPACE option into the parenthesized options list does not look to be convenient and semantically correct, so I do not like it. Maybe others will have a different opinion. Putting it into the WITH (...) options list looks like an option to me. However, doing it only for VACUUM will ruin the consistency, while doing it for CLUSTER and REINDEX is not necessary, so I do not like it either. To summarize, currently I see only 2 + 1 extra options: 1) Keep everything with syntax as it is in 0001-0002 2) Implement tail syntax for VACUUM, but with limitation for VACUUM FULL of the entire database + TABLESPACE change 3) Change TABLESPACE to a fully reserved word Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
On Mon, Mar 30, 2020 at 09:02:22PM +0300, Alexey Kondratov wrote: > Hmm, I went through the well known to me SQL commands in Postgres and a bit > more. Parenthesized options list is mostly used in two common cases: There's also ANALYZE(VERBOSE), REINDEX(VERBOSE). There was debate a year ago [0] as to whether to make "reindex CONCURRENTLY" a separate command, or to use parenthesized syntax "REINDEX (CONCURRENTLY)". I would propose to support that now (and implemented that locally). ..and explain(...) > - In the beginning for boolean options only, e.g. VACUUM You're right that those are currently boolean, but note that explain(FORMAT ..) is not boolean. > Putting it into the WITH (...) options list looks like an option to me. > However, doing it only for VACUUM will ruin the consistency, while doing it > for CLUSTER and REINDEX is not necessary, so I do not like it either. It's not necessary but I think it's a more flexible way to add new functionality (requiring no changes to the grammar for vacuum, and for REINDEX/CLUSTER it would allow future options to avoid changing the grammar). If we use parenthesized syntax for vacuum, my proposal is to do it for REINDEX, and consider adding parenthesized syntax for cluster, too. > To summarize, currently I see only 2 + 1 extra options: > > 1) Keep everything with syntax as it is in 0001-0002 > 2) Implement tail syntax for VACUUM, but with limitation for VACUUM FULL of > the entire database + TABLESPACE change > 3) Change TABLESPACE to a fully reserved word + 4) Use parenthesized syntax for all three. Note, I mentioned that maybe VACUUM/CLUSTER should support not only "TABLESPACE foo" but also "INDEX TABLESPACE bar" (I would use that, too). I think that would be easy to implement, and for sure it would suggest using () for both. (For sure we don't want to implement "VACUUM t TABLESPACE foo" now, and then later implement "INDEX TABLESPACE bar" and realize that for consistency we cannot parenthesize it. Michael ? Alvaro ? Robert ? -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 2020-03-30 21:34, Justin Pryzby wrote: > On Mon, Mar 30, 2020 at 09:02:22PM +0300, Alexey Kondratov wrote: >> Hmm, I went through the well known to me SQL commands in Postgres and >> a bit >> more. Parenthesized options list is mostly used in two common cases: > > There's also ANALYZE(VERBOSE), REINDEX(VERBOSE). > There was debate a year ago [0] as to whether to make "reindex > CONCURRENTLY" a > separate command, or to use parenthesized syntax "REINDEX > (CONCURRENTLY)". I > would propose to support that now (and implemented that locally). > I am fine with allowing REINDEX (CONCURRENTLY), but then we will have to support both syntaxes as we already do for VACUUM. Anyway, if we agree to add parenthesized options to REINDEX/CLUSTER, then it should be done as a separated patch before the current patch set. > > ..and explain(...) > >> - In the beginning for boolean options only, e.g. VACUUM > > You're right that those are currently boolean, but note that > explain(FORMAT ..) > is not boolean. > Yep, I forgot EXPLAIN, this is a good example. > > .. and create table (LIKE ..) > LIKE is used in the table definition, so it is a slightly different case. > >> Putting it into the WITH (...) options list looks like an option to >> me. >> However, doing it only for VACUUM will ruin the consistency, while >> doing it >> for CLUSTER and REINDEX is not necessary, so I do not like it either. > > It's not necessary but I think it's a more flexible way to add new > functionality (requiring no changes to the grammar for vacuum, and for > REINDEX/CLUSTER it would allow future options to avoid changing the > grammar). > > If we use parenthesized syntax for vacuum, my proposal is to do it for > REINDEX, and > consider adding parenthesized syntax for cluster, too. > >> To summarize, currently I see only 2 + 1 extra options: >> >> 1) Keep everything with syntax as it is in 0001-0002 >> 2) Implement tail syntax for VACUUM, but with limitation for VACUUM >> FULL of >> the entire database + TABLESPACE change >> 3) Change TABLESPACE to a fully reserved word > > + 4) Use parenthesized syntax for all three. > > Note, I mentioned that maybe VACUUM/CLUSTER should support not only > "TABLESPACE > foo" but also "INDEX TABLESPACE bar" (I would use that, too). I think > that > would be easy to implement, and for sure it would suggest using () for > both. > (For sure we don't want to implement "VACUUM t TABLESPACE foo" now, and > then > later implement "INDEX TABLESPACE bar" and realize that for consistency > we > cannot parenthesize it. > > Michael ? Alvaro ? Robert ? > Yes, I would be glad to hear other opinions too, before doing this preliminary refactoring. -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Michael Paquier
Date:
On Tue, Mar 31, 2020 at 01:56:07PM +0300, Alexey Kondratov wrote: > I am fine with allowing REINDEX (CONCURRENTLY), but then we will have to > support both syntaxes as we already do for VACUUM. Anyway, if we agree to > add parenthesized options to REINDEX/CLUSTER, then it should be done as a > separated patch before the current patch set. Last year for the patch for REINDEX CONCURRENTLY, we had the argument of supporting only the parenthesized grammar or not, and the choice has been made to use what we have now, as you mentioned upthread. I would honestly prefer that for now on we only add the parenthesized version of an option if something new is added to such utility commands (vacuum, analyze, reindex, etc.) as that's much more extensible from the point of view of the parser. And this, even if you need to rework things a bit more things around reindex_option_elem for the tablespace option proposed here. -- Michael
Attachment
On Wed, Apr 01, 2020 at 03:03:34PM +0900, Michael Paquier wrote: > On Tue, Mar 31, 2020 at 01:56:07PM +0300, Alexey Kondratov wrote: > > I am fine with allowing REINDEX (CONCURRENTLY), but then we will have to > > support both syntaxes as we already do for VACUUM. Anyway, if we agree to > > add parenthesized options to REINDEX/CLUSTER, then it should be done as a > > separated patch before the current patch set. > > would honestly prefer that for now on we only add the parenthesized > version of an option if something new is added to such utility > commands (vacuum, analyze, reindex, etc.) as that's much more > extensible from the point of view of the parser. And this, even if > you need to rework things a bit more things around > reindex_option_elem for the tablespace option proposed here. Thanks for your input. I'd already converted VACUUM and REINDEX to use a parenthesized TABLESPACE option, and just converted CLUSTER to take an option list and do the same. Alexey suggested that those changes should be done as a separate patch, with the tablespace options built on top. Which makes sense. I had quite some fun rebasing these with patches in that order. However, I've kept my changes separate from Alexey's patch, to make it easier for him to integrate. So there's "fix!" commits which are not logically separate and should be read as if they're merged with their parent commits. That makes the patchset look kind of dirty. So I'm first going to send the "before rebase" patchset. There's a few fixme items, but I think this is in pretty good shape, and I'd appreciate review. I'll follow up later with the "after rebase" patchset. Maybe Alexey will want to integrate that. I claimed it would be easy, so I also implemented (INDEX_TABESPACE ..) option: template1=# VACUUM (TABLESPACE pg_default, INDEX_TABLESPACE ts, FULL) t; template1=# CLUSTER (TABLESPACE pg_default, INDEX_TABLESPACE ts) t; -- Justin
Attachment
- v15-0001-Allow-REINDEX-to-change-tablespace.patch
- v15-0002-Allow-CLUSTER-and-VACUUM-FULL-to-change-tablespa.patch
- v15-0003-fixes2.patch
- v15-0004-Parenthesized-syntax-VACUUM-FULL-TABLESPACE.patch
- v15-0005-Change-REINDEX-CLUSTER-to-accept-an-option-list.patch
- v15-0006-Implement-VACUUM-FULL-CLUSTER-INDEX_TABLESPACE-t.patch
On Wed, Apr 01, 2020 at 06:57:18AM -0500, Justin Pryzby wrote: > Alexey suggested that those changes should be done as a separate patch, with > the tablespace options built on top. Which makes sense. I had quite some fun > rebasing these with patches in that order. > > However, I've kept my changes separate from Alexey's patch, to make it easier > for him to integrate. So there's "fix!" commits which are not logically > separate and should be read as if they're merged with their parent commits. > That makes the patchset look kind of dirty. So I'm first going to send the > "before rebase" patchset. There's a few fixme items, but I think this is in > pretty good shape, and I'd appreciate review. > > I'll follow up later with the "after rebase" patchset. Attached. As I said, the v15 patches might be easier to review, even though v16 is closer to what's desirable to merge. > Maybe Alexey will want to integrate that. Or maybe you'd want me to squish my changes into yours and resend after any review comments ? -- Justin
Attachment
- v16-0001-Change-REINDEX-CLUSTER-to-accept-an-option-list.patch
- v16-0002-Allow-REINDEX-to-change-tablespace.patch
- v16-0003-fix-Parenthesized-syntax-REINDEX-TABLESPACE.patch
- v16-0004-Allow-CLUSTER-and-VACUUM-FULL-to-change-tablespa.patch
- v16-0005-fixes2.patch
- v16-0006-fix-Parenthesized-syntax-VACUUM-CLUSTER-TABLESPA.patch
- v16-0007-Implement-vacuum-full-cluster-INDEX_TABLESPACE-t.patch
On Wed, Apr 01, 2020 at 08:08:36AM -0500, Justin Pryzby wrote: > Or maybe you'd want me to squish my changes into yours and resend after any > review comments ? I didn't hear any feedback, so I've now squished all "parenthesized" and "fix" commits. 0004 reduces duplicative error handling, as a separate commit so Alexey can review it and/or integrate it. The last two commits save a few dozen lines of code, but not sure they're desirable. As this changes REINDEX/CLUSTER to allow parenthesized options, it might be pretty reasonable if someone were to kick this to the July CF. -- Justin
Attachment
- v17-0001-tab-completion-for-reindex-verbose.patch
- v17-0002-Change-REINDEX-CLUSTER-to-accept-an-option-list.patch
- v17-0003-Allow-REINDEX-to-change-tablespace.patch
- v17-0004-indexcmds-remove-redundant-checks.patch
- v17-0005-Allow-CLUSTER-and-VACUUM-FULL-to-change-tablespa.patch
- v17-0006-Implement-vacuum-full-cluster-INDEX_TABLESPACE-t.patch
- v17-0007-pass-idxtablespaceNAME-to-reduce-error-duplicati.patch
- v17-0008-Also-pass-the-table-s-tablespace-as-char.patch
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 2020-04-03 21:27, Justin Pryzby wrote: > On Wed, Apr 01, 2020 at 08:08:36AM -0500, Justin Pryzby wrote: >> Or maybe you'd want me to squish my changes into yours and resend >> after any >> review comments ? > > I didn't hear any feedback, so I've now squished all "parenthesized" > and "fix" > commits. > Thanks for the input, but I am afraid that the patch set became a bit messy now. I have eyeballed it and found some inconsistencies. const char *name; /* name of database to reindex */ - int options; /* Reindex options flags */ + List *rawoptions; /* Raw options */ + int options; /* Parsed options */ bool concurrent; /* reindex concurrently? */ You introduced rawoptions in the 0002, but then removed it in 0003. So is it required or not? Probably this is a rebase artefact. +/* XXX: reusing reindex_option_list */ + | CLUSTER opt_verbose '(' reindex_option_list ')' qualified_name cluster_index_specification Could we actually simply reuse vac_analyze_option_list? From the first sight it does just the right thing, excepting the special handling of spelling ANALYZE/ANALYSE, but it does not seem to be a problem. > > 0004 reduces duplicative error handling, as a separate commit so > Alexey can review it and/or integrate it. > @@ -2974,27 +2947,6 @@ ReindexRelationConcurrently(Oid relationOid, Oid tablespaceOid, int options) /* Open relation to get its indexes */ heapRelation = table_open(relationOid, ShareUpdateExclusiveLock); - /* - * We don't support moving system relations into different tablespaces, - * unless allow_system_table_mods=1. - */ - if (OidIsValid(tablespaceOid) && - !allowSystemTableMods && IsSystemRelation(heapRelation)) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("permission denied: \"%s\" is a system catalog", - RelationGetRelationName(heapRelation)))); ReindexRelationConcurrently is used for all cases, but it hits different code paths in the case of database, table and index. I have not checked yet, but are you sure it is safe removing these validations in the case of REINDEX CONCURRENTLY? > > The last two commits save a few > dozen lines of code, but not sure they're desirable. > Sincerely, I do not think that passing raw strings down to the guts is a good idea. Yes, it saves us a few checks here and there now, but it may reduce a further reusability of these internal routines in the future. > > XXX: for cluster/vacuum, it might be more friendly to check before > clustering > the table, rather than after clustering and re-indexing. > Yes, I think it would be much more user-friendly. -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
On Mon, Apr 06, 2020 at 08:43:46PM +0300, Alexey Kondratov wrote: > Thanks for the input, but I am afraid that the patch set became a bit messy > now. I have eyeballed it and found some inconsistencies. > > const char *name; /* name of database to reindex */ > - int options; /* Reindex options flags */ > + List *rawoptions; /* Raw options */ > + int options; /* Parsed options */ > bool concurrent; /* reindex concurrently? */ > > You introduced rawoptions in the 0002, but then removed it in 0003. So is it > required or not? Probably this is a rebase artefact. You're right; I first implemented REINDEX() and when I later did CLUSTER(), I did it better, so I went back and did REINDEX() that way, but it looks like I maybe fixup!ed the wrong commit. Fixed now. > +/* XXX: reusing reindex_option_list */ > + | CLUSTER opt_verbose '(' reindex_option_list ')' qualified_name > cluster_index_specification > > Could we actually simply reuse vac_analyze_option_list? From the first sight > it does just the right thing, excepting the special handling of spelling > ANALYZE/ANALYSE, but it does not seem to be a problem. Hm, do you mean to let cluster.c reject the other options like "analyze" ? I'm not sure why that would be better than reusing reindex? I think the suggestion will probably be to just copy+paste the reindex option list and rename it to cluster (possibly with the explanation that they're separate and independant and so their behavior shouldn't be tied together). > > 0004 reduces duplicative error handling, as a separate commit so > > Alexey can review it and/or integrate it. > > ReindexRelationConcurrently is used for all cases, but it hits different > code paths in the case of database, table and index. I have not checked yet, > but are you sure it is safe removing these validations in the case of > REINDEX CONCURRENTLY? You're right about the pg_global case, fixed. System catalogs can't be reindexed CONCURRENTLY, so they're already caught by that check. > > XXX: for cluster/vacuum, it might be more friendly to check before > > clustering > > the table, rather than after clustering and re-indexing. > > Yes, I think it would be much more user-friendly. I realized it's not needed or useful to check indexes in advance of clustering, since 1) a mapped index will be on a mapped relation, which is already checked; 2) a system index will be on a system relation. Right ? -- we already knew that ts=# SELECT COUNT(1) FROM pg_index i JOIN pg_class a ON i.indrelid=a.oid JOIN pg_class b ON i.indexrelid=b.oid WHERE a.relnamespace!=b.relnamespace; count | 0 -- not true in general, but true here and true for system relations ts=# SELECT COUNT(1) FROM pg_index i JOIN pg_class a ON i.indrelid=a.oid JOIN pg_class b ON i.indexrelid=b.oid WHERE a.reltablespace!= b.reltablespace; count | 0 -- Justin
Attachment
- v18-0001-tab-completion-for-reindex-verbose.patch
- v18-0002-Change-REINDEX-CLUSTER-to-accept-an-option-list.patch
- v18-0003-Allow-REINDEX-to-change-tablespace.patch
- v18-0004-indexcmds-remove-redundant-checks.patch
- v18-0005-Allow-CLUSTER-and-VACUUM-FULL-to-change-tablespa.patch
- v18-0006-Implement-vacuum-full-cluster-INDEX_TABLESPACE-t.patch
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Alexey Kondratov
Date:
On 2020-04-06 21:44, Justin Pryzby wrote: > On Mon, Apr 06, 2020 at 08:43:46PM +0300, Alexey Kondratov wrote: >> >> +/* XXX: reusing reindex_option_list */ >> + | CLUSTER opt_verbose '(' reindex_option_list ')' qualified_name >> cluster_index_specification >> >> Could we actually simply reuse vac_analyze_option_list? From the first >> sight >> it does just the right thing, excepting the special handling of >> spelling >> ANALYZE/ANALYSE, but it does not seem to be a problem. > > Hm, do you mean to let cluster.c reject the other options like > "analyze" ? > I'm not sure why that would be better than reusing reindex? > I think the suggestion will probably be to just copy+paste the reindex > option > list and rename it to cluster (possibly with the explanation that > they're > separate and independant and so their behavior shouldn't be tied > together). > I mean to literally use vac_analyze_option_list for reindex and cluster as well. Please, check attached 0007. Now, vacuum, reindex and cluster filter options list and reject everything that is not supported, so it seems completely fine to just reuse vac_analyze_option_list, doesn't it? >> >> ReindexRelationConcurrently is used for all cases, but it hits >> different >> code paths in the case of database, table and index. I have not >> checked yet, >> but are you sure it is safe removing these validations in the case of >> REINDEX CONCURRENTLY? > > You're right about the pg_global case, fixed. System catalogs can't be > reindexed CONCURRENTLY, so they're already caught by that check. > >> > XXX: for cluster/vacuum, it might be more friendly to check before >> > clustering >> > the table, rather than after clustering and re-indexing. >> >> Yes, I think it would be much more user-friendly. > > I realized it's not needed or useful to check indexes in advance of > clustering, > since 1) a mapped index will be on a mapped relation, which is already > checked; > 2) a system index will be on a system relation. Right ? > Yes, it seems that you are right. I have tried to create user index on system relation with allow_system_table_mods=1, but this new index appeared to become system as well. That way, we do not have to check indexes in advance. -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Attachment
On Tue, Apr 07, 2020 at 03:40:18PM +0300, Alexey Kondratov wrote: > On 2020-04-06 21:44, Justin Pryzby wrote: > > On Mon, Apr 06, 2020 at 08:43:46PM +0300, Alexey Kondratov wrote: > > > > > > +/* XXX: reusing reindex_option_list */ > > > + | CLUSTER opt_verbose '(' reindex_option_list ')' qualified_name > > > cluster_index_specification > > > > > > Could we actually simply reuse vac_analyze_option_list? From the first > > > sight it does just the right thing, excepting the special handling of > > > spelling ANALYZE/ANALYSE, but it does not seem to be a problem. > > > > Hm, do you mean to let cluster.c reject the other options like "analyze" ? > > I'm not sure why that would be better than reusing reindex? I think the > > suggestion will probably be to just copy+paste the reindex option list and > > rename it to cluster (possibly with the explanation that they're separate > > and independant and so their behavior shouldn't be tied together). > > I mean to literally use vac_analyze_option_list for reindex and cluster as > well. Please, check attached 0007. Now, vacuum, reindex and cluster filter > options list and reject everything that is not supported, so it seems > completely fine to just reuse vac_analyze_option_list, doesn't it? It's fine with me :) Possibly we could rename vac_analyze_option_list as generic_option_list. I'm resending the patchset like that, and fixed cluster/index to handle not just "VERBOSE" but "verbose OFF", rather than just ignoring the argument. That's the last known issue with the patch. I doubt anyone will elect to pick it up in the next 8 hours, but I think it's in very good shape for v14 :) BTW, if you resend a *.patch or *.diff file to a thread, it's best to also include all the previous patches. Otherwise the CF bot is likely to complain that the patch "doesn't apply", or else it'll only test the one patch instead of the whole series. http://cfbot.cputube.org/alexey-kondratov.html -- Justin
Attachment
On Tue, Apr 07, 2020 at 03:44:06PM -0500, Justin Pryzby wrote: > > I mean to literally use vac_analyze_option_list for reindex and cluster as > > well. Please, check attached 0007. Now, vacuum, reindex and cluster filter > > options list and reject everything that is not supported, so it seems > > completely fine to just reuse vac_analyze_option_list, doesn't it? > > It's fine with me :) > > Possibly we could rename vac_analyze_option_list as generic_option_list. > > I'm resending the patchset like that, and fixed cluster/index to handle not > just "VERBOSE" but "verbose OFF", rather than just ignoring the argument. > > That's the last known issue with the patch. I doubt anyone will elect to pick > it up in the next 8 hours, but I think it's in very good shape for v14 :) I tweaked some comments and docs and plan to mark this RfC. -- Justin
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
From
Michael Paquier
Date:
On Sat, Apr 11, 2020 at 08:33:52PM -0500, Justin Pryzby wrote: > On Tue, Apr 07, 2020 at 03:44:06PM -0500, Justin Pryzby wrote: >> That's the last known issue with the patch. I doubt anyone will elect to pick >> it up in the next 8 hours, but I think it's in very good shape for v14 :) > > I tweaked some comments and docs and plan to mark this RfC. Yeah, unfortunately this will have to wait at least until v14 opens for business :( -- Michael
Attachment
On Sat, Apr 11, 2020 at 08:33:52PM -0500, Justin Pryzby wrote: > > That's the last known issue with the patch. I doubt anyone will elect to pick > > it up in the next 8 hours, but I think it's in very good shape for v14 :) > > I tweaked some comments and docs and plan to mark this RfC. Rebased onto d12bdba77b0fce9df818bc84ad8b1d8e7a96614b Restored two tests from Alexey's original patch which exposed issue with REINDEX DATABASE when allow_system_table_mods=off. -- Justin
Attachment
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
From
Alexey Kondratov
Date:
On 2020-09-01 13:12, Justin Pryzby wrote: > This patch seems to be missing a call to RelationAssumeNewRelfilenode() > in > reindex_index(). > > That's maybe the related to the cause of the crashes I pointed out > earlier this > year. > > Alexey's v4 patch changed RelationSetNewRelfilenode() to accept a > tablespace > parameter, but Michael seemed to object to that. However that seems > cleaner > and ~30 line shorter. > > Michael, would you comment on that ? The v4 patch and your comments > are here. > https://www.postgresql.org/message-id/attachment/105574/v4-0001-Allow-REINDEX-and-REINDEX-CONCURRENTLY-to-change-tablespace.patch > https://www.postgresql.org/message-id/20191127035416.GG5435%40paquier.xyz > Actually, the last time we discussed this point I only got the gut feeling that this is a subtle place and it is very easy to break things with these changes. However, it isn't clear for me how exactly. That way, I'd be glad if Michael could reword his explanation, so it'd more clear for me as well. BTW, I've started doing a review of the last patch set yesterday and will try to post some comments later. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company