Re: [PATCH] Full support for index LP_DEAD hint bits on standby

Michail Nikolaev <michail(dot)nikolaev(at)gmail(dot)com> wrote:

> > * Is the purpose of the repeatable read (RR) snapshot to test that
> > heap_hot_search_buffer() does not set deadness->all_dead if some transaction
> > can still see a tuple of the chain?
>
> The main purpose is to test xactStartedInRecovery logic after the promotion.
> For example -
> > if (scan->xactStartedInRecovery && !RecoveryInProgress())`

I understand that the RR snapshot is used to check the MVCC behaviour, however
this comment seems to indicate that the RR snapshot should also prevent the
standb from setting the hint bits.

# Make sure previous queries not set the hints on standby because
# of RR snapshot

I can imagine that on the primary, but I don't think that the backend that
checks visibility on standby does checks other snapshots/backends. And it
didn't work when I ran the test manually, although I could have missed
something.

A few more notes regarding the tests:

* 026_standby_index_lp_dead.pl should probably be renamed to
027_standby_index_lp_dead.pl (026_* was created in the master branch
recently)

* The test fails, although I do have convigrured the build with
--enable-tap-tests.

BEGIN failed--compilation aborted at t/026_standby_index_lp_dead.pl line 5.
t/026_standby_index_lp_dead.pl .. Dubious, test returned 2 (wstat 512, 0x200)

I suspect the testing infrastructure changed recently.

* The messages like this

is(hints_num($node_standby_1), qq(10),
'hints are set on standby1 because FPI but marked as non-safe');

say that the hints are "marked as non-safe", but the hints_num() function
does not seem to check that.

* wording:

is(hints_num($node_standby_2), qq(10), 'index hint bits already set on second standby 2');
->
is(hints_num($node_standby_2), qq(10), 'index hint bits already set on standby 2');

And a few more notes on the code:

* There's an extra colon in mask_lp_dead():

bufmask.c:148:38: warning: for loop has empty body [-Wempty-body]
offnum = OffsetNumberNext(offnum));
^
bufmask.c:148:38: note: put the semicolon on a separate line to silence this warning

* the header comment of heap_hot_search_buffer() still says "*all_dead"
whereas I'd expect "->all_dead".

The same for "*page_lsn".

* I can see no test for the INDEX_LP_DEAD_OK_MIN_LSN value of the
IndexLpDeadAllowedResult enumeration. Shouldn't there be only two values,
e.g. INDEX_LP_DEAD_OK and INDEX_LP_DEAD_MAYBE_OK ? Or a boolean variable (in
index_fetch_heap()) of the appropriate name, e.g. kill_maybe_allowed, and
rename the function is_index_lp_dead_allowed() to
is_index_lp_dead_maybe_allowed()?

--
Antonin Houska
Web: https://www.cybertec-postgresql.com