Thread: Extension tracking temp table and causing update failure
Running Postgres 9.1.3. As far as I can tell, when you do an 'alter table' and add a new column with a new default value a temp table is created and tracked by the extension as a new object, but when the 'alter table' statement tries to drop the temp table at the end, the extension complains. I recreated it with a simple test case. I've attached the test extension, but I think the important piece is this line from the update script: alter table t1 add column created_at timestamp with time zone not null default now(); Example output: postgres=# SELECT version(); version -------------------------------------------------------------------------------------------------------------- PostgreSQL 9.1.3 on x86_64-unknown-linux-gnu, compiled by gcc (GCC) 4.6.2 20111027 (Red Hat 4.6.2-1), 64-bit (1 row) postgres=# CREATE SCHEMA myext; CREATE SCHEMA postgres=# CREATE EXTENSION myext WITH SCHEMA myext; CREATE EXTENSION postgres=# \d myext.t1 Table "myext.t1" Column | Type | Modifiers --------+---------+------------------------------------------------------- id | integer | not null default nextval('myext.t1_id_seq'::regclass) foo | text | Indexes: "t1_pkey" PRIMARY KEY, btree (id) postgres=# ALTER EXTENSION myext UPDATE TO '0.2'; ERROR: cannot drop table pg_temp_28906 because extension myext requires it HINT: You can drop extension myext instead. postgres=# DROP EXTENSION myext; DROP EXTENSION postgres=# CREATE EXTENSION myext WITH SCHEMA myext VERSION '0.2'; CREATE EXTENSION postgres=# \d myext.t1 Table "myext.t1" Column | Type | Modifiers ------------+--------------------------+------------------------------------------------------- id | integer | not null default nextval('myext.t1_id_seq'::regclass) foo | text | created_at | timestamp with time zone | not null default now() Indexes: "t1_pkey" PRIMARY KEY, btree (id)
Attachment
Phil Sorber <phil@omniti.com> writes: > Running Postgres 9.1.3. As far as I can tell, when you do an 'alter > table' and add a new column with a new default value a temp table is > created and tracked by the extension as a new object, but when the > 'alter table' statement tries to drop the temp table at the end, the > extension complains. I recreated it with a simple test case. Hm, interesting. ATRewriteTable creates a meant-to-be-transient table to serve as a holder for the new heap until it's done building that. However, recordDependencyOnCurrentExtension doesn't know that the table is meant to be transient and links it to the current extension; so when the table gets dropped a bit later, the dependency code complains. More generally, this means that an extension creation or update script cannot create, use, and drop any sort of transient object, unless it remembers to explicitly unlink the transient object from the extension before the DROP. We could probably kluge something to prevent the table made by make_new_heap from being entered as an extension dependency, or else do an explicit removal of the dependency in finish_heap_swap. But I'm worried that there may be other similar cases, which we'd have to find piecemeal. Instead, I'm tempted to propose that dependency.c explicitly allow drops of objects that belong to the current extension, when an extension is being created or updated. (That is, if we come across a dependency reference to the active extension, we just ignore it. A quick look suggests that this would require only a very small patch.) That would prevent the entire class of problems. It would also have the effect that explicit DROPs of member objects in extension scripts could be done without an explicit ALTER EXTENSION DROP first. I think we'd originally decided that requiring the ALTER was a good safety feature, but is it really more than nanny-ism? The intent of a DROP command seems pretty clear. Thoughts? regards, tom lane
On Tue, Mar 6, 2012 at 5:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Instead, I'm tempted to propose that dependency.c explicitly allow drops > of objects that belong to the current extension, when an extension is > being created or updated. =A0(That is, if we come across a dependency > reference to the active extension, we just ignore it. =A0A quick look > suggests that this would require only a very small patch.) =A0That would > prevent the entire class of problems. > > It would also have the effect that explicit DROPs of member objects in > extension scripts could be done without an explicit ALTER EXTENSION DROP > first. =A0I think we'd originally decided that requiring the ALTER was a > good safety feature, but is it really more than nanny-ism? =A0The intent > of a DROP command seems pretty clear. > > Thoughts? I know you were more looking for Dimitri's answer to this, but I like the i= dea.
Tom Lane <tgl@sss.pgh.pa.us> writes: > However, recordDependencyOnCurrentExtension doesn't know that the table > is meant to be transient and links it to the current extension; so when > the table gets dropped a bit later, the dependency code complains. > > [...] > > Instead, I'm tempted to propose that dependency.c explicitly allow drops > of objects that belong to the current extension, when an extension is > being created or updated. (That is, if we come across a dependency > reference to the active extension, we just ignore it. A quick look > suggests that this would require only a very small patch.) That would > prevent the entire class of problems. Thinking about it, what I think makes sense at the user level is that you can either DROP an extension's object in the extension script or detach it so that it still exists on its own. That means we still need to be able to ALTER EXTENSION =E2=80=A6 DROP and t= hat this operation should be automatically handled when the extension's script contains a DROP command. The way to implement that seems to be exactly what you're saying. (So that view is mostly useful for how to document the behaviour). > It would also have the effect that explicit DROPs of member objects in > extension scripts could be done without an explicit ALTER EXTENSION DROP > first. I think we'd originally decided that requiring the ALTER was a > good safety feature, but is it really more than nanny-ism? The intent > of a DROP command seems pretty clear. What I remember we decided is that you can't DROP any single object of an extension alone, you have to drop the extension wholesale or not at all. So that you first =E2=80=9Cdetach=E2=80=9D the object from the extensi= on then drop it. That makes perfect sense in general but is a useless restriction when executing an extension's script. I consider that bugfix for back branches, by the way (well, 9.1). Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes: > Tom Lane <tgl@sss.pgh.pa.us> writes: >> It would also have the effect that explicit DROPs of member objects in >> extension scripts could be done without an explicit ALTER EXTENSION DROP >> first. I think we'd originally decided that requiring the ALTER was a >> good safety feature, but is it really more than nanny-ism? The intent >> of a DROP command seems pretty clear. > What I remember we decided is that you can't DROP any single object of > an extension alone, you have to drop the extension wholesale or not at > all. So that you first âdetachâ the object from the extension then drop > it. That makes perfect sense in general but is a useless restriction > when executing an extension's script. Actually, on further thought I am not sure we really considered the idea of a DROP in an extension script at all --- it's not something that you would normally expect an extension to want to do for an exported object, and I'm pretty sure none of us thought about transient objects. But anyway, we all seem to agree that this seems like a reasonable fix, so I will look into making it happen. regards, tom lane
I wrote: > But anyway, we all seem to agree that this seems like a reasonable fix, > so I will look into making it happen. The attached proposed patch fixes the symptom Phil reported. However, I think we still have some work to do. I experimented with creating temp tables within an extension upgrade script, and found two interesting misbehaviors that the patch doesn't fix: 1. If you forget to drop the temp table before ending the script, then when the session ends and the temp table is forcibly dropped, the whole extension goes away (following the rule that a forced drop of an extension member makes the whole extension go away). This is mildly annoying, but since not dropping the temp table is a clear bug in an extension script, I think we can live with it. 2. #1 applies only in the typical case where the backend's temp table schema already exists. Otherwise, when we create the pg_temp_N schema it gets listed as one of the extension's objects. Then, when you exit the session, this happens (behind the scenes; it's logged in the postmaster log but not shown to the client): FATAL: cannot drop schema pg_temp_2 because extension myext requires it HINT: You can drop extension myext instead. This is really bad: any temp tables created in this session won't be cleaned out. And the same for any temp tables created in future sessions using the same backend ID slot, since they'll get the identical error on exit. Even worse, if you decide to drop the extension, you might be taking out temp tables that belong to some active session other than your own. Given those hazards and the fact that there's no reasonable way for an extension script to avoid the risk, I think this one is a must-fix. I don't see any easy way around this one other than kluging temp-schema creation to not link the temp schema to the active extension. Which is exactly what I'd not wanted to do in ATRewriteTable. The one bright spot about it is that temp-table schemas are inherently a special case because of their weird creation process, so we could have some comfort that there are probably not other similar bugs lurking. Thoughts, better ideas? regards, tom lane diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index db86262b4f06ec5b78f834eb10fbae45a1e77033..eca064f0cdef7ee6367b76a4da785e29a9cfdbc6 100644 *** a/src/backend/catalog/dependency.c --- b/src/backend/catalog/dependency.c *************** findDependentObjects(const ObjectAddress *** 560,576 **** * another object, or is part of the extension that is the * other object. We have three cases: * ! * 1. At the outermost recursion level, disallow the DROP. (We ! * just ereport here, rather than proceeding, since no other ! * dependencies are likely to be interesting.) However, if ! * the owning object is listed in pendingObjects, just release ! * the caller's lock and return; we'll eventually complete the ! * DROP when we reach that entry in the pending list. */ if (stack == NULL) { char *otherObjDesc; if (pendingObjects && object_address_present(&otherObject, pendingObjects)) { --- 560,580 ---- * another object, or is part of the extension that is the * other object. We have three cases: * ! * 1. At the outermost recursion level, we normally disallow ! * the DROP. (We just ereport here, rather than proceeding, ! * since no other dependencies are likely to be interesting.) ! * However, there are exceptions. */ if (stack == NULL) { char *otherObjDesc; + /* + * Exception 1a: if the owning object is listed in + * pendingObjects, just release the caller's lock and + * return. We'll eventually complete the DROP when we + * reach that entry in the pending list. + */ if (pendingObjects && object_address_present(&otherObject, pendingObjects)) { *************** findDependentObjects(const ObjectAddress *** 579,584 **** --- 583,603 ---- ReleaseDeletionLock(object); return; } + + /* + * Exception 1b: if the owning object is the extension + * currently being created/altered, it's okay to continue + * with the deletion. This allows dropping of an + * extension's objects within the extension's scripts, + * as well as corner cases such as dropping a transient + * object created within such a script. + */ + if (creating_extension && + otherObject.classId == ExtensionRelationId && + otherObject.objectId == CurrentExtensionObject) + break; + + /* No exception applies, so throw the error */ otherObjDesc = getObjectDescription(&otherObject); ereport(ERROR, (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
On Wed, Mar 7, 2012 at 1:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > The attached proposed patch fixes the symptom Phil reported. =A0However, > I think we still have some work to do. =A0I experimented with creating > temp tables within an extension upgrade script, and found two > interesting misbehaviors that the patch doesn't fix: > > 1. If you forget to drop the temp table before ending the script, > then when the session ends and the temp table is forcibly dropped, > the whole extension goes away (following the rule that a forced drop > of an extension member makes the whole extension go away). =A0This is > mildly annoying, but since not dropping the temp table is a clear bug > in an extension script, I think we can live with it. This seems a little bit more than mildly annoying to me. In the CREATE TABLE docs it reads: "Temporary tables are automatically dropped at the end of a session..." On a cursory read through those docs and also section 35.15 on extensions I see no mention of temp tables not being dropped as a bug. It seems like it would be an intuitive thing for people to assume that they wouldn't need to drop them inside of an extension. Also, if I am understanding this correctly, all objects that depend on said extension would also get dropped. So, for example, any tables that have a column with a data type from the extension would also be dropped. And if that table had foreign key references, all those tables would get dropped as well. So on and so forth, you get the idea. This seems like a pretty heavy penalty to pay for what seems like an easy mistake to make.
Phil Sorber <phil@omniti.com> writes: > On Wed, Mar 7, 2012 at 1:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> 1. If you forget to drop the temp table before ending the script, >> then when the session ends and the temp table is forcibly dropped, >> the whole extension goes away (following the rule that a forced drop >> of an extension member makes the whole extension go away). This is >> mildly annoying, but since not dropping the temp table is a clear bug >> in an extension script, I think we can live with it. > This seems a little bit more than mildly annoying to me. Well, I didn't say it wasn't annoying, but it would only be catastrophic if applied to a production database; and it's certainly the sort of thing you'd notice while testing the extension script. Keep in mind that those scripts run as superuser, so they can already do arbitrary amounts of damage to your DB if inadequately tested. If there were a reasonably non-ugly way to deal with the case I'd be willing to put it in, but I don't see one. I think the best we can do is document it. The other case, which only triggers if the script is run in a previously virgin backend ID, scares me a lot more because it's the kind of corner case that could easily escape reasonable testing of an extension script. regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > 1. If you forget to drop the temp table before ending the script, > then when the session ends and the temp table is forcibly dropped, > the whole extension goes away (following the rule that a forced drop > of an extension member makes the whole extension go away). This is > mildly annoying, but since not dropping the temp table is a clear bug > in an extension script, I think we can live with it. Could we force temp tables created in an extension script to be ON COMMIT DROP so that CurrentExtensionObject is still set and your patch kicks in, preventing the DROP cascading? > 2. #1 applies only in the typical case where the backend's temp table > schema already exists. Otherwise, when we create the pg_temp_N schema > it gets listed as one of the extension's objects. Then, when you exit > the session, this happens (behind the scenes; it's logged in the > postmaster log but not shown to the client): > > FATAL: cannot drop schema pg_temp_2 because extension myext requires it > HINT: You can drop extension myext instead. Interesting. > This is really bad: any temp tables created in this session won't be > cleaned out. And the same for any temp tables created in future > sessions using the same backend ID slot, since they'll get the identical > error on exit. Even worse, if you decide to drop the extension, you > might be taking out temp tables that belong to some active session other > than your own. Given those hazards and the fact that there's no > reasonable way for an extension script to avoid the risk, I think this > one is a must-fix. > > I don't see any easy way around this one other than kluging temp-schema > creation to not link the temp schema to the active extension. Which is > exactly what I'd not wanted to do in ATRewriteTable. The one bright spot > about it is that temp-table schemas are inherently a special case > because of their weird creation process, so we could have some comfort > that there are probably not other similar bugs lurking. Yeah protecting against the temp schema special case (can't be registered as a dependency against an extension object) seems good to me, and I'm not able to think about a better answer here. We might want to protect in the same way against temp schema explicitly created by the extension script too (IIRC you can actually do that): it could be just about documentation, but if that's not too much contorting the code it would be better to just ERROR out, I think. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes: > Tom Lane <tgl@sss.pgh.pa.us> writes: >> 1. If you forget to drop the temp table before ending the script, >> then when the session ends and the temp table is forcibly dropped, >> the whole extension goes away (following the rule that a forced drop >> of an extension member makes the whole extension go away). This is >> mildly annoying, but since not dropping the temp table is a clear bug >> in an extension script, I think we can live with it. > Could we force temp tables created in an extension script to be ON > COMMIT DROP so that CurrentExtensionObject is still set and your patch > kicks in, preventing the DROP cascading? Huh, yeah, that might work. It's ugly but at least the ugliness is localized; and there's no obvious reason why such a temp table ought to survive past the end of the script. In a quick look, we can't do it exactly like that because CurrentExtensionObject isn't still set at transaction end; but I think we could call PreCommit_on_commit_actions just before we clear CurrentExtensionObject, so that the drops are done a tad early. > We might want to protect in the same way against temp schema explicitly > created by the extension script too (IIRC you can actually do that): it > could be just about documentation, but if that's not too much contorting > the code it would be better to just ERROR out, I think. Nah, that's not a reasonable thing for an extension script to do IMO. We are not in the business of trying to prevent superusers from thinking of creative ways to break their database. I'll be satisfied if a routine CREATE TEMP TABLE in an extension script is safe. regards, tom lane
I wrote: > Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes: >> Could we force temp tables created in an extension script to be ON >> COMMIT DROP so that CurrentExtensionObject is still set and your patch >> kicks in, preventing the DROP cascading? > Huh, yeah, that might work. It's ugly but at least the ugliness is > localized; and there's no obvious reason why such a temp table ought to > survive past the end of the script. Actually, after I got done hacking the temp-schema case, I realized that preventing temp tables from becoming extension members isn't so ugly as I first thought; in fact, it's pretty much a one-liner, and much cleaner than hacking ON COMMIT DROP. PFA a patch that fixes both of the temp-table issues. regards, tom lane diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index a8653cd49562d95748c8fe38517aa1d0785c9aff..d1d458d7fa485606f1c0e877fdd558b42f6ea273 100644 *** a/src/backend/catalog/heap.c --- b/src/backend/catalog/heap.c *************** AddNewRelationType(const char *typeName, *** 957,966 **** --- 957,968 ---- * reltablespace: OID of tablespace it goes in * relid: OID to assign to new rel, or InvalidOid to select a new OID * reltypeid: OID to assign to rel's rowtype, or InvalidOid to select one + * reloftypeid: if a typed table, OID of underlying type; else InvalidOid * ownerid: OID of new rel's owner * tupdesc: tuple descriptor (source of column definitions) * cooked_constraints: list of precooked check constraints and defaults * relkind: relkind for new rel + * relpersistence: rel's persistence status (permanent, temp, or unlogged) * shared_relation: TRUE if it's to be a shared relation * mapped_relation: TRUE if the relation will use the relfilenode map * oidislocal: TRUE if oid column (if any) should be marked attislocal *************** heap_create_with_catalog(const char *rel *** 1235,1240 **** --- 1237,1246 ---- * should they have any ACL entries. The same applies for extension * dependencies. * + * If it's a temp table, we do not make it an extension member; this + * prevents the unintuitive result that deletion of the temp table at + * session end would make the whole extension go away. + * * Also, skip this in bootstrap mode, since we don't make dependencies * while bootstrapping. */ *************** heap_create_with_catalog(const char *rel *** 1255,1261 **** recordDependencyOnOwner(RelationRelationId, relid, ownerid); ! recordDependencyOnCurrentExtension(&myself, false); if (reloftypeid) { --- 1261,1268 ---- recordDependencyOnOwner(RelationRelationId, relid, ownerid); ! if (relpersistence != RELPERSISTENCE_TEMP) ! recordDependencyOnCurrentExtension(&myself, false); if (reloftypeid) { diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 80d6fc7d0c2c721b43751cfe0d1d7233796f7d31..dc8f8eaf3f3f60f51fd8b59aa78ccfc36e1b23f9 100644 *** a/src/backend/catalog/namespace.c --- b/src/backend/catalog/namespace.c *************** InitTempTableNamespace(void) *** 3558,3564 **** * temp tables. This works because the places that access the temp * namespace for my own backend skip permissions checks on it. */ ! namespaceId = NamespaceCreate(namespaceName, BOOTSTRAP_SUPERUSERID); /* Advance command counter to make namespace visible */ CommandCounterIncrement(); } --- 3558,3565 ---- * temp tables. This works because the places that access the temp * namespace for my own backend skip permissions checks on it. */ ! namespaceId = NamespaceCreate(namespaceName, BOOTSTRAP_SUPERUSERID, ! true); /* Advance command counter to make namespace visible */ CommandCounterIncrement(); } *************** InitTempTableNamespace(void) *** 3582,3588 **** toastspaceId = get_namespace_oid(namespaceName, true); if (!OidIsValid(toastspaceId)) { ! toastspaceId = NamespaceCreate(namespaceName, BOOTSTRAP_SUPERUSERID); /* Advance command counter to make namespace visible */ CommandCounterIncrement(); } --- 3583,3590 ---- toastspaceId = get_namespace_oid(namespaceName, true); if (!OidIsValid(toastspaceId)) { ! toastspaceId = NamespaceCreate(namespaceName, BOOTSTRAP_SUPERUSERID, ! true); /* Advance command counter to make namespace visible */ CommandCounterIncrement(); } diff --git a/src/backend/catalog/pg_namespace.c b/src/backend/catalog/pg_namespace.c index d5ab48a59dc76eb1eff7e6ad4229e87693ba00c2..be812a246c09dbc92f762fb50f1a6fac0e074f73 100644 *** a/src/backend/catalog/pg_namespace.c --- b/src/backend/catalog/pg_namespace.c *************** *** 26,35 **** /* ---------------- * NamespaceCreate * --------------- */ Oid ! NamespaceCreate(const char *nspName, Oid ownerId) { Relation nspdesc; HeapTuple tup; --- 26,43 ---- /* ---------------- * NamespaceCreate + * + * Create a namespace (schema) with the given name and owner OID. + * + * If isTemp is true, this schema is a per-backend schema for holding + * temporary tables. Currently, the only effect of that is to prevent it + * from being linked as a member of any active extension. (If someone + * does CREATE TEMP TABLE in an extension script, we don't want the temp + * schema to become part of the extension.) * --------------- */ Oid ! NamespaceCreate(const char *nspName, Oid ownerId, bool isTemp) { Relation nspdesc; HeapTuple tup; *************** NamespaceCreate(const char *nspName, Oid *** 82,89 **** /* dependency on owner */ recordDependencyOnOwner(NamespaceRelationId, nspoid, ownerId); ! /* dependency on extension */ ! recordDependencyOnCurrentExtension(&myself, false); /* Post creation hook for new schema */ InvokeObjectAccessHook(OAT_POST_CREATE, NamespaceRelationId, nspoid, 0); --- 90,98 ---- /* dependency on owner */ recordDependencyOnOwner(NamespaceRelationId, nspoid, ownerId); ! /* dependency on extension ... but not for magic temp schemas */ ! if (!isTemp) ! recordDependencyOnCurrentExtension(&myself, false); /* Post creation hook for new schema */ InvokeObjectAccessHook(OAT_POST_CREATE, NamespaceRelationId, nspoid, 0); diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c index 916328529a071d94b668f7b789a15cb179e631e4..6745af501d4680ad08d5945cb289a1af3716afb3 100644 *** a/src/backend/commands/schemacmds.c --- b/src/backend/commands/schemacmds.c *************** CreateSchemaCommand(CreateSchemaStmt *st *** 94,100 **** save_sec_context | SECURITY_LOCAL_USERID_CHANGE); /* Create the schema's namespace */ ! namespaceId = NamespaceCreate(schemaName, owner_uid); /* Advance cmd counter to make the namespace visible */ CommandCounterIncrement(); --- 94,100 ---- save_sec_context | SECURITY_LOCAL_USERID_CHANGE); /* Create the schema's namespace */ ! namespaceId = NamespaceCreate(schemaName, owner_uid, false); /* Advance cmd counter to make the namespace visible */ CommandCounterIncrement(); diff --git a/src/include/catalog/pg_namespace.h b/src/include/catalog/pg_namespace.h index aad76a1452aae126c468553bec752182a980ff12..1daba477b409a453bfbe19885fd57455c56245cc 100644 *** a/src/include/catalog/pg_namespace.h --- b/src/include/catalog/pg_namespace.h *************** DESCR("standard public schema"); *** 79,84 **** /* * prototypes for functions in pg_namespace.c */ ! extern Oid NamespaceCreate(const char *nspName, Oid ownerId); #endif /* PG_NAMESPACE_H */ --- 79,84 ---- /* * prototypes for functions in pg_namespace.c */ ! extern Oid NamespaceCreate(const char *nspName, Oid ownerId, bool isTemp); #endif /* PG_NAMESPACE_H */
Tom Lane <tgl@sss.pgh.pa.us> writes: > Actually, after I got done hacking the temp-schema case, I realized that > preventing temp tables from becoming extension members isn't so ugly as > I first thought; in fact, it's pretty much a one-liner, and much cleaner > than hacking ON COMMIT DROP. PFA a patch that fixes both of the > temp-table issues. Awesome. I'm surprised we have so few callers of NamespaceCreate, but that makes sense indeed. Nice localized patch, and I know why I want to upgrade to 9.1.4 sometime later=C2=A0:) (extensions with PL functions that you need to drop when the API changes, you need to alter extension drop function in the script before 9.1.4). Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support