Re: [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(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Marko Kreen" <markokr(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On 11/24/08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> "Marko Kreen" <markokr(at)gmail(dot)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(at)gmail(dot)com> writes:
> On Mon, Nov 24, 2008 at 10:25 AM, Marko Kreen <markokr(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Merlin Moncure" <mmoncure(at)gmail(dot)com> writes:
> > On Mon, Nov 24, 2008 at 10:25 AM, Marko Kreen <markokr(at)gmail(dot)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 dependency on
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(at)gmail(dot)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(at)gmail(dot)com> writes:

> On Wed, Nov 26, 2008 at 11:06 AM, Marko Kreen <markokr(at)gmail(dot)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(at)enterprisedb(dot)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(at)enterprisedb(dot)com> writes:
> "Merlin Moncure" <mmoncure(at)gmail(dot)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

"Guillaume Smet" <guillaume(dot)smet(at)gmail(dot)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 3:42 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Gregory Stark <stark(at)enterprisedb(dot)com> writes:
>> "Merlin Moncure" <mmoncure(at)gmail(dot)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

On Wed, Nov 26, 2008 at 9:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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