Concurrent CREATE TABLE/DROP SCHEMA leaves inconsistent leftovers

On Wed, Nov 9, 2011 at 4:56 AM, Nikhil Sontakke <nikkhils@gmail.com> wrote:

Consider the following sequence of events:

s1 #> CREATE SCHEMA test_schema;

s1 #> CREATE TABLE test_schema.c1(x int);

Now open another session s2 and via gdb issue a breakpoint on
heap_create_with_catalog() which is called by DefineRelation().

s2 #> CREATE TABLE test_schema.c2(y int);

The above will break on the function. Now issue a drop schema in session s1

s1 #> DROP SCHEMA test_schema CASCADE;
NOTICE:  drop cascades to table test_schema.c1
DROP SCHEMA

Continuing in gdb, also completes the creation of c2 table without any
errors. We are now left with a dangling entry in pg_class along with all the
corresponding data files in our data directory. The problem becomes worse if
c2 was created using a TABLESPACE. Now dropping of that tablespace does not
work at all. Am sure we can come up with myriad such other issues.

Am sure other CREATE commands in this namespace will have similar issues
when faced with a concurrent DROP SCHEMA.

We definitely need some interlocking to handle this. For lack of better
APIs, we could do a LockDatabaseObject() call in AccessShareLock mode on the
namespace and release the same on completion of the creation of the object.

Thoughts?

In general, we've been reluctant to add locking on non-table objects
for reasons of overhead. You can, for example, drop a type or
function while a query is running that depends on it (which is not
true for tables). But I think it is sensible to do it for DDL
commands, which shouldn't be frequent enough for the overhead to
matter much. When I rewrote the comment code for 9.1, I added locking
that works just this way, to prevent pg_description entries from being
orphaned; see the end of get_object_address().

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

We definitely need some interlocking to handle this. For lack of better
APIs, we could do a LockDatabaseObject() call in AccessShareLock mode on

the

namespace and release the same on completion of the creation of the

object.

Thoughts?

In general, we've been reluctant to add locking on non-table objects
for reasons of overhead. You can, for example, drop a type or
function while a query is running that depends on it (which is not
true for tables). But I think it is sensible to do it for DDL
commands, which shouldn't be frequent enough for the overhead to
matter much.

Agreed. Especially if the race condition has non-trivial downsides as
mentioned in the tablespace case.

When I rewrote the comment code for 9.1, I added locking
that works just this way, to prevent pg_description entries from being
orphaned; see the end of get_object_address().

Yeah thanks, that does the object locking. For pre-9.1 versions, we will
need a similar solution. I encountered the issue on 8.3.x..

Regards,
Nikhils

On Wed, Nov 9, 2011 at 9:51 AM, Nikhil Sontakke <nikkhils@gmail.com> wrote:

Yeah thanks, that does the object locking. For pre-9.1 versions, we will
need a similar solution. I encountered the issue on 8.3.x..

I don't think we should back-patch a fix of this type. There is a lot
of cruftiness of this type scattered throughout the code base, and if
we start back-patching all the fixes for it, we're going to end up
destabilizing older branches for little real benefit.

Also, the fix would need to be quite different in older branches. For
example, in the master branch, you can probably fix 90% of the issue
by adjusting dropcmds.c, which now handles drop operations for most
object types. I believe KaiGai Kohei is still working on code which
will allow that code to support drop operations for most of the
remaining object types as well. But in any previous release you would
need scattered fixes all over the code base.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Yeah thanks, that does the object locking. For pre-9.1 versions, we will
need a similar solution. I encountered the issue on 8.3.x..

I don't think we should back-patch a fix of this type. There is a lot
of cruftiness of this type scattered throughout the code base, and if
we start back-patching all the fixes for it, we're going to end up
destabilizing older branches for little real benefit.

Ok, understood.

Thanks and Regards,
Nikhils

Hi,

Ok, understood.

PFA, a patch against git head. We take the AccessShareLock lock on the
schema in DefineRelation now. Note that we do not want to interlock with
other concurrent creations in the schema. We only want to interlock with
deletion activity. So even performance wise this should not be much of an
overhead in case of concurrent DDL operations to the same schema.

Adding this in DefineRelation handles creation of
tables/views/types/sequences. I do not think we need to do stuff in ALTER
commands, because the objects pre-exist and this issue appears to be with
new objects only.

Comments?

Regards,
Nikhils

Nikhil Sontakke <nikkhils@gmail.com> writes:

PFA, a patch against git head. We take the AccessShareLock lock on the
schema in DefineRelation now.

Um ... why would we do this only for tables, and not for creations of
other sorts of objects that belong to schemas?

Also, if we are going to believe that this is a serious problem, what
of ALTER ... SET SCHEMA?

