Обсуждение: [bugfix] DISCARD ALL does not release advisory locks
It was brought to my attention that DISCARD ALL does not release advisory locks: http://pgfoundry.org/tracker/index.php?func=detail&aid=1010499&group_id=1000258&atid=983 Thus connection managers / poolers need to do that manually. This obviously means DISCARD ALL needs fixing. Applied patch adds LockReleaseAll(USER_LOCKMETHOD, true) call to DiscardAll(). Should this also be fixed in 8.3? -- marko
Вложения
"Marko Kreen" <markokr@gmail.com> writes: > It was brought to my attention that DISCARD ALL > does not release advisory locks: What is the argument that it should? regards, tom lane
On 11/24/08, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Marko Kreen" <markokr@gmail.com> writes: > > It was brought to my attention that DISCARD ALL > > does not release advisory locks: > > What is the argument that it should? DISCARD ALL is supposed to be used by poolers to reset connection back to startup state to reuse server connection after client disconnect. New client should see no difference between fresh backend and old backend where DISCARD ALL was issued. IOW, DISCARD ALL should be functionally equivalent to backend exit. If user want more explicit control over what resources are released, he should avoid use of DISCARD ALL, instead he should manually pick individual components from the command sequence DISCARD ALL is equivalent to. Eg. when user wants to keep old plans or advisory locks around, he should manually construct command list that resets everything except those. But DISCARD ALL should release everything possible, never should additional commands be needed in addition to it to do full reset. -- marko
On Mon, Nov 24, 2008 at 10:25 AM, Marko Kreen <markokr@gmail.com> wrote: > On 11/24/08, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> "Marko Kreen" <markokr@gmail.com> writes: >> > It was brought to my attention that DISCARD ALL >> > does not release advisory locks: >> >> What is the argument that it should? > > DISCARD ALL is supposed to be used by poolers to reset connection > back to startup state to reuse server connection after client > disconnect. New client should see no difference between fresh > backend and old backend where DISCARD ALL was issued. > > IOW, DISCARD ALL should be functionally equivalent to backend exit. > > If user want more explicit control over what resources are released, > he should avoid use of DISCARD ALL, instead he should manually pick > individual components from the command sequence DISCARD ALL > is equivalent to. Eg. when user wants to keep old plans or > advisory locks around, he should manually construct command list > that resets everything except those. > > But DISCARD ALL should release everything possible, never should additional > commands be needed in addition to it to do full reset. Having done a lot of work with advisory locks, I support this change. Advisory locks are essentially session scoped objects like prepared statements or notifies. It's only natural to clean them up in the same way. That said, I don't think this should be backpatched to 8.3. I'm aware of at least one project that makes heavy use of advisory locks (openads). Since this project and possibly others are probably used in bouncing web environments, you have to be careful with behavior changes like that. People need time...unexpected advisory lock issues can get nasty. If you need the behavior now, just install the patch yourself :-) merlin
"Merlin Moncure" <mmoncure@gmail.com> writes: > On Mon, Nov 24, 2008 at 10:25 AM, Marko Kreen <markokr@gmail.com> wrote: >> IOW, DISCARD ALL should be functionally equivalent to backend exit. > Having done a lot of work with advisory locks, I support this change. > Advisory locks are essentially session scoped objects like prepared > statements or notifies. It's only natural to clean them up in the > same way. > That said, I don't think this should be backpatched to 8.3. Done but not back-patched. regards, tom lane
On 11/26/08, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Merlin Moncure" <mmoncure@gmail.com> writes: > > On Mon, Nov 24, 2008 at 10:25 AM, Marko Kreen <markokr@gmail.com> wrote: > >> IOW, DISCARD ALL should be functionally equivalent to backend exit. > > > Having done a lot of work with advisory locks, I support this change. > > Advisory locks are essentially session scoped objects like prepared > > statements or notifies. It's only natural to clean them up in the > > same way. > > > That said, I don't think this should be backpatched to 8.3. > > Done but not back-patched. I think this should be back-patched as well: - The fact that disconnect will clean up used resources has been always true, thus most clients assume at some level. - DISCARD ALL was new feature in 8.3. It is highly doubtful some adv-locks using project has managed to hard-code dependencyon buggy behaviour of DISCARD. - The bug was reported by regular user who encountered deadlocks on 8.3 because of it. -- marko
On Wed, Nov 26, 2008 at 11:06 AM, Marko Kreen <markokr@gmail.com> wrote: > > I think this should be back-patched as well: > > - The fact that disconnect will clean up used resources has been > always true, thus most clients assume at some level. > > - DISCARD ALL was new feature in 8.3. It is highly doubtful some > adv-locks using project has managed to hard-code dependency on > buggy behaviour of DISCARD. > > - The bug was reported by regular user who encountered deadlocks > on 8.3 because of it. I see your point but there's a pretty high standard for changing existing behavior in bugfix releases. It's just as likely to introduce an application bug as to fix one...suppose the application is using both 'discard all' for prepared statements and advisory locks for other purposes. You could break that application. merlin
> I see your point but there's a pretty high standard for changing > existing behavior in bugfix releases. It's just as likely to introduce > an application bug as to fix one...suppose the application is using > both 'discard all' for prepared statements and advisory locks for > other purposes. You could break that application. I would expect someone to use "DEALLOCATE ALL" for that purpose, but it is true that people don't always do what you expect... ...Robert
"Merlin Moncure" <mmoncure@gmail.com> writes: > On Wed, Nov 26, 2008 at 11:06 AM, Marko Kreen <markokr@gmail.com> wrote: >> >> I think this should be back-patched as well: >> >> - The fact that disconnect will clean up used resources has been >> always true, thus most clients assume at some level. >> >> - DISCARD ALL was new feature in 8.3. It is highly doubtful some >> adv-locks using project has managed to hard-code dependency on >> buggy behaviour of DISCARD. >> >> - The bug was reported by regular user who encountered deadlocks >> on 8.3 because of it. > > I see your point but there's a pretty high standard for changing > existing behavior in bugfix releases. It's just as likely to introduce > an application bug as to fix one...suppose the application is using > both 'discard all' for prepared statements and advisory locks for > other purposes. You could break that application. DISCARD ALL was specifically added in 8.3 for the purpose of connection poolers to be a "big hammer" that exactly emulates a new session. I'm somewhat skeptical that there are any applications using it directly at all, and doubly so that they would be using it and expecting advisory locks to persist. I think the second and third points are pretty convincing. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's On-Demand Production Tuning
On Wed, Nov 26, 2008 at 8:13 PM, Gregory Stark <stark@enterprisedb.com> wrote: > DISCARD ALL was specifically added in 8.3 for the purpose of connection > poolers to be a "big hammer" that exactly emulates a new session. I'm somewhat > skeptical that there are any applications using it directly at all, and doubly > so that they would be using it and expecting advisory locks to persist. > > I think the second and third points are pretty convincing. I kinda agree with you. The only problem IMHO is that we described in the doc exactly what it does and not simply as the big hammer it was supposed to be. See http://www.postgresql.org/docs/8.3/interactive/sql-discard.html . You can't guarantee that nobody checked the doc to see if it discards advisory locks even if it's highly unlikely. That said, I'm more for the backpatching too. -- Guillaume
Gregory Stark <stark@enterprisedb.com> writes: > "Merlin Moncure" <mmoncure@gmail.com> writes: >> I see your point but there's a pretty high standard for changing >> existing behavior in bugfix releases. > DISCARD ALL was specifically added in 8.3 for the purpose of > connection poolers to be a "big hammer" that exactly emulates a new > session. I'm somewhat skeptical that there are any applications using > it directly at all, and doubly so that they would be using it and > expecting advisory locks to persist. The fact that it is new in 8.3 definitely weakens the backwards- compatibility argument. I tend to agree that it's unlikely anyone is really depending on this behavior yet. You could make a case that if we don't backpatch now, we'd actually be *more* likely to create a problem, because the longer that 8.3 is out with the current behavior, the more likely that someone might actually come to depend on it. On balance I'm for back-patching, but wanted to see what others thought. regards, tom lane
On Wed, Nov 26, 2008 at 3:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Gregory Stark <stark@enterprisedb.com> writes: >> "Merlin Moncure" <mmoncure@gmail.com> writes: >>> I see your point but there's a pretty high standard for changing >>> existing behavior in bugfix releases. > >> DISCARD ALL was specifically added in 8.3 for the purpose of >> connection poolers to be a "big hammer" that exactly emulates a new >> session. I'm somewhat skeptical that there are any applications using >> it directly at all, and doubly so that they would be using it and >> expecting advisory locks to persist. > > The fact that it is new in 8.3 definitely weakens the backwards- > compatibility argument. I tend to agree that it's unlikely anyone is > really depending on this behavior yet. You could make a case that if we > don't backpatch now, we'd actually be *more* likely to create a problem, > because the longer that 8.3 is out with the current behavior, the more > likely that someone might actually come to depend on it. > > On balance I'm for back-patching, but wanted to see what others thought. ok...i give :-) merlin
"Guillaume Smet" <guillaume.smet@gmail.com> writes: > I kinda agree with you. The only problem IMHO is that we described in > the doc exactly what it does and not simply as the big hammer it was > supposed to be. See > http://www.postgresql.org/docs/8.3/interactive/sql-discard.html . Well, the *first* sentence there says it "resets the session to its initial state", so it seems to me the intent is clear. But maybe we should alter the second sentence to read, say, "This _currently_ has the same effect as ...", thereby making it clear that this is implementation detail and not the controlling definition. regards, tom lane
On Wed, Nov 26, 2008 at 9:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Well, the *first* sentence there says it "resets the session to its > initial state", so it seems to me the intent is clear. But maybe we > should alter the second sentence to read, say, "This _currently_ has the > same effect as ...", thereby making it clear that this is implementation > detail and not the controlling definition. +1. -- Guillaume