Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly

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 Alexey
Here are a few comment
On Sat, Aug 31, 2019 at 11:54 PM <a.kondratov@postgrespro.ru> wrote:

Hi hackers,

Anyway, new patch, which adds SET TABLESPACE to REINDEX is attached and
this functionality seems really useful, so I will be very appreciate if
someone will take a look on it.

* There are NOWAIT option in alter index, is there a reason not to have
similar option here?
* SET TABLESPACE command is not documented
* There are multiple checking for whether the relation is temporary tables
of other sessions, one in check_relation_is_movable and other independently

*+ char *tablespacename;

calling it new_tablespacename will make it consistent with other places

*The patch did't applied cleanly http://cfbot.cputube.org/patch_24_2269.log

regards

Surafel

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]/messages/by-id/6b2a5c4de19f111ef24b63428033bb67@postgrespro.ru. 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]: /messages/by-id/6b2a5c4de19f111ef24b63428033bb67@postgrespro.ru
/messages/by-id/6b2a5c4de19f111ef24b63428033bb67@postgrespro.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 or somehow 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 it or 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 bad idea.

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 it or 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) /messages/by-id/20200208150453.GV403@telsasoft.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> ] [ <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.

I implemented this last night but forgot to attach it.

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> ] [ <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.

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> ] [ <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".

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 Sun, Apr 26, 2020 at 12:56:14PM -0500, Justin Pryzby wrote:

Rebased onto d12bdba77b0fce9df818bc84ad8b1d8e7a96614b

Restored two tests from Alexey's original patch which exposed issue with
REINDEX DATABASE when allow_system_table_mods=off.

I have been looking at 0001 as a start, and your patch is incorrect on
a couple of aspects for the completion of REINDEX:
- "(" is not proposed as a completion option after the initial
REINDEX, and I think that it should.
- completion gets incorrect for all the commands once a parenthesized
list of options is present, as CONCURRENTLY goes missing.

The presence of CONCURRENTLY makes the completion a bit more complex
than the other commands, as we need to add this keyword if still not
specified with the other objects of the wanted type to reindex, but it
can be done as the attached. What do you think?
--
Michael

On Sun, Aug 09, 2020 at 08:02:52PM +0900, Michael Paquier wrote:

I have been looking at 0001 as a start, and your patch is incorrect on
a couple of aspects for the completion of REINDEX:
- "(" is not proposed as a completion option after the initial
REINDEX, and I think that it should.

That part of your patch handles REINDEX and REINDEX(*) differently than mine.
Yours is technically more correct/complete. But, I recall Tom objected a
different patch because of completing to a single char. I think the case is
arguable either way: if only some completions are shown, then it hides the
others..
/messages/by-id/14255.1536781029@sss.pgh.pa.us

-       else if (Matches("REINDEX") || Matches("REINDEX", "(*)"))
+       else if (Matches("REINDEX"))
+               COMPLETE_WITH("TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", "(");
+       else if (Matches("REINDEX", "(*)"))
                COMPLETE_WITH("TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE");

- completion gets incorrect for all the commands once a parenthesized
list of options is present, as CONCURRENTLY goes missing.

The rest of your patch looks fine. In my mind, REINDEX(CONCURRENTLY) was the
"new way" to write things, and it's what's easy to support, so I think I didn't
put special effort into making tab completion itself complete.

--
Justin

On Sun, Aug 09, 2020 at 09:24:43PM -0500, Justin Pryzby wrote:

That part of your patch handles REINDEX and REINDEX(*) differently than mine.
Yours is technically more correct/complete. But, I recall Tom objected a
different patch because of completing to a single char. I think the case is
arguable either way: if only some completions are shown, then it hides the
others..
/messages/by-id/14255.1536781029@sss.pgh.pa.us

Thanks for the reference. Indeed, I can see this argument going both
ways. Now showing "(" after typing REINDEX as a completion option
does not let the user know that parenthesized options are supported,
but on the contrary this can also clutter the output. The latter
makes more sense now to be consistent with VACUUM and ANALYZE though,
so I have removed that part, and applied the patch.

The rest of your patch looks fine. In my mind, REINDEX(CONCURRENTLY) was the
"new way" to write things, and it's what's easy to support, so I think I didn't
put special effort into making tab completion itself complete.

The grammar that has been committed was the one that for the most
support, so we need to live with that. I wonder if we should simplify
ReindexStmt and move the "concurrent" flag to be under "options", but
that may not be worth the time spent on as long as we don't have
CONCURRENTLY part of the parenthesized grammar.
--
Michael

On Tue, Aug 11, 2020 at 02:39:45PM +0900, Michael Paquier wrote:

so I have removed that part, and applied the patch.

Thank you

The rest of your patch looks fine. In my mind, REINDEX(CONCURRENTLY) was the
"new way" to write things, and it's what's easy to support, so I think I didn't
put special effort into making tab completion itself complete.

The grammar that has been committed was the one that for the most
support, so we need to live with that. I wonder if we should simplify
ReindexStmt and move the "concurrent" flag to be under "options", but
that may not be worth the time spent on as long as we don't have
CONCURRENTLY part of the parenthesized grammar.

I think it's kind of a good idea, since the next patch does exactly that
(parenthesize (CONCURRENTLY)).

I included that as a new 0002, but it doesn't save anything though, so maybe
it's not a win.

$ git diff --stat
src/backend/commands/indexcmds.c | 20 +++++++++++---------
src/backend/nodes/copyfuncs.c | 1 -
src/backend/nodes/equalfuncs.c | 1 -
src/backend/parser/gram.y | 16 ++++++++++++----
src/backend/tcop/utility.c | 6 +++---
src/include/nodes/parsenodes.h | 2 +-
6 files changed, 27 insertions(+), 19 deletions(-)

--
Justin

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.
/messages/by-id/attachment/105574/v4-0001-Allow-REINDEX-and-REINDEX-CONCURRENTLY-to-change-tablespace.patch
/messages/by-id/20191127035416.GG5435@paquier.xyz

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.
/messages/by-id/attachment/105574/v4-0001-Allow-REINDEX-and-REINDEX-CONCURRENTLY-to-change-tablespace.patch
/messages/by-id/20191127035416.GG5435@paquier.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

On 2020-Aug-11, Justin Pryzby wrote:

On Tue, Aug 11, 2020 at 02:39:45PM +0900, Michael Paquier wrote:

The grammar that has been committed was the one that for the most
support, so we need to live with that. I wonder if we should simplify
ReindexStmt and move the "concurrent" flag to be under "options", but
that may not be worth the time spent on as long as we don't have
CONCURRENTLY part of the parenthesized grammar.

I think it's kind of a good idea, since the next patch does exactly that
(parenthesize (CONCURRENTLY)).

I included that as a new 0002, but it doesn't save anything though, so maybe
it's not a win.

The advantage of using a parenthesized option list is that you can add
*further* options without making the new keywords reserved. Of course,
we already reserve CONCURRENTLY and VERBOSE pretty severely, so there's
no change. If you wanted REINDEX FLUFFY then it wouldn't work without
making that at least type_func_name_keyword I think; but REINDEX
(FLUFFY) would work just fine. And of course the new feature at hand
can be implemented.

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

On Tue, Sep 01, 2020 at 11:40:18AM -0400, Alvaro Herrera wrote:

On 2020-Aug-11, Justin Pryzby wrote:

On Tue, Aug 11, 2020 at 02:39:45PM +0900, Michael Paquier wrote:

The grammar that has been committed was the one that for the most
support, so we need to live with that. I wonder if we should simplify
ReindexStmt and move the "concurrent" flag to be under "options", but
that may not be worth the time spent on as long as we don't have
CONCURRENTLY part of the parenthesized grammar.

I think it's kind of a good idea, since the next patch does exactly that
(parenthesize (CONCURRENTLY)).

I included that as a new 0002, but it doesn't save anything though, so maybe
it's not a win.

The advantage of using a parenthesized option list is that you can add
*further* options without making the new keywords reserved. Of course,
we already reserve CONCURRENTLY and VERBOSE pretty severely, so there's
no change. If you wanted REINDEX FLUFFY then it wouldn't work without
making that at least type_func_name_keyword I think; but REINDEX
(FLUFFY) would work just fine. And of course the new feature at hand
can be implemented.

The question isn't whether to use a parenthesized option list. I realized that
long ago (even though Alexey didn't initially like it). Check 0002, which gets
rid of "bool concurrent" in favour of stmt->options&REINDEXOPT_CONCURRENT.

--
Justin

On 2020-Sep-01, Justin Pryzby wrote:

On Tue, Sep 01, 2020 at 11:40:18AM -0400, Alvaro Herrera wrote:

The advantage of using a parenthesized option list is that you can add
*further* options without making the new keywords reserved. Of course,
we already reserve CONCURRENTLY and VERBOSE pretty severely, so there's
no change. If you wanted REINDEX FLUFFY then it wouldn't work without
making that at least type_func_name_keyword I think; but REINDEX
(FLUFFY) would work just fine. And of course the new feature at hand
can be implemented.

The question isn't whether to use a parenthesized option list. I realized that
long ago (even though Alexey didn't initially like it). Check 0002, which gets
rid of "bool concurrent" in favour of stmt->options&REINDEXOPT_CONCURRENT.

Ah! I see, sorry for the noise. Well, respectfully, having a separate
boolean to store one option when you already have a bitmask for options
is silly.

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

On Tue, Sep 01, 2020 at 11:48:30AM -0400, Alvaro Herrera wrote:

On 2020-Sep-01, Justin Pryzby wrote:

The question isn't whether to use a parenthesized option list. I realized that
long ago (even though Alexey didn't initially like it). Check 0002, which gets
rid of "bool concurrent" in favour of stmt->options&REINDEXOPT_CONCURRENT.

Ah! I see, sorry for the noise. Well, respectfully, having a separate
boolean to store one option when you already have a bitmask for options
is silly.

Yeah, I am all for removing "concurrent" from ReindexStmt, but I don't
think that the proposed 0002 is that, because it is based on the
assumption that we'd want more than just boolean-based options in
those statements, and this case is not justified yet except if it
becomes possible to enforce tablespaces. At this stage, I think that
it is more sensible to just update gram.y and add a
REINDEXOPT_CONCURRENTLY. I also think that it would also make sense
to pass down "options" within ReindexIndexCallbackState() (for example
imagine a SKIP_LOCKED for REINDEX).
--
Michael

On 2020-Sep-02, Michael Paquier wrote:

Yeah, I am all for removing "concurrent" from ReindexStmt, but I don't
think that the proposed 0002 is that, because it is based on the
assumption that we'd want more than just boolean-based options in
those statements, and this case is not justified yet except if it
becomes possible to enforce tablespaces. At this stage, I think that
it is more sensible to just update gram.y and add a
REINDEXOPT_CONCURRENTLY.

Yes, agreed. I had not seen the "params" business.

I also think that it would also make sense to pass down "options"
within ReindexIndexCallbackState() (for example imagine a SKIP_LOCKED
for REINDEX).

Seems sensible, but only to be done when actually needed, right?

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

On Tue, Sep 01, 2020 at 09:29:28PM -0400, Alvaro Herrera wrote:

Seems sensible, but only to be done when actually needed, right?

Of course.
--
Michael

On Wed, Sep 02, 2020 at 10:00:12AM +0900, Michael Paquier wrote:

On Tue, Sep 01, 2020 at 11:48:30AM -0400, Alvaro Herrera wrote:

On 2020-Sep-01, Justin Pryzby wrote:

The question isn't whether to use a parenthesized option list. I realized that
long ago (even though Alexey didn't initially like it). Check 0002, which gets
rid of "bool concurrent" in favour of stmt->options&REINDEXOPT_CONCURRENT.

Ah! I see, sorry for the noise. Well, respectfully, having a separate
boolean to store one option when you already have a bitmask for options
is silly.

Yeah, I am all for removing "concurrent" from ReindexStmt, but I don't
think that the proposed 0002 is that, because it is based on the
assumption that we'd want more than just boolean-based options in
those statements, and this case is not justified yet except if it
becomes possible to enforce tablespaces. At this stage, I think that
it is more sensible to just update gram.y and add a
REINDEXOPT_CONCURRENTLY. I also think that it would also make sense
to pass down "options" within ReindexIndexCallbackState() (for example
imagine a SKIP_LOCKED for REINDEX).

Uh, this whole thread is about implementing REINDEX (TABLESPACE foo), and the
preliminary patch 0001 is to keep separate the tablespace parts of that
content. 0002 is a minor tangent which I assume would be squished into 0001
which cleans up historic cruft, using new params in favour of historic options.

I think my change is probably incomplete, and ReindexStmt node should not have
an int options. parse_reindex_params() would parse it into local int *options
and char **tablespacename params.

--
Justin

On Tue, Sep 01, 2020 at 09:24:10PM -0500, Justin Pryzby wrote:

On Wed, Sep 02, 2020 at 10:00:12AM +0900, Michael Paquier wrote:

On Tue, Sep 01, 2020 at 11:48:30AM -0400, Alvaro Herrera wrote:

On 2020-Sep-01, Justin Pryzby wrote:

The question isn't whether to use a parenthesized option list. I realized that
long ago (even though Alexey didn't initially like it). Check 0002, which gets
rid of "bool concurrent" in favour of stmt->options&REINDEXOPT_CONCURRENT.

Ah! I see, sorry for the noise. Well, respectfully, having a separate
boolean to store one option when you already have a bitmask for options
is silly.

Yeah, I am all for removing "concurrent" from ReindexStmt, but I don't
think that the proposed 0002 is that, because it is based on the
assumption that we'd want more than just boolean-based options in
those statements, and this case is not justified yet except if it
becomes possible to enforce tablespaces. At this stage, I think that
it is more sensible to just update gram.y and add a
REINDEXOPT_CONCURRENTLY. I also think that it would also make sense
to pass down "options" within ReindexIndexCallbackState() (for example
imagine a SKIP_LOCKED for REINDEX).

Uh, this whole thread is about implementing REINDEX (TABLESPACE foo), and the
preliminary patch 0001 is to keep separate the tablespace parts of that
content. 0002 is a minor tangent which I assume would be squished into 0001
which cleans up historic cruft, using new params in favour of historic options.

I think my change is probably incomplete, and ReindexStmt node should not have
an int options. parse_reindex_params() would parse it into local int *options
and char **tablespacename params.

Done in the attached, which is also rebased on 1d6541666.

And added RelationAssumeNewRelfilenode() as I mentioned - but I'm hoping to
hear from Michael about any reason not to call RelationSetNewRelfilenode()
instead of directly calling the things it would itself call.

--
Justin

On 2020-09-02 07:56, Justin Pryzby wrote:

Done in the attached, which is also rebased on 1d6541666.

And added RelationAssumeNewRelfilenode() as I mentioned - but I'm
hoping to
hear from Michael about any reason not to call
RelationSetNewRelfilenode()
instead of directly calling the things it would itself call.

The latest patch set immediately got a conflict with Michael's fixup
01767533, so I've rebased it first of all.

+      Prints a progress report as each table is clustered.
+<!-- When specified within parenthesis, <literal>VERBOSE</literal> may 
be followed by a boolean ...-->

I think that we can remove this comment completely as it is already
explained in the docs later.

+			| CLUSTER opt_verbose '(' vac_analyze_option_list ')' qualified_name 
cluster_index_specification
+				{

What's the point in allowing a mixture of old options with new
parenthesized option list? VACUUM doesn't do so. I can understand it for
REINDEX CONCURRENTLY, since parenthesized options were already there,
but not for CLUSTER.

With v23 it is possible to write:

CLUSTER VERBOSE (VERBOSE) table USING ...

which is untidy. Furthermore, 'CLUSTER VERBOSE (' is tab-completed to
'CLUSTER VERBOSE (USING'. That way I propose to only allow either new
options or old similarly to the VACUUM. See attached 0002.

-			COMPLETE_WITH("VERBOSE");
+			COMPLETE_WITH("TABLESPACE|VERBOSE");

Tab completion in the CLUSTER was broken for parenthesized options, so
I've fixed it in the 0005.

Also, I noticed that you used vac_analyze_option_list instead of
reindex_option_list and I checked other option lists in the grammar.
I've found that explain_option_list and vac_analyze_option_list are
identical, so it makes sense to leave just one of them and rename it to,
e.g., common_option_list in order to use it everywhere needed (REINDEX,
VACUUM, EXPLAIN, CLUSTER, ANALYZE). The whole grammar is already
complicated enough to keep the exact duplicates and new options will be
added to the lists in the backend code, not parser. What do you think?

It is done in the 0007 attached. I think it should be applied altogether
with 0001 or before/after, but I put this as the last patch in the set
in order to easier discard it if others would disagree.

Otherwise, everything seems to be working fine. Cannot find any problems
so far.

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On Thu, Sep 03, 2020 at 12:00:17AM +0300, Alexey Kondratov wrote:

On 2020-09-02 07:56, Justin Pryzby wrote:

Done in the attached, which is also rebased on 1d6541666.

And added RelationAssumeNewRelfilenode() as I mentioned - but I'm hoping
to
hear from Michael about any reason not to call
RelationSetNewRelfilenode()
instead of directly calling the things it would itself call.

The latest patch set immediately got a conflict with Michael's fixup
01767533, so I've rebased it first of all.

On my side, I've also rearranged function parameters to make the diff more
readable. And squishes your changes into the respective patches.

Michael started a new thread about retiring ReindexStmt->concurrent, which I
guess will cause more conflicts (although I don't see why we wouldn't implement
a generic List grammar now rather than only after a preliminary patch).

--
Justin

On Wed, Sep 02, 2020 at 06:07:06PM -0500, Justin Pryzby wrote:

On my side, I've also rearranged function parameters to make the diff more
readable. And squishes your changes into the respective patches.

This resolves a breakage I failed to notice from a last-minute edit.
And squishes two commits.
And rebased on Michael's commit removing ReindexStmt->concurrent.

--
Justin

On Thu, Sep 03, 2020 at 09:43:51PM -0500, Justin Pryzby wrote:

And rebased on Michael's commit removing ReindexStmt->concurrent.

Rebased on a6642b3ae: Add support for partitioned tables and indexes in REINDEX

So now this includes the new functionality and test for reindexing a
partitioned table onto a new tablespace. That part could use some additional
review.

I guess this patch series will also conflict with the CLUSTER part of this
other one. Once its CLUSTER patch is commited, this patch should to be updated
to test clustering a partitioned table to a new tbspc.
https://commitfest.postgresql.org/29/2584/
REINDEX/CIC/CLUSTER of partitioned tables

--
Justin

On 2020-Sep-08, Justin Pryzby wrote:

From 992e0121925c74d5c5a4e5b132cddb3d6b31da86 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 27 Mar 2020 17:50:46 -0500
Subject: [PATCH v27 1/5] Change REINDEX/CLUSTER to accept an option list..

..like EXPLAIN (..), VACUUM (..), and ANALYZE (..).

Change docs in the style of VACUUM. See also: 52dcfda48778d16683c64ca4372299a099a15b96

I don't understand why you change all options to DefElem instead of
keeping the bitmask for those options that can use it.

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

On Tue, Sep 08, 2020 at 09:02:38PM -0300, Alvaro Herrera wrote:

On 2020-Sep-08, Justin Pryzby wrote:

From 992e0121925c74d5c5a4e5b132cddb3d6b31da86 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 27 Mar 2020 17:50:46 -0500
Subject: [PATCH v27 1/5] Change REINDEX/CLUSTER to accept an option list..

..like EXPLAIN (..), VACUUM (..), and ANALYZE (..).

Change docs in the style of VACUUM. See also: 52dcfda48778d16683c64ca4372299a099a15b96

I don't understand why you change all options to DefElem instead of
keeping the bitmask for those options that can use it.

That's originally how I did it, too.

Initially I added List *params, and Michael suggested to retire
ReindexStmt->concurrent. I provided a patch to do so, initially by leaving int
options and then, after this, removing it to "complete the thought", and get
rid of the remnants of the "old way" of doing it. This is also how vacuum and
explain are done.
/messages/by-id/20200902022410.GA20149@telsasoft.com

--
Justin

On Tue, Sep 08, 2020 at 07:17:58PM -0500, Justin Pryzby wrote:

Initially I added List *params, and Michael suggested to retire
ReindexStmt->concurrent. I provided a patch to do so, initially by leaving int
options and then, after this, removing it to "complete the thought", and get
rid of the remnants of the "old way" of doing it. This is also how vacuum and
explain are done.
/messages/by-id/20200902022410.GA20149@telsasoft.com

Defining a set of DefElem when parsing and then using the int
"options" with bitmasks where necessary at the beginning of the
execution looks like a good balance to me. This way, you can extend
the grammar to use things like (verbose = true), etc.

By the way, skimming through the patch set, I was wondering if we
could do the refactoring of patch 0005 as a first step, until I
noticed this part:
+common_option_name:
NonReservedWord { $$ = $1; }
| analyze_keyword { $$ = "analyze"; }
This is not a good idea as you make ANALYZE an option available for
all the commands involved in the refactoring. A portion of that could
be considered though, like the use of common_option_arg.
--
Michael

On 2020-09-09 15:22, Michael Paquier wrote:

On Tue, Sep 08, 2020 at 07:17:58PM -0500, Justin Pryzby wrote:

Initially I added List *params, and Michael suggested to retire
ReindexStmt->concurrent. I provided a patch to do so, initially by
leaving int
options and then, after this, removing it to "complete the thought",
and get
rid of the remnants of the "old way" of doing it. This is also how
vacuum and
explain are done.
/messages/by-id/20200902022410.GA20149@telsasoft.com

Defining a set of DefElem when parsing and then using the int
"options" with bitmasks where necessary at the beginning of the
execution looks like a good balance to me. This way, you can extend
the grammar to use things like (verbose = true), etc.

By the way, skimming through the patch set, I was wondering if we
could do the refactoring of patch 0005 as a first step

Yes, I did it with intention to put as a first patch, but wanted to get
some feedback. It's easier to refactor the last patch without rebasing
others.

until I
noticed this part:
+common_option_name:
NonReservedWord { $$ = $1; }
| analyze_keyword { $$ = "analyze"; }
This is not a good idea as you make ANALYZE an option available for
all the commands involved in the refactoring. A portion of that could
be considered though, like the use of common_option_arg.

From the grammar perspective ANY option is available for any command
that uses parenthesized option list. All the checks and validations are
performed at the corresponding command code.
This analyze_keyword is actually doing only an ANALYZE word
normalization if it's used as an option. Why it could be harmful?

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On Wed, Sep 09, 2020 at 09:22:00PM +0900, Michael Paquier wrote:

On Tue, Sep 08, 2020 at 07:17:58PM -0500, Justin Pryzby wrote:

Initially I added List *params, and Michael suggested to retire
ReindexStmt->concurrent. I provided a patch to do so, initially by leaving int
options and then, after this, removing it to "complete the thought", and get
rid of the remnants of the "old way" of doing it. This is also how vacuum and
explain are done.
/messages/by-id/20200902022410.GA20149@telsasoft.com

Defining a set of DefElem when parsing and then using the int
"options" with bitmasks where necessary at the beginning of the
execution looks like a good balance to me. This way, you can extend
the grammar to use things like (verbose = true), etc.

It doesn't need to be extended - defGetBoolean already handles that. I don't
see what good can come from storing the information in two places in the same
structure.

|postgres=# CLUSTER (VERBOSE on) pg_attribute USING pg_attribute_relid_attnum_index ;
|INFO: clustering "pg_catalog.pg_attribute" using index scan on "pg_attribute_relid_attnum_index"
|INFO: "pg_attribute": found 0 removable, 2968 nonremovable row versions in 55 pages
|DETAIL: 0 dead row versions cannot be removed yet.
|CPU: user: 0.01 s, system: 0.00 s, elapsed: 0.01 s.
|CLUSTER

--
Justin

On 2020-09-09 18:36, Justin Pryzby wrote:

Rebased on a6642b3ae: Add support for partitioned tables and indexes in
REINDEX

So now this includes the new functionality and test for reindexing a
partitioned table onto a new tablespace. That part could use some
additional
review.

I have finally had a look on your changes regarding partitioned tables.

+set_rel_tablespace(Oid indexid, char *tablespace)
+{
+	Oid tablespaceOid = tablespace ? get_tablespace_oid(tablespace, false) 
:
+		InvalidOid;

You pass a tablespace name to set_rel_tablespace(), but it is already
parsed into the Oid before. So I do not see why we need this extra work
here instead of just passing Oid directly.

Also set_rel_tablespace() does not check for a no-op case, i.e. if
requested tablespace is the same as before.

+	/*
+	 * Set the new tablespace for the relation.  Do that only in the
+	 * case where the reindex caller wishes to enforce a new tablespace.
+	 */
+	if (set_tablespace &&
+		tablespaceOid != iRel->rd_rel->reltablespace)

Just noticed that this check is not completely correct as well, since it
does not check for MyDatabaseTableSpace (stored as InvalidOid) logic.

I put these small fixes directly into the attached 0003.

Also, I thought about your comment above set_rel_tablespace() and did a
bit 'extreme' refactoring, which is attached as a separated patch 0004.
The only one doubtful change IMO is reordering of RelationDropStorage()
operation inside reindex_index(). However, it only schedules unlinking
of physical storage at transaction commit, so this refactoring seems to
be safe.

If there will be no objections I would merge it with 0003.

On 2020-09-09 16:03, Alexey Kondratov wrote:

On 2020-09-09 15:22, Michael Paquier wrote:

By the way, skimming through the patch set, I was wondering if we
could do the refactoring of patch 0005 as a first step

Yes, I did it with intention to put as a first patch, but wanted to
get some feedback. It's easier to refactor the last patch without
rebasing others.

until I
noticed this part:
+common_option_name:
NonReservedWord { $$ = $1; }
| analyze_keyword { $$ = "analyze"; }
This is not a good idea as you make ANALYZE an option available for
all the commands involved in the refactoring. A portion of that could
be considered though, like the use of common_option_arg.

From the grammar perspective ANY option is available for any command
that uses parenthesized option list. All the checks and validations
are performed at the corresponding command code.
This analyze_keyword is actually doing only an ANALYZE word
normalization if it's used as an option. Why it could be harmful?

Michael has not replied since then, but he was relatively positive about
0005 initially, so I put it as a first patch now.

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On Wed, Sep 23, 2020 at 07:43:01PM +0300, Alexey Kondratov wrote:

On 2020-09-09 18:36, Justin Pryzby wrote:

Rebased on a6642b3ae: Add support for partitioned tables and indexes in
REINDEX

So now this includes the new functionality and test for reindexing a
partitioned table onto a new tablespace. That part could use some
additional
review.

I have finally had a look on your changes regarding partitioned tables.

+set_rel_tablespace(Oid indexid, char *tablespace)
+{
+	Oid tablespaceOid = tablespace ? get_tablespace_oid(tablespace, false) :
+		InvalidOid;

You pass a tablespace name to set_rel_tablespace(), but it is already parsed
into the Oid before. So I do not see why we need this extra work here
instead of just passing Oid directly.

Also set_rel_tablespace() does not check for a no-op case, i.e. if requested
tablespace is the same as before.

+	/*
+	 * Set the new tablespace for the relation.  Do that only in the
+	 * case where the reindex caller wishes to enforce a new tablespace.
+	 */
+	if (set_tablespace &&
+		tablespaceOid != iRel->rd_rel->reltablespace)

Just noticed that this check is not completely correct as well, since it
does not check for MyDatabaseTableSpace (stored as InvalidOid) logic.

I put these small fixes directly into the attached 0003.

Also, I thought about your comment above set_rel_tablespace() and did a bit
'extreme' refactoring, which is attached as a separated patch 0004. The only
one doubtful change IMO is reordering of RelationDropStorage() operation
inside reindex_index(). However, it only schedules unlinking of physical
storage at transaction commit, so this refactoring seems to be safe.

If there will be no objections I would merge it with 0003.

On 2020-09-09 16:03, Alexey Kondratov wrote:

On 2020-09-09 15:22, Michael Paquier wrote:

By the way, skimming through the patch set, I was wondering if we
could do the refactoring of patch 0005 as a first step

Yes, I did it with intention to put as a first patch, but wanted to
get some feedback. It's easier to refactor the last patch without
rebasing others.

until I
noticed this part:
+common_option_name:
NonReservedWord { $$ = $1; }
| analyze_keyword { $$ = "analyze"; }
This is not a good idea as you make ANALYZE an option available for
all the commands involved in the refactoring. A portion of that could
be considered though, like the use of common_option_arg.

From the grammar perspective ANY option is available for any command
that uses parenthesized option list. All the checks and validations
are performed at the corresponding command code.
This analyze_keyword is actually doing only an ANALYZE word
normalization if it's used as an option. Why it could be harmful?

Michael has not replied since then, but he was relatively positive about
0005 initially, so I put it as a first patch now.

Thanks. I rebased Alexey's latest patch on top of recent changes to cluster.c.
This puts the generic grammar changes first. I wasn't paying much attention to
that part, so still waiting for a committer review.

--
Justin

On Sat, Oct 31, 2020 at 01:36:11PM -0500, Justin Pryzby wrote:

From the grammar perspective ANY option is available for any command
that uses parenthesized option list. All the checks and validations
are performed at the corresponding command code.
This analyze_keyword is actually doing only an ANALYZE word
normalization if it's used as an option. Why it could be harmful?

Michael has not replied since then, but he was relatively positive about
0005 initially, so I put it as a first patch now.

Thanks. I rebased Alexey's latest patch on top of recent changes to cluster.c.
This puts the generic grammar changes first. I wasn't paying much attention to
that part, so still waiting for a committer review.

@cfbot: rebased

On Tue, Nov 24, 2020 at 09:31:23AM -0600, Justin Pryzby wrote:

@cfbot: rebased

Catching up with the activity here, I can see four different things in
the patch set attached:
1) Refactoring of the grammar of CLUSTER, VACUUM, ANALYZE and REINDEX
to support values in parameters.
2) Tablespace change for REINDEX.
3) Tablespace change for VACUUM FULL/CLUSTER.
4) Tablespace change for indexes with VACUUM FULL/CLUSTER.

I am not sure yet about the last three points, so let's begin with 1)
that is dealt with in 0001 and 0002. I have spent some time on 0001,
renaming the rule names to be less generic than "common", and applied
it. 0002 looks to be in rather good shape, still there are a few
things that have caught my eyes. I'll look at that more closely
tomorrow.
--
Michael

On 2020-11-30 14:33, Michael Paquier wrote:

On Tue, Nov 24, 2020 at 09:31:23AM -0600, Justin Pryzby wrote:

@cfbot: rebased

Catching up with the activity here, I can see four different things in
the patch set attached:
1) Refactoring of the grammar of CLUSTER, VACUUM, ANALYZE and REINDEX
to support values in parameters.
2) Tablespace change for REINDEX.
3) Tablespace change for VACUUM FULL/CLUSTER.
4) Tablespace change for indexes with VACUUM FULL/CLUSTER.

I am not sure yet about the last three points, so let's begin with 1)
that is dealt with in 0001 and 0002. I have spent some time on 0001,
renaming the rule names to be less generic than "common", and applied
it. 0002 looks to be in rather good shape, still there are a few
things that have caught my eyes. I'll look at that more closely
tomorrow.

Thanks. I have rebased the remaining patches on top of 873ea9ee to use
'utility_option_list' instead of 'common_option_list'.

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On Mon, Nov 30, 2020 at 05:12:42PM +0300, Alexey Kondratov wrote:

Thanks. I have rebased the remaining patches on top of 873ea9ee to use
'utility_option_list' instead of 'common_option_list'.

Thanks, that helps a lot. I have gone through 0002, and tweaked it as
the attached (note that this patch is also interesting for another
thing in development: backend-side reindex filtering of
collation-sensitive indexes). Does that look right to you?

These are mostly matters of consistency with the other commands using
DefElem, but I think that it is important to get things right:
- Having the list of options in parsenodes.h becomes incorrect,
because these get now only used at execution time, like VACUUM. So I
have moved that to cluster.h and index.h.
- Let's use an enum for REINDEX, like the others.
- Having parse_reindex_params() in utility.c is wrong for something
aimed at being used only for REINDEX, so I have moved that to
indexcmds.c, and renamed the routine to be more consistent with the
rest. I think that we could more here by having an ExecReindex() that
does all the work based on object types, but I have left that out for
now to keep the change minimal.
- Switched one of the existing tests to stress CONCURRENTLY within
parenthesis.
- Indented the whole.

A couple of extra things below.

* CLUSTER [VERBOSE] <qualified_name> [ USING <index_name> ]
+ * CLUSTER [VERBOSE] [(options)] <qualified_name> [ USING <index_name> ]
This line is wrong, and should be:
CLUSTER [ (options) ] <qualified_name> [ USING <index_name> ]

+CLUSTER [VERBOSE] [ ( <replaceable class="parameter">option</replaceable>
+CLUSTER [VERBOSE] [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ]
The docs in cluster.sgml are wrong as well, you can have VERBOSE as a
single option or as a parenthesized option, but never both at the same
time.  On the contrary, psql completion got that right.  I was first a
bit surprised that you would not allow the parenthesized set for the
case where a relation is not specified in the command, but I agree
that this does not seem worth the extra complexity now as this thread
aims at being able to use TABLESPACE which makes little sense
database-wide.

-    VERBOSE
+    VERBOSE [ <replaceable class="parameter">boolean</replaceable> ]
Forgot about CONCURRENTLY as an option here, as this becomes
possible.
--
Michael

On Tue, Dec 01, 2020 at 11:46:55AM +0900, Michael Paquier wrote:

On Mon, Nov 30, 2020 at 05:12:42PM +0300, Alexey Kondratov wrote:

Thanks. I have rebased the remaining patches on top of 873ea9ee to use
'utility_option_list' instead of 'common_option_list'.

Thanks, that helps a lot. I have gone through 0002, and tweaked it as
the attached (note that this patch is also interesting for another
thing in development: backend-side reindex filtering of
collation-sensitive indexes). Does that look right to you?

I eyeballed the patch and rebased the rest of the series on top if it to play
with. Looks fine - thanks.

FYI, the commit messages have the proper "author" for attribution. I proposed
and implemented the grammar changes in 0002, and implemented INDEX_TABLESPACE,
but I'm a reviewer for the main patches.

--
Justin

On Mon, Nov 30, 2020 at 11:43:08PM -0600, Justin Pryzby wrote:

I eyeballed the patch and rebased the rest of the series on top if it to play
with. Looks fine - thanks.

Cool, thanks.

FYI, the commit messages have the proper "author" for attribution. I proposed
and implemented the grammar changes in 0002, and implemented INDEX_TABLESPACE,
but I'm a reviewer for the main patches.

Well, my impression is that both of you kept exchanging patches,
touching and reviewing each other's patch (note that Alexei has also
sent a rebase of 0002 just yesterday), so I think that it is fair to
say that both of you should be listed as authors and credited as such
in the release notes for this one.
--
Michael

On Tue, Dec 01, 2020 at 03:10:13PM +0900, Michael Paquier wrote:

Well, my impression is that both of you kept exchanging patches,
touching and reviewing each other's patch (note that Alexei has also
sent a rebase of 0002 just yesterday), so I think that it is fair to
say that both of you should be listed as authors and credited as such
in the release notes for this one.

OK, this one is now committed. As of this thread, I think that we are
going to need to do a bit more once we add options that are not just
booleans for both commands (the grammar rules do not need to be
changed now):
- Have a ReindexParams, similarly to VacuumParams except that we store
the results of the parsing in a single place. With the current HEAD,
I did not see yet the point in doing so because we just need an
integer that maps to a bitmask made of ReindexOption.
- The part related to ReindexStmt in utility.c is getting more and
more complicated, so we could move most of the execution into
indexcmds.c with some sort of ExecReindex() doing the option parsing
job, and go to the correct code path depending on the object type
dealt with.
--
Michael

On Thu, Dec 03, 2020 at 10:19:43AM +0900, Michael Paquier wrote:

OK, this one is now committed. As of this thread, I think that we are
going to need to do a bit more once we add options that are not just
booleans for both commands (the grammar rules do not need to be
changed now):
- Have a ReindexParams, similarly to VacuumParams except that we store
the results of the parsing in a single place. With the current HEAD,
I did not see yet the point in doing so because we just need an
integer that maps to a bitmask made of ReindexOption.
- The part related to ReindexStmt in utility.c is getting more and
more complicated, so we could move most of the execution into
indexcmds.c with some sort of ExecReindex() doing the option parsing
job, and go to the correct code path depending on the object type
dealt with.

Good idea. I think you mean like this.

I don't know where to put the struct.
I thought maybe the lowlevel, integer options should live in the struct, in
addition to bools, but it's not important.

--
Justin

On Wed, Dec 02, 2020 at 10:30:08PM -0600, Justin Pryzby wrote:

Good idea. I think you mean like this.

Yes, something like that. Thanks.

+typedef struct ReindexParams {
+	bool concurrently;
+	bool verbose;
+	bool missingok;
+
+	int options;	/* bitmask of lowlevel REINDEXOPT_* */
+} ReindexParams;
+

By moving everything into indexcmds.c, keeping ReindexParams within it
makes sense to me. Now, there is no need for the three booleans
because options stores the same information, no?

struct ReindexIndexCallbackState
{
-	int			options;		/* options from statement */
+	bool		concurrently;
Oid			locked_table_oid;	/* tracks previously locked table */
};

Here also, I think that we should just pass down the full
ReindexParams set.
--
Michael

A side comment on this patch: I think using enums as bit mask values is
bad style. So changing this:

-/* Reindex options */
-#define REINDEXOPT_VERBOSE (1 << 0) /* print progress info */
-#define REINDEXOPT_REPORT_PROGRESS (1 << 1) /* report pgstat progress */
-#define REINDEXOPT_MISSING_OK (1 << 2) /* skip missing relations */
-#define REINDEXOPT_CONCURRENTLY (1 << 3) /* concurrent mode */

to this:

+typedef enum ReindexOption
+{
+   REINDEXOPT_VERBOSE = 1 << 0,    /* print progress info */
+   REINDEXOPT_REPORT_PROGRESS = 1 << 1,    /* report pgstat progress */
+   REINDEXOPT_MISSING_OK = 1 << 2, /* skip missing relations */
+   REINDEXOPT_CONCURRENTLY = 1 << 3    /* concurrent mode */
+} ReindexOption;

seems wrong.

There are a couple of more places like this, including the existing
ClusterOption that this patched moved around, but we should be removing
those.

My reasoning is that if you look at an enum value of this type, either
say in a switch statement or a debugger, the enum value might not be any
of the defined symbols. So that way you lose all the type checking that
an enum might give you.

Let's just keep the #define's like it is done in almost all other places.

On Thu, Dec 03, 2020 at 04:12:53PM +0900, Michael Paquier wrote:

+typedef struct ReindexParams {
+	bool concurrently;
+	bool verbose;
+	bool missingok;
+
+	int options;	/* bitmask of lowlevel REINDEXOPT_* */
+} ReindexParams;
+

By moving everything into indexcmds.c, keeping ReindexParams within it
makes sense to me. Now, there is no need for the three booleans
because options stores the same information, no?

I liked the bools, but dropped them so the patch is smaller.

struct ReindexIndexCallbackState
{
-	int			options;		/* options from statement */
+	bool		concurrently;
Oid			locked_table_oid;	/* tracks previously locked table */
};

Here also, I think that we should just pass down the full
ReindexParams set.

Ok.

Regarding the REINDEX patch, I think this comment is misleading:

| /*
| * If the relation has a secondary toast rel, reindex that too while we
| * still hold the lock on the main table.
| */
| if ((flags & REINDEX_REL_PROCESS_TOAST) && OidIsValid(toast_relid))
| {
| /*
| * Note that this should fail if the toast relation is missing, so
| * reset REINDEXOPT_MISSING_OK.
|+ *
|+ * Even if table was moved to new tablespace, normally toast cannot move.
| */
|+ Oid toasttablespaceOid = allowSystemTableMods ? tablespaceOid : InvalidOid;
| result |= reindex_relation(toast_relid, flags,
|- options & ~(REINDEXOPT_MISSING_OK));
|+ options & ~(REINDEXOPT_MISSING_OK),
|+ toasttablespaceOid);
| }

