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

Michail Nikolaev <michail.nikolaev@gmail.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