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

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