public inbox for [email protected]
help / color / mirror / Atom feedFrom: Masahiko Sawada <[email protected]>
To: Andrei Lepikhov <[email protected]>
Cc: PostgreSQL mailing lists <[email protected]>
Subject: Re: TRAP: failed Assert("offsets[i] > offsets[i - 1]"), File: "tidstore.c"
Date: Tue, 28 Apr 2026 10:09:17 -0700
Message-ID: <CAD21AoB-Q=oaDzbk5r-HNHu1E0X8uj2zs62fXkEdLfVcjTkTjA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CAD21AoBVEcC5stzLr80RgaWuBh0EoyRQys_aeOz0ceogMVREcQ@mail.gmail.com>
<[email protected]>
<CAD21AoC86nuoxy4r6G3_Ysb2y3K+0sybznRkjVq=Yc4URZUN4g@mail.gmail.com>
<[email protected]>
<CAD21AoAit-Qp9OriVbb9c_Qebi=Cgxz0AQJu+zxQpp=_1Lt2dQ@mail.gmail.com>
<[email protected]>
<CAD21AoCmEqiQVgJc34yGK7DSQj-p-zBVrm4-PfrwYHdNkJGt5g@mail.gmail.com>
<[email protected]>
<CAD21AoAORy3MmSxPEaRbE_BuwW3qyxsfLGj81YcOtn6gv2iJww@mail.gmail.com>
<[email protected]>
On Sun, Apr 26, 2026 at 2:06 AM Andrei Lepikhov <[email protected]> wrote:
>
> On 25/04/2026 01:23, Masahiko Sawada wrote:
> > On Wed, Apr 22, 2026 at 10:23 AM Andrei Lepikhov <[email protected]> wrote:
> >> Both tools are experimental and not meant for core use; they are only used to
> >> trigger potential issues. In this case, I think the query picked a costly sorted
> >> path, which led to the crash.
> >
> > Does this imply that array_agg() could return unsorted results
> > depending on the plan the optimizer chooses? Or is such a path
> > currently never selected by the optimizer?
>
> The array_agg() function does not sort its output. In theory, this means the
> join could return results in any order, but in practice, I have not seen this
> happen.
>
> >
> > I’m asking because if this scenario never occurs with the current
> > optimizer, it might make sense to apply the patch only to HEAD (i.e.,
> > for PG20). On the other hand, backpatching to PG17 might be justified,
> > given that DISTINCT does not guarantee sorted results in principle,
> > and the fix could benefit extension development on stable branches.
>
> In stable versions, the planner's logic remains unchanged. So, it seems
> reliable. However, backpatching could help extension developers a little bit.
> Since this code fixes a real issue and does not break anything complex, I would
> backpatch it. Still, I am fine with just committing it to master if you prefer.
>
> P.S.
>
> I looked into the issue further. The problem happens when the join sides are
> shuffled. Here is what I found:
>
> EXPLAIN of the successful execution (unnecessary details stripped):
>
> Insert on pg_temp.hideblocks (cost=1.21..1.66 rows=0 width=0)
> -> Subquery Scan on unnamed_subquery (cost=1.21..1.66 rows=5 width=8)
> Output: unnamed_subquery.do_set_block_offsets
> -> GroupAggregate (cost=1.21..1.61 rows=5 width=16)
> Output: do_set_block_offsets("*VALUES*".column1,
> (array_agg("*VALUES*_1".column1))::smallint[]), ...
> Group Key: "*VALUES*".column1
> -> Sort (cost=1.21..1.27 rows=25 width=12)
> Output: "*VALUES*".column1, "*VALUES*_1".column1
> Sort Key: "*VALUES*".column1
> -> Nested Loop (cost=0.00..0.62 rows=25 width=12)
> Output: "*VALUES*".column1, "*VALUES*_1".column1
> -> Values Scan on "*VALUES*"
> (cost=0.00..0.06 rows=5 width=8)
> Output: "*VALUES*".column1
> -> Values Scan on "*VALUES*_1"
> (cost=0.00..0.06 rows=5 width=4)
> Output: "*VALUES*_1".column1
>
> EXPLAIN that causes assertion:
>
> Insert on pg_temp.hideblocks (cost=1.03..1.48 rows=0 width=0)
> -> Subquery Scan on unnamed_subquery (cost=1.03..1.48 rows=5 width=8)
> Output: unnamed_subquery.do_set_block_offsets
> -> GroupAggregate (cost=1.03..1.43 rows=5 width=16)
> Output: do_set_block_offsets("*VALUES*".column1,
> (array_agg("*VALUES*_1".column1))::smallint[]),...
> Group Key: "*VALUES*".column1
> -> Sort (cost=1.03..1.09 rows=25 width=12)
> Output: "*VALUES*".column1, "*VALUES*_1".column1
> Sort Key: "*VALUES*".column1
> -> Nested Loop (cost=0.00..0.45 rows=25 width=12)
> Output: "*VALUES*".column1, "*VALUES*_1".column1
> -> Values Scan on "*VALUES*_1"
> (cost=0.00..0.06 rows=5 width=4)
> Output: "*VALUES*_1".column1
> -> Materialize (cost=0.00..0.09 rows=5 width=8)
> Output: "*VALUES*".column1
> -> Values Scan on "*VALUES*"
> (cost=0.00..0.06 rows=5 width=8)
> Output: "*VALUES*".column1
>
> At the second case offsets have come to the aggregation without order that
> highlighted the issue.
Thank you for sharing the details.
While the assertion failure is not observed during regular regression
tests because the query is simple enough that the optimizer
consistently chooses plans producing the sorted results, given that
the DISTINCT without the ORDER BY doesn't guarantee to produce the
sorted results in theory, I think it makes sense to apply the proposed
patch. And, it would also make sense to backpatch to PG17, where
tid_store was introduced, for extension development on back branches.
I've attached the patches. I'm going to push them, barring any objections.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-patch] REL_17_0001-test_tidstore-Stabilize-regression-tests-by-sorting-.patch (2.6K, 2-REL_17_0001-test_tidstore-Stabilize-regression-tests-by-sorting-.patch)
download | inline diff:
From d3d63ff758f238f632342ca8ff0c957beb82ac75 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <[email protected]>
Date: Wed, 15 Apr 2026 14:41:51 +0200
Subject: [PATCH] test_tidstore: Stabilize regression tests by sorting offsets.
TidStoreSetBlockOffsets() requires its offsets array to be strictly
ascending and asserts this precondition. In test_tidstore, we were
passing random offset numbers deduplicated by a DISTINCT clause in an
array_agg() call directly to the do_set_block_offsets() test
harness. However, DISTINCT without an ORDER BY clause does not
guarantee sorted results according to the SQL standard.
Fix this by sorting the offsets in-place inside do_set_block_offsets()
before calling TidStoreSetBlockOffsets().
While this assertion failure is not observed during regular regression
tests because they use queries simple enough that the optimizer
consistently chooses plans yielding sorted results, it makes sense to
stabilize the test. The failure could theoretically occur depending on
the optimizer's plan choice, and has been reported when experimenting
with certain third-party extensions.
Backpatch to v17, where tid_store was introduced, to ensure extension
development on stable branches does not hit this assertion.
Reported-by: Andrei Lepikhov <[email protected]>
Author: Andrei Lepikhov <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/test/modules/test_tidstore/test_tidstore.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/src/test/modules/test_tidstore/test_tidstore.c b/src/test/modules/test_tidstore/test_tidstore.c
index 3f6a11bf21c..e32ce4e5716 100644
--- a/src/test/modules/test_tidstore/test_tidstore.c
+++ b/src/test/modules/test_tidstore/test_tidstore.c
@@ -76,6 +76,19 @@ itemptr_cmp(const void *left, const void *right)
return 0;
}
+static int
+offsetnumber_cmp(const void *a, const void *b)
+{
+ OffsetNumber l = *(const OffsetNumber *) a;
+ OffsetNumber r = *(const OffsetNumber *) b;
+
+ if (l < r)
+ return -1;
+ else if (l > r)
+ return 1;
+ return 0;
+}
+
/*
* Create a TidStore. If shared is false, the tidstore is created
* on TopMemoryContext, otherwise on DSA. Although the tidstore
@@ -180,6 +193,9 @@ do_set_block_offsets(PG_FUNCTION_ARGS)
noffs = ArrayGetNItems(ARR_NDIM(ta), ARR_DIMS(ta));
offs = ((OffsetNumber *) ARR_DATA_PTR(ta));
+ /* TidStoreSetBlockOffsets() requires offsets to be strictly ascending. */
+ qsort(offs, noffs, sizeof(OffsetNumber), offsetnumber_cmp);
+
/* Set TIDs in the store */
TidStoreLockExclusive(tidstore);
TidStoreSetBlockOffsets(tidstore, blkno, offs, noffs);
--
2.54.0
[text/x-patch] master_0001-test_tidstore-Stabilize-regression-tests-by-sorting-.patch (2.6K, 3-master_0001-test_tidstore-Stabilize-regression-tests-by-sorting-.patch)
download | inline diff:
From d12d864634ec2d6445397362844609ab7528106e Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <[email protected]>
Date: Wed, 15 Apr 2026 14:41:51 +0200
Subject: [PATCH] test_tidstore: Stabilize regression tests by sorting offsets.
TidStoreSetBlockOffsets() requires its offsets array to be strictly
ascending and asserts this precondition. In test_tidstore, we were
passing random offset numbers deduplicated by a DISTINCT clause in an
array_agg() call directly to the do_set_block_offsets() test
harness. However, DISTINCT without an ORDER BY clause does not
guarantee sorted results according to the SQL standard.
Fix this by sorting the offsets in-place inside do_set_block_offsets()
before calling TidStoreSetBlockOffsets().
While this assertion failure is not observed during regular regression
tests because they use queries simple enough that the optimizer
consistently chooses plans yielding sorted results, it makes sense to
stabilize the test. The failure could theoretically occur depending on
the optimizer's plan choice, and has been reported when experimenting
with certain third-party extensions.
Backpatch to v17, where tid_store was introduced, to ensure extension
development on stable branches does not hit this assertion.
Reported-by: Andrei Lepikhov <[email protected]>
Author: Andrei Lepikhov <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/test/modules/test_tidstore/test_tidstore.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/src/test/modules/test_tidstore/test_tidstore.c b/src/test/modules/test_tidstore/test_tidstore.c
index c9a035fa494..bfc40bce24c 100644
--- a/src/test/modules/test_tidstore/test_tidstore.c
+++ b/src/test/modules/test_tidstore/test_tidstore.c
@@ -75,6 +75,19 @@ itemptr_cmp(const void *left, const void *right)
return 0;
}
+static int
+offsetnumber_cmp(const void *a, const void *b)
+{
+ OffsetNumber l = *(const OffsetNumber *) a;
+ OffsetNumber r = *(const OffsetNumber *) b;
+
+ if (l < r)
+ return -1;
+ else if (l > r)
+ return 1;
+ return 0;
+}
+
/*
* Create a TidStore. If shared is false, the tidstore is created
* on TopMemoryContext, otherwise on DSA. Although the tidstore
@@ -178,6 +191,9 @@ do_set_block_offsets(PG_FUNCTION_ARGS)
noffs = ArrayGetNItems(ARR_NDIM(ta), ARR_DIMS(ta));
offs = ((OffsetNumber *) ARR_DATA_PTR(ta));
+ /* TidStoreSetBlockOffsets() requires offsets to be strictly ascending. */
+ qsort(offs, noffs, sizeof(OffsetNumber), offsetnumber_cmp);
+
/* Set TIDs in the store */
TidStoreLockExclusive(tidstore);
TidStoreSetBlockOffsets(tidstore, blkno, offs, noffs);
--
2.54.0
[text/x-patch] REL_18_0001-test_tidstore-Stabilize-regression-tests-by-sorting-.patch (2.6K, 4-REL_18_0001-test_tidstore-Stabilize-regression-tests-by-sorting-.patch)
download | inline diff:
From dd3bf8933fc1368bfe22b039339a4633e2f510e1 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <[email protected]>
Date: Wed, 15 Apr 2026 14:41:51 +0200
Subject: [PATCH] test_tidstore: Stabilize regression tests by sorting offsets.
TidStoreSetBlockOffsets() requires its offsets array to be strictly
ascending and asserts this precondition. In test_tidstore, we were
passing random offset numbers deduplicated by a DISTINCT clause in an
array_agg() call directly to the do_set_block_offsets() test
harness. However, DISTINCT without an ORDER BY clause does not
guarantee sorted results according to the SQL standard.
Fix this by sorting the offsets in-place inside do_set_block_offsets()
before calling TidStoreSetBlockOffsets().
While this assertion failure is not observed during regular regression
tests because they use queries simple enough that the optimizer
consistently chooses plans yielding sorted results, it makes sense to
stabilize the test. The failure could theoretically occur depending on
the optimizer's plan choice, and has been reported when experimenting
with certain third-party extensions.
Backpatch to v17, where tid_store was introduced, to ensure extension
development on stable branches does not hit this assertion.
Reported-by: Andrei Lepikhov <[email protected]>
Author: Andrei Lepikhov <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/test/modules/test_tidstore/test_tidstore.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/src/test/modules/test_tidstore/test_tidstore.c b/src/test/modules/test_tidstore/test_tidstore.c
index eb16e0fbfa6..d12d54f3677 100644
--- a/src/test/modules/test_tidstore/test_tidstore.c
+++ b/src/test/modules/test_tidstore/test_tidstore.c
@@ -75,6 +75,19 @@ itemptr_cmp(const void *left, const void *right)
return 0;
}
+static int
+offsetnumber_cmp(const void *a, const void *b)
+{
+ OffsetNumber l = *(const OffsetNumber *) a;
+ OffsetNumber r = *(const OffsetNumber *) b;
+
+ if (l < r)
+ return -1;
+ else if (l > r)
+ return 1;
+ return 0;
+}
+
/*
* Create a TidStore. If shared is false, the tidstore is created
* on TopMemoryContext, otherwise on DSA. Although the tidstore
@@ -179,6 +192,9 @@ do_set_block_offsets(PG_FUNCTION_ARGS)
noffs = ArrayGetNItems(ARR_NDIM(ta), ARR_DIMS(ta));
offs = ((OffsetNumber *) ARR_DATA_PTR(ta));
+ /* TidStoreSetBlockOffsets() requires offsets to be strictly ascending. */
+ qsort(offs, noffs, sizeof(OffsetNumber), offsetnumber_cmp);
+
/* Set TIDs in the store */
TidStoreLockExclusive(tidstore);
TidStoreSetBlockOffsets(tidstore, blkno, offs, noffs);
--
2.54.0
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: TRAP: failed Assert("offsets[i] > offsets[i - 1]"), File: "tidstore.c"
In-Reply-To: <CAD21AoB-Q=oaDzbk5r-HNHu1E0X8uj2zs62fXkEdLfVcjTkTjA@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