Thread: DROP INDEX CONCURRENTLY is not really concurrency safe & leaves around undroppable indexes

Hi,

Problem 1: concurrency:

Testcase:

Session 1:
CREATE TABLE test_drop_concurrently(id serial primary key, data int);
INSERT INTO test_drop_concurrently(data) SELECT * FROM generate_series(1, 
100000);
CREATE INDEX test_drop_concurrently_data ON test_drop_concurrently(data);
BEGIN;
EXPLAIN ANALYZE SELECT * FROM test_drop_concurrently WHERE data = 34343;
SELECT * FROM test_drop_concurrently WHERE data = 34343;
(1 row)

Session 2:
BEGIN;
SELECT * FROM test_drop_concurrently WHERE data = 34343;

Session 3:
DROP INDEX CONCURRENTLY test_drop_concurrently_data;
(in-progress)

Session 2:
INSERT INTO test_drop_concurrently(data) SELECT * FROM generate_series(1, 
100000);
COMMIT;

Session 1:
SELECT * FROM test_drop_concurrently WHERE data = 34343;
(1 row)
SET enable_bitmapscan = false;
SET enable_indexscan = false;
SELECT * FROM test_drop_concurrently WHERE data = 34343;
(2 rows)

Explanation:
index_drop does:    indexForm->indisvalid = false;    /* make unusable for queries */    indexForm->indisready = false;
  /* make invisible to changes */
 

Setting indisready = false is problematic because that prevents index updates 
which in turn breaks READ COMMITTED semantics. I think there need to be one 
more phase that waits for concurrent users of the index to finish before 
setting indisready = false.


Problem 2: undroppable indexes:

Session 1:
CREATE TABLE test_drop_concurrently(id serial primary key, data int);
CREATE INDEX test_drop_concurrently_data ON test_drop_concurrently(data);
BEGIN;
EXPLAIN ANALYZE SELECT * FROM test_drop_concurrently WHERE data = 34343;

Session 2:
DROP INDEX CONCURRENTLY test_drop_concurrently_data;
<waiting>
^CCancel request sent
ERROR:  canceling statement due to user request

Session 1:
ROLLBACK;
DROP TABLE test_drop_concurrently;
SELECT indexrelid, indrelid, indexrelid::regclass, indrelid::regclass, 
indisvalid, indisready FROM pg_index WHERE indexrelid = 
'test_drop_concurrently_data'::regclass;indexrelid | indrelid |         indexrelid          | indrelid | indisvalid | 
indisready 
------------+----------+-----------------------------+----------+------------+------------     24703 |    24697 |
test_drop_concurrently_data| 24697    | f          | 
 
f
(1 row)

DROP INDEX test_drop_concurrently_data;
ERROR:  could not open relation with OID 24697

Haven't looked at this one at all.

Greetings,

Andres
-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Monday, September 24, 2012 01:27:54 PM Andres Freund wrote:
> Problem 2: undroppable indexes:
> 
> Session 1:
> CREATE TABLE test_drop_concurrently(id serial primary key, data int);
> CREATE INDEX test_drop_concurrently_data ON test_drop_concurrently(data);
> BEGIN;
> EXPLAIN ANALYZE SELECT * FROM test_drop_concurrently WHERE data = 34343;
> 
> Session 2:
> DROP INDEX CONCURRENTLY test_drop_concurrently_data;
> <waiting>
> ^CCancel request sent
> ERROR:  canceling statement due to user request
> 
> Session 1:
> ROLLBACK;
> DROP TABLE test_drop_concurrently;
> SELECT indexrelid, indrelid, indexrelid::regclass, indrelid::regclass,
> indisvalid, indisready FROM pg_index WHERE indexrelid =
> 'test_drop_concurrently_data'::regclass;
>  indexrelid | indrelid |         indexrelid          | indrelid |
> indisvalid | indisready
> ------------+----------+-----------------------------+----------+----------
> --+------------ 24703 |    24697 | test_drop_concurrently_data | 24697    |
> f          | f
> (1 row)
> 
> DROP INDEX test_drop_concurrently_data;
> ERROR:  could not open relation with OID 24697
> 
> Haven't looked at this one at all.
Thats because it has to commit transactions inbetween to make the catalog 
changes visible. Unfortunately at that point it already deleted the 
dependencies in deleteOneObject...


