Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sat, May 25, 2024 at 3:53 PM Alexander Korotkov wrote:
> On Fri, May 24, 2024 at 11:00 PM Alexander Lakhin wrote:
> >
> > 24.05.2024 22:29, Tom Lane wrote:
> > > The partition_split test has unstable results, as shown at [1].
> > > I suggest adding "ORDER BY conname" to the two queries shown
> > > to fail there. Better look at other queries in the test for
> > > possible similar problems, too.
> >
> > Yes, I've just reproduced it on an aarch64 device as follows:
> > echo "autovacuum_naptime = 1
> > autovacuum_vacuum_threshold = 1
> > autovacuum_analyze_threshold = 1
> > " > ~/temp.config
> > TEMP_CONFIG=~/temp.config TESTS="$(printf 'partition_split %.0s' `seq
> > 100`)" make -s check-tests
> > ...
> > ok 80- partition_split 749 ms
> > not ok 81- partition_split 728 ms
> > ok 82- partition_split 732 ms
> >
> > $ cat src/test/regress/regression.diffs
> > diff -U3 .../src/test/regress/expected/partition_split.out
> > .../src/test/regress/results/partition_split.out
> > --- .../src/test/regress/expected/partition_split.out 2024-05-15
> > 17:15:57.171999830 +
> > +++ .../src/test/regress/results/partition_split.out2024-05-24
> > 19:28:37.32749 +
> > @@ -625,8 +625,8 @@
> > SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint
> > WHERE conrelid =
> > 'sales_feb_mar_apr2022'::regclass::oid;
> > pg_get_constraintdef | conname | conkey
> >
> > -+-+
> > - CHECK ((sales_amount > 1)) |
> > sales_range_sales_amount_check | {2}
> >FOREIGN KEY (salesperson_id) REFERENCES salespeople(salesperson_id) |
> > sales_range_salesperson_id_fkey | {1}
> > + CHECK ((sales_amount > 1)) |
> > sales_range_sales_amount_check | {2}
> > (2 rows)
> >
> > ALTER TABLE sales_range SPLIT PARTITION sales_feb_mar_apr2022 INTO
>
> Tom, Alexander, thank you for spotting this.
> I'm going to care about it later today.
ORDER BY is added in d53a4286d7 in these queries altogether with other
catalog queries with potentially unstable result.
--
Regards,
Alexander Korotkov
Supabase
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sat, May 25, 2024 at 8:53 PM Justin Pryzby wrote: > On Fri, May 03, 2024 at 04:32:25PM +0300, Alexander Korotkov wrote: > > On Fri, May 3, 2024 at 4:23 PM Justin Pryzby wrote: > > > Note that the error that led to "EXCLUDING IDENTITY" is being discused > > > over here: > > > https://www.postgresql.org/message-id/3b8a9dc1-bbc7-0ef5-6863-c432afac7...@gmail.com > > > > > > It's possible that once that's addressed, the exclusion should be > > > removed here, too. > > > > +1 > > Can EXCLUDING IDENTITY be removed now ? > > I wasn't able to find why it was needed - at one point, I think there > was a test case that threw an error, but now when I remove the EXCLUDE, > nothing goes wrong. Yes, it was broken before [1][2], but now it seems to work. At the same time, I'm not sure if we need to remove the EXCLUDE now. IDENTITY is anyway successfully created when the new partition gets attached. Links. 1. https://www.postgresql.org/message-id/171085360143.2046436.7217841141682511557.p...@coridan.postgresql.org 2. https://www.postgresql.org/message-id/flat/ZiGH0xc1lxJ71ZfB%40pryzbyj2023#297b6aef85cb089abb38e9b1a9a7 -- Regards, Alexander Korotkov Supabase
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Fri, May 03, 2024 at 04:32:25PM +0300, Alexander Korotkov wrote: > On Fri, May 3, 2024 at 4:23 PM Justin Pryzby wrote: > > Note that the error that led to "EXCLUDING IDENTITY" is being discused > > over here: > > https://www.postgresql.org/message-id/3b8a9dc1-bbc7-0ef5-6863-c432afac7...@gmail.com > > > > It's possible that once that's addressed, the exclusion should be > > removed here, too. > > +1 Can EXCLUDING IDENTITY be removed now ? I wasn't able to find why it was needed - at one point, I think there was a test case that threw an error, but now when I remove the EXCLUDE, nothing goes wrong. -- Justin
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Fri, May 24, 2024 at 11:00 PM Alexander Lakhin wrote:
>
> 24.05.2024 22:29, Tom Lane wrote:
> > The partition_split test has unstable results, as shown at [1].
> > I suggest adding "ORDER BY conname" to the two queries shown
> > to fail there. Better look at other queries in the test for
> > possible similar problems, too.
>
> Yes, I've just reproduced it on an aarch64 device as follows:
> echo "autovacuum_naptime = 1
> autovacuum_vacuum_threshold = 1
> autovacuum_analyze_threshold = 1
> " > ~/temp.config
> TEMP_CONFIG=~/temp.config TESTS="$(printf 'partition_split %.0s' `seq 100`)"
> make -s check-tests
> ...
> ok 80- partition_split 749 ms
> not ok 81- partition_split 728 ms
> ok 82- partition_split 732 ms
>
> $ cat src/test/regress/regression.diffs
> diff -U3 .../src/test/regress/expected/partition_split.out
> .../src/test/regress/results/partition_split.out
> --- .../src/test/regress/expected/partition_split.out 2024-05-15
> 17:15:57.171999830 +
> +++ .../src/test/regress/results/partition_split.out2024-05-24
> 19:28:37.32749 +
> @@ -625,8 +625,8 @@
> SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE
> conrelid =
> 'sales_feb_mar_apr2022'::regclass::oid;
> pg_get_constraintdef | conname | conkey
>
> -+-+
> - CHECK ((sales_amount > 1)) |
> sales_range_sales_amount_check | {2}
>FOREIGN KEY (salesperson_id) REFERENCES salespeople(salesperson_id) |
> sales_range_salesperson_id_fkey | {1}
> + CHECK ((sales_amount > 1)) |
> sales_range_sales_amount_check | {2}
> (2 rows)
>
> ALTER TABLE sales_range SPLIT PARTITION sales_feb_mar_apr2022 INTO
Tom, Alexander, thank you for spotting this.
I'm going to care about it later today.
--
Regards,
Alexander Korotkov
Supabase
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hello,
24.05.2024 22:29, Tom Lane wrote:
The partition_split test has unstable results, as shown at [1].
I suggest adding "ORDER BY conname" to the two queries shown
to fail there. Better look at other queries in the test for
possible similar problems, too.
Yes, I've just reproduced it on an aarch64 device as follows:
echo "autovacuum_naptime = 1
autovacuum_vacuum_threshold = 1
autovacuum_analyze_threshold = 1
" > ~/temp.config
TEMP_CONFIG=~/temp.config TESTS="$(printf 'partition_split %.0s' `seq 100`)"
make -s check-tests
...
ok 80 - partition_split 749 ms
not ok 81 - partition_split 728 ms
ok 82 - partition_split 732 ms
$ cat src/test/regress/regression.diffs
diff -U3 .../src/test/regress/expected/partition_split.out
.../src/test/regress/results/partition_split.out
--- .../src/test/regress/expected/partition_split.out 2024-05-15
17:15:57.171999830 +
+++ .../src/test/regress/results/partition_split.out 2024-05-24
19:28:37.32749 +
@@ -625,8 +625,8 @@
SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid =
'sales_feb_mar_apr2022'::regclass::oid;
pg_get_constraintdef | conname | conkey
-+-+
- CHECK ((sales_amount > 1)) |
sales_range_sales_amount_check | {2}
FOREIGN KEY (salesperson_id) REFERENCES salespeople(salesperson_id) |
sales_range_salesperson_id_fkey | {1}
+ CHECK ((sales_amount > 1)) |
sales_range_sales_amount_check | {2}
(2 rows)
ALTER TABLE sales_range SPLIT PARTITION sales_feb_mar_apr2022 INTO
Best regards,
Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
The partition_split test has unstable results, as shown at [1]. I suggest adding "ORDER BY conname" to the two queries shown to fail there. Better look at other queries in the test for possible similar problems, too. regards, tom lane [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jackdaw=2024-05-24%2015%3A58%3A17
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi, Pavel! On Fri, May 17, 2024 at 2:02 PM Pavel Borisov wrote: > On Fri, 17 May 2024 at 14:05, Alexander Korotkov wrote: >> >> On Tue, May 14, 2024 at 5:49 PM Justin Pryzby wrote: >> > On Thu, May 09, 2024 at 12:51:32AM +0300, Alexander Korotkov wrote: >> > > > > However, parent's table extended statistics already covers all its >> > > > > children. >> > > > >> > > > => That's the wrong explanation. It's not that "stats on the parent >> > > > table cover its children". It's that there are two types of stats: >> > > > stats for the "table hierarchy" and stats for the individual table. >> > > > That's true for single-column stats as well as for extended stats. >> > > > In both cases, that's indicated by the inh flag in the code and in the >> > > > catalog. >> > > > >> > > > The right explanation is that extended stats on partitioned tables are >> > > > not similar to indexes. Indexes on parent table are nothing other than >> > > > a mechanism to create indexes on the child tables. That's not true for >> > > > stats. >> > > > >> > > > See also my prior messages >> > > > ZiJW1g2nbQs9ekwK@pryzbyj2023 >> > > > Zi5Msg74C61DjJKW@pryzbyj2023 >> > > >> > > Yes, I understand that parents pg_statistic entry with stainherit == >> > > true includes statistics for the children. I tried to express this by >> > > word "covers". But you're right, this is the wrong explanation. >> > > >> > > Can I, please, ask you to revise the patch? >> > >> > I tried to make this clear but it'd be nice if someone (Tomas/Alvaro?) >> > would check that this says what's wanted. >> >> Thank you! >> >> I've assembled the patches with the pending fixes. >> 0001 – The patch by Dmitry Koval for fixing detection of name >> collision in SPLIT partition operation. Also, I found that name >> collision detection doesn't work well for MERGE partitions. I've >> added fix for that to this patch as well. >> 0002 -– Patch for skipping copy of extended statistics. I would >> appreciate more feedback about wording, but I'd like to get a correct >> behavior into the source tree sooner. If the docs and/or comments >> need further improvements, we can fix that later. >> >> I'm going to push both if no objections. > > Thank you for working on this patch set! > > Some minor things: > 0001: > partition_split.sql > 157 +-- Check that detection, that the new partition has the same name as one > of > 158 +-- the merged partitions, works correctly for temporary partitions > Test for split with comment for merge. Maybe better something like: > "Split partition of a temporary table when one of the partitions after split > has the same name as the partition being split" Thank you, fixed as proposed. > 0002: > analgous -> analogous (maybe better using "like" instead of "analogous to") > heirarchy -> hierarchy Changed "are not analgous to" to "don't behave like". > alter_table.sgml: > Maybe in documentation it's better not to provide reasoning, just state how > it works: > for consistency with CREATE TABLE PARTITION OF -> similar > to CREATE TABLE PARTITION OF I'd like to keep this. This is the question, which should naturally arise when you read: "Why this is not just INCLUDING ALL?" -- Regards, Alexander Korotkov Supabase v3-0001-Fix-the-name-collision-detection-in-MERGE-SPLIT-p.patch Description: Binary data v3-0002-Don-t-copy-extended-statistics-during-MERGE-SPLIT.patch Description: Binary data
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi, Alexander: On Fri, 17 May 2024 at 14:05, Alexander Korotkov wrote: > On Tue, May 14, 2024 at 5:49 PM Justin Pryzby > wrote: > > On Thu, May 09, 2024 at 12:51:32AM +0300, Alexander Korotkov wrote: > > > > > However, parent's table extended statistics already covers all its > > > > > children. > > > > > > > > => That's the wrong explanation. It's not that "stats on the parent > > > > table cover its children". It's that there are two types of stats: > > > > stats for the "table hierarchy" and stats for the individual table. > > > > That's true for single-column stats as well as for extended stats. > > > > In both cases, that's indicated by the inh flag in the code and in > the > > > > catalog. > > > > > > > > The right explanation is that extended stats on partitioned tables > are > > > > not similar to indexes. Indexes on parent table are nothing other > than > > > > a mechanism to create indexes on the child tables. That's not true > for > > > > stats. > > > > > > > > See also my prior messages > > > > ZiJW1g2nbQs9ekwK@pryzbyj2023 > > > > Zi5Msg74C61DjJKW@pryzbyj2023 > > > > > > Yes, I understand that parents pg_statistic entry with stainherit == > > > true includes statistics for the children. I tried to express this by > > > word "covers". But you're right, this is the wrong explanation. > > > > > > Can I, please, ask you to revise the patch? > > > > I tried to make this clear but it'd be nice if someone (Tomas/Alvaro?) > > would check that this says what's wanted. > > Thank you! > > I've assembled the patches with the pending fixes. > 0001 – The patch by Dmitry Koval for fixing detection of name > collision in SPLIT partition operation. Also, I found that name > collision detection doesn't work well for MERGE partitions. I've > added fix for that to this patch as well. > 0002 -– Patch for skipping copy of extended statistics. I would > appreciate more feedback about wording, but I'd like to get a correct > behavior into the source tree sooner. If the docs and/or comments > need further improvements, we can fix that later. > > I'm going to push both if no objections. > Thank you for working on this patch set! Some minor things: 0001: partition_split.sql 157 +-- Check that detection, that the new partition has the same name as one of 158 +-- the merged partitions, works correctly for temporary partitions Test for split with comment for merge. Maybe better something like: "Split partition of a temporary table when one of the partitions after split has the same name as the partition being split" 0002: analgous -> analogous (maybe better using "like" instead of "analogous to") heirarchy -> hierarchy alter_table.sgml: Maybe in documentation it's better not to provide reasoning, just state how it works: for consistency with CREATE TABLE PARTITION OF -> similar to CREATE TABLE PARTITION OF Regards, Pavel Borisov
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Tue, May 14, 2024 at 5:49 PM Justin Pryzby wrote: > On Thu, May 09, 2024 at 12:51:32AM +0300, Alexander Korotkov wrote: > > > > However, parent's table extended statistics already covers all its > > > > children. > > > > > > => That's the wrong explanation. It's not that "stats on the parent > > > table cover its children". It's that there are two types of stats: > > > stats for the "table hierarchy" and stats for the individual table. > > > That's true for single-column stats as well as for extended stats. > > > In both cases, that's indicated by the inh flag in the code and in the > > > catalog. > > > > > > The right explanation is that extended stats on partitioned tables are > > > not similar to indexes. Indexes on parent table are nothing other than > > > a mechanism to create indexes on the child tables. That's not true for > > > stats. > > > > > > See also my prior messages > > > ZiJW1g2nbQs9ekwK@pryzbyj2023 > > > Zi5Msg74C61DjJKW@pryzbyj2023 > > > > Yes, I understand that parents pg_statistic entry with stainherit == > > true includes statistics for the children. I tried to express this by > > word "covers". But you're right, this is the wrong explanation. > > > > Can I, please, ask you to revise the patch? > > I tried to make this clear but it'd be nice if someone (Tomas/Alvaro?) > would check that this says what's wanted. Thank you! I've assembled the patches with the pending fixes. 0001 – The patch by Dmitry Koval for fixing detection of name collision in SPLIT partition operation. Also, I found that name collision detection doesn't work well for MERGE partitions. I've added fix for that to this patch as well. 0002 -– Patch for skipping copy of extended statistics. I would appreciate more feedback about wording, but I'd like to get a correct behavior into the source tree sooner. If the docs and/or comments need further improvements, we can fix that later. I'm going to push both if no objections. Links. 1. https://www.postgresql.org/message-id/147426d9-b793-4571-a5e5-7438affeeb5a%40postgrespro.ru -- Regards, Alexander Korotkov Supabase v2-0001-Fix-the-name-collision-detection-in-MERGE-SPLIT-p.patch Description: Binary data v2-0002-Don-t-copy-extended-statistics-during-MERGE-SPLIT.patch Description: Binary data
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Thu, May 09, 2024 at 12:51:32AM +0300, Alexander Korotkov wrote: > > > However, parent's table extended statistics already covers all its > > > children. > > > > => That's the wrong explanation. It's not that "stats on the parent > > table cover its children". It's that there are two types of stats: > > stats for the "table hierarchy" and stats for the individual table. > > That's true for single-column stats as well as for extended stats. > > In both cases, that's indicated by the inh flag in the code and in the > > catalog. > > > > The right explanation is that extended stats on partitioned tables are > > not similar to indexes. Indexes on parent table are nothing other than > > a mechanism to create indexes on the child tables. That's not true for > > stats. > > > > See also my prior messages > > ZiJW1g2nbQs9ekwK@pryzbyj2023 > > Zi5Msg74C61DjJKW@pryzbyj2023 > > Yes, I understand that parents pg_statistic entry with stainherit == > true includes statistics for the children. I tried to express this by > word "covers". But you're right, this is the wrong explanation. > > Can I, please, ask you to revise the patch? I tried to make this clear but it'd be nice if someone (Tomas/Alvaro?) would check that this says what's wanted. -- Justin >From 265207e5bdb215600ce5d7b45f627bc41fc2bc26 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Wed, 8 May 2024 20:32:20 +0300 Subject: [PATCH] Don't copy extended statistics during MERGE/SPLIT partition operations When MERGE/SPLIT created new partitions, it was cloning the extended statistics of the parent table. However, extended stats on partitioned tables are not analgous to indexes on partitioned tables (which exist only to create physical indexes on child tables). Rather, extended stats on a parent 1) cause extended stats to be collected and computed across the whole partition heirarchy, and 2) do not cause extended stats to be computed for the individual partitions. "CREATE TABLE ... PARTITION OF" command doesn't copy extended statistics. This commit makes createPartitionTable() behave consistently. Reported-by: Justin Pryzby Discussion: https://postgr.es/m/ZiJW1g2nbQs9ekwK%40pryzbyj2023 --- doc/src/sgml/ref/alter_table.sgml | 9 +++-- src/backend/commands/tablecmds.c | 8 +--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 891fa4a7a04..313c722ee7f 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -1154,9 +1154,12 @@ WITH ( MODULUS numeric_literal, REM The new partitions will be created the same as tables created with the - SQL command CREATE TABLE partition_nameN (LIKE name INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY). + SQL command CREATE TABLE partition_nameN (LIKE name INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY EXCLUDING STATISTICS). The indexes and identity are created later, after moving the data into the new partitions. + Extended statistics aren't copied from the parent table, for consistency with + CREATE TABLE PARTITION OF. + New partitions will have the same table access method as the parent. If the parent table is persistent then new partitions are created persistent. If the parent table is temporary then new partitions @@ -1224,9 +1227,11 @@ WITH ( MODULUS numeric_literal, REM The new partition will be created the same as a table created with the - SQL command CREATE TABLE partition_name (LIKE name INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY). + SQL command CREATE TABLE partition_name (LIKE name INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY EXCLUDING STATISTICS). The indexes and identity are created later, after moving the data into the new partition. + Extended statistics aren't copied from the parent table, for consistency with + CREATE TABLE PARTITION OF. The new partition will have the same table access method as the parent. If the parent table is persistent then the new partition is created persistent. If the parent table is temporary then the new partition diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 79c9c031833..50fc54cb309 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -20419,7 +20419,7 @@ moveSplitTableRows(Relation rel, Relation splitRel, List *partlist, List *newPar * (newPartName) like table (modelRel) * * Emulates command: CREATE [TEMP] TABLE (LIKE - * INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY) + * INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY EXCLUDING STATISTICS) * * Also, this function sets the new partition access method same as parent * table access methods (similarly to CREATE TABLE ... PARTITION OF). It @@ -20463,9 +20463,11 @@ createPartitionTable(RangeVar *newPartName, Relation
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Mon, May 13, 2024 at 12:45 PM Dmitry Koval wrote: > 13.05.2024 11:45, Daniel Gustafsson пишет: > > Commit 3ca43dbbb67f which adds the permission checks seems to cause > > conflicts > > in the pg_upgrade tests > > Thanks! > > It will probably be enough to rename the roles: > > regress_partition_merge_alice -> regress_partition_split_alice > regress_partition_merge_bob -> regress_partition_split_bob Thanks to Danial for spotting this. Thanks to Dmitry for the proposed fix. The actual problem appears to be a bit more complex. Additionally to the role names, the lack of permissions on schemas lead to creation of tables in public schema and potential conflict there. Fixed in 2a679ae94e. -- Regards, Alexander Korotkov
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi! 13.05.2024 11:45, Daniel Gustafsson пишет: Commit 3ca43dbbb67f which adds the permission checks seems to cause conflicts in the pg_upgrade tests Thanks! It will probably be enough to rename the roles: regress_partition_merge_alice -> regress_partition_split_alice regress_partition_merge_bob -> regress_partition_split_bob (changes in attachment) -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.comFrom f307add79397bcfe20f7c779e77630fb0df73020 Mon Sep 17 00:00:00 2001 From: Koval Dmitry Date: Mon, 13 May 2024 12:32:43 +0300 Subject: [PATCH v1] Rename roles to avoid conflicts in concurrent work --- src/test/regress/expected/partition_split.out | 20 +-- src/test/regress/sql/partition_split.sql | 20 +-- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/test/regress/expected/partition_split.out b/src/test/regress/expected/partition_split.out index b1108c92a2..999e923ef1 100644 --- a/src/test/regress/expected/partition_split.out +++ b/src/test/regress/expected/partition_split.out @@ -1516,33 +1516,33 @@ DROP TABLE t; DROP ACCESS METHOD partition_split_heap; -- Test permission checks. The user needs to own the parent table and the -- the partition to split to do the split. -CREATE ROLE regress_partition_merge_alice; -CREATE ROLE regress_partition_merge_bob; -SET SESSION AUTHORIZATION regress_partition_merge_alice; +CREATE ROLE regress_partition_split_alice; +CREATE ROLE regress_partition_split_bob; +SET SESSION AUTHORIZATION regress_partition_split_alice; CREATE TABLE t (i int) PARTITION BY RANGE (i); CREATE TABLE tp_0_2 PARTITION OF t FOR VALUES FROM (0) TO (2); -SET SESSION AUTHORIZATION regress_partition_merge_bob; +SET SESSION AUTHORIZATION regress_partition_split_bob; ALTER TABLE t SPLIT PARTITION tp_0_2 INTO (PARTITION tp_0_1 FOR VALUES FROM (0) TO (1), PARTITION tp_1_2 FOR VALUES FROM (1) TO (2)); ERROR: must be owner of table t RESET SESSION AUTHORIZATION; -ALTER TABLE t OWNER TO regress_partition_merge_bob; -SET SESSION AUTHORIZATION regress_partition_merge_bob; +ALTER TABLE t OWNER TO regress_partition_split_bob; +SET SESSION AUTHORIZATION regress_partition_split_bob; ALTER TABLE t SPLIT PARTITION tp_0_2 INTO (PARTITION tp_0_1 FOR VALUES FROM (0) TO (1), PARTITION tp_1_2 FOR VALUES FROM (1) TO (2)); ERROR: must be owner of table tp_0_2 RESET SESSION AUTHORIZATION; -ALTER TABLE tp_0_2 OWNER TO regress_partition_merge_bob; -SET SESSION AUTHORIZATION regress_partition_merge_bob; +ALTER TABLE tp_0_2 OWNER TO regress_partition_split_bob; +SET SESSION AUTHORIZATION regress_partition_split_bob; ALTER TABLE t SPLIT PARTITION tp_0_2 INTO (PARTITION tp_0_1 FOR VALUES FROM (0) TO (1), PARTITION tp_1_2 FOR VALUES FROM (1) TO (2)); RESET SESSION AUTHORIZATION; DROP TABLE t; -DROP ROLE regress_partition_merge_alice; -DROP ROLE regress_partition_merge_bob; +DROP ROLE regress_partition_split_alice; +DROP ROLE regress_partition_split_bob; -- Check extended statistics aren't copied from the parent table to new -- partitions. CREATE TABLE t (i int, j int) PARTITION BY RANGE (i); diff --git a/src/test/regress/sql/partition_split.sql b/src/test/regress/sql/partition_split.sql index 7f231b0d39..be4a785b75 100644 --- a/src/test/regress/sql/partition_split.sql +++ b/src/test/regress/sql/partition_split.sql @@ -896,36 +896,36 @@ DROP ACCESS METHOD partition_split_heap; -- Test permission checks. The user needs to own the parent table and the -- the partition to split to do the split. -CREATE ROLE regress_partition_merge_alice; -CREATE ROLE regress_partition_merge_bob; +CREATE ROLE regress_partition_split_alice; +CREATE ROLE regress_partition_split_bob; -SET SESSION AUTHORIZATION regress_partition_merge_alice; +SET SESSION AUTHORIZATION regress_partition_split_alice; CREATE TABLE t (i int) PARTITION BY RANGE (i); CREATE TABLE tp_0_2 PARTITION OF t FOR VALUES FROM (0) TO (2); -SET SESSION AUTHORIZATION regress_partition_merge_bob; +SET SESSION AUTHORIZATION regress_partition_split_bob; ALTER TABLE t SPLIT PARTITION tp_0_2 INTO (PARTITION tp_0_1 FOR VALUES FROM (0) TO (1), PARTITION tp_1_2 FOR VALUES FROM (1) TO (2)); RESET SESSION AUTHORIZATION; -ALTER TABLE t OWNER TO regress_partition_merge_bob; -SET SESSION AUTHORIZATION regress_partition_merge_bob; +ALTER TABLE t OWNER TO regress_partition_split_bob; +SET SESSION AUTHORIZATION regress_partition_split_bob; ALTER TABLE t SPLIT PARTITION tp_0_2 INTO (PARTITION tp_0_1 FOR VALUES FROM (0) TO (1), PARTITION tp_1_2 FOR VALUES FROM (1) TO (2)); RESET SESSION AUTHORIZATION; -ALTER TABLE tp_0_2 OWNER TO regress_partition_merge_bob; -SET SESSION AUTHORIZATION regress_partition_merge_bob; +ALTER TABLE tp_0_2 OWNER TO regress_partition_split_bob; +SET SESSION AUTHORIZATION regress_partition_split_bob; ALTER TABLE t SPLIT PARTITION tp_0_2 INTO (PARTITION tp_0_1 FOR VALUES FROM (0) TO (1), PARTITION tp_1_2
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Commit 3ca43dbbb67f which adds the permission checks seems to cause conflicts in the pg_upgrade tests: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=piculet=2024-05-13%2008%3A36%3A37 There is an issue with dropping and creating roles which seems to stem from this commit: CREATE ROLE regress_partition_merge_alice; +ERROR: role "regress_partition_merge_alice" already exists -- Daniel Gustafsson
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi! Attached draft version of fix for [1]. [1] https://www.postgresql.org/message-id/86b4f1e3-0b5d-315c-9225-19860d64d685%40gmail.com -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.comFrom ece01564aeb848bab2a61617412a1d175e45b934 Mon Sep 17 00:00:00 2001 From: Koval Dmitry Date: Sun, 12 May 2024 17:17:10 +0300 Subject: [PATCH v1 3/3] Fix for the search of temporary partition for the SPLIT SECTION operation --- src/backend/commands/tablecmds.c | 1 + src/test/regress/expected/partition_split.out | 8 src/test/regress/sql/partition_split.sql | 8 3 files changed, 17 insertions(+) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index fe66d9e58d..a5babcfbc6 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -21389,6 +21389,7 @@ ATExecSplitPartition(List **wqueue, AlteredTableInfo *tab, Relation rel, * against concurrent drop, and mark stmt->relation as * RELPERSISTENCE_TEMP if a temporary namespace is selected. */ + sps->name->relpersistence = rel->rd_rel->relpersistence; namespaceId = RangeVarGetAndCheckCreationNamespace(sps->name, NoLock, NULL); diff --git a/src/test/regress/expected/partition_split.out b/src/test/regress/expected/partition_split.out index 461318db86..b1108c92a2 100644 --- a/src/test/regress/expected/partition_split.out +++ b/src/test/regress/expected/partition_split.out @@ -1569,6 +1569,14 @@ Partition constraint: ((i IS NOT NULL) AND (i >= 0) AND (i < 1)) Partition of: t FOR VALUES FROM (1) TO (2) Partition constraint: ((i IS NOT NULL) AND (i >= 1) AND (i < 2)) +DROP TABLE t; +-- Check that the search for a temporary partition is correct during +-- the SPLIT PARTITION operation. +CREATE TEMP TABLE t (a int) PARTITION BY RANGE (a); +CREATE TEMP TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (2) ; +ALTER TABLE t SPLIT PARTITION tp_0 INTO + (PARTITION tp_0 FOR VALUES FROM (0) TO (1), + PARTITION tp_1 FOR VALUES FROM (1) TO (2)); DROP TABLE t; -- DROP SCHEMA partition_split_schema; diff --git a/src/test/regress/sql/partition_split.sql b/src/test/regress/sql/partition_split.sql index dc7424256e..7f231b0d39 100644 --- a/src/test/regress/sql/partition_split.sql +++ b/src/test/regress/sql/partition_split.sql @@ -939,6 +939,14 @@ ALTER TABLE t SPLIT PARTITION tp_0_2 INTO \d+ tp_1_2 DROP TABLE t; +-- Check that the search for a temporary partition is correct during +-- the SPLIT PARTITION operation. +CREATE TEMP TABLE t (a int) PARTITION BY RANGE (a); +CREATE TEMP TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (2) ; +ALTER TABLE t SPLIT PARTITION tp_0 INTO + (PARTITION tp_0 FOR VALUES FROM (0) TO (1), + PARTITION tp_1 FOR VALUES FROM (1) TO (2)); +DROP TABLE t; -- DROP SCHEMA partition_split_schema; -- 2.34.1
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi! 11.05.2024 12:00, Alexander Lakhin wrote: Please look at one more anomaly with temporary tables: Thank you, Alexander! The problem affects the SPLIT PARTITION command. CREATE TEMP TABLE t (a int) PARTITION BY RANGE (a); CREATE TEMP TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (2) ; -- ERROR: relation "tp_0" already exists ALTER TABLE t SPLIT PARTITION tp_0 INTO (PARTITION tp_0 FOR VALUES FROM (0) TO (1), PARTITION tp_1 FOR VALUES FROM (1) TO (2)); I'll try to fix it soon. -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hello Dmitry and Alexander, Please look at one more anomaly with temporary tables: CREATE TEMP TABLE t (a int) PARTITION BY RANGE (a); CREATE TEMP TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (1) ; CREATE TEMP TABLE tp_1 PARTITION OF t FOR VALUES FROM (1) TO (2); ALTER TABLE t MERGE PARTITIONS (tp_0, tp_1) INTO tp_0; -- succeeds, but: ALTER TABLE t SPLIT PARTITION tp_0 INTO (PARTITION tp_0 FOR VALUES FROM (0) TO (1), PARTITION tp_1 FOR VALUES FROM (1) TO (2)); -- fails with: ERROR: relation "tp_0" already exists Though the same SPLIT succeeds with non-temporary tables... Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Thu, May 9, 2024 at 12:37 AM Justin Pryzby wrote: > > On Wed, May 08, 2024 at 09:00:10PM +0300, Alexander Korotkov wrote: > > On Fri, May 3, 2024 at 4:32 PM Alexander Korotkov > > wrote: > > > On Fri, May 3, 2024 at 4:23 PM Justin Pryzby wrote: > > > > On Wed, May 01, 2024 at 10:51:24PM +0300, Dmitry Koval wrote: > > > > > 30.04.2024 23:15, Justin Pryzby пишет: > > > > > > Is this issue already fixed ? > > > > > > I wasn't able to reproduce it. Maybe it only happened with earlier > > > > > > patch versions applied ? > > > > > > > > > > I think this was fixed in commit [1]. > > > > > > > > > > [1] > > > > > https://github.com/postgres/postgres/commit/fcf80c5d5f0f3787e70fca8fd029d2e08a923f91 > > > > > > > > I tried to reproduce it at fcf80c5d5f~, but couldn't. > > > > I don't see how that patch would fix it anyway. > > > > I'm hoping Alexander can confirm what happened. > > > > > > This problem is only relevant for an old version of fix [1], which > > > overrides schemas for new partitions. That version was never > > > committed. > > > > Here are the patches. > > 0002 Skips copying extended statistics while creating new partitions in > > MERGE/SPLIT > > > > For 0002 I'd like to hear some feedback on wordings used in docs and > > comments. > > commit message: > > Currenlty => Currently > partiions => partitios > copying => by copying Thank you! > > > However, parent's table extended statistics already covers all its > > children. > > => That's the wrong explanation. It's not that "stats on the parent > table cover its children". It's that there are two types of stats: > stats for the "table hierarchy" and stats for the individual table. > That's true for single-column stats as well as for extended stats. > In both cases, that's indicated by the inh flag in the code and in the > catalog. > > The right explanation is that extended stats on partitioned tables are > not similar to indexes. Indexes on parent table are nothing other than > a mechanism to create indexes on the child tables. That's not true for > stats. > > See also my prior messages > ZiJW1g2nbQs9ekwK@pryzbyj2023 > Zi5Msg74C61DjJKW@pryzbyj2023 Yes, I understand that parents pg_statistic entry with stainherit == true includes statistics for the children. I tried to express this by word "covers". But you're right, this is the wrong explanation. Can I, please, ask you to revise the patch? > I think EXCLUDE IDENTITY can/should now also be removed - see 509199587. > I'm not able to reproduce that problem anyway, even before that... I will check this. -- Regards, Alexander Korotkov Supabase
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Wed, May 08, 2024 at 09:00:10PM +0300, Alexander Korotkov wrote: > On Fri, May 3, 2024 at 4:32 PM Alexander Korotkov > wrote: > > On Fri, May 3, 2024 at 4:23 PM Justin Pryzby wrote: > > > On Wed, May 01, 2024 at 10:51:24PM +0300, Dmitry Koval wrote: > > > > 30.04.2024 23:15, Justin Pryzby пишет: > > > > > Is this issue already fixed ? > > > > > I wasn't able to reproduce it. Maybe it only happened with earlier > > > > > patch versions applied ? > > > > > > > > I think this was fixed in commit [1]. > > > > > > > > [1] > > > > https://github.com/postgres/postgres/commit/fcf80c5d5f0f3787e70fca8fd029d2e08a923f91 > > > > > > I tried to reproduce it at fcf80c5d5f~, but couldn't. > > > I don't see how that patch would fix it anyway. > > > I'm hoping Alexander can confirm what happened. > > > > This problem is only relevant for an old version of fix [1], which > > overrides schemas for new partitions. That version was never > > committed. > > Here are the patches. > 0002 Skips copying extended statistics while creating new partitions in > MERGE/SPLIT > > For 0002 I'd like to hear some feedback on wordings used in docs and comments. commit message: Currenlty => Currently partiions => partitios copying => by copying > However, parent's table extended statistics already covers all its > children. => That's the wrong explanation. It's not that "stats on the parent table cover its children". It's that there are two types of stats: stats for the "table hierarchy" and stats for the individual table. That's true for single-column stats as well as for extended stats. In both cases, that's indicated by the inh flag in the code and in the catalog. The right explanation is that extended stats on partitioned tables are not similar to indexes. Indexes on parent table are nothing other than a mechanism to create indexes on the child tables. That's not true for stats. See also my prior messages ZiJW1g2nbQs9ekwK@pryzbyj2023 Zi5Msg74C61DjJKW@pryzbyj2023 I think EXCLUDE IDENTITY can/should now also be removed - see 509199587. I'm not able to reproduce that problem anyway, even before that... -- Justin
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Wed, May 1, 2024 at 12:14 AM Dmitry Koval wrote: > 30.04.2024 6:00, Alexander Lakhin пишет: > > Maybe I'm doing something wrong, but the following script: > > CREATE TABLE t (i int, PRIMARY KEY(i)) PARTITION BY RANGE (i); > > CREATE TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (1); > > CREATE TABLE tp_1 PARTITION OF t FOR VALUES FROM (1) TO (2); > > > > CREATE TABLE t2 (LIKE t INCLUDING ALL); > > CREATE TABLE tp2 (LIKE tp_0 INCLUDING ALL); > > creates tables t2, tp2 without not-null constraints. > > To create partitions is used the "CREATE TABLE ... LIKE ..." command > with the "EXCLUDING INDEXES" modifier (to speed up the insertion of values). > > CREATE TABLE t (i int, PRIMARY KEY(i)) PARTITION BY RANGE(i); > CREATE TABLE t2 (LIKE t INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY); > \d+ t2; > ... > Not-null constraints: > "t2_i_not_null" NOT NULL "i" > Access method: heap I've explored this a little bit more. If the parent table has explicit not null constraint than results of MERGE/SPLIT look the same as result of CREATE TABLE ... PARTITION OF. In every case there is explicit not null constraint in all the cases. # CREATE TABLE t (i int not null, PRIMARY KEY(i)) PARTITION BY RANGE(i); # \d+ t Partitioned table "public.t" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description +-+---+--+-+-+-+--+- i | integer | | not null | | plain | | | Partition key: RANGE (i) Indexes: "t_pkey" PRIMARY KEY, btree (i) Not-null constraints: "t_i_not_null" NOT NULL "i" Number of partitions: 0 # CREATE TABLE tp_0_2 PARTITION OF t FOR VALUES FROM (0) TO (2); # \d+ tp_0_2 Table "public.tp_0_2" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description +-+---+--+-+-+-+--+- i | integer | | not null | | plain | | | Partition of: t FOR VALUES FROM (0) TO (2) Partition constraint: ((i IS NOT NULL) AND (i >= 0) AND (i < 2)) Indexes: "tp_0_2_pkey" PRIMARY KEY, btree (i) Not-null constraints: "t_i_not_null" NOT NULL "i" (inherited) Access method: heap # ALTER TABLE t SPLIT PARTITION tp_0_2 INTO #(PARTITION tp_0_1 FOR VALUES FROM (0) TO (1), # PARTITION tp_1_2 FOR VALUES FROM (1) TO (2)) # \d+ tp_0_1 Table "public.tp_0_1" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description +-+---+--+-+-+-+--+- i | integer | | not null | | plain | | | Partition of: t FOR VALUES FROM (0) TO (1) Partition constraint: ((i IS NOT NULL) AND (i >= 0) AND (i < 1)) Indexes: "tp_0_1_pkey" PRIMARY KEY, btree (i) Not-null constraints: "t_i_not_null" NOT NULL "i" (inherited) Access method: heap However, if not null constraint is implicit and derived from primary key, the situation is different. The partition created by CREATE TABLE ... PARTITION OF doesn't have explicit not null constraint just like the parent. But the partition created by MERGE/SPLIT has explicit not null contraint. # CREATE TABLE t (i int not null, PRIMARY KEY(i)) PARTITION BY RANGE(i); # \d+ t Partitioned table "public.t" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description +-+---+--+-+-+-+--+- i | integer | | not null | | plain | | | Partition key: RANGE (i) Indexes: "t_pkey" PRIMARY KEY, btree (i) Number of partitions: 0 # CREATE TABLE tp_0_2 PARTITION OF t FOR VALUES FROM (0) TO (2); # \d+ tp_0_2 Table "public.tp_0_2" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description +-+---+--+-+-+-+--+- i | integer | | not null | | plain | | | Partition of: t FOR VALUES FROM (0) TO (2) Partition constraint: ((i IS NOT NULL) AND (i >= 0) AND (i < 2)) Indexes: "tp_0_2_pkey" PRIMARY KEY, btree (i) Access method: heap # ALTER TABLE t SPLIT PARTITION tp_0_2 INTO #(PARTITION tp_0_1 FOR VALUES FROM (0) TO (1), # PARTITION tp_1_2 FOR VALUES FROM (1) TO (2)) # \d+ tp_0_1 Table "public.tp_0_1" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Fri, May 3, 2024 at 4:32 PM Alexander Korotkov wrote: > On Fri, May 3, 2024 at 4:23 PM Justin Pryzby wrote: > > On Wed, May 01, 2024 at 10:51:24PM +0300, Dmitry Koval wrote: > > > 30.04.2024 23:15, Justin Pryzby пишет: > > > > Is this issue already fixed ? > > > > I wasn't able to reproduce it. Maybe it only happened with earlier > > > > patch versions applied ? > > > > > > I think this was fixed in commit [1]. > > > > > > [1] > > > https://github.com/postgres/postgres/commit/fcf80c5d5f0f3787e70fca8fd029d2e08a923f91 > > > > I tried to reproduce it at fcf80c5d5f~, but couldn't. > > I don't see how that patch would fix it anyway. > > I'm hoping Alexander can confirm what happened. > > This problem is only relevant for an old version of fix [1], which > overrides schemas for new partitions. That version was never > committed. Here are the patches. 0001 Adds permission checks on the partitions before doing MERGE/SPLIT 0002 Skips copying extended statistics while creating new partitions in MERGE/SPLIT 0001 looks quite simple and trivial for me. I'm going to push it if no objections. For 0002 I'd like to hear some feedback on wordings used in docs and comments. -- Regards, Alexander Korotkov Supabase v1-0002-Don-t-copy-extended-statistics-during-MERGE-SPLIT.patch Description: Binary data v1-0001-Add-permission-check-for-MERGE-SPLIT-partition-op.patch Description: Binary data
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Fri, May 3, 2024 at 4:23 PM Justin Pryzby wrote: > On Wed, May 01, 2024 at 10:51:24PM +0300, Dmitry Koval wrote: > > 30.04.2024 23:15, Justin Pryzby пишет: > > > Is this issue already fixed ? > > > I wasn't able to reproduce it. Maybe it only happened with earlier > > > patch versions applied ? > > > > I think this was fixed in commit [1]. > > > > [1] > > https://github.com/postgres/postgres/commit/fcf80c5d5f0f3787e70fca8fd029d2e08a923f91 > > I tried to reproduce it at fcf80c5d5f~, but couldn't. > I don't see how that patch would fix it anyway. > I'm hoping Alexander can confirm what happened. This problem is only relevant for an old version of fix [1], which overrides schemas for new partitions. That version was never committed. > The other remaining issues I'm aware of are for EXCLUDING STATISTICS and > refusing to ALTER if the owners don't match. These two are in my list. I'm planning to work on them in the next few days. > Note that the error that led to "EXCLUDING IDENTITY" is being discused > over here: > https://www.postgresql.org/message-id/3b8a9dc1-bbc7-0ef5-6863-c432afac7...@gmail.com > > It's possible that once that's addressed, the exclusion should be > removed here, too. +1 Links. 1. https://www.postgresql.org/message-id/edfbd846-dcc1-42d1-ac26-715691b687d3%40postgrespro.ru -- Regards, Alexander Korotkov Supabase
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Wed, May 01, 2024 at 10:51:24PM +0300, Dmitry Koval wrote: > Hi! > > 30.04.2024 23:15, Justin Pryzby пишет: > > Is this issue already fixed ? > > I wasn't able to reproduce it. Maybe it only happened with earlier > > patch versions applied ? > > I think this was fixed in commit [1]. > > [1] > https://github.com/postgres/postgres/commit/fcf80c5d5f0f3787e70fca8fd029d2e08a923f91 I tried to reproduce it at fcf80c5d5f~, but couldn't. I don't see how that patch would fix it anyway. I'm hoping Alexander can confirm what happened. The other remaining issues I'm aware of are for EXCLUDING STATISTICS and refusing to ALTER if the owners don't match. Note that the error that led to "EXCLUDING IDENTITY" is being discused over here: https://www.postgresql.org/message-id/3b8a9dc1-bbc7-0ef5-6863-c432afac7...@gmail.com It's possible that once that's addressed, the exclusion should be removed here, too. -- Justin
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi! 30.04.2024 23:15, Justin Pryzby пишет: Is this issue already fixed ? I wasn't able to reproduce it. Maybe it only happened with earlier patch versions applied ? I think this was fixed in commit [1]. [1] https://github.com/postgres/postgres/commit/fcf80c5d5f0f3787e70fca8fd029d2e08a923f91 -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi! 30.04.2024 6:00, Alexander Lakhin пишет: Maybe I'm doing something wrong, but the following script: CREATE TABLE t (i int, PRIMARY KEY(i)) PARTITION BY RANGE (i); CREATE TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (1); CREATE TABLE tp_1 PARTITION OF t FOR VALUES FROM (1) TO (2); CREATE TABLE t2 (LIKE t INCLUDING ALL); CREATE TABLE tp2 (LIKE tp_0 INCLUDING ALL); creates tables t2, tp2 without not-null constraints. To create partitions is used the "CREATE TABLE ... LIKE ..." command with the "EXCLUDING INDEXES" modifier (to speed up the insertion of values). CREATE TABLE t (i int, PRIMARY KEY(i)) PARTITION BY RANGE(i); CREATE TABLE t2 (LIKE t INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY); \d+ t2; ... Not-null constraints: "t2_i_not_null" NOT NULL "i" Access method: heap [1] https://github.com/postgres/postgres/blob/d12b4ba1bd3eedd862064cf1dad5ff107c5cba90/src/backend/commands/tablecmds.c#L21215 -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Thu, Apr 11, 2024 at 08:00:00PM +0300, Alexander Lakhin wrote: > 11.04.2024 16:27, Dmitry Koval wrote: > > > > Added correction (and test), see > > v3-0001-Fix-for-SPLIT-MERGE-partitions-of-temporary-table.patch. > > Thank you for the correction, but may be an attempt to merge into implicit > pg_temp should fail just like CREATE TABLE ... PARTITION OF ... does? > > Please look also at another anomaly with schemas: > CREATE SCHEMA s1; > CREATE TABLE t (i int) PARTITION BY RANGE (i); > CREATE TABLE tp_0_2 PARTITION OF t > FOR VALUES FROM (0) TO (2); > ALTER TABLE t SPLIT PARTITION tp_0_2 INTO > (PARTITION s1.tp0 FOR VALUES FROM (0) TO (1), PARTITION s1.tp1 FOR VALUES > FROM (1) TO (2)); > results in: > \d+ s1.* > Did not find any relation named "s1.*" > \d+ tp* > Table "public.tp0" Hi, Is this issue already fixed ? I wasn't able to reproduce it. Maybe it only happened with earlier patch versions applied ? Thanks, -- Justin
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
30.04.2024 03:10, Dmitry Koval wrote: Hi! 1. 29.04.2024 21:00, Alexander Lakhin wrote: I still wonder, why that constraint (now with a less questionable name) is created during MERGE? The SPLIT/MERGE PARTITION(S) commands for creating partitions reuse the existing code of CREATE TABLE .. LIKE ... command. A new partition was created with the name "merge-16385-26BCB0-tmp" (since there was an old partition with the same name). The constraint "merge-16385-26BCB0-tmp_i_not_null" was created too together with the partition. Subsequently, the table was renamed, but the constraint was not. Now a new partition is immediately created with the correct name (the old partition is renamed). Maybe I'm doing something wrong, but the following script: CREATE TABLE t (i int, PRIMARY KEY(i)) PARTITION BY RANGE (i); CREATE TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (1); CREATE TABLE tp_1 PARTITION OF t FOR VALUES FROM (1) TO (2); CREATE TABLE t2 (LIKE t INCLUDING ALL); CREATE TABLE tp2 (LIKE tp_0 INCLUDING ALL); creates tables t2, tp2 without not-null constraints. But after ALTER TABLE t MERGE PARTITIONS (tp_0, tp_1) INTO tp_0; I see: \d+ tp_0 ... Indexes: "tp_0_pkey" PRIMARY KEY, btree (i) Not-null constraints: "tp_0_i_not_null" NOT NULL "i" Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi! 1. 29.04.2024 21:00, Alexander Lakhin wrote: I still wonder, why that constraint (now with a less questionable name) is created during MERGE? The SPLIT/MERGE PARTITION(S) commands for creating partitions reuse the existing code of CREATE TABLE .. LIKE ... command. A new partition was created with the name "merge-16385-26BCB0-tmp" (since there was an old partition with the same name). The constraint "merge-16385-26BCB0-tmp_i_not_null" was created too together with the partition. Subsequently, the table was renamed, but the constraint was not. Now a new partition is immediately created with the correct name (the old partition is renamed). 2. Just in case, I am attaching a small fix v9_fix.diff for situation [1]. [1] https://www.postgresql.org/message-id/0520c72e-8d97-245e-53f9-173beca2ab2e%40gmail.com -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.comdiff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index fef084f5d5..e918a623c5 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -3446,6 +3446,11 @@ checkPartition(Relation rel, Oid partRelOid) RelationGetRelationName(partRel), RelationGetRelationName(rel; + /* Permissions checks */ + if (!object_ownercheck(RelationRelationId, RelationGetRelid(partRel), GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(partRel->rd_rel->relkind), + RelationGetRelationName(partRel)); + relation_close(partRel, AccessShareLock); }
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi Dmitry, 19.04.2024 02:26, Dmitry Koval wrote: 18.04.2024 19:00, Alexander Lakhin wrote: leaves a strange constraint: \d+ t* Table "public.tp_0" ... Not-null constraints: "merge-16385-26BCB0-tmp_i_not_null" NOT NULL "i" Thanks! Attached fix (with test) for this case. The patch should be applied after patches v6-0001- ... .patch ... v6-0004- ... .patch I still wonder, why that constraint (now with a less questionable name) is created during MERGE? That is, before MERGE, two partitions have only PRIMARY KEY indexes, with no not-null constraint, and you can manually remove the constraint after MERGE, so maybe it's not necessary... Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sun, Apr 28, 2024 at 08:18:42AM -0500, Justin Pryzby wrote: > > I will explore this. Do we copy extended stats when we do CREATE > > TABLE ... PARTITION OF? I think we need to do the same here. > > Right, they're not copied because an extended stats objs on the parent > does something different than putting stats objects on each child. > I've convinced myself that it's wrong to copy the parent's stats obj. > If someone wants stats objects on each child, they'll have to handle > them specially after MERGE/SPLIT, just as they would for per-child > defaults/constraints/etc. I dug up this thread, in which the idea of copying extended stats from parent to child was considered some 6 years ago, but never implemented; for consistency, MERGE/SPLIT shouldn't copy extended stats, either. https://www.postgresql.org/message-id/20180305195750.aecbpihhcvuskzba%40alvherre.pgsql -- Justin
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sunday, April 28, 2024, Alexander Lakhin wrote: > > When we deal with mixed ownership, say, bob is an owner of a > partitioned table, but not an owner of a partition, should we > allow him to perform merge with that partition? > > Attaching via alter table requires the user to own both the partitioned table and the table being acted upon. Merge needs to behave similarly. The fact that we let the superuser break the requirement of common ownership is unfortunate but I guess understandable. But given the existing behavior of attach merge should likewise fail if it find the user doesn’t own the partitions being merged. The fact that the user can select from those tables can be acted upon manually if desired; these administrative commands should all ensure common ownership and fail if that precondition is not met. David J.
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sunday, April 28, 2024, Alexander Lakhin wrote: > > When we deal with mixed ownership, say, bob is an owner of a > partitioned table, but not an owner of a partition, should we > allow him to perform merge with that partition? > > IIUC Merge causes the source tables to be dropped, their data having been effectively moved into the new partition. bob must not be allowed to drop Alice’s tables. Only an owner may do that. So if we do allow bob to build a new partition using his select access, the tables he selected from would have to remain behind if he is not an owner of them. David J.
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sun, Apr 28, 2024 at 04:04:54AM +0300, Alexander Korotkov wrote: > Hi Justin, > > Thank you for your review. Please check v9 of the patchset [1]. > > On Wed, Apr 24, 2024 at 11:26 PM Justin Pryzby wrote: > > This patch also/already fixes the schema issue I reported. Thanks. > > > > If you wanted to include a test case for that: > > > > begin; > > CREATE SCHEMA s; > > CREATE SCHEMA t; > > CREATE TABLE p(i int) PARTITION BY RANGE(i); > > CREATE TABLE s.c1 PARTITION OF p FOR VALUES FROM (1)TO(2); > > CREATE TABLE s.c2 PARTITION OF p FOR VALUES FROM (2)TO(3); > > ALTER TABLE p MERGE PARTITIONS (s.c1, s.c2) INTO s.c1; -- misbehaves if > > merging into the same name as an existing partition > > \d+ p > > ... > > Partitions: c1 FOR VALUES FROM (1) TO (3) > > There is already a test which checks merging into the same name as an > existing partition. And there are tests with schema-qualified names. > I'm not yet convinced we need a test with both these properties > together. I mentioned that the combination of schemas and merge-into-same-name is what currently doesn't work right. > > Also, extended stats objects are currently cloned to new child tables. > > But I suggested in [0] that they probably shouldn't be. > > I will explore this. Do we copy extended stats when we do CREATE > TABLE ... PARTITION OF? I think we need to do the same here. Right, they're not copied because an extended stats objs on the parent does something different than putting stats objects on each child. I've convinced myself that it's wrong to copy the parent's stats obj. If someone wants stats objects on each child, they'll have to handle them specially after MERGE/SPLIT, just as they would for per-child defaults/constraints/etc. On Sun, Apr 28, 2024 at 04:04:54AM +0300, Alexander Korotkov wrote: > On Wed, Apr 24, 2024 at 11:26 PM Justin Pryzby wrote: > > This patch adds documentation saying: > > + Any indexes, constraints and user-defined row-level triggers that > > exist > > + in the parent table are cloned on new partitions [...] > > > > Which is good to say, and addresses part of my message [0] > > [0] ZiJW1g2nbQs9ekwK@pryzbyj2023 > > Makes sense. Extracted this into a separate patch in v10. I adjusted the language some and fixed a typo in the commit message. s/parition/partition/ -- Justin >From e00033fc4b8254c70bf8a3d41d513edd9540e2d7 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Sun, 28 Apr 2024 03:39:30 +0300 Subject: [PATCH] Document the way partition MERGE/SPLIT operations create new partitions Reported-by: Justin Pryzby Discussion: https://postgr.es/m/ZilrByTp-pbz6Mvf%40pryzbyj2023 --- doc/src/sgml/ref/alter_table.sgml | 12 1 file changed, 12 insertions(+) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index fe36ff82e52..fc2dfffe49f 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -1153,6 +1153,12 @@ WITH ( MODULUS numeric_literal, REM splitting we have a partition with the same name). Only simple, non-partitioned partition can be split. + + The new partitions will be created the same as tables created with the + SQL command CREATE TABLE partition_nameN (LIKE name INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY). + The indexes and identity are created later, after moving the data + into the new partitions. + This command acquires an ACCESS EXCLUSIVE lock. @@ -1213,6 +1219,12 @@ WITH ( MODULUS numeric_literal, REM can have the same name as one of the merged partitions. Only simple, non-partitioned partitions can be merged. + + The new partition will be created the same as a table created with the + SQL command CREATE TABLE partition_name (LIKE name INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY). + The indexes and identity are created later, after moving the data + into the new partition. + This command acquires an ACCESS EXCLUSIVE lock. -- 2.42.0
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sun, Apr 28, 2024 at 2:00 PM Alexander Lakhin wrote: > 28.04.2024 03:59, Alexander Korotkov wrote: > > The revised patchset is attached. I'm going to push it if there are > > no objections. > > I have one additional question regarding security, if you don't mind: > What permissions should a user have to perform split/merge? > > When we deal with mixed ownership, say, bob is an owner of a > partitioned table, but not an owner of a partition, should we > allow him to perform merge with that partition? > Consider the following script: > CREATE ROLE alice; > GRANT CREATE ON SCHEMA public TO alice; > > SET SESSION AUTHORIZATION alice; > CREATE TABLE t (i int PRIMARY KEY, t text, u text) PARTITION BY RANGE (i); > CREATE TABLE tp_00 PARTITION OF t FOR VALUES FROM (0) TO (10); > CREATE TABLE tp_10 PARTITION OF t FOR VALUES FROM (10) TO (20); > > CREATE POLICY p1 ON tp_00 USING (u = current_user); > ALTER TABLE tp_00 ENABLE ROW LEVEL SECURITY; > > INSERT INTO t(i, t, u) VALUES (0, 'info for bob', 'bob'); > INSERT INTO t(i, t, u) VALUES (1, 'info for alice', 'alice'); > RESET SESSION AUTHORIZATION; > > CREATE ROLE bob; > GRANT CREATE ON SCHEMA public TO bob; > ALTER TABLE t OWNER TO bob; > GRANT SELECT ON TABLE tp_00 TO bob; > > SET SESSION AUTHORIZATION bob; > SELECT * FROM tp_00; > --- here bob can see his info only > \d > Schema | Name | Type| Owner > +---+---+--- > public | t | partitioned table | bob > public | tp_00 | table | alice > public | tp_10 | table | alice > > -- but then bob can do: > ALTER TABLE t MERGE PARTITIONS (tp_00, tp_10) INTO tp_00; > -- (yes, he also can detach the partition tp_00, but then he couldn't > -- re-attach nor read it) > > \d > Schema | Name | Type| Owner > +---+---+--- > public | t | partitioned table | bob > public | tp_00 | table | bob > > Thus bob effectively have captured the partition with the data. > > What do you think, does this create a new security risk? Alexander, thank you for discovering this. I believe that the one who merges partitions should have permissions for all the partitions merged. I'll recheck this and provide the patch. -- Regards, Alexander Korotkov
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hello, 28.04.2024 03:59, Alexander Korotkov wrote: The revised patchset is attached. I'm going to push it if there are no objections. I have one additional question regarding security, if you don't mind: What permissions should a user have to perform split/merge? When we deal with mixed ownership, say, bob is an owner of a partitioned table, but not an owner of a partition, should we allow him to perform merge with that partition? Consider the following script: CREATE ROLE alice; GRANT CREATE ON SCHEMA public TO alice; SET SESSION AUTHORIZATION alice; CREATE TABLE t (i int PRIMARY KEY, t text, u text) PARTITION BY RANGE (i); CREATE TABLE tp_00 PARTITION OF t FOR VALUES FROM (0) TO (10); CREATE TABLE tp_10 PARTITION OF t FOR VALUES FROM (10) TO (20); CREATE POLICY p1 ON tp_00 USING (u = current_user); ALTER TABLE tp_00 ENABLE ROW LEVEL SECURITY; INSERT INTO t(i, t, u) VALUES (0, 'info for bob', 'bob'); INSERT INTO t(i, t, u) VALUES (1, 'info for alice', 'alice'); RESET SESSION AUTHORIZATION; CREATE ROLE bob; GRANT CREATE ON SCHEMA public TO bob; ALTER TABLE t OWNER TO bob; GRANT SELECT ON TABLE tp_00 TO bob; SET SESSION AUTHORIZATION bob; SELECT * FROM tp_00; --- here bob can see his info only \d Schema | Name | Type | Owner +---+---+--- public | t | partitioned table | bob public | tp_00 | table | alice public | tp_10 | table | alice -- but then bob can do: ALTER TABLE t MERGE PARTITIONS (tp_00, tp_10) INTO tp_00; -- (yes, he also can detach the partition tp_00, but then he couldn't -- re-attach nor read it) \d Schema | Name | Type | Owner +---+---+--- public | t | partitioned table | bob public | tp_00 | table | bob Thus bob effectively have captured the partition with the data. What do you think, does this create a new security risk? Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi Justin, Thank you for your review. Please check v9 of the patchset [1]. On Wed, Apr 24, 2024 at 11:26 PM Justin Pryzby wrote: > This patch also/already fixes the schema issue I reported. Thanks. > > If you wanted to include a test case for that: > > begin; > CREATE SCHEMA s; > CREATE SCHEMA t; > CREATE TABLE p(i int) PARTITION BY RANGE(i); > CREATE TABLE s.c1 PARTITION OF p FOR VALUES FROM (1)TO(2); > CREATE TABLE s.c2 PARTITION OF p FOR VALUES FROM (2)TO(3); > ALTER TABLE p MERGE PARTITIONS (s.c1, s.c2) INTO s.c1; -- misbehaves if > merging into the same name as an existing partition > \d+ p > ... > Partitions: c1 FOR VALUES FROM (1) TO (3) There is already a test which checks merging into the same name as an existing partition. And there are tests with schema-qualified names. I'm not yet convinced we need a test with both these properties together. > > 0002 > > The persistence of the new partition is copied as suggested in [1]. > > But the checks are in-place, because search_path could influence new > > table persistence. Per review [2], commit message typos are fixed, > > documentation is revised, revised tests to cover schema-qualification, > > usage of search_path. > > Subject: [PATCH v8 2/7] Make new partitions with parent's persistence during > MERGE/SPLIT operations > > This patch adds documentation saying: > + Any indexes, constraints and user-defined row-level triggers that exist > + in the parent table are cloned on new partitions [...] > > Which is good to say, and addresses part of my message [0] > [0] ZiJW1g2nbQs9ekwK@pryzbyj2023 > > But it doesn't have anything to do with "creating new partitions with > parent's persistence". Maybe there was a merge conflict and the docs > ended up in the wrong patch ? Makes sense. Extracted this into a separate patch in v10. > Also, defaults, storage options, compression are also copied. As will > be anything else from LIKE. And since anything added in the future will > also be copied, maybe it's better to just say that the tables will be > created the same way as "LIKE .. INCLUDING ALL EXCLUDING ..", or > similar. Otherwise, the next person who adds a new option for LIKE > would have to remember to update this paragraph... Reworded that way. Thank you. > Also, extended stats objects are currently cloned to new child tables. > But I suggested in [0] that they probably shouldn't be. I will explore this. Do we copy extended stats when we do CREATE TABLE ... PARTITION OF? I think we need to do the same here. > > 007 – doc review by Justin [3] > > I suggest to drop this patch for now. I'll send some more minor fixes to > docs and code comments once the other patches are settled. Your edits are welcome. Dropped this for now. And waiting for the next revision from you. Links. 1. https://www.postgresql.org/message-id/CAPpHfduYuYECrqpHMgcOsNr%2B4j3uJK%2BJPUJ_zDBn-tqjjh3p1Q%40mail.gmail.com -- Regards, Alexander Korotkov Supabase
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi, Hackers! On Thu, 25 Apr 2024 at 00:26, Justin Pryzby wrote: > On Mon, Apr 22, 2024 at 01:31:48PM +0300, Alexander Korotkov wrote: > > Hi! > > > > On Fri, Apr 19, 2024 at 4:29 PM Alexander Korotkov > wrote: > > > On Fri, Apr 19, 2024 at 2:26 AM Dmitry Koval > wrote: > > > > 18.04.2024 19:00, Alexander Lakhin wrote: > > > > > leaves a strange constraint: > > > > > \d+ t* > > > > >Table "public.tp_0" > > > > > ... > > > > > Not-null constraints: > > > > > "merge-16385-26BCB0-tmp_i_not_null" NOT NULL "i" > > > > > > > > Thanks! > > > > Attached fix (with test) for this case. > > > > The patch should be applied after patches > > > > v6-0001- ... .patch ... v6-0004- ... .patch > > > > > > I've incorporated this fix with 0001 patch. > > > > > > Also added to the patchset > > > 005 – tab completion by Dagfinn [1] > > > 006 – draft fix for table AM issue spotted by Alexander Lakhin [2] > > > 007 – doc review by Justin [3] > > > > > > I'm continuing work on this. > > > > > > Links > > > 1. > https://www.postgresql.org/message-id/87plumiox2.fsf%40wibble.ilmari.org > > > 2. > https://www.postgresql.org/message-id/84ada05b-be5c-473e-6d1c-ebe5dd21b190%40gmail.com > > > 3. > https://www.postgresql.org/message-id/ZiGH0xc1lxJ71ZfB%40pryzbyj2023 > > > > 0001 > > The way we handle name collisions during MERGE PARTITIONS operation is > > reworked by integration of patch [3]. This makes note about commit in > > [2] not relevant. > > This patch also/already fixes the schema issue I reported. Thanks. > > If you wanted to include a test case for that: > > begin; > CREATE SCHEMA s; > CREATE SCHEMA t; > CREATE TABLE p(i int) PARTITION BY RANGE(i); > CREATE TABLE s.c1 PARTITION OF p FOR VALUES FROM (1)TO(2); > CREATE TABLE s.c2 PARTITION OF p FOR VALUES FROM (2)TO(3); > ALTER TABLE p MERGE PARTITIONS (s.c1, s.c2) INTO s.c1; -- misbehaves if > merging into the same name as an existing partition > \d+ p > ... > Partitions: c1 FOR VALUES FROM (1) TO (3) > > > 0002 > > The persistence of the new partition is copied as suggested in [1]. > > But the checks are in-place, because search_path could influence new > > table persistence. Per review [2], commit message typos are fixed, > > documentation is revised, revised tests to cover schema-qualification, > > usage of search_path. > > Subject: [PATCH v8 2/7] Make new partitions with parent's persistence > during MERGE/SPLIT operations > > This patch adds documentation saying: > + Any indexes, constraints and user-defined row-level triggers that > exist > + in the parent table are cloned on new partitions [...] > > Which is good to say, and addresses part of my message [0] > [0] ZiJW1g2nbQs9ekwK@pryzbyj2023 > > But it doesn't have anything to do with "creating new partitions with > parent's persistence". Maybe there was a merge conflict and the docs > ended up in the wrong patch ? > > Also, defaults, storage options, compression are also copied. As will > be anything else from LIKE. And since anything added in the future will > also be copied, maybe it's better to just say that the tables will be > created the same way as "LIKE .. INCLUDING ALL EXCLUDING ..", or > similar. Otherwise, the next person who adds a new option for LIKE > would have to remember to update this paragraph... > > Also, extended stats objects are currently cloned to new child tables. > But I suggested in [0] that they probably shouldn't be. > > > 007 – doc review by Justin [3] > > I suggest to drop this patch for now. I'll send some more minor fixes to > docs and code comments once the other patches are settled. > I've looked at the patchset: 0001 Look good. 0002 Also right with docs modification proposed by Justin. 0003: Looks like unused code 5268 datum = cmpval ? list_nth(spec->lowerdatums, abs(cmpval) - 1) : NULL; overridden by 5278 datum = list_nth(spec->upperdatums, abs(cmpval) - 1); and 5290 datum = list_nth(spec->upperdatums, abs(cmpval) - 1); Otherwise - good. 0004: I suggest also getting rid of thee-noun compound words like: salesperson_name. Maybe salesperson -> clerk? Or maybe use the same terms like in pgbench: branches, tellers, accounts, balance. 0005: Good 0006: Patch is right In comments: + New partitions will have the same table access method, + same column names and types as the partitioned table to which they belong. (I'd suggest to remove second "same") Tests are passed. I suppose that it's better to add similar tests for SPLIT/MERGE PARTITION(S) to those covering ATTACH/DETACH PARTITION (e.g.: subscription/t/013_partition.pl and regression tests) Overall, great work! Thanks! Regards, Pavel Borisov, Supabase.
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Mon, Apr 22, 2024 at 01:31:48PM +0300, Alexander Korotkov wrote: > Hi! > > On Fri, Apr 19, 2024 at 4:29 PM Alexander Korotkov > wrote: > > On Fri, Apr 19, 2024 at 2:26 AM Dmitry Koval wrote: > > > 18.04.2024 19:00, Alexander Lakhin wrote: > > > > leaves a strange constraint: > > > > \d+ t* > > > >Table "public.tp_0" > > > > ... > > > > Not-null constraints: > > > > "merge-16385-26BCB0-tmp_i_not_null" NOT NULL "i" > > > > > > Thanks! > > > Attached fix (with test) for this case. > > > The patch should be applied after patches > > > v6-0001- ... .patch ... v6-0004- ... .patch > > > > I've incorporated this fix with 0001 patch. > > > > Also added to the patchset > > 005 – tab completion by Dagfinn [1] > > 006 – draft fix for table AM issue spotted by Alexander Lakhin [2] > > 007 – doc review by Justin [3] > > > > I'm continuing work on this. > > > > Links > > 1. https://www.postgresql.org/message-id/87plumiox2.fsf%40wibble.ilmari.org > > 2. > > https://www.postgresql.org/message-id/84ada05b-be5c-473e-6d1c-ebe5dd21b190%40gmail.com > > 3. https://www.postgresql.org/message-id/ZiGH0xc1lxJ71ZfB%40pryzbyj2023 > > 0001 > The way we handle name collisions during MERGE PARTITIONS operation is > reworked by integration of patch [3]. This makes note about commit in > [2] not relevant. This patch also/already fixes the schema issue I reported. Thanks. If you wanted to include a test case for that: begin; CREATE SCHEMA s; CREATE SCHEMA t; CREATE TABLE p(i int) PARTITION BY RANGE(i); CREATE TABLE s.c1 PARTITION OF p FOR VALUES FROM (1)TO(2); CREATE TABLE s.c2 PARTITION OF p FOR VALUES FROM (2)TO(3); ALTER TABLE p MERGE PARTITIONS (s.c1, s.c2) INTO s.c1; -- misbehaves if merging into the same name as an existing partition \d+ p ... Partitions: c1 FOR VALUES FROM (1) TO (3) > 0002 > The persistence of the new partition is copied as suggested in [1]. > But the checks are in-place, because search_path could influence new > table persistence. Per review [2], commit message typos are fixed, > documentation is revised, revised tests to cover schema-qualification, > usage of search_path. Subject: [PATCH v8 2/7] Make new partitions with parent's persistence during MERGE/SPLIT operations This patch adds documentation saying: + Any indexes, constraints and user-defined row-level triggers that exist + in the parent table are cloned on new partitions [...] Which is good to say, and addresses part of my message [0] [0] ZiJW1g2nbQs9ekwK@pryzbyj2023 But it doesn't have anything to do with "creating new partitions with parent's persistence". Maybe there was a merge conflict and the docs ended up in the wrong patch ? Also, defaults, storage options, compression are also copied. As will be anything else from LIKE. And since anything added in the future will also be copied, maybe it's better to just say that the tables will be created the same way as "LIKE .. INCLUDING ALL EXCLUDING ..", or similar. Otherwise, the next person who adds a new option for LIKE would have to remember to update this paragraph... Also, extended stats objects are currently cloned to new child tables. But I suggested in [0] that they probably shouldn't be. > 007 – doc review by Justin [3] I suggest to drop this patch for now. I'll send some more minor fixes to docs and code comments once the other patches are settled. -- Justin
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Thu, Apr 11, 2024 at 10:20:53PM -0400, Robert Haas wrote:
> On Thu, Apr 11, 2024 at 9:54 PM Alexander Korotkov
> wrote:
> > I think we shouldn't unconditionally copy schema name and
> > relpersistence from the parent table. Instead we should throw the
> > error on a mismatch like CREATE TABLE ... PARTITION OF ... does. I'm
> > working on revising this fix.
>
> We definitely shouldn't copy the schema name from the parent table. It
> should be possible to schema-qualify the new partition names, and if
> you don't, then the search_path should determine where they get
> placed.
+1. Alexander Lakhin reported an issue with schemas and SPLIT, and I
noticed an issue with schemas with MERGE. The issue I hit is occurs
when MERGE'ing into a partition with the same name, and it's fixed like
so:
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -21526,8 +21526,7 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
{
/* Create partition table with generated temporary name. */
sprintf(tmpRelName, "merge-%u-%X-tmp", RelationGetRelid(rel),
MyProcPid);
- mergePartName =
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
-
tmpRelName, -1);
+ mergePartName = makeRangeVar(mergePartName->schemaname,
tmpRelName, -1);
}
createPartitionTable(mergePartName,
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
> One of the things I dislike about this type of feature -- not this
> implementation specifically, but just this kind of idea in general --
> is that the syntax mentions a whole bunch of tables but in a way where
> you can't set their properties. Persistence, reloptions, whatever.
> There's just no place to mention any of that stuff - and if you wanted
> to create a place, you'd have to invent special syntax for each
> separate thing. That's why I think it's good that the normal way of
> creating a partition is CREATE TABLE .. PARTITION OF. Because that
> way, we know that the full power of the CREATE TABLE statement is
> always available, and you can set anything that you could set for a
> table that is not a partition.
Right. The current feature is useful and will probably work for 90% of
people's partitioned tables.
Currently, CREATE TABLE .. PARTITION OF does not create stats objects on
the child table, but MERGE PARTITIONS does, which seems strange.
Maybe stats should not be included on the new child ?
Note that stats on parent table are not analagous to indexes -
partitioned indexes do nothing other than cause indexes to be created on
any new/attached partitions. But stats objects on the parent 1) cause
extended stats to be collected and computed across the whole partition
heirarchy, and 2) do not cause stats to be computed for the individual
partitions.
Partitions can have different column definitions, for example null
constraints, FKs, defaults. And currently, if you MERGE partitions,
those will all be lost (or rather, replaced by whatever LIKE parent
gives). I think that's totally fine - anyone using different defaults
on child tables could either not use MERGE PARTITIONS, or fix up the
defaults afterwards. There's not much confusion that the details of the
differences between individual partitions will be lost when the
individual partitions are merged and no longer exist.
But I think it'd be useful to document how the new partitions will be
constructed.
--
Justin
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
18.04.2024 20:49, Alvaro Herrera wrote: On 2024-Apr-18, Alexander Lakhin wrote: I think the feature implementation should also provide tab completion for SPLIT/MERGE. I don't think that we should be imposing on feature authors or committers the task of filling in tab-completion for whatever features they contribute. I mean, if they want to add that, cool; but if not, somebody else can do that, too. It's not a critical piece. I agree, I just wanted to note the lack of the current implementation. But now, thanks to Dagfinn, we have the tab completion too. I have also a question regarding "ALTER TABLE ... SET ACCESS METHOD". The current documentation says: When applied to a partitioned table, there is no data to rewrite, but partitions created afterwards will default to the given access method unless overridden by a USING clause. But MERGE/SPLIT behave differently (if one can assume that MERGE/SPLIT create new partitions under the hood): CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler; CREATE TABLE t (i int, PRIMARY KEY(i)) PARTITION BY RANGE (i); ALTER TABLE t SET ACCESS METHOD heap2; CREATE TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (1); CREATE TABLE tp_1 PARTITION OF t FOR VALUES FROM (1) TO (2); \d t+ Partitioned table "public.t" ... Access method: heap2 Table "public.tp_0" ... Access method: heap2 Table "public.tp_1" ... Access method: heap2 ALTER TABLE t MERGE PARTITIONS (tp_0, tp_1) INTO tp_0; Partitioned table "public.t" ... Access method: heap2 Table "public.tp_0" ... Access method: heap Shouldn't it be changed, what do you think? Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi!
18.04.2024 19:00, Alexander Lakhin wrote:
leaves a strange constraint:
\d+ t*
Table "public.tp_0"
...
Not-null constraints:
"merge-16385-26BCB0-tmp_i_not_null" NOT NULL "i"
Thanks!
Attached fix (with test) for this case.
The patch should be applied after patches
v6-0001- ... .patch ... v6-0004- ... .patch
--
With best regards,
Dmitry Koval
Postgres Professional: http://postgrespro.comFrom 58e4b7fb1d3b15cdf1c742c28690392dda34915d Mon Sep 17 00:00:00 2001
From: Koval Dmitry
Date: Fri, 19 Apr 2024 01:57:49 +0300
Subject: [PATCH v6] Fix
---
src/backend/commands/tablecmds.c | 49 ++-
src/test/regress/expected/partition_merge.out | 13 -
src/test/regress/sql/partition_merge.sql | 8 ++-
3 files changed, 33 insertions(+), 37 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 72874295cb..8985747180 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -21508,9 +21508,6 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
ListCell *listptr;
List *mergingPartitionsList = NIL;
Oid defaultPartOid;
- chartmpRelName[NAMEDATALEN];
- RangeVar *mergePartName = cmd->name;
- boolisSameName = false;
/*
* Lock all merged partitions, check them and create list with
partitions
@@ -21532,8 +21529,22 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
* function transformPartitionCmdForMerge().
*/
if (equal(name, cmd->name))
+ {
/* One new partition can have the same name as merged
partition. */
- isSameName = true;
+ chartmpRelName[NAMEDATALEN];
+
+ /* Generate temporary name. */
+ sprintf(tmpRelName, "merge-%u-%X-tmp",
RelationGetRelid(rel), MyProcPid);
+
+ /* Rename partition. */
+
RenameRelationInternal(RelationGetRelid(mergingPartition),
+ tmpRelName,
false, false);
+ /*
+* We must bump the command counter to make the new
partition tuple
+* visible for rename.
+*/
+ CommandCounterIncrement();
+ }
/* Store a next merging partition into the list. */
mergingPartitionsList = lappend(mergingPartitionsList,
@@ -21553,16 +21564,8 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
DetachPartitionFinalize(rel, mergingPartition, false,
defaultPartOid);
}
- /* Create table for new partition, use partitioned table as model. */
- if (isSameName)
- {
- /* Create partition table with generated temporary name. */
- sprintf(tmpRelName, "merge-%u-%X-tmp", RelationGetRelid(rel),
MyProcPid);
- mergePartName = makeRangeVar(cmd->name->schemaname, tmpRelName,
-1);
- }
-
newPartRel = createPartitionTable(rel,
-
mergePartName,
+
cmd->name,
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
RelationGetRelationName(rel), -1),
context);
@@ -21588,26 +21591,6 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
}
list_free(mergingPartitionsList);
- /* Rename new partition if it is needed. */
- if (isSameName)
- {
- /*
-* We must bump the command counter to make the new partition
tuple
-* visible for rename.
-*/
- CommandCounterIncrement();
-
- /* Rename partition. */
- RenameRelationInternal(RelationGetRelid(newPartRel),
- cmd->name->relname,
false, false);
-
- /*
-* Bump the command counter to make the tuple of renamed
partition
-* visible for attach partition operation.
-*/
- CommandCounterIncrement();
- }
-
/*
* Attach a new partition to the partitioned table. wqueue = NULL:
* verification for each cloned constraint is not needed.
diff --git a/src/test/regress/expected/partition_merge.out
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Here are some additional fixes to docs. >From 6da8beaa5a2b78e785e5b6519894f8357002d916 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Thu, 18 Apr 2024 15:40:44 -0500 Subject: [PATCH] doc review for ALTER TABLE ... SPLIT/MERGE PARTITION --- doc/src/sgml/ddl.sgml | 4 ++-- doc/src/sgml/ref/alter_table.sgml | 22 +++--- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 026bfff70f3..01277b1d327 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -4384,7 +4384,7 @@ ALTER INDEX measurement_city_id_logdate_key There is also an option for merging multiple table partitions into - a single partition using the + a single partition using ALTER TABLE ... MERGE PARTITIONS. This feature simplifies the management of partitioned tables by allowing users to combine partitions that are no longer needed as @@ -4403,7 +4403,7 @@ ALTER TABLE measurement Similarly to merging multiple table partitions, there is an option for - splitting a single partition into multiple using the + splitting a single partition into multiple partitions using ALTER TABLE ... SPLIT PARTITION. This feature could come in handy when one partition grows too big and needs to be split into multiple. It's important to note that diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index fe36ff82e52..e52cfee840c 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -1136,16 +1136,16 @@ WITH ( MODULUS numeric_literal, REM If the split partition is a DEFAULT partition, one of the new partitions must be DEFAULT. In case one of the new partitions or one of existing partitions is DEFAULT, new partitions partition_name1, - partition_name2, ... can have spaces + partition_name2, ... can have gaps between partitions bounds. If the partitioned table does not have a DEFAULT partition, the DEFAULT partition can be defined as one of the new partitions. In case new partitions do not contain a DEFAULT partition and the partitioned table - does not have a DEFAULT partition, the following must be true: sum bounds of + does not have a DEFAULT partition, the following must be true: the sum bounds of new partitions partition_name1, partition_name2, ... should be - equal to bound of split partition partition_name. + equal to the bounds of split partition partition_name. One of the new partitions partition_name1, partition_name2, ... can have the same name as split partition partition_name @@ -1168,24 +1168,24 @@ WITH ( MODULUS numeric_literal, REM - This form merges several partitions into the one partition of the target table. - Hash-partitioning is not supported. If DEFAULT partition is not in the + This form merges several partitions of the target table into a single partition. + Hash-partitioning is not supported. If a DEFAULT partition is not in the list of partitions partition_name1, partition_name2 [, ...]: - For range-partitioned tables it is necessary that the ranges + For range-partitioned tables, it is necessary that the ranges of the partitions partition_name1, partition_name2 [, ...] can - be merged into one range without spaces and overlaps (otherwise an error + be merged into one range with neither gaps nor overlaps (otherwise an error will be generated). The combined range will be the range for the partition partition_name. - For list-partitioned tables the value lists of all partitions + For list-partitioned tables, the value lists of all partitions partition_name1, partition_name2 [, ...] are combined and form the list of values of partition @@ -1193,7 +1193,7 @@ WITH ( MODULUS numeric_literal, REM - If DEFAULT partition is in the list of partitions partition_name1, + If a DEFAULT partition is in the list of partitions partition_name1, partition_name2 [, ...]: @@ -1204,8 +1204,8 @@ WITH ( MODULUS numeric_literal, REM - For range- and list-partitioned tables the ranges and lists of values - of the merged partitions can be any. + For range- and list-partitioned tables, the ranges and lists of values + of the merged partitions can be anything. -- 2.42.0
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Thu, Apr 18, 2024 at 6:35 AM Alexander Korotkov wrote: > The revised patchset is attached. > 1) I've split the fix for the CommandCounterIncrement() issue and the > fix for relation persistence issue into a separate patch. > 2) I've validated that the lock on the new partition is held in > createPartitionTable() after ProcessUtility() as pointed out by > Robert. So, no need to place the lock again. > 3) Added fix for problematic error message as a separate patch [1]. > 4) Added rename "salemans" => "salesmen" for tests as a separate patch. > > I think these fixes are reaching committable shape, but I'd like > someone to check it before I push. Reviewing 0001: - Seems mostly fine. I think the comment /* Unlock and drop merged partitions. */ is wrong. I think it should say something like /* Drop the current partitions before adding the new one. */ because (a) it doesn't unlock anything, and there's another comment saying that and (b) we now know that the drop vs. add order matters. Reviewing 0002: - Commit message typos: behavious, corresponsing - Given the change to the header comment of createPartitionTable, it's rather surprising to me that this patch doesn't touch the documentation. Isn't that a big change in semantics? - My previous review comment was really about the code comment, I believe, rather than the use of AccessExclusiveLock. NoLock is probably fine, but if it were me I'd be tempted to write AccessExclusiveLock and just make the comment say something like /* We should already have the lock, but do it this way just to be certain */. But what you have is probably fine, too. Mostly, I want to clarify the intent of my previous comment. - Do we, or can we, have a test that if you split a partition that's not in the search path, the resulting partitions end up in your creation namespace? And similarly for merge? And maybe also that schema-qualification works properly? I haven't exhaustively verified the patch, but these are some things I noticed when scrolling through it. Reviewing 0003: - Are you sure this can't dereference datum when datum is NULL, in either the upper or lower half? It sure looks strange to have code that looks like it can make datum a null pointer, and then an unconditional deference just after. - In general I think the wording changes are improvements. I'm slightly suspicious that there might be an even better way to word it, but I can't think of it right at this very moment. - I'm kind of unhappy (but not totally unhappy) with the semantics. Suppose I have a partition that allows values from 0 to 1000, but actually only contains values that are either between 0 and 99 or between 901 and 1000. If I try to to split the partition into one that allows 0..100 and a second that allows 900..1000, it will fail. Maybe that's good, because that means that if a failure is going to happen, it will happen right at the beginning, rather than maybe after doing a lot of work. But on the other hand, it also kind of stinks, because it feels like I'm being told I can't do something that I know is perfectly fine. Reviewing 0004: - Obviously this is quite trivial and there's no real problem with it, but if we're changing it anyway, how about a gender-neutral term (salesperson/salespeople)? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On 2024-Apr-18, Alexander Lakhin wrote: > I think the feature implementation should also provide tab completion > for SPLIT/MERGE. I don't think that we should be imposing on feature authors or committers the task of filling in tab-completion for whatever features they contribute. I mean, if they want to add that, cool; but if not, somebody else can do that, too. It's not a critical piece. Now, if we're talking about whether a patch to add tab-completion to a feature post feature-freeze is acceptable, I think it absolutely is (even though you could claim that it's a new psql feature). But for sure we shouldn't mandate that a feature be reverted just because it lacks tab-completion -- such lack is not an open-item against the feature in that sense. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "That sort of implies that there are Emacs keystrokes which aren't obscure. I've been using it daily for 2 years now and have yet to discover any key sequence which makes any sense."(Paul Thomas)
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Alexander Lakhin writes:
> Hi Alexander,
>
> 18.04.2024 13:35, Alexander Korotkov wrote:
>>
>> The revised patchset is attached.
>> 1) I've split the fix for the CommandCounterIncrement() issue and the
>> fix for relation persistence issue into a separate patch.
>> 2) I've validated that the lock on the new partition is held in
>> createPartitionTable() after ProcessUtility() as pointed out by
>> Robert. So, no need to place the lock again.
>> 3) Added fix for problematic error message as a separate patch [1].
>> 4) Added rename "salemans" => "salesmen" for tests as a separate patch.
>>
>> I think these fixes are reaching committable shape, but I'd like
>> someone to check it before I push.
>
> I think the feature implementation should also provide tab completion for
> SPLIT/MERGE.
> (ALTER TABLE t S
> fills in only SET now.)
Here's a patch for that. One thing I noticed while testing it was that
the tab completeion for partitions (Query_for_partition_of_table) shows
all the schemas in the DB, even ones that don't contain any partitions
of the table being altered.
- ilmari
>From 26db03b7a7675aa7dbff1f18ee084296caa1e181 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?=
Date: Thu, 18 Apr 2024 17:47:22 +0100
Subject: [PATCH] =?UTF-8?q?Add=20tab=20completion=20for=20ALTER=20TABLE=20?=
=?UTF-8?q?=E2=80=A6=20SPLIT|MERGE=20PARTITION(S)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
---
src/bin/psql/tab-complete.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6fee3160f0..97cd5d9f62 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2353,6 +2353,7 @@ psql_completion(const char *text, int start, int end)
"OWNER TO", "SET", "VALIDATE CONSTRAINT",
"REPLICA IDENTITY", "ATTACH PARTITION",
"DETACH PARTITION", "FORCE ROW LEVEL SECURITY",
+ "SPLIT PARTITION", "MERGE PARTITIONS (",
"OF", "NOT OF");
/* ALTER TABLE xxx ADD */
else if (Matches("ALTER", "TABLE", MatchAny, "ADD"))
@@ -2609,10 +2610,10 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH("FROM (", "IN (", "WITH (");
/*
- * If we have ALTER TABLE DETACH PARTITION, provide a list of
+ * If we have ALTER TABLE DETACH|SPLIT PARTITION, provide a list of
* partitions of .
*/
- else if (Matches("ALTER", "TABLE", MatchAny, "DETACH", "PARTITION"))
+ else if (Matches("ALTER", "TABLE", MatchAny, "DETACH|SPLIT", "PARTITION"))
{
set_completion_reference(prev3_wd);
COMPLETE_WITH_SCHEMA_QUERY(Query_for_partition_of_table);
@@ -2620,6 +2621,19 @@ psql_completion(const char *text, int start, int end)
else if (Matches("ALTER", "TABLE", MatchAny, "DETACH", "PARTITION", MatchAny))
COMPLETE_WITH("CONCURRENTLY", "FINALIZE");
+ /* ALTER TABLE SPLIT PARTITION */
+ else if (Matches("ALTER", "TABLE", MatchAny, "SPLIT", "PARTITION", MatchAny))
+ COMPLETE_WITH("INTO ( PARTITION");
+
+ /* ALTER TABLE MERGE PARTITIONS ( */
+ else if (Matches("ALTER", "TABLE", MatchAny, "MERGE", "PARTITIONS", "("))
+ {
+ set_completion_reference(prev4_wd);
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_partition_of_table);
+ }
+ else if (Matches("ALTER", "TABLE", MatchAny, "MERGE", "PARTITIONS", "(*)"))
+ COMPLETE_WITH("INTO");
+
/* ALTER TABLE OF */
else if (Matches("ALTER", "TABLE", MatchAny, "OF"))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_composite_datatypes);
--
2.39.2
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi Alexander, 18.04.2024 13:35, Alexander Korotkov wrote: The revised patchset is attached. 1) I've split the fix for the CommandCounterIncrement() issue and the fix for relation persistence issue into a separate patch. 2) I've validated that the lock on the new partition is held in createPartitionTable() after ProcessUtility() as pointed out by Robert. So, no need to place the lock again. 3) Added fix for problematic error message as a separate patch [1]. 4) Added rename "salemans" => "salesmen" for tests as a separate patch. I think these fixes are reaching committable shape, but I'd like someone to check it before I push. I think the feature implementation should also provide tab completion for SPLIT/MERGE. (ALTER TABLE t S fills in only SET now.) Also, the following MERGE operation: CREATE TABLE t (i int, PRIMARY KEY(i)) PARTITION BY RANGE (i); CREATE TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (1); CREATE TABLE tp_1 PARTITION OF t FOR VALUES FROM (1) TO (2); ALTER TABLE t MERGE PARTITIONS (tp_0, tp_1) INTO tp_0; leaves a strange constraint: \d+ t* Table "public.tp_0" ... Not-null constraints: "merge-16385-26BCB0-tmp_i_not_null" NOT NULL "i" Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi, Dmitry! On Mon, Apr 15, 2024 at 6:26 PM Dmitry Koval wrote: > > Hi! > > > Please, find a my version of this fix attached. > > Is it possible to make a small addition to the file v6-0001 ... .patch > (see attachment)? > > Most important: > 1) Line 19: > > + mergePartName = makeRangeVar(cmd->name->schemaname, tmpRelName, -1); > > (temporary table should use the same schema as the partition); > > 2) Lines 116-123: > > +RESET search_path; > + > +-- Can't merge persistent partitions into a temporary partition > +ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO pg_temp.tp_0_2; > + > +SET search_path = pg_temp, public; > > (Alexandr Lakhin's test for using of pg_temp schema explicitly). > > > The rest of the changes in v6_afterfix.diff are not very important and > can be ignored. Thank you. I've integrated your changes. The revised patchset is attached. 1) I've split the fix for the CommandCounterIncrement() issue and the fix for relation persistence issue into a separate patch. 2) I've validated that the lock on the new partition is held in createPartitionTable() after ProcessUtility() as pointed out by Robert. So, no need to place the lock again. 3) Added fix for problematic error message as a separate patch [1]. 4) Added rename "salemans" => "salesmen" for tests as a separate patch. I think these fixes are reaching committable shape, but I'd like someone to check it before I push. Links. 1. https://postgr.es/m/20240408.152402.1485994009160660141.horikyota.ntt%40gmail.com -- Regards, Alexander Korotkov v6-0002-Verify-persistence-of-new-partitions-during-MERGE.patch Description: Binary data v6-0004-Grammar-fix-for-tests-of-partition-MERGE-SPLIT-op.patch Description: Binary data v6-0003-Fix-error-message-in-check_partition_bounds_for_s.patch Description: Binary data v6-0001-Add-missing-CommandCounterIncrement-to-ATExecMerg.patch Description: Binary data
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Mon, Apr 15, 2024 at 11:00 AM Alexander Lakhin wrote: > Initially I was confused by that message, because of: > CREATE TABLE t (i int) PARTITION BY RANGE (i); > CREATE FOREIGN TABLE ftp_0_1 PARTITION OF t >FOR VALUES FROM (0) TO (1) >SERVER loopback OPTIONS (table_name 'lt_0_1'); > CREATE FOREIGN TABLE ftp_1_2 PARTITION OF t >FOR VALUES FROM (1) TO (2) >SERVER loopback OPTIONS (table_name 'lt_1_2'); > ALTER TABLE t MERGE PARTITIONS (ftp_0_1, ftp_1_2) INTO ftp_0_2; > ERROR: "ftp_0_1" is not a table > (Isn't a foreign table a table?) I agree that this can be confusing, but a patch that is about adding SPLIT and MERGE PARTITION operations cannot decide to also invent a new error message phraseology and use it only in one place. We need to maintain consistency across the whole code base. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi!
Please, find a my version of this fix attached.
Is it possible to make a small addition to the file v6-0001 ... .patch
(see attachment)?
Most important:
1) Line 19:
+ mergePartName = makeRangeVar(cmd->name->schemaname, tmpRelName, -1);
(temporary table should use the same schema as the partition);
2) Lines 116-123:
+RESET search_path;
+
+-- Can't merge persistent partitions into a temporary partition
+ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO pg_temp.tp_0_2;
+
+SET search_path = pg_temp, public;
(Alexandr Lakhin's test for using of pg_temp schema explicitly).
The rest of the changes in v6_afterfix.diff are not very important and
can be ignored.
--
With best regards,
Dmitry Koval
Postgres Professional: http://postgrespro.comdiff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c6ce7b94d9..bce5e39b64 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -21130,6 +21130,7 @@ moveSplitTableRows(Relation rel, Relation splitRel,
List *partlist, List *newPar
*
* Emulates command: CREATE [TEMP] TABLE (LIKE
* INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY)
+ * Function locks created relation in AccessExclusiveLock mode and returns it.
*/
static Relation
createPartitionTable(Relation rel, RangeVar *newPartName, RangeVar
*modelRelName,
@@ -21500,8 +21501,7 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
{
/* Create partition table with generated temparary name. */
sprintf(tmpRelName, "merge-%u-%X-tmp", RelationGetRelid(rel),
MyProcPid);
- mergePartName =
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
-
tmpRelName, -1);
+ mergePartName = makeRangeVar(cmd->name->schemaname, tmpRelName,
-1);
}
newPartRel = createPartitionTable(rel,
diff --git a/src/test/regress/expected/partition_merge.out
b/src/test/regress/expected/partition_merge.out
index b1d0b50b0b..0a4022f714 100644
--- a/src/test/regress/expected/partition_merge.out
+++ b/src/test/regress/expected/partition_merge.out
@@ -793,23 +793,58 @@ SELECT indexname FROM pg_indexes WHERE tablename =
'tp_1_2';
DROP TABLE t;
--
--- Try creating a partition in the temporary schema.
+-- Try mixing permanent and temporary partitions.
--
SET search_path = public, pg_temp;
CREATE TABLE t (i int) PARTITION BY RANGE (i);
CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
+SELECT c.oid::pg_catalog.regclass, c.relpersistence FROM pg_catalog.pg_class c
WHERE c.oid = 't'::regclass;
+ oid | relpersistence
+-+
+ t | p
+(1 row)
+
+SELECT c.oid::pg_catalog.regclass, pg_catalog.pg_get_expr(c.relpartbound,
c.oid), c.relpersistence
+ FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i
+ WHERE c.oid = i.inhrelid AND i.inhparent = 't'::regclass
+ ORDER BY pg_catalog.pg_get_expr(c.relpartbound, c.oid) = 'DEFAULT',
c.oid::pg_catalog.regclass::pg_catalog.text;
+ oid |pg_get_expr | relpersistence
+++
+ tp_0_1 | FOR VALUES FROM (0) TO (1) | p
+ tp_1_2 | FOR VALUES FROM (1) TO (2) | p
+(2 rows)
+
SET search_path = pg_temp, public;
-- Can't merge persistent partitions into a temporary partition
ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2;
ERROR: cannot create a temporary relation as partition of permanent relation
"t"
+RESET search_path;
+-- Can't merge persistent partitions into a temporary partition
+ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO pg_temp.tp_0_2;
+ERROR: cannot create a temporary relation as partition of permanent relation
"t"
DROP TABLE t;
-DROP TAble tp_0_2;
-ERROR: table "tp_0_2" does not exist
+SET search_path = pg_temp, public;
BEGIN;
CREATE TABLE t (i int) PARTITION BY RANGE (i);
CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
+SELECT c.oid::pg_catalog.regclass, c.relpersistence FROM pg_catalog.pg_class c
WHERE c.oid = 't'::regclass;
+ oid | relpersistence
+-+
+ t | t
+(1 row)
+
+SELECT c.oid::pg_catalog.regclass, pg_catalog.pg_get_expr(c.relpartbound,
c.oid), c.relpersistence
+ FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i
+ WHERE c.oid = i.inhrelid AND i.inhparent = 't'::regclass
+ ORDER BY pg_catalog.pg_get_expr(c.relpartbound, c.oid) = 'DEFAULT',
c.oid::pg_catalog.regclass::pg_catalog.text;
+ oid |pg_get_expr | relpersistence
+++
+ tp_0_1 | FOR VALUES FROM (0) TO (1) | t
+ tp_1_2 | FOR VALUES FROM (1) TO (2) | t
+(2 rows)
+
SET search_path = public, pg_temp;
-- Can't merge temporary partitions into a persistent partition
ALTER TABLE t MERGE PARTITIONS
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hello Robert,
15.04.2024 17:30, Robert Haas wrote:
On Sat, Apr 13, 2024 at 6:05 AM Alexander Korotkov wrote:
Please, find a my version of this fix attached. I think we need to
check relpersistence in a similar way ATTACH PARTITION or CREATE TABLE
... PARTITION OF do. I'm going to polish this a little bit more.
+ errmsg("\"%s\" is not an ordinary table",
This is not a phrasing that we use in any other error message. We
always just say "is not a table".
Initially I was confused by that message, because of:
CREATE TABLE t (i int) PARTITION BY RANGE (i);
CREATE FOREIGN TABLE ftp_0_1 PARTITION OF t
FOR VALUES FROM (0) TO (1)
SERVER loopback OPTIONS (table_name 'lt_0_1');
CREATE FOREIGN TABLE ftp_1_2 PARTITION OF t
FOR VALUES FROM (1) TO (2)
SERVER loopback OPTIONS (table_name 'lt_1_2');
ALTER TABLE t MERGE PARTITIONS (ftp_0_1, ftp_1_2) INTO ftp_0_2;
ERROR: "ftp_0_1" is not a table
(Isn't a foreign table a table?)
And also:
CREATE TABLE t (i int) PARTITION BY RANGE (i);
CREATE TABLE tp_0_1 PARTITION OF t
FOR VALUES FROM (0) TO (1);
CREATE TABLE t2 (i int) PARTITION BY RANGE (i);
ALTER TABLE t MERGE PARTITIONS (tp_0_1, t2) INTO tpn;
ERROR: "t2" is not a table
(Isn't a partitioned table a table?)
And in fact, an ordinary table is not suitable for MERGE anyway:
CREATE TABLE t (i int) PARTITION BY RANGE (i);
CREATE TABLE tp_0_1 PARTITION OF t
FOR VALUES FROM (0) TO (1);
CREATE TABLE t2 (i int);
ALTER TABLE t MERGE PARTITIONS (tp_0_1, t2) INTO tpn;
ERROR: "t2" is not a partition
So I don't think that "an ordinary table" is a good (unambiguous) term
either.
Best regards,
Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sat, Apr 13, 2024 at 6:05 AM Alexander Korotkov wrote:
> Please, find a my version of this fix attached. I think we need to
> check relpersistence in a similar way ATTACH PARTITION or CREATE TABLE
> ... PARTITION OF do. I'm going to polish this a little bit more.
+ errmsg("\"%s\" is not an ordinary table",
This is not a phrasing that we use in any other error message. We
always just say "is not a table".
+ * Open the new partition and acquire exclusive lock on it. This will
A minor nitpick is that this should probably say access exclusive
rather than exclusive. But the bigger thing that confuses me here is
that if we just created the partition, surely we must *already* hold
AccessExclusiveLoc on it. No?
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi, Dmitry! On Fri, Apr 12, 2024 at 10:59 PM Dmitry Koval wrote: > > Thanks, Alexander! > > > Still now we're able to create a partition in the pg_temp schema > > explicitly. > > Attached patches with fix. Please, find a my version of this fix attached. I think we need to check relpersistence in a similar way ATTACH PARTITION or CREATE TABLE ... PARTITION OF do. I'm going to polish this a little bit more. -- Regards, Alexander Korotkov v6-0001-Fix-for-SPLIT-MERGE-partitions-of-temporary-table.patch Description: Binary data
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Thanks, Alexander!
Still now we're able to create a partition in the pg_temp schema
explicitly.
Attached patches with fix.
--
With best regards,
Dmitry Koval
Postgres Professional: http://postgrespro.comFrom 2b68bbdb068e881e8ca6e34dec735f7ce656374f Mon Sep 17 00:00:00 2001
From: Koval Dmitry
Date: Wed, 10 Apr 2024 18:54:05 +0300
Subject: [PATCH v5 1/2] Fix for SPLIT/MERGE partitions of temporary table
---
src/backend/commands/tablecmds.c | 32 +++--
src/backend/parser/parse_utilcmd.c| 36 -
src/test/regress/expected/partition_merge.out | 124 +-
src/test/regress/expected/partition_split.out | 29
src/test/regress/sql/partition_merge.sql | 101 +-
src/test/regress/sql/partition_split.sql | 23
6 files changed, 324 insertions(+), 21 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8a98a0af48..b59e1dda03 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -21145,17 +21145,19 @@ moveSplitTableRows(Relation rel, Relation splitRel,
List *partlist, List *newPar
* createPartitionTable: create table for a new partition with given name
* (newPartName) like table (modelRelName)
*
- * Emulates command: CREATE TABLE (LIKE
+ * Emulates command: CREATE [TEMP] TABLE (LIKE
* INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY)
*/
static void
-createPartitionTable(RangeVar *newPartName, RangeVar *modelRelName,
+createPartitionTable(Relation rel, RangeVar *newPartName, RangeVar
*modelRelName,
AlterTableUtilityContext *context)
{
CreateStmt *createStmt;
TableLikeClause *tlc;
PlannedStmt *wrapper;
+ newPartName->relpersistence = rel->rd_rel->relpersistence;
+
createStmt = makeNode(CreateStmt);
createStmt->relation = newPartName;
createStmt->tableElts = NIL;
@@ -21291,7 +21293,7 @@ ATExecSplitPartition(List **wqueue, AlteredTableInfo
*tab, Relation rel,
SinglePartitionSpec *sps = (SinglePartitionSpec *)
lfirst(listptr);
RelationnewPartRel;
- createPartitionTable(sps->name, parentName, context);
+ createPartitionTable(rel, sps->name, parentName, context);
/* Open the new partition and acquire exclusive lock on it. */
newPartRel = table_openrv(sps->name, AccessExclusiveLock);
@@ -21488,10 +21490,10 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
{
/* Create partition table with generated temparary name. */
sprintf(tmpRelName, "merge-%u-%X-tmp", RelationGetRelid(rel),
MyProcPid);
- mergePartName =
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
-
tmpRelName, -1);
+ mergePartName = makeRangeVar(cmd->name->schemaname, tmpRelName,
-1);
}
- createPartitionTable(mergePartName,
+ createPartitionTable(rel,
+mergePartName,
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
RelationGetRelationName(rel), -1),
context);
@@ -21507,12 +21509,6 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
/* Copy data from merged partitions to new partition. */
moveMergedTablesRows(rel, mergingPartitionsList, newPartRel);
- /*
-* Attach a new partition to the partitioned table. wqueue = NULL:
-* verification for each cloned constraint is not need.
-*/
- attachPartitionTable(NULL, rel, newPartRel, cmd->bound);
-
/* Unlock and drop merged partitions. */
foreach(listptr, mergingPartitionsList)
{
@@ -21542,7 +21538,19 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
/* Rename partition. */
RenameRelationInternal(RelationGetRelid(newPartRel),
cmd->name->relname,
false, false);
+ /*
+* Bump the command counter to make the tuple of renamed
partition
+* visible for attach partition operation.
+*/
+ CommandCounterIncrement();
}
+
+ /*
+* Attach a new partition to the partitioned table. wqueue = NULL:
+* verification for each cloned constraint is not needed.
+*/
+ attachPartitionTable(NULL, rel, newPartRel, cmd->bound);
+
/* Keep the lock until commit. */
table_close(newPartRel, NoLock);
}
diff --git a/src/backend/parser/parse_utilcmd.c
b/src/backend/parser/parse_utilcmd.c
index
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi Dmitry, 12.04.2024 16:04, Dmitry Koval wrote: Hi! Attached is a patch with corrections based on comments in previous letters (I think these corrections are not final). I'll be very grateful for feedbacks and bug reports. 11.04.2024 20:00, Alexander Lakhin wrote: > may be an attempt to merge into implicit > pg_temp should fail just like CREATE TABLE ... PARTITION OF ... does? Corrected. Result is: Thank you! Still now we're able to create a partition in the pg_temp schema explicitly. Please try: ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO pg_temp.tp_0_2; in the scenario [1] and you'll get the same empty table. [1] https://www.postgresql.org/message-id/fdaa003e-919c-cbc9-4f0c-e4546e96bd65%40gmail.com Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi!
Attached is a patch with corrections based on comments in previous
letters (I think these corrections are not final).
I'll be very grateful for feedbacks and bug reports.
11.04.2024 20:00, Alexander Lakhin wrote:
> may be an attempt to merge into implicit
> pg_temp should fail just like CREATE TABLE ... PARTITION OF ... does?
Corrected. Result is:
\d+ s1.*
Table "s1.tp0"
...
Table "s1.tp1"
...
\d+ tp*
Did not find any relation named "tp*".
12.04.2024 4:53, Alexander Korotkov wrote:
> I think we shouldn't unconditionally copy schema name and
> relpersistence from the parent table. Instead we should throw the
> error on a mismatch like CREATE TABLE ... PARTITION OF ... does.
12.04.2024 5:20, Robert Haas wrote:
> We definitely shouldn't copy the schema name from the parent table.
Fixed.
12.04.2024 5:20, Robert Haas wrote:
> One of the things I dislike about this type of feature -- not this
> implementation specifically, but just this kind of idea in general --
> is that the syntax mentions a whole bunch of tables but in a way where
> you can't set their properties. Persistence, reloptions, whatever.
In next releases I want to allow specifying options (probably, first of
all, specifying tablespace of the partitions).
But before that, I would like to get a users reaction - what options
they really need?
--
With best regards,
Dmitry Koval
Postgres Professional: http://postgrespro.comFrom 707d15cf9ee4673a1deed2825f48ffbc09a34e9d Mon Sep 17 00:00:00 2001
From: Koval Dmitry
Date: Wed, 10 Apr 2024 18:54:05 +0300
Subject: [PATCH v4 1/2] Fix for SPLIT/MERGE partitions of temporary table
---
src/backend/commands/tablecmds.c | 32 +++--
src/backend/parser/parse_utilcmd.c| 19 ++-
src/test/regress/expected/partition_merge.out | 118 +-
src/test/regress/expected/partition_split.out | 29 +
src/test/regress/sql/partition_merge.sql | 95 +-
src/test/regress/sql/partition_split.sql | 23
6 files changed, 297 insertions(+), 19 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8a98a0af48..b59e1dda03 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -21145,17 +21145,19 @@ moveSplitTableRows(Relation rel, Relation splitRel,
List *partlist, List *newPar
* createPartitionTable: create table for a new partition with given name
* (newPartName) like table (modelRelName)
*
- * Emulates command: CREATE TABLE (LIKE
+ * Emulates command: CREATE [TEMP] TABLE (LIKE
* INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY)
*/
static void
-createPartitionTable(RangeVar *newPartName, RangeVar *modelRelName,
+createPartitionTable(Relation rel, RangeVar *newPartName, RangeVar
*modelRelName,
AlterTableUtilityContext *context)
{
CreateStmt *createStmt;
TableLikeClause *tlc;
PlannedStmt *wrapper;
+ newPartName->relpersistence = rel->rd_rel->relpersistence;
+
createStmt = makeNode(CreateStmt);
createStmt->relation = newPartName;
createStmt->tableElts = NIL;
@@ -21291,7 +21293,7 @@ ATExecSplitPartition(List **wqueue, AlteredTableInfo
*tab, Relation rel,
SinglePartitionSpec *sps = (SinglePartitionSpec *)
lfirst(listptr);
RelationnewPartRel;
- createPartitionTable(sps->name, parentName, context);
+ createPartitionTable(rel, sps->name, parentName, context);
/* Open the new partition and acquire exclusive lock on it. */
newPartRel = table_openrv(sps->name, AccessExclusiveLock);
@@ -21488,10 +21490,10 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
{
/* Create partition table with generated temparary name. */
sprintf(tmpRelName, "merge-%u-%X-tmp", RelationGetRelid(rel),
MyProcPid);
- mergePartName =
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
-
tmpRelName, -1);
+ mergePartName = makeRangeVar(cmd->name->schemaname, tmpRelName,
-1);
}
- createPartitionTable(mergePartName,
+ createPartitionTable(rel,
+mergePartName,
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
RelationGetRelationName(rel), -1),
context);
@@ -21507,12 +21509,6 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
/* Copy data from merged partitions to new partition. */
moveMergedTablesRows(rel, mergingPartitionsList, newPartRel);
- /*
-* Attach a new partition to the partitioned table. wqueue =
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Thu, Apr 11, 2024 at 9:54 PM Alexander Korotkov wrote: > I think we shouldn't unconditionally copy schema name and > relpersistence from the parent table. Instead we should throw the > error on a mismatch like CREATE TABLE ... PARTITION OF ... does. I'm > working on revising this fix. We definitely shouldn't copy the schema name from the parent table. It should be possible to schema-qualify the new partition names, and if you don't, then the search_path should determine where they get placed. But I am inclined to think that relpersistence should be copied. It's weird that you split an unlogged partition and you get logged partitions. One of the things I dislike about this type of feature -- not this implementation specifically, but just this kind of idea in general -- is that the syntax mentions a whole bunch of tables but in a way where you can't set their properties. Persistence, reloptions, whatever. There's just no place to mention any of that stuff - and if you wanted to create a place, you'd have to invent special syntax for each separate thing. That's why I think it's good that the normal way of creating a partition is CREATE TABLE .. PARTITION OF. Because that way, we know that the full power of the CREATE TABLE statement is always available, and you can set anything that you could set for a table that is not a partition. Of course, that is not to say that some people won't like to have a feature of this sort. I expect they will. The approach does have some drawbacks, though. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Thu, Apr 11, 2024 at 8:00 PM Alexander Lakhin wrote: > 11.04.2024 16:27, Dmitry Koval wrote: > > > > Added correction (and test), see > > v3-0001-Fix-for-SPLIT-MERGE-partitions-of-temporary-table.patch. > > > > Thank you for the correction, but may be an attempt to merge into implicit > pg_temp should fail just like CREATE TABLE ... PARTITION OF ... does? > > Please look also at another anomaly with schemas: > CREATE SCHEMA s1; > CREATE TABLE t (i int) PARTITION BY RANGE (i); > CREATE TABLE tp_0_2 PARTITION OF t >FOR VALUES FROM (0) TO (2); > ALTER TABLE t SPLIT PARTITION tp_0_2 INTO >(PARTITION s1.tp0 FOR VALUES FROM (0) TO (1), PARTITION s1.tp1 FOR VALUES > FROM (1) TO (2)); > results in: > \d+ s1.* > Did not find any relation named "s1.*" > \d+ tp* >Table "public.tp0" > ... > Table "public.tp1" +1 I think we shouldn't unconditionally copy schema name and relpersistence from the parent table. Instead we should throw the error on a mismatch like CREATE TABLE ... PARTITION OF ... does. I'm working on revising this fix. -- Regards, Alexander Korotkov
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
11.04.2024 16:27, Dmitry Koval wrote: Added correction (and test), see v3-0001-Fix-for-SPLIT-MERGE-partitions-of-temporary-table.patch. Thank you for the correction, but may be an attempt to merge into implicit pg_temp should fail just like CREATE TABLE ... PARTITION OF ... does? Please look also at another anomaly with schemas: CREATE SCHEMA s1; CREATE TABLE t (i int) PARTITION BY RANGE (i); CREATE TABLE tp_0_2 PARTITION OF t FOR VALUES FROM (0) TO (2); ALTER TABLE t SPLIT PARTITION tp_0_2 INTO (PARTITION s1.tp0 FOR VALUES FROM (0) TO (1), PARTITION s1.tp1 FOR VALUES FROM (1) TO (2)); results in: \d+ s1.* Did not find any relation named "s1.*" \d+ tp* Table "public.tp0" ... Table "public.tp1" ... Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi, Dmitry! On Thu, Apr 11, 2024 at 4:27 PM Dmitry Koval wrote: > 1. > Alexander Lakhin sent a question about index name after MERGE (partition > name is the same as one of the merged partitions): > > start of quote > I'm also confused by an index name after MERGE: > CREATE TABLE t (i int) PARTITION BY RANGE (i); > > CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1); > CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2); > > CREATE INDEX tidx ON t(i); > ALTER TABLE t MERGE PARTITIONS (tp_1_2, tp_0_1) INTO tp_1_2; > \d+ t* > > Table "public.tp_1_2" > Column | Type | Collation | Nullable | Default | Storage | > Compression | Stats target | Description > +-+---+--+-+-+-+--+- > i | integer | | | | plain | > | | > Partition of: t FOR VALUES FROM (0) TO (2) > Partition constraint: ((i IS NOT NULL) AND (i >= 0) AND (i < 2)) > Indexes: > "merge-16385-3A14B2-tmp_i_idx" btree (i) > > Is the name "merge-16385-3A14B2-tmp_i_idx" valid or it's something > temporary? > end of quote > > Fix for this case added to file > v3-0001-Fix-for-SPLIT-MERGE-partitions-of-temporary-table.patch. > > > > 2. > >It seems to me that v2-0001-Fix-for-SPLIT-MERGE-partitions-of- > >temporary-table.patch is not complete either. > > Added correction (and test), see > v3-0001-Fix-for-SPLIT-MERGE-partitions-of-temporary-table.patch. Thank you, I'll review this later today. -- Regards, Alexander Korotkov
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi!
1.
Alexander Lakhin sent a question about index name after MERGE (partition
name is the same as one of the merged partitions):
start of quote
I'm also confused by an index name after MERGE:
CREATE TABLE t (i int) PARTITION BY RANGE (i);
CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
CREATE INDEX tidx ON t(i);
ALTER TABLE t MERGE PARTITIONS (tp_1_2, tp_0_1) INTO tp_1_2;
\d+ t*
Table "public.tp_1_2"
Column | Type | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
+-+---+--+-+-+-+--+-
i | integer | | | | plain |
| |
Partition of: t FOR VALUES FROM (0) TO (2)
Partition constraint: ((i IS NOT NULL) AND (i >= 0) AND (i < 2))
Indexes:
"merge-16385-3A14B2-tmp_i_idx" btree (i)
Is the name "merge-16385-3A14B2-tmp_i_idx" valid or it's something
temporary?
end of quote
Fix for this case added to file
v3-0001-Fix-for-SPLIT-MERGE-partitions-of-temporary-table.patch.
2.
>It seems to me that v2-0001-Fix-for-SPLIT-MERGE-partitions-of-
>temporary-table.patch is not complete either.
Added correction (and test), see
v3-0001-Fix-for-SPLIT-MERGE-partitions-of-temporary-table.patch.
--
With best regards,
Dmitry Koval
Postgres Professional: http://postgrespro.comFrom 58eb4abd4f065b6aa423bbddf62acd1799eba22e Mon Sep 17 00:00:00 2001
From: Koval Dmitry
Date: Wed, 10 Apr 2024 18:54:05 +0300
Subject: [PATCH v3 1/2] Fix for SPLIT/MERGE partitions of temporary table
---
src/backend/commands/tablecmds.c | 30 ---
src/backend/parser/parse_utilcmd.c| 2 +-
src/test/regress/expected/partition_merge.out | 87 ++-
src/test/regress/expected/partition_split.out | 29 +++
src/test/regress/sql/partition_merge.sql | 70 ++-
src/test/regress/sql/partition_split.sql | 23 +
6 files changed, 227 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8a98a0af48..2769be55be 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -21145,17 +21145,20 @@ moveSplitTableRows(Relation rel, Relation splitRel,
List *partlist, List *newPar
* createPartitionTable: create table for a new partition with given name
* (newPartName) like table (modelRelName)
*
- * Emulates command: CREATE TABLE (LIKE
+ * Emulates command: CREATE [TEMP] TABLE (LIKE
* INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY)
*/
static void
-createPartitionTable(RangeVar *newPartName, RangeVar *modelRelName,
+createPartitionTable(Relation rel, RangeVar *newPartName, RangeVar
*modelRelName,
AlterTableUtilityContext *context)
{
CreateStmt *createStmt;
TableLikeClause *tlc;
PlannedStmt *wrapper;
+ newPartName->relpersistence = rel->rd_rel->relpersistence;
+ newPartName->schemaname = modelRelName->schemaname;
+
createStmt = makeNode(CreateStmt);
createStmt->relation = newPartName;
createStmt->tableElts = NIL;
@@ -21291,7 +21294,7 @@ ATExecSplitPartition(List **wqueue, AlteredTableInfo
*tab, Relation rel,
SinglePartitionSpec *sps = (SinglePartitionSpec *)
lfirst(listptr);
RelationnewPartRel;
- createPartitionTable(sps->name, parentName, context);
+ createPartitionTable(rel, sps->name, parentName, context);
/* Open the new partition and acquire exclusive lock on it. */
newPartRel = table_openrv(sps->name, AccessExclusiveLock);
@@ -21491,7 +21494,8 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
mergePartName =
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
tmpRelName, -1);
}
- createPartitionTable(mergePartName,
+ createPartitionTable(rel,
+mergePartName,
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
RelationGetRelationName(rel), -1),
context);
@@ -21507,12 +21511,6 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
/* Copy data from merged partitions to new partition. */
moveMergedTablesRows(rel, mergingPartitionsList, newPartRel);
- /*
-* Attach a new partition to the partitioned table. wqueue = NULL:
-* verification for each cloned constraint is not need.
-*/
- attachPartitionTable(NULL, rel,
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi Dmitry, 11.04.2024 11:59, Dmitry Koval wrote: FWIW, I also proposed a patch earlier that fixes error messages and comments in the split partition code Sorry, I thought all the fixes you suggested were already included in v1-0002-Fixes-for-english-text.patch (but they are not). Added missing lines to v2-0002-Fixes-for-english-text.patch. It seems to me that v2-0001-Fix-for-SPLIT-MERGE-partitions-of-temporary-table.patch is not complete either. Take a look, please: CREATE TABLE t (i int) PARTITION BY RANGE (i); SET search_path = pg_temp, public; CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1); -- fails with: ERROR: cannot create a temporary relation as partition of permanent relation "t" But: CREATE TABLE t (i int) PARTITION BY RANGE (i); CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1); CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2); INSERT INTO t VALUES(0), (1); SELECT * FROM t; -- the expected result is: i --- 0 1 (2 rows) SET search_path = pg_temp, public; ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2; -- succeeds, and \c - SELECT * FROM t; -- gives: i --- (0 rows) Please also ask your tech writers to check contents of src/test/sql/*, if possible (perhaps, they'll fix "salesmans" and improve grammar). Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi!
FWIW, I also proposed a patch earlier that fixes error messages and
comments in the split partition code
Sorry, I thought all the fixes you suggested were already included in
v1-0002-Fixes-for-english-text.patch (but they are not).
Added missing lines to v2-0002-Fixes-for-english-text.patch.
--
With best regards,
Dmitry Koval
Postgres Professional: http://postgrespro.comFrom cfd5a56d15e7ead6dd7ae66cd382de3fa38150d7 Mon Sep 17 00:00:00 2001
From: Koval Dmitry
Date: Wed, 10 Apr 2024 18:54:05 +0300
Subject: [PATCH v2 1/2] Fix for SPLIT/MERGE partitions of temporary table
---
src/backend/commands/tablecmds.c | 11 ---
src/backend/parser/parse_utilcmd.c| 2 +-
src/test/regress/expected/partition_merge.out | 32 +--
src/test/regress/expected/partition_split.out | 29 +
src/test/regress/sql/partition_merge.sql | 24 +-
src/test/regress/sql/partition_split.sql | 23 +
6 files changed, 113 insertions(+), 8 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8a98a0af48..3da9f6389d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -21145,17 +21145,19 @@ moveSplitTableRows(Relation rel, Relation splitRel,
List *partlist, List *newPar
* createPartitionTable: create table for a new partition with given name
* (newPartName) like table (modelRelName)
*
- * Emulates command: CREATE TABLE (LIKE
+ * Emulates command: CREATE [TEMP] TABLE (LIKE
* INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY)
*/
static void
-createPartitionTable(RangeVar *newPartName, RangeVar *modelRelName,
+createPartitionTable(Relation rel, RangeVar *newPartName, RangeVar
*modelRelName,
AlterTableUtilityContext *context)
{
CreateStmt *createStmt;
TableLikeClause *tlc;
PlannedStmt *wrapper;
+ newPartName->relpersistence = rel->rd_rel->relpersistence;
+
createStmt = makeNode(CreateStmt);
createStmt->relation = newPartName;
createStmt->tableElts = NIL;
@@ -21291,7 +21293,7 @@ ATExecSplitPartition(List **wqueue, AlteredTableInfo
*tab, Relation rel,
SinglePartitionSpec *sps = (SinglePartitionSpec *)
lfirst(listptr);
RelationnewPartRel;
- createPartitionTable(sps->name, parentName, context);
+ createPartitionTable(rel, sps->name, parentName, context);
/* Open the new partition and acquire exclusive lock on it. */
newPartRel = table_openrv(sps->name, AccessExclusiveLock);
@@ -21491,7 +21493,8 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
mergePartName =
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
tmpRelName, -1);
}
- createPartitionTable(mergePartName,
+ createPartitionTable(rel,
+mergePartName,
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
RelationGetRelationName(rel), -1),
context);
diff --git a/src/backend/parser/parse_utilcmd.c
b/src/backend/parser/parse_utilcmd.c
index ceba069905..a3bbcc99c0 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3430,7 +3430,7 @@ checkPartition(Relation rel, Oid partRelOid)
if (partRel->rd_rel->relkind != RELKIND_RELATION)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-errmsg("\"%s\" is not a table",
+errmsg("\"%s\" is not an ordinary table",
RelationGetRelationName(partRel;
if (!partRel->rd_rel->relispartition)
diff --git a/src/test/regress/expected/partition_merge.out
b/src/test/regress/expected/partition_merge.out
index 60eacf6bf3..9fe97a801d 100644
--- a/src/test/regress/expected/partition_merge.out
+++ b/src/test/regress/expected/partition_merge.out
@@ -25,9 +25,9 @@ ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022,
sales_mar2022, sales_fe
ERROR: partition with name "sales_feb2022" already used
LINE 1: ...e MERGE PARTITIONS (sales_feb2022, sales_mar2022, sales_feb2...
^
--- ERROR: "sales_apr2022" is not a table
+-- ERROR: "sales_apr2022" is not an ordinary table
ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022, sales_mar2022,
sales_apr2022) INTO sales_feb_mar_apr2022;
-ERROR: "sales_apr2022" is not a table
+ERROR: "sales_apr2022" is not an ordinary table
-- ERROR: invalid partitions order, partition
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Thu, Apr 11, 2024 at 1:22 AM Dmitry Koval wrote: > 2) v1-0002-Fixes-for-english-text.patch - fixes for English text > (comments, error messages etc.). FWIW, I also proposed a patch earlier that fixes error messages and comments in the split partition code at https://www.postgresql.org/message-id/flat/CAMbWs49DDsknxyoycBqiE72VxzL_sYHF6zqL8dSeNehKPJhkKg%40mail.gmail.com Thanks Richard
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi!
Alexander Korotkov, thanks for the commit of previous fix.
Alexander Lakhin, thanks for the problem you found.
There are two corrections attached to the letter:
1) v1-0001-Fix-for-SPLIT-MERGE-partitions-of-temporary-table.patch - fix
for the problem [1].
2) v1-0002-Fixes-for-english-text.patch - fixes for English text
(comments, error messages etc.).
Links:
[1]
https://www.postgresql.org/message-id/dbc8b96c-3cf0-d1ee-860d-0e491da20485%40gmail.com
--
With best regards,
Dmitry Koval
Postgres Professional: http://postgrespro.comFrom 3c443a57c334c74e9218fd4e2f1ced45e6d4141d Mon Sep 17 00:00:00 2001
From: Koval Dmitry
Date: Wed, 10 Apr 2024 18:54:05 +0300
Subject: [PATCH v1 1/2] Fix for SPLIT/MERGE partitions of temporary table
---
src/backend/commands/tablecmds.c | 11 ---
src/backend/parser/parse_utilcmd.c| 2 +-
src/test/regress/expected/partition_merge.out | 32 +--
src/test/regress/expected/partition_split.out | 29 +
src/test/regress/sql/partition_merge.sql | 24 +-
src/test/regress/sql/partition_split.sql | 23 +
6 files changed, 113 insertions(+), 8 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8a98a0af48..3da9f6389d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -21145,17 +21145,19 @@ moveSplitTableRows(Relation rel, Relation splitRel,
List *partlist, List *newPar
* createPartitionTable: create table for a new partition with given name
* (newPartName) like table (modelRelName)
*
- * Emulates command: CREATE TABLE (LIKE
+ * Emulates command: CREATE [TEMP] TABLE (LIKE
* INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY)
*/
static void
-createPartitionTable(RangeVar *newPartName, RangeVar *modelRelName,
+createPartitionTable(Relation rel, RangeVar *newPartName, RangeVar
*modelRelName,
AlterTableUtilityContext *context)
{
CreateStmt *createStmt;
TableLikeClause *tlc;
PlannedStmt *wrapper;
+ newPartName->relpersistence = rel->rd_rel->relpersistence;
+
createStmt = makeNode(CreateStmt);
createStmt->relation = newPartName;
createStmt->tableElts = NIL;
@@ -21291,7 +21293,7 @@ ATExecSplitPartition(List **wqueue, AlteredTableInfo
*tab, Relation rel,
SinglePartitionSpec *sps = (SinglePartitionSpec *)
lfirst(listptr);
RelationnewPartRel;
- createPartitionTable(sps->name, parentName, context);
+ createPartitionTable(rel, sps->name, parentName, context);
/* Open the new partition and acquire exclusive lock on it. */
newPartRel = table_openrv(sps->name, AccessExclusiveLock);
@@ -21491,7 +21493,8 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
mergePartName =
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
tmpRelName, -1);
}
- createPartitionTable(mergePartName,
+ createPartitionTable(rel,
+mergePartName,
makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
RelationGetRelationName(rel), -1),
context);
diff --git a/src/backend/parser/parse_utilcmd.c
b/src/backend/parser/parse_utilcmd.c
index ceba069905..ef1a2a97c0 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3430,7 +3430,7 @@ checkPartition(Relation rel, Oid partRelOid)
if (partRel->rd_rel->relkind != RELKIND_RELATION)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-errmsg("\"%s\" is not a table",
+errmsg("\"%s\" is not a ordinary table",
RelationGetRelationName(partRel;
if (!partRel->rd_rel->relispartition)
diff --git a/src/test/regress/expected/partition_merge.out
b/src/test/regress/expected/partition_merge.out
index 60eacf6bf3..c69a717aaa 100644
--- a/src/test/regress/expected/partition_merge.out
+++ b/src/test/regress/expected/partition_merge.out
@@ -25,9 +25,9 @@ ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022,
sales_mar2022, sales_fe
ERROR: partition with name "sales_feb2022" already used
LINE 1: ...e MERGE PARTITIONS (sales_feb2022, sales_mar2022, sales_feb2...
^
--- ERROR: "sales_apr2022" is not a table
+-- ERROR: "sales_apr2022" is not a ordinary table
ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022, sales_mar2022,
sales_apr2022) INTO
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
10.04.2024 12:00, Alexander Lakhin wrote: Hello Alexander and Dmitry, 10.04.2024 02:03, Alexander Korotkov wrote: Thank you. I've pushed this fix with minor corrections from me. Please look at another anomaly with MERGE. CREATE TEMP TABLE t (i int) PARTITION BY RANGE (i); CREATE TABLE tp_0_2 PARTITION OF t FOR VALUES FROM (0) TO (2); fails with ERROR: cannot create a permanent relation as partition of temporary relation "t" But CREATE TEMP TABLE t (i int) PARTITION BY RANGE (i); CREATE TEMP TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1); CREATE TEMP TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2); ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2; succeeds and we get: regression=# \d+ t* Partitioned table "pg_temp_1.t" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description +-+---+--+-+-+-+--+- i | integer | | | | plain | | | Partition key: RANGE (i) Partitions: tp_0_2 FOR VALUES FROM (0) TO (2) Table "public.tp_0_2" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description +-+---+--+-+-+-+--+- i | integer | | | | plain | | | Partition of: t FOR VALUES FROM (0) TO (2) Partition constraint: ((i IS NOT NULL) AND (i >= 0) AND (i < 2)) Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hello Alexander and Dmitry, 10.04.2024 02:03, Alexander Korotkov wrote: On Mon, Apr 8, 2024 at 11:43 PM Dmitry Koval wrote: Attached fix for the problems found by Alexander Lakhin. About grammar errors. Unfortunately, I don't know English well. Therefore, I plan (in the coming days) to show the text to specialists who perform technical translation of documentation. Thank you. I've pushed this fix with minor corrections from me. Thank you for fixing that defect! Please look at an error message emitted for foreign tables: CREATE TABLE t (i int) PARTITION BY RANGE (i); CREATE FOREIGN TABLE ftp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1) SERVER loopback OPTIONS (table_name 'lt_0_1'); CREATE FOREIGN TABLE ftp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2) SERVER loopback OPTIONS (table_name 'lt_1_2'); ALTER TABLE t MERGE PARTITIONS (ftp_0_1, ftp_1_2) INTO ftp_0_2; ERROR: "ftp_0_1" is not a table Shouldn't it be more correct/precise? Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Mon, Apr 8, 2024 at 11:43 PM Dmitry Koval wrote: > Attached fix for the problems found by Alexander Lakhin. > > About grammar errors. > Unfortunately, I don't know English well. > Therefore, I plan (in the coming days) to show the text to specialists > who perform technical translation of documentation. Thank you. I've pushed this fix with minor corrections from me. -- Regards, Alexander Korotkov
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi!
Attached fix for the problems found by Alexander Lakhin.
About grammar errors.
Unfortunately, I don't know English well.
Therefore, I plan (in the coming days) to show the text to specialists
who perform technical translation of documentation.
--
With best regards,
Dmitry Koval
Postgres Professional: http://postgrespro.comFrom 578b3fae50baffa3626570447d55ce4177fc6e7d Mon Sep 17 00:00:00 2001
From: Koval Dmitry
Date: Mon, 8 Apr 2024 23:10:52 +0300
Subject: [PATCH v1] Fixes for ALTER TABLE ... SPLIT/MERGE PARTITIONS ...
commands
---
doc/src/sgml/ddl.sgml | 2 +-
src/backend/commands/tablecmds.c | 12 --
src/backend/parser/parse_utilcmd.c| 38 +++
src/test/regress/expected/partition_merge.out | 29 +++---
src/test/regress/expected/partition_split.out | 13 +++
src/test/regress/sql/partition_merge.sql | 24 ++--
src/test/regress/sql/partition_split.sql | 15
7 files changed, 111 insertions(+), 22 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 8ff9a520ca..cd8304ef75 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -4410,7 +4410,7 @@ ALTER TABLE measurement
this operation is not supported for hash-partitioned tables and acquires
an ACCESS EXCLUSIVE lock, which could impact high-load
systems due to the lock's restrictive nature. For example, we can split
- the quarter partition back to monthly partitions:
+ the quarter partition back to monthly partitions:
ALTER TABLE measurement SPLIT PARTITION measurement_y2006q1 INTO
(PARTITION measurement_y2006m01 FOR VALUES FROM ('2006-01-01') TO
('2006-02-01'),
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 865c6331c1..8a98a0af48 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -21223,12 +21223,6 @@ ATExecSplitPartition(List **wqueue, AlteredTableInfo
*tab, Relation rel,
*/
splitRel = table_openrv(cmd->name, AccessExclusiveLock);
- if (splitRel->rd_rel->relkind != RELKIND_RELATION)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-errmsg("cannot split non-table partition
\"%s\"",
-
RelationGetRelationName(splitRel;
-
splitRelOid = RelationGetRelid(splitRel);
/* Check descriptions of new partitions. */
@@ -21463,12 +21457,6 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
*/
mergingPartition = table_openrv(name, AccessExclusiveLock);
- if (mergingPartition->rd_rel->relkind != RELKIND_RELATION)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-errmsg("cannot merge non-table
partition \"%s\"",
-
RelationGetRelationName(mergingPartition;
-
/*
* Checking that two partitions have the same name was before,
in
* function transformPartitionCmdForMerge().
diff --git a/src/backend/parser/parse_utilcmd.c
b/src/backend/parser/parse_utilcmd.c
index 9e3e14087f..0d5ed0079c 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -32,6 +32,7 @@
#include "catalog/heap.h"
#include "catalog/index.h"
#include "catalog/namespace.h"
+#include "catalog/partition.h"
#include "catalog/pg_am.h"
#include "catalog/pg_collation.h"
#include "catalog/pg_constraint.h"
@@ -3415,6 +3416,38 @@ transformRuleStmt(RuleStmt *stmt, const char
*queryString,
}
+/*
+ * checkPartition: check that partRelOid is partition of rel
+ */
+static void
+checkPartition(Relation rel, Oid partRelOid)
+{
+ RelationpartRel;
+
+ partRel = relation_open(partRelOid, AccessShareLock);
+
+ if (partRel->rd_rel->relkind != RELKIND_RELATION)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("\"%s\" is not a table",
+
RelationGetRelationName(partRel;
+
+ if (!partRel->rd_rel->relispartition)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("\"%s\" is not a partition",
+
RelationGetRelationName(partRel;
+
+ if (get_partition_parent(partRelOid, false) != RelationGetRelid(rel))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+errmsg("relation \"%s\" is not a partition of
relation \"%s\"",
+
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi Tender Wang, 08.04.2024 13:43, Tender Wang wrote: Hi all, I went through the MERGE/SPLIT partition codes today, thanks for the works. I found some grammar errors: i. in error messages(Users can see this grammar errors, not friendly). ii. in codes comments On a quick glance, I saw also: NULL-value partitionde splited temparary And a trailing whitespace at: the quarter partition back to monthly partitions: warning: 1 line adds whitespace errors. I'm also confused by "administrators" here: https://www.postgresql.org/docs/devel/ddl-partitioning.html (We can find on the same page, for instance: ... whereas table inheritance allows data to be divided in a manner of the user's choosing. It seems to me, that "users" should work for merging partitions as well.) Though the documentation addition requires more than just a quick glance, of course. Best regards, Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi all, I went through the MERGE/SPLIT partition codes today, thanks for the works. I found some grammar errors: i. in error messages(Users can see this grammar errors, not friendly). ii. in codes comments Alexander Korotkov 于2024年4月7日周日 06:23写道： > Hi, Dmitry! > > On Fri, Apr 5, 2024 at 4:00 PM Dmitry Koval > wrote: > > > I've revised the patchset. > > > > Thanks for the corrections (especially ddl.sgml). > > Could you also look at a small optimization for the MERGE PARTITIONS > > command (in a separate file > > v31-0003-Additional-patch-for-ALTER-TABLE-.-MERGE-PARTITI.patch, I wrote > > about it in an email 2024-03-31 00:56:50)? > > > > Files v31-0001-*.patch, v31-0002-*.patch are the same as > > v30-0001-*.patch, v30-0002-*.patch (after rebasing because patch stopped > > applying due to changes in upstream). > > I've pushed 0001 and 0002. I didn't push 0003 for the following reasons. > 1) This doesn't keep functionality equivalent to 0001. With 0003, the > merged partition will inherit indexes, constraints, and so on from the > one of merging partitions. > 2) This is not necessarily an optimization. Without 0003 indexes on > the merged partition are created after moving the rows in > attachPartitionTable(). With 0003 we merge data into the existing > partition which saves its indexes. That might cause a significant > performance loss because mass inserts into indexes may be much slower > than building indexes from scratch. > I think both aspects need to be carefully considered. Even if we > accept them, this needs to be documented. I think now it's too late > for both of these. So, this should wait for v18. > > -- > Regards, > Alexander Korotkov > > > -- Tender Wang OpenPie: https://en.openpie.com/ 0001-Fix-some-grammer-errors-from-error-messages-and-code.patch Description: Binary data
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Alexander Lakhin, thanks for the problems you found! Unfortunately I can't watch them immediately (event [1]). I will try to start solving them in 12-14 hours. [1] https://pgconf.ru/2024 -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
08.04.2024 01:15, Alexander Korotkov wrote:
Thank you for spotting this. This seems like a missing check. I'm
going to get a closer look at this tomorrow.
Thanks!
There is also an anomaly with the MERGE command:
CREATE TABLE t1 (i int, a int, b int, c int) PARTITION BY RANGE (a, b);
CREATE TABLE t1p1 PARTITION OF t1 FOR VALUES FROM (1, 1) TO (1, 2);
CREATE TABLE t2 (i int, t text) PARTITION BY RANGE (t);
CREATE TABLE t2pa PARTITION OF t2 FOR VALUES FROM ('A') TO ('C');
CREATE TABLE t3 (i int, t text);
ALTER TABLE t2 MERGE PARTITIONS (t1p1, t2pa, t3) INTO t2p;
leads to:
ERROR: partition bound for relation "t3" is null
WARNING: problem in alloc set PortalContext: detected write past chunk end in
block 0x55f1ef42f820, chunk 0x55f1ef42ff40
WARNING: problem in alloc set PortalContext: detected write past chunk end in
block 0x55f1ef42f820, chunk 0x55f1ef42ff40
(I'm also not sure that the error message is clear enough (can't we say
"relation X is not a partition of relation Y" in this context, as in
MarkInheritDetached(), for example?).)
Whilst with
ALTER TABLE t2 MERGE PARTITIONS (t1p1, t2pa) INTO t2p;
I get:
Program terminated with signal SIGSEGV, Segmentation fault.
#0 pg_detoast_datum_packed (datum=0x1) at fmgr.c:1866
1866 if (VARATT_IS_COMPRESSED(datum) || VARATT_IS_EXTERNAL(datum))
(gdb) bt
#0 pg_detoast_datum_packed (datum=0x1) at fmgr.c:1866
#1 0x55d77d00fde2 in bttextcmp (...) at
../../../../src/include/postgres.h:314
#2 0x55d77d03fa27 in FunctionCall2Coll (...) at fmgr.c:1161
#3 0x55d77ce1572f in partition_rbound_cmp (...) at partbounds.c:3525
#4 0x55d77ce157b9 in qsort_partition_rbound_cmp (...) at partbounds.c:3816
#5 0x55d77d0982ef in qsort_arg (...) at
../../src/include/lib/sort_template.h:316
#6 0x55d77ce1d109 in calculate_partition_bound_for_merge (...) at
partbounds.c:5786
#7 0x55d77cc24b2b in transformPartitionCmdForMerge (...) at
parse_utilcmd.c:3524
#8 0x55d77cc2b555 in transformAlterTableStmt (...) at parse_utilcmd.c:3812
#9 0x55d77ccab17c in ATParseTransformCmd (...) at tablecmds.c:5650
#10 0x55d77ccafd09 in ATExecCmd (...) at tablecmds.c:5589
...
Best regards,
Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi, Alexander!
On Sun, Apr 7, 2024 at 10:00 PM Alexander Lakhin wrote:
> 07.04.2024 01:22, Alexander Korotkov wrote:
> > I've pushed 0001 and 0002. I didn't push 0003 for the following reasons.
>
> Please try the following (erroneous) query:
> CREATE TABLE t1(i int, t text) PARTITION BY LIST (t);
> CREATE TABLE t1pa PARTITION OF t1 FOR VALUES IN ('A');
>
> CREATE TABLE t2 (i int, t text) PARTITION BY RANGE (t);
> ALTER TABLE t2 SPLIT PARTITION t1pa INTO
>(PARTITION t2a FOR VALUES FROM ('A') TO ('B'),
> PARTITION t2b FOR VALUES FROM ('B') TO ('C'));
>
> that triggers an assertion failure:
> TRAP: failed Assert("datums != NIL"), File: "partbounds.c", Line: 3434, PID:
> 1841459
>
> or a segfault (in a non-assert build):
> Program terminated with signal SIGSEGV, Segmentation fault.
>
> #0 pg_detoast_datum_packed (datum=0x0) at fmgr.c:1866
> 1866if (VARATT_IS_COMPRESSED(datum) || VARATT_IS_EXTERNAL(datum))
> (gdb) bt
> #0 pg_detoast_datum_packed (datum=0x0) at fmgr.c:1866
> #1 0x55f38c5d5e3f in bttextcmp (...) at varlena.c:1834
> #2 0x55f38c6030dd in FunctionCall2Coll (...) at fmgr.c:1161
> #3 0x55f38c417c83 in partition_rbound_cmp (...) at partbounds.c:3525
> #4 check_partition_bounds_for_split_range (...) at partbounds.c:5221
> #5 check_partitions_for_split (...) at partbounds.c:5688
> #6 0x55f38c256c49 in transformPartitionCmdForSplit (...) at
> parse_utilcmd.c:3451
> #7 transformAlterTableStmt (...) at parse_utilcmd.c:3810
> #8 0x55f38c2bdf9c in ATParseTransformCmd (...) at tablecmds.c:5650
Thank you for spotting this. This seems like a missing check. I'm
going to get a closer look at this tomorrow.
--
Regards,
Alexander Korotkov
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi Alexander and Dmitry,
07.04.2024 01:22, Alexander Korotkov wrote:
I've pushed 0001 and 0002. I didn't push 0003 for the following reasons.
Please try the following (erroneous) query:
CREATE TABLE t1(i int, t text) PARTITION BY LIST (t);
CREATE TABLE t1pa PARTITION OF t1 FOR VALUES IN ('A');
CREATE TABLE t2 (i int, t text) PARTITION BY RANGE (t);
ALTER TABLE t2 SPLIT PARTITION t1pa INTO
(PARTITION t2a FOR VALUES FROM ('A') TO ('B'),
PARTITION t2b FOR VALUES FROM ('B') TO ('C'));
that triggers an assertion failure:
TRAP: failed Assert("datums != NIL"), File: "partbounds.c", Line: 3434, PID:
1841459
or a segfault (in a non-assert build):
Program terminated with signal SIGSEGV, Segmentation fault.
#0 pg_detoast_datum_packed (datum=0x0) at fmgr.c:1866
1866 if (VARATT_IS_COMPRESSED(datum) || VARATT_IS_EXTERNAL(datum))
(gdb) bt
#0 pg_detoast_datum_packed (datum=0x0) at fmgr.c:1866
#1 0x55f38c5d5e3f in bttextcmp (...) at varlena.c:1834
#2 0x55f38c6030dd in FunctionCall2Coll (...) at fmgr.c:1161
#3 0x55f38c417c83 in partition_rbound_cmp (...) at partbounds.c:3525
#4 check_partition_bounds_for_split_range (...) at partbounds.c:5221
#5 check_partitions_for_split (...) at partbounds.c:5688
#6 0x55f38c256c49 in transformPartitionCmdForSplit (...) at
parse_utilcmd.c:3451
#7 transformAlterTableStmt (...) at parse_utilcmd.c:3810
#8 0x55f38c2bdf9c in ATParseTransformCmd (...) at tablecmds.c:5650
...
Best regards,
Alexander
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi, Alexander! I didn't push 0003 for the following reasons. Thanks for clarifying. You are right, these are serious reasons. -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi, Dmitry! On Fri, Apr 5, 2024 at 4:00 PM Dmitry Koval wrote: > > I've revised the patchset. > > Thanks for the corrections (especially ddl.sgml). > Could you also look at a small optimization for the MERGE PARTITIONS > command (in a separate file > v31-0003-Additional-patch-for-ALTER-TABLE-.-MERGE-PARTITI.patch, I wrote > about it in an email 2024-03-31 00:56:50)? > > Files v31-0001-*.patch, v31-0002-*.patch are the same as > v30-0001-*.patch, v30-0002-*.patch (after rebasing because patch stopped > applying due to changes in upstream). I've pushed 0001 and 0002. I didn't push 0003 for the following reasons. 1) This doesn't keep functionality equivalent to 0001. With 0003, the merged partition will inherit indexes, constraints, and so on from the one of merging partitions. 2) This is not necessarily an optimization. Without 0003 indexes on the merged partition are created after moving the rows in attachPartitionTable(). With 0003 we merge data into the existing partition which saves its indexes. That might cause a significant performance loss because mass inserts into indexes may be much slower than building indexes from scratch. I think both aspects need to be carefully considered. Even if we accept them, this needs to be documented. I think now it's too late for both of these. So, this should wait for v18. -- Regards, Alexander Korotkov
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed All three patches applied nivcely. Code fits standart, comments are relevant. The new status of this patch is: Ready for Committer
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi! > I've fixed that by skipping a copy of the identity of another > partition (remove CREATE_TABLE_LIKE_IDENTITY from > TableLikeClause.options). Thanks for correction! Probably I should have looked at the code more closely after commit [1]. I'm also very glad that situation [2] was reproduced. > When merging partitions you're creating a merged partition like the > parent table. But when splitting a partition you're creating new > partitions like the partition being split. What motivates this > difference? When splitting a partition, I planned to set parameters for each of the new partitions (for example, tablespace parameter). It would make sense if we want to transfer part of the data of splitting partition to a slower (archive) storage device. Right now I haven't seen any interest in this functionality, so it hasn't been implemented yet. But I think this will be needed in the future. Special thanks for the hint that new structures should be added to the list src\tools\pgindent\typedefs.list. Links. [1] https://github.com/postgres/postgres/commit/699586315704a8268808e3bdba4cb5924a038c49 [2] https://www.postgresql.org/message-id/171085360143.2046436.7217841141682511557.pgcf%40coridan.postgresql.org -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Tue, Sep 20, 2022 at 3:21 PM Robert Haas wrote: > On Mon, Sep 19, 2022 at 4:42 PM Dmitry Koval wrote: > > Thanks for comments and advice! > > I thought about this problem and discussed about it with colleagues. > > Unfortunately, I don't know of a good general solution. > > Yeah, me neither. > > > But for specific situation like this (certain partition is not changing) > > we can add CONCURRENTLY modifier. > > Our DDL query can be like > > > > ALTER TABLE...SPLIT PARTITION [CONCURRENTLY]; > > > > With CONCURRENTLY modifier we can lock partitioned table in > > ShareUpdateExclusiveLock mode and split partition - in > > AccessExclusiveLock mode. So we don't lock partitioned table in > > AccessExclusiveLock mode and can modify other partitions during SPLIT > > operation (except split partition). > > If smb try to modify split partition, he will receive error "relation > > does not exist" at end of operation (because split partition will be drop). > > I think that a built-in DDL command can't really assume that the user > won't modify anything. You'd have to take a ShareLock. > > But you might be able to have a CONCURRENTLY variant of the command > that does the same kind of multi-transaction thing as, e.g., CREATE > INDEX CONCURRENTLY. You would probably have to be quite careful about > race conditions (e.g. you commit the first transaction and before you > start the second one, someone drops or detaches the partition you were > planning to merge or split). Might take some thought, but feels > possibly doable. I've never been excited enough about this kind of > thing to want to put a lot of energy into engineering it, because > doing it "manually" feels so much nicer to me, and doubly so given > that we now have ATTACH CONCURRENTLY and DETACH CONCURRENTLY, but it > does seem like a thing some people would probably use and value. +1 Currently people are using external tools to implement this kind of task. However, having this functionality in core would be great. Implementing concurrent merge/split seems quite a difficult task, which needs careful design. It might be too hard to carry around the syntax altogether. So, I think having basic syntax in-core is a good step forward. But I think we need a clear notice in the documentation about the concurrency to avoid wrong user expectations. -- Regards, Alexander Korotkov
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi! The following review has been posted through the commitfest application: make installcheck-world: tested, passed Thanks for info! I was unable to reproduce the problem and I wanted to ask for clarification. But your message was ahead of my question. -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: not tested Spec compliant: not tested Documentation:not tested Sorry, tests passed when applying all patches. I planned to check without optimisation first. The new status of this patch is: Needs review
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation:not tested
Hi,
I have failing tap test after patches apply:
ok 201 + partition_merge 2635 ms
not ok 202 + partition_split 5719 ms
@@ -805,6 +805,7 @@
(PARTITION salesmans2_3 FOR VALUES FROM (2) TO (3),
PARTITION salesmans3_4 FOR VALUES FROM (3) TO (4),
PARTITION salesmans4_5 FOR VALUES FROM (4) TO (5));
+ERROR: no owned sequence found
INSERT INTO salesmans (salesman_name) VALUES ('May');
INSERT INTO salesmans (salesman_name) VALUES ('Ford');
SELECT * FROM salesmans1_2;
@@ -814,23 +815,17 @@
(1 row)
SELECT * FROM salesmans2_3;
- salesman_id | salesman_name
--+---
- 2 | Ivanov
-(1 row)
-
+ERROR: relation "salesmans2_3" does not exist
+LINE 1: SELECT * FROM salesmans2_3;
The new status of this patch is: Waiting on Author
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
> On 26 Jan 2024, at 23:36, Dmitry Koval wrote: > > The CF entry was in Ready for Committer state no so long ago. Stephane, you might want to review recent version after it was rebased on current HEAD. CFbot's test passed successfully. Thanks! Best regards, Andrey Borodin.
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On 2024-Jan-26, Alvaro Herrera wrote:
> On 2024-Jan-26, vignesh C wrote:
>
> > Please post an updated version for the same.
>
> Here's a rebase. I only fixed the conflicts, didn't review.
Hmm, but I got the attached regression.diffs with it. I didn't
investigate further, but it looks like the recent changes to replication
identity for partitioned tables has broken the regression tests.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"This is what I like so much about PostgreSQL. Most of the surprises
are of the "oh wow! That's cool" Not the "oh shit!" kind. :)"
Scott Marlowe, http://archives.postgresql.org/pgsql-admin/2008-10/msg00152.php
diff -U3 /pgsql/source/master/src/test/regress/expected/partition_split.out
/home/alvherre/Code/pgsql-build/master/src/test/regress/results/partition_split.out
--- /pgsql/source/master/src/test/regress/expected/partition_split.out
2024-01-26 14:57:39.549730792 +0100
+++
/home/alvherre/Code/pgsql-build/master/src/test/regress/results/partition_split.out
2024-01-26 15:22:15.007059433 +0100
@@ -777,8 +777,12 @@
-- Create new partition with identity-column:
CREATE TABLE salesmans2_5(salesman_id INT GENERATED ALWAYS AS IDENTITY PRIMARY
KEY, salesman_name VARCHAR(30));
ALTER TABLE salesmans ATTACH PARTITION salesmans2_5 FOR VALUES FROM (2) TO (5);
+ERROR: table "salesmans2_5" being attached contains an identity column
"salesman_id"
+DETAIL: The new partition may not contain an identity column.
INSERT INTO salesmans (salesman_name) VALUES ('Poirot');
INSERT INTO salesmans (salesman_name) VALUES ('Ivanov');
+ERROR: no partition of relation "salesmans" found for row
+DETAIL: Partition key of the failing row contains (salesman_id) = (2).
SELECT attname, attidentity, attgenerated FROM pg_attribute WHERE attnum > 0
AND attrelid = 'salesmans'::regclass::oid;
attname| attidentity | attgenerated
---+-+--
@@ -789,7 +793,7 @@
SELECT attname, attidentity, attgenerated FROM pg_attribute WHERE attnum > 0
AND attrelid = 'salesmans1_2'::regclass::oid;
attname| attidentity | attgenerated
---+-+--
- salesman_id | |
+ salesman_id | a |
salesman_name | |
(2 rows)
@@ -805,8 +809,13 @@
(PARTITION salesmans2_3 FOR VALUES FROM (2) TO (3),
PARTITION salesmans3_4 FOR VALUES FROM (3) TO (4),
PARTITION salesmans4_5 FOR VALUES FROM (4) TO (5));
+ERROR: partition bound for relation "salesmans2_5" is null
INSERT INTO salesmans (salesman_name) VALUES ('May');
+ERROR: no partition of relation "salesmans" found for row
+DETAIL: Partition key of the failing row contains (salesman_id) = (3).
INSERT INTO salesmans (salesman_name) VALUES ('Ford');
+ERROR: no partition of relation "salesmans" found for row
+DETAIL: Partition key of the failing row contains (salesman_id) = (4).
SELECT * FROM salesmans1_2;
salesman_id | salesman_name
-+---
@@ -814,23 +823,17 @@
(1 row)
SELECT * FROM salesmans2_3;
- salesman_id | salesman_name
--+---
- 2 | Ivanov
-(1 row)
-
+ERROR: relation "salesmans2_3" does not exist
+LINE 1: SELECT * FROM salesmans2_3;
+ ^
SELECT * FROM salesmans3_4;
- salesman_id | salesman_name
--+---
- 3 | May
-(1 row)
-
+ERROR: relation "salesmans3_4" does not exist
+LINE 1: SELECT * FROM salesmans3_4;
+ ^
SELECT * FROM salesmans4_5;
- salesman_id | salesman_name
--+---
- 4 | Ford
-(1 row)
-
+ERROR: relation "salesmans4_5" does not exist
+LINE 1: SELECT * FROM salesmans4_5;
+ ^
SELECT attname, attidentity, attgenerated FROM pg_attribute WHERE attnum > 0
AND attrelid = 'salesmans'::regclass::oid;
attname| attidentity | attgenerated
---+-+--
@@ -841,32 +844,23 @@
SELECT attname, attidentity, attgenerated FROM pg_attribute WHERE attnum > 0
AND attrelid = 'salesmans1_2'::regclass::oid;
attname| attidentity | attgenerated
---+-+--
- salesman_id | |
+ salesman_id | a |
salesman_name | |
(2 rows)
-- New partitions have identity-columns:
SELECT attname, attidentity, attgenerated FROM pg_attribute WHERE attnum > 0
AND attrelid = 'salesmans2_3'::regclass::oid;
-attname| attidentity | attgenerated
+-+--
- salesman_id | a |
- salesman_name | |
-(2 rows)
-
+ERROR: relation "salesmans2_3" does not exist
+LINE 1: ...FROM pg_attribute WHERE attnum > 0 AND attrelid = 'salesmans...
+ ^
SELECT attname, attidentity, attgenerated FROM pg_attribute WHERE attnum > 0
AND attrelid = 'salesmans3_4'::regclass::oid;
-attname|
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Mon, 4 Dec 2023 at 13:22, Dmitry Koval wrote: > > Hello! > > Added commit v21-0004-SPLIT-PARTITION-optimization.patch. CFBot shows that the patch does not apply anymore as in [1]: === Applying patches on top of PostgreSQL commit ID 8ba6fdf905d0f5aef70ced4504c6ad297bfe08ea === === applying patch ./v21-0001-ALTER-TABLE-MERGE-PARTITIONS-command.patch patching file src/backend/commands/tablecmds.c ... Hunk #7 FAILED at 18735. Hunk #8 succeeded at 20608 (offset 315 lines). 1 out of 8 hunks FAILED -- saving rejects to file src/backend/commands/tablecmds.c.rej patching file src/backend/parser/gram.y Please post an updated version for the same. [1] - http://cfbot.cputube.org/patch_46_3659.log Regards, Vignesh
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed It is just a rebase I check with make and meson run manual split and merge on list and range partition Doc fits The new status of this patch is: Ready for Committer
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested Only documentation patch applied on 4e465aac36ce9a9533c68dbdc83e67579880e628 Checked with v18 The new status of this patch is: Waiting on Author
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
This patch no longer applies to master, please submit a rebased version to the thread. I've marked the CF entry as waiting for author in the meantime. -- Daniel Gustafsson
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, failed Hi, Just a minor warning with documentation patch git apply ../v16-0003-Documentation-for-ALTER-TABLE-SPLIT-PARTITION-ME.patch ../v16-0003-Documentation-for-ALTER-TABLE-SPLIT-PARTITION-ME.patch:54: trailing whitespace. One of the new partitions partition_name1, warning: 1 ligne a ajouté des erreurs d'espace. (perhaps due to my Ubuntu 22.04.2 french install) Everything else is ok. Thanks a lot for your work Stéphane
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi, Patch v15-0001-ALTER-TABLE-MERGE-PARTITIONS-command.patch Apply nicely. One warning on meson compile (configure -Dssl=openssl -Dldap=enabled -Dauto_features=enabled -DPG_TEST_EXTRA='ssl,ldap,kerberos' -Dbsd_auth=disabled -Dbonjour=disabled -Dpam=disabled -Dpltcl=disabled -Dsystemd=disabled -Dzstd=disabled -Db_coverage=true) ../../src/pgmergesplit/src/test/modules/test_ddl_deparse/test_ddl_deparse.c: In function ‘get_altertable_subcmdinfo’: ../../src/pgmergesplit/src/test/modules/test_ddl_deparse/test_ddl_deparse.c:112:17: warning: enumeration value ‘AT_MergePartitions’ not handled in switch [-Wswitch] 112 | switch (subcmd->subtype) | ^~ Should be the same with 0002... meson test perfect, patch coverage is very good. Patch v15-0002-ALTER-TABLE-SPLIT-PARTITION-command.patch Doesn't apply on 326a33a289c7ba2dbf45f17e610b7be98dc11f67 Patch v15-0003-Documentation-for-ALTER-TABLE-SPLIT-PARTITION-ME.patch Apply with one warning 1 line add space error (translate from french "warning: 1 ligne a ajouté des erreurs d'espace"). v15-0003-Documentation-for-ALTER-TABLE-SPLIT-PARTITION-ME.patch:54: trailing whitespace. One of the new partitions partition_name1, Comment are ok for me. A non native english speaker. Perhaps you could add some remarks in ddl.html and alter-ddl.html Stéphane The new status of this patch is: Waiting on Author
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, failed Feature is clearly missing with partition handling in PostgreSQL, so, this patch is very welcome (as are futur steps) Code presents good, comments are explicit Patch v14 apply nicely on 4f46f870fa56fa73d6678273f1bd059fdd93d5e6 Compilation ok with meson compile LCOV after meson test shows good new code coverage. Documentation is missing in v14.
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Tue, Oct 11, 2022 at 9:58 AM Zhihong Yu wrote: > > > On Tue, Oct 11, 2022 at 9:22 AM Dmitry Koval > wrote: > >> Hi! >> >> Fixed couple warnings (for cfbot). >> >> -- >> With best regards, >> Dmitry Koval >> >> Postgres Professional: http://postgrespro.com > > Hi, > For v12-0001-PGPRO-ALTER-TABLE-MERGE-PARTITIONS-command.patch: > > + if (equal(name, cmd->name)) > + /* One new partition can have the same name as merged > partition. */ > + isSameName = true; > > I think there should be a check before assigning true to isSameName - if > isSameName is true, that means there are two partitions with this same name. > > Cheers > Pardon - I see that transformPartitionCmdForMerge() compares the partition names. Maybe you can add a comment in ATExecMergePartitions referring to transformPartitionCmdForMerge() so that people can more easily understand the logic.
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Tue, Oct 11, 2022 at 9:22 AM Dmitry Koval wrote: > Hi! > > Fixed couple warnings (for cfbot). > > -- > With best regards, > Dmitry Koval > > Postgres Professional: http://postgrespro.com Hi, For v12-0001-PGPRO-ALTER-TABLE-MERGE-PARTITIONS-command.patch: + if (equal(name, cmd->name)) + /* One new partition can have the same name as merged partition. */ + isSameName = true; I think there should be a check before assigning true to isSameName - if isSameName is true, that means there are two partitions with this same name. Cheers
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Mon, Sep 19, 2022 at 4:42 PM Dmitry Koval wrote: > Thanks for comments and advice! > I thought about this problem and discussed about it with colleagues. > Unfortunately, I don't know of a good general solution. Yeah, me neither. > But for specific situation like this (certain partition is not changing) > we can add CONCURRENTLY modifier. > Our DDL query can be like > > ALTER TABLE...SPLIT PARTITION [CONCURRENTLY]; > > With CONCURRENTLY modifier we can lock partitioned table in > ShareUpdateExclusiveLock mode and split partition - in > AccessExclusiveLock mode. So we don't lock partitioned table in > AccessExclusiveLock mode and can modify other partitions during SPLIT > operation (except split partition). > If smb try to modify split partition, he will receive error "relation > does not exist" at end of operation (because split partition will be drop). I think that a built-in DDL command can't really assume that the user won't modify anything. You'd have to take a ShareLock. But you might be able to have a CONCURRENTLY variant of the command that does the same kind of multi-transaction thing as, e.g., CREATE INDEX CONCURRENTLY. You would probably have to be quite careful about race conditions (e.g. you commit the first transaction and before you start the second one, someone drops or detaches the partition you were planning to merge or split). Might take some thought, but feels possibly doable. I've never been excited enough about this kind of thing to want to put a lot of energy into engineering it, because doing it "manually" feels so much nicer to me, and doubly so given that we now have ATTACH CONCURRENTLY and DETACH CONCURRENTLY, but it does seem like a thing some people would probably use and value. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Thanks for comments and advice! I thought about this problem and discussed about it with colleagues. Unfortunately, I don't know of a good general solution. 19.09.2022 22:56, Robert Haas пишет: If you know that a certain partition is not changing, and you would like to split it, you can create two or more new standalone tables and populate them from the original partition using INSERT .. SELECT. Then you can BEGIN a transaction, DETACH the existing partitions, and ATTACH the replacement ones. By doing this, you take an ACCESS EXCLUSIVE lock on the partitioned table only for a brief period. The same kind of idea can be used to merge partitions. But for specific situation like this (certain partition is not changing) we can add CONCURRENTLY modifier. Our DDL query can be like ALTER TABLE...SPLIT PARTITION [CONCURRENTLY]; With CONCURRENTLY modifier we can lock partitioned table in ShareUpdateExclusiveLock mode and split partition - in AccessExclusiveLock mode. So we don't lock partitioned table in AccessExclusiveLock mode and can modify other partitions during SPLIT operation (except split partition). If smb try to modify split partition, he will receive error "relation does not exist" at end of operation (because split partition will be drop). -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Tue, May 31, 2022 at 5:33 AM Dmitry Koval wrote: > There are not many commands in PostgreSQL for working with partitioned > tables. This is an obstacle to their widespread use. > Adding SPLIT PARTITION/MERGE PARTITIONS operations can make easier to > use partitioned tables in PostgreSQL. > (This is especially important when migrating projects from ORACLE DBMS.) > > SPLIT PARTITION/MERGE PARTITIONS commands are supported for range > partitioning (BY RANGE) and for list partitioning (BY LIST). > For hash partitioning (BY HASH) these operations are not supported. This may be a good idea, but I would like to point out one disadvantage of this approach. If you know that a certain partition is not changing, and you would like to split it, you can create two or more new standalone tables and populate them from the original partition using INSERT .. SELECT. Then you can BEGIN a transaction, DETACH the existing partitions, and ATTACH the replacement ones. By doing this, you take an ACCESS EXCLUSIVE lock on the partitioned table only for a brief period. The same kind of idea can be used to merge partitions. It seems hard to do something comparable with built-in DDL for SPLIT PARTITION and MERGE PARTITION. You could start by taking e.g. SHARE lock on the existing partition(s) and then wait until the end to take ACCESS EXCLUSIVE lock on the partitions, but we typically avoid such coding patterns, because the lock upgrade might deadlock and then a lot of work would be wasted. So most likely with the approach you propose here you will end up acquiring ACCESS EXCLUSIVE lock at the beginning of the operation and then shuffle a lot of data around while still holding it, which is pretty painful. Because of this problem, I find it hard to believe that these commands would get much use, except perhaps on small tables or in non-production environments, unless people just didn't know about the alternatives. That's not to say that something like this has no value. As a convenience feature, it's fine. It's just hard for me to see it as any more than that. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Thanks for your advice, Justin and Alvaro! I'll try to reduce the size of this patch and split it into separate parts (for MERGE and SPLIT). -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.com
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On 2022-Sep-08, Justin Pryzby wrote: > If the patch were split into separate parts for MERGE and SPLIT, would > the first patch be significantly smaller than the existing patch > (hopefully half as big) ? That would help to review it, even if both > halves were ultimately squished together. (An easy way to do this is to > open up all the files in separate editor instances, trim out the parts > that aren't needed for the first patch, save the files but don't quit > the editors, test compilation and regression tests, then git commit > --amend -a. Then in each editor, "undo" all the trimmed changes, save, > and git commit -a). An easier (IMO) way to do that is to use "git gui" or even "git add -p", which allow you to selectively add changed lines/hunks to the index. You add a few, commit, then add the rest, commit again. With "git add -p" you can even edit individual hunks in an editor in case you have a mix of both wanted and unwanted in a single hunk (after "s"plitting, of course), which turns out to be easier than it sounds. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "El sudor es la mejor cura para un pensamiento enfermo" (Bardia)
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Thu, Sep 08, 2022 at 02:35:24PM +0300, Dmitry Koval wrote: > Thanks a lot Justin! > > After compilation PostgreSQL+patch with macros > RELCACHE_FORCE_RELEASE, > RANDOMIZE_ALLOCATED_MEMORY, > I saw a problem on Windows 10, MSVC2019. Yes, it passes tests on my CI improvements branch. https://github.com/justinpryzby/postgres/runs/8248668269 Thanks to Alexander Pyhalov for reminding me about RELCACHE_FORCE_RELEASE last year ;) On Tue, May 31, 2022 at 12:32:43PM +0300, Dmitry Koval wrote: > This can be useful for this example cases: > need to merge all one-day partitions > into a month partition. +1, we would use this (at least the MERGE half). I wonder if it's possible to reduce the size of this patch (I'm starting to try to absorb it). Is there a way to refactor/reuse existing code to reduce its footprint ? partbounds.c is adding 500+ LOC about checking if proposed partitions meet the requirements (don't overlap, etc). But a lot of those checks must already happen, no? Can you re-use/refactor the existing checks ? An UPDATE on a partitioned table will move tuples from one partition to another. Is there a way to re-use that ? Also, postgres already supports concurrent DDL (CREATE+ATTACH and DETACH CONCURRENTLY). Is it possible to leverage that ? (Mostly to reduce the patch size, but also because maybe some cases could be concurrent?). If the patch were split into separate parts for MERGE and SPLIT, would the first patch be significantly smaller than the existing patch (hopefully half as big) ? That would help to review it, even if both halves were ultimately squished together. (An easy way to do this is to open up all the files in separate editor instances, trim out the parts that aren't needed for the first patch, save the files but don't quit the editors, test compilation and regression tests, then git commit --amend -a. Then in each editor, "undo" all the trimmed changes, save, and git commit -a). Would it save much code if "default" partitions weren't handled in the first patch ? -- Justin
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Wed, Sep 07, 2022 at 08:03:09PM +0300, Dmitry Koval wrote: > Hi! > > Patch stop applying due to changes in upstream. > Here is a rebased version. This crashes on freebsd with -DRELCACHE_FORCE_RELEASE https://cirrus-ci.com/task/6565371623768064 https://cirrus-ci.com/task/6145355992530944 Note that that's a modified cirrus script from my CI improvements branch which also does some extra/different things. -- Justin
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
This is not a review, but I think the isolation tests should be expanded. At least, include the case of serializable transactions being involved. Thanks! I will expand the tests for the next commitfest. -- With best regards, Dmitry Koval Postgres Professional: http://postgrespro.com
