Thread: Plan invalidation vs temp sequences
In bug #3662 http://archives.postgresql.org/pgsql-bugs/2007-10/msg00047.php we see that it doesn't work to do nextval('seq') on a temp sequence in a plpgsql function except via EXECUTE, because the sequence OID gets embedded into the cached plan, same as any other temp table. This is to be expected in existing releases, but I notice it still doesn't work in CVS HEAD, which is a tad annoying given the existence of the plan inval mechanism. The reason is that plancache.c only searches the plan's RangeTable for references to a just-invalidated relation; and the argument of nextval() isn't mentioned in the RangeTable (it is in fact just a Const of type regclass). There doesn't seem to be any very nice way to fix this. There is not any existing support mechanism (comparable to query_tree_walker) for scanning whole plan trees, which means that searching a cached plan for regclass Consts is going to involve a chunk of new code no matter how we approach it. We might want to do that someday --- in particular, if we ever try to extend the plan inval mechanism to react to redefinitions of non-table objects, we'd likely need some such thing anyway. I'm disinclined to try to do it for 8.3 though. The use-case for temp sequences seems a bit narrow and there are several workarounds (see followups to bug report), so I'm feeling this is a fix-some-other-day kind of issue. Comments? regards, tom lane
Tom Lane wrote:
> ... We might want to do that someday --- in particular,
> if we ever try to extend the plan inval mechanism to react to
> redefinitions of non-table objects, we'd likely need some such thing
> anyway. I'm disinclined to try to do it for 8.3 though. The use-case
> for temp sequences seems a bit narrow and there are several workarounds
> (see followups to bug report), so I'm feeling this is a
> fix-some-other-day kind of issue.
Agreed. I was a bit worried about this kind of usage:
CREATE OR REPLACE FUNCTION testfunc(val int) RETURNS int AS $$
DECLARE
BEGIN CREATE TEMPORARY SEQUENCE tempseq; CREATE TEMPORARY TABLE inttable (key integer DEFAULT
nextval('tempseq'), data text); INSERT INTO inttable (data) VALUES ('foo'); DROP TABLE inttable; DROP SEQUENCE tempseq;
return1;
END;
$$ LANGUAGE plpgsql;
but that seems to work, because creating/dropping the temp table
triggers the plan invalidation.
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > There doesn't seem to be any very nice way to fix this. There is > not any existing support mechanism (comparable to query_tree_walker) > for scanning whole plan trees, which means that searching a cached plan > for regclass Consts is going to involve a chunk of new code no matter > how we approach it. We might want to do that someday --- in particular, > if we ever try to extend the plan inval mechanism to react to > redefinitions of non-table objects, we'd likely need some such thing > anyway. I'm disinclined to try to do it for 8.3 though. The use-case > for temp sequences seems a bit narrow and there are several workarounds > (see followups to bug report), so I'm feeling this is a > fix-some-other-day kind of issue. Given that sequences are in fact relations is there some way to work around the issue at least in this case by stuffing the sequence's relid someplace which the plan invalldation code can check for it? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark wrote:
> "Tom Lane" <tgl@sss.pgh.pa.us> writes:
>
>> There doesn't seem to be any very nice way to fix this. There is
>> not any existing support mechanism (comparable to query_tree_walker)
>> for scanning whole plan trees, which means that searching a cached plan
>> for regclass Consts is going to involve a chunk of new code no matter
>> how we approach it. We might want to do that someday --- in particular,
>> if we ever try to extend the plan inval mechanism to react to
>> redefinitions of non-table objects, we'd likely need some such thing
>> anyway. I'm disinclined to try to do it for 8.3 though. The use-case
>> for temp sequences seems a bit narrow and there are several workarounds
>> (see followups to bug report), so I'm feeling this is a
>> fix-some-other-day kind of issue.
>
> Given that sequences are in fact relations is there some way to work around
> the issue at least in this case by stuffing the sequence's relid someplace
> which the plan invalldation code can check for it?
Hm... couldn't this be worked around by doing
create or replace function dynamic_oid(text) returning regclass as
'select $1::regclass' language 'pl/pgsql' stable;
And then writing
nextval(dynamic_oid('mysequence')).
I didn't test this, but it it actually works, maybe we should just stick this
into the docs somewhere. It's probably too late to add that function to the
backend, though...
As long as mysequence is really a temporary sequence, this wont even have
searchpath issues I think, because those are always on top of the searchpatch
anyway, aren't they?
greetings, Florian Pflug
"Florian G. Pflug" <fgp@phlo.org> writes:
> Gregory Stark wrote:
>> Given that sequences are in fact relations is there some way to work around
>> the issue at least in this case by stuffing the sequence's relid someplace
>> which the plan invalldation code can check for it?
Well, we *have* the sequence's Oid in the regclass constant, the problem
is the difficulty of digging through the plan tree to find it. I did
consider having the planner extract it and save it aside somewhere, but
there doesn't seem to be any very convenient place to do that, short of
an extra traversal of the query tree, which is pretty annoying/expensive
for data that will probably never be needed for most queries.
> Hm... couldn't this be worked around by doing
> create or replace function dynamic_oid(text) returning regclass as
> 'select $1::regclass' language 'pl/pgsql' stable;
> And then writing
> nextval(dynamic_oid('mysequence')).
The cast-to-text workaround that I suggested does exactly that.
regards, tom lane
I wrote:
> Well, we *have* the sequence's Oid in the regclass constant, the problem
> is the difficulty of digging through the plan tree to find it. I did
> consider having the planner extract it and save it aside somewhere, but
> there doesn't seem to be any very convenient place to do that, short of
> an extra traversal of the query tree, which is pretty annoying/expensive
> for data that will probably never be needed for most queries.
Actually ... now that I've consumed a bit more caffeine, it seems this
could be done relatively cheaply in setrefs.c. We could add a
list-of-relation-OIDs to PlannedStmt, and charge setrefs.c with creating
the list, and simplify plancache.c to just use list_member_oid() instead
of groveling over the rangetable for itself. I'll go try that out.
regards, tom lane
Gregory Stark wrote:
> "Tom Lane" <tgl@sss.pgh.pa.us> writes:
>
>> There doesn't seem to be any very nice way to fix this. There is
>> not any existing support mechanism (comparable to query_tree_walker)
>> for scanning whole plan trees, which means that searching a cached plan
>> for regclass Consts is going to involve a chunk of new code no matter
>> how we approach it. We might want to do that someday --- in particular,
>> if we ever try to extend the plan inval mechanism to react to
>> redefinitions of non-table objects, we'd likely need some such thing
>> anyway. I'm disinclined to try to do it for 8.3 though. The use-case
>> for temp sequences seems a bit narrow and there are several workarounds
>> (see followups to bug report), so I'm feeling this is a
>> fix-some-other-day kind of issue.
>
> Given that sequences are in fact relations is there some way to work around
> the issue at least in this case by stuffing the sequence's relid someplace
> which the plan invalldation code can check for it?
Hm... couldn't this be worked around by doing
create or replace function dynamic_oid(text) returning regclass as
'select $1::regclass' language 'pl/pgsql' stable;
And then writing
nextval(dynamic_oid('mysequence')).
I didn't test this, but it it actually works, maybe we should just stick this
into the docs somewhere. It's probably too late to add that function to the
backend, though...
As long as mysequence is really a temporary sequence, this wont even have
searchpath issues I think, because those are always on top of the searchpatch
anyway, aren't they?
greetings, Florian Pflug
---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to
majordomo@postgresql.orgso that your message can get through to the mailing list cleanly