From: | Michail Nikolaev <michail(dot)nikolaev(at)gmail(dot)com> |
---|---|
To: | Antonin Houska <ah(at)cybertec(dot)at> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [PATCH] Full support for index LP_DEAD hint bits on standby |
Date: | 2021-09-29 22:09:23 |
Message-ID: | CANtu0ogE9A-MfeBTuBi3RUHUJ-q6c5AN=d-KjnxWRRYpt675wA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello, Antonin.
> I'm trying to continue the review, sorry for the delay. Following are a few
> question about the code:
Thanks for the review :) And sorry for the delay too :)
> * 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.
RmgrData.rm_fpi_mask clears a single BTP_LP_SAFE_ON_STANDBY bit only
to indicate that hints bit are not safe to be used on standby.
Why do not clear LP_DEAD bits in rm_fpi_mask? There is no sense
because we could get such bits in multiple ways:
* the standby was created from the base backup of the primary
* some pages were changed by pg_rewind
* the standby was updated to the version having this feature (so, old
pages still contains LP_DEAD)
So, AM code needs to know when and why clear LP_DEAD bits if
BTP_LP_SAFE_ON_STANDBY is not set.
Also, the important moment here is pg_memory_barrier() usage.
> * 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.
I have tried this way but the code was looking dirty and complicated.
Also, the separated fpi_mask provides some semantics to the function.
> * 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.
Oh, good catch. I made mask_lp_dead for this. Also, added such a
situation to the test.
> ** is bufmgr.c the best location for this function?
Moved to indexam.c and made static (is_index_lp_dead_allowed).
> should probably be
> /* It is always allowed on primary if ->all_dead. */
Fixed.
> * comment: XLOG_HEAP2_CLEAN has been renamed to XLOG_HEAP2_PRUNE in PG14.
Fixed.
> * 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())`
> * 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.
It is tested on different standby, see
> is(hints_num($node_standby_2), qq(10), 'index hint bits already
set on second standby 2');
Also, I added checks for BTP_LP_SAFE_ON_STANDBY to make sure
everything in the test goes by scenario.
Thanks a lot,
Michail.
Attachment | Content-Type | Size |
---|---|---|
v4-0003-doc.patch | application/x-patch | 4.3 KB |
v4-0001-code.patch | application/x-patch | 52.0 KB |
v4-0002-test.patch | application/x-patch | 12.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2021-09-29 22:20:56 | Re: jsonb crash |
Previous Message | Tom Lane | 2021-09-29 21:54:56 | Re: jsonb crash |