I think it ought to say "Even if a table's indexes were moved to a new
tablespace, its toast table's index is not normally moved"
Right ?

Also, I don't know whether we should check for GLOBALTABLESPACE_OID after
calling get_tablespace_oid(), or in the lowlevel routines. Note that
reindex_relation is called during cluster/vacuum, and in the later patches, I
moved the test from from cluster() and ExecVacuum() to rebuild_relation().

--
Justin

On Thu, Dec 03, 2020 at 08:46:09PM +0100, Peter Eisentraut wrote:

There are a couple of more places like this, including the existing
ClusterOption that this patched moved around, but we should be removing
those.

My reasoning is that if you look at an enum value of this type, either say
in a switch statement or a debugger, the enum value might not be any of the
defined symbols. So that way you lose all the type checking that an enum
might give you.

VacuumOption does that since 6776142, and ClusterOption since 9ebe057,
so switching ReindexOption to just match the two others still looks
like the most consistent move. Please note that there is more than
that, like ScanOptions, relopt_kind, RVROption, InstrumentOption,
TableLikeOption.

I would not mind changing that, though I am not sure that this
improves readability. And if we'd do it, it may make sense to extend
that even more to the places where it would apply like the places
mentioned one paragraph above.
--
Michael

On 2020-12-04 04:25, Justin Pryzby wrote:

On Thu, Dec 03, 2020 at 04:12:53PM +0900, Michael Paquier wrote:

+typedef struct ReindexParams {
+	bool concurrently;
+	bool verbose;
+	bool missingok;
+
+	int options;	/* bitmask of lowlevel REINDEXOPT_* */
+} ReindexParams;
+

By moving everything into indexcmds.c, keeping ReindexParams within it
makes sense to me. Now, there is no need for the three booleans
because options stores the same information, no?

I liked the bools, but dropped them so the patch is smaller.

I had a look on 0001 and it looks mostly fine to me except some strange
mixture of tabs/spaces in the ExecReindex(). There is also a couple of
meaningful comments:

-	options =
-		(verbose ? REINDEXOPT_VERBOSE : 0) |
-		(concurrently ? REINDEXOPT_CONCURRENTLY : 0);
+	if (verbose)
+		params.options |= REINDEXOPT_VERBOSE;

Why do we need this intermediate 'verbose' variable here? We only use it
once to set a bitmask. Maybe we can do it like this:

params.options |= defGetBoolean(opt) ?
REINDEXOPT_VERBOSE : 0;

See also attached txt file with diff (I wonder can I trick cfbot this
way, so it does not apply the diff).

+ int options; /* bitmask of lowlevel REINDEXOPT_* */

