Re: 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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)com> wrote:
> At 2012-09-25 01:46:18 +0200, andres(at)2ndquadrant(dot)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