Also, the proposed solution is pretty silly on its face, because it has
not removed the race condition only made the window somewhat narrower.
You would have to acquire the lock as part of the initial schema lookup,
not lock the OID after the fact. And could we please not do something
as silly as translate the OID back to a string and then look up that
string a second time?

(To be clear, I don't particularly believe that this is a problem worthy
of spending code space and cycles on. But if it's deemed to be a
problem, I want to see a solution that's actually watertight.)

regards, tom lane

Um ... why would we do this only for tables, and not for creations of
other sorts of objects that belong to schemas?

Right, we need to do it for other objects like functions etc. too.

Also, if we are going to believe that this is a serious problem, what
of ALTER ... SET SCHEMA?

I admit, I hadn't thought of this.

Also, the proposed solution is pretty silly on its face, because it has
not removed the race condition only made the window somewhat narrower.
You would have to acquire the lock as part of the initial schema lookup,
not lock the OID after the fact. And could we please not do something
as silly as translate the OID back to a string and then look up that
string a second time?

The comment mentions that part is a kluge but that we get to re-use the
existing function because of it. The get_object_address function will bail
out anyways if the schema has vanished from down under and it does lock it
up immediately after it's found to be valid.

(To be clear, I don't particularly believe that this is a problem worthy
of spending code space and cycles on. But if it's deemed to be a
problem, I want to see a solution that's actually watertight.)

Got the message.

Regards,
Nikhils

On Wed, Nov 9, 2011 at 1:56 AM, Nikhil Sontakke <nikkhils@gmail.com> wrote:

Hi,

Consider the following sequence of events:

s1 #> CREATE SCHEMA test_schema;

s1 #> CREATE TABLE test_schema.c1(x int);

Now open another session s2 and via gdb issue a breakpoint on
heap_create_with_catalog() which is called by DefineRelation().

s2 #> CREATE TABLE test_schema.c2(y int);

The above will break on the function. Now issue a drop schema in session s1

s1 #> DROP SCHEMA test_schema CASCADE;
NOTICE:  drop cascades to table test_schema.c1
DROP SCHEMA

Continuing in gdb, also completes the creation of c2 table without any
errors. We are now left with a dangling entry in pg_class along with all the
corresponding data files in our data directory. The problem becomes worse if
c2 was created using a TABLESPACE. Now dropping of that tablespace does not
work at all. Am sure we can come up with myriad such other issues.

Hmm. Does this break pg_dump? I have reported a bug whereby dangling
pg_class entries point to a namespace that has since been dropped in
the past (and has been reported many times before that, even).

The bug report is here, whereby I also aggregate other similar bug
reports that have taken place over a very long period of time:

http://archives.postgresql.org/pgsql-bugs/2011-02/msg00185.php

Given that the schema is successfully dropped, yet another table is
created presumably using this already-resolved schema OID, it seems
like it would run into this...

You could run this query, which should return 0, but may not in your case:

select count(distinct typnamespace) from pg_type where not exists
(select 1 from pg_namespace where oid = pg_type.typnamespace);

--
fdr

Continuing in gdb, also completes the creation of c2 table without any
errors. We are now left with a dangling entry in pg_class along with all

the

corresponding data files in our data directory. The problem becomes

worse if

c2 was created using a TABLESPACE. Now dropping of that tablespace does

not

work at all. Am sure we can come up with myriad such other issues.

Hmm. Does this break pg_dump? I have reported a bug whereby dangling
pg_class entries point to a namespace that has since been dropped in
the past (and has been reported many times before that, even).

Sure does! I just tried it and got:
pg_dump: schema with OID 16384 does not exist

The bug report is here, whereby I also aggregate other similar bug
reports that have taken place over a very long period of time:

http://archives.postgresql.org/pgsql-bugs/2011-02/msg00185.php

I guess we DO need to pay attention to fix this properly now as there are
some reports with 9.x too. And I have just provided a way to reproduce this
reliably too.

Regards,
Nikhils

On Thu, Nov 10, 2011 at 7:00 AM, Nikhil Sontakke <nikkhils@gmail.com> wrote:

PFA, a patch against git head. We take the AccessShareLock lock on the
schema in DefineRelation now. Note that we do not want to interlock with
other concurrent creations in the schema. We only want to interlock with
deletion activity. So even performance wise this should not be much of an
overhead in case of concurrent DDL operations to the same schema.

If all you need to do is lock a schema, you can just call
LockDatabaseObject(NamespaceRelationId, namespace_oid, 0,
AccessShareLock); there's no need to fake up an objectaddress just to
take a lock. But I think that's not really all you need to do,
because somebody could drop the namespace between the time that you
decide what OID to lock and the time you acquire the lock. So I think
you need something like what we did in RangeVarGetRelid(). See
attached patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

If all you need to do is lock a schema, you can just call
LockDatabaseObject(NamespaceRelationId, namespace_oid, 0,
AccessShareLock); there's no need to fake up an objectaddress just to
take a lock. But I think that's not really all you need to do,
because somebody could drop the namespace between the time that you
decide what OID to lock and the time you acquire the lock. So I think
you need something like what we did in RangeVarGetRelid(). See
attached patch.

Thanks Robert. But currently there are very few callers of
RangeVarGetAndCheckCreationNamespace() function. For the sake of
completeness we will have to introduce a call to this function while
creating all other objects too.

Regards,
Nikhils

On Mon, Nov 14, 2011 at 11:48 AM, Nikhil Sontakke
<nikhil.sontakke@enterprisedb.com> wrote:

If all you need to do is lock a schema, you can just call
LockDatabaseObject(NamespaceRelationId, namespace_oid, 0,
AccessShareLock); there's no need to fake up an objectaddress just to
take a lock.  But I think that's not really all you need to do,
because somebody could drop the namespace between the time that you
decide what OID to lock and the time you acquire the lock.  So I think
you need something like what we did in RangeVarGetRelid().  See
attached patch.

Thanks Robert. But currently there are very few callers of
RangeVarGetAndCheckCreationNamespace() function. For the sake of
completeness we will have to introduce a call to this function while
creating all other objects too.

Well, RangeVarGetAndCheckCreationNamespace is only (and can only) be
used for relations. To get similar protection for other object types,
we'd need to add a similar logic elsewhere. I haven't looked at where
it would need to go.

In fact, I think that the technique demonstrated here (which was
pioneered by Noah Misch) is actually quite general, and there are
probably a lot of places where we need to be doing it but currently
are not. So it's probably going to take a while to get this
completely nailed down, but we can keep chipping away at it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

So it's probably going to take a while to get this
completely nailed down, but we can keep chipping away at it.

Agreed. So are you planning to commit this change? Or we want some more
objects to be fixed? Last I looked at this, we will need locking to be done
while creating tables, views, types, sequences, functions, indexes,
extensions, constraints, operators stuff, ts stuff, rules, domains, etc.
that can go into schemas.

So might even make sense to write a schema specific function based on your
patch template to cater in general to schema locking during object creation.

Regards,
Nikhils

On Mon, Nov 14, 2011 at 12:48 PM, Nikhil Sontakke <nikkhils@gmail.com> wrote:

So it's probably going to take a while to get this
completely nailed down, but we can keep chipping away at it.

Agreed. So are you planning to commit this change? Or we want some more
objects to be fixed? Last I looked at this, we will need locking to be done
while creating tables, views, types, sequences, functions, indexes,
extensions, constraints, operators stuff, ts stuff, rules, domains, etc.
that can go into schemas.

<reads the code>

Well, it looks to me like there are three different places that we
need to nail down: RangeVarGetAndCheckCreationNamespace() is used for
relations (except that a few places call RangeVarGetCreationNamespace
directly, which means my previous patch probably needs some tweaking
before commit), QualifiedNameGetCreationNamespace() is used for pretty
much all other schema-qualified objects, and LookupCreationNamespace()
is used for ALTER BLAH SET SCHEMA (which I think has a problem when
you rename an object into a schema that is concurrently being
dropped).

I'm fairly unhappy with the idea of modifying a function that is
described as doing a "get" or "lookup" to have the side effect of
"locking something". So probably some renaming or refactoring is in
order here. It seems like we're duplicating almost identical logic in
an awful lot of places in namespace.c.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Robert Haas's message of lun nov 14 15:56:43 -0300 2011:

Well, it looks to me like there are three different places that we
need to nail down: RangeVarGetAndCheckCreationNamespace() is used for
relations (except that a few places call RangeVarGetCreationNamespace
directly, which means my previous patch probably needs some tweaking
before commit), QualifiedNameGetCreationNamespace() is used for pretty
much all other schema-qualified objects, and LookupCreationNamespace()
is used for ALTER BLAH SET SCHEMA (which I think has a problem when
you rename an object into a schema that is concurrently being
dropped).

I'm fairly unhappy with the idea of modifying a function that is
described as doing a "get" or "lookup" to have the side effect of
"locking something". So probably some renaming or refactoring is in
order here. It seems like we're duplicating almost identical logic in
an awful lot of places in namespace.c.

So RangeVarGetCheckAndLockCreationNamespace(), uh? Pity you can't
stick a comma in there.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Mon, Nov 14, 2011 at 2:26 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Excerpts from Robert Haas's message of lun nov 14 15:56:43 -0300 2011:

Well, it looks to me like there are three different places that we
need to nail down: RangeVarGetAndCheckCreationNamespace() is used for
relations (except that a few places call RangeVarGetCreationNamespace
directly, which means my previous patch probably needs some tweaking
before commit), QualifiedNameGetCreationNamespace() is used for pretty
much all other schema-qualified objects, and LookupCreationNamespace()
is used for ALTER BLAH SET SCHEMA (which I think has a problem when
you rename an object into a schema that is concurrently being
dropped).

I'm fairly unhappy with the idea of modifying a function that is
described as doing a "get" or "lookup" to have the side effect of
"locking something".  So probably some renaming or refactoring is in
order here.  It seems like we're duplicating almost identical logic in
an awful lot of places in namespace.c.

So RangeVarGetCheckAndLockCreationNamespace(), uh?  Pity you can't
stick a comma in there.

Yeah, really. :-)

Actually, I think that one could probably stay as-is. "Check" implies
that there's something else going on besides just a lookup, and we
can't go nuts with it. I'm more concerned about
QualifiedNameGetCreationNamespace() and LookupCreationNamespace().

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Nov 14, 2011 at 12:07 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Nov 14, 2011 at 2:26 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Excerpts from Robert Haas's message of lun nov 14 15:56:43 -0300 2011:

Well, it looks to me like there are three different places that we
need to nail down: RangeVarGetAndCheckCreationNamespace() is used for
relations (except that a few places call RangeVarGetCreationNamespace
directly, which means my previous patch probably needs some tweaking
before commit), QualifiedNameGetCreationNamespace() is used for pretty
much all other schema-qualified objects, and LookupCreationNamespace()
is used for ALTER BLAH SET SCHEMA (which I think has a problem when
you rename an object into a schema that is concurrently being
dropped).

I'm fairly unhappy with the idea of modifying a function that is
described as doing a "get" or "lookup" to have the side effect of
"locking something".  So probably some renaming or refactoring is in
order here.  It seems like we're duplicating almost identical logic in
an awful lot of places in namespace.c.

So RangeVarGetCheckAndLockCreationNamespace(), uh?  Pity you can't
stick a comma in there.

Yeah, really.  :-)

