Обсуждение: 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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.
 

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> 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

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;

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

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

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

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

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

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

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

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

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

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

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

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

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

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