[bug fix] ALTER TABLE SET LOGGED/UNLOGGED on a partitioned table does nothing silently
Hello,
ALTER TABLE SET LOGGED/UNLOGED on a partitioned table completes successfully, but nothing changes. I expected the underlying partitions are changed accordingly. The attached patch fixes this.
Regards
Takayuki Tsunakawa
Attachments:
v1-0001-Make-ALTER-TABLE-SET-LOGGED-UNLOGGED-on-a-partiti.patchapplication/octet-stream; name=v1-0001-Make-ALTER-TABLE-SET-LOGGED-UNLOGGED-on-a-partiti.patchDownload
From 6abf2236b59c6c5c569d2ee71df843114869ba56 Mon Sep 17 00:00:00 2001
From: Takayuki Tsunakawa <tsunakawa.takay@fujitsu.com>
Date: Wed, 2 Dec 2020 17:04:12 +0900
Subject: [PATCH v1] Make ALTER TABLE SET LOGGED/UNLOGGED on a partitioned
table recurse
---
src/backend/commands/tablecmds.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a29c14b..81b2fe9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4246,6 +4246,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE;
tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
}
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
pass = AT_PASS_MISC;
break;
case AT_SetUnLogged: /* SET UNLOGGED */
@@ -4261,6 +4263,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE;
tab->newrelpersistence = RELPERSISTENCE_UNLOGGED;
}
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
pass = AT_PASS_MISC;
break;
case AT_DropOids: /* SET WITHOUT OIDS */
--
2.10.1
On Wed, Dec 2, 2020 at 1:52 PM tsunakawa.takay@fujitsu.com
<tsunakawa.takay@fujitsu.com> wrote:
ALTER TABLE SET LOGGED/UNLOGED on a partitioned table completes successfully, but nothing changes.
Yeah, true.
I expected the underlying partitions are changed accordingly. The attached patch fixes this.
My thoughts:
1) What happens if a partitioned table has a foreign partition along
with few other local partitions[1]? Currently, if we try to set
logged/unlogged of a foreign table, then an "ERROR: "XXXX" is not a
table" is thrown. This makes sense. With your patch also we see the
same error, but I'm not quite sure, whether it is setting the parent
and local partitions to logged/unlogged and then throwing the error?
Or do we want the parent relation and the all the local partitions
become logged/unlogged and give a warning saying foreign table can not
be made logged/unlogged?
2) What is the order in which the parent table and the partitions are
made logged/unlogged? Is it that first the parent table and then all
the partitions? What if an error occurs as in above point for a
foreign partition or a partition having foreign key reference to a
logged table? During the rollback of the txn, will we undo the setting
logged/unlogged?
3) Say, I have two logged tables t1 and t2. I altered t1 to be
unlogged, and then I attach logged table t2 as a partition to t1, then
what's the expectation? While attaching the partition, should we also
make t2 as unlogged?
4) Currently, if we try to set logged/unlogged of a foreign table,
then an "ERROR: "XXXX" is not a table" is thrown. I also see that, in
general ATWrongRelkindError() throws an error saying the given
relation is not of expected types, but it doesn't say what is the
given relation kind. Should we improve ATWrongRelkindError() by
passing the actual relation type along with the allowed relation types
to show a bit more informative error message, something like "ERROR:
"XXXX" is a foreign table" with a hint "Allowed relation types are
table, view, index."
5) Coming to the patch, it is missing to add test cases.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Wed, Dec 2, 2020 at 3:57 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
On Wed, Dec 2, 2020 at 1:52 PM tsunakawa.takay@fujitsu.com
<tsunakawa.takay@fujitsu.com> wrote:ALTER TABLE SET LOGGED/UNLOGED on a partitioned table completes successfully, but nothing changes.
Yeah, true.
I expected the underlying partitions are changed accordingly. The attached patch fixes this.
My thoughts:
1) What happens if a partitioned table has a foreign partition along
with few other local partitions[1]? Currently, if we try to set
logged/unlogged of a foreign table, then an "ERROR: "XXXX" is not a
table" is thrown. This makes sense. With your patch also we see the
same error, but I'm not quite sure, whether it is setting the parent
and local partitions to logged/unlogged and then throwing the error?
Or do we want the parent relation and the all the local partitions
become logged/unlogged and give a warning saying foreign table can not
be made logged/unlogged?
2) What is the order in which the parent table and the partitions are
made logged/unlogged? Is it that first the parent table and then all
the partitions? What if an error occurs as in above point for a
foreign partition or a partition having foreign key reference to a
logged table? During the rollback of the txn, will we undo the setting
logged/unlogged?
3) Say, I have two logged tables t1 and t2. I altered t1 to be
unlogged, and then I attach logged table t2 as a partition to t1, then
what's the expectation? While attaching the partition, should we also
make t2 as unlogged?
4) Currently, if we try to set logged/unlogged of a foreign table,
then an "ERROR: "XXXX" is not a table" is thrown. I also see that, in
general ATWrongRelkindError() throws an error saying the given
relation is not of expected types, but it doesn't say what is the
given relation kind. Should we improve ATWrongRelkindError() by
passing the actual relation type along with the allowed relation types
to show a bit more informative error message, something like "ERROR:
"XXXX" is a foreign table" with a hint "Allowed relation types are
table, view, index."
5) Coming to the patch, it is missing to add test cases.
Sorry, I missed to add the foreign partition use case, specified as
[1]: CREATE TABLE tbl1 ( a INT, b INT, c TEXT default 'stuff', d TEXT, e TEXT ) PARTITION BY LIST (b);
[1]: CREATE TABLE tbl1 ( a INT, b INT, c TEXT default 'stuff', d TEXT, e TEXT ) PARTITION BY LIST (b);
CREATE TABLE tbl1 (
a INT,
b INT,
c TEXT default 'stuff',
d TEXT,
e TEXT
) PARTITION BY LIST (b);
CREATE FOREIGN TABLE foreign_part_tbl1_1(c TEXT, b INT, a INT, e TEXT,
d TEXT) SERVER loopback;
CREATE TABLE part_tbl1_2 (a INT, c TEXT, b INT, d TEXT, e TEXT);
ALTER TABLE tbl1 ATTACH PARTITION foreign_part_tbl1_1 FOR VALUES IN(1);
ALTER TABLE tbl1 ATTACH PARTITION part_tbl1_2 FOR VALUES IN(2);
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
1) What happens if a partitioned table has a foreign partition along
with few other local partitions[1]? Currently, if we try to set
logged/unlogged of a foreign table, then an "ERROR: "XXXX" is not a
table" is thrown. This makes sense. With your patch also we see the
same error, but I'm not quite sure, whether it is setting the parent
and local partitions to logged/unlogged and then throwing the error?
Or do we want the parent relation and the all the local partitions
become logged/unlogged and give a warning saying foreign table can not
be made logged/unlogged?
Good point, thanks. I think the foreign partitions should be ignored, otherwise the user would have to ALTER each local partition manually or detach the foreign partitions temporarily. Done like that.
2) What is the order in which the parent table and the partitions are
made logged/unlogged? Is it that first the parent table and then all
the partitions? What if an error occurs as in above point for a
foreign partition or a partition having foreign key reference to a
logged table? During the rollback of the txn, will we undo the setting
logged/unlogged?
The parent is not changed because it does not have storage.
If some partition has undesirable foreign key relationship, the entire ALTER command fails. All the effects are undone when the transaction rolls back.
3) Say, I have two logged tables t1 and t2. I altered t1 to be
unlogged, and then I attach logged table t2 as a partition to t1, then
what's the expectation? While attaching the partition, should we also
make t2 as unlogged?
The attached partition retains its property.
4) Currently, if we try to set logged/unlogged of a foreign table,
then an "ERROR: "XXXX" is not a table" is thrown. I also see that, in
general ATWrongRelkindError() throws an error saying the given
relation is not of expected types, but it doesn't say what is the
given relation kind. Should we improve ATWrongRelkindError() by
passing the actual relation type along with the allowed relation types
to show a bit more informative error message, something like "ERROR:
"XXXX" is a foreign table" with a hint "Allowed relation types are
table, view, index."
Ah, maybe that's a bit more friendly. But I don't think it's worth bothering to mess ATWrongRelkindError() with a long switch statement to map a relation kind to its string representation. Anyway, I'd like it to be a separate topic.
5) Coming to the patch, it is missing to add test cases.
Yes, added in the revised patch.
I added this to the next CF.
Regards
Takayuki Tsunakawa
Attachments:
v2-0001-Make-ALTER-TABLE-SET-LOGGED-UNLOGGED-on-a-partiti.patchapplication/octet-stream; name=v2-0001-Make-ALTER-TABLE-SET-LOGGED-UNLOGGED-on-a-partiti.patchDownload
From 93cf9663834c39b8228ded84b4c16e70dd477501 Mon Sep 17 00:00:00 2001
From: Takayuki Tsunakawa <tsunakawa.takay@fujitsu.com>
Date: Wed, 2 Dec 2020 17:04:12 +0900
Subject: [PATCH v2] Make ALTER TABLE SET LOGGED/UNLOGGED on a partitioned
table recurse
---
src/backend/commands/tablecmds.c | 12 ++++
src/test/regress/expected/alter_table.out | 92 +++++++++++++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 37 +++++++++++++
3 files changed, 141 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a29c14b..1392736 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4246,6 +4246,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE;
tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
}
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
pass = AT_PASS_MISC;
break;
case AT_SetUnLogged: /* SET UNLOGGED */
@@ -4261,6 +4263,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE;
tab->newrelpersistence = RELPERSISTENCE_UNLOGGED;
}
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
pass = AT_PASS_MISC;
break;
case AT_DropOids: /* SET WITHOUT OIDS */
@@ -5713,6 +5717,14 @@ ATSimpleRecursion(List **wqueue, Relation rel,
continue;
/* find_all_inheritors already got lock */
childrel = relation_open(childrelid, NoLock);
+ /* Skip foreign partitions when SET LOGGED/UNLOGGED */
+ if ((cmd->subtype == AT_SetLogged ||
+ cmd->subtype == AT_SetUnLogged) &&
+ childrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
+ {
+ relation_close(childrel, NoLock);
+ continue;
+ }
CheckTableNotInUse(childrel, "ALTER TABLE");
ATPrepCmd(wqueue, childrel, cmd, false, true, lockmode, context);
relation_close(childrel, NoLock);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 0ce6ee4..e9e21a4 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3472,6 +3472,98 @@ ALTER TABLE logged1 SET UNLOGGED; -- silently do nothing
DROP TABLE logged3;
DROP TABLE logged2;
DROP TABLE logged1;
+-- set partitioned table logged/unlogged
+CREATE TABLE part_logged_parent (key integer PRIMARY KEY, val text)
+ PARTITION BY RANGE (key);
+CREATE TABLE part_logged_child1 PARTITION OF part_logged_parent
+ FOR VALUES FROM (1) TO (10);
+CREATE TABLE part_logged_child2 PARTITION OF part_logged_parent
+ FOR VALUES FROM (11) TO (20);
+ALTER TABLE part_logged_parent SET UNLOGGED; -- children become unlogged
+CREATE TABLE part_logged_child3 (LIKE part_logged_parent);
+ALTER TABLE part_logged_parent ATTACH PARTITION part_logged_child3
+ FOR VALUES FROM (21) TO (30); -- child3 remains logged
+-- check relpersistence
+SELECT relname, relkind, relpersistence FROM pg_class WHERE relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast table for ' || r.relname, t.relkind, t.relpersistence FROM pg_class r JOIN pg_class t ON t.oid = r.reltoastrelid WHERE r.relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast index for ' || r.relname, ri.relkind, ri.relpersistence FROM pg_class r join pg_class t ON t.oid = r.reltoastrelid JOIN pg_index i ON i.indrelid = t.oid JOIN pg_class ri ON ri.oid = i.indexrelid WHERE r.relname ~ '^part_logged'
+ORDER BY relname;
+ relname | relkind | relpersistence
+------------------------------------+---------+----------------
+ part_logged_child1 | r | u
+ part_logged_child1_pkey | i | u
+ part_logged_child2 | r | u
+ part_logged_child2_pkey | i | u
+ part_logged_child3 | r | p
+ part_logged_child3_pkey | i | p
+ part_logged_parent | p | p
+ part_logged_parent_pkey | I | p
+ toast index for part_logged_child1 | i | u
+ toast index for part_logged_child2 | i | u
+ toast index for part_logged_child3 | i | p
+ toast table for part_logged_child1 | t | u
+ toast table for part_logged_child2 | t | u
+ toast table for part_logged_child3 | t | p
+(14 rows)
+
+ALTER TABLE part_logged_parent SET LOGGED; -- children become logged
+-- check relpersistence
+SELECT relname, relkind, relpersistence FROM pg_class WHERE relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast table for ' || r.relname, t.relkind, t.relpersistence FROM pg_class r JOIN pg_class t ON t.oid = r.reltoastrelid WHERE r.relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast index for ' || r.relname, ri.relkind, ri.relpersistence FROM pg_class r join pg_class t ON t.oid = r.reltoastrelid JOIN pg_index i ON i.indrelid = t.oid JOIN pg_class ri ON ri.oid = i.indexrelid WHERE r.relname ~ '^part_logged'
+ORDER BY relname;
+ relname | relkind | relpersistence
+------------------------------------+---------+----------------
+ part_logged_child1 | r | p
+ part_logged_child1_pkey | i | p
+ part_logged_child2 | r | p
+ part_logged_child2_pkey | i | p
+ part_logged_child3 | r | p
+ part_logged_child3_pkey | i | p
+ part_logged_parent | p | p
+ part_logged_parent_pkey | I | p
+ toast index for part_logged_child1 | i | p
+ toast index for part_logged_child2 | i | p
+ toast index for part_logged_child3 | i | p
+ toast table for part_logged_child1 | t | p
+ toast table for part_logged_child2 | t | p
+ toast table for part_logged_child3 | t | p
+(14 rows)
+
+CREATE TABLE logged_fk_child (key integer PRIMARY KEY, fk integer REFERENCES part_logged_child2);
+ALTER TABLE part_logged_parent SET UNLOGGED; -- fails because logged table references a child
+ERROR: could not change table "part_logged_child2" to unlogged because it references logged table "logged_fk_child"
+-- check relpersistence
+SELECT relname, relkind, relpersistence FROM pg_class WHERE relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast table for ' || r.relname, t.relkind, t.relpersistence FROM pg_class r JOIN pg_class t ON t.oid = r.reltoastrelid WHERE r.relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast index for ' || r.relname, ri.relkind, ri.relpersistence FROM pg_class r join pg_class t ON t.oid = r.reltoastrelid JOIN pg_index i ON i.indrelid = t.oid JOIN pg_class ri ON ri.oid = i.indexrelid WHERE r.relname ~ '^part_logged'
+ORDER BY relname;
+ relname | relkind | relpersistence
+------------------------------------+---------+----------------
+ part_logged_child1 | r | p
+ part_logged_child1_pkey | i | p
+ part_logged_child2 | r | p
+ part_logged_child2_pkey | i | p
+ part_logged_child3 | r | p
+ part_logged_child3_pkey | i | p
+ part_logged_parent | p | p
+ part_logged_parent_pkey | I | p
+ toast index for part_logged_child1 | i | p
+ toast index for part_logged_child2 | i | p
+ toast index for part_logged_child3 | i | p
+ toast table for part_logged_child1 | t | p
+ toast table for part_logged_child2 | t | p
+ toast table for part_logged_child3 | t | p
+(14 rows)
+
+DROP TABLE logged_fk_child;
+DROP TABLE part_logged_parent;
-- 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 4cc55d8..18c0a69 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2188,6 +2188,43 @@ ALTER TABLE logged1 SET UNLOGGED; -- silently do nothing
DROP TABLE logged3;
DROP TABLE logged2;
DROP TABLE logged1;
+-- set partitioned table logged/unlogged
+CREATE TABLE part_logged_parent (key integer PRIMARY KEY, val text)
+ PARTITION BY RANGE (key);
+CREATE TABLE part_logged_child1 PARTITION OF part_logged_parent
+ FOR VALUES FROM (1) TO (10);
+CREATE TABLE part_logged_child2 PARTITION OF part_logged_parent
+ FOR VALUES FROM (11) TO (20);
+ALTER TABLE part_logged_parent SET UNLOGGED; -- children become unlogged
+CREATE TABLE part_logged_child3 (LIKE part_logged_parent);
+ALTER TABLE part_logged_parent ATTACH PARTITION part_logged_child3
+ FOR VALUES FROM (21) TO (30); -- child3 remains logged
+-- check relpersistence
+SELECT relname, relkind, relpersistence FROM pg_class WHERE relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast table for ' || r.relname, t.relkind, t.relpersistence FROM pg_class r JOIN pg_class t ON t.oid = r.reltoastrelid WHERE r.relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast index for ' || r.relname, ri.relkind, ri.relpersistence FROM pg_class r join pg_class t ON t.oid = r.reltoastrelid JOIN pg_index i ON i.indrelid = t.oid JOIN pg_class ri ON ri.oid = i.indexrelid WHERE r.relname ~ '^part_logged'
+ORDER BY relname;
+ALTER TABLE part_logged_parent SET LOGGED; -- children become logged
+-- check relpersistence
+SELECT relname, relkind, relpersistence FROM pg_class WHERE relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast table for ' || r.relname, t.relkind, t.relpersistence FROM pg_class r JOIN pg_class t ON t.oid = r.reltoastrelid WHERE r.relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast index for ' || r.relname, ri.relkind, ri.relpersistence FROM pg_class r join pg_class t ON t.oid = r.reltoastrelid JOIN pg_index i ON i.indrelid = t.oid JOIN pg_class ri ON ri.oid = i.indexrelid WHERE r.relname ~ '^part_logged'
+ORDER BY relname;
+CREATE TABLE logged_fk_child (key integer PRIMARY KEY, fk integer REFERENCES part_logged_child2);
+ALTER TABLE part_logged_parent SET UNLOGGED; -- fails because logged table references a child
+-- check relpersistence
+SELECT relname, relkind, relpersistence FROM pg_class WHERE relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast table for ' || r.relname, t.relkind, t.relpersistence FROM pg_class r JOIN pg_class t ON t.oid = r.reltoastrelid WHERE r.relname ~ '^part_logged'
+UNION ALL
+SELECT 'toast index for ' || r.relname, ri.relkind, ri.relpersistence FROM pg_class r join pg_class t ON t.oid = r.reltoastrelid JOIN pg_index i ON i.indrelid = t.oid JOIN pg_class ri ON ri.oid = i.indexrelid WHERE r.relname ~ '^part_logged'
+ORDER BY relname;
+DROP TABLE logged_fk_child;
+DROP TABLE part_logged_parent;
-- test ADD COLUMN IF NOT EXISTS
CREATE TABLE test_add_column(c1 integer);
--
2.10.1
On Fri, Dec 4, 2020 at 8:22 AM tsunakawa.takay@fujitsu.com
<tsunakawa.takay@fujitsu.com> wrote:
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
1) What happens if a partitioned table has a foreign partition along
with few other local partitions[1]? Currently, if we try to set
logged/unlogged of a foreign table, then an "ERROR: "XXXX" is not a
table" is thrown. This makes sense. With your patch also we see the
same error, but I'm not quite sure, whether it is setting the parent
and local partitions to logged/unlogged and then throwing the error?
Or do we want the parent relation and the all the local partitions
become logged/unlogged and give a warning saying foreign table can not
be made logged/unlogged?Good point, thanks. I think the foreign partitions should be ignored, otherwise the user would have to ALTER each local partition manually or detach the foreign partitions temporarily. Done like that.
2) What is the order in which the parent table and the partitions are
made logged/unlogged? Is it that first the parent table and then all
the partitions? What if an error occurs as in above point for a
foreign partition or a partition having foreign key reference to a
logged table? During the rollback of the txn, will we undo the setting
logged/unlogged?The parent is not changed because it does not have storage.
If some partition has undesirable foreign key relationship, the entire ALTER command fails. All the effects are undone when the transaction rolls back.3) Say, I have two logged tables t1 and t2. I altered t1 to be
unlogged, and then I attach logged table t2 as a partition to t1, then
what's the expectation? While attaching the partition, should we also
make t2 as unlogged?The attached partition retains its property.
4) Currently, if we try to set logged/unlogged of a foreign table,
then an "ERROR: "XXXX" is not a table" is thrown. I also see that, in
general ATWrongRelkindError() throws an error saying the given
relation is not of expected types, but it doesn't say what is the
given relation kind. Should we improve ATWrongRelkindError() by
passing the actual relation type along with the allowed relation types
to show a bit more informative error message, something like "ERROR:
"XXXX" is a foreign table" with a hint "Allowed relation types are
table, view, index."Ah, maybe that's a bit more friendly. But I don't think it's worth bothering to mess ATWrongRelkindError() with a long switch statement to map a relation kind to its string representation. Anyway, I'd like it to be a separate topic.
5) Coming to the patch, it is missing to add test cases.
Yes, added in the revised patch.
I added this to the next CF.
Thanks! I will review the v2 patch and provide my thoughts.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Fri, Dec 4, 2020 at 8:22 AM tsunakawa.takay@fujitsu.com
<tsunakawa.takay@fujitsu.com> wrote:
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
1) What happens if a partitioned table has a foreign partition along
with few other local partitions[1]? Currently, if we try to set
logged/unlogged of a foreign table, then an "ERROR: "XXXX" is not a
table" is thrown. This makes sense. With your patch also we see the
same error, but I'm not quite sure, whether it is setting the parent
and local partitions to logged/unlogged and then throwing the error?
Or do we want the parent relation and the all the local partitions
become logged/unlogged and give a warning saying foreign table can not
be made logged/unlogged?Good point, thanks. I think the foreign partitions should be ignored, otherwise the user would have to ALTER each local partition manually or detach the foreign partitions temporarily. Done like that.
Agreed.
2) What is the order in which the parent table and the partitions are
made logged/unlogged? Is it that first the parent table and then all
the partitions? What if an error occurs as in above point for a
foreign partition or a partition having foreign key reference to a
logged table? During the rollback of the txn, will we undo the setting
logged/unlogged?The parent is not changed because it does not have storage.
IMHO, we should also change the parent table. Say, I have 2 local
partitions for a logged table, then I alter that table to
unlogged(with your patch, parent table doesn't become unlogged whereas
the partitions will), and I detach all the partitions for some reason.
Now, the user would think that he set the parent table to unlogged but
it didn't happen. So, I think we should also change the parent table's
logged/unlogged property though it may not have data associated with
it when it has all the partitions. Thoughts?
If some partition has undesirable foreign key relationship, the entire ALTER command fails. All the effects are undone when the transaction rolls back.
Hmm.
3) Say, I have two logged tables t1 and t2. I altered t1 to be
unlogged, and then I attach logged table t2 as a partition to t1, then
what's the expectation? While attaching the partition, should we also
make t2 as unlogged?The attached partition retains its property.
This may lead to a situation where a partition table has few
partitions as logged and few as unlogged. Will it lead to some
problems? I'm not quite sure though. I feel while attaching a
partition to a table, we have to check whether the parent is
logged/unlogged and set the partition accordingly. While detaching a
partition we do not need to do anything, just retain the existing
state. I read this from the docs "Any indexes created on an unlogged
table are automatically unlogged as well". So, on the similar lines we
also should automatically make partitions logged/unlogged based on the
parent table. We may have to also think of cases where there exists a
multi level parent child relationship i.e. the table to which a
partition is being attached may be a child to another parent table.
Thoughts?
5) Coming to the patch, it is missing to add test cases.
Yes, added in the revised patch.
I think we can add foreign partition case into postgres_fdw.sql so
that the tests will run as part of make check-world.
How about documenting all these points in alter_table.sgml,
create_table.sgml and create_foreign_table.sgml under the partition
section?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
IMHO, we should also change the parent table. Say, I have 2 local
partitions for a logged table, then I alter that table to
unlogged(with your patch, parent table doesn't become unlogged whereas
the partitions will), and I detach all the partitions for some reason.
Now, the user would think that he set the parent table to unlogged but
it didn't happen. So, I think we should also change the parent table's
logged/unlogged property though it may not have data associated with
it when it has all the partitions. Thoughts?
I'd like to think that the logged/unlogged property is basically specific to each storage unit, which is a partition here, and ALTER TABLE on a partitioned table conveniently changes the properties of underlying storage units. (In that regard, it's unfortunate that it fails with an ERROR to try to change storage parameters like fillfactor with ALTER TABLE.)
This may lead to a situation where a partition table has few
partitions as logged and few as unlogged. Will it lead to some
problems?
No, I don't see any problem.
I think we can add foreign partition case into postgres_fdw.sql so
that the tests will run as part of make check-world.
I was hesitant to add this test in postgres_fdw, but it seems to be a good place considering that postgres_fdw, and other contrib modules as well, are part of Postgres core.
How about documenting all these points in alter_table.sgml,
create_table.sgml and create_foreign_table.sgml under the partition
section?
I'd like to add a statement in the description of ALTER TABLE SET LOGGED/UNLOGGED as other ALTER actions do. I don't want to add a new partition section for all CREATE/ALTER actions in this patch.
If there's no objection, I think I'll submit the (hopefully final) revised patch after a few days.
Regards
Takayuki Tsunakawa
On Mon, Dec 7, 2020 at 11:36 AM tsunakawa.takay@fujitsu.com
<tsunakawa.takay@fujitsu.com> wrote:
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
IMHO, we should also change the parent table. Say, I have 2 local
partitions for a logged table, then I alter that table to
unlogged(with your patch, parent table doesn't become unlogged whereas
the partitions will), and I detach all the partitions for some reason.
Now, the user would think that he set the parent table to unlogged but
it didn't happen. So, I think we should also change the parent table's
logged/unlogged property though it may not have data associated with
it when it has all the partitions. Thoughts?I'd like to think that the logged/unlogged property is basically specific to each storage unit, which is a partition here, and ALTER TABLE on a partitioned table conveniently changes the properties of underlying storage units. (In that regard, it's unfortunate that it fails with an ERROR to try to change storage parameters like fillfactor with ALTER TABLE.)
Do you mean to say that if we detach all the partitions(assuming they
are all unlogged) then the parent table(assuming logged) gets changed
to unlogged? Does it happen on master? Am I missing something here?
I think we can add foreign partition case into postgres_fdw.sql so
that the tests will run as part of make check-world.I was hesitant to add this test in postgres_fdw, but it seems to be a good place considering that postgres_fdw, and other contrib modules as well, are part of Postgres core.
+1 to add tests in postgres_fdw.
How about documenting all these points in alter_table.sgml,
create_table.sgml and create_foreign_table.sgml under the partition
section?I'd like to add a statement in the description of ALTER TABLE SET LOGGED/UNLOGGED as other ALTER actions do. I don't want to add a new partition section for all CREATE/ALTER actions in this patch.
+1.
If there's no objection, I think I'll submit the (hopefully final) revised patch after a few days.
Please do so. Thanks.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Does "ALTER TABLE ONLY parent" work correctly? Namely, do not affect
existing partitions, but cause future partitions to acquire the new
setting.
This sounds very much related to previous discussion on REPLICA IDENTITY
not propagating to partitions; see
/messages/by-id/201902041630.gpadougzab7v@alvherre.pgsql
particularly Robert Haas' comments at
/messages/by-id/CA+TgmoYjksObOzY8b1U1=BsP_m+702eOf42fqRtQTYio1NunbA@mail.gmail.com
and downstream discussion from there.
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Do you mean to say that if we detach all the partitions(assuming they
are all unlogged) then the parent table(assuming logged) gets changed
to unlogged? Does it happen on master? Am I missing something here?
No, the parent remains logged in that case both on master and with patched. I understand this behavior is based on the idea that (1) each storage unit (=partition) is independent, and (2) a partitioned table has no storage so the logged/unlogged setting has no meaning. (I don't know there was actually such an idea and the feature was implemented on that idea, though.)
Regards
Takayuki Tsunakawa
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Does "ALTER TABLE ONLY parent" work correctly? Namely, do not affect
existing partitions, but cause future partitions to acquire the new
setting.
Yes, it works correctly in the sense that ALTER TABLE ONLY on a partitioned table does nothing because it has no storage and therefore logged/unlogged has no meaning.
This sounds very much related to previous discussion on REPLICA IDENTITY
not propagating to partitions; see
/messages/by-id/201902041630.gpadougzab7v@alvherre.pgsql
particularly Robert Haas' comments at
/messages/by-id/CA+TgmoYjksObOzY8b1U1=BsP_m+702eOf42fqRtQTYio
1NunbA@mail.gmail.com
and downstream discussion from there.
There was a hot discussion. I've read through it. I hope I'm not doing strange in light of that discussioin.
Regards
Takayuki Tsunakawa
On 2020-Dec-08, tsunakawa.takay@fujitsu.com wrote:
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Does "ALTER TABLE ONLY parent" work correctly? Namely, do not affect
existing partitions, but cause future partitions to acquire the new
setting.Yes, it works correctly in the sense that ALTER TABLE ONLY on a
partitioned table does nothing because it has no storage and therefore
logged/unlogged has no meaning.
But what happens when you create another partition after you change the
"loggedness" of the partitioned table?
This sounds very much related to previous discussion on REPLICA IDENTITY
not propagating to partitions; see
/messages/by-id/201902041630.gpadougzab7v@alvherre.pgsql
particularly Robert Haas' comments at
/messages/by-id/CA+TgmoYjksObOzY8b1U1=BsP_m+702eOf42fqRtQTYio
1NunbA@mail.gmail.com
and downstream discussion from there.There was a hot discussion. I've read through it. I hope I'm not
doing strange in light of that discussioin.
Well, my main take from that is we should strive to change all
subcommands together, in some principled way, rather than change only
one or some, in arbitrary ways.
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
But what happens when you create another partition after you change the
"loggedness" of the partitioned table?
The new partition will have a property specified when the user creates it. That is, while the storage property of each storage unit (=partition) is basically independent, ALTER TABLE on a partitioned table is a convenient way to set the same property value to all its underlying storage units.
Well, my main take from that is we should strive to change all
subcommands together, in some principled way, rather than change only
one or some, in arbitrary ways.
I got an impression from the discussion that some form of consensus on the principle was made and you were trying to create a fix for REPLICA IDENTITY. Do you think the principle was unclear and we should state it first (partly to refresh people's memories)?
I'm kind of for it, but I'm hesitant to create the fix for all ALTER actions, because it may take a lot of time and energy as you were afraid. Can we define the principle, and then create individual fixes independently based on that principle?
Regards
Takayuki Tsunakawa
On 2020-Dec-09, tsunakawa.takay@fujitsu.com wrote:
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
But what happens when you create another partition after you change the
"loggedness" of the partitioned table?The new partition will have a property specified when the user creates
it. That is, while the storage property of each storage unit
(=partition) is basically independent, ALTER TABLE on a partitioned
table is a convenient way to set the same property value to all its
underlying storage units.
Well, that definition seems unfriendly to me. I prefer the stance that
if you change the value for the parent, then future partitions inherit
that value.
I got an impression from the discussion that some form of consensus on
the principle was made and you were trying to create a fix for REPLICA
IDENTITY. Do you think the principle was unclear and we should state
it first (partly to refresh people's memories)?
I think the principle was sound -- namely, that we should make all those
commands recurse by default, and for cases where it matters, the
parent's setting should determine the default for future children.
I'm kind of for it, but I'm hesitant to create the fix for all ALTER
actions, because it may take a lot of time and energy as you were
afraid. Can we define the principle, and then create individual fixes
independently based on that principle?
That seems acceptable to me, as long as we get all changes in the same
release. What we don't want (according to my reading of that
discussion) is to change the semantics of a few subcommands in one
release, and the semantics of a few other subcommands in another
release.
On Wed, Dec 9, 2020 at 6:22 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2020-Dec-09, tsunakawa.takay@fujitsu.com wrote:
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
But what happens when you create another partition after you change the
"loggedness" of the partitioned table?The new partition will have a property specified when the user creates
it. That is, while the storage property of each storage unit
(=partition) is basically independent, ALTER TABLE on a partitioned
table is a convenient way to set the same property value to all its
underlying storage units.Well, that definition seems unfriendly to me. I prefer the stance that
if you change the value for the parent, then future partitions inherit
that value.I got an impression from the discussion that some form of consensus on
the principle was made and you were trying to create a fix for REPLICA
IDENTITY. Do you think the principle was unclear and we should state
it first (partly to refresh people's memories)?I think the principle was sound -- namely, that we should make all those
commands recurse by default, and for cases where it matters, the
parent's setting should determine the default for future children.I'm kind of for it, but I'm hesitant to create the fix for all ALTER
actions, because it may take a lot of time and energy as you were
afraid. Can we define the principle, and then create individual fixes
independently based on that principle?That seems acceptable to me, as long as we get all changes in the same
release. What we don't want (according to my reading of that
discussion) is to change the semantics of a few subcommands in one
release, and the semantics of a few other subcommands in another
release.
I'm not sure how many more of such commands exist which require changes.
How about doing it this way?
1) Have a separate common thread listing the commands and specifying
clearly the agreed behaviour for such commands.
2) Whenever the separate patches are submitted(hopefully) by the
hackers for such commands, after the review we can make a note of that
patch in the common thread.
3) Once the patches for all the listed commands are
received(hopefully) , then they can be committed together in one
release.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On 2020-Dec-09, Bharath Rupireddy wrote:
I'm not sure how many more of such commands exist which require changes.
The other thread has a list. I think it is complete, but if you want to
double-check, that would be helpful.
How about doing it this way?
1) Have a separate common thread listing the commands and specifying
clearly the agreed behaviour for such commands.
2) Whenever the separate patches are submitted(hopefully) by the
hackers for such commands, after the review we can make a note of that
patch in the common thread.
3) Once the patches for all the listed commands are
received(hopefully) , then they can be committed together in one
release.
Sounds good. I think this thread is a good place to collect those
patches, but if you would prefer to have a new thread, feel free to
start one (I'd suggest CC'ing me and Tsunakawa-san).
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Well, that definition seems unfriendly to me. I prefer the stance that
if you change the value for the parent, then future partitions inherit
that value.
That would be right when the storage property is an optional specification such as fillfactor. For example, when I run ALTER TABLE mytable SET (fillfactor = 70) and then CREATE TABLE mytable_p1 PARTITION OF mytable, I find it nice that the fillfactor os mytable_p1 is also 70 (but I won't complain if it isn't, since I can run ALTER TABLE SET on the parent table again.)
OTOH, CREATE TABLE and CREATE UNLOGGED TABLE is an explicit request to create a logged and unlogged relation respectively. I feel it a strange? if CREATE TABLE mytable_p1 PARTITION OF mytable creates an unlogged partition.
I got an impression from the discussion that some form of consensus on
the principle was made and you were trying to create a fix for REPLICA
IDENTITY. Do you think the principle was unclear and we should state
it first (partly to refresh people's memories)?I think the principle was sound -- namely, that we should make all those
commands recurse by default, and for cases where it matters, the
parent's setting should determine the default for future children.
Yeah, recurse by default sounded nice. But I didn't find a consensus related to "parent's setting should determine the default for future children." Could you point me there?
I'm kind of for it, but I'm hesitant to create the fix for all ALTER
actions, because it may take a lot of time and energy as you were
afraid. Can we define the principle, and then create individual fixes
independently based on that principle?That seems acceptable to me, as long as we get all changes in the same
release. What we don't want (according to my reading of that
discussion) is to change the semantics of a few subcommands in one
release, and the semantics of a few other subcommands in another
release.
All fixes at one release seems constricting to me... Reading from the following quote in the past discussion, I understood consistency is a must and simultaneous release is an ideal. So, I think we can release individual fixes separately. I don't think it won't worsen the situation for users at least.
"try to make them all work the same, ideally in one release, rather than changing them at different times, back-patching sometimes, and having no consistency in the details.
BTW, do you think you can continue to work on your REPLICA IDENTITY patch soon?
Regards
Takayuki Tsunakawa
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
I'm not sure how many more of such commands exist which require changes.
How about doing it this way?
1) Have a separate common thread listing the commands and specifying
clearly the agreed behaviour for such commands.
2) Whenever the separate patches are submitted(hopefully) by the
hackers for such commands, after the review we can make a note of that
patch in the common thread.
3) Once the patches for all the listed commands are
received(hopefully) , then they can be committed together in one
release.
That sounds interesting and nice. Having said that, I rather think it's better to release the fixes separately. Of course, we confirm the principle of consistency that individual fixes base on beforehand and what actions need sixing (SET LOGGED/UNLOGGED was missing in the past thread.)
Regards
Takayuki Tsunakawa
On Wed, Dec 09, 2020 at 09:52:17AM -0300, Alvaro Herrera wrote:
On 2020-Dec-09, tsunakawa.takay@fujitsu.com wrote:
The new partition will have a property specified when the user creates
it. That is, while the storage property of each storage unit
(=partition) is basically independent, ALTER TABLE on a partitioned
table is a convenient way to set the same property value to all its
underlying storage units.Well, that definition seems unfriendly to me. I prefer the stance that
if you change the value for the parent, then future partitions inherit
that value.
That's indeed more interesting from the user perspective. So +1 from
me.
--
Michael
On Wed, Dec 09, 2020 at 10:56:45AM -0300, Alvaro Herrera wrote:
Sounds good. I think this thread is a good place to collect those
patches, but if you would prefer to have a new thread, feel free to
start one (I'd suggest CC'ing me and Tsunakawa-san).
There is an entry listed in the CF for this thread:
https://commitfest.postgresql.org/31/2857/
And it seems to me that waiting on author is most appropriate here, so
I have changed the entry to reflect that.
--
Michael
From: Michael Paquier <michael@paquier.xyz>
On Wed, Dec 09, 2020 at 09:52:17AM -0300, Alvaro Herrera wrote:
Well, that definition seems unfriendly to me. I prefer the stance
that if you change the value for the parent, then future partitions
inherit that value.That's indeed more interesting from the user perspective. So +1 from me.
As I mentioned as below, some properties apply to that, and some don't.
--------------------------------------------------
That would be right when the storage property is an optional specification such as fillfactor. For example, when I run ALTER TABLE mytable SET (fillfactor = 70) and then CREATE TABLE mytable_p1 PARTITION OF mytable, I find it nice that the fillfactor os mytable_p1 is also 70 (but I won't complain if it isn't, since I can run ALTER TABLE SET on the parent table again.)
OTOH, CREATE TABLE and CREATE UNLOGGED TABLE is an explicit request to create a logged and unlogged relation respectively. I feel it a strange? if CREATE TABLE mytable_p1 PARTITION OF mytable creates an unlogged partition.
--------------------------------------------------
Anyway, I think I'll group ALTER TABLE/INDEX altering actions based on some common factors and suggest what would be a desirable behavior, asking for opinions. I'd like to explore the consensus on the basic policy for fixes. Then, I hope we will be able to work on fixes for each ALTER action in patches that can be released separately. I'd like to regist requiring all fixes to be arranged at once, since that may become a high bar for those who volunteer to fix some of the actions. (Even a committer Alvaro-san struggled to fix one action, ALTER TABLE REPLICA IDENTITY.)
Regards
Takayuki Tsunakawa
At Sat, 26 Dec 2020 01:56:02 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in
From: Michael Paquier <michael@paquier.xyz>
On Wed, Dec 09, 2020 at 09:52:17AM -0300, Alvaro Herrera wrote:
Well, that definition seems unfriendly to me. I prefer the stance
that if you change the value for the parent, then future partitions
inherit that value.That's indeed more interesting from the user perspective. So +1 from me.
As I mentioned as below, some properties apply to that, and some don't.
--------------------------------------------------
That would be right when the storage property is an optional
specification such as fillfactor. For example, when I run ALTER
TABLE mytable SET (fillfactor = 70) and then CREATE TABLE mytable_p1
PARTITION OF mytable, I find it nice that the fillfactor os
mytable_p1 is also 70 (but I won't complain if it isn't, since I can
run ALTER TABLE SET on the parent table again.)OTOH, CREATE TABLE and CREATE UNLOGGED TABLE is an explicit request
to create a logged and unlogged relation respectively. I feel it a
strange? if CREATE TABLE mytable_p1 PARTITION OF mytable creates an
unlogged partition.
--------------------------------------------------
"CREATE TABLE" is not "CREATE LOGGED TABLE". We can assume that as
"CREATE <default logged-ness> TABLE", where the default logged-ness
varies according to context. Or it might have been so since the
beginning. Currently we don't have the syntax "CREATE LOGGED TABLE",
but we can add that syntax.
Anyway, I think I'll group ALTER TABLE/INDEX altering actions based
on some common factors and suggest what would be a desirable
behavior, asking for opinions. I'd like to explore the consensus on
the basic policy for fixes. Then, I hope we will be able to work on
fixes for each ALTER action in patches that can be released
separately. I'd like to regist requiring all fixes to be arranged
at once, since that may become a high bar for those who volunteer to
fix some of the actions. (Even a committer Alvaro-san struggled to
fix one action, ALTER TABLE REPLICA IDENTITY.)
I'd vote +1 to:
ALTER TABLE/INDEX on a parent recurses to all descendants. ALTER
TABLE/INDEX ONLY doesn't, and the change takes effect on future
children.
We pursue relasing all fixes at once but we might release all fixes
other than some items that cannot be fixed for some technical reasons
at the time, like REPLICA IDENITTY.
I'm not sure how long we will wait for the time of release, though.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
"CREATE TABLE" is not "CREATE LOGGED TABLE". We can assume that as
"CREATE <default logged-ness> TABLE", where the default logged-ness
varies according to context. Or it might have been so since the beginning.
Currently we don't have the syntax "CREATE LOGGED TABLE", but we can add
that syntax.
Yes, I thought of that possibility after a while too. But I felt a bit(?) hesitant to do it considering back-patching. Also, the idea requires ALTER TABLE ATTACH PARTITION will have to modify the logged-ness property of the target partition and its subpartitions with that of the parent partitioned table. However, your idea is one candidate worth pursuing, including whether or not to back-patch what.
We pursue relasing all fixes at once but we might release all fixes other than
some items that cannot be fixed for some technical reasons at the time, like
REPLICA IDENITTY.I'm not sure how long we will wait for the time of release, though.
Anyway, we have to define the ideal behavior for each ALTER action based on some comprehensible principle. Yeah... this may become a long, tiring journey. (I anticipate some difficulty and compromise in reaching agreement, as was seen in the past discussion for the fix for ALTER TABLE REPLICA IDENTITY. Scary)
FWIW, I wonder why Postgres decided to allow different logical structure of tables such as DEFAULT values and constraints between the parent partitioned table and a child partition. That extra flexibility might stand in the way to consensus. I think it'd be easy to understand that partitions are simply physically independent containers that have identical logical structure.
Regards
Takayuki Tsunakawa
At Wed, 27 Jan 2021 05:30:29 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
"CREATE TABLE" is not "CREATE LOGGED TABLE". We can assume that as
"CREATE <default logged-ness> TABLE", where the default logged-ness
varies according to context. Or it might have been so since the beginning.
Currently we don't have the syntax "CREATE LOGGED TABLE", but we can add
that syntax.Yes, I thought of that possibility after a while too. But I felt a bit(?) hesitant to do it considering back-patching. Also, the idea requires ALTER TABLE ATTACH PARTITION will have to modify the logged-ness property of the target partition and its subpartitions with that of the parent partitioned table. However, your idea is one candidate worth pursuing, including whether or not to back-patch what.
I think this and all possible similar changes won't be back-patched at
all. It is a desaster for any establish version to change this kind
of behavior.
We pursue relasing all fixes at once but we might release all fixes other than
some items that cannot be fixed for some technical reasons at the time, like
REPLICA IDENITTY.I'm not sure how long we will wait for the time of release, though.
Anyway, we have to define the ideal behavior for each ALTER action based on some comprehensible principle. Yeah... this may become a long, tiring journey. (I anticipate some difficulty and compromise in reaching agreement, as was seen in the past discussion for the fix for ALTER TABLE REPLICA IDENTITY. Scary)
It seems to me that we have agreed that the ideal behavior for ALTER
TABLE on a parent to propagate to descendants. An observed objection
is that behavior is dangerous for operations that requires large
amount of resources that could lead to failure after elapsing a long
time. The revealed difficulty of REPLICA IDENTITY is regarded as a
technical issue that should be overcome.
FWIW, I wonder why Postgres decided to allow different logical structure of tables such as DEFAULT values and constraints between the parent partitioned table and a child partition. That extra flexibility might stand in the way to consensus. I think it'd be easy to understand that partitions are simply physically independent containers that have identical logical structure.
I understand that -- once upon a time the "traditional" partitioning
was mere a use case of inheritance tables. Parents and children are
basically independent from each other other than inherited attributes.
The nature of table inheritance compel column addition to a parent to
inherit to children, but other changes need not to be propagated. As
time goes by, partition becomes the major use and new features added
at the time are affected by the recognition. After the appearance of
native partitioning, the table inheritance seems to be regarded as a
heritage that we need to maintain.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On 1/27/21 1:00 AM, Kyotaro Horiguchi wrote:
At Wed, 27 Jan 2021 05:30:29 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
"CREATE TABLE" is not "CREATE LOGGED TABLE". We can assume that as
"CREATE <default logged-ness> TABLE", where the default logged-ness
varies according to context. Or it might have been so since the beginning.
Currently we don't have the syntax "CREATE LOGGED TABLE", but we can add
that syntax.Yes, I thought of that possibility after a while too. But I felt a bit(?) hesitant to do it considering back-patching. Also, the idea requires ALTER TABLE ATTACH PARTITION will have to modify the logged-ness property of the target partition and its subpartitions with that of the parent partitioned table. However, your idea is one candidate worth pursuing, including whether or not to back-patch what.
I think this and all possible similar changes won't be back-patched at
all. It is a desaster for any establish version to change this kind
of behavior.We pursue relasing all fixes at once but we might release all fixes other than
some items that cannot be fixed for some technical reasons at the time, like
REPLICA IDENITTY.I'm not sure how long we will wait for the time of release, though.
Anyway, we have to define the ideal behavior for each ALTER action based on some comprehensible principle. Yeah... this may become a long, tiring journey. (I anticipate some difficulty and compromise in reaching agreement, as was seen in the past discussion for the fix for ALTER TABLE REPLICA IDENTITY. Scary)
It seems to me that we have agreed that the ideal behavior for ALTER
TABLE on a parent to propagate to descendants. An observed objection
is that behavior is dangerous for operations that requires large
amount of resources that could lead to failure after elapsing a long
time. The revealed difficulty of REPLICA IDENTITY is regarded as a
technical issue that should be overcome.
This thread seems to be stalled. Since it represents a bug fix is there
something we can do in the meantime while a real solution is worked out?
Perhaps just inform the user that nothing was done? Or get something
into the documentation?
Regards,
--
-David
david@pgmasters.net
On Thu, Mar 25, 2021 at 10:21 PM David Steele <david@pgmasters.net> wrote:
This thread seems to be stalled. Since it represents a bug fix is there
something we can do in the meantime while a real solution is worked out?Perhaps just inform the user that nothing was done? Or get something
into the documentation?
Attaching a small patch that emits a warning when the persistence
setting of a partitioned table is changed and also added a note into
the docs. Please have a look at it.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v1-0001-Emit-warning-when-partitioned-table-s-persistence.patchapplication/octet-stream; name=v1-0001-Emit-warning-when-partitioned-table-s-persistence.patchDownload
From 6d68410e597a11d07c224501075d378795ce2970 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Fri, 2 Apr 2021 10:10:34 +0530
Subject: [PATCH v1] Emit warning when partitioned table's persistence is
changed
Changing persistence setting(LOGGED/UNLOGGED) for a partitioned
table does not affect existing or future partitions. If required,
set the persistence setting for each partition separately. Perhaps
this could be improved.
---
doc/src/sgml/ref/alter_table.sgml | 5 ++++-
src/backend/commands/tablecmds.c | 20 ++++++++++++++++++++
src/test/regress/expected/alter_table.out | 13 +++++++++++++
src/test/regress/sql/alter_table.sql | 10 ++++++++++
4 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 07e37a6dc8..538e42a666 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -712,7 +712,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<para>
This form changes the table from unlogged to logged or vice-versa
(see <xref linkend="sql-createtable-unlogged"/>). It cannot be applied
- to a temporary table.
+ to a temporary table. Changing persistence setting for a partitioned
+ table does not affect existing or future partitions. If required, set the
+ persistence setting for each partition separately. Perhaps this could be
+ improved.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 88a68a4697..b017c967cb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4457,6 +4457,16 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change persistence setting twice")));
+ /*
+ * Changing persistence setting for a partitioned table does not
+ * affect existing or future partitions. If required, set the
+ * persistence setting for each partition separately. XXX Perhaps
+ * this could be improved someday.
+ */
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ereport(WARNING,
+ (errmsg("changing persistence setting for a partitioned table does not affect existing or future partitions"),
+ errhint("If required, set the persistence setting for each partition separately.")));
tab->chgPersistence = ATPrepChangePersistence(rel, true);
/* force rewrite if necessary; see comment in ATRewriteTables */
if (tab->chgPersistence)
@@ -4472,6 +4482,16 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change persistence setting twice")));
+ /*
+ * Changing persistence setting for a partitioned table does not
+ * affect existing or future partitions. If required, set the
+ * persistence setting for each partition separately. XXX Perhaps
+ * this could be improved someday.
+ */
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ereport(WARNING,
+ (errmsg("changing persistence setting for a partitioned table does not affect existing or future partitions"),
+ errhint("If required, set the persistence setting for each partition separately.")));
tab->chgPersistence = ATPrepChangePersistence(rel, false);
/* force rewrite if necessary; see comment in ATRewriteTables */
if (tab->chgPersistence)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index ec14b060a6..ff10aef087 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3472,6 +3472,19 @@ ALTER TABLE logged1 SET UNLOGGED; -- silently do nothing
DROP TABLE logged3;
DROP TABLE logged2;
DROP TABLE logged1;
+-- setting partitioned table's logged/unlogged ness has no effect on existing
+-- or future partitions, so we just emit a warning.
+CREATE TABLE part_logged_parent (key integer PRIMARY KEY, val text)
+ PARTITION BY RANGE (key);
+CREATE TABLE part_logged_child1 PARTITION OF part_logged_parent
+ FOR VALUES FROM (1) TO (10);
+ALTER TABLE part_logged_parent SET UNLOGGED; -- warning is emitted
+WARNING: changing persistence setting for a partitioned table does not affect existing or future partitions
+HINT: If required, set the persistence setting for each partition separately.
+ALTER TABLE part_logged_parent SET LOGGED; -- warning is emitted
+WARNING: changing persistence setting for a partitioned table does not affect existing or future partitions
+HINT: If required, set the persistence setting for each partition separately.
+DROP TABLE part_logged_parent;
-- 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 7a9c9252dc..89fdbd2b88 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2189,6 +2189,16 @@ DROP TABLE logged3;
DROP TABLE logged2;
DROP TABLE logged1;
+-- setting partitioned table's logged/unlogged ness has no effect on existing
+-- or future partitions, so we just emit a warning.
+CREATE TABLE part_logged_parent (key integer PRIMARY KEY, val text)
+ PARTITION BY RANGE (key);
+CREATE TABLE part_logged_child1 PARTITION OF part_logged_parent
+ FOR VALUES FROM (1) TO (10);
+ALTER TABLE part_logged_parent SET UNLOGGED; -- warning is emitted
+ALTER TABLE part_logged_parent SET LOGGED; -- warning is emitted
+DROP TABLE part_logged_parent;
+
-- test ADD COLUMN IF NOT EXISTS
CREATE TABLE test_add_column(c1 integer);
\d test_add_column
--
2.25.1
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Attaching a small patch that emits a warning when the persistence setting of a
partitioned table is changed and also added a note into the docs. Please have a
look at it.
Thank you. However, I think I'll withdraw this CF entry since I'm not sure I can take time for the time being to research and fix other various ALTER TABLE/INDEX issues. I'll mark this as withdrawn around the middle of next week unless someone wants to continue this.
Regards
Takayuki Tsunakawa