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

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

diff --git a/src/test/recovery/t/027_standby_index_lp_dead.pl b/src/test/recovery/t/027_standby_index_lp_dead.pl
index 5a23eea052..e65000b6bd 100644
--- a/src/test/recovery/t/027_standby_index_lp_dead.pl
+++ b/src/test/recovery/t/027_standby_index_lp_dead.pl
@@ -7,7 +7,7 @@ use PostgreSQL::Test::Utils;
 use Test::More;
 use Config;
 
-plan tests => 29;
+plan tests => 30;
 
 # Initialize primary node
 my $node_primary = PostgreSQL::Test::Cluster->new('primary');
@@ -246,6 +246,7 @@ is(btp_safe_on_stanby($node_new_standby), qq(0), 'hint from FPI');
 
 # Make sure bits are set only if minRecoveryPoint > than index page LSN
 try_to_set_hint_bits($node_new_standby);
+is(hints_num($node_new_standby), qq(11), 'no new index hint bits are set on new standby');
 is(btp_safe_on_stanby($node_new_standby), qq(0), 'hint not marked as standby-safe');
 
 # Issue checkpoint on primary to update minRecoveryPoint on standby