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

Hi Hackers,

I would like to propose a change, which allow CLUSTER, VACUUM FULL and
REINDEX to modify relation tablespace on the fly. Actually, all these
commands rebuild relation filenodes from the scratch, thus it seems
natural to allow specifying them a new location. It may be helpful, when
a server went out of disk, so you can attach new partition and perform
e.g. VACUUM FULL, which will free some space and move data to a new
location at the same time. Otherwise, you cannot complete VACUUM FULL
until you have up to x2 relation disk space on a single partition.

Please, find attached a patch, which extend CLUSTER, VACUUM FULL and
REINDEX with additional options:

REINDEX [ ( VERBOSE ) ] { INDEX | TABLE } name [ SET TABLESPACE
new_tablespace ]

CLUSTER [VERBOSE] table_name [ USING index_name ] [ SET TABLESPACE
new_tablespace ]
CLUSTER [VERBOSE] [ SET TABLESPACE new_tablespace ]

VACUUM ( FULL [, ...] ) [ SET TABLESPACE new_tablespace ] [
table_and_columns [, ...] ]
VACUUM FULL [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ SET TABLESPACE
new_tablespace ] [ table_and_columns [, ...] ]

Thereby I have a few questions:

1) What do you think about this concept in general?

2) Is SET TABLESPACE an appropriate syntax for this functionality? I
thought also about a plain TABLESPACE keyword, but it seems to be
misleading, and WITH (options) clause like in CREATE SUBSCRIPTION ...
WITH (options). So I preferred SET TABLESPACE, since the same syntax is
used currently in ALTER to change tablespace, but maybe someone will
have a better idea.

3) I was not able to update the lexer for VACUUM FULL to use SET
TABLESPACE after table_and_columns and completely get rid of
shift/reduce conflicts. I guess it happens, since table_and_columns is
optional and may be of variable length, but have no idea how to deal
with it. Any thoughts?

Regards

--
Alexey Kondratov

Postgres Professionalhttps://www.postgrespro.com
Russian Postgres Company

On Mon, Dec 24, 2018 at 6:08 AM Alexey Kondratov
<a(dot)kondratov(at)postgrespro(dot)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(dot)kondratov(at)postgrespro(dot)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(at)2ndquadrant(dot)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(dot)mshk(at)gmail(dot)com> wrote:
> On Thu, Dec 27, 2018 at 10:24 PM Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)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(dot)mshk(at)gmail(dot)com> wrote:
>> On Thu, Dec 27, 2018 at 10:24 PM Alvaro Herrera
>> <alvherre(at)2ndquadrant(dot)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(dot)kondratov(at)postgrespro(dot)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(at)paquier(dot)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(at)paquier(dot)xyz> wrote:
>> It seems to me that it would be good to keep the patch as simple as
>> possible for its first version, and split it into two if you would
>> like to add this new option instead of bundling both together. This
>> makes the review of one and the other more simple. Anyway, regarding
>> the grammar, is SET TABLESPACE really our best choice here? What
>> about:
>> - TABLESPACE = foo, in parenthesis only?
>> - Only using TABLESPACE, without SET at the end of the query?
>>
>> SET is used in ALTER TABLE per the set of subqueries available there,
>> but that's not the case of REINDEX.
> So, earlier in this thread, I suggested making this part of ALTER
> TABLE, and several people seemed to like that idea. Did we have a
> reason for dropping that approach?

If we add this option to REINDEX, then for 'ALTER TABLE tb_name action1,
REINDEX SET TABLESPACE tbsp_name, action3' action2 will be just a direct
alias to 'REINDEX TABLE tb_name SET TABLESPACE tbsp_name'. So it seems
practical to do this for REINDEX first.

The only one concern I have against adding REINDEX to ALTER TABLE in
this context is that it will allow user to write such a chimera:

ALTER TABLE tb_name REINDEX SET TABLESPACE tbsp_name, SET TABLESPACE
tbsp_name;

when they want to move both table and all the indexes. Because simple

ALTER TABLE tb_name REINDEX, SET TABLESPACE tbsp_name;

