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

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.