public inbox for [email protected]
help / color / mirror / Atom feedFrom: Junwang Zhao <[email protected]>
To: Michael Paquier <[email protected]>
Cc: David G. Johnston <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: Partitioned tables and [un]loggedness
Date: Sun, 1 Sep 2024 13:24:38 +0800
Message-ID: <CAEG8a3K4jMVCb469_24Sf53JuDUh8+161KYmK7rgJ+tOBMDsHg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<20240424202640.GB861150@nathanxps13>
<CAKFQuwY3RTTrp7x4OmEruMmOghdorhq4=eC0ZfG0NM+HnBWFsw@mail.gmail.com>
<[email protected]>
<CAKFQuwZwMOkw-7112u1-gXEhRWuXiZe9otp801Z3dPVvarKdsA@mail.gmail.com>
<[email protected]>
<[email protected]>
On Thu, May 2, 2024 at 2:07 PM Michael Paquier <[email protected]> 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
view thread (8+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Partitioned tables and [un]loggedness
In-Reply-To: <CAEG8a3K4jMVCb469_24Sf53JuDUh8+161KYmK7rgJ+tOBMDsHg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox