Re: BUG #17401: REINDEX TABLE CONCURRENTLY creates a race condition on a streaming replica

Hi,

On 2022-04-21 12:54:00 +0900, Michael Paquier wrote:
> Based on the analysis of upthread, I have stuck with logging standby
> snapshots once we are done in WaitForOlderSnapshots() so as we make sure
> that a standby does not attempt to look at the data of any running
> transactions it should wait for and the log of AELs at the end of
> WaitForLockersMultiple() to avoid the access of the lockers too early.

Why is this necessary? The comments in WaitForOlderSnapshots() don't really
explain it. This is pretty darn expensive.


> amcheck could be made more robust here, by calling
> RelationGetIndexList() after opening the Relation of the parent to
> check if it is still listed in the returned list as it would look at
> the relcache and discard any invalid indexes on the way.

That seems like a weird approach. Why can't the check just be done on the
relcache entry of the index itself? If that doesn't work, something is still
broken in cache invalidation.


> diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
> index cd30f15eba..704d8f3744 100644
> --- a/src/backend/commands/indexcmds.c
> +++ b/src/backend/commands/indexcmds.c
> @@ -475,6 +475,16 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
>          if (progress)
>              pgstat_progress_update_param(PROGRESS_WAITFOR_DONE, i + 1);
>      }
> +
> +    /*
> +     * Take a snapshot of running transactions and write this to WAL.  With
> +     * this information, a standby is able to know that it should not access
> +     * too soon any information we expect to wait for with a concurrent build.
> +     *
> +     * Skip the log of this information if disabled.
> +     */
> +    if (XLogStandbyInfoActive())
> +        LogStandbySnapshot();
>  }

"should not access too soon" - no idea what that means / guarantees.

The "Skip the log" bit is redundant with the code.


Greetings,

Andres Freund