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

Hello,
Antonin.

> My review that started in [1] continues here.
Thanks a lot for the review.

> (Please note that code.patch does not apply to the current master branch.)
Rebased.

> Especially for the problem discussed in [1] it should be
> explained what would happen if kill_prior_tuple_min_lsn was not checked.
Updated README, hope it is better now. Also, added few details related
to the flush of hint bits.

> However I think there's one more case: if heap_hot_search_buffer() considers
> all tuples in the chain to be "surely dead", but
> HeapTupleHeaderAdvanceLatestRemovedXid() skips them all for this reason:
Yes, good catch, missed it.

> I think that the dead tuples produced this way should never be visible on the
> standby (and even if they were, they would change the page LSN so your
> algorithm would treat them correctly) so I see no correctness problem. But it
> might be worth explaining better the meaning of invalid "latest_removed_xid"
> in comments.
Added additional comment.

> but it's not clear to me what "latestRemovedXid" is. If you mean the
> scan->kill_prior_tuple_min_lsn field, you probably need more words to explain
> it.
Hope it is better now.

> should probably be
> /* It is always allowed on primary if *all_dead. */
Fixed.

> As the function is only called if (so->numKilled > 0), I think both
> "killedsomething" and "dirty" variables should always have the same value, so
> one variable should be enough. Assert(so->numKilled) would be appropriate in
> that case.
Fixed, but partly. It is because I have added additional checks for a
long transaction in the case of promoted server.

> "+applying the fill page write."
Fixed.

Updated version in attach.

Thanks a lot,
Michail.