Partitioned tables and [un]loggedness
Hi all,
As a recent poke on a thread of 2019 has reminded me, the current
situation of partitioned tables with unlogged is kind of weird:
/messages/by-id/15954-b61523bed4b110c4@postgresql.org
To sum up the situation:
- ALTER TABLE .. SET LOGGED/UNLOGGED does not affect partitioned
tables.
- New partitions don't inherit the loggedness of the partitioned
table.
One of the things that could be done is to forbid the use of UNLOGGED
in partitioned tables, but I'm wondering if we could be smarter than
that to provide a more natural user experience. I've been
experiencing with that and finished with the POC attached, that uses
the following properties:
- Support ALTER TABLE .. SET LOGGED/UNLOGGED for partitioned tables,
where the command only works on partitioned tables so that's only a
catalog switch.
- CREATE TABLE PARTITION OF would make a new partition inherit the
logged ness of the parent if UNLOGGED is not directly specified.
There are some open questions that need attention:
- How about ONLY? Would it make sense to support it so as ALTER TABLE
ONLY on a partitioned table does not touch any of its partitions?
Would not specifying ONLY mean that the loggedness of the partitioned
table and all its partitions is changed? That would mean a burst in
WAL when switching to LOGGED, which was a concern when this feature
was first implemented even for a single table, so for potentially
hundreds of them, that would really hurt if a DBA is not careful.
- CREATE TABLE does not have a LOGGED keyword, hence it is not
possible through the parser to force a partition to have a permanent
persistence even if its partitioned table uses UNLOGGED.
Like tablespaces or even recently access methods, the heuristics can
be fun to define depending on the impact of the table rewrites. The
attached has the code and regression tests to make the rewrites
cheaper, which is to make new partitions inherit the loggedness of the
parent while altering a parent's property leaves the partitions
untouched. With the lack of a LOGGED keyword to force a partition to
be permanent when its partitioned table is unlogged, that's a bit
funny but feel free to comment.
Thanks,
--
Michael
Attachments:
0001-Support-LOGGED-UNLOGGED-for-partitioned-tables.patchtext/x-diff; charset=us-asciiDownload
From f6462a5f4b9b35d5ace52d55759fefc350e371d3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 24 Apr 2024 16:15:29 +0900
Subject: [PATCH] Support LOGGED/UNLOGGED for partitioned tables
When using ALTET TABLE SET LOGGED/UNLOGGED, indexes and sequences that
are owned by the partitioned table changed need to have their
relpersistence also changed. There are a few things that need to be
considered here about ONLY:
- If ONLY is used only on a partitioned table, switch the persistence be
switched only for the partitioned table?
- If ONLY is not used, should this operation recurse across all the
partitions?
CREATE TABLE comes with its own limitation because there is no way to
detect in the parser if a new partition should be forced as logged. For
now, this patch does:
- If UNLOGGED is specified, the partition is unlogged.
- If PERMANENT is found, the partition inherits the loggedness from the
parent.
- There is no way to force a partition to be permanent at query level.
---
src/backend/commands/tablecmds.c | 145 ++++++++++++++++++----
src/test/regress/expected/alter_table.out | 84 +++++++++++++
src/test/regress/sql/alter_table.sql | 35 ++++++
doc/src/sgml/ref/alter_table.sgml | 6 +
doc/src/sgml/ref/create_table.sgml | 5 +
5 files changed, 252 insertions(+), 23 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3556240c8e..90b3993fc4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -601,7 +601,9 @@ static ObjectAddress ATExecClusterOn(Relation rel, const char *indexName,
static void ATExecDropCluster(Relation rel, LOCKMODE lockmode);
static void ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname);
static void ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethod);
-static bool ATPrepChangePersistence(Relation rel, bool toLogged);
+static void ATExecSetPersistenceNoStorage(Relation rel, char newrelpersistence);
+static void ATPrepSetPersistence(AlteredTableInfo *tab, Relation rel,
+ bool toLogged);
static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel,
const char *tablespacename, LOCKMODE lockmode);
static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode);
@@ -985,6 +987,30 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
accessMethodId = get_table_am_oid(default_table_access_method, false);
}
+ /* determine the persistence to use for a partition */
+ if (stmt->partbound)
+ {
+ /* If UNLOGGED has been specified by the query, just use it */
+ if (stmt->relation->relpersistence == RELPERSISTENCE_TEMP)
+ {
+ /* Nothing to do, should be temp */
+ }
+ else if (stmt->relation->relpersistence == RELPERSISTENCE_UNLOGGED)
+ {
+ /* UNLOGGED has been specified by the query, just use it */
+ }
+ else
+ {
+ /*
+ * Case of RELPERSISTENCE_PERMANENT, where the query specified
+ * nothing so inherit the persistency from the parent.
+ */
+ Assert(list_length(inheritOids) == 1);
+ stmt->relation->relpersistence =
+ get_rel_persistence(linitial_oid(inheritOids));
+ }
+ }
+
/*
* Create the relation. Inherited defaults and constraints are passed in
* for immediate handling --- since they don't need parsing, they can be
@@ -5036,13 +5062,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change persistence setting twice")));
- tab->chgPersistence = ATPrepChangePersistence(rel, true);
- /* force rewrite if necessary; see comment in ATRewriteTables */
- if (tab->chgPersistence)
- {
- tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE;
- tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
- }
+ ATPrepSetPersistence(tab, rel, true);
pass = AT_PASS_MISC;
break;
case AT_SetUnLogged: /* SET UNLOGGED */
@@ -5051,13 +5071,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change persistence setting twice")));
- tab->chgPersistence = ATPrepChangePersistence(rel, false);
- /* force rewrite if necessary; see comment in ATRewriteTables */
- if (tab->chgPersistence)
- {
- tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE;
- tab->newrelpersistence = RELPERSISTENCE_UNLOGGED;
- }
+ ATPrepSetPersistence(tab, rel, false);
pass = AT_PASS_MISC;
break;
case AT_DropOids: /* SET WITHOUT OIDS */
@@ -5427,6 +5441,14 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
break;
case AT_SetLogged: /* SET LOGGED */
case AT_SetUnLogged: /* SET UNLOGGED */
+
+ /*
+ * Only do this for partitioned tables, for which this is just a
+ * catalog change. Tables with storage are handled by Phase 3.
+ */
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
+ tab->chgPersistence)
+ ATExecSetPersistenceNoStorage(rel, tab->newrelpersistence);
break;
case AT_DropOids: /* SET WITHOUT OIDS */
/* nothing to do here, oid columns don't exist anymore */
@@ -15518,6 +15540,79 @@ ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethodId)
table_close(pg_class, RowExclusiveLock);
}
+/*
+ * Special handling of ALTER TABLE SET LOGGED/UNLOGGED for relations with no
+ * storage that have an interest in changing their persistence.
+ *
+ * Since these have no storage, setting the persistence to permanent or
+ * unlogged is a catalog-only operation. This needs to switch the
+ * persistence of all sequences and indexes related to this relation.
+ */
+static void
+ATExecSetPersistenceNoStorage(Relation rel, char newrelpersistence)
+{
+ Relation pg_class;
+ HeapTuple tuple;
+ List *reloids; /* for indexes and sequences */
+ ListCell *elt;
+ Form_pg_class rd_rel;
+ Oid reloid = RelationGetRelid(rel);
+
+ /*
+ * Shouldn't be called on relations having storage; these are processed in
+ * phase 3.
+ */
+ Assert(!RELKIND_HAS_STORAGE(rel->rd_rel->relkind));
+
+ /* Get a modifiable copy of the relation's pg_class row. */
+ pg_class = table_open(RelationRelationId, RowExclusiveLock);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", reloid);
+ rd_rel = (Form_pg_class) GETSTRUCT(tuple);
+
+ /* Leave if no update required */
+ if (rd_rel->relpersistence == newrelpersistence)
+ {
+ heap_freetuple(tuple);
+ table_close(pg_class, RowExclusiveLock);
+ return;
+ }
+
+ /* Update the pg_class row. */
+ rd_rel->relpersistence = newrelpersistence;
+ CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+ InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(rel), 0);
+
+ heap_freetuple(tuple);
+
+ /* Update the per-sequence and per-index relpersistence */
+ reloids = getOwnedSequences(reloid);
+ reloids = list_union_oid(reloids, RelationGetIndexList(rel));
+ foreach(elt, reloids)
+ {
+ Oid classoid = lfirst_oid(elt);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(classoid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", classoid);
+ rd_rel = (Form_pg_class) GETSTRUCT(tuple);
+
+ rd_rel->relpersistence = newrelpersistence;
+ CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+ InvokeObjectPostAlterHook(RelationRelationId, classoid, 0);
+
+ heap_freetuple(tuple);
+ }
+
+ /* Make sure the persistence changes are visible */
+ CommandCounterIncrement();
+
+ table_close(pg_class, RowExclusiveLock);
+}
+
/*
* ALTER TABLE SET TABLESPACE
*/
@@ -17749,12 +17844,10 @@ ATExecSetCompression(Relation rel,
* This verifies that we're not trying to change a temp table. Also,
* existing foreign key constraints are checked to avoid ending up with
* permanent tables referencing unlogged tables.
- *
- * Return value is false if the operation is a no-op (in which case the
- * checks are skipped), otherwise true.
*/
-static bool
-ATPrepChangePersistence(Relation rel, bool toLogged)
+static void
+ATPrepSetPersistence(AlteredTableInfo *tab, Relation rel,
+ bool toLogged)
{
Relation pg_constraint;
HeapTuple tuple;
@@ -17778,12 +17871,12 @@ ATPrepChangePersistence(Relation rel, bool toLogged)
case RELPERSISTENCE_PERMANENT:
if (toLogged)
/* nothing to do */
- return false;
+ return;
break;
case RELPERSISTENCE_UNLOGGED:
if (!toLogged)
/* nothing to do */
- return false;
+ return;
break;
}
@@ -17866,7 +17959,13 @@ ATPrepChangePersistence(Relation rel, bool toLogged)
table_close(pg_constraint, AccessShareLock);
- return true;
+ /* force rewrite if necessary; see comment in ATRewriteTables */
+ tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE;
+ if (toLogged)
+ tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
+ else
+ tab->newrelpersistence = RELPERSISTENCE_UNLOGGED;
+ tab->chgPersistence = true;
}
/*
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 7666c76238..5f3caf6baf 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3597,6 +3597,90 @@ ALTER TABLE logged1 SET UNLOGGED; -- silently do nothing
DROP TABLE logged3;
DROP TABLE logged2;
DROP TABLE logged1;
+-- SET LOGGED/UNLOGGED with partitioned tables
+CREATE TABLE logged_part_1(f1 SERIAL PRIMARY KEY)
+ PARTITION BY LIST (f1); -- has sequence, index
+CREATE TABLE logged_part_2(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES logged_part_1)
+ PARTITION BY LIST (f1); -- foreign key
+CREATE TABLE logged_part_3(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES logged_part_3)
+ PARTITION BY LIST (f1); -- self-referencing foreign key
+-- Check relpersistence of all the objects created.
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part'
+ ORDER BY relname;
+ relname | relpersistence
+----------------------+----------------
+ logged_part_1 | p
+ logged_part_1_f1_seq | p
+ logged_part_1_pkey | p
+ logged_part_2 | p
+ logged_part_2_f1_seq | p
+ logged_part_2_pkey | p
+ logged_part_3 | p
+ logged_part_3_f1_seq | p
+ logged_part_3_pkey | p
+(9 rows)
+
+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"
+ALTER TABLE logged_part_2 SET UNLOGGED;
+ALTER TABLE logged_part_3 SET UNLOGGED; -- skip self-referencing foreign key
+-- Re-check relpersistence of all the objects created.
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part'
+ ORDER BY relname;
+ relname | relpersistence
+----------------------+----------------
+ logged_part_1 | p
+ logged_part_1_f1_seq | p
+ logged_part_1_pkey | p
+ logged_part_2 | u
+ logged_part_2_f1_seq | u
+ logged_part_2_pkey | u
+ logged_part_3 | u
+ logged_part_3_f1_seq | u
+ logged_part_3_pkey | u
+(9 rows)
+
+ALTER TABLE logged_part_1 SET LOGGED; -- no-op
+ALTER TABLE logged_part_2 SET LOGGED;
+ALTER TABLE logged_part_3 SET LOGGED; -- skip self-referencing foreign key
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_[2|3]'
+ ORDER BY relname;
+ relname | relpersistence
+----------------------+----------------
+ logged_part_2 | p
+ logged_part_2_f1_seq | p
+ logged_part_2_pkey | p
+ logged_part_3 | p
+ logged_part_3_f1_seq | p
+ logged_part_3_pkey | p
+(6 rows)
+
+-- Partitions
+CREATE TABLE logged_part_2_1 PARTITION OF logged_part_2
+ FOR VALUES IN (1); -- permanent, inherited
+CREATE UNLOGGED TABLE logged_part_2_2 PARTITION OF logged_part_2
+ FOR VALUES IN (2); -- unlogged, not inherited
+ALTER TABLE logged_part_2 SET UNLOGGED;
+CREATE TABLE logged_part_2_3 PARTITION OF logged_part_2
+ FOR VALUES IN (3); -- unlogged, inherited
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_2'
+ ORDER BY relname;
+ relname | relpersistence
+----------------------+----------------
+ logged_part_2 | u
+ logged_part_2_1 | p
+ logged_part_2_1_pkey | p
+ logged_part_2_2 | u
+ logged_part_2_2_pkey | u
+ logged_part_2_3 | u
+ logged_part_2_3_pkey | u
+ logged_part_2_f1_seq | u
+ logged_part_2_pkey | u
+(9 rows)
+
+DROP TABLE logged_part_3;
+DROP TABLE logged_part_2;
+DROP TABLE logged_part_1;
-- test ADD COLUMN IF NOT EXISTS
CREATE TABLE test_add_column(c1 integer);
\d test_add_column
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 9df5a63bdf..903e3aa217 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2259,6 +2259,41 @@ DROP TABLE logged3;
DROP TABLE logged2;
DROP TABLE logged1;
+-- SET LOGGED/UNLOGGED with partitioned tables
+CREATE TABLE logged_part_1(f1 SERIAL PRIMARY KEY)
+ PARTITION BY LIST (f1); -- has sequence, index
+CREATE TABLE logged_part_2(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES logged_part_1)
+ PARTITION BY LIST (f1); -- foreign key
+CREATE TABLE logged_part_3(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES logged_part_3)
+ PARTITION BY LIST (f1); -- self-referencing foreign key
+-- Check relpersistence of all the objects created.
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part'
+ ORDER BY relname;
+ALTER TABLE logged_part_1 SET UNLOGGED; -- fails as a foreign-key exists
+ALTER TABLE logged_part_2 SET UNLOGGED;
+ALTER TABLE logged_part_3 SET UNLOGGED; -- skip self-referencing foreign key
+-- Re-check relpersistence of all the objects created.
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part'
+ ORDER BY relname;
+ALTER TABLE logged_part_1 SET LOGGED; -- no-op
+ALTER TABLE logged_part_2 SET LOGGED;
+ALTER TABLE logged_part_3 SET LOGGED; -- skip self-referencing foreign key
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_[2|3]'
+ ORDER BY relname;
+-- Partitions
+CREATE TABLE logged_part_2_1 PARTITION OF logged_part_2
+ FOR VALUES IN (1); -- permanent, inherited
+CREATE UNLOGGED TABLE logged_part_2_2 PARTITION OF logged_part_2
+ FOR VALUES IN (2); -- unlogged, not inherited
+ALTER TABLE logged_part_2 SET UNLOGGED;
+CREATE TABLE logged_part_2_3 PARTITION OF logged_part_2
+ FOR VALUES IN (3); -- unlogged, inherited
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_2'
+ ORDER BY relname;
+DROP TABLE logged_part_3;
+DROP TABLE logged_part_2;
+DROP TABLE logged_part_1;
+
-- test ADD COLUMN IF NOT EXISTS
CREATE TABLE test_add_column(c1 integer);
\d test_add_column
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index fe36ff82e5..b85d8dcd15 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -803,6 +803,12 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
(for identity or serial columns). However, it is also possible to
change the persistence of such sequences separately.
</para>
+
+ <para>
+ Setting this property for a partitioned table has no direct effect,
+ because such tables have no storage of their own, but the configured
+ value will be inherited by newly-created partitions.
+ </para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 02f31d2d6f..d1ff9d9e95 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -221,6 +221,11 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
If this is specified, any sequences created together with the unlogged
table (for identity or serial columns) are also created as unlogged.
</para>
+
+ <para>
+ When applied to a partitioned table, newly-created partitions and
+ their objects (sequences and indexes) will inherit this property.
+ </para>
</listitem>
</varlistentry>
--
2.43.0
On Wed, Apr 24, 2024 at 04:17:44PM +0900, Michael Paquier wrote:
- Support ALTER TABLE .. SET LOGGED/UNLOGGED for partitioned tables,
where the command only works on partitioned tables so that's only a
catalog switch.
I'm not following what this means. Does SET [UN]LOGGED on a partitioned
table recurse to its partitions? Does this mean that you cannot changed
whether a single partition is [UN]LOGGED? How does this work with
sub-partitioning?
- CREATE TABLE PARTITION OF would make a new partition inherit the
logged ness of the parent if UNLOGGED is not directly specified.
This one seems generally reasonable to me, provided it's properly noted in
the docs.
- How about ONLY? Would it make sense to support it so as ALTER TABLE
ONLY on a partitioned table does not touch any of its partitions?
Would not specifying ONLY mean that the loggedness of the partitioned
table and all its partitions is changed? That would mean a burst in
WAL when switching to LOGGED, which was a concern when this feature
was first implemented even for a single table, so for potentially
hundreds of them, that would really hurt if a DBA is not careful.
I guess ONLY could be a way of changing the default for new partitions
without changing whether existing ones were logged. I'm not tremendously
concerned about the burst-of-WAL problem. Or, at least, I'm not any more
concerned about it for partitioned tables than I am for regular tables. I
do wonder if we can improve the performance of setting tables LOGGED, but
that's for a separate thread...
- CREATE TABLE does not have a LOGGED keyword, hence it is not
possible through the parser to force a partition to have a permanent
persistence even if its partitioned table uses UNLOGGED.
Could we add LOGGED for CREATE TABLE?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, Apr 24, 2024 at 1:26 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:
On Wed, Apr 24, 2024 at 04:17:44PM +0900, Michael Paquier wrote:
- Support ALTER TABLE .. SET LOGGED/UNLOGGED for partitioned tables,
where the command only works on partitioned tables so that's only a
catalog switch.I'm not following what this means. Does SET [UN]LOGGED on a partitioned
table recurse to its partitions? Does this mean that you cannot changed
whether a single partition is [UN]LOGGED? How does this work with
sub-partitioning?- CREATE TABLE PARTITION OF would make a new partition inherit the
logged ness of the parent if UNLOGGED is not directly specified.This one seems generally reasonable to me, provided it's properly noted in
the docs.
I don't see this being desirable at this point. And especially not by
itself. It is an error to not specify TEMP on the partition create table
command when the parent is temporary, and that one is a no-brainer for
having the persistence mode of the child be derived from the parent. The
current policy of requiring the persistence of the child to be explicitly
specified seems perfectly reasonable. Or, put differently, the specific
current persistence of the parent doesn't get copied or even considered
when creating children.
In any case we aren't going to be able to do exactly what it means by
marking a partitioned table unlogged - namely that we execute the truncate
command on it after a crash. Forcing the implementation here just so that
our existing decision to ignore unlogged on the parent table is, IMO,
letting optics corrupt good design.
I do agree with having an in-core way for the DBA to communicate that they
wish for partitions to be created with a known persistence unless the
create table command specifies something different. The way I would
approach this is to add something like the following to the syntax/system:
CREATE [ persistence_mode ] TABLE ...
...
WITH (partition_default_persistence = { logged, unlogged, temporary }) --
storage_parameter, logged is effectively the default
This method is more explicit and has zero implications for existing backups
or upgrading.
- How about ONLY? Would it make sense to support it so as ALTER TABLE
ONLY on a partitioned table does not touch any of its partitions?
I'd leave it to the community to develop and maintain scripts that iterate
over the partition hierarchy and toggle the persistence mode between logged
and unlogged on the individual partitions using whatever throttling and
batching strategy each individual user requires for their situation.
- CREATE TABLE does not have a LOGGED keyword, hence it is not
possible through the parser to force a partition to have a permanent
persistence even if its partitioned table uses UNLOGGED.Could we add LOGGED for CREATE TABLE?
I do agree with adding LOGGED to the list of optional persistence_mode
specifiers, possibly regardless of whether any of this happens. Seems
desirable to give our default mode a name.
David J.
On Wed, Apr 24, 2024 at 03:26:40PM -0500, Nathan Bossart wrote:
On Wed, Apr 24, 2024 at 04:17:44PM +0900, Michael Paquier wrote:
- Support ALTER TABLE .. SET LOGGED/UNLOGGED for partitioned tables,
where the command only works on partitioned tables so that's only a
catalog switch.I'm not following what this means. Does SET [UN]LOGGED on a partitioned
table recurse to its partitions? Does this mean that you cannot changed
whether a single partition is [UN]LOGGED? How does this work with
sub-partitioning?
The POC implements ALTER TABLE .. SET LOGGED/UNLOGGED so as the change
only reflects on the relation defined on the query. In short, if
specifying a partitioned table as relation, only its relpersistence is
changed in the patch. If sub-partitioning is involved, the POC
behaves the same way, relpersistence for partitioned table A1 attached
to partitioned table A does not change under ALTER TABLE A.
Recursing to all the partitions and partitioned tables attached to a
partitioned table A when using an ALTER TABLE A, when ONLY is not
specified, is OK by me if that's the consensus folks prefer. I just
wanted to gather some opinions first about what people find the most
natural.
- CREATE TABLE PARTITION OF would make a new partition inherit the
logged ness of the parent if UNLOGGED is not directly specified.This one seems generally reasonable to me, provided it's properly noted in
the docs.
Noted. That's a second piece. This part felt natural to me, but it
would not fly far without a LOGGED keyword and a way to attach a new
"undefined" RELPERSISTENCE_ in gram.y's OptTemp, likely a '\0'.
That's going to require some tweaks for CTAS as these cannot be
partitioned, but that should be a few lines to fall back to a
permanent if the query is parsed with the new "undefined".
- How about ONLY? Would it make sense to support it so as ALTER TABLE
ONLY on a partitioned table does not touch any of its partitions?
Would not specifying ONLY mean that the loggedness of the partitioned
table and all its partitions is changed? That would mean a burst in
WAL when switching to LOGGED, which was a concern when this feature
was first implemented even for a single table, so for potentially
hundreds of them, that would really hurt if a DBA is not careful.I guess ONLY could be a way of changing the default for new partitions
without changing whether existing ones were logged. I'm not tremendously
concerned about the burst-of-WAL problem. Or, at least, I'm not any more
concerned about it for partitioned tables than I am for regular tables. I
do wonder if we can improve the performance of setting tables LOGGED, but
that's for a separate thread...
Yeah. A burst of WAL caused by a switch to LOGGED for a few thousand
partitions would never be fun, perhaps I'm just worrying to much as
that should not be a regular operation.
- CREATE TABLE does not have a LOGGED keyword, hence it is not
possible through the parser to force a partition to have a permanent
persistence even if its partitioned table uses UNLOGGED.Could we add LOGGED for CREATE TABLE?
I don't see why not if people agree with it, that's a patch of its own
that would help greatly in making the [un]logged attribute be
inherited for new partitions, because it is them possible to force a
persistence to be permanent as much as unlogged at query level, easing
the manipulation of partition trees.
--
Michael
On Wed, Apr 24, 2024 at 03:36:35PM -0700, David G. Johnston wrote:
On Wed, Apr 24, 2024 at 1:26 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:On Wed, Apr 24, 2024 at 04:17:44PM +0900, Michael Paquier wrote:
- CREATE TABLE PARTITION OF would make a new partition inherit the
logged ness of the parent if UNLOGGED is not directly specified.This one seems generally reasonable to me, provided it's properly noted in
the docs.I don't see this being desirable at this point. And especially not by
itself. It is an error to not specify TEMP on the partition create table
command when the parent is temporary, and that one is a no-brainer for
having the persistence mode of the child be derived from the parent. The
current policy of requiring the persistence of the child to be explicitly
specified seems perfectly reasonable. Or, put differently, the specific
current persistence of the parent doesn't get copied or even considered
when creating children.
I disagree here, actually. Temporary tables are a different beast
because they require automated cleanup which would include interacting
with the partitionining information if temp and non-temp relations are
mixed. That's why the current restrictions are in place: you should
be able to mix them. Mixing unlogged and logged is OK, they retain a
state in their catalogs.
In any case we aren't going to be able to do exactly what it means by
marking a partitioned table unlogged - namely that we execute the truncate
command on it after a crash. Forcing the implementation here just so that
our existing decision to ignore unlogged on the parent table is, IMO,
letting optics corrupt good design.
It depends on retention policies, for one. I could imagine an
application where partitioning is used based on a key where we
classify records based on their persistency, and one does not care
about a portion of them, still would like some retention as long as
the application is healthy.
I do agree with having an in-core way for the DBA to communicate that they
wish for partitions to be created with a known persistence unless the
create table command specifies something different. The way I would
approach this is to add something like the following to the syntax/system:CREATE [ persistence_mode ] TABLE ...
...
WITH (partition_default_persistence = { logged, unlogged, temporary }) --
storage_parameter, logged is effectively the default
While we have keywords to drive that at query level for TEMP and
UNLOGGED? Not sure to be on board with this idea. pg_dump may need
some changes to reflect the loggedness across the partitions, now that
I think about it even if there should be an ATTACH once the table is
created to link it to its partitioned table. There should be no
rewrites at restore.
I do agree with adding LOGGED to the list of optional persistence_mode
specifiers, possibly regardless of whether any of this happens. Seems
desirable to give our default mode a name.
Yeah, at least it looks like Nathan and you are OK with this addition.
--
Michael
On Wed, Apr 24, 2024 at 4:35 PM Michael Paquier <michael@paquier.xyz> wrote:
I disagree here, actually. Temporary tables are a different beast
because they require automated cleanup which would include interacting
with the partitionining information if temp and non-temp relations are
mixed. That's why the current restrictions are in place: you should
[ not ] ?
be able to mix them.
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.
David J.
On Thu, Apr 25, 2024 at 08:35:32AM +0900, Michael Paquier wrote:
That's why the current restrictions are in place: you should
be able to mix them.
Correction due to a ENOCOFFEE: you should *not* be able to mix them.
--
Michael
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.
--
Michael
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
Attachments:
v2-0001-Refactor-some-code-of-ALTER-TABLE-SET-LOGGED-UNLO.patchtext/x-diff; charset=us-asciiDownload
From 30d572fac2aa58ce2d62ba929c30fded9b020e0b Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 2 May 2024 08:16:33 +0900
Subject: [PATCH v2 1/4] Refactor some code of ALTER TABLE SET LOGGED/UNLOGGED
This is in preparation for an upcoming patch set to extend the
possibilities in this area, making the code more consistent with the
surroundings related to access methods and tablespaces.
---
src/backend/commands/tablecmds.c | 38 +++++++++++++-------------------
1 file changed, 15 insertions(+), 23 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 08c87e6029..baabbf82e7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -602,7 +602,8 @@ static ObjectAddress ATExecClusterOn(Relation rel, const char *indexName,
static void ATExecDropCluster(Relation rel, LOCKMODE lockmode);
static void ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname);
static void ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethod);
-static bool ATPrepChangePersistence(Relation rel, bool toLogged);
+static void ATPrepSetPersistence(AlteredTableInfo *tab, Relation rel,
+ bool toLogged);
static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel,
const char *tablespacename, LOCKMODE lockmode);
static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode);
@@ -5037,13 +5038,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change persistence setting twice")));
- tab->chgPersistence = ATPrepChangePersistence(rel, true);
- /* force rewrite if necessary; see comment in ATRewriteTables */
- if (tab->chgPersistence)
- {
- tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE;
- tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
- }
+ ATPrepSetPersistence(tab, rel, true);
pass = AT_PASS_MISC;
break;
case AT_SetUnLogged: /* SET UNLOGGED */
@@ -5052,13 +5047,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change persistence setting twice")));
- tab->chgPersistence = ATPrepChangePersistence(rel, false);
- /* force rewrite if necessary; see comment in ATRewriteTables */
- if (tab->chgPersistence)
- {
- tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE;
- tab->newrelpersistence = RELPERSISTENCE_UNLOGGED;
- }
+ ATPrepSetPersistence(tab, rel, false);
pass = AT_PASS_MISC;
break;
case AT_DropOids: /* SET WITHOUT OIDS */
@@ -17775,12 +17764,9 @@ ATExecSetCompression(Relation rel,
* This verifies that we're not trying to change a temp table. Also,
* existing foreign key constraints are checked to avoid ending up with
* permanent tables referencing unlogged tables.
- *
- * Return value is false if the operation is a no-op (in which case the
- * checks are skipped), otherwise true.
*/
-static bool
-ATPrepChangePersistence(Relation rel, bool toLogged)
+static void
+ATPrepSetPersistence(AlteredTableInfo *tab, Relation rel, bool toLogged)
{
Relation pg_constraint;
HeapTuple tuple;
@@ -17804,12 +17790,12 @@ ATPrepChangePersistence(Relation rel, bool toLogged)
case RELPERSISTENCE_PERMANENT:
if (toLogged)
/* nothing to do */
- return false;
+ return;
break;
case RELPERSISTENCE_UNLOGGED:
if (!toLogged)
/* nothing to do */
- return false;
+ return;
break;
}
@@ -17892,7 +17878,13 @@ ATPrepChangePersistence(Relation rel, bool toLogged)
table_close(pg_constraint, AccessShareLock);
- return true;
+ /* force rewrite if necessary; see comment in ATRewriteTables */
+ tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE;
+ if (toLogged)
+ tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
+ else
+ tab->newrelpersistence = RELPERSISTENCE_UNLOGGED;
+ tab->chgPersistence = true;
}
/*
--
2.43.0
v2-0002-Add-support-for-LOGGED-keyword-similar-to-UNLOGGE.patchtext/x-diff; charset=us-asciiDownload
From f00c1295bf7d1a7f48409f8ddfedeffab87dabaa Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 2 May 2024 09:29:48 +0900
Subject: [PATCH v2 2/4] Add support for LOGGED keyword, similar to UNLOGGED
but for permanent
This extends the following statements with a new keyword called LOGGED,
to be able to force a relation to be permanent:
- CREATE SEQUENCE
- CREATE TABLE AS
- SELECT INTO
- CREATE TABLE
The implementation is done here with the introduction of a
RELPERSISTENCE_INVALID, which is set by the grammar when neither TEMP,
UNLOGGED or LOGGED are specified. This is handy for an upcoming patch
that aims to introduce relpersistence inheritance for partitions, based
on its partitioned table.
---
src/include/catalog/pg_class.h | 1 +
src/backend/catalog/namespace.c | 8 ++++++
src/backend/commands/createas.c | 7 +++++
src/backend/commands/sequence.c | 7 +++++
src/backend/commands/tablecmds.c | 9 +++++++
src/backend/commands/view.c | 7 +++++
src/backend/parser/gram.y | 8 +++++-
src/test/regress/expected/identity.out | 11 ++++++++
src/test/regress/expected/select_into.out | 32 +++++++++++++++++++++++
src/test/regress/expected/sequence.out | 9 +++++++
src/test/regress/sql/identity.sql | 6 +++++
src/test/regress/sql/select_into.sql | 13 +++++++++
src/test/regress/sql/sequence.sql | 5 ++++
doc/src/sgml/ref/create_sequence.sgml | 12 ++++++++-
doc/src/sgml/ref/create_table.sgml | 22 +++++++++++++---
doc/src/sgml/ref/create_table_as.sgml | 12 ++++++++-
doc/src/sgml/ref/select_into.sgml | 12 ++++++++-
17 files changed, 174 insertions(+), 7 deletions(-)
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 0fc2c093b0..1b3fd060ca 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -175,6 +175,7 @@ MAKE_SYSCACHE(RELNAMENSP, pg_class_relname_nsp_index, 128);
#define RELPERSISTENCE_PERMANENT 'p' /* regular table */
#define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent table */
#define RELPERSISTENCE_TEMP 't' /* temporary table */
+#define RELPERSISTENCE_INVALID '\0' /* persistence not allowed */
/* default selection for replica identity (primary key or nothing) */
#define REPLICA_IDENTITY_DEFAULT 'd'
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index a2510cf80c..4bb9fafe1d 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -853,6 +853,14 @@ RangeVarAdjustRelationPersistence(RangeVar *newRelation, Oid nspid)
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("cannot create relations in temporary schemas of other sessions")));
break;
+ case RELPERSISTENCE_INVALID: /* persistence not specified by grammar */
+ if (isTempOrTempToastNamespace(nspid))
+ newRelation->relpersistence = RELPERSISTENCE_TEMP;
+ else if (isAnyTempNamespace(nspid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("only temporary relations may be created in temporary schemas")));
+ break;
default:
if (isAnyTempNamespace(nspid))
ereport(ERROR,
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 62050f4dc5..2a26fe0b6f 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -238,6 +238,13 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
if (CreateTableAsRelExists(stmt))
return InvalidObjectAddress;
+ /*
+ * If the grammar did not specify a relpersistence, assume that the
+ * relation is permanent.
+ */
+ if (into->rel->relpersistence == RELPERSISTENCE_INVALID)
+ into->rel->relpersistence = RELPERSISTENCE_PERMANENT;
+
/*
* Create the tuple receiver object and insert info it will need
*/
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 46103561c3..3388e59f56 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -162,6 +162,13 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
}
}
+ /*
+ * If the grammar did not specify a relpersistence, assume that the
+ * relation is permanent.
+ */
+ if (seq->sequence->relpersistence == RELPERSISTENCE_INVALID)
+ seq->sequence->relpersistence = RELPERSISTENCE_PERMANENT;
+
/* Check and set all option values */
init_params(pstate, seq->options, seq->for_identity, true,
&seqform, &seqdataform,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index baabbf82e7..97ba34f0d5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -810,6 +810,15 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
inheritOids = lappend_oid(inheritOids, parentOid);
}
+ /*
+ * If the grammar did not specify a relpersistence, assume that the
+ * relation is permanent. Note that this is done before selecting
+ * the relation's tablespace, as this change may impact the tablespace
+ * location depending on the persistence set here.
+ */
+ if (stmt->relation->relpersistence == RELPERSISTENCE_INVALID)
+ stmt->relation->relpersistence = RELPERSISTENCE_PERMANENT;
+
/*
* Select tablespace to use: an explicitly indicated one, or (in the case
* of a partitioned table) the parent's, if it has one.
diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
index fdad833832..6e691525d7 100644
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -388,6 +388,13 @@ DefineView(ViewStmt *stmt, const char *queryString,
if (viewParse->commandType != CMD_SELECT)
elog(ERROR, "unexpected parse analysis result");
+ /*
+ * If the grammar did not specify a relpersistence, assume that the
+ * relation is permanent.
+ */
+ if (stmt->view->relpersistence == RELPERSISTENCE_INVALID)
+ stmt->view->relpersistence = RELPERSISTENCE_PERMANENT;
+
/*
* Check for unsupported cases. These tests are redundant with ones in
* DefineQueryRewrite(), but that function will complain about a bogus ON
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e8b619926e..85ef8dd372 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3775,7 +3775,8 @@ OptTemp: TEMPORARY { $$ = RELPERSISTENCE_TEMP; }
$$ = RELPERSISTENCE_TEMP;
}
| UNLOGGED { $$ = RELPERSISTENCE_UNLOGGED; }
- | /*EMPTY*/ { $$ = RELPERSISTENCE_PERMANENT; }
+ | LOGGED { $$ = RELPERSISTENCE_PERMANENT; }
+ | /*EMPTY*/ { $$ = RELPERSISTENCE_INVALID; }
;
OptTableElementList:
@@ -13129,6 +13130,11 @@ OptTempTableName:
$$ = $3;
$$->relpersistence = RELPERSISTENCE_UNLOGGED;
}
+ | LOGGED opt_table qualified_name
+ {
+ $$ = $3;
+ $$->relpersistence = RELPERSISTENCE_PERMANENT;
+ }
| TABLE qualified_name
{
$$ = $2;
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index f357b9b63b..373df20661 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -365,6 +365,17 @@ SELECT seqtypid::regtype FROM pg_sequence WHERE seqrelid = 'itest3_a_seq'::regcl
ALTER TABLE itest3 ALTER COLUMN a TYPE text; -- error
ERROR: identity column type must be smallint, integer, or bigint
+-- check that LOGGED propagates to sequence (for grammar)
+CREATE LOGGED TABLE itest16 (a int NOT NULL, b text);
+ALTER TABLE itest16 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
+\d itest16_a_seq
+ Sequence "public.itest16_a_seq"
+ Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
+---------+-------+---------+------------+-----------+---------+-------
+ integer | 1 | 1 | 2147483647 | 1 | no | 1
+Sequence for identity column: public.itest16.a
+
+DROP TABLE itest16;
-- check that unlogged propagates to sequence
CREATE UNLOGGED TABLE itest17 (a int NOT NULL, b text);
ALTER TABLE itest17 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
diff --git a/src/test/regress/expected/select_into.out b/src/test/regress/expected/select_into.out
index b79fe9a1c0..fb9c3eaa2a 100644
--- a/src/test/regress/expected/select_into.out
+++ b/src/test/regress/expected/select_into.out
@@ -220,3 +220,35 @@ NOTICE: relation "ctas_ine_tbl" already exists, skipping
(0 rows)
DROP TABLE ctas_ine_tbl;
+-- CREATE TABLE AS with LOGGED and UNLOGGED.
+CREATE UNLOGGED TABLE ctas_unlogged_tbl AS SELECT 1 AS a;
+\d ctas_unlogged_tbl
+ Unlogged table "public.ctas_unlogged_tbl"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+
+CREATE LOGGED TABLE ctas_logged_tbl AS SELECT 1 AS a;
+\d ctas_logged_tbl
+ Table "public.ctas_logged_tbl"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+
+DROP TABLE ctas_logged_tbl, ctas_unlogged_tbl;
+-- SELECT INTO with LOGGED and UNLOGGED.
+SELECT 1 AS a INTO UNLOGGED ctas_unlogged_tbl;
+\d ctas_unlogged_tbl
+ Unlogged table "public.ctas_unlogged_tbl"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+
+SELECT 1 AS a INTO LOGGED ctas_logged_tbl;
+\d ctas_logged_tbl
+ Table "public.ctas_logged_tbl"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+
+DROP TABLE ctas_logged_tbl, ctas_unlogged_tbl;
diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index 2b47b7796b..653428ddf6 100644
--- a/src/test/regress/expected/sequence.out
+++ b/src/test/regress/expected/sequence.out
@@ -599,6 +599,15 @@ DROP SEQUENCE seq2;
-- should fail
SELECT lastval();
ERROR: lastval is not yet defined in this session
+-- logged sequences (for grammar)
+CREATE LOGGED SEQUENCE sequence_test_logged;
+\d sequence_test_logged
+ Sequence "public.sequence_test_logged"
+ Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
+--------+-------+---------+---------------------+-----------+---------+-------
+ bigint | 1 | 1 | 9223372036854775807 | 1 | no | 1
+
+DROP SEQUENCE sequence_test_logged;
-- unlogged sequences
-- (more tests in src/test/recovery/)
CREATE UNLOGGED SEQUENCE sequence_test_unlogged;
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 7b0800226c..833c6f49fb 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -214,6 +214,12 @@ SELECT seqtypid::regtype FROM pg_sequence WHERE seqrelid = 'itest3_a_seq'::regcl
ALTER TABLE itest3 ALTER COLUMN a TYPE text; -- error
+-- check that LOGGED propagates to sequence (for grammar)
+CREATE LOGGED TABLE itest16 (a int NOT NULL, b text);
+ALTER TABLE itest16 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
+\d itest16_a_seq
+DROP TABLE itest16;
+
-- check that unlogged propagates to sequence
CREATE UNLOGGED TABLE itest17 (a int NOT NULL, b text);
ALTER TABLE itest17 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
diff --git a/src/test/regress/sql/select_into.sql b/src/test/regress/sql/select_into.sql
index 689c448cc2..55ffb25c12 100644
--- a/src/test/regress/sql/select_into.sql
+++ b/src/test/regress/sql/select_into.sql
@@ -136,3 +136,16 @@ EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
CREATE TABLE IF NOT EXISTS ctas_ine_tbl AS EXECUTE ctas_ine_query; -- ok
DROP TABLE ctas_ine_tbl;
+
+-- CREATE TABLE AS with LOGGED and UNLOGGED.
+CREATE UNLOGGED TABLE ctas_unlogged_tbl AS SELECT 1 AS a;
+\d ctas_unlogged_tbl
+CREATE LOGGED TABLE ctas_logged_tbl AS SELECT 1 AS a;
+\d ctas_logged_tbl
+DROP TABLE ctas_logged_tbl, ctas_unlogged_tbl;
+-- SELECT INTO with LOGGED and UNLOGGED.
+SELECT 1 AS a INTO UNLOGGED ctas_unlogged_tbl;
+\d ctas_unlogged_tbl
+SELECT 1 AS a INTO LOGGED ctas_logged_tbl;
+\d ctas_logged_tbl
+DROP TABLE ctas_logged_tbl, ctas_unlogged_tbl;
diff --git a/src/test/regress/sql/sequence.sql b/src/test/regress/sql/sequence.sql
index 674f5f1f66..189112fef7 100644
--- a/src/test/regress/sql/sequence.sql
+++ b/src/test/regress/sql/sequence.sql
@@ -271,6 +271,11 @@ DROP SEQUENCE seq2;
-- should fail
SELECT lastval();
+-- logged sequences (for grammar)
+CREATE LOGGED SEQUENCE sequence_test_logged;
+\d sequence_test_logged
+DROP SEQUENCE sequence_test_logged;
+
-- unlogged sequences
-- (more tests in src/test/recovery/)
CREATE UNLOGGED SEQUENCE sequence_test_unlogged;
diff --git a/doc/src/sgml/ref/create_sequence.sgml b/doc/src/sgml/ref/create_sequence.sgml
index 34e9084b5c..ef8654cee5 100644
--- a/doc/src/sgml/ref/create_sequence.sgml
+++ b/doc/src/sgml/ref/create_sequence.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-CREATE [ { TEMPORARY | TEMP } | UNLOGGED ] SEQUENCE [ IF NOT EXISTS ] <replaceable class="parameter">name</replaceable>
+CREATE [ { TEMPORARY | TEMP } | LOGGED | UNLOGGED ] SEQUENCE [ IF NOT EXISTS ] <replaceable class="parameter">name</replaceable>
[ AS <replaceable class="parameter">data_type</replaceable> ]
[ INCREMENT [ BY ] <replaceable class="parameter">increment</replaceable> ]
[ MINVALUE <replaceable class="parameter">minvalue</replaceable> | NO MINVALUE ] [ MAXVALUE <replaceable class="parameter">maxvalue</replaceable> | NO MAXVALUE ]
@@ -92,6 +92,16 @@ SELECT * FROM <replaceable>name</replaceable>;
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>LOGGED</literal></term>
+ <listitem>
+ <para>
+ If specified, the sequence is created as an permanent sequence. Changes
+ to permanent sequences are written to the write-ahead log.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>UNLOGGED</literal></term>
<listitem>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 02f31d2d6f..29dfd68dc8 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name</replaceable> ( [
+CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | LOGGED | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name</replaceable> ( [
{ <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [ STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN | DEFAULT } ] [ COMPRESSION <replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
| <replaceable>table_constraint</replaceable>
| LIKE <replaceable>source_table</replaceable> [ <replaceable>like_option</replaceable> ... ] }
@@ -34,7 +34,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
[ ON COMMIT { PRESERVE ROWS | DELETE ROWS | DROP } ]
[ TABLESPACE <replaceable class="parameter">tablespace_name</replaceable> ]
-CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name</replaceable>
+CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | LOGGED | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name</replaceable>
OF <replaceable class="parameter">type_name</replaceable> [ (
{ <replaceable class="parameter">column_name</replaceable> [ WITH OPTIONS ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
| <replaceable>table_constraint</replaceable> }
@@ -46,7 +46,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
[ ON COMMIT { PRESERVE ROWS | DELETE ROWS | DROP } ]
[ TABLESPACE <replaceable class="parameter">tablespace_name</replaceable> ]
-CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name</replaceable>
+CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | LOGGED | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name</replaceable>
PARTITION OF <replaceable class="parameter">parent_table</replaceable> [ (
{ <replaceable class="parameter">column_name</replaceable> [ WITH OPTIONS ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
| <replaceable>table_constraint</replaceable> }
@@ -203,6 +203,22 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
</listitem>
</varlistentry>
+ <varlistentry id="sql-createtable-logged">
+ <term><literal>LOGGED</literal></term>
+ <listitem>
+ <para>
+ If specified, the table is created as an permanent table. Data written
+ to permanent tables is written to the write-ahead log (see
+ <xref linkend="wal"/>).
+ </para>
+
+ <para>
+ If this is specified, any sequences created together with the permanent
+ table (for identity or serial columns) are also created as permanent.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="sql-createtable-unlogged">
<term><literal>UNLOGGED</literal></term>
<listitem>
diff --git a/doc/src/sgml/ref/create_table_as.sgml b/doc/src/sgml/ref/create_table_as.sgml
index 8429333e3a..4f250f059c 100644
--- a/doc/src/sgml/ref/create_table_as.sgml
+++ b/doc/src/sgml/ref/create_table_as.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable>table_name</replaceable>
+CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | LOGGED | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable>table_name</replaceable>
[ (<replaceable>column_name</replaceable> [, ...] ) ]
[ USING <replaceable class="parameter">method</replaceable> ]
[ WITH ( <replaceable class="parameter">storage_parameter</replaceable> [= <replaceable class="parameter">value</replaceable>] [, ... ] ) | WITHOUT OIDS ]
@@ -86,6 +86,16 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>LOGGED</literal></term>
+ <listitem>
+ <para>
+ If specified, the table is created as a permanent table.
+ Refer to <xref linkend="sql-createtable"/> for details.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>UNLOGGED</literal></term>
<listitem>
diff --git a/doc/src/sgml/ref/select_into.sgml b/doc/src/sgml/ref/select_into.sgml
index 82a77784b9..d818c620fc 100644
--- a/doc/src/sgml/ref/select_into.sgml
+++ b/doc/src/sgml/ref/select_into.sgml
@@ -24,7 +24,7 @@ PostgreSQL documentation
[ WITH [ RECURSIVE ] <replaceable class="parameter">with_query</replaceable> [, ...] ]
SELECT [ ALL | DISTINCT [ ON ( <replaceable class="parameter">expression</replaceable> [, ...] ) ] ]
* | <replaceable class="parameter">expression</replaceable> [ [ AS ] <replaceable class="parameter">output_name</replaceable> ] [, ...]
- INTO [ TEMPORARY | TEMP | UNLOGGED ] [ TABLE ] <replaceable class="parameter">new_table</replaceable>
+ INTO [ TEMPORARY | TEMP | LOGGED | UNLOGGED ] [ TABLE ] <replaceable class="parameter">new_table</replaceable>
[ FROM <replaceable class="parameter">from_item</replaceable> [, ...] ]
[ WHERE <replaceable class="parameter">condition</replaceable> ]
[ GROUP BY <replaceable class="parameter">expression</replaceable> [, ...] ]
@@ -75,6 +75,16 @@ SELECT [ ALL | DISTINCT [ ON ( <replaceable class="parameter">expression</replac
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>LOGGED</literal></term>
+ <listitem>
+ <para>
+ If specified, the table is created as a permanent table. Refer
+ to <xref linkend="sql-createtable"/> for details.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">new_table</replaceable></term>
<listitem>
--
2.43.0
v2-0003-Support-LOGGED-UNLOGGED-for-partitioned-tables.patchtext/x-diff; charset=us-asciiDownload
From de44be520f8c157dc558f63dd4059d80b6ad00e9 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 2 May 2024 13:45:18 +0900
Subject: [PATCH v2 3/4] Support LOGGED/UNLOGGED for partitioned tables
When using ALTER TABLE SET LOGGED/UNLOGGED, indexes and sequences that
are owned by the partitioned table changed need to have their
relpersistence also changed.
This patch does not apply recursion when using ALTER TABLE on a
partitioned table: only new partitions inherit the loggedness of the
parent if the query does not give a persistence (either LOGGED or
UNLOGGED).
---
src/backend/commands/tablecmds.c | 118 +++++++++++++++++++++-
src/test/regress/expected/alter_table.out | 88 ++++++++++++++++
src/test/regress/sql/alter_table.sql | 37 +++++++
doc/src/sgml/ref/alter_table.sgml | 6 ++
doc/src/sgml/ref/create_table.sgml | 5 +
5 files changed, 249 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 97ba34f0d5..8854e31f14 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -602,6 +602,7 @@ static ObjectAddress ATExecClusterOn(Relation rel, const char *indexName,
static void ATExecDropCluster(Relation rel, LOCKMODE lockmode);
static void ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname);
static void ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethod);
+static void ATExecSetPersistenceNoStorage(Relation rel, char newrelpersistence);
static void ATPrepSetPersistence(AlteredTableInfo *tab, Relation rel,
bool toLogged);
static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel,
@@ -811,13 +812,39 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
}
/*
- * If the grammar did not specify a relpersistence, assume that the
- * relation is permanent. Note that this is done before selecting
- * the relation's tablespace, as this change may impact the tablespace
- * location depending on the persistence set here.
+ * If the grammar did not specify a relpersistence, determine which one
+ * to use depending on the relation to create. Note that this is done
+ * before selecting the relation's tablespace, as this change may impact
+ * the tablespace location depending on the persistence set here.
+ *
+ * If the relation is not partitioned, assume that it is permanent.
+ *
+ * A partition inherits the persistence of its partitioned table, it the
+ * latter is unlogged or logged as the namespace where the relation will
+ * be created is known. This property cannot be enforced for temporary
+ * partitioned tables because the namespace of the relation is locked
+ * before it is possible to know the inheritance tree of this new
+ * relation, when its RangeVar is locked earlier when transforming the
+ * CreateStmt query.
*/
if (stmt->relation->relpersistence == RELPERSISTENCE_INVALID)
- stmt->relation->relpersistence = RELPERSISTENCE_PERMANENT;
+ {
+ if (stmt->partbound)
+ {
+ Oid parentOid = linitial_oid(inheritOids);
+ char relpersistence = get_rel_persistence(parentOid);
+
+ Assert(list_length(inheritOids) == 1);
+ /*
+ * The parent's persistence is logged or unlogged, so rely on
+ * it when creating the new relation.
+ */
+ if (relpersistence != RELPERSISTENCE_TEMP)
+ stmt->relation->relpersistence = relpersistence;
+ }
+ else
+ stmt->relation->relpersistence = RELPERSISTENCE_PERMANENT;
+ }
/*
* Select tablespace to use: an explicitly indicated one, or (in the case
@@ -5426,6 +5453,14 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
break;
case AT_SetLogged: /* SET LOGGED */
case AT_SetUnLogged: /* SET UNLOGGED */
+
+ /*
+ * Only do this for partitioned tables, for which this is just a
+ * catalog change. Tables with storage are handled by Phase 3.
+ */
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
+ tab->chgPersistence)
+ ATExecSetPersistenceNoStorage(rel, tab->newrelpersistence);
break;
case AT_DropOids: /* SET WITHOUT OIDS */
/* nothing to do here, oid columns don't exist anymore */
@@ -15542,6 +15577,79 @@ ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethodId)
table_close(pg_class, RowExclusiveLock);
}
+/*
+ * Special handling of ALTER TABLE SET LOGGED/UNLOGGED for relations with no
+ * storage that have an interest in changing their persistence.
+ *
+ * Since these have no storage, setting the persistence to permanent or
+ * unlogged is a catalog-only operation. This needs to switch the
+ * persistence of all sequences and indexes related to this relation.
+ */
+static void
+ATExecSetPersistenceNoStorage(Relation rel, char newrelpersistence)
+{
+ Relation pg_class;
+ HeapTuple tuple;
+ List *reloids; /* for indexes and sequences */
+ ListCell *elt;
+ Form_pg_class rd_rel;
+ Oid reloid = RelationGetRelid(rel);
+
+ /*
+ * Shouldn't be called on relations having storage; these are processed in
+ * phase 3.
+ */
+ Assert(!RELKIND_HAS_STORAGE(rel->rd_rel->relkind));
+
+ /* Get a modifiable copy of the relation's pg_class row. */
+ pg_class = table_open(RelationRelationId, RowExclusiveLock);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", reloid);
+ rd_rel = (Form_pg_class) GETSTRUCT(tuple);
+
+ /* Leave if no update required */
+ if (rd_rel->relpersistence == newrelpersistence)
+ {
+ heap_freetuple(tuple);
+ table_close(pg_class, RowExclusiveLock);
+ return;
+ }
+
+ /* Update the pg_class row. */
+ rd_rel->relpersistence = newrelpersistence;
+ CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+ InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(rel), 0);
+
+ heap_freetuple(tuple);
+
+ /* Update the per-sequence and per-index relpersistence */
+ reloids = getOwnedSequences(reloid);
+ reloids = list_union_oid(reloids, RelationGetIndexList(rel));
+ foreach(elt, reloids)
+ {
+ Oid classoid = lfirst_oid(elt);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(classoid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", classoid);
+ rd_rel = (Form_pg_class) GETSTRUCT(tuple);
+
+ rd_rel->relpersistence = newrelpersistence;
+ CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+ InvokeObjectPostAlterHook(RelationRelationId, classoid, 0);
+
+ heap_freetuple(tuple);
+ }
+
+ /* Make sure the persistence changes are visible */
+ CommandCounterIncrement();
+
+ table_close(pg_class, RowExclusiveLock);
+}
+
/*
* ALTER TABLE SET TABLESPACE
*/
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 7666c76238..5071e7d963 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3597,6 +3597,94 @@ ALTER TABLE logged1 SET UNLOGGED; -- silently do nothing
DROP TABLE logged3;
DROP TABLE logged2;
DROP TABLE logged1;
+-- SET LOGGED/UNLOGGED with partitioned tables
+CREATE TABLE logged_part_1(f1 SERIAL PRIMARY KEY)
+ PARTITION BY LIST (f1); -- has sequence, index
+CREATE TABLE logged_part_2(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES logged_part_1)
+ PARTITION BY LIST (f1); -- foreign key
+CREATE TABLE logged_part_3(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES logged_part_3)
+ PARTITION BY LIST (f1); -- self-referencing foreign key
+-- Check relpersistence of all the objects created.
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part'
+ ORDER BY relname;
+ relname | relpersistence
+----------------------+----------------
+ logged_part_1 | p
+ logged_part_1_f1_seq | p
+ logged_part_1_pkey | p
+ logged_part_2 | p
+ logged_part_2_f1_seq | p
+ logged_part_2_pkey | p
+ logged_part_3 | p
+ logged_part_3_f1_seq | p
+ logged_part_3_pkey | p
+(9 rows)
+
+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"
+ALTER TABLE logged_part_2 SET UNLOGGED;
+ALTER TABLE logged_part_3 SET UNLOGGED; -- skip self-referencing foreign key
+-- Re-check relpersistence of all the objects created.
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part'
+ ORDER BY relname;
+ relname | relpersistence
+----------------------+----------------
+ logged_part_1 | p
+ logged_part_1_f1_seq | p
+ logged_part_1_pkey | p
+ logged_part_2 | u
+ logged_part_2_f1_seq | u
+ logged_part_2_pkey | u
+ logged_part_3 | u
+ logged_part_3_f1_seq | u
+ logged_part_3_pkey | u
+(9 rows)
+
+ALTER TABLE logged_part_1 SET LOGGED; -- no-op
+ALTER TABLE logged_part_2 SET LOGGED;
+ALTER TABLE logged_part_3 SET LOGGED; -- skip self-referencing foreign key
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_[2|3]'
+ ORDER BY relname;
+ relname | relpersistence
+----------------------+----------------
+ logged_part_2 | p
+ logged_part_2_f1_seq | p
+ logged_part_2_pkey | p
+ logged_part_3 | p
+ logged_part_3_f1_seq | p
+ logged_part_3_pkey | p
+(6 rows)
+
+-- Partitions
+CREATE TABLE logged_part_2_1 PARTITION OF logged_part_2
+ FOR VALUES IN (1); -- permanent, inherited
+CREATE UNLOGGED TABLE logged_part_2_2 PARTITION OF logged_part_2
+ FOR VALUES IN (2); -- unlogged, not inherited
+ALTER TABLE logged_part_2 SET UNLOGGED;
+CREATE TABLE logged_part_2_3 PARTITION OF logged_part_2
+ FOR VALUES IN (3); -- unlogged, inherited
+CREATE LOGGED TABLE logged_part_2_4 PARTITION OF logged_part_2
+ FOR VALUES IN (4); -- logged, not inherited
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_2'
+ ORDER BY relname;
+ relname | relpersistence
+----------------------+----------------
+ logged_part_2 | u
+ logged_part_2_1 | p
+ logged_part_2_1_pkey | p
+ logged_part_2_2 | u
+ logged_part_2_2_pkey | u
+ logged_part_2_3 | u
+ logged_part_2_3_pkey | u
+ logged_part_2_4 | p
+ logged_part_2_4_pkey | p
+ logged_part_2_f1_seq | u
+ logged_part_2_pkey | u
+(11 rows)
+
+DROP TABLE logged_part_3;
+DROP TABLE logged_part_2;
+DROP TABLE logged_part_1;
-- test ADD COLUMN IF NOT EXISTS
CREATE TABLE test_add_column(c1 integer);
\d test_add_column
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 9df5a63bdf..30aa62d256 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2259,6 +2259,43 @@ DROP TABLE logged3;
DROP TABLE logged2;
DROP TABLE logged1;
+-- SET LOGGED/UNLOGGED with partitioned tables
+CREATE TABLE logged_part_1(f1 SERIAL PRIMARY KEY)
+ PARTITION BY LIST (f1); -- has sequence, index
+CREATE TABLE logged_part_2(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES logged_part_1)
+ PARTITION BY LIST (f1); -- foreign key
+CREATE TABLE logged_part_3(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES logged_part_3)
+ PARTITION BY LIST (f1); -- self-referencing foreign key
+-- Check relpersistence of all the objects created.
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part'
+ ORDER BY relname;
+ALTER TABLE logged_part_1 SET UNLOGGED; -- fails as a foreign-key exists
+ALTER TABLE logged_part_2 SET UNLOGGED;
+ALTER TABLE logged_part_3 SET UNLOGGED; -- skip self-referencing foreign key
+-- Re-check relpersistence of all the objects created.
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part'
+ ORDER BY relname;
+ALTER TABLE logged_part_1 SET LOGGED; -- no-op
+ALTER TABLE logged_part_2 SET LOGGED;
+ALTER TABLE logged_part_3 SET LOGGED; -- skip self-referencing foreign key
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_[2|3]'
+ ORDER BY relname;
+-- Partitions
+CREATE TABLE logged_part_2_1 PARTITION OF logged_part_2
+ FOR VALUES IN (1); -- permanent, inherited
+CREATE UNLOGGED TABLE logged_part_2_2 PARTITION OF logged_part_2
+ FOR VALUES IN (2); -- unlogged, not inherited
+ALTER TABLE logged_part_2 SET UNLOGGED;
+CREATE TABLE logged_part_2_3 PARTITION OF logged_part_2
+ FOR VALUES IN (3); -- unlogged, inherited
+CREATE LOGGED TABLE logged_part_2_4 PARTITION OF logged_part_2
+ FOR VALUES IN (4); -- logged, not inherited
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_2'
+ ORDER BY relname;
+DROP TABLE logged_part_3;
+DROP TABLE logged_part_2;
+DROP TABLE logged_part_1;
+
-- test ADD COLUMN IF NOT EXISTS
CREATE TABLE test_add_column(c1 integer);
\d test_add_column
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index ebd8c62038..7937194462 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -803,6 +803,12 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
(for identity or serial columns). However, it is also possible to
change the persistence of such sequences separately.
</para>
+
+ <para>
+ Setting this property for a partitioned table has no direct effect,
+ because such tables have no storage of their own, but the configured
+ value will be inherited by newly-created partitions.
+ </para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 29dfd68dc8..f65c6d14c7 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -216,6 +216,11 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
If this is specified, any sequences created together with the permanent
table (for identity or serial columns) are also created as permanent.
</para>
+
+ <para>
+ When applied to a partitioned table, newly-created partitions and
+ their objects (sequences and indexes) will inherit this property.
+ </para>
</listitem>
</varlistentry>
--
2.43.0
v2-0004-Recurse-ALTER-TABLE-SET-LOGGED-UNLOGGED-for-parti.patchtext/x-diff; charset=us-asciiDownload
From 0f2d517d28042a16d3ebd89d18327f0cfc7525c5 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 2 May 2024 14:43:56 +0900
Subject: [PATCH v2 4/4] Recurse ALTER TABLE SET LOGGED/UNLOGGED for
partitioned tables
This commit recurses the command of $subject to apply on all the
partitions of a partitioned table, except if ONLY is used. Regression
tests are expanded for both cases, with multiple levels of partitioning.
---
src/backend/commands/tablecmds.c | 2 +
src/test/regress/expected/alter_table.out | 91 +++++++++++++++++++----
src/test/regress/sql/alter_table.sql | 15 ++++
doc/src/sgml/ref/alter_table.sgml | 6 +-
4 files changed, 97 insertions(+), 17 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8854e31f14..8f18b7aa39 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5075,6 +5075,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change persistence setting twice")));
ATPrepSetPersistence(tab, rel, true);
+ ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
pass = AT_PASS_MISC;
break;
case AT_SetUnLogged: /* SET UNLOGGED */
@@ -5084,6 +5085,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change persistence setting twice")));
ATPrepSetPersistence(tab, rel, false);
+ ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
pass = AT_PASS_MISC;
break;
case AT_DropOids: /* SET WITHOUT OIDS */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 5071e7d963..8ec5c668c8 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3665,22 +3665,85 @@ CREATE TABLE logged_part_2_3 PARTITION OF logged_part_2
FOR VALUES IN (3); -- unlogged, inherited
CREATE LOGGED TABLE logged_part_2_4 PARTITION OF logged_part_2
FOR VALUES IN (4); -- logged, not inherited
+-- Partitions of partitions
+CREATE TABLE logged_part_2_56 PARTITION OF logged_part_2
+ FOR VALUES IN (5, 6) PARTITION BY LIST(f1);
+CREATE TABLE logged_part_2_5 PARTITION OF logged_part_2_56
+ FOR VALUES IN (5); -- unlogged, inherited
+CREATE LOGGED TABLE logged_part_2_6 PARTITION OF logged_part_2_56
+ FOR VALUES IN (6); -- logged, not inherited
SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_2'
ORDER BY relname;
- relname | relpersistence
-----------------------+----------------
- logged_part_2 | u
- logged_part_2_1 | p
- logged_part_2_1_pkey | p
- logged_part_2_2 | u
- logged_part_2_2_pkey | u
- logged_part_2_3 | u
- logged_part_2_3_pkey | u
- logged_part_2_4 | p
- logged_part_2_4_pkey | p
- logged_part_2_f1_seq | u
- logged_part_2_pkey | u
-(11 rows)
+ relname | relpersistence
+-----------------------+----------------
+ logged_part_2 | u
+ logged_part_2_1 | u
+ logged_part_2_1_pkey | u
+ logged_part_2_2 | u
+ logged_part_2_2_pkey | u
+ logged_part_2_3 | u
+ logged_part_2_3_pkey | u
+ logged_part_2_4 | p
+ logged_part_2_4_pkey | p
+ logged_part_2_5 | u
+ logged_part_2_56 | u
+ logged_part_2_56_pkey | u
+ logged_part_2_5_pkey | u
+ logged_part_2_6 | p
+ logged_part_2_6_pkey | p
+ logged_part_2_f1_seq | u
+ logged_part_2_pkey | u
+(17 rows)
+
+-- All partitions are logged.
+ALTER TABLE logged_part_2 SET LOGGED;
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_2'
+ ORDER BY relname;
+ relname | relpersistence
+-----------------------+----------------
+ logged_part_2 | p
+ logged_part_2_1 | p
+ logged_part_2_1_pkey | p
+ logged_part_2_2 | p
+ logged_part_2_2_pkey | p
+ logged_part_2_3 | p
+ logged_part_2_3_pkey | p
+ logged_part_2_4 | p
+ logged_part_2_4_pkey | p
+ logged_part_2_5 | p
+ logged_part_2_56 | p
+ logged_part_2_56_pkey | p
+ logged_part_2_5_pkey | p
+ logged_part_2_6 | p
+ logged_part_2_6_pkey | p
+ logged_part_2_f1_seq | p
+ logged_part_2_pkey | p
+(17 rows)
+
+-- Only the partitioned partition is unlogged.
+ALTER TABLE ONLY logged_part_2_56 SET UNLOGGED;
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_2'
+ ORDER BY relname;
+ relname | relpersistence
+-----------------------+----------------
+ logged_part_2 | p
+ logged_part_2_1 | p
+ logged_part_2_1_pkey | p
+ logged_part_2_2 | p
+ logged_part_2_2_pkey | p
+ logged_part_2_3 | p
+ logged_part_2_3_pkey | p
+ logged_part_2_4 | p
+ logged_part_2_4_pkey | p
+ logged_part_2_5 | p
+ logged_part_2_56 | u
+ logged_part_2_56_pkey | u
+ logged_part_2_5_pkey | p
+ logged_part_2_6 | p
+ logged_part_2_6_pkey | p
+ logged_part_2_f1_seq | p
+ logged_part_2_pkey | p
+(17 rows)
DROP TABLE logged_part_3;
DROP TABLE logged_part_2;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 30aa62d256..3393c67b9c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2290,6 +2290,21 @@ CREATE TABLE logged_part_2_3 PARTITION OF logged_part_2
FOR VALUES IN (3); -- unlogged, inherited
CREATE LOGGED TABLE logged_part_2_4 PARTITION OF logged_part_2
FOR VALUES IN (4); -- logged, not inherited
+-- Partitions of partitions
+CREATE TABLE logged_part_2_56 PARTITION OF logged_part_2
+ FOR VALUES IN (5, 6) PARTITION BY LIST(f1);
+CREATE TABLE logged_part_2_5 PARTITION OF logged_part_2_56
+ FOR VALUES IN (5); -- unlogged, inherited
+CREATE LOGGED TABLE logged_part_2_6 PARTITION OF logged_part_2_56
+ FOR VALUES IN (6); -- logged, not inherited
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_2'
+ ORDER BY relname;
+-- All partitions are logged.
+ALTER TABLE logged_part_2 SET LOGGED;
+SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_2'
+ ORDER BY relname;
+-- Only the partitioned partition is unlogged.
+ALTER TABLE ONLY logged_part_2_56 SET UNLOGGED;
SELECT relname, relpersistence FROM pg_class WHERE relname ~ '^logged_part_2'
ORDER BY relname;
DROP TABLE logged_part_3;
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 7937194462..79827796cc 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -805,9 +805,9 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
</para>
<para>
- Setting this property for a partitioned table has no direct effect,
- because such tables have no storage of their own, but the configured
- value will be inherited by newly-created partitions.
+ Setting this property on a partitioned table updates any partitions
+ attached to it, unless <literal>ONLY</literal> is specified. The
+ configured value is inherited by newly-created partitions.
</para>
</listitem>
</varlistentry>
--
2.43.0
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
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
On Thu, Aug 29, 2024 at 03:44:45PM +0900, Michael Paquier wrote:
On Tue, Aug 27, 2024 at 04:01:58PM -0500, Nathan Bossart wrote:
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.
+1
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.
IMHO continuing to allow partitioned tables to be marked UNLOGGED just
preserves the illusion that it does something. An ERROR could help dispel
that misconception.
--
nathan
On Thu, May 2, 2024 at 2:07 PM Michael Paquier <michael@paquier.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
On Thu, Aug 29, 2024 at 09:49:44AM -0500, Nathan Bossart wrote:
IMHO continuing to allow partitioned tables to be marked UNLOGGED just
preserves the illusion that it does something. An ERROR could help dispel
that misconception.
Okay. This is going to be disruptive if we do nothing about pg_dump,
unfortunately. How about tweaking dumpTableSchema() so as we'd never
issue UNLOGGED for a partitioned table? We could filter that out as
there is tbinfo->relkind.
--
Michael
On Tue, Sep 03, 2024 at 09:22:58AM +0900, Michael Paquier wrote:
On Thu, Aug 29, 2024 at 09:49:44AM -0500, Nathan Bossart wrote:
IMHO continuing to allow partitioned tables to be marked UNLOGGED just
preserves the illusion that it does something. An ERROR could help dispel
that misconception.Okay. This is going to be disruptive if we do nothing about pg_dump,
unfortunately. How about tweaking dumpTableSchema() so as we'd never
issue UNLOGGED for a partitioned table? We could filter that out as
there is tbinfo->relkind.
That's roughly what I had in mind.
--
nathan
On Mon, Sep 02, 2024 at 08:35:15PM -0500, Nathan Bossart wrote:
On Tue, Sep 03, 2024 at 09:22:58AM +0900, Michael Paquier wrote:
Okay. This is going to be disruptive if we do nothing about pg_dump,
unfortunately. How about tweaking dumpTableSchema() so as we'd never
issue UNLOGGED for a partitioned table? We could filter that out as
there is tbinfo->relkind.That's roughly what I had in mind.
An idea is attached. The pgbench bit was unexpected.
--
Michael
Attachments:
unlogged-part-v3.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b3cc6f8f69..b1cc635498 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -728,6 +728,12 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
else
partitioned = false;
+ if (relkind == RELKIND_PARTITIONED_TABLE &&
+ stmt->relation->relpersistence == RELPERSISTENCE_UNLOGGED)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("partitioned tables cannot be unlogged")));
+
/*
* Look up the namespace in which we are supposed to create the relation,
* check we have permission to create there, lock it against concurrent
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index dacb033e98..c60d2994f2 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15896,8 +15896,13 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
binary_upgrade_set_pg_class_oids(fout, q,
tbinfo->dobj.catId.oid);
+ /*
+ * PostgreSQL 18 has disabled UNLOGGED for partitioned tables, so
+ * ignore it when dumping if it was set in this case.
+ */
appendPQExpBuffer(q, "CREATE %s%s %s",
- tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED ?
+ tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED &&
+ tbinfo->relkind != RELKIND_PARTITIONED_TABLE ?
"UNLOGGED " : "",
reltypename,
qualrelname);
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 61618f2e18..dc4d7408cd 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4865,7 +4865,7 @@ initCreateTables(PGconn *con)
/* Construct new create table statement. */
printfPQExpBuffer(&query, "create%s table %s(%s)",
- unlogged_tables ? " unlogged" : "",
+ unlogged_tables && partition_method == PART_NONE ? " unlogged" : "",
ddl->table,
(scale >= SCALE_32BIT_THRESHOLD) ? ddl->bigcols : ddl->smcols);
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 284a7fb85c..2654891524 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -50,6 +50,8 @@ ERROR: cannot create temporary relation in non-temporary schema
LINE 1: CREATE TEMP TABLE public.temp_to_perm (a int primary key);
^
DROP TABLE unlogged1, public.unlogged2;
+CREATE UNLOGGED TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- fail
+ERROR: partitioned tables cannot be unlogged
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
ERROR: relation "as_select1" already exists
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 1fd4cbfa7e..5dbb25a092 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -30,6 +30,8 @@ CREATE TEMP TABLE pg_temp.doubly_temp (a int primary key); -- also OK
CREATE TEMP TABLE public.temp_to_perm (a int primary key); -- not OK
DROP TABLE unlogged1, public.unlogged2;
+CREATE UNLOGGED TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- fail
+
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 93b3f664f2..0859afd75e 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -220,6 +220,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
If this is specified, any sequences created together with the unlogged
table (for identity or serial columns) are also created as unlogged.
</para>
+
+ <para>
+ This option is not supported for partitioned tables.
+ </para>
</listitem>
</varlistentry>
On Tue, Sep 03, 2024 at 04:59:18PM +0900, Michael Paquier wrote:
An idea is attached. The pgbench bit was unexpected.
This works correctly for CREATE TABLE, but ALTER TABLE still succeeds.
Interestingly, it doesn't seem to actually change relpersistence for
partitioned tables. I think we might need to adjust
ATPrepChangePersistence().
--
nathan
On Tue, Sep 03, 2024 at 10:33:02AM -0500, Nathan Bossart wrote:
This works correctly for CREATE TABLE, but ALTER TABLE still succeeds.
Interestingly, it doesn't seem to actually change relpersistence for
partitioned tables. I think we might need to adjust
ATPrepChangePersistence().
Adjusting ATPrepChangePersistence() looks incorrect to me seeing that
we have all the machinery in ATSimplePermissions() at our disposal,
and that this routine decides that ATT_TABLE should map to both
RELKIND_RELATION and RELKIND_PARTITIONED_TABLE.
How about inventing a new ATT_PARTITIONED_TABLE and make a clean split
between both relkinds? I'd guess that blocking both SET LOGGED and
UNLOGGED for partitioned tables is the best move, even if it is
possible to block only one or the other, of course.
--
Michael
On Mon, Sep 09, 2024 at 03:56:14PM +0900, Michael Paquier wrote:
How about inventing a new ATT_PARTITIONED_TABLE and make a clean split
between both relkinds? I'd guess that blocking both SET LOGGED and
UNLOGGED for partitioned tables is the best move, even if it is
possible to block only one or the other, of course.
I gave it a try, and while it is much more invasive, it is also much
more consistent with the rest of the file.
Thoughts?
--
Michael
Attachments:
v4-0001-Introduce-ATT_PARTITIONED_TABLE-in-tablecmds.c.patchtext/x-diff; charset=us-asciiDownload
From fefb821c033784a01c39d99d477982ded1aebf40 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 10 Sep 2024 09:16:43 +0900
Subject: [PATCH v4 1/2] Introduce ATT_PARTITIONED_TABLE in tablecmds.c
---
src/backend/commands/tablecmds.c | 92 +++++++++++++++++---------------
1 file changed, 48 insertions(+), 44 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b3cc6f8f69..f6a6e3e148 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -330,6 +330,7 @@ struct DropRelationCallbackState
#define ATT_FOREIGN_TABLE 0x0020
#define ATT_PARTITIONED_INDEX 0x0040
#define ATT_SEQUENCE 0x0080
+#define ATT_PARTITIONED_TABLE 0x0100
/*
* ForeignTruncateInfo
@@ -4777,7 +4778,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
{
case AT_AddColumn: /* ADD COLUMN */
ATSimplePermissions(cmd->subtype, rel,
- ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE);
+ ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE);
ATPrepAddColumn(wqueue, rel, recurse, recursing, false, cmd,
lockmode, context);
/* Recursion occurs during execution phase */
@@ -4798,7 +4799,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
* substitutes default values into INSERTs before it expands
* rules.
*/
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
/* No command-specific prep needed */
pass = cmd->def ? AT_PASS_ADD_OTHERCONSTR : AT_PASS_DROP;
@@ -4806,19 +4807,19 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_CookedColumnDefault: /* add a pre-cooked default */
/* This is currently used only in CREATE TABLE */
/* (so the permission check really isn't necessary) */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* This command never recurses */
pass = AT_PASS_ADD_OTHERCONSTR;
break;
case AT_AddIdentity:
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
/* Set up recursion for phase 2; no other prep needed */
if (recurse)
cmd->recurse = true;
pass = AT_PASS_ADD_OTHERCONSTR;
break;
case AT_SetIdentity:
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
/* Set up recursion for phase 2; no other prep needed */
if (recurse)
cmd->recurse = true;
@@ -4826,82 +4827,82 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_MISC;
break;
case AT_DropIdentity:
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
/* Set up recursion for phase 2; no other prep needed */
if (recurse)
cmd->recurse = true;
pass = AT_PASS_DROP;
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
ATPrepDropNotNull(rel, recurse, recursing);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
pass = AT_PASS_DROP;
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* Need command-specific recursion decision */
ATPrepSetNotNull(wqueue, rel, cmd, recurse, recursing,
lockmode, context);
pass = AT_PASS_COL_ATTRS;
break;
case AT_CheckNotNull: /* check column is already marked NOT NULL */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
/* No command-specific prep needed */
pass = AT_PASS_COL_ATTRS;
break;
case AT_SetExpression: /* ALTER COLUMN SET EXPRESSION */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
pass = AT_PASS_SET_EXPRESSION;
break;
case AT_DropExpression: /* ALTER COLUMN DROP EXPRESSION */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
ATPrepDropExpression(rel, cmd, recurse, recursing, lockmode);
pass = AT_PASS_DROP;
break;
case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_SetOptions: /* ALTER COLUMN SET ( options ) */
case AT_ResetOptions: /* ALTER COLUMN RESET ( options ) */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE);
/* This command never recurses */
pass = AT_PASS_MISC;
break;
case AT_SetStorage: /* ALTER COLUMN SET STORAGE */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_SetCompression: /* ALTER COLUMN SET COMPRESSION */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW);
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_DropColumn: /* DROP COLUMN */
ATSimplePermissions(cmd->subtype, rel,
- ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE);
+ ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE);
ATPrepDropColumn(wqueue, rel, recurse, recursing, cmd,
lockmode, context);
/* Recursion occurs during execution phase */
pass = AT_PASS_DROP;
break;
case AT_AddIndex: /* ADD INDEX */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE);
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_ADD_INDEX;
break;
case AT_AddConstraint: /* ADD CONSTRAINT */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* Recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
if (recurse)
@@ -4909,13 +4910,13 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_ADD_CONSTR;
break;
case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE);
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_ADD_INDEXCONSTR;
break;
case AT_DropConstraint: /* DROP CONSTRAINT */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
ATCheckPartitionsNotInUse(rel, lockmode);
/* Other recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
@@ -4925,7 +4926,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_AlterColumnType: /* ALTER COLUMN TYPE */
ATSimplePermissions(cmd->subtype, rel,
- ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE);
+ ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE);
/* See comments for ATPrepAlterColumnType */
cmd = ATParseTransformCmd(wqueue, tab, rel, cmd, recurse, lockmode,
AT_PASS_UNSET, context);
@@ -4948,14 +4949,14 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_ClusterOn: /* CLUSTER ON */
case AT_DropCluster: /* SET WITHOUT CLUSTER */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW);
/* These commands never recurse */
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_SetLogged: /* SET LOGGED */
case AT_SetUnLogged: /* SET UNLOGGED */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_SEQUENCE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_SEQUENCE);
if (tab->chgPersistence)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -4964,11 +4965,11 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_MISC;
break;
case AT_DropOids: /* SET WITHOUT OIDS */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
pass = AT_PASS_DROP;
break;
case AT_SetAccessMethod: /* SET ACCESS METHOD */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW);
/* check if another access method change was already requested */
if (tab->chgAccessMethod)
@@ -4980,7 +4981,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_MISC; /* does not matter; no work in Phase 2 */
break;
case AT_SetTableSpace: /* SET TABLESPACE */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW | ATT_INDEX |
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW | ATT_INDEX |
ATT_PARTITIONED_INDEX);
/* This command never recurses */
ATPrepSetTableSpace(tab, rel, cmd->name, lockmode);
@@ -4989,30 +4990,30 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_SetRelOptions: /* SET (...) */
case AT_ResetRelOptions: /* RESET (...) */
case AT_ReplaceRelOptions: /* reset them all, then set just these */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_VIEW | ATT_MATVIEW | ATT_INDEX);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_VIEW | ATT_MATVIEW | ATT_INDEX);
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_AddInherit: /* INHERIT */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* This command never recurses */
ATPrepAddInherit(rel);
pass = AT_PASS_MISC;
break;
case AT_DropInherit: /* NO INHERIT */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_AlterConstraint: /* ALTER CONSTRAINT */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE);
/* Recursion occurs during execution phase */
pass = AT_PASS_MISC;
break;
case AT_ValidateConstraint: /* VALIDATE CONSTRAINT */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* Recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
if (recurse)
@@ -5020,7 +5021,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_MISC;
break;
case AT_ReplicaIdentity: /* REPLICA IDENTITY ... */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW);
pass = AT_PASS_MISC;
/* This command never recurses */
/* No command-specific prep needed */
@@ -5033,7 +5034,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_DisableTrig: /* DISABLE TRIGGER variants */
case AT_DisableTrigAll:
case AT_DisableTrigUser:
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* Set up recursion for phase 2; no other prep needed */
if (recurse)
cmd->recurse = true;
@@ -5049,7 +5050,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_DisableRowSecurity:
case AT_ForceRowSecurity:
case AT_NoForceRowSecurity:
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE);
/* These commands never recurse */
/* No command-specific prep needed */
pass = AT_PASS_MISC;
@@ -5060,17 +5061,17 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_MISC;
break;
case AT_AttachPartition:
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_INDEX);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_PARTITIONED_INDEX);
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_DetachPartition:
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE);
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_DetachPartitionFinalize:
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE);
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
@@ -6486,9 +6487,11 @@ ATSimplePermissions(AlterTableType cmdtype, Relation rel, int allowed_targets)
switch (rel->rd_rel->relkind)
{
case RELKIND_RELATION:
- case RELKIND_PARTITIONED_TABLE:
actual_target = ATT_TABLE;
break;
+ case RELKIND_PARTITIONED_TABLE:
+ actual_target = ATT_PARTITIONED_TABLE;
+ break;
case RELKIND_VIEW:
actual_target = ATT_VIEW;
break;
@@ -6982,7 +6985,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
- ATSimplePermissions((*cmd)->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions((*cmd)->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
if (rel->rd_rel->relispartition && !recursing)
ereport(ERROR,
@@ -8906,7 +8909,7 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
- ATSimplePermissions(AT_DropColumn, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(AT_DropColumn, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* Initialize addrs on the first invocation */
Assert(!recursing || addrs != NULL);
@@ -9395,7 +9398,7 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
- ATSimplePermissions(AT_AddConstraint, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(AT_AddConstraint, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/*
* Call AddRelationNewConstraints to do the work, making sure it works on
@@ -12400,7 +12403,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
- ATSimplePermissions(AT_DropConstraint, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(AT_DropConstraint, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
conrel = table_open(ConstraintRelationId, RowExclusiveLock);
@@ -15481,7 +15484,7 @@ ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode)
* Must be owner of both parent and child -- child was checked by
* ATSimplePermissions call in ATPrepCmd
*/
- ATSimplePermissions(AT_AddInherit, parent_rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(AT_AddInherit, parent_rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* Permanent rels cannot inherit from temporary ones */
if (parent_rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
@@ -18326,7 +18329,8 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd,
* Must be owner of both parent and source table -- parent was checked by
* ATSimplePermissions call in ATPrepCmd
*/
- ATSimplePermissions(AT_AttachPartition, attachrel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(AT_AttachPartition, attachrel,
+ ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* A partition can only have one parent */
if (attachrel->rd_rel->relispartition)
--
2.45.2
v4-0002-Remove-support-for-ALTER-TABLE-.-SET-UN-LOGGED-on.patchtext/x-diff; charset=us-asciiDownload
From 996a52b7d49607ddd62bbe0534718bcba894ea9a Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 10 Sep 2024 09:35:00 +0900
Subject: [PATCH v4 2/2] Remove support for ALTER TABLE .. SET [UN]LOGGED on
partitioned tables
---
src/backend/commands/tablecmds.c | 8 +++++++-
src/bin/pg_dump/pg_dump.c | 7 ++++++-
src/bin/pgbench/pgbench.c | 2 +-
src/test/regress/expected/create_table.out | 10 ++++++++++
src/test/regress/sql/create_table.sql | 6 ++++++
doc/src/sgml/ref/alter_table.sgml | 4 ++++
doc/src/sgml/ref/create_table.sgml | 4 ++++
7 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f6a6e3e148..f5d7f2b9f8 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -729,6 +729,12 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
else
partitioned = false;
+ if (relkind == RELKIND_PARTITIONED_TABLE &&
+ stmt->relation->relpersistence == RELPERSISTENCE_UNLOGGED)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("partitioned tables cannot be unlogged")));
+
/*
* Look up the namespace in which we are supposed to create the relation,
* check we have permission to create there, lock it against concurrent
@@ -4956,7 +4962,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_SetLogged: /* SET LOGGED */
case AT_SetUnLogged: /* SET UNLOGGED */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_SEQUENCE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_SEQUENCE);
if (tab->chgPersistence)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 546e7e4ce1..c207436f93 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15899,8 +15899,13 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
binary_upgrade_set_pg_class_oids(fout, q,
tbinfo->dobj.catId.oid);
+ /*
+ * PostgreSQL 18 has disabled UNLOGGED for partitioned tables, so
+ * ignore it when dumping if it was set in this case.
+ */
appendPQExpBuffer(q, "CREATE %s%s %s",
- tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED ?
+ tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED &&
+ tbinfo->relkind != RELKIND_PARTITIONED_TABLE ?
"UNLOGGED " : "",
reltypename,
qualrelname);
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 61618f2e18..dc4d7408cd 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4865,7 +4865,7 @@ initCreateTables(PGconn *con)
/* Construct new create table statement. */
printfPQExpBuffer(&query, "create%s table %s(%s)",
- unlogged_tables ? " unlogged" : "",
+ unlogged_tables && partition_method == PART_NONE ? " unlogged" : "",
ddl->table,
(scale >= SCALE_32BIT_THRESHOLD) ? ddl->bigcols : ddl->smcols);
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 284a7fb85c..c45e02d42f 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -50,6 +50,16 @@ ERROR: cannot create temporary relation in non-temporary schema
LINE 1: CREATE TEMP TABLE public.temp_to_perm (a int primary key);
^
DROP TABLE unlogged1, public.unlogged2;
+CREATE UNLOGGED TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- fail
+ERROR: partitioned tables cannot be unlogged
+CREATE TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- ok
+ALTER TABLE unlogged1 SET LOGGED; -- fails
+ERROR: ALTER action SET LOGGED cannot be performed on relation "unlogged1"
+DETAIL: This operation is not supported for partitioned tables.
+ALTER TABLE unlogged1 SET UNLOGGED; -- fails
+ERROR: ALTER action SET UNLOGGED cannot be performed on relation "unlogged1"
+DETAIL: This operation is not supported for partitioned tables.
+DROP TABLE unlogged1;
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
ERROR: relation "as_select1" already exists
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 1fd4cbfa7e..37a227148e 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -30,6 +30,12 @@ CREATE TEMP TABLE pg_temp.doubly_temp (a int primary key); -- also OK
CREATE TEMP TABLE public.temp_to_perm (a int primary key); -- not OK
DROP TABLE unlogged1, public.unlogged2;
+CREATE UNLOGGED TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- fail
+CREATE TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- ok
+ALTER TABLE unlogged1 SET LOGGED; -- fails
+ALTER TABLE unlogged1 SET UNLOGGED; -- fails
+DROP TABLE unlogged1;
+
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1a49f321cf..19f29a0470 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -795,6 +795,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
(for identity or serial columns). However, it is also possible to
change the persistence of such sequences separately.
</para>
+
+ <para>
+ This form is not supported for partitioned tables.
+ </para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 93b3f664f2..65715b13b8 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -220,6 +220,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
If this is specified, any sequences created together with the unlogged
table (for identity or serial columns) are also created as unlogged.
</para>
+
+ <para>
+ This form is not supported for partitioned tables.
+ </para>
</listitem>
</varlistentry>
--
2.45.2
On Tue, Sep 10, 2024 at 09:42:31AM +0900, Michael Paquier wrote:
On Mon, Sep 09, 2024 at 03:56:14PM +0900, Michael Paquier wrote:
How about inventing a new ATT_PARTITIONED_TABLE and make a clean split
between both relkinds? I'd guess that blocking both SET LOGGED and
UNLOGGED for partitioned tables is the best move, even if it is
possible to block only one or the other, of course.I gave it a try, and while it is much more invasive, it is also much
more consistent with the rest of the file.
This looks reasonable to me. Could we also use ATT_PARTITIONED_TABLE to
remove the partitioned table check in ATExecAddIndexConstraint()?
--
nathan
On Wed, Sep 18, 2024 at 10:17:47AM -0500, Nathan Bossart wrote:
Could we also use ATT_PARTITIONED_TABLE to remove the partitioned table
check in ATExecAddIndexConstraint()?
Eh, never mind. That ends up being gross because you have to redo the
relation type check in a few places.
--
nathan
Attachments:
add_constraint_using_index_on_part_table.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9966d1f53c..777de48c69 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4922,7 +4922,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_ADD_CONSTR;
break;
case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE);
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_ADD_INDEXCONSTR;
@@ -5583,6 +5583,7 @@ ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
pass = AT_PASS_ADD_INDEX;
break;
case AT_AddIndexConstraint:
+ ATSimplePermissions(cmd2->subtype, rel, ATT_TABLE);
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_ADD_INDEXCONSTR;
@@ -5596,6 +5597,7 @@ ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
case CONSTR_PRIMARY:
case CONSTR_UNIQUE:
case CONSTR_EXCLUSION:
+ ATSimplePermissions(cmd2->subtype, rel, ATT_TABLE);
pass = AT_PASS_ADD_INDEXCONSTR;
break;
default:
@@ -9208,15 +9210,6 @@ ATExecAddIndexConstraint(AlteredTableInfo *tab, Relation rel,
Assert(OidIsValid(index_oid));
Assert(stmt->isconstraint);
- /*
- * Doing this on partitioned tables is not a simple feature to implement,
- * so let's punt for now.
- */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("ALTER TABLE / ADD CONSTRAINT USING INDEX is not supported on partitioned tables")));
-
indexRel = index_open(index_oid, AccessShareLock);
indexName = pstrdup(RelationGetRelationName(indexRel));
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index cf6eac5734..bdb7731543 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1593,7 +1593,8 @@ DROP TABLE cwi_test;
CREATE TABLE cwi_test(a int) PARTITION BY hash (a);
create unique index on cwi_test (a);
alter table cwi_test add primary key using index cwi_test_a_idx ;
-ERROR: ALTER TABLE / ADD CONSTRAINT USING INDEX is not supported on partitioned tables
+ERROR: ALTER action ADD CONSTRAINT cannot be performed on relation "cwi_test"
+DETAIL: This operation is not supported for partitioned tables.
DROP TABLE cwi_test;
-- PRIMARY KEY constraint cannot be backed by a NULLS NOT DISTINCT index
CREATE TABLE cwi_test(a int, b int);
On Wed, Sep 18, 2024 at 10:58:34AM -0500, Nathan Bossart wrote:
On Wed, Sep 18, 2024 at 10:17:47AM -0500, Nathan Bossart wrote:
Could we also use ATT_PARTITIONED_TABLE to remove the partitioned table
check in ATExecAddIndexConstraint()?Eh, never mind. That ends up being gross because you have to redo the
relation type check in a few places.
I did not notice this one. I have to admit that the error message
consistency is welcome, not the extra checks required at
transformation.
--
Michael
On Thu, Sep 19, 2024 at 08:06:19AM +0900, Michael Paquier wrote:
I did not notice this one. I have to admit that the error message
consistency is welcome, not the extra checks required at
transformation.
I have applied 0001 for now to add ATT_PARTITIONED_TABLE. Attached is
the remaining piece.
--
Michael
Attachments:
v5-0002-Remove-support-for-ALTER-TABLE-.-SET-UN-LOGGED-on.patchtext/x-diff; charset=us-asciiDownload
From f29099559497e05419bece407fb0be17e35d52e8 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 19 Sep 2024 13:07:45 +0900
Subject: [PATCH v5] Remove support for ALTER TABLE .. SET [UN]LOGGED on
partitioned tables
---
src/backend/commands/tablecmds.c | 9 +++++++--
src/bin/pg_dump/pg_dump.c | 7 ++++++-
src/bin/pgbench/pgbench.c | 2 +-
src/test/regress/expected/create_table.out | 10 ++++++++++
src/test/regress/sql/create_table.sql | 6 ++++++
doc/src/sgml/ref/alter_table.sgml | 4 ++++
doc/src/sgml/ref/create_table.sgml | 4 ++++
7 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2d703aa22e..5deeb6490c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -735,6 +735,12 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
else
partitioned = false;
+ if (relkind == RELKIND_PARTITIONED_TABLE &&
+ stmt->relation->relpersistence == RELPERSISTENCE_UNLOGGED)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("partitioned tables cannot be unlogged")));
+
/*
* Look up the namespace in which we are supposed to create the relation,
* check we have permission to create there, lock it against concurrent
@@ -4989,8 +4995,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_SetLogged: /* SET LOGGED */
case AT_SetUnLogged: /* SET UNLOGGED */
- ATSimplePermissions(cmd->subtype, rel,
- ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_SEQUENCE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_SEQUENCE);
if (tab->chgPersistence)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 130b80775d..24a8067ed6 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15909,8 +15909,13 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
binary_upgrade_set_pg_class_oids(fout, q,
tbinfo->dobj.catId.oid);
+ /*
+ * PostgreSQL 18 has disabled UNLOGGED for partitioned tables, so
+ * ignore it when dumping if it was set in this case.
+ */
appendPQExpBuffer(q, "CREATE %s%s %s",
- tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED ?
+ tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED &&
+ tbinfo->relkind != RELKIND_PARTITIONED_TABLE ?
"UNLOGGED " : "",
reltypename,
qualrelname);
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 61618f2e18..dc4d7408cd 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4865,7 +4865,7 @@ initCreateTables(PGconn *con)
/* Construct new create table statement. */
printfPQExpBuffer(&query, "create%s table %s(%s)",
- unlogged_tables ? " unlogged" : "",
+ unlogged_tables && partition_method == PART_NONE ? " unlogged" : "",
ddl->table,
(scale >= SCALE_32BIT_THRESHOLD) ? ddl->bigcols : ddl->smcols);
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 284a7fb85c..c45e02d42f 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -50,6 +50,16 @@ ERROR: cannot create temporary relation in non-temporary schema
LINE 1: CREATE TEMP TABLE public.temp_to_perm (a int primary key);
^
DROP TABLE unlogged1, public.unlogged2;
+CREATE UNLOGGED TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- fail
+ERROR: partitioned tables cannot be unlogged
+CREATE TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- ok
+ALTER TABLE unlogged1 SET LOGGED; -- fails
+ERROR: ALTER action SET LOGGED cannot be performed on relation "unlogged1"
+DETAIL: This operation is not supported for partitioned tables.
+ALTER TABLE unlogged1 SET UNLOGGED; -- fails
+ERROR: ALTER action SET UNLOGGED cannot be performed on relation "unlogged1"
+DETAIL: This operation is not supported for partitioned tables.
+DROP TABLE unlogged1;
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
ERROR: relation "as_select1" already exists
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 1fd4cbfa7e..37a227148e 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -30,6 +30,12 @@ CREATE TEMP TABLE pg_temp.doubly_temp (a int primary key); -- also OK
CREATE TEMP TABLE public.temp_to_perm (a int primary key); -- not OK
DROP TABLE unlogged1, public.unlogged2;
+CREATE UNLOGGED TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- fail
+CREATE TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- ok
+ALTER TABLE unlogged1 SET LOGGED; -- fails
+ALTER TABLE unlogged1 SET UNLOGGED; -- fails
+DROP TABLE unlogged1;
+
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1a49f321cf..19f29a0470 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -795,6 +795,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
(for identity or serial columns). However, it is also possible to
change the persistence of such sequences separately.
</para>
+
+ <para>
+ This form is not supported for partitioned tables.
+ </para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index c1855b8d82..83859bac76 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -220,6 +220,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
If this is specified, any sequences created together with the unlogged
table (for identity or serial columns) are also created as unlogged.
</para>
+
+ <para>
+ This form is not supported for partitioned tables.
+ </para>
</listitem>
</varlistentry>
--
2.45.2
On 2024-Sep-10, Michael Paquier wrote:
@@ -5060,17 +5061,17 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, pass = AT_PASS_MISC; break; case AT_AttachPartition: - ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_INDEX); + ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_PARTITIONED_INDEX); /* No command-specific prep needed */ pass = AT_PASS_MISC; break; case AT_DetachPartition: - ATSimplePermissions(cmd->subtype, rel, ATT_TABLE); + ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE); /* No command-specific prep needed */ pass = AT_PASS_MISC; break; case AT_DetachPartitionFinalize: - ATSimplePermissions(cmd->subtype, rel, ATT_TABLE); + ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE); /* No command-specific prep needed */ pass = AT_PASS_MISC; break;
It looks to me like these cases could be modified to accept only
ATT_PARTITIONED_TABLE, not ATT_TABLE anymore. The ATT_TABLE cases are
useless anyway, because they're rejected by transformPartitionCmd.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Attachments:
0001-Don-t-take-ATT_TABLE-for-ALTER-TABLE-.-ATTACH-DETACH.patchtext/x-diff; charset=utf-8Download
From eab048c9c7e01df46b0d98d68507c98d9fa187c1 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 19 Sep 2024 09:51:07 +0200
Subject: [PATCH] Don't take ATT_TABLE for ALTER TABLE ... ATTACH/DETACH
---
src/backend/commands/tablecmds.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2d703aa22e..d27e6cf345 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5107,19 +5107,17 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_AttachPartition:
ATSimplePermissions(cmd->subtype, rel,
- ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_PARTITIONED_INDEX);
+ ATT_PARTITIONED_TABLE | ATT_PARTITIONED_INDEX);
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_DetachPartition:
- ATSimplePermissions(cmd->subtype, rel,
- ATT_TABLE | ATT_PARTITIONED_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_PARTITIONED_TABLE);
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_DetachPartitionFinalize:
- ATSimplePermissions(cmd->subtype, rel,
- ATT_TABLE | ATT_PARTITIONED_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_PARTITIONED_TABLE);
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
--
2.39.2
On Thu, Sep 19, 2024 at 10:03:09AM +0200, Alvaro Herrera wrote:
It looks to me like these cases could be modified to accept only
ATT_PARTITIONED_TABLE, not ATT_TABLE anymore. The ATT_TABLE cases are
useless anyway, because they're rejected by transformPartitionCmd.
+1. If anything, this makes the error messages more consistent.
--
nathan
On Thu, Sep 19, 2024 at 10:03:09AM +0200, Alvaro Herrera wrote:
It looks to me like these cases could be modified to accept only
ATT_PARTITIONED_TABLE, not ATT_TABLE anymore. The ATT_TABLE cases are
useless anyway, because they're rejected by transformPartitionCmd.
Makes sense to me, thanks for the suggestion.
What do you think about adding a test with DETACH FINALIZE when
attempting it on a normal table, its path being under a different
subcommand than DETACH [CONCURRENTLY]?
There are no tests for normal tables with DETACH CONCURRENTLY, but as
it is the same as DETACH under the AT_DetachPartition subcommand, that
does not seem worth the extra cycles.
--
Michael
On Fri, Sep 20, 2024 at 09:37:54AM +0900, Michael Paquier wrote:
What do you think about adding a test with DETACH FINALIZE when
attempting it on a normal table, its path being under a different
subcommand than DETACH [CONCURRENTLY]?There are no tests for normal tables with DETACH CONCURRENTLY, but as
it is the same as DETACH under the AT_DetachPartition subcommand, that
does not seem worth the extra cycles.
Added an extra test for the CONCURRENTLY case at the end, and applied
the simplification.
Hmm. Should we replace the error messages in transformPartitionCmd()
with some assertions at this stage? I don't see a high cost in
keeping these even now, and keeping errors is perhaps more useful for
some extension code that builds AT_AttachPartition or
AT_DetachPartition commands?
--
Michael
On Thu, Sep 19, 2024 at 01:08:33PM +0900, Michael Paquier wrote:
I have applied 0001 for now to add ATT_PARTITIONED_TABLE. Attached is
the remaining piece.
And the second piece is now applied as of e2bab2d79204.
--
Michael
On Wed, Jun 4, 2025 at 6:42 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Sep 19, 2024 at 01:08:33PM +0900, Michael Paquier wrote:
I have applied 0001 for now to add ATT_PARTITIONED_TABLE. Attached is
the remaining piece.And the second piece is now applied as of e2bab2d79204.
--
Michael
Hi,
Should we consider preventing tab completion for PARTITION BY
immediately after CREATE TABLE name (...)? Or is it fine to leave it
as is, given that it's syntactically correct?
--
Best regards,
Shinya Kato
NTT OSS Center
On Wed, Jun 4, 2025 at 6:55 PM Shinya Kato <shinya11.kato@gmail.com> wrote:
On Wed, Jun 4, 2025 at 6:42 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Sep 19, 2024 at 01:08:33PM +0900, Michael Paquier wrote:
I have applied 0001 for now to add ATT_PARTITIONED_TABLE. Attached is
the remaining piece.And the second piece is now applied as of e2bab2d79204.
--
MichaelHi,
Should we consider preventing tab completion for PARTITION BY
immediately after CREATE TABLE name (...)? Or is it fine to leave it
as is, given that it's syntactically correct?
Sorry.
CREATE TABLE name (...) -> CREATE UNLOGGED TABLE name (...)
--
Best regards,
Shinya Kato
NTT OSS Center
On Wed, Jun 04, 2025 at 07:05:20PM +0900, Shinya Kato wrote:
On Wed, Jun 4, 2025 at 6:55 PM Shinya Kato <shinya11.kato@gmail.com> wrote:
Should we consider preventing tab completion for PARTITION BY
immediately after CREATE TABLE name (...)? Or is it fine to leave it
as is, given that it's syntactically correct?Sorry.
CREATE TABLE name (...) -> CREATE UNLOGGED TABLE name (...)
I see no benefit in recommending things that are guaranteed to error. In
commit 5c1ce1b, we removed tab completion for CREATE UNLOGGED MATERIALIZED
VIEW even though it is supported by the grammar. The partitioned table
case sounds like roughly the same situation.
--
nathan
On Wed, Jun 04, 2025 at 11:15:29AM -0500, Nathan Bossart wrote:
I see no benefit in recommending things that are guaranteed to error. In
commit 5c1ce1b, we removed tab completion for CREATE UNLOGGED MATERIALIZED
VIEW even though it is supported by the grammar. The partitioned table
case sounds like roughly the same situation.
Agreed to not suggest the PARTITION BY clause in the tab completion as
it is not supported by the backend for unlogged tables.
tab-complete.in.c has some handling for CREATE UNLOGGED TABLE around
line 3667, so we could just have an extra case for it, like in the
attached patch. A split already exists for temporary tables to handle
the ON COMMIT clause after the attribute list.
Thoughts?
--
Michael
Attachments:
psql-tab-unlogged.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index ec65ab79fecb..620830feb9d7 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -3664,9 +3664,10 @@ match_previous_words(int pattern_id,
TailMatches("CREATE", "TEMP|TEMPORARY|UNLOGGED", "TABLE", MatchAny, "(*)", "AS"))
COMPLETE_WITH("EXECUTE", "SELECT", "TABLE", "VALUES", "WITH");
/* Complete CREATE TABLE name (...) with supported options */
- else if (TailMatches("CREATE", "TABLE", MatchAny, "(*)") ||
- TailMatches("CREATE", "UNLOGGED", "TABLE", MatchAny, "(*)"))
+ else if (TailMatches("CREATE", "TABLE", MatchAny, "(*)"))
COMPLETE_WITH("AS", "INHERITS (", "PARTITION BY", "USING", "TABLESPACE", "WITH (");
+ else if (TailMatches("CREATE", "UNLOGGED", "TABLE", MatchAny, "(*)"))
+ COMPLETE_WITH("AS", "INHERITS (", "USING", "TABLESPACE", "WITH (");
else if (TailMatches("CREATE", "TEMP|TEMPORARY", "TABLE", MatchAny, "(*)"))
COMPLETE_WITH("AS", "INHERITS (", "ON COMMIT", "PARTITION BY", "USING",
"TABLESPACE", "WITH (");
On Thu, Jun 5, 2025 at 9:23 AM Michael Paquier <michael@paquier.xyz> wrote:
Agreed to not suggest the PARTITION BY clause in the tab completion as
it is not supported by the backend for unlogged tables.
tab-complete.in.c has some handling for CREATE UNLOGGED TABLE around
line 3667, so we could just have an extra case for it, like in the
attached patch. A split already exists for temporary tables to handle
the ON COMMIT clause after the attribute list.Thoughts?
Thank you. It looks good to me.
--
Best regards,
Shinya Kato
NTT OSS Center
On Thu, Jun 05, 2025 at 10:04:23AM +0900, Shinya Kato wrote:
Thank you. It looks good to me.
How does the RMT feel about this change? Nathan, would you be OK with
that? It's not a big problem, as well, if the code is kept as-is, but
as it's a simple change..
--
Michael
On Thu, Jun 05, 2025 at 01:57:30PM +0900, Michael Paquier wrote:
How does the RMT feel about this change? Nathan, would you be OK with
that? It's not a big problem, as well, if the code is kept as-is, but
as it's a simple change..
IMHO a case can be reasonably made that this is an oversight in the related
commit. I've added the rest of the RMT here in case they see it
differently.
The patch LGTM, too.
--
nathan
On Thu, Jun 05, 2025 at 03:04:45PM -0500, Nathan Bossart wrote:
IMHO a case can be reasonably made that this is an oversight in the related
commit. I've added the rest of the RMT here in case they see it
differently.The patch LGTM, too.
Okay, thanks. Let's wait for a couple of days in case there are more
opinions and/or comments. I propose to apply the patch around the
beginning of next week if there are no objections.
--
Michael
On Fri, Jun 06, 2025 at 10:55:32AM +0900, Michael Paquier wrote:
Okay, thanks. Let's wait for a couple of days in case there are more
opinions and/or comments. I propose to apply the patch around the
beginning of next week if there are no objections.
It took a bit longer than wanted, but applied now.
--
Michael