public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alexander Korotkov <[email protected]>
To: Justin Pryzby <[email protected]>
Cc: Dmitry Koval <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Cc: [email protected]
Cc: Tomas Vondra <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Date: Fri, 17 May 2024 13:05:01 +0300
Message-ID: <CAPpHfdvz3S7SoRiaSk9C-Juy2EzHs6NC_c9uUR0i7bymKXkvuQ@mail.gmail.com> (raw)
In-Reply-To: <ZkN6EfjQGz8TOhbL@pryzbyj2023>
References: <[email protected]>
<[email protected]>
<[email protected]>
<ZjFRSVnvvGrsdndU@pryzbyj2023>
<[email protected]>
<ZjTlQiBPYm707Rxv@pryzbyj2023>
<CAPpHfduhHm+4yeObsj9wK4iZvV2Ve5xcr6BwZDFiNBYbE1PfLg@mail.gmail.com>
<CAPpHfdtgLiT2d++-qLircLB8ATUfzgR=aTmRDdF7w=2E9yDHbQ@mail.gmail.com>
<Zjvwqs-GV0MjWIhU@pryzbyj2023>
<CAPpHfdvh_XYHahtpoxfzDkvYOFUH1F5tm3JTV=sOZt8M4CTBcA@mail.gmail.com>
<ZkN6EfjQGz8TOhbL@pryzbyj2023>
On Tue, May 14, 2024 at 5:49 PM Justin Pryzby <[email protected]> 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
Attachments:
[application/octet-stream] v2-0001-Fix-the-name-collision-detection-in-MERGE-SPLIT-p.patch (8.4K, ../CAPpHfdvz3S7SoRiaSk9C-Juy2EzHs6NC_c9uUR0i7bymKXkvuQ@mail.gmail.com/2-v2-0001-Fix-the-name-collision-detection-in-MERGE-SPLIT-p.patch)
download | inline diff:
From eb21dcdba3263bc4702567b0af18e3ba32c4a280 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 17 May 2024 12:44:09 +0300
Subject: [PATCH v2 1/2] Fix the name collision detection in MERGE/SPLIT
partition operations
Both MERGE and SPLIT partition operations support the case when the name of the
new partition matches the name of the existing partition to be merged/split.
But the name collision detection doesn't always work as intended. The SPLIT
partition operation finds the namespace to search for an existing partition
without taking into account the parent's persistence. The MERGE partition
operation checks for the name collision with simple equal() on RangeVar's
simply ignoring the search_path.
This commit fixes this behavior as follows.
1. The SPLIT partition operation now finds the namespace to search for
an existing partition according to the parent's persistence.
2. The MERGE partition operation now checks for the name collision similarly
to the SPLIT partition operation using
RangeVarGetAndCheckCreationNamespace() and get_relname_relid().
Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/86b4f1e3-0b5d-315c-9225-19860d64d685%40gmail.com
Author: Dmitry Koval, Alexander Korotkov
---
src/backend/commands/tablecmds.c | 62 +++++++++++++++----
src/test/regress/expected/partition_merge.out | 3 +-
src/test/regress/expected/partition_split.out | 8 +++
src/test/regress/sql/partition_merge.sql | 3 +-
src/test/regress/sql/partition_split.sql | 9 +++
5 files changed, 72 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 313c782cae2..7a063ca8ae0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -20409,6 +20409,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);
@@ -20601,6 +20602,8 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
ListCell *listptr;
List *mergingPartitionsList = NIL;
Oid defaultPartOid;
+ Oid namespaceId;
+ Oid existingRelid;
/*
* Lock all merged partitions, check them and create list with partitions
@@ -20617,13 +20620,48 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
*/
mergingPartition = table_openrv(name, AccessExclusiveLock);
- /*
- * Checking that two partitions have the same name was before, in
- * function transformPartitionCmdForMerge().
- */
- if (equal(name, cmd->name))
+ /* Store a next merging partition into the list. */
+ mergingPartitionsList = lappend(mergingPartitionsList,
+ mergingPartition);
+ }
+
+ /*
+ * Look up the namespace in which we are supposed to create the partition,
+ * check we have permission to create there, lock it against concurrent
+ * drop, and mark stmt->relation as RELPERSISTENCE_TEMP if a temporary
+ * namespace is selected.
+ */
+ cmd->name->relpersistence = rel->rd_rel->relpersistence;
+ namespaceId =
+ RangeVarGetAndCheckCreationNamespace(cmd->name, NoLock, NULL);
+
+ /*
+ * Check if this name is already taken. This helps us to detect the
+ * situation when one of the merging partitions has the same name as the
+ * new partition. Otherwise, this would fail later on anyway but catching
+ * this here allows us to emit a nicer error message.
+ */
+ existingRelid = get_relname_relid(cmd->name->relname, namespaceId);
+
+ if (OidIsValid(existingRelid))
+ {
+ Relation sameNamePartition = NULL;
+
+ foreach_ptr(RelationData, mergingPartition, mergingPartitionsList)
{
- /* One new partition can have the same name as merged partition. */
+ if (RelationGetRelid(mergingPartition) == existingRelid)
+ {
+ sameNamePartition = mergingPartition;
+ break;
+ }
+ }
+
+ if (sameNamePartition)
+ {
+ /*
+ * The new partition has the same name as one of merging
+ * partitions.
+ */
char tmpRelName[NAMEDATALEN];
/* Generate temporary name. */
@@ -20635,7 +20673,7 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
* in the future because we're going to eventually drop the
* existing partition anyway.
*/
- RenameRelationInternal(RelationGetRelid(mergingPartition),
+ RenameRelationInternal(RelationGetRelid(sameNamePartition),
tmpRelName, false, false);
/*
@@ -20644,10 +20682,12 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
*/
CommandCounterIncrement();
}
-
- /* Store a next merging partition into the list. */
- mergingPartitionsList = lappend(mergingPartitionsList,
- mergingPartition);
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_TABLE),
+ errmsg("relation \"%s\" already exists", cmd->name->relname)));
+ }
}
/* Detach all merged partitions. */
diff --git a/src/test/regress/expected/partition_merge.out b/src/test/regress/expected/partition_merge.out
index 6361732d104..9c67a4a8b15 100644
--- a/src/test/regress/expected/partition_merge.out
+++ b/src/test/regress/expected/partition_merge.out
@@ -214,7 +214,8 @@ INSERT INTO sales_range VALUES (13, 'Gandi', 377, '2022-01-09');
INSERT INTO sales_range VALUES (14, 'Smith', 510, '2022-05-04');
-- Merge partitions (include DEFAULT partition) into partition with the same
-- name
-ALTER TABLE sales_range MERGE PARTITIONS (sales_jan2022, sales_mar2022, sales_others) INTO sales_others;
+ALTER TABLE sales_range MERGE PARTITIONS
+ (sales_jan2022, sales_mar2022, partitions_merge_schema.sales_others) INTO sales_others;
select * from sales_others;
salesperson_id | salesperson_name | sales_amount | sales_date
----------------+------------------+--------------+------------
diff --git a/src/test/regress/expected/partition_split.out b/src/test/regress/expected/partition_split.out
index 14c4f97c9ff..6369f06b841 100644
--- a/src/test/regress/expected/partition_split.out
+++ b/src/test/regress/expected/partition_split.out
@@ -1547,6 +1547,14 @@ REVOKE ALL ON SCHEMA partition_split_schema FROM regress_partition_split_alice;
REVOKE ALL ON SCHEMA partition_split_schema FROM regress_partition_split_bob;
DROP ROLE regress_partition_split_alice;
DROP ROLE regress_partition_split_bob;
+-- Check that detection, that the new partition has the same name as one of
+-- the merged partitions, works correctly for temporary partitions
+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;
RESET search_path;
--
DROP SCHEMA partition_split_schema;
diff --git a/src/test/regress/sql/partition_merge.sql b/src/test/regress/sql/partition_merge.sql
index 5a741efa09b..56249732002 100644
--- a/src/test/regress/sql/partition_merge.sql
+++ b/src/test/regress/sql/partition_merge.sql
@@ -140,7 +140,8 @@ INSERT INTO sales_range VALUES (14, 'Smith', 510, '2022-05-04');
-- Merge partitions (include DEFAULT partition) into partition with the same
-- name
-ALTER TABLE sales_range MERGE PARTITIONS (sales_jan2022, sales_mar2022, sales_others) INTO sales_others;
+ALTER TABLE sales_range MERGE PARTITIONS
+ (sales_jan2022, sales_mar2022, partitions_merge_schema.sales_others) INTO sales_others;
select * from sales_others;
diff --git a/src/test/regress/sql/partition_split.sql b/src/test/regress/sql/partition_split.sql
index 70d70499ec6..67ed274f529 100644
--- a/src/test/regress/sql/partition_split.sql
+++ b/src/test/regress/sql/partition_split.sql
@@ -931,6 +931,15 @@ REVOKE ALL ON SCHEMA partition_split_schema FROM regress_partition_split_bob;
DROP ROLE regress_partition_split_alice;
DROP ROLE regress_partition_split_bob;
+-- Check that detection, that the new partition has the same name as one of
+-- the merged partitions, works correctly for temporary partitions
+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;
+
RESET search_path;
--
--
2.39.3 (Apple Git-145)
[application/octet-stream] v2-0002-Don-t-copy-extended-statistics-during-MERGE-SPLIT.patch (4.9K, ../CAPpHfdvz3S7SoRiaSk9C-Juy2EzHs6NC_c9uUR0i7bymKXkvuQ@mail.gmail.com/3-v2-0002-Don-t-copy-extended-statistics-during-MERGE-SPLIT.patch)
download | inline diff:
From a544336aabe5daee7e2124927de017644a01bc32 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 17 May 2024 12:56:46 +0300
Subject: [PATCH v2 2/2] 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
Author: Alexander Korotkov, Justin Pryzby
---
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 5d352abf991..c062a36880d 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1154,9 +1154,12 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
</para>
<para>
The new partitions will be created the same as tables created with the
- SQL command <literal>CREATE TABLE <replaceable class="parameter">partition_nameN</replaceable> (LIKE <replaceable class="parameter">name</replaceable> INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY)</literal>.
+ SQL command <literal>CREATE TABLE <replaceable class="parameter">partition_nameN</replaceable> (LIKE <replaceable class="parameter">name</replaceable> INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY EXCLUDING STATISTICS)</literal>.
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
+ <command>CREATE TABLE PARTITION OF</command>.
+
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 <replaceable class="parameter">numeric_literal</replaceable>, REM
</para>
<para>
The new partition will be created the same as a table created with the
- SQL command <literal>CREATE TABLE <replaceable class="parameter">partition_name</replaceable> (LIKE <replaceable class="parameter">name</replaceable> INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY)</literal>.
+ SQL command <literal>CREATE TABLE <replaceable class="parameter">partition_name</replaceable> (LIKE <replaceable class="parameter">name</replaceable> INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY EXCLUDING STATISTICS)</literal>.
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
+ <command>CREATE TABLE PARTITION OF</command>.
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 7a063ca8ae0..7b6c69b7a52 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -20269,7 +20269,7 @@ moveSplitTableRows(Relation rel, Relation splitRel, List *partlist, List *newPar
* (newPartName) like table (modelRel)
*
* Emulates command: CREATE [TEMP] TABLE <newPartName> (LIKE <modelRel's name>
- * 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
@@ -20313,9 +20313,11 @@ createPartitionTable(RangeVar *newPartName, Relation modelRel,
/*
* Indexes will be inherited on "attach new partitions" stage, after data
- * moving.
+ * moving. We also don't copy the extended statistics for consistency
+ * with CREATE TABLE PARTITION OF.
*/
- tlc->options = CREATE_TABLE_LIKE_ALL & ~(CREATE_TABLE_LIKE_INDEXES | CREATE_TABLE_LIKE_IDENTITY);
+ tlc->options = CREATE_TABLE_LIKE_ALL &
+ ~(CREATE_TABLE_LIKE_INDEXES | CREATE_TABLE_LIKE_IDENTITY | CREATE_TABLE_LIKE_STATISTICS);
tlc->relationOid = InvalidOid;
createStmt->tableElts = lappend(createStmt->tableElts, tlc);
--
2.39.3 (Apple Git-145)
view thread (33+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
In-Reply-To: <CAPpHfdvz3S7SoRiaSk9C-Juy2EzHs6NC_c9uUR0i7bymKXkvuQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox