Обсуждение: waiteventset.c XXX

Hi,

Almost at the very end of waiteventset.c, we have a comment saying
should be sure to do a thing that we don't do:

 * NB: be sure to save and restore errno around it.  (That's standard practice
 * in most signal handlers, of course, but we used to omit it in handlers that
 * only set a flag.) XXX
  *

As a minor nitpick, the alignment of the last * here is off by a
column, but the real problem is that a few lines later we come to this
function body:

void
WakeupMyProc(void)
{
#if defined(WAIT_USE_SELF_PIPE)
        if (waiting)
                sendSelfPipeByte();
#else
        if (waiting)
                kill(MyProcPid, SIGURG);
#endif
}

I submit that we should not have both a comment that says we must save
and restore errno and code that doesn't. Maybe the comment is intended
to imply that the caller should be doing such a save and restore, but
if so, then (a) that's not very clear from the wording and (b) why the
XXX?

I think this is the fault of
https://git.postgresql.org/pg/commitdiff/393e0d2314050576c9c039853fdabe7f685a4f47

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 2025-Sep-18, Robert Haas wrote:

> I submit that we should not have both a comment that says we must save
> and restore errno and code that doesn't. Maybe the comment is intended
> to imply that the caller should be doing such a save and restore, but
> if so, then (a) that's not very clear from the wording and (b) why the
> XXX?

I understand the comment as saying that the caller should do
save/restore of errno.  However, looking at several places that are
calling SetLatch() (which is the only caller of this function) it
appears that at least several of them have actually forgotten to do so
(didn't scan all of them but checked a couple and they weren't doing
it).  I think what you have found is not just a bogus comment, but an
actual bug or class of bugs.

I think your contact list has an outdated address for Heikki, or at
least one he hasn't used in a very long time.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)

Hi,

On 2025-09-22 17:23:33 +0200, Álvaro Herrera wrote:
> On 2025-Sep-18, Robert Haas wrote:
> 
> > I submit that we should not have both a comment that says we must save
> > and restore errno and code that doesn't. Maybe the comment is intended
> > to imply that the caller should be doing such a save and restore, but
> > if so, then (a) that's not very clear from the wording and (b) why the
> > XXX?
> 
> I understand the comment as saying that the caller should do
> save/restore of errno.  However, looking at several places that are
> calling SetLatch() (which is the only caller of this function) it
> appears that at least several of them have actually forgotten to do so
> (didn't scan all of them but checked a couple and they weren't doing
> it).  I think what you have found is not just a bogus comment, but an
> actual bug or class of bugs.

These days pqsignal saves/restores errno in a wrapper around signal
handlers. So at least in newer branches this shouldn't be an issue when called
from signal handlers.

Greetings,

Andres Freund