I would prefer if the comment says '/* bitmask of ReindexOption */' as
in the VacuumOptions, since citing the exact enum type make it easier to
navigate source code.

Regarding the REINDEX patch, I think this comment is misleading:

|+ * Even if table was moved to new tablespace,
normally toast cannot move.
| */
|+ Oid toasttablespaceOid = allowSystemTableMods ?
tablespaceOid : InvalidOid;
| result |= reindex_relation(toast_relid, flags,

I think it ought to say "Even if a table's indexes were moved to a new
tablespace, its toast table's index is not normally moved"
Right ?

Yes, I think so, we are dealing only with index tablespace changing
here. Thanks for noticing.

Also, I don't know whether we should check for GLOBALTABLESPACE_OID
after
calling get_tablespace_oid(), or in the lowlevel routines. Note that
reindex_relation is called during cluster/vacuum, and in the later
patches, I
moved the test from from cluster() and ExecVacuum() to
rebuild_relation().

IIRC, I wanted to do GLOBALTABLESPACE_OID check as early as possible
(just after getting Oid), since it does not make sense to proceed
further if tablespace is set to that value. So initially there were a
lot of duplicative GLOBALTABLESPACE_OID checks, since there were a lot
of reindex entry-points (index, relation, concurrently, etc.). Now we
are going to have ExecReindex(), so there are much less entry-points and
in my opinion it is fine to keep this validation just after
get_tablespace_oid().

However, this is mostly a sanity check. I can hardly imagine a lot of
users trying to constantly move indexes to the global tablespace, so it
is also OK to put this check deeper into guts.

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On 2020-Dec-04, Michael Paquier wrote:

VacuumOption does that since 6776142, and ClusterOption since 9ebe057,
so switching ReindexOption to just match the two others still looks
like the most consistent move.

9ebe057 goes to show why this is a bad idea, since it has this:

+typedef enum ClusterOption
+{
+   CLUOPT_RECHECK,             /* recheck relation state */
+   CLUOPT_VERBOSE              /* print progress info */
+} ClusterOption;

and then you do things like

+                   if ($2)
+                       n->options |= CLUOPT_VERBOSE;

and then tests like

+ if ((options & VACOPT_VERBOSE) != 0)

Now if you were to ever define third and fourth values in that enum,
this would immediately start malfunctioning.

FWIW I'm with Peter on this.

On Fri, Dec 04, 2020 at 09:40:31PM +0300, Alexey Kondratov wrote:

I liked the bools, but dropped them so the patch is smaller.

I had a look on 0001 and it looks mostly fine to me except some strange
mixture of tabs/spaces in the ExecReindex(). There is also a couple of
meaningful comments:

-	options =
-		(verbose ? REINDEXOPT_VERBOSE : 0) |
-		(concurrently ? REINDEXOPT_CONCURRENTLY : 0);
+	if (verbose)
+		params.options |= REINDEXOPT_VERBOSE;

Why do we need this intermediate 'verbose' variable here? We only use it
once to set a bitmask. Maybe we can do it like this:

params.options |= defGetBoolean(opt) ?
REINDEXOPT_VERBOSE : 0;

That allows *setting* REINDEXOPT_VERBOSE, but doesn't *unset* it if someone
runs (VERBOSE OFF). So I kept the bools like Michael originally had rather
than writing "else: params.options &= ~REINDEXOPT_VERBOSE"

See also attached txt file with diff (I wonder can I trick cfbot this way,
so it does not apply the diff).

Yes, I think that works :)
I believe it looks for *.diff and *.patch.

+ int options; /* bitmask of lowlevel REINDEXOPT_* */

I would prefer if the comment says '/* bitmask of ReindexOption */' as in
the VacuumOptions, since citing the exact enum type make it easier to
navigate source code.

Yes, thanks.

This also fixes some minor formatting and rebase issues, including broken doc/.

--
Justin

On Fri, Dec 04, 2020 at 04:28:26PM -0300, Alvaro Herrera wrote:

FWIW I'm with Peter on this.

Okay, attached is a patch to adjust the enums for the set of utility
commands that is the set of things I have touched lately. Should that
be extended more? I have not done that as a lot of those structures
exist as such for a long time.
--
Michael

On 2020-12-05 02:30, Michael Paquier wrote:

On Fri, Dec 04, 2020 at 04:28:26PM -0300, Alvaro Herrera wrote:

FWIW I'm with Peter on this.

Okay, attached is a patch to adjust the enums for the set of utility
commands that is the set of things I have touched lately. Should that
be extended more? I have not done that as a lot of those structures
exist as such for a long time.

I think this patch is good.

I have in the meantime committed a similar patch for cleaning up this
issue in pg_dump.

By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?

On Fri, Dec 11, 2020 at 05:27:03PM -0300, Alvaro Herrera wrote:

By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?

Right, we could just do that while the area is changed. It is worth
noting that all the REINDEX_REL_* handling could be brushed. Another
point that has been raised on a recent thread by Peter was that people
preferred an hex style for the declarations rather than bit shifts.
What do you think?
--
Michael

On 2020-12-11 21:27, Alvaro Herrera wrote:

By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?

If we want to make it clearer, why not turn the thing into a struct, as
in the attached patch, and avoid the bit fiddling altogether.

On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:

On 2020-12-11 21:27, Alvaro Herrera wrote:

By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?

If we want to make it clearer, why not turn the thing into a struct, as in
the attached patch, and avoid the bit fiddling altogether.

I like this.
It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
with an "int options" bitmask which is passed to reindex_index() et al. Your
patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
which I think is good.

So I've rebased this branch on your patch.

Some thoughts:

- what about removing the REINDEXOPT_* prefix ?
- You created local vars with initialization like "={}". But I thought it's
needed to include at least one struct member like "={false}", or else
they're not guaranteed to be zerod ?
- You passed the structure across function calls. The usual convention is to
pass a pointer.

I also changed the errcode and detail for this one.
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("incompatible TABLESPACE option"),
errdetail("TABLESPACE can only be used with VACUUM FULL.")));

--
Justin

On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:

On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:

On 2020-12-11 21:27, Alvaro Herrera wrote:

By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?

If we want to make it clearer, why not turn the thing into a struct, as in
the attached patch, and avoid the bit fiddling altogether.

I like this.
It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
with an "int options" bitmask which is passed to reindex_index() et al. Your
patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
which I think is good.

So I've rebased this branch on your patch.

Also, the cfbot's windows VS compilation failed due to "compound literal",
which I don't think is used anywhere else.

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.120108

