From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> |
Cc: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Interruptible sleeps (was Re: CommitFest 2009-07: Yay, Kevin! Thanks, reviewers!) |
Date: | 2010-09-02 20:13:38 |
Message-ID: | 11257.1283458418@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2010-09-02 20:25:48 | Re: Needs Suggestion |
Previous Message | Kevin Grittner | 2010-09-02 19:13:54 | Re: "serializable" in comments and names |