Re: Partitioned tables and [un]loggedness

On Thu, May 2, 2024 at 2:07 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Thu, Apr 25, 2024 at 08:55:27AM +0900, Michael Paquier wrote:
> > On Wed, Apr 24, 2024 at 04:43:58PM -0700, David G. Johnston wrote:
> >> My point is that if you feel that treating logged as a copy-able property
> >> is OK then doing the following should also just work:
> >>
> >> postgres=# create temp table parentt ( id integer ) partition by range (id);
> >> CREATE TABLE
> >> postgres=# create table child10t partition of parentt for values from (0)
> >> to (9);
> >> ERROR: cannot create a permanent relation as partition of temporary
> >> relation "parentt"
> >>
> >> i.e., child10t should be created as a temporary partition under parentt.
> >
> > Ah, indeed, I've missed your point here. Lifting the error and
> > inheriting temporary in this case would make sense.
>
> 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.
>
> The addition of LOGGED makes the logic deciding how the loggedness of
> a partition table based on its partitioned table or the query quite
> easy to follow, but this needs some safety nets in the sequence, view
> and CTAS code paths to handle with the case where the query specifies
> no relpersistence.
>
> I have also looked at support for ONLY, and I've been surprised that
> it is not that complicated. tablecmds.c has a ATSimpleRecursion()
> that is smart enough to do an inheritance tree lookup and apply the
> rewrites where they should happen in the step 3 of ALTER TABLE, while
> handling ONLY on its own. The relpersistence of partitioned tables is
> updated in step 2, with the catalog changes.
>
> Attached is a new patch series:
> - 0001 refactors some code around ATPrepChangePersistence() that I
> found confusing after applying the operation to partitioned tables.
> - 0002 adds support for a LOGGED keyword.
> - 0003 expands ALTER TABLE SET [UN]LOGGED to partitioned tables,
> without recursion to partitions.
> - 0004 adds the recursion logic, expanding regression tests to show
> the difference.
>
> 0003 and 0004 should be merged together, I think. Still, splitting
> them makes reviews a bit easier.
> --
> Michael

While reviewing the patches, I found a weird error msg:

+ALTER TABLE logged_part_1 SET UNLOGGED; -- fails as a foreign-key exists
+ERROR: could not change table "logged_part_1" to unlogged because it
references logged table "logged_part_2"

should this be *it is referenced by* here?

The error msg is from ATPrepChangePersistence, and I think we should
do something like:

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b3cc6f8f69..30fbc3836a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16986,7 +16986,7 @@ ATPrepChangePersistence(AlteredTableInfo *tab,
Relation rel, bool toLogged)
if (RelationIsPermanent(foreignrel))
ereport(ERROR,

(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("could
not change table \"%s\" to unlogged because it references logged table
\"%s\"",
+ errmsg("could
not change table \"%s\" to unlogged because it is referenced by logged
table \"%s\"",

What do you think?

--
Regards
Junwang Zhao