src/backend/commands/cluster.c(1580): warning C4133: 'return' : incompatible types - from 'List *' to 'int *' [C:\projects\postgresql\postgres.vcxproj]
"C:\projects\postgresql\pgsql.sln" (default target) (1) ->
"C:\projects\postgresql\cyrillic_and_mic.vcxproj" (default target) (5) ->
"C:\projects\postgresql\postgres.vcxproj" (default target) (6) ->
(ClCompile target) ->
src/backend/commands/cluster.c(1415): error C2059: syntax error : '}' [C:\projects\postgresql\postgres.vcxproj]
src/backend/commands/cluster.c(1534): error C2143: syntax error : missing '{' before '*' [C:\projects\postgresql\postgres.vcxproj]
src/backend/commands/cluster.c(1536): error C2371: 'get_tables_to_cluster' : redefinition; different basic types [C:\projects\postgresql\postgres.vcxproj]
src/backend/commands/indexcmds.c(2462): error C2059: syntax error : '}' [C:\projects\postgresql\postgres.vcxproj]
src/backend/commands/tablecmds.c(1894): error C2059: syntax error : '}' [C:\projects\postgresql\postgres.vcxproj]

My fix! patch resolves that.

--
Justin

On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:

On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:

On 2020-12-11 21:27, Alvaro Herrera wrote:

By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?

If we want to make it clearer, why not turn the thing into a struct, as in
the attached patch, and avoid the bit fiddling altogether.

-   reindex_relation(OIDOldHeap, reindex_flags, 0);
+   reindex_relation(OIDOldHeap, reindex_flags, (ReindexOptions){});
This is not common style in the PG code.  Usually we would do that
with memset(0) or similar.

+   bool REINDEXOPT_VERBOSE;            /* print progress info */
+   bool REINDEXOPT_REPORT_PROGRESS;    /* report pgstat progress */
+   bool REINDEXOPT_MISSING_OK;         /* skip missing relations */
+   bool REINDEXOPT_CONCURRENTLY;       /* concurrent mode */
+} ReindexOptions
Neither is this one to use upper-case characters for variable names.

Now, we will need a ReindexOptions in the long-term to store all those
options and there would be much more coming that booleans here (this
thread talks about tablespaces, there is another thread about
collation filtering). Between using bits32 with some hex flags or
just a set of booleans within a structure, I would choose the former
as a matter of habit but yours has the advantage to make debugging a
no-brainer, which is good. For any approach taken, it seems to me
that the same style should be applied to ClusterOption and
Vacuum{Option,Params}.
--
Michael

On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:

On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:

On 2020-12-11 21:27, Alvaro Herrera wrote:

By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?

If we want to make it clearer, why not turn the thing into a struct, as in
the attached patch, and avoid the bit fiddling altogether.

I like this.
It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
with an "int options" bitmask which is passed to reindex_index() et al. Your
patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
which I think is good.

So I've rebased this branch on your patch.

Some thoughts:

- what about removing the REINDEXOPT_* prefix ?
- You created local vars with initialization like "={}". But I thought it's
needed to include at least one struct member like "={false}", or else
they're not guaranteed to be zerod ?
- You passed the structure across function calls. The usual convention is to
pass a pointer.

I think maybe Michael missed this message (?)
I had applied some changes on top of Peter's patch.

I squished those commits now, and also handled ClusterOption and VacuumOption
in the same style.

Some more thoughts:
- should the structures be named in plural ? "ReindexOptions" etc. Since they
define *all* the options, not just a single bit.
- For vacuum, do we even need a separate structure, or should the members be
directly within VacuumParams ? It's a bit odd to write
params.options.verbose. Especially since there's also ternary options which
are directly within params.
- Then, for cluster, I think it should be called ClusterParams, and eventually
include the tablespaceOid, like what we're doing for Reindex.

I am awaiting feedback on these before going further since I've done too much
rebasing with these ideas going back and forth and back.

--
Justin

On 2020-12-15 03:14, Justin Pryzby wrote:

On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:

On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:

On 2020-12-11 21:27, Alvaro Herrera wrote:

By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?

If we want to make it clearer, why not turn the thing into a struct, as in
the attached patch, and avoid the bit fiddling altogether.

I like this.
It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and
ReindexParams
In my v31 patch, I moved ReindexOptions to a private structure in
indexcmds.c,
with an "int options" bitmask which is passed to reindex_index() et
al. Your
patch keeps/puts ReindexOptions index.h, so it also applies to
reindex_index,
which I think is good.

So I've rebased this branch on your patch.

Some thoughts:

- what about removing the REINDEXOPT_* prefix ?
- You created local vars with initialization like "={}". But I
thought it's
needed to include at least one struct member like "={false}", or
else
they're not guaranteed to be zerod ?
- You passed the structure across function calls. The usual
convention is to
pass a pointer.

I think maybe Michael missed this message (?)
I had applied some changes on top of Peter's patch.

I squished those commits now, and also handled ClusterOption and
VacuumOption
in the same style.

Some more thoughts:
- should the structures be named in plural ? "ReindexOptions" etc.
Since they
define *all* the options, not just a single bit.
- For vacuum, do we even need a separate structure, or should the
members be
directly within VacuumParams ? It's a bit odd to write
params.options.verbose. Especially since there's also ternary
options which
are directly within params.

This is exactly what I have thought after looking on Peter's patch.
Writing 'params.options.some_option' looks just too verbose. I even
started moving all vacuum options into VacuumParams on my own and was in
the middle of the process when realized that there are some places that
do not fit well, like:

if (!vacuum_is_relation_owner(RelationGetRelid(onerel),
onerel->rd_rel,
params->options & VACOPT_ANALYZE))

Here we do not want to set option permanently, but rather to trigger
some additional code path in the vacuum_is_relation_owner(), IIUC. With
unified VacuumParams we should do:

bool opt_analyze = params->analyze;
...
params->analyze = true;
if (!vacuum_is_relation_owner(RelationGetRelid(onerel),
onerel->rd_rel, params))
...
params->analyze = opt_analyze;

to achieve the same, but it does not look good to me, or change the 
whole interface. I have found at least one other place like that so far 
--- vacuum_open_relation() in the analyze_rel().

Not sure how we could better cope with such logic.

- Then, for cluster, I think it should be called ClusterParams, and
eventually
include the tablespaceOid, like what we're doing for Reindex.

I am awaiting feedback on these before going further since I've done
too much
rebasing with these ideas going back and forth and back.

Yes, we have moved a bit from my original patch set in the thread with
all this refactoring. However, all the consequent patches are heavily
depend on this interface, so let us decide first on the proposed
refactoring.

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On Mon, Dec 14, 2020 at 06:14:17PM -0600, Justin Pryzby wrote:

On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:

On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:

On 2020-12-11 21:27, Alvaro Herrera wrote:

By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?

If we want to make it clearer, why not turn the thing into a struct, as in
the attached patch, and avoid the bit fiddling altogether.

I like this.
It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
with an "int options" bitmask which is passed to reindex_index() et al. Your
patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
which I think is good.

So I've rebased this branch on your patch.

Some thoughts:

- what about removing the REINDEXOPT_* prefix ?
- You created local vars with initialization like "={}". But I thought it's
needed to include at least one struct member like "={false}", or else
they're not guaranteed to be zerod ?
- You passed the structure across function calls. The usual convention is to
pass a pointer.

I think maybe Michael missed this message (?)
I had applied some changes on top of Peter's patch.

I squished those commits now, and also handled ClusterOption and VacuumOption
in the same style.

Some more thoughts:
- should the structures be named in plural ? "ReindexOptions" etc. Since they
define *all* the options, not just a single bit.
- For vacuum, do we even need a separate structure, or should the members be
directly within VacuumParams ? It's a bit odd to write
params.options.verbose. Especially since there's also ternary options which
are directly within params.
- Then, for cluster, I think it should be called ClusterParams, and eventually
include the tablespaceOid, like what we're doing for Reindex.

I am awaiting feedback on these before going further since I've done too much
rebasing with these ideas going back and forth and back.

With Alexey's agreement, I propose something like this.

I've merged some commits and kept separate the ones which are more likely to be
disputed/amended. But it may be best to read the series as a single commit,
like "git diff origin.."

--
Justin

On 2020-Dec-12, Peter Eisentraut wrote:

On 2020-12-11 21:27, Alvaro Herrera wrote:

By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?

If we want to make it clearer, why not turn the thing into a struct, as in
the attached patch, and avoid the bit fiddling altogether.

I don't like this idea too much, because adding an option causes an ABI
break. I don't think we commonly add options in backbranches, but it
has happened. The bitmask is much easier to work with in that regard.

On Tue, Dec 15, 2020 at 09:45:17PM -0300, Alvaro Herrera wrote:

I don't like this idea too much, because adding an option causes an ABI
break. I don't think we commonly add options in backbranches, but it
has happened. The bitmask is much easier to work with in that regard.

ABI flexibility is a good point here. I did not consider this point
of view. Thanks!
--
Michael

On Wed, Dec 16, 2020 at 10:01:11AM +0900, Michael Paquier wrote:

On Tue, Dec 15, 2020 at 09:45:17PM -0300, Alvaro Herrera wrote:

I don't like this idea too much, because adding an option causes an ABI
break. I don't think we commonly add options in backbranches, but it
has happened. The bitmask is much easier to work with in that regard.

ABI flexibility is a good point here. I did not consider this point
of view. Thanks!

FWIW, I have taken a shot at this part of the patch, and finished with
the attached. This uses bits32 for the bitmask options and an hex
style for the bitmask params, while bundling all the flags into
dedicated structures for all the options that can be extended for the
tablespace case (or some filtering for REINDEX).
--
Michael

On Tue, Dec 22, 2020 at 03:47:57PM +0900, Michael Paquier wrote:

On Wed, Dec 16, 2020 at 10:01:11AM +0900, Michael Paquier wrote:

On Tue, Dec 15, 2020 at 09:45:17PM -0300, Alvaro Herrera wrote:

I don't like this idea too much, because adding an option causes an ABI
break. I don't think we commonly add options in backbranches, but it
has happened. The bitmask is much easier to work with in that regard.

ABI flexibility is a good point here. I did not consider this point
of view. Thanks!

FWIW, I have taken a shot at this part of the patch, and finished with
the attached. This uses bits32 for the bitmask options and an hex
style for the bitmask params, while bundling all the flags into
dedicated structures for all the options that can be extended for the
tablespace case (or some filtering for REINDEX).

Seems fine, but why do you do memcpy() instead of a structure assignment ?

@@ -3965,8 +3965,11 @@ reindex_relation(Oid relid, int flags, int options)
* Note that this should fail if the toast relation is missing, so
* reset REINDEXOPT_MISSING_OK.
*/
-		result |= reindex_relation(toast_relid, flags,
-								   options & ~(REINDEXOPT_MISSING_OK));
+		ReindexOptions newoptions;
+
+		memcpy(&newoptions, options, sizeof(ReindexOptions));
+		newoptions.flags &= ~(REINDEXOPT_MISSING_OK);
+		result |= reindex_relation(toast_relid, flags, &newoptions);

Could be newoptions = *options;

Also, this one is going to be subsumed by ExecReindex(), so the palloc will go
away (otherwise I would ask to pass it in from the caller):

