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

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

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

Ah, ok. I thought that only KnownAssignedXids is used on standby, but that
would ignore the RR snapshot. It wasn't clear to me when the xmin of the
hot-standby backends is set, now I think it's done by GetSnapshotData().

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

Attached is a proposal for a minor addition that would make sense to me, add
it if you think it's appropriate.

I think I've said enough, changing the status to "ready for committer" :-)

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