Re: [HACKERS] Another bug(?) turned up by the llvm optimization checker
On Mon, Nov 11, 2013 at 4:00 AM, Tom Lane wrote: > In any case, the issue looks bigger than just addRangeTableEntry > itself. Do you want to write up a patch? > I was going to include it in the overflow patch but I'm now thinking I should make it a separate commit to make sure the change in the contract isn't buried in overflow check changes that are supposed to not change functionality. I'll do that. -- greg
Re: [HACKERS] Another bug(?) turned up by the llvm optimization checker
Greg Stark 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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. commit 61e532820824504aa92ad93c427722d3fa9c1632 Author: Tom Lane Date: Tue Oct 27 17:11:18 2009 + 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
