Hello, Antonin.
Thanks for pushing it forward.
> 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.
Yes, it checks - you could see ComputeXidHorizons for details. It is
the main part of the correctness of the whole feature. I added some
details about it to the test.
> * 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)
Done.
> 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)
Fixed.
> * The messages like this
Fixed.
> * There's an extra colon in mask_lp_dead():
Oh, it is a huge error really (the loop was empty) :) Fixed.
> * the header comment of heap_hot_search_buffer() still says "*all_dead"
> whereas I'd expect "->all_dead".
> The same for "*page_lsn".
I was trying to mimic the style of comment (it says about “*tid” from
2007). So, I think it is better to keep it in the same style for the
whole function comment.
> * 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()?
Yes, this way it is looks better. Done. Also, I have added some checks
for “maybe” LSN-related logic to the test.
Thanks a lot,
Michail.