looks ambiguous. Should it change tablespace of table, indexes or both?

--
Alexey Kondratov

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

On Thu, Sep 19, 2019 at 05:40:41PM +0300, Alexey Kondratov wrote:
> On 19.09.2019 16:21, Robert Haas wrote:
>> So, earlier in this thread, I suggested making this part of ALTER
>> TABLE, and several people seemed to like that idea. Did we have a
>> reason for dropping that approach?

Personally, I don't find this idea very attractive as ALTER TABLE is
already complicated enough with all the subqueries we already support
in the command, all the logic we need to maintain to make combinations
of those subqueries in a minimum number of steps, and also the number
of bugs we have seen because of the amount of complication present.

> If we add this option to REINDEX, then for 'ALTER TABLE tb_name action1,
> REINDEX SET TABLESPACE tbsp_name, action3' action2 will be just a direct
> alias to 'REINDEX TABLE tb_name SET TABLESPACE tbsp_name'. So it seems
> practical to do this for REINDEX first.
>
> The only one concern I have against adding REINDEX to ALTER TABLE in this
> context is that it will allow user to write such a chimera:
>
> ALTER TABLE tb_name REINDEX SET TABLESPACE tbsp_name, SET TABLESPACE
> tbsp_name;
>
> when they want to move both table and all the indexes. Because simple
> ALTER TABLE tb_name REINDEX, SET TABLESPACE tbsp_name;
> looks ambiguous. Should it change tablespace of table, indexes or both?

Tricky question, but we don't change the tablespace of indexes when
using an ALTER TABLE, so I would say no on compatibility grounds.
ALTER TABLE has never touched the tablespace of indexes, and I don't
think that we should begin to do so.
--
Michael

On 20/9/19 4:06, Michael Paquier wrote:
> On Thu, Sep 19, 2019 at 05:40:41PM +0300, Alexey Kondratov wrote:
>> On 19.09.2019 16:21, Robert Haas wrote:
>>> So, earlier in this thread, I suggested making this part of ALTER
>>> TABLE, and several people seemed to like that idea. Did we have a
>>> reason for dropping that approach?
> Personally, I don't find this idea very attractive as ALTER TABLE is
> already complicated enough with all the subqueries we already support
> in the command, all the logic we need to maintain to make combinations
> of those subqueries in a minimum number of steps, and also the number
> of bugs we have seen because of the amount of complication present.

Yes, but please keep the other options: At it is, cluster, vacuum full
and reindex already rewrite the table in full; Being able to write the
result to a different tablespace than the original object was stored in
enables a whole world of very interesting possibilities.... including a
quick way out of a "so little disk space available that vacuum won't
work properly" situation --- which I'm sure MANY users will appreciate,
including me

> If we add this option to REINDEX, then for 'ALTER TABLE tb_name action1,
> REINDEX SET TABLESPACE tbsp_name, action3' action2 will be just a direct
> alias to 'REINDEX TABLE tb_name SET TABLESPACE tbsp_name'. So it seems
> practical to do this for REINDEX first.
>
> The only one concern I have against adding REINDEX to ALTER TABLE in this
> context is that it will allow user to write such a chimera:
>
> ALTER TABLE tb_name REINDEX SET TABLESPACE tbsp_name, SET TABLESPACE
> tbsp_name;
>
> when they want to move both table and all the indexes. Because simple
> ALTER TABLE tb_name REINDEX, SET TABLESPACE tbsp_name;
> looks ambiguous. Should it change tablespace of table, indexes or both?

Indeed.

IMHO, that form of the command should not allow that much flexibility...
even on the "principle of least surprise" grounds :S

That is, I'd restrict the ability to change (output) tablespace to the
"direct" form --- REINDEX name, VACUUM (FULL) name, CLUSTER name ---
whereas the ALTER table|index SET TABLESPACE would continue to work.

Now that I come to think of it, maybe saying "output" or "move to"
rather than "set tablespace" would make more sense for this variation of
the commands? (clearer, less prone to confusion)?

> Tricky question, but we don't change the tablespace of indexes when
> using an ALTER TABLE, so I would say no on compatibility grounds.
> ALTER TABLE has never touched the tablespace of indexes, and I don't
> think that we should begin to do so.

