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

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.