Re: Interruptible sleeps (was Re: CommitFest 2009-07: Yay, Kevin! Thanks, reviewers!) - Mailing list pgsql-hackers

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Ok, here's an updated patch with WaitLatchOrSocket that let's you do that.

Minor code review items:

s/There is three/There are three/ in unix_latch.c header comment

The header comment points out the correct usage of ResetLatch, but I
think it should also emphasize that the correct usage of SetLatch is
to set whatever flags indicate work-to-do before SetLatch.

I don't care for the Asserts in InitLatch and InitSharedLatch.
Initialization functions ought not assume that the struct they are
told to initialize contains anything but garbage.  And *especially*
not assume that without documentation.

s/inter-proess/inter-process/, at least 2 places

Does ReleaseLatch() have any actual use-case, and if so what would it be?
I think we could do without it.

The WaitLatch timeout API could use a bit of refinement.  I'd suggest
defining negative timeout as meaning wait forever, so that timeout = 0
can be used for "check but don't wait".  Also, it seems like the
function shouldn't just return void but should return a bool to show
whether it saw the latch set or timed out.  (Yeah, I realize the caller
could look into the latch to find that out, but callers really ought to
treat latches as opaque structs.)

I don't think you have the select-failed logic right in
WaitLatchOrSocket; on EINTR it will suppose that FD_ISSET is a valid
test to make, which I think ain't the case.  Just "continue" around
the loop.

Comment for unix_latch's latch_sigusr1_handler refers to SetEvent,
which is a Windows-ism.

+             * XXX: Is it safe to elog(ERROR) in a signal handler?

No, it isn't.

It seems like both implementations are #include'ing more than they
ought to --- why replication/walsender.h, in particular?
I don't think unix_latch needs spin.h either.

+typedef struct
+{
+    volatile sig_atomic_t    is_set;
+    volatile sig_atomic_t    owner_pid;
+} Latch;

I don't believe it is either sane or portable to declare struct fields
as volatile.  You need to attach the volatile qualifier to the function
arguments instead, eg
extern WaitLatch(volatile Latch *latch, ...)

Also, using sig_atomic_t for owner_pid is entirely not sane.
On many platforms sig_atomic_t is only a byte, and besides
which you have no need for that field to be settable by a
signal handler.
        regards, tom lane