Date: Tue, 27 Aug 2024 16:01:58 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
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: <Zs4-xnNDMEdiKQXo@nathan>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZjMte53zQpZNfTWv@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/Zs4-xnNDMEdiKQXo%40nathan>
Precedence: bulk

On Thu, May 02, 2024 at 03:06:51PM +0900, Michael Paquier wrote:
> The case of a temporary persistence is actually *very* tricky.  The
> namespace, where the relation is created, is guessed and locked with
> permission checks done based on the RangeVar when the CreateStmt is
> transformed, which is before we try to look at its inheritance tree to
> find its partitioned parent.  So we would somewhat need to shortcut
> the existing RangeVar lookup and include the parents in the loop to
> find out the correct namespace.  And this is much earlier than now.
> The code complexity is not trivial, so I am getting cold feet when
> trying to support this case in a robust fashion.  For now, I have
> discarded this case and focused on the main problem with SET LOGGED
> and UNLOGGED.
> 
> Switching between logged <-> unlogged does not have such
> complications, because the namespace where the relation is created is
> going to be the same.  So we won't lock or perform permission checks
> on an incorrect namespace.

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.

* 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.

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.

-- 
nathan