Thread: Another bug(?) turned up by the llvm optimization checker
The commit below, specifically the change mentioned in the last paragraph to fix isLockedRel broke the following comment in addRangeTableEntry:
* If pstate is NULL, we just build an RTE and return it without adding it
* to an rtable list.
In fact isLockedRefname() will seg fault promptly if pstate is NULL. I'm not clear why this behaviour is needed though since as far as I can tell nowhere in the code calls addRangeTableEntry or any of its derivatives with pstate==NULL. I'm inclined to just remove the comment and the test for pstate==NULL lower down but I don't really know what the motivation for this promised behaviour was in the first place so I'm hesitant to do it on my own. * If pstate is NULL, we just build an RTE and return it without adding it
* to an rtable list.
commit 61e532820824504aa92ad93c427722d3fa9c1632
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Tue Oct 27 17:11:18 2009 +0000
Make FOR UPDATE/SHARE in the primary query not propagate into WITH queries;
for example in
WITH w AS (SELECT * FROM foo) SELECT * FROM w, bar ... FOR UPDATE
the FOR UPDATE will now affect bar but not foo. This is more useful and
consistent than the original 8.4 behavior, which tried to propagate FOR UPDATE
into the WITH query but always failed due to assorted implementation
restrictions. Even though we are in process of removing those restrictions,
it seems correct on philosophical grounds to not let the outer query's
FOR UPDATE affect the WITH query.
In passing, fix isLockedRel which frequently got things wrong in
nested-subquery cases: "FOR UPDATE OF foo" applies to an alias foo in the
current query level, not subqueries. This has been broken for a long time,
but it doesn't seem worth back-patching further than 8.4 because the actual
consequences are minimal. At worst the parser would sometimes get
RowShareLock on a relation when it should be AccessShareLock or vice versa.
That would only make a difference if someone were using ExclusiveLock
concurrently, which no standard operation does, and anyway FOR UPDATE
doesn't result in visible changes so it's not clear that the someone would
notice any problem. Between that and the fact that FOR UPDATE barely works
with subqueries at all in existing releases, I'm not excited about worrying
about it.
--
greg
Greg Stark <stark@mit.edu> writes:
> The commit below, specifically the change mentioned in the last paragraph
> to fix isLockedRel broke the following comment in addRangeTableEntry:
> * If pstate is NULL, we just build an RTE and return it without adding it
> * to an rtable list.
> In fact isLockedRefname() will seg fault promptly if pstate is NULL. I'm
> not clear why this behaviour is needed though since as far as I can tell
> nowhere in the code calls addRangeTableEntry or any of its derivatives with
> pstate==NULL. I'm inclined to just remove the comment and the test for
> pstate==NULL lower down but I don't really know what the motivation for
> this promised behaviour was in the first place so I'm hesitant to do it on
> my own.
Hm. I think you are right that this is dead code at the moment, but if
you look around you'll find that there are several places that call
addRangeTableEntry's sister routines with NULL pstate, eg look at the
addRangeTableEntryForRelation calls in view.c. There may once have been
code that called plain addRangeTableEntry that way, or maybe not, but
I'm inclined to think we ought to keep the API contracts similar for
all those functions.
However, that logic doesn't immediately say whether it's better to
make these functions safe against null pstate arguments, or to insist
the callers conjure up a pstate. After a quick look at the number
of pstate uses that have evolved in the addRangeTableEntryForFoo
functions, I'm inclined to think the latter might be the safer
course of action. It's not that hard to make a dummy pstate,
and we could delete the logic for null pstate argument in multiple
places. And not worry about whether the need to defend against
null pstate will propagate into called functions.
In any case, the issue looks bigger than just addRangeTableEntry
itself. Do you want to write up a patch?
regards, tom lane
<div dir="ltr"><div class="gmail_extra"><br /><div class="gmail_quote">On Mon, Nov 11, 2013 at 4:00 AM, Tom Lane <span dir="ltr"><<ahref="mailto:tgl@sss.pgh.pa.us" target="_blank">tgl@sss.pgh.pa.us</a>></span> wrote:<br /><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":3zb" style="overflow:hidden">In any case, the issue looks bigger than just addRangeTableEntry<br /> itself. Do you want to writeup a patch?</div></blockquote></div><br /></div><div class="gmail_extra">I was going to include it in the overflow patchbut I'm now thinking I should make it a separate commit to make sure the change in the contract isn't buried in overflowcheck changes that are supposed to not change functionality. I'll do that.<br /></div><div class="gmail_extra"><brclear="all" /><br />-- <br />greg<br /></div></div>