Indeed.

I might be missing something, but is there any reason to not *require* a
explicit transaction for the above multi-action commands? I mean, have
it be:

BEGIN;

ALTER TABLE tb_name SET TABLESPACE tbsp_name;    -- moves the table ....
but possibly NOT the indexes?

ALTER TABLE tb_name REINDEX [OUTPUT TABLESPACE tbsp_name];    --
REINDEX, placing the resulting index on tbsp_name instead of the
original one

COMMIT;

... and have the parser/planner combine the steps if it'd make sense (it
probably wouldn't in this example)?

Just my .02€

Thanks,

    / J.L.

On 2019-Sep-19, Robert Haas wrote:

> So, earlier in this thread, I suggested making this part of ALTER
> TABLE, and several people seemed to like that idea. Did we have a
> reason for dropping that approach?

Hmm, my own reading of that was to add tablespace changing abilities to
ALTER TABLE *in addition* to this patch, not instead of it.

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

On 20.09.2019 19:38, Alvaro Herrera wrote:
> On 2019-Sep-19, Robert Haas wrote:
>
>> So, earlier in this thread, I suggested making this part of ALTER
>> TABLE, and several people seemed to like that idea. Did we have a
>> reason for dropping that approach?
> Hmm, my own reading of that was to add tablespace changing abilities to
> ALTER TABLE *in addition* to this patch, not instead of it.

That was my understanding too.

On 20.09.2019 11:26, Jose Luis Tallon wrote:
> On 20/9/19 4:06, Michael Paquier wrote:
>> Personally, I don't find this idea very attractive as ALTER TABLE is
>> already complicated enough with all the subqueries we already support
>> in the command, all the logic we need to maintain to make combinations
>> of those subqueries in a minimum number of steps, and also the number
>> of bugs we have seen because of the amount of complication present.
>
> Yes, but please keep the other options: At it is, cluster, vacuum full
> and reindex already rewrite the table in full; Being able to write the
> result to a different tablespace than the original object was stored
> in enables a whole world of very interesting possibilities....
> including a quick way out of a "so little disk space available that
> vacuum won't work properly" situation --- which I'm sure MANY users
> will appreciate, including me

Yes, sure, that was my main motivation. The first message in the thread
contains a patch, which adds SET TABLESPACE support to all of CLUSTER,
VACUUM FULL and REINDEX. However, there came up an idea to integrate
CLUSTER/VACUUM FULL with ALTER TABLE and do their work + all the ALTER
TABLE stuff in a single table rewrite. I've dig a little bit into this
and ended up with some architectural questions and concerns [1]. So I
decided to start with a simple REINDEX patch.

Anyway, I've followed Michael's advice and split the last patch into two:

1) Adds all the main functionality, but with simplified 'REINDEX INDEX [
CONCURRENTLY ] ... [ TABLESPACE ... ]' grammar;

2) Adds a more sophisticated syntax with '[ SET TABLESPACE ... [ NOWAIT
] ]'.

Patch 1 contains all the docs and tests and may be applied/committed
separately or together with 2, which is fully optional.

Recent merge conflicts and reindex_index validations order are also
fixed in the attached version.

[1]
/message-id/6b2a5c4de19f111ef24b63428033bb67%40postgrespro.ru

Regards

--
Alexey Kondratov

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

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, failed
Spec compliant: not tested
Documentation: tested, failed

* I had to replace heap_open/close with table_open/close to get the
patch to compile against master

In the documentation

+ <para>
+ This specifies a tablespace, where all rebuilt indexes will be created.
+ Can be used only with <literal>REINDEX INDEX</literal> and
+ <literal>REINDEX TABLE</literal>, since the system indexes are not
+ movable, but <literal>SCHEMA</literal>, <literal>DATABASE</literal> or
+ <literal>SYSTEM</literal> very likely will has one.
+ </para>

I found the "SCHEMA,DATABASE or SYSTEM very likely will has one." portion confusing and would be inclined to remove it 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(dot)kondratov(at)postgrespro(dot)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) /message-id/flat/20200208150453.GV403%40telsasoft.com

