Thread

1. Think we need major revisions in async.c...

   Tom Lane <tgl@sss.pgh.pa.us> — 1998-09-27T01:05:13Z

   Well, I've spent the last several hours studying how NOTIFY works, and
   I think it's a mess :-(.  We need some significant revisions in there.
   
   I have not been able to trace an exact flow of control that would
   yield the NOTIFY-message-interspersed-in-data result that I saw this
   afternoon.  However, I am pretty certain that I know the basic cause
   of the problem.  The risky aspect of NOTIFY is that very substantial
   amounts of processing (an entire transaction, in fact) are launched from
   a signal handler routine.  As a rule of thumb, doing more than setting a
   flag variable in a signal handler is a big NO NO, because you don't have
   any idea what bit of code you've interrupted or what you can safely
   call.  (I have a strong suspicion, in fact, that the behavior I saw
   arose from the signal interrupt occuring during the backend's fflush()
   of data destined for the frontend at the end of a query cycle.  I dunno
   how re-entrant the stdio library is on my machine, but it seems quite
   possible that it behaves undesirably if one attempts to queue more
   output data from a routine that interrupts fflush()...)
   
   The Postgres code tries to protect itself against this sort of problem
   by having the SIGUSR2 handler not do anything except set a flag if the
   current transaction state says that a transaction is in progress.  This
   is almost good enough, but evidently not quite.  The first issue is that
   some query initialization and finalization code in PostgresMain happens
   outside the StartTransactionCommand...CommitTransactionCommand zone of
   safety.  (In particular, the final fflush of data bound for the
   frontend.)  The second issue is that async.c itself sometimes starts and
   ends transactions, by calling xact.c, and yet xact.c also calls async.c
   as part of transaction commit.  At least two levels of recursion are
   possible in there.  I have not convinced myself whether the bug I
   encountered is due to some subtly incorrect ordering of events in that
   interaction, or whether it's just due to non-reentrancy in stdio.
   But I no longer care.  This code is demonstrably buggy, and it is too
   complex to understand or maintain anyway.  I propose to rewrite it.
   
   I propose that:
   
   1. The link between inbound-notify processing and transaction
   start/commit should be removed.  Having inbound-notify execution called
   from transaction commit is too complicated because that notify execution
   itself may have to induce a transaction start/commit.  Instead, inbound
   notify execution ought to be triggered from the PostgresMain main loop.
   This has the additional advantage that we can have more control over
   just which parts of the main loop allow or disallow notify execution
   directly from the signal handler.  The main loop should now contain
   something like:
   
   	for (;;) {
   
   		ReadyForQuery();	// contains fflush() call
   
   		AllowNotifyInterrupt();
   
   		ReadCommand();
   
   		DisableNotifyInterrupt();
   
   		execute command, including transaction start/commit
   	}
   
   so that the SIGUSR2 interrupt is allowed to do something serious
   only in the *very* tiny window where we are just waiting for or
   reading the next frontend query.  (Since the interrupt has no
   reason to touch the state of stdin, this should be safe enough.)
   An interrupt occuring any other time will just set a flag, which
   will be read by AllowNotifyInterrupt --- if an interrupt has occured
   since the last DisableNotifyInterrupt, then AllowNotifyInterrupt will
   carry out the necessary processing.  Thus, all inbound-notify execution
   effectively happens at the outer loop --- either while waiting for
   frontend input, or just before doing so.
   
   
   2. The innards of NOTIFY execution are unnecessarily complicated and
   inefficient, quite aside from the above problem of controlling interrupt
   safety.  The NOTIFY statement itself (routine Async_Notify) makes one
   pass over the pg_listener relation, writing a 1 into the notification
   field of each tuple with a matching relname.  Then at commit time, we
   have to make another pass over the pg_listener relation to issue signals
   (and/or send a message to our own front end, if we're NOTIFYing
   something we also LISTEN on, which is pretty common in my applications
   anyway).  This is unnecessary processing; worse, if the NOTIFY is done
   early in a long transaction block, we end up holding a write lock on
   pg_listen for a long time.  Since pg_listen is a global resource, this
   can effectively bring all the rest of the backends to a halt!
   
   The right way to do this is for NOTIFY to simply add the target relation
   name to a list of pending notifies (as indeed it does already).  Then
   when we are ready to commit, we scan pg_listen *once*, performing tuple
   updates as needed and also sending signals and/or frontend messages.
   (Note this is for an *outbound* notify, which we do want to have happen
   during transaction commit.  The real problem with the current async.c is
   that it tries to combine outbound and inbound notify code.  Keeping them
   separate will be better.)  This approach takes less processing, and it
   also means that we need not do anything so risky as
   RelationUnsetLockForWrite() to gain performance.  The pg_listen lock
   will be held for only a very short time before we release it as a normal
   part of commit.  Moreover, a self-notify need never modify the pg_listen
   table at all, saving cycles and opening the door for another feature
   (see below).
   
   If the transaction is aborted rather than committed, we need only
   discard the list of pending notify names.  pg_listen is never touched
   in that case.
   
   
   3. While I am at it, I would like to add a feature that I've been
   wanting for a while.  It seems to me that the be_pid field provided in a
   PGnotify message ought not always be your own backend's PID --- what use
   is that?  What it ought to be is the PID of the *signaling* backend, or
   at least of one of the signaling backends other than your own.  I have
   a bunch of applications that all read and write the same tables and let
   each other know about updates by NOTIFY messages.  When one of these
   apps gets a NOTIFY, it has to re-read the table *even if the notify was
   the one it just issued*, and the updates in question are the ones it
   just wrote out.  This is a waste of cycles, but right now there is no
   choice because you cannot tell whether the NOTIFY you get is due
   (solely) to your own NOTIFY or (perhaps in part) to someone else's.
   
   What I'd like to do is to have cross-backend notifies write the
   originating backend's PID into the "notification" field, rather than
   just a "1".  The receiving backend can then send that value to its
   frontend, as opposed to sending its own PID as it will do in the
   self-notify case.  In this way the frontend can unambiguously
   distinguish its own NOTIFY bouncing back from a NOTIFY from another
   client.  (If several other clients send NOTIFY at about the same time,
   you may get only one message with only one of their PIDs, since there
   is only room for one originating PID in pg_listen.  But it will be kept
   distinct from any message resulting from a NOTIFY of your own.)  Note
   that we already made the backend's own PID available to the frontend as
   part of the CANCEL-related changes, so that part of the info is there
   for free.
   
   A more radical solution would be to suppress self-notifies altogether,
   but that is a change in semantics that would likely break some existing
   apps.  I will be satisfied if the frontend can tell the difference
   between a self-notify and an incoming notify.
   
   
   Any objections?  If there's something crucial I've missed in all this,
   better let me know ASAP!
   
   I know that it's a tad late in the beta cycle to be making such significant
   changes, but my applications really need NOTIFY to work and work reliably.
   We have to have this *now*.  I hope y'all will bear with me.
   
   			regards, tom lane
   
   

