Re: [PATCH] Full support for index LP_DEAD hint bits on standby - Mailing list pgsql-hackers

From Antonin Houska
Subject Re: [PATCH] Full support for index LP_DEAD hint bits on standby
Date
Msg-id 40427.1635960741@antos
Whole thread Raw
In response to Re: [PATCH] Full support for index LP_DEAD hint bits on standby  (Michail Nikolaev <michail.nikolaev@gmail.com>)
Responses Re: [PATCH] Full support for index LP_DEAD hint bits on standby
List pgsql-hackers
Michail Nikolaev <michail.nikolaev@gmail.com> wrote:

> > * 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?
>
> The main purpose is to test xactStartedInRecovery logic after the promotion.
> For example -
>     > if (scan->xactStartedInRecovery && !RecoveryInProgress())`

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.


A few more notes regarding the tests:

* 026_standby_index_lp_dead.pl should probably be renamed to
  027_standby_index_lp_dead.pl (026_* was created in the master branch
  recently)


* The test fails, although I do have convigrured the build with
  --enable-tap-tests.

BEGIN failed--compilation aborted at t/026_standby_index_lp_dead.pl line 5.
t/026_standby_index_lp_dead.pl .. Dubious, test returned 2 (wstat 512, 0x200)

I suspect the testing infrastructure changed recently.

* The messages like this

is(hints_num($node_standby_1), qq(10),
    'hints are set on standby1 because FPI but marked as non-safe');

  say that the hints are "marked as non-safe", but the hints_num() function
  does not seem to check that.

* wording:

is(hints_num($node_standby_2), qq(10), 'index hint bits already set on second standby 2');
->
is(hints_num($node_standby_2), qq(10), 'index hint bits already set on standby 2');


And a few more notes on the code:

* There's an extra colon in mask_lp_dead():

bufmask.c:148:38: warning: for loop has empty body [-Wempty-body]
                 offnum = OffsetNumberNext(offnum));
                                                   ^
bufmask.c:148:38: note: put the semicolon on a separate line to silence this warning

* the header comment of heap_hot_search_buffer() still says "*all_dead"
  whereas I'd expect "->all_dead".

  The same for "*page_lsn".

* 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()?

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



pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: Perl warnings when building contrib on RHEL 9 beta
Next
From: James Coleman
Date:
Subject: Re: Parallelize correlated subqueries that execute within each worker