On Sat, Feb 29, 2020 at 08:53:04AM -0600, Justin Pryzby wrote:
> On Sat, Feb 29, 2020 at 03:35:27PM +0300, Alexey Kondratov wrote:
> > Anyway, new version is attached. It is rebased in order to resolve conflicts
> > with a recent fix of REINDEX CONCURRENTLY + temp relations, and includes
> > this small comment fix.
>
> Thanks for rebasing - I actually started to do that yesterday.
>
> I extracted the bits from your original 0001 patch which handled CLUSTER and
> VACUUM FULL. I don't think if there's any interest in combining that with
> ALTER anymore. On another thread (1), I tried to implement that, and Tom
> pointed out problem with the implementation, but also didn't like the idea.
>
> I'm including some proposed fixes, but didn't yet update the docs, errors or
> tests for that. (I'm including your v8 untouched in hopes of not messing up
> the cfbot). My fixes avoid an issue if you try to REINDEX onto pg_default, I
> think due to moving system toast indexes.

I was able to avoid this issue by adding a call to GetNewRelFileNode, even
though that's already called by RelationSetNewRelfilenode(). Not sure if
there's a better way, or if it's worth Alexey's v3 patch which added a
tablespace param to RelationSetNewRelfilenode.

The current logic allows moving all the indexes and toast indexes, but I think
we should use IsSystemRelation() unless allow_system_table_mods, like existing
behavior of ALTER.

template1=# ALTER TABLE pg_extension_oid_index SET tablespace pg_default;
ERROR: permission denied: "pg_extension_oid_index" is a system catalog
template1=# REINDEX INDEX pg_extension_oid_index TABLESPACE pg_default;
REINDEX

Finally, I think the CLUSTER is missing permission checks. It looks like
relation_is_movable was factored out, but I don't see how that helps ?

Alexey, I'm hoping to hear back if you think these changes are ok or if you'll
publish a new version of the patch addressing the crash I reported.
Or if you're too busy, maybe someone else can adopt the patch (I can help).

--
Justin

Hi Justin,

On 09.03.2020 23:04, Justin Pryzby wrote:
> On Sat, Feb 29, 2020 at 08:53:04AM -0600, Justin Pryzby wrote:
>> On Sat, Feb 29, 2020 at 03:35:27PM +0300, Alexey Kondratov wrote:
>>> Anyway, new version is attached. It is rebased in order to resolve conflicts
>>> with a recent fix of REINDEX CONCURRENTLY + temp relations, and includes
>>> this small comment fix.
>> Thanks for rebasing - I actually started to do that yesterday.
>>
>> I extracted the bits from your original 0001 patch which handled CLUSTER and
>> VACUUM FULL. I don't think if there's any interest in combining that with
>> ALTER anymore. On another thread (1), I tried to implement that, and Tom
>> pointed out problem with the implementation, but also didn't like the idea.
>>
>> I'm including some proposed fixes, but didn't yet update the docs, errors or
>> tests for that. (I'm including your v8 untouched in hopes of not messing up
>> the cfbot). My fixes avoid an issue if you try to REINDEX onto pg_default, I
>> think due to moving system toast indexes.
> I was able to avoid this issue by adding a call to GetNewRelFileNode, even
> though that's already called by RelationSetNewRelfilenode(). Not sure if
> there's a better way, or if it's worth Alexey's v3 patch which added a
> tablespace param to RelationSetNewRelfilenode.

Do you have any understanding of what exactly causes this error? I have
tried to debug it a little bit, but still cannot figure out why we need
this extra GetNewRelFileNode() call and a mechanism how it helps.

Probably you mean v4 patch. Yes, interestingly, if we do everything at
once inside RelationSetNewRelfilenode(), then there is no issue at all with:

REINDEX DATABASE template1 TABLESPACE pg_default;

It feels like I am doing a monkey coding here, so I want to understand
it better :)

