Re: Index file got removed

On Thu, Nov 17, 2016 at 2:01 PM, Julien Rouhaud
<julien.rouhaud@dalibo.com> wrote:
> After some more thoughts, this is probably broken if a single command
> recreates a mix of indexes that should be and should not be rebuilt.
> The original tablespace information could be saved and restored around
> DefineIndex(), but since it looks like an ugly hack I prefer to wait if
> there's a better fix for this.

OK, I have spent some time on this one. And per the docs
default_tablespace should only assign tablespace to the specified
default only for CREATE commands, and having an index tablespace
switched to the value of default_tablespace is definitely incorrect on
ALTER TABLE TYPE. So we need to find a way to keep around the
tablespace of the original index...

First I had a look at the patch proposed. As far as I can see, it is
broken for at least two reasons. First, in some of the tests I have
added in the patch attached, aka one table with two indexes, one being
on pg_default and the second on a custom tablespace, HEAD switches
both indexes to a custom tablespace, while your patch switches both to
pg_default. In any case, be it an index rewrite or not, both indexes
should stay on their original tablespace. The second problem is that
oldNode refers to relfilenode, so if an index has been reindexed at
least once ALTER TABLE would be broken.

So I have explored in the code what would be the best way to fix the problem:
1) Append the correct tablespace to the SQL string of CREATE INDEX.
Those command strings are stored in changedIndexDefs in tablecmds.c,
and those get generated by pg_get_indexdef_string(), which can append
a tablespace if it is *not* a default. Appending *pg_default* to it
would be dangerous and it would have user-visible changes. OK _string
is not directly SQL callable but any caller of this function would be
impacted.
2) Take advantage of is_alter_table in DefineIndex(), and do some
extra processing when assigning the tablespace of an index. In short,
if IndexStmt->tablespace is NULL and DefineIndex() is used for an
ALTER TABLE, ignore default_tablespace and assign pg_default. This
way, the current tablespace of an index is protected all the time,
even if the tablespace of an index is not the default.

2) is more solid, and is far less invasive than 1), and has no impact
on existing routines of ruleutils.c, so I have implemented it with a
set of regression tests. I am adding that to the next CF so as we
don't forget about it. Thoughts are welcome.
--
Michael