+ReindexOptions *
ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt)
{
ListCell *lc;
- int options = 0;
+ ReindexOptions *options;
bool concurrently = false;
bool verbose = false;

+	options = (ReindexOptions *) palloc0(sizeof(ReindexOptions));
+

--
Justin

On Tue, Dec 22, 2020 at 02:32:05AM -0600, Justin Pryzby wrote:

Also, this one is going to be subsumed by ExecReindex(), so the palloc will go
away (otherwise I would ask to pass it in from the caller):

Yeah, maybe. Still you need to be very careful if you have any
allocated variables like a tablespace or a path which requires to be
in the private context used by ReindexMultipleInternal() or even
ReindexRelationConcurrently(), so I am not sure you can avoid that
completely. For now, we could choose the option to still use a
palloc(), and then save the options in the private contexts. Forgot
that in the previous version actually.
--
Michael

On Tue, Dec 22, 2020 at 06:57:41PM +0900, Michael Paquier wrote:

On Tue, Dec 22, 2020 at 02:32:05AM -0600, Justin Pryzby wrote:

Also, this one is going to be subsumed by ExecReindex(), so the palloc will go
away (otherwise I would ask to pass it in from the caller):

Yeah, maybe. Still you need to be very careful if you have any
allocated variables like a tablespace or a path which requires to be
in the private context used by ReindexMultipleInternal() or even
ReindexRelationConcurrently(), so I am not sure you can avoid that
completely. For now, we could choose the option to still use a
palloc(), and then save the options in the private contexts. Forgot
that in the previous version actually.

I can't see why this still uses memset instead of structure assignment.

Now, I really think utility.c ought to pass in a pointer to a local
ReindexOptions variable to avoid all the memory context, which is unnecessary
and prone to error.

ExecReindex() will set options.tablesapceOid, not a pointer. Like this.

I also changed the callback to be a ReindexOptions and not a pointer.

--
Justin

Justin:
For reindex_index() :

+   if (options->tablespaceOid == MyDatabaseTableSpace)
+       options->tablespaceOid = InvalidOid;
...
+   if (set_tablespace &&
+       (options->tablespaceOid != oldTablespaceOid ||
+       (options->tablespaceOid == MyDatabaseTableSpace &&
OidIsValid(oldTablespaceOid))))

I wonder why the options->tablespaceOid == MyDatabaseTableSpace clause
appears again in the second if statement.
Since the first if statement would assign InvalidOid
to options->tablespaceOid when the first if condition is satisfied.

Cheers

On Tue, Dec 22, 2020 at 1:15 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Tue, Dec 22, 2020 at 03:22:19PM -0800, Zhihong Yu wrote:

Justin:
For reindex_index() :

+   if (options->tablespaceOid == MyDatabaseTableSpace)
+       options->tablespaceOid = InvalidOid;
...
+   oldTablespaceOid = iRel->rd_rel->reltablespace;
+   if (set_tablespace &&
+       (options->tablespaceOid != oldTablespaceOid ||
+       (options->tablespaceOid == MyDatabaseTableSpace &&
OidIsValid(oldTablespaceOid))))

I wonder why the options->tablespaceOid == MyDatabaseTableSpace clause
appears again in the second if statement.
Since the first if statement would assign InvalidOid
to options->tablespaceOid when the first if condition is satisfied.

Good question. Alexey mentioned on Sept 23 that he added the first stanza. to
avoid storing the DB's tablespace OID (rather than InvalidOid).

I think the 2nd half of the "or" is unnecessary since that was added setting to
options->tablespaceOid = InvalidOid.
If requesting to move to the DB's default tablespace, it'll now hit the first
part of the OR:

+ (options->tablespaceOid != oldTablespaceOid ||

Without the first stanza setting, it would've hit the 2nd condition:

+ (options->tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid))))

which means: "user requested to move a table to the DB's default tblspace, and
it was previously on a nondefault space".

So I think we can drop the 2nd half of the OR. Thanks for noticing.

--
Justin

On Tue, Dec 22, 2020 at 03:15:37PM -0600, Justin Pryzby wrote:

Now, I really think utility.c ought to pass in a pointer to a local
ReindexOptions variable to avoid all the memory context, which is unnecessary
and prone to error.

Yeah, it sounds right to me to just bite the bullet and do this
refactoring, limiting the manipulations of the options as much as
possible across contexts. So +1 from me to merge 0001 and 0002
together.

I have adjusted a couple of comments and simplified a bit more the
code in utility.c. I think that this is commitable, but let's wait
more than a couple of days for Alvaro and Peter first. This is a
period of vacations for a lot of people, and there is no point to
apply something that would need more work at the end. Using hexas for
the flags with bitmasks is the right conclusion IMO, but we are not
alone.

ExecReindex() will set options.tablespaceOid, not a pointer. Like
this.

OK. Good to know, I have not looked at this part of the patch yet.
--
Michael

On 2020-12-23 08:22, Justin Pryzby wrote:

On Tue, Dec 22, 2020 at 03:22:19PM -0800, Zhihong Yu wrote:

Justin:
For reindex_index() :

+   if (options->tablespaceOid == MyDatabaseTableSpace)
+       options->tablespaceOid = InvalidOid;
...
+   oldTablespaceOid = iRel->rd_rel->reltablespace;
+   if (set_tablespace &&
+       (options->tablespaceOid != oldTablespaceOid ||
+       (options->tablespaceOid == MyDatabaseTableSpace &&
OidIsValid(oldTablespaceOid))))

I wonder why the options->tablespaceOid == MyDatabaseTableSpace clause
appears again in the second if statement.
Since the first if statement would assign InvalidOid
to options->tablespaceOid when the first if condition is satisfied.

Good question. Alexey mentioned on Sept 23 that he added the first
stanza. to
avoid storing the DB's tablespace OID (rather than InvalidOid).

I think the 2nd half of the "or" is unnecessary since that was added
setting to
options->tablespaceOid = InvalidOid.
If requesting to move to the DB's default tablespace, it'll now hit the
first
part of the OR:

+ (options->tablespaceOid != oldTablespaceOid ||

Without the first stanza setting, it would've hit the 2nd condition:

+ (options->tablespaceOid == MyDatabaseTableSpace &&
OidIsValid(oldTablespaceOid))))

which means: "user requested to move a table to the DB's default
tblspace, and
it was previously on a nondefault space".

So I think we can drop the 2nd half of the OR. Thanks for noticing.

Yes, I have not noticed that we would have already assigned
tablespaceOid to InvalidOid in this case. Back to the v7 we were doing
this assignment a bit later, so this could make sense, but now it seems
to be redundant. For some reason I have mixed these refactorings
separated by a dozen of versions...

Thanks
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On 2020-12-23 10:38, Michael Paquier wrote:

On Tue, Dec 22, 2020 at 03:15:37PM -0600, Justin Pryzby wrote:

Now, I really think utility.c ought to pass in a pointer to a local
ReindexOptions variable to avoid all the memory context, which is
unnecessary
and prone to error.

Yeah, it sounds right to me to just bite the bullet and do this
refactoring, limiting the manipulations of the options as much as
possible across contexts. So +1 from me to merge 0001 and 0002
together.

I have adjusted a couple of comments and simplified a bit more the
code in utility.c. I think that this is commitable, but let's wait
more than a couple of days for Alvaro and Peter first. This is a
period of vacations for a lot of people, and there is no point to
apply something that would need more work at the end. Using hexas for
the flags with bitmasks is the right conclusion IMO, but we are not
alone.

After eyeballing the patch I can add that we should alter this comment:

int options; /* bitmask of VacuumOption */

as you are going to replace VacuumOption with VACOPT_* defs. So this
should say:

/* bitmask of VACOPT_* */

Also I have found naming to be a bit inconsistent:

* we have ReindexOptions, but VacuumParams
* and ReindexOptions->flags, but VacuumParams->options

And the last one, you have used bits32 for Cluster/ReindexOptions, but
left VacuumParams->options as int. Maybe we should also change it to
bits32 for consistency?

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On 2020-Dec-23, Michael Paquier wrote:

bool
-reindex_relation(Oid relid, int flags, int options)
+reindex_relation(Oid relid, int flags, ReindexOptions *options)
{
Relation	rel;
Oid			toast_relid;

Wait a minute. reindex_relation has 'flags' and *also* 'options' with
an embedded 'flags' member? Surely that's not right. I see that they
carry orthogonal sets of options ... but why aren't they a single
bitmask instead of two separate ones? This looks weird and confusing.

Also: it seems a bit weird to me to put the flags inside the options
struct. I would keep them separate -- so initially the options struct
would only have the tablespace OID, on API cleanliness grounds:

struct ReindexOptions
{
tablepaceOid oid;
};
extern bool
reindex_relation(Oid relid, bits32 flags, ReindexOptions *options);

I guess you could argue that it's more performance to set up only two
arguments to the function call instead of three .. but I doubt that's
measurable for anything in DDL-land.

But also, are we really envisioning that these routines would have all
that many additional options? Maybe it is sufficient to do just

extern bool
reindex_relation(Oid relid, bits32 flags, tablespaceOid Oid);

On Wed, Dec 23, 2020 at 07:22:05PM -0300, Alvaro Herrera wrote:

Also: it seems a bit weird to me to put the flags inside the options
struct. I would keep them separate -- so initially the options struct
would only have the tablespace OID, on API cleanliness grounds:

I don't see why they'd be separate or why it's cleaner ?

If the user says REINDEX (CONCURRENTLY, VERBOSE, TABLESPACE ts) , why would we
pass around the boolean flags separately from the other options ?

struct ReindexOptions
{
tablepaceOid oid;
};
extern bool
reindex_relation(Oid relid, bits32 flags, ReindexOptions *options);

But also, are we really envisioning that these routines would have all
that many additional options? Maybe it is sufficient to do just

extern bool
reindex_relation(Oid relid, bits32 flags, tablespaceOid Oid);

That's what we did initially, and Michael suggested to put it into a struct.
Which makes the tablespace patches cleaner for each of REINDEX, CLUSTER,
VACUUM, since it doesn't require modifying the signature of 5-10 functions.
And future patches get to reap the benefit.

These are intended to be like VacuumParams. Consider that ClusterOptions is
proposed to get not just a tablespaceOid but also an idxtablespaceOid.

This was getting ugly:

extern void reindex_index(Oid indexId, bool skip_constraint_checks,
char relpersistence, int options, Oid tablespaceOid);

--
Justin

On 2020-Dec-23, Justin Pryzby wrote:

This was getting ugly:

extern void reindex_index(Oid indexId, bool skip_constraint_checks,
char relpersistence, int options, Oid tablespaceOid)Z

Is this what I suggested?

On Wed, Dec 23, 2020 at 09:14:18PM -0300, Alvaro Herrera wrote:

On 2020-Dec-23, Justin Pryzby wrote:

This was getting ugly:

extern void reindex_index(Oid indexId, bool skip_constraint_checks,
char relpersistence, int options, Oid tablespaceOid)Z

Is this what I suggested?

No ; that was from an earlier revision of the patch adding the tablespace,
before Michael suggested a ReindexOptions struct, which subsumes 'options' and
'tablespaceOid'.

I see now that 'skip_constraint_checks' is from REINDEX_REL_CHECK_CONSTRAINTS.
It seems liek that should be a REINDEXOPT_* flag, rather than REINDEX_REL_*,
so doesn't need to be a separate boolean. See also: 2d3320d3d.

--
Justin

On Wed, Dec 23, 2020 at 07:07:54PM -0600, Justin Pryzby wrote:

On Wed, Dec 23, 2020 at 09:14:18PM -0300, Alvaro Herrera wrote:

On 2020-Dec-23, Justin Pryzby wrote:

This was getting ugly:

extern void reindex_index(Oid indexId, bool skip_constraint_checks,
char relpersistence, int options, Oid tablespaceOid)

Is this what I suggested?

No idea if this is what you suggested, but it would be annoying to
have to change this routine's signature each time we need to pass down
a new option.

No ; that was from an earlier revision of the patch adding the tablespace,
before Michael suggested a ReindexOptions struct, which subsumes 'options' and
'tablespaceOid'.

I see now that 'skip_constraint_checks' is from REINDEX_REL_CHECK_CONSTRAINTS.
It seems like that should be a REINDEXOPT_* flag, rather than REINDEX_REL_*,
so doesn't need to be a separate boolean. See also: 2d3320d3d.

FWIW, it still makes the most sense to me to keep the options that are
extracted from the grammar or things that apply to all the
sub-routines of REINDEX to be tracked in a single structure, so this
should include only the REINDEXOPT_* set for now, with the tablespace
OID as of this thread, and also the reindex filtering options.
REINDEX_REL_* is in my opinion of a different family because they only
apply to reindex_relation(), and partially to reindex_index(), so they
are very localized. In short, anything in need of only
reindex_relation() has no need to know about the whole ReindexOption
business.
--
Michael

On Thu, Dec 24, 2020 at 10:50:34AM +0900, Michael Paquier wrote:

FWIW, it still makes the most sense to me to keep the options that are
extracted from the grammar or things that apply to all the
sub-routines of REINDEX to be tracked in a single structure, so this
should include only the REINDEXOPT_* set for now, with the tablespace
OID as of this thread, and also the reindex filtering options.
REINDEX_REL_* is in my opinion of a different family because they only
apply to reindex_relation(), and partially to reindex_index(), so they
are very localized. In short, anything in need of only
reindex_relation() has no need to know about the whole ReindexOption
business.

I need more coffee here.. reindex_relation() knows about
ReindexOptions. Still it would be weird to track REINDEX_REL_* at a
global level as ExecReindex(), ReindexTable(), ReindexMultipleTables()
and the like don't need to know about that.
--
Michael

On Wed, Dec 23, 2020 at 07:30:35PM +0300, Alexey Kondratov wrote:

After eyeballing the patch I can add that we should alter this comment:

int options; /* bitmask of VacuumOption */

as you are going to replace VacuumOption with VACOPT_* defs. So this should
say:

/* bitmask of VACOPT_* */

Check.

Also I have found naming to be a bit inconsistent:
* we have ReindexOptions, but VacuumParams
* and ReindexOptions->flags, but VacuumParams->options

Check. As ReindexOptions and ClusterOptions are the new members of
the family here, we could change them to use Params instead with
"options" as bits32 internally.

And the last one, you have used bits32 for Cluster/ReindexOptions, but left
VacuumParams->options as int. Maybe we should also change it to bits32 for
consistency?

Yeah, that makes sense. I'll send an updated patch based on that.
--
Michael

On Wed, Jan 13, 2021 at 05:22:49PM +0900, Michael Paquier wrote:

Yeah, that makes sense. I'll send an updated patch based on that.

And here you go as per the attached. I don't think that there was
anything remaining on my radar. This version still needs to be
indented properly though.

Thoughts?
--
Michael

On 2021-01-13 14:34, Michael Paquier wrote:

On Wed, Jan 13, 2021 at 05:22:49PM +0900, Michael Paquier wrote:

Yeah, that makes sense. I'll send an updated patch based on that.

And here you go as per the attached. I don't think that there was
anything remaining on my radar. This version still needs to be
indented properly though.

Thoughts?

Thanks.

+ bits32 options; /* bitmask of CLUSTEROPT_* */

This should say '/* bitmask of CLUOPT_* */', I guess, since there are
only CLUOPT's defined. Otherwise, everything looks as per discussed
upthread.

By the way, something went wrong with the last email subject, so I have
changed it back to the original in this response. I also attached your
patch (with only this CLUOPT_* correction) to keep it in the thread for
sure. Although, postgresql.org's web archive is clever enough to link
your email to the same thread even with different subject.

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On Wed, Jan 13, 2021 at 04:39:40PM +0300, Alexey Kondratov wrote:

+ bits32 options; /* bitmask of CLUSTEROPT_* */

This should say '/* bitmask of CLUOPT_* */', I guess, since there are only
CLUOPT's defined. Otherwise, everything looks as per discussed upthread.

Indeed. Let's first wait a couple of days and see if others have any
comments or objections about this v6.

By the way, something went wrong with the last email subject, so I have
changed it back to the original in this response. I also attached your patch
(with only this CLUOPT_* correction) to keep it in the thread for sure.
Although, postgresql.org's web archive is clever enough to link your email
to the same thread even with different subject.

Oops. Not sure what went wrong here. Thanks.
--
Michael

On Thu, Jan 14, 2021 at 02:18:45PM +0900, Michael Paquier wrote:

Indeed. Let's first wait a couple of days and see if others have any
comments or objections about this v6.

Hearing nothing, I have looked at that again this morning and applied
v6 after a reindent and some adjustments in the comments.
--
Michael

On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:

It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
with an "int options" bitmask which is passed to reindex_index() et al. Your
patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
which I think is good.

a3dc926 is an equivalent of 0001~0003 merged together. 0008 had
better be submitted into a separate thread if there is value to it.
0004~0007 are the pieces remaining. Could it be possible to rebase
things on HEAD and put the tablespace bits into the structures
{Vacuum,Reindex,Cluster}Params?
--
Michael

On Mon, Jan 18, 2021 at 02:18:44PM +0900, Michael Paquier wrote:

On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:

It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
with an "int options" bitmask which is passed to reindex_index() et al. Your
patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
which I think is good.

a3dc926 is an equivalent of 0001~0003 merged together. 0008 had
better be submitted into a separate thread if there is value to it.
0004~0007 are the pieces remaining. Could it be possible to rebase
things on HEAD and put the tablespace bits into the structures
{Vacuum,Reindex,Cluster}Params?

Attached. I will re-review these myself tomorrow.

--
Justin

Hi,
For 0001-Allow-REINDEX-to-change-tablespace.patch :

+ * InvalidOid, use the tablespace in-use instead.

'in-use' seems a bit redundant in the sentence.
How about : InvalidOid, use the tablespace of the index instead.

Cheers

On Mon, Jan 18, 2021 at 12:38 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Mon, Jan 18, 2021 at 02:37:57AM -0600, Justin Pryzby wrote:

Attached. I will re-review these myself tomorrow.

I have begun looking at 0001 and 0002...

+/*
+ * This is mostly duplicating ATExecSetTableSpaceNoStorage,
+ * which should maybe be factored out to a library function.
+ */
Wouldn't it be better to do first the refactoring of 0002 and then
0001 so as REINDEX can use the new routine, instead of putting that
into a comment?

+      This specifies that indexes will be rebuilt on a new tablespace.
+      Cannot be used with "mapped" 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.
What is an unsuitable relation?  How can the end user know that?

This is missing ACL checks when moving the index into a new location,
so this requires some pg_tablespace_aclcheck() calls, and the other
patches share the same issue.

+       else if (partkind == RELKIND_PARTITIONED_TABLE)
+       {
+           Relation rel = table_open(partoid, ShareLock);
+           List    *indexIds = RelationGetIndexList(rel);
+           ListCell *lc;
+
+           table_close(rel, NoLock);
+           foreach (lc, indexIds)
+           {
+               Oid indexid = lfirst_oid(lc);
+               (void) set_rel_tablespace(indexid, params->tablespaceOid);
+           }
+       }
This is really a good question.  ReindexPartitions() would trigger one
transaction per leaf to work on.  Changing the tablespace of the
partitioned table(s) before doing any work has the advantage to tell
any new partition to use the new tablespace.  Now, I see a struggling
point here: what should we do if the processing fails in the middle of
the move, leaving a portion of the leaves in the previous tablespace?
On a follow-up reindex with the same command, should the command force
a reindex even on the partitions that have been moved?  Or could there
be a point in skipping the partitions that are already on the new
tablespace and only process the ones on the previous tablespace?  It
seems to me that the first scenario makes the most sense as currently
a REINDEX works on all the relations defined, though there could be
use cases for the second case.  This should be documented, I think.

There are no tests for partitioned tables, aka we'd want to make sure
that the new partitioned index is on the correct tablespace, as well
as all its leaves. It may be better to have at least two levels of
partitioned tables, as well as a partitioned table with no leaves in
the cases dealt with.

+        *
+        * Even if a table's indexes were moved to a new tablespace, the index
+        * on its toast table is not normally moved.
         */
Still, REINDEX (TABLESPACE) TABLE should move all of them to be
consistent with ALTER TABLE SET TABLESPACE, but that's not the case
with this code, no?  This requires proper test coverage, but there is
nothing of the kind in this patch.
--
Michael

On 2021-Jan-20, Michael Paquier wrote:

+/*
+ * This is mostly duplicating ATExecSetTableSpaceNoStorage,
+ * which should maybe be factored out to a library function.
+ */
Wouldn't it be better to do first the refactoring of 0002 and then
0001 so as REINDEX can use the new routine, instead of putting that
into a comment?

I think merging 0001 and 0002 into a single commit is a reasonable
approach. I don't oppose an initial refactoring commit if you want to
do that, but it doesn't seem necessary.

--
Álvaro Herrera 39°49'30"S 73°17'W

On 2021-Jan-20, Alvaro Herrera wrote:

On 2021-Jan-20, Michael Paquier wrote:

+/*
+ * This is mostly duplicating ATExecSetTableSpaceNoStorage,
+ * which should maybe be factored out to a library function.
+ */
Wouldn't it be better to do first the refactoring of 0002 and then
0001 so as REINDEX can use the new routine, instead of putting that
into a comment?

I think merging 0001 and 0002 into a single commit is a reasonable
approach.

... except it doesn't make a lot of sense to have set_rel_tablespace in
either indexcmds.c or index.c. I think tablecmds.c is a better place
for it. (I would have thought catalog/storage.c, but that one's not the
right abstraction level it seems.)

But surely ATExecSetTableSpaceNoStorage should be using this new
routine. (I first thought 0002 was doing that, since that commit is
calling itself a "refactoring", but now that I look closer, it's not.)

--
Álvaro Herrera 39°49'30"S 73°17'W
"On the other flipper, one wrong move and we're Fatal Exceptions"
(T.U.X.: Term Unit X - http://www.thelinuxreview.com/TUX/)

On 2021-01-20 18:54, Alvaro Herrera wrote:

On 2021-Jan-20, Alvaro Herrera wrote:

On 2021-Jan-20, Michael Paquier wrote:

+/*
+ * This is mostly duplicating ATExecSetTableSpaceNoStorage,
+ * which should maybe be factored out to a library function.
+ */
Wouldn't it be better to do first the refactoring of 0002 and then
0001 so as REINDEX can use the new routine, instead of putting that
into a comment?

I think merging 0001 and 0002 into a single commit is a reasonable
approach.

... except it doesn't make a lot of sense to have set_rel_tablespace in
either indexcmds.c or index.c. I think tablecmds.c is a better place
for it. (I would have thought catalog/storage.c, but that one's not
the
right abstraction level it seems.)

I did a refactoring of ATExecSetTableSpaceNoStorage() in the 0001. New
function SetRelTablesapce() is placed into the tablecmds.c. Following
0002 gets use of it. Is it close to what you and Michael suggested?

But surely ATExecSetTableSpaceNoStorage should be using this new
routine. (I first thought 0002 was doing that, since that commit is
calling itself a "refactoring", but now that I look closer, it's not.)

Yeah, this 'refactoring' was initially referring to refactoring of what
Justin added to one of the previous 0001. And it was meant to be merged
with 0001, once agreed, but we got distracted by other stuff.

I have not yet addressed Michael's concerns regarding reindex of
partitions. I am going to look closer on it tomorrow.

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On 2021-01-20 21:08, Alexey Kondratov wrote:

On 2021-01-20 18:54, Alvaro Herrera wrote:

On 2021-Jan-20, Alvaro Herrera wrote:

On 2021-Jan-20, Michael Paquier wrote:

+/*
+ * This is mostly duplicating ATExecSetTableSpaceNoStorage,
+ * which should maybe be factored out to a library function.
+ */
Wouldn't it be better to do first the refactoring of 0002 and then
0001 so as REINDEX can use the new routine, instead of putting that
into a comment?

I think merging 0001 and 0002 into a single commit is a reasonable
approach.

... except it doesn't make a lot of sense to have set_rel_tablespace
in
either indexcmds.c or index.c. I think tablecmds.c is a better place
for it. (I would have thought catalog/storage.c, but that one's not
the
right abstraction level it seems.)

I did a refactoring of ATExecSetTableSpaceNoStorage() in the 0001. New
function SetRelTablesapce() is placed into the tablecmds.c. Following
0002 gets use of it. Is it close to what you and Michael suggested?

Ugh, forgot to attach the patches. Here they are.

--
Alexey

On 2021-Jan-20, Alexey Kondratov wrote:

On 2021-01-20 21:08, Alexey Kondratov wrote:

I did a refactoring of ATExecSetTableSpaceNoStorage() in the 0001. New
function SetRelTablesapce() is placed into the tablecmds.c. Following
0002 gets use of it. Is it close to what you and Michael suggested?

Ugh, forgot to attach the patches. Here they are.

Yeah, looks reasonable.

+	/* No work if no change in tablespace. */
+	oldTablespaceOid = rd_rel->reltablespace;
+	if (tablespaceOid != oldTablespaceOid ||
+		(tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid)))
+	{
+		/* Update the pg_class row. */
+		rd_rel->reltablespace = (tablespaceOid == MyDatabaseTableSpace) ?
+			InvalidOid : tablespaceOid;
+		CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+		changed = true;
+	}
+
+	if (changed)
+		/* Record dependency on tablespace */
+		changeDependencyOnTablespace(RelationRelationId,
+									 reloid, rd_rel->reltablespace);

Why have a separate "if (changed)" block here instead of merging with
the above?

--
Álvaro Herrera 39°49'30"S 73°17'W

On Wed, Jan 20, 2021 at 03:34:39PM -0300, Alvaro Herrera wrote:

On 2021-Jan-20, Alexey Kondratov wrote:

Ugh, forgot to attach the patches. Here they are.

Yeah, looks reasonable.

Patch 0002 still has a whole set of issues as I pointed out a couple
of hours ago, but if we agree on 0001 as being useful if done
independently, I'd rather get that done first. This way or just
merging both things in a single commit is not a big deal seeing the
amount of code, so I am fine with any approach. It may be possible
that 0001 requires more changes depending on the work to-be-done for
0002 though?

+	/* No work if no change in tablespace. */
+	oldTablespaceOid = rd_rel->reltablespace;
+	if (tablespaceOid != oldTablespaceOid ||
+		(tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid)))
+	{
+		/* Update the pg_class row. */
+		rd_rel->reltablespace = (tablespaceOid == MyDatabaseTableSpace) ?
+			InvalidOid : tablespaceOid;
+		CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+		changed = true;
+	}
+
+	if (changed)
+		/* Record dependency on tablespace */
+		changeDependencyOnTablespace(RelationRelationId,
+									 reloid, rd_rel->reltablespace);

Why have a separate "if (changed)" block here instead of merging with
the above?

Yep.

+       if (SetRelTablespace(reloid, newTableSpace))
+               /* Make sure the reltablespace change is visible */
+               CommandCounterIncrement();
At quick glance, I am wondering why you just don't do a CCI within
SetRelTablespace().
--
Michael

On 2021-01-21 04:41, Michael Paquier wrote:

On Wed, Jan 20, 2021 at 03:34:39PM -0300, Alvaro Herrera wrote:

On 2021-Jan-20, Alexey Kondratov wrote:

Ugh, forgot to attach the patches. Here they are.

Yeah, looks reasonable.

+
+	if (changed)
+		/* Record dependency on tablespace */
+		changeDependencyOnTablespace(RelationRelationId,
+									 reloid, rd_rel->reltablespace);

Why have a separate "if (changed)" block here instead of merging with
the above?

Yep.

Sure, this is a refactoring artifact.

+       if (SetRelTablespace(reloid, newTableSpace))
+               /* Make sure the reltablespace change is visible */
+               CommandCounterIncrement();
At quick glance, I am wondering why you just don't do a CCI within
SetRelTablespace().

I did it that way for a better readability at first, since it looks more
natural when you do some change (SetRelTablespace) and then make them
visible with CCI. Second argument was that in the case of
reindex_index() we have to also call RelationAssumeNewRelfilenode() and
RelationDropStorage() before doing CCI and making the new tablespace
visible. And this part is critical, I guess.

+      This specifies that indexes will be rebuilt on a new tablespace.
+      Cannot be used with "mapped" 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.
What is an unsuitable relation?  How can the end user know that?

This was referring to mapped relations mentioned in the previous
sentence. I have tried to rewrite this part and make it more specific in
my current version. Also added Justin's changes to the docs and comment.

This is missing ACL checks when moving the index into a new location,
so this requires some pg_tablespace_aclcheck() calls, and the other
patches share the same issue.

I added proper pg_tablespace_aclcheck()'s into the reindex_index() and
ReindexPartitions().

+       else if (partkind == RELKIND_PARTITIONED_TABLE)
+       {
+           Relation rel = table_open(partoid, ShareLock);
+           List    *indexIds = RelationGetIndexList(rel);
+           ListCell *lc;
+
+           table_close(rel, NoLock);
+           foreach (lc, indexIds)
+           {
+               Oid indexid = lfirst_oid(lc);
+               (void) set_rel_tablespace(indexid, 
params->tablespaceOid);
+           }
+       }
This is really a good question.  ReindexPartitions() would trigger one
transaction per leaf to work on.  Changing the tablespace of the
partitioned table(s) before doing any work has the advantage to tell
any new partition to use the new tablespace.  Now, I see a struggling
point here: what should we do if the processing fails in the middle of
the move, leaving a portion of the leaves in the previous tablespace?
On a follow-up reindex with the same command, should the command force
a reindex even on the partitions that have been moved?  Or could there
be a point in skipping the partitions that are already on the new
tablespace and only process the ones on the previous tablespace?  It
seems to me that the first scenario makes the most sense as currently
a REINDEX works on all the relations defined, though there could be
use cases for the second case.  This should be documented, I think.

I agree that follow-up REINDEX should also reindex moved partitions,
since REINDEX (TABLESPACE ...) is still reindex at first. I will try to
put something about this part into the docs. Also I think that we cannot
be sure that nothing happened with already reindexed partitions between
two consequent REINDEX calls.

There are no tests for partitioned tables, aka we'd want to make sure
that the new partitioned index is on the correct tablespace, as well
as all its leaves. It may be better to have at least two levels of
partitioned tables, as well as a partitioned table with no leaves in
the cases dealt with.

Yes, sure, it makes sense.

+        *
+        * Even if a table's indexes were moved to a new tablespace, 
the index
+        * on its toast table is not normally moved.
*/
Still, REINDEX (TABLESPACE) TABLE should move all of them to be
consistent with ALTER TABLE SET TABLESPACE, but that's not the case
with this code, no?  This requires proper test coverage, but there is
nothing of the kind in this patch.

You are right, we do not move TOAST indexes now, since
IsSystemRelation() is true for TOAST indexes, so I thought that we
should not allow moving them without allow_system_table_mods=true. Now I
wonder why ALTER TABLE does that.

I am going to attach the new version of patch set today or tomorrow.

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On 2021-01-21 17:06, Alexey Kondratov wrote:

On 2021-01-21 04:41, Michael Paquier wrote:

There are no tests for partitioned tables, aka we'd want to make sure
that the new partitioned index is on the correct tablespace, as well
as all its leaves. It may be better to have at least two levels of
partitioned tables, as well as a partitioned table with no leaves in
the cases dealt with.

Yes, sure, it makes sense.

+        *
+        * Even if a table's indexes were moved to a new tablespace, 
the index
+        * on its toast table is not normally moved.
*/
Still, REINDEX (TABLESPACE) TABLE should move all of them to be
consistent with ALTER TABLE SET TABLESPACE, but that's not the case
with this code, no?  This requires proper test coverage, but there is
nothing of the kind in this patch.

You are right, we do not move TOAST indexes now, since
IsSystemRelation() is true for TOAST indexes, so I thought that we
should not allow moving them without allow_system_table_mods=true. Now
I wonder why ALTER TABLE does that.

I am going to attach the new version of patch set today or tomorrow.

Attached is a new patch set of first two patches, that should resolve
all the issues raised before (ACL, docs, tests) excepting TOAST. Double
thanks for suggestion to add more tests with nested partitioning. I have
found and squashed a huge bug related to the returning back to the
default tablespace using newly added tests.

Regarding TOAST. Now we skip moving toast indexes or throw error if
someone wants to move TOAST index directly. I had a look on ALTER TABLE
SET TABLESPACE and it has a bit complicated logic:

1) You cannot move TOAST table directly.
2) But if you move basic relation that TOAST table belongs to, then they
are moved altogether.
3) Same logic as 2) happens if one does ALTER TABLE ALL IN TABLESPACE
...

