Thread: Index file got removed
Hi,
Please look at a below sequence of SQL.
set default_tablespace to '';
create table test(id int primary key, processid bigint not null ,
ruleid bigint not null );
create index on test (ruleid );
create index on test (processid );
set default_tablespace to tblsp;
alter table test alter processid type bigint, alter processid drop
not null;
I am creating a table test and index for that table on default tablespace.
Then i'm changing default_tablespace to some other tablespace.
After that altering one of the index column.
This alter operation moves the index to new default_tablespace, but index
file is missing in new tablespace.
So we can not access the table.
----
db=# select * from test;
ERROR: could not open file "pg_tblspc/16703/PG_9.6_201608131/16385/16710":
No such file or directory
db=# \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
-----------+---------+-----------+---------+--------------+-------------
id | integer | not null | plain | |
processid | bigint | | plain | |
ruleid | bigint | not null | plain | |
Indexes:
"test_pkey" PRIMARY KEY, btree (id)
"test_processid_idx" btree (processid), tablespace "tblsp"
"test_ruleid_idx" btree (ruleid)
---
I tried it on 9.5.1 & 9.6.0, index got missed in both version.
-sudalai
-----
sudalai
--
View this message in context: http://postgresql.nabble.com/Index-file-got-removed-tp5930602.html
Sent from the PostgreSQL - bugs mailing list archive at Nabble.com.
On Wed, Nov 16, 2016 at 3:16 AM, sudalai <sudalait2@gmail.com> wrote: > I am creating a table test and index for that table on default tablespace. > Then i'm changing default_tablespace to some other tablespace. > After that altering one of the index column. > This alter operation moves the index to new default_tablespace, but index > file is missing in new tablespace. > So we can not access the table. > > ---- > db=# select * from test; > ERROR: could not open file "pg_tblspc/16703/PG_9.6_201608131/16385/16710": > No such file or directory That's easily reproducible, your test case is just missing the tablespace creation to be complete: set default_tablespace to ''; \! rm -rf /home/ioltas/Desktop/tbspace/* create tablespace tblsp location '/home/ioltas/ioltas/tbspace/'; create table test(id int primary key, processid bigint not null , ruleid bigint not null ); create index on test (ruleid); create index on test (processid); set default_tablespace to tblsp; alter table test alter processid type bigint, alter processid drop not null; select * from test; -- boom I am just digging into it, instinctively that would be something in tablecmds.c.. -- Michael
Michael Paquier <michael.paquier@gmail.com> writes:
> On Wed, Nov 16, 2016 at 3:16 AM, sudalai <sudalait2@gmail.com> wrote:
>> I am creating a table test and index for that table on default tablespace.
>> Then i'm changing default_tablespace to some other tablespace.
>> After that altering one of the index column.
>> This alter operation moves the index to new default_tablespace, but index
>> file is missing in new tablespace.
> I am just digging into it, instinctively that would be something in
> tablecmds.c..
I'm betting that where it reconstructs textual commands to create the new
indexes, it's forgotten to do anything about tablespaces.
regards, tom lane
On 16/11/2016 18:26, Tom Lane wrote: > Michael Paquier <michael.paquier@gmail.com> writes: >> On Wed, Nov 16, 2016 at 3:16 AM, sudalai <sudalait2@gmail.com> wrote: >>> I am creating a table test and index for that table on default tablespace. >>> Then i'm changing default_tablespace to some other tablespace. >>> After that altering one of the index column. >>> This alter operation moves the index to new default_tablespace, but index >>> file is missing in new tablespace. > >> I am just digging into it, instinctively that would be something in >> tablecmds.c.. > > I'm betting that where it reconstructs textual commands to create the new > indexes, it's forgotten to do anything about tablespaces. I looked at it, and IIUC the issue is that during AT_PASS_OLD_INDEX pass, ATExecAddIndex defines a new index using the old heap in original tablespace (stmt->oldNode is valid), and DefineIndex will create a new cat entry using the current default tablespace instead of the existing index one. The only way I found to fix this is to save the original tablespace in ATExecAddIndex() if the old heap is reused, see attached patch. I'm not at all familiar with this part of code, but at least initdb and regression tests don't fail. -- Julien Rouhaud http://dalibo.com - http://dalibo.org
Attachment
On 17/11/2016 22:31, Julien Rouhaud wrote: > On 16/11/2016 18:26, Tom Lane wrote: >> Michael Paquier <michael.paquier@gmail.com> writes: >>> On Wed, Nov 16, 2016 at 3:16 AM, sudalai <sudalait2@gmail.com> wrote: >>>> I am creating a table test and index for that table on default tablespace. >>>> Then i'm changing default_tablespace to some other tablespace. >>>> After that altering one of the index column. >>>> This alter operation moves the index to new default_tablespace, but index >>>> file is missing in new tablespace. >> >>> I am just digging into it, instinctively that would be something in >>> tablecmds.c.. >> >> I'm betting that where it reconstructs textual commands to create the new >> indexes, it's forgotten to do anything about tablespaces. > > I looked at it, and IIUC the issue is that during AT_PASS_OLD_INDEX > pass, ATExecAddIndex defines a new index using the old heap in original > tablespace (stmt->oldNode is valid), and DefineIndex will create a new > cat entry using the current default tablespace instead of the existing > index one. > > The only way I found to fix this is to save the original tablespace in > ATExecAddIndex() if the old heap is reused, see attached patch. I'm not > at all familiar with this part of code, but at least initdb and > regression tests don't fail. 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. -- Julien Rouhaud http://dalibo.com - http://dalibo.org
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
Attachment
On Tue, Nov 22, 2016 at 09:57:29PM +0900, Michael Paquier wrote: >=20 > So I have explored in the code what would be the best way to fix the prob= lem: > 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. >=20 > 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. I agree that 2nd solution looks better. I reviewed your patch and everythi= ng works as intended, same for new regression tests which seem to cover all possible cases. I mark it as ready for committers. -- Julien Rouhaud http://dalibo.com - http://dalibo.org
Julien Rouhaud <julien.rouhaud@dalibo.com> writes:
> I agree that 2nd solution looks better. I reviewed your patch and every=
thing
> works as intended, same for new regression tests which seem to cover all
> possible cases. I mark it as ready for committers.
Thanks for the review. I plan to look at this shortly.
regards, tom lane
Michael Paquier <michael.paquier@gmail.com> writes:
> So I have explored in the code what would be the best way to fix the pro=
blem:
> 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.
I think that this is the correct fix. There aren't any other callers
in the core code, and even if some third party is calling it, it's
fairly hard to see what usage would be okay with a TABLESPACE clause
being appended some of the time but not all of the time. Also, both
of the other proposed fixes seem like kluges to me.
I pushed a patch that fixes it that way but incorporates your regression
test cases.
regards, tom lane
On Thu, Nov 24, 2016 at 3:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.com> writes: >> 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. > > I think that this is the correct fix. There aren't any other callers > in the core code, and even if some third party is calling it, it's > fairly hard to see what usage would be okay with a TABLESPACE clause > being appended some of the time but not all of the time. Also, both > of the other proposed fixes seem like kluges to me. > > I pushed a patch that fixes it that way but incorporates your regression > test cases. This fix looked like a kludge to me, that's why I did not do it :) Any code paths of ALTER TABLE calling DefineIndex won't take advantage of is_alter_table to define the tablespace, so we may be bitten again by the same kind of problems in the future. That won't be an immediate problem as the command strings are generated by this caller, right. Thanks for the commit. -- Michael
Michael Paquier <michael.paquier@gmail.com> writes:
> On Thu, Nov 24, 2016 at 3:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think that this is the correct fix. There aren't any other callers
>> in the core code, and even if some third party is calling it, it's
>> fairly hard to see what usage would be okay with a TABLESPACE clause
>> being appended some of the time but not all of the time. Also, both
>> of the other proposed fixes seem like kluges to me.
> This fix looked like a kludge to me, that's why I did not do it :)
> Any code paths of ALTER TABLE calling DefineIndex won't take advantage
> of is_alter_table to define the tablespace, so we may be bitten again
> by the same kind of problems in the future.
Well, the reason that way seemed like a kluge is that I don't see what
connection is_alter_table has to overriding DefineIndex's normal idea
of which tablespace to use. I think that's at least as likely to
induce new bugs as fix them. For instance, wouldn't it change the
behavior of ALTER TABLE ADD PRIMARY KEY? It seems to me that that
command *should* honor default_tablespace, if there's no tablespace
clause in it.
regards, tom lane
On Thu, Nov 24, 2016 at 7:46 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > For instance, wouldn't it change the behavior of ALTER TABLE ADD > PRIMARY KEY? Good point. I forgot this one. -- Michael
On Thu, Nov 24, 2016 at 8:31 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Nov 24, 2016 at 7:46 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> For instance, wouldn't it change the behavior of ALTER TABLE ADD
>> PRIMARY KEY?
>
> Good point. I forgot this one.
And I just wrote a test case breaking my previous patch, please see
attached. The idea is just to use ADD CONSTRAINT foo { PRIMARY KEY |
UNIQUE } and check if default_tablespace works as expected. Perhaps
that's worth having in the test suite? There is currently no coverage
regarding for index additions done with ALTER TABLE and tablespaces.
--
Michael
Attachment
Michael Paquier <michael.paquier@gmail.com> writes:
> And I just wrote a test case breaking my previous patch, please see
> attached. The idea is just to use ADD CONSTRAINT foo { PRIMARY KEY |
> UNIQUE } and check if default_tablespace works as expected. Perhaps
> that's worth having in the test suite? There is currently no coverage
> regarding for index additions done with ALTER TABLE and tablespaces.
Looks like a good idea, except that I reordered the tests so that
the stanza leaves default_tablespace back at its usual value, so
it won't affect later tests.
regards, tom lane
On Fri, Nov 25, 2016 at 4:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> And I just wrote a test case breaking my previous patch, please see
>> attached. The idea is just to use ADD CONSTRAINT foo { PRIMARY KEY |
>> UNIQUE } and check if default_tablespace works as expected. Perhaps
>> that's worth having in the test suite? There is currently no coverage
>> regarding for index additions done with ALTER TABLE and tablespaces.
>
> Looks like a good idea, except that I reordered the tests so that
> the stanza leaves default_tablespace back at its usual value, so
> it won't affect later tests.
OK, thanks for the commit.
--
Michael