If we think the patch has a risk of introducing subtle errors, then it
probably can't be justified based on the small performance gains you saw.
But if we think it has little risk, then I think it is justified simply based on cleaner code, and less confusion for people who are tracing a problematic process. If we want it to do a random escalation, it should probably look like a random escalation to the interested observer.
I think it has little risk. If it turns out to be worse for performance, we can always revert it, but I expect it'll be better or a wash, and the results so far seem to bear that out. Another interesting question is whether we should fool with the actual values for minimum and maximum delays, but that's a separate and much harder question, so I think we should just do this for now and call it good.
My thoughts exactly. I wanted to see if David Gould would like to do some testing with it, but there's realy no need to hold off committing for that, I'm not expecting any surprises there. Committed.
Thanks.
Jeff, in the patch you changed the datatype of cur_delay variable from int to long. AFAICS that makes no difference, so I kept it as int. Let me know if there was a reason for that change.
I think my thought process at the time was that since the old code multiplied by "1000L", not "1000" in the expression argument to pg_usleep, I wanted to keep the spirit of the L in place. It seemed safer than trying to prove to myself that it was not necessary.
If int suffices, should the L come out of the defines for MIN_DELAY_USEC and MAX_DELAY_USEC ?