Обсуждение: UNLISTEN bug

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.

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

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. +

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. +

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

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