> The current logic allows moving all the indexes and toast indexes, but I think
> we should use IsSystemRelation() unless allow_system_table_mods, like existing
> behavior of ALTER.
>
> template1=# ALTER TABLE pg_extension_oid_index SET tablespace pg_default;
> ERROR: permission denied: "pg_extension_oid_index" is a system catalog
> template1=# REINDEX INDEX pg_extension_oid_index TABLESPACE pg_default;
> REINDEX

Yeah, we definitely should obey the same rules as ALTER TABLE / INDEX in
my opinion.

> Finally, I think the CLUSTER is missing permission checks. It looks like
> relation_is_movable was factored out, but I don't see how that helps ?

I did this relation_is_movable refactoring in order to share the same
check between REINDEX + TABLESPACE and ALTER INDEX + SET TABLESPACE.
Then I realized that REINDEX already has its own temp tables check and
does mapped relations validation in multiple places, so I just added
global tablespace checks instead. Thus, relation_is_movable seems to be
outdated right now. Probably, we have to do another refactoring here
once all proper validations will be accumulated in this patch set.

> Alexey, I'm hoping to hear back if you think these changes are ok or if you'll
> publish a new version of the patch addressing the crash I reported.
> Or if you're too busy, maybe someone else can adopt the patch (I can help).

Sorry for the late response, I was not going to abandon this patch, but
was a bit busy last month.

Many thanks for you review and fixups! There are some inconsistencies
like mentions of SET TABLESPACE in error messages and so on. I am going
to refactor and include your fixes 0003-0004 into 0001 and 0002, but
keep 0005 separated for now, since this part requires more understanding
IMO (and comparison with v4 implementation).

That way, I am going to prepare a more clear patch set till the middle
of the next week. I will be glad to receive more feedback from you then.

Regards

--
Alexey Kondratov

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

On Thu, Mar 12, 2020 at 08:08:46PM +0300, Alexey Kondratov wrote:
> On 09.03.2020 23:04, Justin Pryzby wrote:
>> On Sat, Feb 29, 2020 at 08:53:04AM -0600, Justin Pryzby wrote:
>>> On Sat, Feb 29, 2020 at 03:35:27PM +0300, Alexey Kondratov wrote:
>>> tests for that. (I'm including your v8 untouched in hopes of not messing up
>>> the cfbot). My fixes avoid an issue if you try to REINDEX onto pg_default, I
>>> think due to moving system toast indexes.
>> I was able to avoid this issue by adding a call to GetNewRelFileNode, even
>> though that's already called by RelationSetNewRelfilenode(). Not sure if
>> there's a better way, or if it's worth Alexey's v3 patch which added a
>> tablespace param to RelationSetNewRelfilenode.
>
> Do you have any understanding of what exactly causes this error? I have
> tried to debug it a little bit, but still cannot figure out why we need this
> extra GetNewRelFileNode() call and a mechanism how it helps.

The PANIC is from smgr hashtable, which couldn't find an entry it expected. My
very tentative understanding is that smgr is prepared to handle a *relation*
which is dropped/recreated multiple times in a transaction, but it's *not*
prepared to deal with a given RelFileNode(Backend) being dropped/recreated,
since that's used as a hash key.

I revisited it and solved it in a somewhat nicer way. It's still not clear to
me if there's an issue with your original way of adding a tablespace parameter
to RelationSetNewRelfilenode().

> Probably you mean v4 patch. Yes, interestingly, if we do everything at once
> inside RelationSetNewRelfilenode(), then there is no issue at all with:

Yes, I meant to say "worth revisiting the v4 patch".

> Many thanks for you review and fixups! There are some inconsistencies like
> mentions of SET TABLESPACE in error messages and so on. I am going to
> refactor and include your fixes 0003-0004 into 0001 and 0002, but keep 0005
> separated for now, since this part requires more understanding IMO (and
> comparison with v4 implementation).

I'd suggest to keep the CLUSTER/VACUUM FULL separate from REINDEX, in case
Michael or someone else wants to progress one but cannot commit to both. But
probably we should plan to finish this in July.

--
Justin