Actually, I think that one could probably stay as-is.  "Check" implies
that there's something else going on besides just a lookup, and we
can't go nuts with it.  I'm more concerned about
QualifiedNameGetCreationNamespace() and LookupCreationNamespace().

Hmm, just to prod this thread: has any fix for this been committed?
After Nikhil confirmed that this bug could cause pg_dump to not be
able to operate without direct catalog surgery I have encountered this
bug (and treated its symptoms in the same manner) twice. I tried to
do my homework in a backbranch, but am not seeing anything beaming out
at me.

Cheers,

--
fdr

On Tue, Nov 29, 2011 at 3:37 AM, Daniel Farina <daniel@heroku.com> wrote:

Hmm, just to prod this thread: has any fix for this been committed?
After Nikhil confirmed that this bug could cause pg_dump to not be
able to operate without direct catalog surgery I have encountered this
bug (and treated its symptoms in the same manner) twice.  I tried to
do my homework in a backbranch, but am not seeing anything beaming out
at me.

I have plans to try to improve this, but it's one of those things that
I care about more than the people who write the checks do, so it
hasn't quite gotten to the top of the priority list yet.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Nov 29, 2011 at 10:10 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Nov 29, 2011 at 3:37 AM, Daniel Farina <daniel@heroku.com> wrote:

Hmm, just to prod this thread: has any fix for this been committed?
After Nikhil confirmed that this bug could cause pg_dump to not be
able to operate without direct catalog surgery I have encountered this
bug (and treated its symptoms in the same manner) twice.  I tried to
do my homework in a backbranch, but am not seeing anything beaming out
at me.

I have plans to try to improve this, but it's one of those things that
I care about more than the people who write the checks do, so it
hasn't quite gotten to the top of the priority list yet.

All right... I have a patch that I think fixes this, at least so far
as relations are concerned. I rewrote
RangeVarGetAndCheckCreationNamespace() extensively, did surgery on its
callers, and then modified CREATE OR REPLACE VIEW and ALTER <relkind>
.. SET SCHEMA to make use of it rather than doing things as they did
before.

So this patch prevents (1) concurrently dropping a schema in which a
new relation is being created, (2) concurrently dropping a schema into
which an existing relation is being moved, and (3) using CREATE OR
REPLACE VIEW to attempt to obtain a lock on a relation you don't own
(the command would eventually fail, of course, but if there were, say,
an outstanding AccessShareLock on the relation, you'd queue up for the
lock and thus prevent any further locks from being granted, despite
your lack of any rights on the target).

It doesn't fix any of the non-relation object types. That would be
nice to do, but I think it's material for a separate patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company