Re: [PATCH] lock_timeout and common SIGALRM framework - Mailing list pgsql-hackers

<div class="moz-cite-prefix">2012-06-26 18:12 keltezéssel, Alvaro Herrera írta:<br /></div><blockquote
cite="mid:1340726100-sup-3131@alvh.no-ip.org"type="cite"><pre wrap="">Excerpts from Boszormenyi Zoltan's message of mar
jun26 03:59:06 -0400 2012: 
</pre><blockquote type="cite"><pre wrap="">
2012-06-26 06:59 keltezéssel, Alvaro Herrera írta:
</pre><blockquote type="cite"><pre wrap="">I cleaned up the framework patch a bit.  My version's attached.  Mainly,
returning false for failure in some code paths that are only going to
have the caller elog(FATAL) is rather pointless -- it seems much better
to just have the code itself do the elog(FATAL).  In any case, I
searched for reports of those error messages being reported in the wild
and saw none.
</pre></blockquote><pre wrap="">
OK. The cleanups are always good.

One nitpick, though. Your version doesn't contain the timeout.h
and compilation fails because of it. :-) You might have forgotten
to do "git commit -a".
</pre></blockquote><pre wrap="">
Oops.  Attached. It's pretty much the same you had, except some "bools"
are changed to void.

</pre><blockquote type="cite"><blockquote type="cite"><pre wrap="">There is one thing that still bothers me on this
wholeframework patch, 
but I'm not sure it's easily fixable.  Basically the API to timeout.c is
the whole list of timeouts and their whole behaviors.  If you want to
add a new one, you can't just call into the entry points, you have to
modify timeout.c itself (as well as timeout.h as well as whatever code
it is that you want to add timeouts to).  This may be good enough, but I
don't like it.  I think it boils down to proctimeout.h is cheating.

The interface I would actually like to have is something that lets the
modules trying to get a timeout register the timeout-checking function
as a callback.  API-wise, it would be much cleaner.  However, I'm not
raelly sure that code-wise it would be any cleaner at all.  In fact I
think it'd be rather cumbersome.  However, if you could give that idea
some thought, it'd be swell.
</pre></blockquote><pre wrap="">
Well, I can make the registration interface similar to how LWLocks
are treated, but that doesn't avoid modification of the base_timeouts
array in case a new internal use case arises. Say:

#define USER_TIMEOUTS    4

int    n_timeouts = TIMEOUT_MAX;
static timeout_params base_timeouts[TIMEOUT_MAX + USER_TIMEOUTS];

and register_timeout() adds data after TIMEOUT_MAX.
</pre></blockquote><pre wrap="">
Well, I don't expect that we're going to have many external uses.  My
point about registration is so that internal callers use it as well.  I
was thinking we could do something like xact.c does, which is to have
somewhere in proc.c a call like this:

TimeoutRegister(DEADLOCK_TIMEOUT, 1, true, CheckDeadLock, GetCurrentTimestamp);

at process startup (the magic value 1 is the priority.  Maybe there's a
better way to handle this).  That way, timeout.c or timeout.h do not
need to know anything about proc.c at all; we don't need proctimeout.h
at all.  The only thing it (the timeout module) needs to know is that
there is a symbolic constant named DEADLOCK_TIMEOUT.  Similarly for
statement timeout, etc.

When you call enable_timeout you first have to ensure that
DEADLOCK_TIMEOUT has been registered; and if it's not, die on an
Assert().  That way you ensure that there are no bugs where you try to
enable a timeout that hasn't been registered.</pre></blockquote><br /> Currently, TimeoutName (as an index value) and
"priority"is the same<br /> semantically.<br /><br /> I would also add an Assert into register_timeout() to avoid
doubleregistration<br /> of the same to prevent overriding he callback function pointer. If that's in<br /> place, the
TimeoutNamevalue may still work as is: an index into base_timeouts[].<br /><br /><blockquote
cite="mid:1340726100-sup-3131@alvh.no-ip.org"type="cite"><pre wrap="">(If we later find the need to let external
modulesadd timeouts, which I 
find unlikely, we can have TimeoutRegister return TimeoutName and have
this value be dynamically allocated.  But for now I think that would be
useless complication).

The difference with lwlocks is that each lwlock is just a number.</pre></blockquote><br /> Strictly speaking, just as
TimeoutName.<br/><br /><blockquote cite="mid:1340726100-sup-3131@alvh.no-ip.org" type="cite"><pre wrap="">  The 
lwlock.c module doesn't need to know anything about how each lwlock is
actually used.  It's not the same thing here -- which is why I think it
would be better to have all modules, even the hardcoded ones, use a
registering interface.</pre></blockquote><br /> OK.<br /><br /><blockquote
cite="mid:1340726100-sup-3131@alvh.no-ip.org"type="cite"><pre wrap="">... Now, it could be argued that it would be even
betterto have 
TimeoutRegister not take the TimeoutName argument, and instead generate
it dynamically and pass it back to the caller -- that way you don't need
the enum in timeout.h.</pre></blockquote><br /> This was what I had in mind at first ...<br /><br /><blockquote
cite="mid:1340726100-sup-3131@alvh.no-ip.org"type="cite"><pre wrap="">  The problem I have with that idea is that it 
would force the caller modules to have a global variable to keep track
of that, which seems worse to me.</pre></blockquote><br /> ... and realized this as well.<br /><br /> So, should I keep
theenum TimeoutName? Are global variables for<br /> keeping dynamically assigned values preferred over the enum?<br />
Currentlywe have 5 timeout sources in total, 3 of them are used by<br /> regular backends, the remaining 2 are used by
replicationstandby.<br /> We can have a fixed size array (say with 8 or 16 elements) for future use<br /> and this
wouldbe plenty.<br /><br /> Opinions?<br /><br /><blockquote cite="mid:1340726100-sup-3131@alvh.no-ip.org"
type="cite"><prewrap=""> 

</pre><blockquote type="cite"><blockquote type="cite"><pre wrap="">I haven't looked at the second patch at all yet.
</pre></blockquote><pre wrap="">
This is the whole point of the first patch.
</pre></blockquote><pre wrap="">
I know that.  But I want to get the details of the framework right
before we move on to add more stuff to it.

</pre><blockquote type="cite"><pre wrap="">But as I said above for
registering a new timeout source, it's a new internal use case.
One that touches a lot of places which cannot simply be done
by registering a new timeout source.
</pre></blockquote><pre wrap="">
Sure.  That's going to be the case for any other timeout we want to add
(which is why I think we don't really need dynamic timeouts).

</pre><br /><fieldset class="mimeAttachmentHeader"></fieldset><br /><pre wrap="">
</pre></blockquote><br /><br /><pre class="moz-signature" cols="90">--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: <a class="moz-txt-link-freetext" href="http://www.postgresql-support.de">http://www.postgresql-support.de</a>
<aclass="moz-txt-link-freetext" href="http://www.postgresql.at/">http://www.postgresql.at/</a> 
</pre>