Greetings,

Andres
-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 24 September 2012 06:27, Andres Freund <andres@2ndquadrant.com> wrote:

> Problem 1: concurrency:

> Problem 2: undroppable indexes:

Thanks for posting. I'll think some more before replying.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

Hi,

On Monday, September 24, 2012 01:27:54 PM Andres Freund wrote:
> Hi,
> 
> Problem 1: concurrency:
> 
> Testcase:
> 
> Session 1:
> CREATE TABLE test_drop_concurrently(id serial primary key, data int);
> INSERT INTO test_drop_concurrently(data) SELECT * FROM generate_series(1,
> 100000);
> CREATE INDEX test_drop_concurrently_data ON test_drop_concurrently(data);
> BEGIN;
> EXPLAIN ANALYZE SELECT * FROM test_drop_concurrently WHERE data = 34343;
> SELECT * FROM test_drop_concurrently WHERE data = 34343;
> (1 row)
> 
> Session 2:
> BEGIN;
> SELECT * FROM test_drop_concurrently WHERE data = 34343;
> 
> Session 3:
> DROP INDEX CONCURRENTLY test_drop_concurrently_data;
> (in-progress)
> 
> Session 2:
> INSERT INTO test_drop_concurrently(data) SELECT * FROM generate_series(1,
> 100000);
> COMMIT;
> 
> Session 1:
> SELECT * FROM test_drop_concurrently WHERE data = 34343;
> (1 row)
> SET enable_bitmapscan = false;
> SET enable_indexscan = false;
> SELECT * FROM test_drop_concurrently WHERE data = 34343;
> (2 rows)
> 
> Explanation:
> index_drop does:
>         indexForm->indisvalid = false;    /* make unusable for queries */
>         indexForm->indisready = false;    /* make invisible to changes */
> 
> Setting indisready = false is problematic because that prevents index
> updates which in turn breaks READ COMMITTED semantics. I think there need
> to be one more phase that waits for concurrent users of the index to
> finish before setting indisready = false.
The attached patch fixes this issue. Haven't looked at the other one in detail 
yet.

Greetings,

Andres
-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Monday, September 24, 2012 01:37:59 PM Andres Freund wrote:
> On Monday, September 24, 2012 01:27:54 PM Andres Freund wrote:
> > Problem 2: undroppable indexes:
> > 
> >
> > Session 1:
> > CREATE TABLE test_drop_concurrently(id serial primary key, data int);
> > CREATE INDEX test_drop_concurrently_data ON test_drop_concurrently(data);
> > BEGIN;
> > EXPLAIN ANALYZE SELECT * FROM test_drop_concurrently WHERE data = 34343;
> >
> > 
> >
> > Session 2:
> > DROP INDEX CONCURRENTLY test_drop_concurrently_data;
> > <waiting>
> > ^CCancel request sent
> > ERROR:  canceling statement due to user request
> >
> > 
> >
> > Session 1:
> > ROLLBACK;
> > DROP TABLE test_drop_concurrently;
> > SELECT indexrelid, indrelid, indexrelid::regclass, indrelid::regclass,
> > indisvalid, indisready FROM pg_index WHERE indexrelid =
> > 'test_drop_concurrently_data'::regclass;
> >
> >  indexrelid | indrelid |         indexrelid          | indrelid |
> >
> > indisvalid | indisready
> > ------------+----------+-----------------------------+----------+--------
> > -- --+------------ 24703 |    24697 | test_drop_concurrently_data |
> > 24697    | f          | f
> > (1 row)
> >
> > 
> >
> > DROP INDEX test_drop_concurrently_data;
> > ERROR:  could not open relation with OID 24697
> >
> > 
> >
> > Haven't looked at this one at all.
> 
> Thats because it has to commit transactions inbetween to make the catalog 
> changes visible. Unfortunately at that point it already deleted the 
> dependencies in deleteOneObject...
Seems to be solvable with some minor reshuffling in deleteOneObject. We can 
only perform the deletion of outgoing pg_depend records *after* we have dropped 
the object with doDeletion in the concurrent case. As the actual drop of the 
relation happens in the same transaction that will then go on to drop the 
dependency records that seems to be fine.
I don't see any problems with that right now, will write a patch tomorrow. We 
will see if thats problematic...

