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
signature.asc
Description: PGP signature
