public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alexander Korotkov <[email protected]>
To: Chao Li <[email protected]>
Cc: Dmitry Koval <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Fix SPLIT PARTITION bound-overlap bug and other improvements
Date: Thu, 21 May 2026 00:17:20 +0300
Message-ID: <CAPpHfdtRNFwVBrekh_b6Pg5fmi+QCoyEdAs7h=ikxXoDcOMdHA@mail.gmail.com> (raw)
In-Reply-To: <CAPpHfdvht=_VcpDybYvwon96Enh=-S-9wGLGPzbu+6_5QmwmwQ@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<CAPpHfdvdevTMpjQe09aWdK_nbrE4+M1z09H2SUe3bgxY3_QUdA@mail.gmail.com>
<[email protected]>
<CAPpHfdsLnD3NGnPC32oeg4ZWWX441M=SfH0-7bG652G40TpEEQ@mail.gmail.com>
<[email protected]>
<CAPpHfduMLJkOSguaKhm4PEWRYHqqvFDX6bu5+E_LSxuy5YjPEw@mail.gmail.com>
<[email protected]>
<CAPpHfdvn=hPrtMjtvJU-7=HR69ZWDvy2pTE9Prau-dgp5Ua0Jg@mail.gmail.com>
<[email protected]>
<CAPpHfduDPA1vqhrSgLwmGY5+wVsrmK2AKsPQ2rZa_=QAn-y_XQ@mail.gmail.com>
<[email protected]>
<CAPpHfdvqk=nC-_wdK4o5=LyWD53FvJDdH7RSk7cZuWgx-qRCQg@mail.gmail.com>
<CAPpHfdvht=_VcpDybYvwon96Enh=-S-9wGLGPzbu+6_5QmwmwQ@mail.gmail.com>
On Wed, May 20, 2026 at 11:29 PM Alexander Korotkov
<[email protected]> wrote:
> On Wed, May 20, 2026 at 2:46 PM Alexander Korotkov <[email protected]> wrote:
> > On Wed, May 20, 2026 at 9:29 AM Chao Li <[email protected]> wrote:
> > > > On May 20, 2026, at 14:19, Alexander Korotkov <[email protected]> wrote:
> > > > On Wed, May 20, 2026 at 2:37 AM Chao Li <[email protected]> wrote:
> > > >>> On May 19, 2026, at 19:00, Alexander Korotkov <[email protected]> wrote:
> > > >>> On Tue, May 19, 2026 at 5:50 AM Chao Li <[email protected]> wrote:
> > > >>>>> On May 18, 2026, at 20:04, Alexander Korotkov <[email protected]> wrote:
> > > >>>>>
> > > >>>>> On Mon, May 18, 2026 at 2:57 PM Chao Li <[email protected]> wrote:
> > > >>>>>>> <v3-0003-Clarify-SPLIT-PARTITION-bound-requirements-in-doc.patch><v3-0001-Fix-SPLIT-PARTITION-range-bound-validation-with-D.patch><v3-0002-Fix-SPLIT-PARTITION-hint-for-DEFAULT-partition-bo.patch><v3-0004-Reject-degenerate-SPLIT-PARTITION-with-DEFAULT-pa.patch>
> > > >>>>>>
> > > >>>>>> v3-0001 through v3-0003 look good to me.
> > > >>>>>>
> > > >>>>>> For v3-0004, I have a suspicion, but it's late here and my brain is getting slow, so I would like to study it more tomorrow.
> > > >>>>>
> > > >>>>> Sure, take your time.
> > > >>>>>
> > > >>>>> ------
> > > >>>>> Regards,
> > > >>>>> Alexander Korotkov
> > > >>>>> Supabase
> > > >>>>
> > > >>>> My suspicion was that check_split_partition_not_same_bound() now has two paths. The RANGE path honors collation, while the LIST path does not. So I spent some time creating a test that uses a case-insensitive collation:
> > > >>>> ```
> > > >>>> evantest=# create collation case_insensitive (provider=icu, locale='und-u-ks-level2', deterministic = false);
> > > >>>> CREATE COLLATION
> > > >>>> evantest=# create table t (b text collate case_insensitive) partition by list (b);
> > > >>>> CREATE TABLE
> > > >>>> evantest=# create table tp_ab partition of t for values in ('a', 'b');
> > > >>>> CREATE TABLE
> > > >>>> evantest=# alter table t split partition tp_ab into
> > > >>>> evantest-# (partition tp_a for values in ('a', 'A'),
> > > >>>> evantest(# partition tp_default default);
> > > >>>> ERROR: cannot split partition "tp_ab" only to add a DEFAULT partition
> > > >>>> LINE 2: (partition tp_a for values in ('a', 'A'),
> > > >>>> ^
> > > >>>> DETAIL: The non-DEFAULT partition would keep the same partition bound.
> > > >>>> HINT: Use CREATE TABLE ... PARTITION OF ... DEFAULT to add a DEFAULT partition.
> > > >>>> ```
> > > >>>>
> > > >>>> In this test, the split partition’s bound is ('a', 'b'), and the new partition’s bound is ('a', 'A'). Their list lengths are both 2, but the two bounds are actually different, because 'a' and 'A' are considered equal by the collation.
> > > >>>>
> > > >>>> So, in the LIST path, since check_partition_bounds_for_split_list() has already ensured that the new partition’s bound is contained within the split partition’s bound, we need to check the reverse direction as well. Whether the split partition’s bound is also contained in the new partition’s bound. If yes, the two bounds are identical.
> > > >>>>
> > > >>>> See the attached v4 for my changes for 0004. 0001-0003 are unchanged. Since 0001 and 0003 are independent of 0004, maybe they can be pushed first.
> > > >>>
> > > >>> I've pushed 0001-0003.
> > > >>
> > > >> Thanks for pushing them.
> > > >>
> > > >>> Thank you for discovering the collation issue
> > > >>> in 0004. Note that original approach of using
> > > >>> partition_bounds_equal() can't handle different collations too (as it
> > > >>> internally uses datumIsEqual()).
> > > >>
> > > >> Yes, I realized that while reviewing v3. That’s reason I didn’t get back v2 and only worked again based on v3.
> > > >>
> > > >>> I've revised the remaining patch:
> > > >>> made function header comment a bit more detailed
> > > >>
> > > >> This part looks good to me.
> > > >>
> > > >>> and added additional
> > > >>> regression tests. Please, check.
> > > >>>
> > > >>
> > > >> But I don’t see any change for regression test between v4 and v5. Maybe you forgot to save your changes?
> > > >
> > > > Sorry, I just mess up, no changes in tests.
> > > > I'm going to push this if no objection.
> > > >
> > >
> > > No worries. Then v5 looks good to me.
> >
> > Thank you, pushed.
>
> Uhhh, most of buildfarm animals don't support locales used in our
> tests. I've to revert that,
The another attempt is attached. Now use -0.0 and 0.0 as binary
different but logically equivalent values, no locale dependence.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v6-0001-Reject-degenerate-SPLIT-PARTITION-with-DEFAULT-pa.patch (12.7K, 2-v6-0001-Reject-degenerate-SPLIT-PARTITION-with-DEFAULT-pa.patch)
download | inline diff:
From 7d8d3e2fe5150837db3af1b59bb7e6922ca30618 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Mon, 18 May 2026 00:37:52 +0300
Subject: [PATCH v6] Reject degenerate SPLIT PARTITION with DEFAULT partition
ALTER TABLE ... SPLIT PARTITION allows a DEFAULT partition to be created
as one of the replacement partitions when the parent table does not
already have one. However, it should not allow the degenerate case where
a non-DEFAULT partition keeps exactly the same bound as the split
partition and the command merely adds a DEFAULT partition through the
SPLIT PARTITION path.
Detect that case by comparing the bound of the split partition with the
bound of the only non-DEFAULT replacement partition, and raise an error
when they are the same. Users should add a DEFAULT partition directly
with CREATE TABLE ... PARTITION OF ... DEFAULT or ALTER TABLE ... ATTACH
PARTITION ... DEFAULT instead.
The comparison goes through the partition operator family rather than
byte equality so that values which are binary-different but compare
equal under the partition key's comparator are treated as the same
bound. The corresponding regression test uses a float8 LIST partition
with -0.0 and 0.0 -- they have different bit patterns but are equal
under float8 -- to verify that a datumIsEqual()-based check would let
the degenerate split through while the partsupfunc-based check
correctly rejects it.
Author: Chao Li <[email protected]>
Reviewed-by: Alexander Korotkov <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/partitioning/partbounds.c | 150 ++++++++++++++++++
src/test/regress/expected/partition_split.out | 58 +++++++
src/test/regress/sql/partition_split.sql | 51 ++++++
3 files changed, 259 insertions(+)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 7d3580cbc10..6fb150a8763 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -5700,6 +5700,146 @@ check_parent_values_in_new_partitions(Relation parent,
}
}
+/*
+ * split_partition_values_contained_in_new_part
+ *
+ * (function for BY LIST partitioning)
+ *
+ * Returns true if all values in the LIST bound of the partition being split
+ * are contained in the specified non-DEFAULT replacement partition's bound.
+ *
+ * The caller must already have verified containment in the other direction,
+ * so this check is sufficient to prove that the two LIST bounds are equal.
+ */
+static bool
+split_partition_values_contained_in_new_part(Relation parent,
+ Oid splitPartOid,
+ SinglePartitionSpec *part)
+{
+ PartitionKey key = RelationGetPartitionKey(parent);
+ PartitionDesc partdesc = RelationGetPartitionDesc(parent, false);
+ PartitionBoundInfo boundinfo = partdesc->boundinfo;
+ SinglePartitionSpec *parts[1];
+ Datum datum = PointerGetDatum(NULL);
+
+ Assert(key->strategy == PARTITION_STRATEGY_LIST);
+
+ parts[0] = part;
+
+ /*
+ * Special processing for NULL value. Search for a NULL value if the
+ * split partition contains it.
+ */
+ if (partition_bound_accepts_nulls(boundinfo) &&
+ partdesc->oids[boundinfo->null_index] == splitPartOid)
+ {
+ if (!find_value_in_new_partitions_list(&key->partsupfunc[0],
+ key->partcollation, parts, 1,
+ datum, true))
+ return false;
+ }
+
+ /*
+ * Search all values of the split partition in the single non-DEFAULT
+ * replacement partition.
+ */
+ for (int i = 0; i < boundinfo->ndatums; i++)
+ {
+ if (partdesc->oids[boundinfo->indexes[i]] == splitPartOid)
+ {
+ datum = boundinfo->datums[i][0];
+
+ if (!find_value_in_new_partitions_list(&key->partsupfunc[0],
+ key->partcollation, parts, 1,
+ datum, false))
+ return false;
+ }
+ }
+
+ return true;
+}
+
+/*
+ * check_split_partition_not_same_bound
+ *
+ * Reject splitting a non-DEFAULT partition into one non-DEFAULT partition
+ * with the original bound plus a DEFAULT partition. That form does not
+ * perform a real split; it merely adds a DEFAULT partition to the parent
+ * table through the split-partition path. Users should use
+ * CREATE TABLE ... PARTITION OF ... DEFAULT or ALTER TABLE ... ATTACH
+ * PARTITION ... DEFAULT for that.
+ *
+ * Must be called after the per-partition bound validation in
+ * check_partitions_for_split() so that containment of new bounds within the
+ * split partition is already established. Given containment, RANGE bounds
+ * are equal iff their lower and upper rbounds match; LIST bound sets are
+ * equal iff the split partition's values are also contained in the new
+ * partition (the containment is then bidirectional). Both checks go
+ * through the partition operator family (partition_rbound_cmp /
+ * find_value_in_new_partitions_list) rather than byte equality, so e.g.
+ * -0.0 and 0.0 -- which have different bit patterns but compare equal
+ * under float8 -- are correctly recognised as the same bound.
+ */
+static void
+check_split_partition_not_same_bound(Relation parent,
+ Oid splitPartOid,
+ SinglePartitionSpec **parts,
+ int nparts,
+ ParseState *pstate)
+{
+ PartitionKey key = RelationGetPartitionKey(parent);
+ PartitionBoundSpec *new_spec;
+ PartitionBoundSpec *split_spec;
+
+ if (nparts != 1)
+ return;
+
+ new_spec = parts[0]->bound;
+ split_spec = get_partition_bound_spec(splitPartOid);
+
+ Assert(new_spec->strategy == split_spec->strategy);
+
+ if (key->strategy == PARTITION_STRATEGY_RANGE)
+ {
+ PartitionRangeBound *new_lower;
+ PartitionRangeBound *new_upper;
+ PartitionRangeBound *split_lower;
+ PartitionRangeBound *split_upper;
+
+ new_lower = make_one_partition_rbound(key, -1, new_spec->lowerdatums, true);
+ new_upper = make_one_partition_rbound(key, -1, new_spec->upperdatums, false);
+ split_lower = make_one_partition_rbound(key, -1, split_spec->lowerdatums, true);
+ split_upper = make_one_partition_rbound(key, -1, split_spec->upperdatums, false);
+
+ if (partition_rbound_cmp(key->partnatts, key->partsupfunc,
+ key->partcollation,
+ new_lower->datums, new_lower->kind, true,
+ split_lower) != 0)
+ return;
+ if (partition_rbound_cmp(key->partnatts, key->partsupfunc,
+ key->partcollation,
+ new_upper->datums, new_upper->kind, false,
+ split_upper) != 0)
+ return;
+ }
+ else
+ {
+ Assert(key->strategy == PARTITION_STRATEGY_LIST);
+
+ if (!split_partition_values_contained_in_new_part(parent, splitPartOid,
+ parts[0]))
+ return;
+ }
+
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot split partition \"%s\" only to add a DEFAULT partition",
+ get_rel_name(splitPartOid)),
+ errdetail("The non-DEFAULT partition would keep the same partition bound."),
+ errhint("Use CREATE TABLE ... PARTITION OF ... DEFAULT to add a DEFAULT partition."),
+ parser_errposition(pstate, parts[0]->name->location));
+}
+
/*
* check_partitions_for_split
*
@@ -5871,5 +6011,15 @@ check_partitions_for_split(Relation parent,
new_parts, nparts, pstate);
}
+ /*
+ * Reject the degenerate form where the single non-DEFAULT replacement
+ * partition keeps the bound of the split partition; the command then does
+ * nothing beyond adding a DEFAULT partition. Containment was established
+ * by the per-partition validation above, so an equality check is enough.
+ */
+ if (!isSplitPartDefault && createDefaultPart)
+ check_split_partition_not_same_bound(parent, splitPartOid, new_parts,
+ nparts, pstate);
+
pfree(new_parts);
}
diff --git a/src/test/regress/expected/partition_split.out b/src/test/regress/expected/partition_split.out
index 2b9a6aa50ed..ff6027af658 100644
--- a/src/test/regress/expected/partition_split.out
+++ b/src/test/regress/expected/partition_split.out
@@ -1188,6 +1188,64 @@ SELECT tableoid::regclass, * FROM sales_range ORDER BY tableoid::regclass::text
DROP TABLE sales_range;
--
+-- Test that SPLIT PARTITION rejects the degenerate case where the only
+-- non-DEFAULT replacement partition keeps the original bound and the command
+-- merely adds a DEFAULT partition.
+--
+CREATE TABLE t (i int) PARTITION BY RANGE (i);
+CREATE TABLE tp_0_50 PARTITION OF t FOR VALUES FROM (0) TO (50);
+INSERT INTO t VALUES (1);
+-- ERROR
+ALTER TABLE t SPLIT PARTITION tp_0_50 INTO
+ (PARTITION tp_0_50 FOR VALUES FROM (0) TO (50),
+ PARTITION tp_default DEFAULT);
+ERROR: cannot split partition "tp_0_50" only to add a DEFAULT partition
+LINE 2: (PARTITION tp_0_50 FOR VALUES FROM (0) TO (50),
+ ^
+DETAIL: The non-DEFAULT partition would keep the same partition bound.
+HINT: Use CREATE TABLE ... PARTITION OF ... DEFAULT to add a DEFAULT partition.
+DROP TABLE t;
+--
+-- Test that a LIST split with DEFAULT is not considered degenerate when
+-- only NULL is removed from the explicit replacement partition.
+--
+CREATE TABLE t (i int) PARTITION BY LIST (i);
+CREATE TABLE tp_null_1 PARTITION OF t FOR VALUES IN (NULL, 1);
+ALTER TABLE t SPLIT PARTITION tp_null_1 INTO
+ (PARTITION tp_1 FOR VALUES IN (1),
+ PARTITION tp_default DEFAULT);
+INSERT INTO t VALUES (NULL), (1), (2);
+SELECT tableoid::regclass, i FROM t ORDER BY tableoid::regclass::text COLLATE "C", i NULLS FIRST;
+ tableoid | i
+------------+---
+ tp_1 | 1
+ tp_default |
+ tp_default | 2
+(3 rows)
+
+DROP TABLE t;
+--
+-- Test that the same-bound check for LIST partitioning uses the
+-- partition operator family, not byte equality. -0.0 and 0.0 have
+-- different bit patterns but compare equal under float8, so the
+-- replacement bound (-0.0, 1.0) is the same set as the original
+-- (0.0, 1.0) and the SPLIT is degenerate. A datumIsEqual()-based
+-- check would let this through; the partsupfunc-based check correctly
+-- rejects it.
+--
+CREATE TABLE t (v float8) PARTITION BY LIST (v);
+CREATE TABLE tp_zero_one PARTITION OF t FOR VALUES IN (0.0, 1.0);
+-- ERROR
+ALTER TABLE t SPLIT PARTITION tp_zero_one INTO
+ (PARTITION tp_zero_one FOR VALUES IN (-0.0, 1.0),
+ PARTITION tp_default DEFAULT);
+ERROR: cannot split partition "tp_zero_one" only to add a DEFAULT partition
+LINE 2: (PARTITION tp_zero_one FOR VALUES IN (-0.0, 1.0),
+ ^
+DETAIL: The non-DEFAULT partition would keep the same partition bound.
+HINT: Use CREATE TABLE ... PARTITION OF ... DEFAULT to add a DEFAULT partition.
+DROP TABLE t;
+--
-- Test that the explicit partition bound cannot extend outside the split
-- partition's bound when a DEFAULT partition is specified.
--
diff --git a/src/test/regress/sql/partition_split.sql b/src/test/regress/sql/partition_split.sql
index d9821c5e2a3..05de24152d1 100644
--- a/src/test/regress/sql/partition_split.sql
+++ b/src/test/regress/sql/partition_split.sql
@@ -834,6 +834,57 @@ SELECT tableoid::regclass, * FROM sales_range ORDER BY tableoid::regclass::text
DROP TABLE sales_range;
+--
+-- Test that SPLIT PARTITION rejects the degenerate case where the only
+-- non-DEFAULT replacement partition keeps the original bound and the command
+-- merely adds a DEFAULT partition.
+--
+CREATE TABLE t (i int) PARTITION BY RANGE (i);
+CREATE TABLE tp_0_50 PARTITION OF t FOR VALUES FROM (0) TO (50);
+INSERT INTO t VALUES (1);
+
+-- ERROR
+ALTER TABLE t SPLIT PARTITION tp_0_50 INTO
+ (PARTITION tp_0_50 FOR VALUES FROM (0) TO (50),
+ PARTITION tp_default DEFAULT);
+
+DROP TABLE t;
+
+--
+-- Test that a LIST split with DEFAULT is not considered degenerate when
+-- only NULL is removed from the explicit replacement partition.
+--
+CREATE TABLE t (i int) PARTITION BY LIST (i);
+CREATE TABLE tp_null_1 PARTITION OF t FOR VALUES IN (NULL, 1);
+
+ALTER TABLE t SPLIT PARTITION tp_null_1 INTO
+ (PARTITION tp_1 FOR VALUES IN (1),
+ PARTITION tp_default DEFAULT);
+
+INSERT INTO t VALUES (NULL), (1), (2);
+SELECT tableoid::regclass, i FROM t ORDER BY tableoid::regclass::text COLLATE "C", i NULLS FIRST;
+
+DROP TABLE t;
+
+--
+-- Test that the same-bound check for LIST partitioning uses the
+-- partition operator family, not byte equality. -0.0 and 0.0 have
+-- different bit patterns but compare equal under float8, so the
+-- replacement bound (-0.0, 1.0) is the same set as the original
+-- (0.0, 1.0) and the SPLIT is degenerate. A datumIsEqual()-based
+-- check would let this through; the partsupfunc-based check correctly
+-- rejects it.
+--
+CREATE TABLE t (v float8) PARTITION BY LIST (v);
+CREATE TABLE tp_zero_one PARTITION OF t FOR VALUES IN (0.0, 1.0);
+
+-- ERROR
+ALTER TABLE t SPLIT PARTITION tp_zero_one INTO
+ (PARTITION tp_zero_one FOR VALUES IN (-0.0, 1.0),
+ PARTITION tp_default DEFAULT);
+
+DROP TABLE t;
+
--
-- Test that the explicit partition bound cannot extend outside the split
-- partition's bound when a DEFAULT partition is specified.
--
2.39.5 (Apple Git-154)
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]
Subject: Re: Fix SPLIT PARTITION bound-overlap bug and other improvements
In-Reply-To: <CAPpHfdtRNFwVBrekh_b6Pg5fmi+QCoyEdAs7h=ikxXoDcOMdHA@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