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

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

> Hello.
>
> Added a check for standby promotion with the long transaction to the
> test (code and docs are unchanged).

I'm trying to continue the review, sorry for the delay. Following are a few
question about the code:

* 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.

Maybe you're concerned about clearing the "LP-safe-on-standby" bits after
promotion, but I wouldn't consider this a problem: once the standby is
allowed to set the hint bits (i.e. minRecoveryPoint is high enough, see
IsIndexLpDeadAllowed() -> XLogNeedsFlush()), promotion shouldn't break
anything because it should not allow minRecoveryPoint to go backwards.

* 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.

* 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. I
think you only need to revert the effect of prior ItemIdMarkDead(), so you
only need to change the status LP_DEAD to LP_NORMAL if the tuple still has
storage. (And maybe add an assertion to ItemIdMarkDead() confirming that
it's only used for LP_NORMAL items?)

As far as I understand, the current code only uses mask_lp_flags() during
WAL consistency check on copies of pages which don't eventually get written
to disk.

* IsIndexLpDeadAllowed()

** is bufmgr.c the best location for this function?

** the header comment should explain the minLsn argument.

** comment

/* It is always allowed on primary if *all_dead. */

should probably be

/* It is always allowed on primary if ->all_dead. */

* comment: XLOG_HEAP2_CLEAN has been renamed to XLOG_HEAP2_PRUNE in PG14.

On regression tests:

* 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? If so, I think the RR snapshot does not
have to be used in the tests because this patch does not really affect the
logic: heap_hot_search_buffer() only sets deadness->all_dead to false, just
like it sets *all_dead in the current code. Besides that,
IsIndexLpDeadAllowed() too can avoid setting of the LP_DEAD flag on an index
tuple (at least until the commit record of the deleting/updating transaction
gets flushed to disk), so it can hide the behaviour of
heap_hot_search_buffer().

* 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.

* I'm also not sure if promotion needs to be tested. What's specific about the
promoted cluster from the point of view of this feature? The only thing I
can think of is clearing of the "LP-safe-on-standby" bits, but, as I said
above, I'm not sure if the tests ever let standby to set those bits before
the promotion.

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