On 2020-03-26 02:40, Justin Pryzby wrote:
> On Thu, Mar 12, 2020 at 08:08:46PM +0300, Alexey Kondratov wrote:
>> On 09.03.2020 23:04, Justin Pryzby wrote:
>>> On Sat, Feb 29, 2020 at 08:53:04AM -0600, Justin Pryzby wrote:
>>>> On Sat, Feb 29, 2020 at 03:35:27PM +0300, Alexey Kondratov wrote:
>>>> tests for that. (I'm including your v8 untouched in hopes of not
>>>> messing up
>>>> the cfbot). My fixes avoid an issue if you try to REINDEX onto
>>>> pg_default, I
>>>> think due to moving system toast indexes.
>>> I was able to avoid this issue by adding a call to GetNewRelFileNode,
>>> even
>>> though that's already called by RelationSetNewRelfilenode(). Not
>>> sure if
>>> there's a better way, or if it's worth Alexey's v3 patch which added
>>> a
>>> tablespace param to RelationSetNewRelfilenode.
>>
>> Do you have any understanding of what exactly causes this error? I
>> have
>> tried to debug it a little bit, but still cannot figure out why we
>> need this
>> extra GetNewRelFileNode() call and a mechanism how it helps.
>
> The PANIC is from smgr hashtable, which couldn't find an entry it
> expected. My
> very tentative understanding is that smgr is prepared to handle a
> *relation*
> which is dropped/recreated multiple times in a transaction, but it's
> *not*
> prepared to deal with a given RelFileNode(Backend) being
> dropped/recreated,
> since that's used as a hash key.
>
> I revisited it and solved it in a somewhat nicer way.
>

I included your new solution regarding this part from 0004 into 0001. It
seems that at least a tip of the problem was in that we tried to change
tablespace to pg_default being already there.

>
> It's still not clear to
> me if there's an issue with your original way of adding a tablespace
> parameter
> to RelationSetNewRelfilenode().
>

Yes, it is not clear for me too.

>
>> Many thanks for you review and fixups! There are some inconsistencies
>> like
>> mentions of SET TABLESPACE in error messages and so on. I am going to
>> refactor and include your fixes 0003-0004 into 0001 and 0002, but keep
>> 0005
>> separated for now, since this part requires more understanding IMO
>> (and
>> comparison with v4 implementation).
>
> I'd suggest to keep the CLUSTER/VACUUM FULL separate from REINDEX, in
> case
> Michael or someone else wants to progress one but cannot commit to
> both.
>

Yes, sure, I did not have plans to melt everything into a single patch.

So, it has taken much longer to understand and rework all these fixes
and permission validations. Attached is the updated patch set.

0001:
— It is mostly the same, but refactored
— I also included your most recent fix for REINDEX DATABASE with
allow_system_table_mods=1
— With this patch REINDEX + TABLESPACE simply errors out, when index on
TOAST table is met and allow_system_table_mods=0

0002:
— I reworked it a bit, since REINDEX CONCURRENTLY is not allowed on
system catalog anyway, that is checked at the hegher levels of statement
processing. So we have to care about TOAST relations
— Also added the same check into the plain REINDEX
— It works fine, but I am not entirely happy that with this patch
errors/warnings are a bit inconsistent:

template1=# REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_12773_index
TABLESPACE pg_default;
WARNING: skipping tablespace change of "pg_toast_12773_index"
DETAIL: Cannot move system relation, only REINDEX CONCURRENTLY is
performed.

template1=# REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_12773
TABLESPACE pg_default;
ERROR: permission denied: "pg_toast_12773" is a system catalog

And REINDEX DATABASE CONCURRENTLY will generate a warning again.

Maybe we should always throw a warning and do only reindex if it is not
possible to change tablespace?

0003:
— I have get rid of some of previous refactoring pieces like
check_relation_is_movable for now. Let all these validations to settle
and then think whether we could do it better
— Added CLUSTER to copy/equalfuncs
— Cleaned up messages and comments

I hope that I did not forget anything from your proposals.

--
Alexey Kondratov

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

> I included your new solution regarding this part from 0004 into 0001. It
> seems that at least a tip of the problem was in that we tried to change
> tablespace to pg_default being already there.

