Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 29 Aug 2024 15:44:45 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: "David G. Johnston" <david.g.johnston@gmail.com>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Partitioned tables and [un]loggedness
Message-ID: <ZtAY3ZkRVAIp3sxG@paquier.xyz>
References: <ZiiyGFTBNkqcMQi_@paquier.xyz>
 <20240424202640.GB861150@nathanxps13>
 <CAKFQuwY3RTTrp7x4OmEruMmOghdorhq4=eC0ZfG0NM+HnBWFsw@mail.gmail.com>
 <ZimXRKOSHMjQjAy3@paquier.xyz>
 <CAKFQuwZwMOkw-7112u1-gXEhRWuXiZe9otp801Z3dPVvarKdsA@mail.gmail.com>
 <Zimb7xfc79Har77d@paquier.xyz>
 <ZjMte53zQpZNfTWv@paquier.xyz>
 <Zs4-xnNDMEdiKQXo@nathan>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="pIECJp+xGrn1xh1j"
Content-Disposition: inline
In-Reply-To: <Zs4-xnNDMEdiKQXo@nathan>
Archived-At: <https://www.postgresql.org/message-id/ZtAY3ZkRVAIp3sxG%40paquier.xyz>
Precedence: bulk


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

On Tue, Aug 27, 2024 at 04:01:58PM -0500, Nathan Bossart wrote:
> I've been thinking about this thread some more, and I'm finding myself -0=
=2E5
> for adding relpersistence inheritance for UNLOGGED.  There are a few
> reasons:
>=20
> * Existing partitioned tables may be marked UNLOGGED, and after upgrade,
>   new partitions would be UNLOGGED unless the user discovers that they ne=
ed
>   to begin specifying LOGGED or change the persistence of the partitioned
>   table.  I've seen many problems with UNLOGGED over the years, so I am
>   wary about anything that might increase the probability of someone using
>   it accidentally.
>=20
> * I don't think partitions inheriting persistence is necessarily intuitiv=
e.
>   IIUC there's nothing stopping you from having a mix of LOGGED and
>   UNLOGGED partitions, so it's not clear to me why we should assume that
>   users want them to be the same by default.  IMHO UNLOGGED is dangerous
>   enough that we really want users to unambiguously indicate that's what
>   they want.

Okay.  Thanks for sharing an opinion.

> * Inheriting certain persistences (e.g., UNLOGGED) and not others (e.g.,
>   TEMPORARY) seems confusing.  Furthermore, if a partitioned table is
>   marked TEMPORARY, its partitions must also be marked TEMPORARY.  There =
is
>   no such restriction when a partitioned table is marked UNLOGGED.

The reason for temporary tables is different though: we expect
everything to be gone once the backend that created these relations is
gone.  If persistence cocktails were allowed, the worse thing that
could happen would be to have a partitioned table that had temporary
partitions; its catalog state can easily get broken depending on the
DDLs issued on it.  Valid partitioned index that should not be once
the partitions are gone, for example, which would require more exit
logic to flip states in pg_class, pg_index, etc.

> My current thinking is that it would be better to disallow marking
> partitioned tables as LOGGED/UNLOGGED and continue to have users explicit=
ly
> specify what they want for each partition.  It'd still probably be good to
> expand the documentation, but a clear ERROR when trying to set a
> partitioned table as UNLOGGED would hopefully clue folks in.

The addition of the new LOGGED keyword is not required if we limit
ourselves to an error when defining UNLOGGED, so if we drop this
proposal, let's also drop this part entirely and keep DefineRelation()
simpler.  Actually, is really issuing an error the best thing we can
do after so many years allowing this grammar flavor to go through,
even if it is perhaps accidental?  relpersistence is marked correctly
for partitioned tables, it's just useless.  Expanding the
documentation sounds fine to me, one way or the other, to tell what
happens with partitioned tables.

By the way, I was looking at this patch series, and still got annoyed
with the code duplication with ALTER TABLE SET LOGGED/UNLOGGED, so
I've done something about that for now.
--
Michael

--pIECJp+xGrn1xh1j
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmbQGN0ACgkQnvQgOdby
QH2B8w//aIC+WTxVSu8RVUcky6q6uh2+oDnTWZww0BoWqLaFDaphfcYisSwb2DTD
TL3TNY4WhdADKfpruDTs5oULkypqSTXPmG3DcTtKpACrLE/k/BqdRL6YZRRqRYBv
jeqyJR9ONxnJXO207E1ti4FVK4igWeUuGubzus2ROoOOOnokVm++Piuo0zIkdhqw
fDY7YfuZoarEFVmeyBtbXHCQH4K6j5Ac8q9iZfOvwqls5oqrPqNE4vraKUt71oBe
Sx1yJReA23PuckbcBEBrTBM1CduByIcuLdsrJwMRa8k0R918zAure2wHhar9pK5p
fCdnenEEOwFb5uEpyoEm4NBupPxnh8Zn8I0UMgwGra2IHltJBwgTgFPDIykBHDKE
OGUy7ef/6Nmpa1EAgE1KyoM0NI8jODjpGPvY83WoS0m0dkZhcHs/+AkFrcBhYt0M
awB1FSQDxtOIpoR7KdeMMqZnB31pNcLR75gPKurrMKe7lgZU1dtGOBK+YS9Ibuai
TcL0fcT//bRWAXRAdeVY7uugL3p1LtALpZCOOLhVLWrOECzv2++CLnoCk87SC5Gk
qg3sFTuzTgozRvuKR7n9fDuaWRnCDPlld12iHL+7cLAFr5KhA8gNPB0dHmOOkrxD
Zm3SZjGYWj9D3f1D4BpKQ2I7H9buH/0fpQaW4qsEZiBARdcn+V8=
=FpM9
-----END PGP SIGNATURE-----

--pIECJp+xGrn1xh1j--