2. Re: [HACKERS] Think we need major revisions in async.c...

   Bruce Momjian <maillist@candle.pha.pa.us> — 1998-09-27T05:41:32Z

   > What I'd like to do is to have cross-backend notifies write the
   > originating backend's PID into the "notification" field, rather than
   > just a "1".  The receiving backend can then send that value to its
   > frontend, as opposed to sending its own PID as it will do in the
   > self-notify case.  In this way the frontend can unambiguously
   > distinguish its own NOTIFY bouncing back from a NOTIFY from another
   > client.  (If several other clients send NOTIFY at about the same time,
   > you may get only one message with only one of their PIDs, since there
   > is only room for one originating PID in pg_listen.  But it will be kept
   > distinct from any message resulting from a NOTIFY of your own.)  Note
   > that we already made the backend's own PID available to the frontend as
   > part of the CANCEL-related changes, so that part of the info is there
   > for free.
   > 
   > A more radical solution would be to suppress self-notifies altogether,
   > but that is a change in semantics that would likely break some existing
   > apps.  I will be satisfied if the frontend can tell the difference
   > between a self-notify and an incoming notify.
   > 
   > 
   > Any objections?  If there's something crucial I've missed in all this,
   > better let me know ASAP!
   > 
   > I know that it's a tad late in the beta cycle to be making such significant
   > changes, but my applications really need NOTIFY to work and work reliably.
   > We have to have this *now*.  I hope y'all will bear with me.
   
   Sounds good.  You have worked with the Notify code more than anyone
   else, so go ahead.  You are fixing a bug, and that is fine at this
   stage. 
   
   -- 
     Bruce Momjian                        |  http://www.op.net/~candle
     maillist@candle.pha.pa.us            |  (610) 853-3000
     +  If your life is a hard drive,     |  830 Blythe Avenue
     +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
   
   
   

