From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
To: hlinnaka@iki.fi
Cc: pgsql-hackers@postgresql.org
Cc: sfrost@snowman.net
Cc: alexk@hintbits.com
Cc: michael.paquier@gmail.com
Subject: Re: pg_rewind race condition just after promotion
Date: Tue, 08 Dec 2020 13:45:33 +0900 (JST)
Message-ID: <20201208.134533.202575181178574697.horikyota.ntt@gmail.com> (raw)
In-Reply-To: <9f568c97-87fe-a716-bd39-65299b8a60f4@iki.fi>
References: <9f568c97-87fe-a716-bd39-65299b8a60f4@iki.fi>
At Mon, 7 Dec 2020 20:13:25 +0200, Heikki Linnakangas <hlinnaka@iki.fi> wrote in
> There's a race condition between the checkpoint at promotion and
> pg_rewind. When a server is promoted, the startup process writes an
> end-of-recovery checkpoint that includes the new TLI, and the server
> is immediate opened for business. The startup process requests the
> checkpointer process to perform a checkpoint, but it can take a few
> seconds or more to complete. If you run pg_rewind, using the just
> promoted server as the source, pg_rewind will think that the server is
> still on the old timeline, because it only looks at TLI in the control
> file's copy of the checkpoint record. That's not updated until the
> checkpoint is finished.
>
> This isn't a new issue. Stephen Frost first reported it back 2015
> [1]. Back then, it was deemed just a small annoyance, and we just
> worked around it in the tests by issuing a checkpoint command after
> promotion, to wait for the checkpoint to finish. I just ran into it
> again today, with the new pg_rewind test, and silenced it in the
> similar way.
I (or we) faced that and avoided that by checking for history file on
the primary.
> I think we should fix this properly. I'm not sure if it can lead to a
> broken cluster, but at least it can cause pg_rewind to fail
> unnecessarily and in a user-unfriendly way. But this is actually
> pretty simple to fix. pg_rewind looks at the control file to find out
> the timeline the server is on. When promotion happens, the startup
> process updates minRecoveryPoint and minRecoveryPointTLI fields in the
> control file. We just need to read it from there. Patch attached.
Looks fine to me. A bit concerned about making sourceHistory
needlessly file-local but on the other hand unifying sourceHistory and
targetHistory looks better.
For the test part, that change doesn't necessariry catch the failure
of the current version, but I *believe* the prevous code is the result
of an actual failure in the past so the test probablistically (or
dependently on platforms?) hits the failure if it happned.
> I think we should also backpatch this. Back in 2015, we decided that
> we can live with this, but it's always been a bit bogus, and seems
> simple enough to fix.
I don't think this changes any successful behavior and it just saves
the failure case so +1 for back-patching.
> Thoughts?
>
> [1]
> https://www.postgresql.org/message-id/20150428180253.GU30322%40tamriel.snowman.net
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: horikyota.ntt@gmail.com, hlinnaka@iki.fi, sfrost@snowman.net, alexk@hintbits.com, michael.paquier@gmail.com
Subject: Re: pg_rewind race condition just after promotion
In-Reply-To: <20201208.134533.202575181178574697.horikyota.ntt@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox