Date: Wed, 11 Nov 2020 09:56:44 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: osumi.takamichi@fujitsu.com, masao.fujii@oss.nttdata.com,
	ashutosh.bapat.oss@gmail.com, tsunakawa.takay@fujitsu.com,
	pgsql-hackers@lists.postgresql.org
Subject: Re: In-place persistance change of a relation
Message-ID: <20201111145644.GY16415@tamriel.snowman.net>
References: <20201111.173317.460890039962481381.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="sgXjL5fWt0rWYmfy"
Content-Disposition: inline
In-Reply-To: <20201111.173317.460890039962481381.horikyota.ntt@gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--sgXjL5fWt0rWYmfy
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Kyotaro Horiguchi (horikyota.ntt@gmail.com) wrote:
> At Tue, 10 Nov 2020 09:33:12 -0500, Stephen Frost <sfrost@snowman.net> wr=
ote in=20
> > * Kyotaro Horiguchi (horikyota.ntt@gmail.com) wrote:
> > > For fuel(?) of the discussion, I tried a very-quick PoC for in-place
> > > ALTER TABLE SET LOGGED/UNLOGGED and resulted as attached. After some
> > > trials of several ways, I drifted to the following way after poking
> > > several ways.
> > >=20
> > > 1. Flip BM_PERMANENT of active buffers
> > > 2. adding/removing init fork
> > > 3. sync files,
> > > 4. Flip pg_class.relpersistence.
> > >=20
> > > It always skips table copy in the SET UNLOGGED case, and only when
> > > wal_level=3Dminimal in the SET LOGGED case.  Crash recovery seems
> > > working by some brief testing by hand.
> >=20
> > Somehow missed that this patch more-or-less does what I was referring to
> > down-thread, but I did want to mention that it looks like it's missing a
> > necessary FlushRelationBuffers() call before the sync, otherwise there
> > could be dirty buffers for the relation that's being set to LOGGED (with
> > wal_level=3Dminimal), which wouldn't be good.  See the comments above
> > smgrimmedsync().
>=20
> Right. Thanks.  However, since SetRelFileNodeBuffersPersistence()
> called just above scans shared buffers so I don't want to just call
> FlushRelationBuffers() separately.  Instead, I added buffer-flush to
> SetRelFileNodeBuffersPersistence().

Maybe I'm missing something, but it sure looks like in the patch that
SetRelFileNodeBuffersPersistence() is being called after the
smgrimmedsync() call, and I don't think you get to just switch the order
of those- the sync is telling the kernel to make sure it's written to
disk, while the FlushBuffer() is just writing it into the kernel but
doesn't provide any guarantee that the data has actually made it to
disk.  We have to FlushBuffer() first, and then call smgrimmedsync().
Perhaps there's a way to avoid having to go through shared buffers
twice, and I generally agreed it'd be good if we could avoid doing so,
but this approach doesn't look like it actually works.

> FWIW this is a revised version of the PoC, which has some known
> problems.
>=20
> - Flipping of Buffer persistence is not WAL-logged nor even be able to
>   be safely roll-backed. (It might be better to drop buffers).

Not sure if it'd be better to drop buffers or not, but figuring out how
to deal with rollback seems pretty important.  How is the persistence
change in the catalog not WAL-logged though..?

> - This version handles indexes but not yet handle toast relatins.

Would need to be fixed, of course.

> - tableAMs are supposed to support this feature. (but I'm not sure
>   it's worth allowing them not to do so).

Seems like they should.

Thanks,

Stephen

--sgXjL5fWt0rWYmfy
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJfq/usAAoJEO1sijiDR2RVYTsQAIt8qZnl3rXk17l5KstyBNav
kDjzcEUgxEhgFGV2bQB8SyNEn3xDXjaTsbUOGTkuaUrB+IJ74VsRItm538g7Y1R/
tgvJt9TrEwJ1tSqe0xtLdJpctAcWuptCCYoQEg3+Q9f0q7QYNWRzO1gioQnvdwgR
BwNdT45ZOwZe6LBn4E89lGvhB0b5Fts6G9TNHZQon6QH0NdaOtDi7KiboyKjxIes
Nwg6l2qrIJCTS6AZJi2U+89dKxBLoPf8kSW4vIaYprl+lA2p3Ql4Zb5c1BYw+AUO
jjAU8leX+jorEy1P6kinpX8LGQnxpZiYU5bZs4L0IpaR+W+Fn1twK0QobXhUzaHe
dBHKubIDNNcyfZlIZmNvrxq42HCsSXLaXwy+UG76gw+kCm2t0QebMjfJjUFV39zZ
nmxikHhXCTWaXufyCR/egZ75bjTB2BMHFTBMlunrf1W6tFRMVycfBsD20dg7BOu3
IEQBaoNRHJ209Y6fz3Zj1LNAeNiSAslif7A4Gcu7Bul8qTghnGRyEhBjc4BU2b4Y
pAv7yUh+YnaWLX/tChWwImt9ahOSLaGlXJ9skpYZ8PM6tNDyXsg9riNB819vsv5u
2EYvXV49wVffM/hinRJEj8+KcqkubL/CaNmrnPle9qI+tpYIXdSRUFsMFUK6s7yu
fv74l1uZH4gQ2DbFxKTw
=UqCV
-----END PGP SIGNATURE-----

--sgXjL5fWt0rWYmfy--