That way, ALTER TABLE allows moving TOAST tables (with indexes)
implicitly, but does not allow doing that explicitly. In the same time I
found docs to be vague about such behavior it only says:

All tables in the current database in a tablespace can be moved
by using the ALL IN TABLESPACE ... Note that system catalogs are
not moved by this command

Changing any part of a system catalog table is not permitted.

So actually ALTER TABLE treats TOAST relations as system sometimes, but
sometimes not.

From the end user perspective it makes sense to move TOAST with main
table when doing ALTER TABLE SET TABLESPACE. But should we touch indexes
on TOAST table with REINDEX? We cannot move TOAST relation itself, since
we are doing only a reindex, so we end up in the state when TOAST table
and its index are placed in the different tablespaces. This state is not
reachable with ALTER TABLE/INDEX, so it seem we should not allow it with
REINDEX as well, should we?

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On Thu, Jan 21, 2021 at 11:48:08PM +0300, Alexey Kondratov wrote:

Attached is a new patch set of first two patches, that should resolve all
the issues raised before (ACL, docs, tests) excepting TOAST. Double thanks
for suggestion to add more tests with nested partitioning. I have found and
squashed a huge bug related to the returning back to the default tablespace
using newly added tests.

Regarding TOAST. Now we skip moving toast indexes or throw error if someone
wants to move TOAST index directly. I had a look on ALTER TABLE SET
TABLESPACE and it has a bit complicated logic:

1) You cannot move TOAST table directly.
2) But if you move basic relation that TOAST table belongs to, then they are
moved altogether.
3) Same logic as 2) happens if one does ALTER TABLE ALL IN TABLESPACE ...

That way, ALTER TABLE allows moving TOAST tables (with indexes) implicitly,
but does not allow doing that explicitly. In the same time I found docs to
be vague about such behavior it only says:

All tables in the current database in a tablespace can be moved
by using the ALL IN TABLESPACE ... Note that system catalogs are
not moved by this command

Changing any part of a system catalog table is not permitted.

So actually ALTER TABLE treats TOAST relations as system sometimes, but
sometimes not.

From the end user perspective it makes sense to move TOAST with main table
when doing ALTER TABLE SET TABLESPACE. But should we touch indexes on TOAST
table with REINDEX? We cannot move TOAST relation itself, since we are doing
only a reindex, so we end up in the state when TOAST table and its index are
placed in the different tablespaces. This state is not reachable with ALTER
TABLE/INDEX, so it seem we should not allow it with REINDEX as well, should
we?

+		 * Even if a table's indexes were moved to a new tablespace, the index
+		 * on its toast table is not normally moved.
*/
ReindexParams newparams = *params;

newparams.options &= ~(REINDEXOPT_MISSING_OK);
+		if (!allowSystemTableMods)
+			newparams.tablespaceOid = InvalidOid;

I think you're right. So actually TOAST should never move, even if
allowSystemTableMods, right ?

@@ -292,7 +315,11 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
with <command>REINDEX INDEX</command> or <command>REINDEX TABLE</command>,
respectively. Each partition of the specified partitioned relation is
reindexed in a separate transaction. Those commands cannot be used inside
-   a transaction block when working on a partitioned table or index.
+   a transaction block when working on a partitioned table or index. If
+   <command>REINDEX</command> with <literal>TABLESPACE</literal> executed
+   on partitioned relation fails it may have moved some partitions to the new
+   tablespace. Repeated command will still reindex all partitions even if they
+   are already in the new tablespace.

Minor corrections here:

If a <command>REINDEX</command> command fails when run on a partitioned
relation, and <literal>TABLESPACE</literal> was specified, then it may have
moved indexes on some partitions to the new tablespace. Re-running the command
will reindex all partitions and move previously-unprocessed indexes to the new
tablespace.

--
Justin

On 2021-01-22 00:26, Justin Pryzby wrote:

On Thu, Jan 21, 2021 at 11:48:08PM +0300, Alexey Kondratov wrote:

Attached is a new patch set of first two patches, that should resolve
all
the issues raised before (ACL, docs, tests) excepting TOAST. Double
thanks
for suggestion to add more tests with nested partitioning. I have
found and
squashed a huge bug related to the returning back to the default
tablespace
using newly added tests.

Regarding TOAST. Now we skip moving toast indexes or throw error if
someone
wants to move TOAST index directly. I had a look on ALTER TABLE SET
TABLESPACE and it has a bit complicated logic:

1) You cannot move TOAST table directly.
2) But if you move basic relation that TOAST table belongs to, then
they are
moved altogether.
3) Same logic as 2) happens if one does ALTER TABLE ALL IN TABLESPACE
...

That way, ALTER TABLE allows moving TOAST tables (with indexes)
implicitly,
but does not allow doing that explicitly. In the same time I found
docs to
be vague about such behavior it only says:

All tables in the current database in a tablespace can be moved
by using the ALL IN TABLESPACE ... Note that system catalogs are
not moved by this command

Changing any part of a system catalog table is not permitted.

So actually ALTER TABLE treats TOAST relations as system sometimes,
but
sometimes not.

From the end user perspective it makes sense to move TOAST with main
table
when doing ALTER TABLE SET TABLESPACE. But should we touch indexes on
TOAST
table with REINDEX? We cannot move TOAST relation itself, since we are
doing
only a reindex, so we end up in the state when TOAST table and its
index are
placed in the different tablespaces. This state is not reachable with
ALTER
TABLE/INDEX, so it seem we should not allow it with REINDEX as well,
should
we?

+		 * Even if a table's indexes were moved to a new tablespace, the 
index
+		 * on its toast table is not normally moved.
*/
ReindexParams newparams = *params;

newparams.options &= ~(REINDEXOPT_MISSING_OK);
+		if (!allowSystemTableMods)
+			newparams.tablespaceOid = InvalidOid;

I think you're right. So actually TOAST should never move, even if
allowSystemTableMods, right ?