Subject: Re: Partitioned tables and [un]loggedness

From: Nathan Bossart <[email-masked]>
Date: Thu, 29 Aug 2024 09:49:44 -0500

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

---

From: Michael Paquier <[email-masked]>
Date: Tue, 3 Sep 2024 09:22:58 +0900

> preserves the illusion that it does something.  An ERROR could help dispel

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

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 2 Sep 2024 20:35:15 -0500

> there is tbinfo->relkind.

That's roughly what I had in mind.

-- 
nathan

---

From: Michael Paquier <[email-masked]>
Date: Tue, 3 Sep 2024 16:59:18 +0900

> That's roughly what I had in mind.

An idea is attached.  The pgbench bit was unexpected.
--
Michael
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>