Skip site navigation (1) Skip section navigation (2)

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

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 (view raw or flat)
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

In response to

Responses

pgsql-hackers by date

Next:From: Robert HaasDate: 2010-09-02 20:25:48
Subject: Re: Needs Suggestion
Previous:From: Kevin GrittnerDate: 2010-09-02 19:13:54
Subject: Re: "serializable" in comments and names

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group