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

On Wed, 20 Nov 2019 at 19:16, Alexey Kondratov
<a.kondratov@postgrespro.ru> wrote:
>
> Hi Steve,
>
> Thank you for review.
>
> On 17.11.2019 3:53, Steve Singer wrote:
> > The following review has been posted through the commitfest application:
> > make installcheck-world:  tested, passed
> > Implements feature:       tested, failed
> > Spec compliant:           not tested
> > Documentation:            tested, failed
> >
> > * I had to replace heap_open/close with table_open/close to get the
> > patch to compile against master
> >
> > In the documentation
> >
> > +     <para>
> > +      This specifies a tablespace, where all rebuilt indexes will be created.
> > +      Can be used only with <literal>REINDEX INDEX</literal> and
> > +      <literal>REINDEX TABLE</literal>, since the system indexes are not
> > +      movable, but <literal>SCHEMA</literal>, <literal>DATABASE</literal> or
> > +      <literal>SYSTEM</literal> very likely will has one.
> > +     </para>
> >
> > I found the "SCHEMA,DATABASE or SYSTEM very likely will has one." portion confusing and would be inclined to remove it or somehow reword it.
>
> In the attached new version REINDEX with TABLESPACE and {SCHEMA,
> DATABASE, SYSTEM} now behaves more like with CONCURRENTLY, i.e. it skips
> unsuitable relations and shows warning. So this section in docs has been
> updated as well.
>
> Also the whole patch has been reworked. I noticed that my code in
> reindex_index was doing pretty much the same as inside
> RelationSetNewRelfilenode. So I just added a possibility to specify new
> tablespace for RelationSetNewRelfilenode instead. Thus, even with
> addition of new tests the patch becomes less complex.
>
> > Consider the following
> >
> > -------------
> >   create index foo_bar_idx on foo(bar) tablespace pg_default;
> > CREATE INDEX
> > reindex=# \d foo
> >                  Table "public.foo"
> >   Column |  Type   | Collation | Nullable | Default
> > --------+---------+-----------+----------+---------
> >   id     | integer |           | not null |
> >   bar    | text    |           |          |
> > Indexes:
> >      "foo_pkey" PRIMARY KEY, btree (id)
> >      "foo_bar_idx" btree (bar)
> >
> > reindex=# reindex index foo_bar_idx tablespace tst1;
> > REINDEX
> > reindex=# reindex index foo_bar_idx tablespace pg_default;
> > REINDEX
> > reindex=# \d foo
> >                  Table "public.foo"
> >   Column |  Type   | Collation | Nullable | Default
> > --------+---------+-----------+----------+---------
> >   id     | integer |           | not null |
> >   bar    | text    |           |          |
> > Indexes:
> >      "foo_pkey" PRIMARY KEY, btree (id)
> >      "foo_bar_idx" btree (bar), tablespace "pg_default"
> > --------
> >
> > It is a bit strange that it says "pg_default" as the tablespace. If I do
> > this with a alter table to the table, moving the table back to pg_default
> > makes it look as it did before.
> >
> > Otherwise the first patch seems fine.
>
> Yes, I missed the fact that default tablespace of database is stored
> implicitly as InvalidOid, but I was setting it explicitly as specified.
> I have changed this behavior to stay consistent with ALTER TABLE.
>
> > With the second patch(for NOWAIT) I did the following
> >
> > T1: begin;
> > T1: insert into foo select generate_series(1,1000);
> > T2: reindex index foo_bar_idx set tablespace tst1 nowait;
> >
> > T2 is waiting for a lock. This isn't what I would expect.
>
> Indeed, I have added nowait option for RangeVarGetRelidExtended, so it
> should not wait if index is locked. However, for reindex we also have to
> put share lock on the parent table relation, which is done by opening it
> via table_open(heapId, ShareLock).
>
> The only one solution I can figure out right now is to wrap all such
> opens with ConditionalLockRelationOid(relId, ShareLock) and then do
> actual open with NoLock. This is how something similar is implemented in
> VACUUM if VACOPT_SKIP_LOCKED is specified. However, there are multiple
> code paths with table_open, so it becomes a bit ugly.
>
> I will leave the second patch aside for now and experiment with it.
> Actually, its main idea was to mimic ALTER INDEX ... SET TABLESPACE
> [NOWAIT] syntax, but probably it is better to stick with more brief
> plain TABLESPACE like in CREATE INDEX.
>

Thank you for working on this.

I looked at v4 patch. Here are some comments:

+               /* Skip all mapped relations if TABLESPACE is specified */
+               if (OidIsValid(tableSpaceOid) &&
+                       classtuple->relfilenode == 0)

I think we can use OidIsValid(classtuple->relfilenode) instead.

---
+     <para>
+      This specifies a tablespace, where all rebuilt indexes will be created.
+      Cannot be used with "mapped" and temporary relations. If
<literal>SCHEMA</literal>,
+      <literal>DATABASE</literal> or <literal>SYSTEM</literal> is
specified, then
+      all unsuitable relations will be skipped and a single
<literal>WARNING</literal>
+      will be generated.
+     </para>

This change says that temporary relation is not supported but it
actually seems to work. Which is correct?

postgres(1:37821)=# select relname, relpersistence from pg_class where
relname like 'tmp%';
 relname  | relpersistence
----------+----------------
 tmp      | t
 tmp_pkey | t
(2 rows)

postgres(1:37821)=# reindex table tmp tablespace ts;
REINDEX

---

+       if (newTableSpaceName)
+       {
+               tableSpaceOid = get_tablespace_oid(newTableSpaceName, false);
+
+               /* Can't move a non-shared relation into pg_global */
+               if (tableSpaceOid == GLOBALTABLESPACE_OID)
+                       ereport(ERROR,
+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                        errmsg("only shared relations
can be placed in pg_global tablespace")));
+       }

+       if (OidIsValid(tablespaceOid) && RelationIsMapped(iRel))
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("cannot move system relation \"%s\"",
+
RelationGetRelationName(iRel))));

ISTM the kind of above errors are the same: the given tablespace
exists but moving tablespace to it is not allowed since it's not
supported in PostgreSQL. So I think we can use
ERRCODE_FEATURE_NOT_SUPPORTED instead of
ERRCODE_INVALID_PARAMETER_VALUE (which is used at 3 places) .
Thoughts?

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services