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)
Attachments:
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. (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?
I know you were more looking for Dimitri's answer to this, but I like the idea.
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 … DROP and that
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 “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.
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
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. 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.
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
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 :)
(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