Hello, Antonin.
> I'm trying to continue the review, sorry for the delay. Following are a few
> question about the code:
Thanks for the review :) And sorry for the delay too :)
> * Does the masking need to happen in the AM code, e.g. _bt_killitems()?
> I'd expect that the RmgrData.rm_fpi_mask can do all the work.
RmgrData.rm_fpi_mask clears a single BTP_LP_SAFE_ON_STANDBY bit only
to indicate that hints bit are not safe to be used on standby.
Why do not clear LP_DEAD bits in rm_fpi_mask? There is no sense
because we could get such bits in multiple ways:
* the standby was created from the base backup of the primary
* some pages were changed by pg_rewind
* the standby was updated to the version having this feature (so, old
pages still contains LP_DEAD)
So, AM code needs to know when and why clear LP_DEAD bits if
BTP_LP_SAFE_ON_STANDBY is not set.
Also, the important moment here is pg_memory_barrier() usage.
> * How about modifying rm_mask() instead of introducing rm_fpi_mask()? Perhaps
> a boolean argument can be added to distinguish the purpose of the masking.
I have tried this way but the code was looking dirty and complicated.
Also, the separated fpi_mask provides some semantics to the function.
> * Are you sure it's o.k. to use mask_lp_flags() here? It sets the item flags
> to LP_UNUSED unconditionally, which IMO should only be done by VACUUM.
Oh, good catch. I made mask_lp_dead for this. Also, added such a
situation to the test.
> ** is bufmgr.c the best location for this function?
Moved to indexam.c and made static (is_index_lp_dead_allowed).
> should probably be
> /* It is always allowed on primary if ->all_dead. */
Fixed.
> * comment: XLOG_HEAP2_CLEAN has been renamed to XLOG_HEAP2_PRUNE in PG14.
Fixed.
> * 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())`
> * Unless I miss something, the tests check that the hint bits are not
> propagated from primary (or they are propagated but marked non-safe),
> however there's no test to check that standby does set the hint bits itself.
It is tested on different standby, see
> is(hints_num($node_standby_2), qq(10), 'index hint bits already
set on second standby 2');
Also, I added checks for BTP_LP_SAFE_ON_STANDBY to make sure
everything in the test goes by scenario.
Thanks a lot,
Michail.