3. Re: [HACKERS] Think we need major revisions in async.c...

   Massimo Dal Zotto <dz@cs.unitn.it> — 1998-09-28T16:39:47Z

   > 
   > Well, I've spent the last several hours studying how NOTIFY works, and
   > I think it's a mess :-(.  We need some significant revisions in there.
   
   I agreee. I tried to improve it but it's really a mess.
   
   > I have not been able to trace an exact flow of control that would
   > yield the NOTIFY-message-interspersed-in-data result that I saw this
   > afternoon.  However, I am pretty certain that I know the basic cause
   > of the problem.  The risky aspect of NOTIFY is that very substantial
   > amounts of processing (an entire transaction, in fact) are launched from
   > a signal handler routine.  As a rule of thumb, doing more than setting a
   > flag variable in a signal handler is a big NO NO, because you don't have
   > any idea what bit of code you've interrupted or what you can safely
   > call.  (I have a strong suspicion, in fact, that the behavior I saw
   > arose from the signal interrupt occuring during the backend's fflush()
   
   I suspected this could happen but don't really know how signals are handled.
   
   > of data destined for the frontend at the end of a query cycle.  I dunno
   > how re-entrant the stdio library is on my machine, but it seems quite
   > possible that it behaves undesirably if one attempts to queue more
   > output data from a routine that interrupts fflush()...)
   > 
   > The Postgres code tries to protect itself against this sort of problem
   > by having the SIGUSR2 handler not do anything except set a flag if the
   > current transaction state says that a transaction is in progress.  This
   > is almost good enough, but evidently not quite.  The first issue is that
   > some query initialization and finalization code in PostgresMain happens
   > outside the StartTransactionCommand...CommitTransactionCommand zone of
   > safety.  (In particular, the final fflush of data bound for the
   > frontend.)  The second issue is that async.c itself sometimes starts and
   > ends transactions, by calling xact.c, and yet xact.c also calls async.c
   > as part of transaction commit.  At least two levels of recursion are
   > possible in there.  I have not convinced myself whether the bug I
   
   I believe that the transaction code can avoid this type of recursion.
   
   > encountered is due to some subtly incorrect ordering of events in that
   > interaction, or whether it's just due to non-reentrancy in stdio.
   > But I no longer care.  This code is demonstrably buggy, and it is too
   > complex to understand or maintain anyway.  I propose to rewrite it.
   > 
   > I propose that:
   > 
   > 1. The link between inbound-notify processing and transaction
   > start/commit should be removed.  Having inbound-notify execution called
   > from transaction commit is too complicated because that notify execution
   > itself may have to induce a transaction start/commit.  Instead, inbound
   > notify execution ought to be triggered from the PostgresMain main loop.
   
   The reason why it is done at commit is that it is the best place where we
   can know if the transaction completed or not. If we move the notify code
   in the main loop we must also keep some flag which tell us if and when the
   notify can be done.
   
   > This has the additional advantage that we can have more control over
   > just which parts of the main loop allow or disallow notify execution
   > directly from the signal handler.  The main loop should now contain
   > something like:
   > 
   > 	for (;;) {
   > 
   > 		ReadyForQuery();	// contains fflush() call
   > 
   > 		AllowNotifyInterrupt();
   > 
   > 		ReadCommand();
   > 
   > 		DisableNotifyInterrupt();
   > 
   > 		execute command, including transaction start/commit
   > 	}
   > 
   > so that the SIGUSR2 interrupt is allowed to do something serious
   > only in the *very* tiny window where we are just waiting for or
   > reading the next frontend query.  (Since the interrupt has no
   > reason to touch the state of stdin, this should be safe enough.)
   > An interrupt occuring any other time will just set a flag, which
   > will be read by AllowNotifyInterrupt --- if an interrupt has occured
   > since the last DisableNotifyInterrupt, then AllowNotifyInterrupt will
   > carry out the necessary processing.  Thus, all inbound-notify execution
   > effectively happens at the outer loop --- either while waiting for
   > frontend input, or just before doing so.
   > 
   > 
   > 2. The innards of NOTIFY execution are unnecessarily complicated and
   > inefficient, quite aside from the above problem of controlling interrupt
   > safety.  The NOTIFY statement itself (routine Async_Notify) makes one
   > pass over the pg_listener relation, writing a 1 into the notification
   > field of each tuple with a matching relname.  Then at commit time, we
   > have to make another pass over the pg_listener relation to issue signals
   > (and/or send a message to our own front end, if we're NOTIFYing
   > something we also LISTEN on, which is pretty common in my applications
   > anyway).  This is unnecessary processing; worse, if the NOTIFY is done
   > early in a long transaction block, we end up holding a write lock on
   > pg_listen for a long time.  Since pg_listen is a global resource, this
   > can effectively bring all the rest of the backends to a halt!
   
   This is the standard behaviour of the notify code. If you set the flag
   notifyunlock in .../data/pg_options the write lock is released immediately
   after updating the table, but I suspect this is the reason of the
   duplicate oids. For this reason I left the flag disabled by default, and
   maybe this explains why you don't get duplicate oids in 6.4, but I have
   enabled it in a production environment without problems at all.
   
   > The right way to do this is for NOTIFY to simply add the target relation
   > name to a list of pending notifies (as indeed it does already).  Then
   > when we are ready to commit, we scan pg_listen *once*, performing tuple
   > updates as needed and also sending signals and/or frontend messages.
   > (Note this is for an *outbound* notify, which we do want to have happen
   > during transaction commit.  The real problem with the current async.c is
   > that it tries to combine outbound and inbound notify code.  Keeping them
   
   The real problem is that inbound notifiy can happen at any moment from the
   signal handler.
   
   > separate will be better.)  This approach takes less processing, and it
   > also means that we need not do anything so risky as
   > RelationUnsetLockForWrite() to gain performance.  The pg_listen lock
   > will be held for only a very short time before we release it as a normal
   > part of commit.  Moreover, a self-notify need never modify the pg_listen
   > table at all, saving cycles and opening the door for another feature
   > (see below).
   
   But in general you must still get the write lock to prevent deadlocks.
   
   > If the transaction is aborted rather than committed, we need only
   > discard the list of pending notify names.  pg_listen is never touched
   > in that case.
   
   This sounds reasonable.
   
   > 3. While I am at it, I would like to add a feature that I've been
   > wanting for a while.  It seems to me that the be_pid field provided in a
   > PGnotify message ought not always be your own backend's PID --- what use
   > is that?  What it ought to be is the PID of the *signaling* backend, or
   > at least of one of the signaling backends other than your own.  I have
   > a bunch of applications that all read and write the same tables and let
   > each other know about updates by NOTIFY messages.  When one of these
   > apps gets a NOTIFY, it has to re-read the table *even if the notify was
   > the one it just issued*, and the updates in question are the ones it
   > just wrote out.  This is a waste of cycles, but right now there is no
   > choice because you cannot tell whether the NOTIFY you get is due
   > (solely) to your own NOTIFY or (perhaps in part) to someone else's.
   
   The problem is that notify has the same semantic of unix signals. You can
   have more notifies for the same table from many backends but you only know
   that at least one has happened.
   What you propose should be some sort of message queuing between backends
   in which each message is notified separately to the other backend In this
   case it would also be useful to pass some arbitrary text to the listening
   backend.
   
   > What I'd like to do is to have cross-backend notifies write the
   > originating backend's PID into the "notification" field, rather than
   > just a "1".  The receiving backend can then send that value to its
   > frontend, as opposed to sending its own PID as it will do in the
   > self-notify case.  In this way the frontend can unambiguously
   > distinguish its own NOTIFY bouncing back from a NOTIFY from another
   > client.  (If several other clients send NOTIFY at about the same time,
   > you may get only one message with only one of their PIDs, since there
   > is only room for one originating PID in pg_listen.  But it will be kept
   > distinct from any message resulting from a NOTIFY of your own.)  Note
   > that we already made the backend's own PID available to the frontend as
   > part of the CANCEL-related changes, so that part of the info is there
   > for free.
   > 
   > A more radical solution would be to suppress self-notifies altogether,
   > but that is a change in semantics that would likely break some existing
   > apps.  I will be satisfied if the frontend can tell the difference
   > between a self-notify and an incoming notify.
   
   I wouldn't change this.
   
   > Any objections?  If there's something crucial I've missed in all this,
   > better let me know ASAP!
   > 
   > I know that it's a tad late in the beta cycle to be making such significant
   > changes, but my applications really need NOTIFY to work and work reliably.
   > We have to have this *now*.  I hope y'all will bear with me.
   > 
   > 			regards, tom lane
   > 
   
   Massimo Dal Zotto
   
   +----------------------------------------------------------------------+
   |  Massimo Dal Zotto                email:  dz@cs.unitn.it             |
   |  Via Marconi, 141                 phone:  ++39-461-534251            |
   |  38057 Pergine Valsugana (TN)     www:  http://www.cs.unitn.it/~dz/  |
   |  Italy                            pgp:  finger dz@tango.cs.unitn.it  |
   +----------------------------------------------------------------------+