Right, causing it to try to drop that filenode twice.

> +++ b/doc/src/sgml/ref/cluster.sgml
> + The name of a specific tablespace to store clustered relations.

Could you phrase these like you did in the comments:
" the name of the tablespace where the clustered relation is to be rebuilt."

> +++ b/doc/src/sgml/ref/reindex.sgml
> + The name of a specific tablespace to store rebuilt indexes.

" The name of a tablespace where indexes will be rebuilt"

> +++ b/doc/src/sgml/ref/vacuum.sgml
> + The name of a specific tablespace to write a new copy of the table.

> + This specifies a tablespace, where all rebuilt indexes will be created.

say "specifies the tablespace where", with no comma.

> + else if (!OidIsValid(classtuple->relfilenode))
> + {
> + /*
> + * Skip all mapped relations.
> + * relfilenode == 0 checks after that, similarly to
> + * RelationIsMapped().

I would say "OidIsValid(relfilenode) checks for that, ..."

> @@ -262,7 +280,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
> * and error messages should refer to the operation as VACUUM not CLUSTER.
> */
> void
> -cluster_rel(Oid tableOid, Oid indexOid, int options)
> +cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, int options)

Add a comment here about the tablespaceOid parameter, like the other functions
where it's added.

The permission checking is kind of duplicitive, so I'd suggest to factor it
out. Ideally we'd only have one place that checks for pg_global/system/mapped.
It needs to check that it's not a system relation, or that system_table_mods
are allowed, and in any case that if it's a mapped rel, that it's not being
moved. I would pass a boolean indicating if the tablespace is being changed.

Another issue is this:
> +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> ] [ <replaceable class="parameter">table_and_columns</replaceable> [, ...] ]
As you mentioned in your v1 patch, in the other cases, "tablespace
[tablespace]" is added at the end of the command rather than in the middle. I
wasn't able to make that work, maybe because "tablespace" isn't a fully
reserved word (?). I didn't try with "SET TABLESPACE", although I understand
it'd be better without "SET".

--
Justin

On 2020-03-26 21:01, Justin Pryzby wrote:
>
>> @@ -262,7 +280,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
>> * and error messages should refer to the operation as VACUUM not
>> CLUSTER.
>> */
>> void
>> -cluster_rel(Oid tableOid, Oid indexOid, int options)
>> +cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, int
>> options)
>
> Add a comment here about the tablespaceOid parameter, like the other
> functions
> where it's added.
>
> The permission checking is kind of duplicitive, so I'd suggest to
> factor it
> out. Ideally we'd only have one place that checks for
> pg_global/system/mapped.
> It needs to check that it's not a system relation, or that
> system_table_mods
> are allowed, and in any case that if it's a mapped rel, that it's not
> being
> moved. I would pass a boolean indicating if the tablespace is being
> changed.
>

Yes, but I wanted to make sure first that all necessary validations are
there to do not miss something as I did last time. I do not like
repetitive code either, so I would like to introduce more common check
after reviewing the code as a whole.

>
> Another issue is this:
>> +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable
>> class="parameter">new_tablespace</replaceable> ] [ <replaceable
>> class="parameter">table_and_columns</replaceable> [, ...] ]
> As you mentioned in your v1 patch, in the other cases, "tablespace
> [tablespace]" is added at the end of the command rather than in the
> middle. I
> wasn't able to make that work, maybe because "tablespace" isn't a fully
> reserved word (?). I didn't try with "SET TABLESPACE", although I
> understand
> it'd be better without "SET".
>

Initially I tried "SET TABLESPACE", but also failed to completely get
rid of shift/reduce conflicts. I will try to rewrite VACUUM's part again
with OptTableSpace. Maybe I will manage it this time.

I will take into account all your text edits as well.

Thanks
--
Alexey Kondratov

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

> Another issue is this:
> > +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> ] [ <replaceable class="parameter">table_and_columns</replaceable> [, ...] ]
> As you mentioned in your v1 patch, in the other cases, "tablespace
> [tablespace]" is added at the end of the command rather than in the middle. I
> wasn't able to make that work, maybe because "tablespace" isn't a fully
> reserved word (?). I didn't try with "SET TABLESPACE", although I understand
> it'd be better without "SET".