Greetings,

Andres
-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Tuesday, September 25, 2012 01:58:31 AM Andres Freund wrote:
> On Monday, September 24, 2012 01:37:59 PM Andres Freund wrote:
> > On Monday, September 24, 2012 01:27:54 PM Andres Freund wrote:
> > > Problem 2: undroppable indexes:
> > > 
> > > 
> > > Session 1:
> > > CREATE TABLE test_drop_concurrently(id serial primary key, data int);
> > > CREATE INDEX test_drop_concurrently_data ON
> > > test_drop_concurrently(data); BEGIN;
> > > EXPLAIN ANALYZE SELECT * FROM test_drop_concurrently WHERE data =
> > > 34343;
> > > 
> > > 
> > > 
> > > Session 2:
> > > DROP INDEX CONCURRENTLY test_drop_concurrently_data;
> > > <waiting>
> > > ^CCancel request sent
> > > ERROR:  canceling statement due to user request
> > > 
> > > 
> > > 
> > > Session 1:
> > > ROLLBACK;
> > > DROP TABLE test_drop_concurrently;
> > > SELECT indexrelid, indrelid, indexrelid::regclass, indrelid::regclass,
> > > indisvalid, indisready FROM pg_index WHERE indexrelid =
> > > 'test_drop_concurrently_data'::regclass;
> > > 
> > >  indexrelid | indrelid |         indexrelid          | indrelid |
> > > 
> > > indisvalid | indisready
> > > ------------+----------+-----------------------------+----------+------
> > > -- -- --+------------ 24703 |    24697 | test_drop_concurrently_data |
> > > 24697    | f          | f
> > > (1 row)
> > > 
> > > 
> > > 
> > > DROP INDEX test_drop_concurrently_data;
> > > ERROR:  could not open relation with OID 24697
> > > 
> > > 
> > > 
> > > Haven't looked at this one at all.
> > 
> > Thats because it has to commit transactions inbetween to make the catalog
> > changes visible. Unfortunately at that point it already deleted the
> > dependencies in deleteOneObject...
> 
> Seems to be solvable with some minor reshuffling in deleteOneObject. We can
> only perform the deletion of outgoing pg_depend records *after* we have
> dropped the object with doDeletion in the concurrent case. As the actual
> drop of the relation happens in the same transaction that will then go on
> to drop the dependency records that seems to be fine.
> I don't see any problems with that right now, will write a patch tomorrow.
> We will see if thats problematic...
Patch attached. Review appreciated, there might be consequences of moving the 
deletion of dependencies I didn't forsee.

Greetings,

Andres
-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

At 2012-09-25 01:46:18 +0200, andres@2ndquadrant.com wrote:
>
> The attached patch fixes this issue. Haven't looked at the other one
> in detail yet.

Here are tests for both bugs. They currently fail with HEAD.

Note that the first test now uses PREPARE instead of the SELECTs in the
original example, which no longer works after commit #96cc18 because of
the re-added AcceptInvalidationMessages calls (archaeology by Andres).

-- Abhijit

On 18 October 2012 12:56, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
> At 2012-09-25 01:46:18 +0200, andres@2ndquadrant.com wrote:
>>
>> The attached patch fixes this issue. Haven't looked at the other one
>> in detail yet.
>
> Here are tests for both bugs. They currently fail with HEAD.
>
> Note that the first test now uses PREPARE instead of the SELECTs in the
> original example, which no longer works after commit #96cc18 because of
> the re-added AcceptInvalidationMessages calls (archaeology by Andres).

Thanks, I'll apply these now.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services