On Wed, Apr 10, 2024 at 11:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> No, I think you are misreading it, because the assignment is += not =.
> The present coding is
>
> /* increase delay by a random fraction between 1X and 2X */
> status->cur_delay += (int) (status->cur_delay *
> pg_prng_double(&pg_global_prng_state) + 0.5);
>
> which looks fine to me. The +0.5 is so that the conversion to integer
> rounds rather than truncating.
Oh, yeah ... right. But then why does the comment say that it's
increasing the delay between a random fraction between 1X and 2X?
Isn't this a random fraction between 0X and 1X? Or am I still full of
garbage?
> In any case, I concur with Andres: if this behavior is anywhere near
> critical then the right fix is to not be using spinlocks.
It would certainly be interesting to know which spinlocks were at
issue, here. But it also seems to me that it's reasonable to be
unhappy about the possibility of this increasing cur_delay by exactly
0. Storms of spinlock contention can and do happen on real-world
production servers, and trying to guess which spinlocks might be
affected before it happens is difficult. Until we fully eliminate all
spinlocks from our code base, the theoretical possibility of such
storms remains, and if making sure that the increment here is >0
mitigates that, then I think we should. Mind you, I don't think that
the original poster has necessarily provided convincing or complete
evidence to justify a change here, but I find it odd that you and
Andres seem to want to dismiss it out of hand.
--
Robert Haas
EDB: http://www.enterprisedb.com