Re: Partitioned tables and [un]loggedness

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.5
> for adding relpersistence inheritance for UNLOGGED. There are a few
> reasons:
>
> * Existing partitioned tables may be marked UNLOGGED, and after upgrade,
> new partitions would be UNLOGGED unless the user discovers that they need
> 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.
>
> * I don't think partitions inheriting persistence is necessarily intuitive.
> 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 explicitly
> 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