Re: RecoveryInProgress() has critical side effects

On Fri, Nov 12, 2021 at 10:23 AM Bossart, Nathan <bossartn(at)amazon(dot)com> wrote:
> 0001 looks reasonable to me.

Cool.

> Unless there's a demonstrable performance benefit from adding the
> branch, my preference is to leave it out. I could be off-base, but it
> seems possible that future changes might end up depending on any side
> effects from this new branch, which is exactly what you're trying to
> fix. Plus, always using the retry path is a good way to test that it
> works as expected.

Here's a new version that does it that way. Any other opinions?

The best thing I could come up with for a test case for this was to
try repeatedly making a new connection and running "SELECT
txid_current()", which will cause just one WAL record to be generated.
Unfortunately that path has overhead from a lot of other causes so I'm
not sure the results are very meaningful, but here they are:

v1: 0.378 ms
v2: 0.391 ms
common base commit (10eae82b2): 0.376 ms

methodology:
for i in `seq 1 1000`; do psql -c '\timing' -c 'select
txid_current();'; done | grep '^Time:' | awk '{total+=$2} END {print
total/NR}'
run twice, discarding the first result, since the very first
connection attempt after starting a new server process is notably
slower

--
Robert Haas
EDB: http://www.enterprisedb.com