I think we should use the parenthesized syntax for vacuum - it seems clear in
hindsight.

Possibly REINDEX should use that, too, instead of adding OptTablespace at the
end. I'm not sure.

CLUSTER doesn't support parenthesized syntax, but .. maybe it should?

Also, perhaps VAC FULL (and CLUSTER, if it grows parenthesized syntax), should
support something like this:

USING INDEX TABLESPACE name

I guess I would prefer just "index tablespace", without "using":

|VACUUM(FULL, TABLESPACE ts, INDEX TABLESPACE its) t;
|CLUSTER(VERBOSE, TABLESPACE ts, INDEX TABLESPACE its) t;

--
Justin

On Thu, Mar 26, 2020 at 11:01:06PM -0500, Justin Pryzby wrote:
> > Another issue is this:
> > > +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> ] [ <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.

> Possibly REINDEX should use that, too, instead of adding OptTablespace at the
> end. I'm not sure.
>
> CLUSTER doesn't support parenthesized syntax, but .. maybe it should?
>
> Also, perhaps VAC FULL (and CLUSTER, if it grows parenthesized syntax), should
> support something like this:
>
> USING INDEX TABLESPACE name
>
> I guess I would prefer just "index tablespace", without "using":
>
> |VACUUM(FULL, TABLESPACE ts, INDEX TABLESPACE its) t;
> |CLUSTER(VERBOSE, TABLESPACE ts, INDEX TABLESPACE its) t;

On Thu, Mar 26, 2020 at 11:01:06PM -0500, Justin Pryzby wrote:
> > Another issue is this:
> > > +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> ] [ <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..
/message-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..
> /message-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.
/message-id/attachment/105574/v4-0001-Allow-REINDEX-and-REINDEX-CONCURRENTLY-to-change-tablespace.patch
/message-id/20191127035416.GG5435%40paquier.xyz

> --- a/src/backend/catalog/index.c
> +++ b/src/backend/catalog/index.c
> @@ -3480,6 +3518,47 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
> */
> CheckTableNotInUse(iRel, "REINDEX INDEX");
>
> + if (tablespaceOid == MyDatabaseTableSpace)
> + tablespaceOid = InvalidOid;
> +
> + /*
> + * 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)
> + {
> + Relation pg_class;
> + Form_pg_class rd_rel;
> + HeapTuple tuple;
> +
> + /* First get a modifiable copy of the relation's pg_class row */
> + pg_class = table_open(RelationRelationId, RowExclusiveLock);
> +
> + tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(indexId));
> + if (!HeapTupleIsValid(tuple))
> + elog(ERROR, "cache lookup failed for relation %u", indexId);
> + rd_rel = (Form_pg_class) GETSTRUCT(tuple);
> +
> + /*
> + * Mark the relation as ready to be dropped at transaction commit,
> + * before making visible the new tablespace change so as this won't
> + * miss things.
> + */
> + RelationDropStorage(iRel);
> +
> + /* Update the pg_class row */
> + rd_rel->reltablespace = tablespaceOid;
> + CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
> +
> + heap_freetuple(tuple);
> +
> + table_close(pg_class, RowExclusiveLock);
> +
> + /* Make sure the reltablespace change is visible */
> + CommandCounterIncrement();

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

Actually, the last time we discussed this point I only got the gut
feeling that this is a subtle place and it is very easy to break things
with these changes. However, it isn't clear for me how exactly. That
way, I'd be glad if Michael could reword his explanation, so it'd more
clear for me as well.

BTW, I've started doing a review of the last patch set yesterday and
will try to post some comments later.

Regards
--
Alexey Kondratov

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

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(at)telsasoft(dot)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(at)telsasoft(dot)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.
/message-id/20200902022410.GA20149%40telsasoft.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.
> /message-id/20200902022410.GA20149%40telsasoft.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.
>> /message-id/20200902022410.GA20149%40telsasoft.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.
> > /message-id/20200902022410.GA20149%40telsasoft.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