UNLISTEN bug

Started by Jeff Davisover 15 years ago5 messagesbugs
Jump to latest
#1Jeff Davis
pgsql@j-davis.com

In honor of the very first bug report I sent to postgresql more than 10
years ago regarding UNLISTEN[1]http://www.mail-archive.com/pgsql-bugs@postgresql.org/msg00225.html, I have decided to submit another
UNLISTEN bug (against HEAD):

Session1:

LISTEN foo;
BEGIN;
UNLISTEN foo;

Session2:

NOTIFY foo;

Session1:

SELECT 1;
COMMIT;
SELECT 1;

I seem to recall testing out similar situations during my review of this
patch, but I think the code has changed since that time.

The bug is pretty simple: ProcessIncomingNotify() is called in a tight
loop in EnableNotifyInterrupt() while notifyInterruptOccurred is true.
EnableNotifyInterrupt() is assuming that ProcessIncomingNotify() will
unset it, but it has an early return that's triggered by my UNLISTEN.

Simply adding an additional "notifyInterruptOccurred = 0" in the early
return path (patch attached) seems to work. I added it there rather than
moving the whole line up, because I wasn't sure if it's safe to do that
before DisableCatchupInterrupt().

Regards,
Jeff Davis

[1]: http://www.mail-archive.com/pgsql-bugs@postgresql.org/msg00225.html

Note: I couldn't even find that in our email archive, but thanks to our
new git repo, I found the commit fix by Bruce:

bdeeb4fe8ac22179eb0e12f16486e79c16090a2b

Attachments:

unlisten.patchtext/x-patch; charset=UTF-8; name=unlisten.patchDownload+3-0
#2Bruce Momjian
bruce@momjian.us
In reply to: Jeff Davis (#1)
Re: UNLISTEN bug

Jeff Davis wrote:

In honor of the very first bug report I sent to postgresql more than 10
years ago regarding UNLISTEN[1], I have decided to submit another
UNLISTEN bug (against HEAD):

Session1:

LISTEN foo;
BEGIN;
UNLISTEN foo;

Session2:

NOTIFY foo;

Session1:

SELECT 1;
COMMIT;
SELECT 1;

I seem to recall testing out similar situations during my review of this
patch, but I think the code has changed since that time.

So the problem report is? I tested it and the problem is that the final
SELECT 1 hung. Is that the problem?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

#3Bruce Momjian
bruce@momjian.us
In reply to: Bruce Momjian (#2)
Re: UNLISTEN bug

Bruce Momjian wrote:

Jeff Davis wrote:

In honor of the very first bug report I sent to postgresql more than 10
years ago regarding UNLISTEN[1], I have decided to submit another
UNLISTEN bug (against HEAD):

Session1:

LISTEN foo;
BEGIN;
UNLISTEN foo;

Session2:

NOTIFY foo;

Session1:

SELECT 1;
COMMIT;
SELECT 1;

I seem to recall testing out similar situations during my review of this
patch, but I think the code has changed since that time.

So the problem report is? I tested it and the problem is that the final
SELECT 1 hung. Is that the problem?

To confirm, it was majorly hung. Cancel and kill did not work, pg_ctl
-m fast did not work either. I had to kill -3. Bad. :-(

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

#4Jeff Davis
pgsql@j-davis.com
In reply to: Bruce Momjian (#3)
Re: UNLISTEN bug

On Wed, 2010-09-22 at 13:06 -0400, Bruce Momjian wrote:

To confirm, it was majorly hung. Cancel and kill did not work, pg_ctl
-m fast did not work either. I had to kill -3. Bad. :-(

Yes, that was the problem. I believe the fix is simple, however.

If there had to be a problem with in 9.0.0, but we could pick the
command, I think we would all choose UNLISTEN ;)

As an aside, one of the things that really impressed me about PostgreSQL
so long ago was how quickly you, Tom, and Jan responded to my first
UNLISTEN bug report 10 years ago. It was handled seriously,
professionally, honestly, and openly; even though it seemed like the
most trivial bug that I could imagine at the time.

Regards,
Jeff Davis

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jeff Davis (#1)
Re: UNLISTEN bug

Jeff Davis <pgsql@j-davis.com> writes:

The bug is pretty simple: ProcessIncomingNotify() is called in a tight
loop in EnableNotifyInterrupt() while notifyInterruptOccurred is true.
EnableNotifyInterrupt() is assuming that ProcessIncomingNotify() will
unset it, but it has an early return that's triggered by my UNLISTEN.

Simply adding an additional "notifyInterruptOccurred = 0" in the early
return path (patch attached) seems to work. I added it there rather than
moving the whole line up, because I wasn't sure if it's safe to do that
before DisableCatchupInterrupt().

Hm. AFAICS just moving the flag reset up to the top is safe, and the
cleanest way to fix it.

regards, tom lane