Date: Thu, 18 Jun 2020 12:16:27 +0200
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Andres Freund <andres@anarazel.de>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [patch] demote
Message-ID: <20200618121627.1f21e376@firost>
In-Reply-To: <20200617181447.tejjp2x2u6y4q5al@alap3.anarazel.de>
References: <20200617174451.222078b4@firost>
	<20200617181447.tejjp2x2u6y4q5al@alap3.anarazel.de>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, 17 Jun 2020 11:14:47 -0700
Andres Freund <andres@anarazel.de> wrote:

> Hi,
>=20
> On 2020-06-17 17:44:51 +0200, Jehan-Guillaume de Rorthais wrote:
> > As Amul sent a patch about "ALTER SYSTEM READ ONLY"[1], with similar fu=
tur
> > objectives than mine, I decided to share the humble patch I am playing =
with
> > to step down an instance from primary to standby status. =20
>=20
> To make sure we are on the same page: What your patch intends to do is
> to leave the server running, but switch from being a primary to
> replicating from another system. Correct?

Yes. The instance status is retrograded from "in production" to "in archive
recovery".

Of course, it will start replicating depending on archive_mode/command and
primary_conninfo setup.

> > Main difference with Amul's patch is that all backends must be disconne=
cted
> > to process with the demote. Either we wait for them to disconnect (smar=
t)
> > or we kill them (fast). This makes life much easier from the code point=
 of
> > view, but much more naive as well. Eg. calling "SELECT pg_demote('fast'=
)"
> > as an admin would kill the session, with no options to wait for the act=
ion
> > to finish, as we do with pg_promote(). Keeping read only session around
> > could probably be achieved using global barrier as Amul did, but without
> > all the complexity related to WAL writes prohibition. =20
>=20
> FWIW just doing that for normal backends isn't sufficient, you also have
> to deal with bgwriter, checkpointer, ... triggering WAL writes (FPWs due
> to hint bits, the checkpoint record, and some more).

In fact, the patch relies on existing code path in the state machine. The
startup process is started when the code enters in PM_NO_CHILDREN state. Th=
is
state is set when =C2=ABThese other guys should be dead already=C2=BB as st=
ated in the
code:

			/* These other guys should be dead already */
			Assert(StartupPID =3D=3D 0);
			Assert(WalReceiverPID =3D=3D 0);
			Assert(BgWriterPID =3D=3D 0);
			Assert(CheckpointerPID =3D=3D 0);
			Assert(WalWriterPID =3D=3D 0);
			Assert(AutoVacPID =3D=3D 0);
			/* syslogger is not considered here */
			pmState =3D PM_NO_CHILDREN;

> > There's still some questions in the current patch. As I wrote, it's an
> > humble patch, a proof of concept, a bit naive.
> >=20
> > Does it worth discussing it and improving it further or do I miss somet=
hing
> > obvious in this design that leads to a dead end? =20
>=20
> I don't think there's a fundamental issue, but I think it needs to deal
> with a lot more things than it does right now. StartupXLOG doesn't
> currently deal correctly with subsystems that are already
> initialized. And your patch doesn't make it so as far as I can tell.

If you are talking about bgwriter, checkpointer, etc, as far as I understand
the current state machine, my patch actually deal with them.

Thank you for your feedback!

I'll study how hard it would be to keep read only backends around during the
demote step.

Regards,