public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v6 3/7] Propagate changes to indisclustered to child/parents
13+ messages / 4 participants
[nested] [flat]
* [PATCH v6 3/7] Propagate changes to indisclustered to child/parents
@ 2020-10-07 03:11 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Justin Pryzby @ 2020-10-07 03:11 UTC (permalink / raw)
---
src/backend/commands/cluster.c | 109 ++++++++++++++++----------
src/backend/commands/indexcmds.c | 2 +
src/test/regress/expected/cluster.out | 46 +++++++++++
src/test/regress/sql/cluster.sql | 11 +++
4 files changed, 125 insertions(+), 43 deletions(-)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 838bcd9e72..078b97fbb9 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -74,6 +74,7 @@ static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose);
static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
+static void set_indisclustered(Oid indexOid, bool isclustered, Relation pg_index);
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
@@ -510,66 +511,88 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
index_close(OldIndex, NoLock);
}
+/*
+ * Helper for mark_index_clustered
+ * Mark a single index as clustered or not.
+ * pg_index is passed by caller to avoid repeatedly re-opening it.
+ */
+static void
+set_indisclustered(Oid indexOid, bool isclustered, Relation pg_index)
+{
+ HeapTuple indexTuple;
+ Form_pg_index indexForm;
+
+ indexTuple = SearchSysCacheCopy1(INDEXRELID,
+ ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(indexTuple))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+ indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+ /* this was checked earlier, but let's be real sure */
+ if (isclustered && !indexForm->indisvalid)
+ elog(ERROR, "cannot cluster on invalid index %u", indexOid);
+
+ indexForm->indisclustered = isclustered;
+ CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
+ heap_freetuple(indexTuple);
+}
+
/*
* mark_index_clustered: mark the specified index as the one clustered on
*
- * With indexOid == InvalidOid, will mark all indexes of rel not-clustered.
+ * With indexOid == InvalidOid, mark all indexes of rel not-clustered.
+ * Otherwise, mark children of the clustered index as clustered, and parents of
+ * other indexes as unclustered.
+ * We wish to maintain the following properties:
+ * 1) Only one index on a relation can be marked clustered at once
+ * 2) If a partitioned index is clustered, then all its children must be
+ * clustered.
*/
void
mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
{
- HeapTuple indexTuple;
- Form_pg_index indexForm;
- Relation pg_index;
- ListCell *index;
-
- /*
- * If the index is already marked clustered, no need to do anything.
- */
- if (OidIsValid(indexOid))
- {
- if (get_index_isclustered(indexOid))
- return;
- }
+ ListCell *lc, *lc2;
+ List *indexes;
+ Relation pg_index = table_open(IndexRelationId, RowExclusiveLock);
+ List *inh = find_all_inheritors(RelationGetRelid(rel), ShareRowExclusiveLock, NULL);
/*
* Check each index of the relation and set/clear the bit as needed.
+ * Iterate over the relation's children rather than the index's children
+ * since we need to unset cluster for indexes on intermediate children,
+ * too.
*/
- pg_index = table_open(IndexRelationId, RowExclusiveLock);
-
- foreach(index, RelationGetIndexList(rel))
+ foreach(lc, inh)
{
- Oid thisIndexOid = lfirst_oid(index);
-
- indexTuple = SearchSysCacheCopy1(INDEXRELID,
- ObjectIdGetDatum(thisIndexOid));
- if (!HeapTupleIsValid(indexTuple))
- elog(ERROR, "cache lookup failed for index %u", thisIndexOid);
- indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+ Oid inhrelid = lfirst_oid(lc);
+ Relation thisrel = table_open(inhrelid, ShareRowExclusiveLock);
- /*
- * Unset the bit if set. We know it's wrong because we checked this
- * earlier.
- */
- if (indexForm->indisclustered)
+ indexes = RelationGetIndexList(thisrel);
+ foreach (lc2, indexes)
{
- indexForm->indisclustered = false;
- CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
- }
- else if (thisIndexOid == indexOid)
- {
- /* this was checked earlier, but let's be real sure */
- if (!indexForm->indisvalid)
- elog(ERROR, "cannot cluster on invalid index %u", indexOid);
- indexForm->indisclustered = true;
- CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
- }
+ bool isclustered;
+ Oid thisIndexOid = lfirst_oid(lc2);
+ List *parentoids = get_rel_relispartition(thisIndexOid) ?
+ get_partition_ancestors(thisIndexOid) : NIL;
- InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
- InvalidOid, is_internal);
+ /*
+ * A child of the clustered index must be set clustered;
+ * indexes which are not children of the clustered index are
+ * set unclustered
+ */
+ isclustered = (thisIndexOid == indexOid) ||
+ list_member_oid(parentoids, indexOid);
+ Assert(OidIsValid(indexOid) || !isclustered);
+ set_indisclustered(thisIndexOid, isclustered, pg_index);
+
+ InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
+ InvalidOid, is_internal);
+ }
- heap_freetuple(indexTuple);
+ list_free(indexes);
+ table_close(thisrel, ShareRowExclusiveLock);
}
+ list_free(inh);
table_close(pg_index, RowExclusiveLock);
}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index f9f3ff3b62..0a65698c28 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -25,6 +25,7 @@
#include "catalog/catalog.h"
#include "catalog/index.h"
#include "catalog/indexing.h"
+#include "catalog/partition.h"
#include "catalog/pg_am.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_inherits.h"
@@ -32,6 +33,7 @@
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
#include "catalog/pg_type.h"
+#include "commands/cluster.h"
#include "commands/comment.h"
#include "commands/dbcommands.h"
#include "commands/defrem.h"
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index e4448350e7..a9fb9f1021 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -495,6 +495,52 @@ Indexes:
"clstrpart_idx" btree (a) CLUSTER
Number of partitions: 3 (Use \d+ to list them.)
+-- Test that it recurses to grandchildren:
+\d clstrpart33
+ Table "public.clstrpart33"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart3 DEFAULT
+Indexes:
+ "clstrpart33_a_idx" btree (a) CLUSTER
+
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+\d clstrpart33
+ Table "public.clstrpart33"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart3 DEFAULT
+Indexes:
+ "clstrpart33_a_idx" btree (a)
+
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+\d clstrpart33
+ Table "public.clstrpart33"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart3 DEFAULT
+Indexes:
+ "clstrpart33_a_idx" btree (a) CLUSTER
+
+-- Check that only one child is marked clustered after marking clustered on a different parent
+CREATE INDEX clstrpart1_idx_2 ON clstrpart1(a);
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+ALTER TABLE clstrpart1 CLUSTER ON clstrpart1_idx_2;
+\d clstrpart1
+ Partitioned table "public.clstrpart1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart FOR VALUES FROM (1) TO (10)
+Partition key: RANGE (a)
+Indexes:
+ "clstrpart1_a_idx" btree (a)
+ "clstrpart1_idx_2" btree (a) CLUSTER
+Number of partitions: 2 (Use \d+ to list them.)
+
-- Test CLUSTER with external tuplesorting
create table clstr_4 as select * from tenk1;
create index cluster_sort on clstr_4 (hundred, thousand, tenthous);
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 22225dc924..d15bd51496 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -220,6 +220,17 @@ CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitio
CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
\d clstrpart
+-- Test that it recurses to grandchildren:
+\d clstrpart33
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+\d clstrpart33
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+\d clstrpart33
+-- Check that only one child is marked clustered after marking clustered on a different parent
+CREATE INDEX clstrpart1_idx_2 ON clstrpart1(a);
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+ALTER TABLE clstrpart1 CLUSTER ON clstrpart1_idx_2;
+\d clstrpart1
-- Test CLUSTER with external tuplesorting
--
2.17.0
--FsscpQKzF/jJk6ya
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0004-Invalidate-parent-indexes.patch"
^ permalink raw reply [nested|flat] 13+ messages in thread
* strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-06 23:03 Tomas Vondra <[email protected]>
0 siblings, 2 replies; 13+ messages in thread
From: Tomas Vondra @ 2025-03-06 23:03 UTC (permalink / raw)
To: pgsql-hackers
Hi,
while running check-world on 64-bit arm (rpi5 with Debian 12.9), I got a
couple reports like this:
==64550== Use of uninitialised value of size 8
==64550== at 0xA62FE0: wrapper_handler (pqsignal.c:107)
==64550== by 0x580BB9E7: ??? (in
/usr/libexec/valgrind/memcheck-arm64-linux)
==64550== Uninitialised value was created by a stack allocation
==64550== at 0x4F94660: strcoll_l (strcoll_l.c:258)
==64550==
{
<insert_a_suppression_name_here>
Memcheck:Value8
fun:wrapper_handler
obj:/usr/libexec/valgrind/memcheck-arm64-linux
}
**64550** Valgrind detected 1 error(s) during execution of "ANALYZE
mcv_lists;"
The exact command varies, I don't think it's necessarily about analyze
or extended stats.
The line the report refers to is this:
(*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
so I guess it can't be about postgres_signal_arg (as that's an int). But
that leaves just pqsignal_handlers, and why would that be uninitialized?
The closest thing I found in archives is [1] from about a year ago, but
we haven't found any clear explanation there either :-(
[1]
https://www.postgresql.org/message-id/[email protected]
regards
--
Tomas Vondra
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-07 16:15 Nathan Bossart <[email protected]>
parent: Tomas Vondra <[email protected]>
1 sibling, 0 replies; 13+ messages in thread
From: Nathan Bossart @ 2025-03-07 16:15 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: pgsql-hackers
On Fri, Mar 07, 2025 at 12:03:47AM +0100, Tomas Vondra wrote:
> while running check-world on 64-bit arm (rpi5 with Debian 12.9), I got a
> couple reports like this:
>
> ==64550== Use of uninitialised value of size 8
> ==64550== at 0xA62FE0: wrapper_handler (pqsignal.c:107)
> ==64550== by 0x580BB9E7: ??? (in
> /usr/libexec/valgrind/memcheck-arm64-linux)
> ==64550== Uninitialised value was created by a stack allocation
> ==64550== at 0x4F94660: strcoll_l (strcoll_l.c:258)
> ==64550==
> {
> <insert_a_suppression_name_here>
> Memcheck:Value8
> fun:wrapper_handler
> obj:/usr/libexec/valgrind/memcheck-arm64-linux
> }
> **64550** Valgrind detected 1 error(s) during execution of "ANALYZE
> mcv_lists;"
>
> The exact command varies, I don't think it's necessarily about analyze
> or extended stats.
>
> The line the report refers to is this:
>
> (*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
>
> so I guess it can't be about postgres_signal_arg (as that's an int). But
> that leaves just pqsignal_handlers, and why would that be uninitialized?
>
> The closest thing I found in archives is [1] from about a year ago, but
> we haven't found any clear explanation there either :-(
Hm. The pointer to strcoll_l makes me wonder if there might be an issue in
one of the handler functions that wrapper_handler calls, and
wrapper_handler is getting the blame. I don't see how pqsignal_handlers
could be uninitialized. It's static, and we are careful to set it
appropriately before we call sigaction().
--
nathan
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-07 16:32 Andres Freund <[email protected]>
parent: Tomas Vondra <[email protected]>
1 sibling, 2 replies; 13+ messages in thread
From: Andres Freund @ 2025-03-07 16:32 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: pgsql-hackers; Nathan Bossart <[email protected]>
Hi,
On 2025-03-07 00:03:47 +0100, Tomas Vondra wrote:
> while running check-world on 64-bit arm (rpi5 with Debian 12.9), I got a
> couple reports like this:
>
> ==64550== Use of uninitialised value of size 8
> ==64550== at 0xA62FE0: wrapper_handler (pqsignal.c:107)
> ==64550== by 0x580BB9E7: ??? (in
> /usr/libexec/valgrind/memcheck-arm64-linux)
> ==64550== Uninitialised value was created by a stack allocation
> ==64550== at 0x4F94660: strcoll_l (strcoll_l.c:258)
> ==64550==
> {
> <insert_a_suppression_name_here>
> Memcheck:Value8
> fun:wrapper_handler
> obj:/usr/libexec/valgrind/memcheck-arm64-linux
> }
> **64550** Valgrind detected 1 error(s) during execution of "ANALYZE
> mcv_lists;"
> The exact command varies, I don't think it's necessarily about analyze
> or extended stats.
Do you have a few other examples from where it was triggered?
Is the source of the uninitialized value always strcoll_l?
Can you reliably reproduce it in certain scenarios or is it probabilistic in
some form?
Do you know what signal was delivered (I think that could be detected using
valgrinds --vgdb)?
> The line the report refers to is this:
>
> (*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
>
> so I guess it can't be about postgres_signal_arg (as that's an int). But
> that leaves just pqsignal_handlers, and why would that be uninitialized?
Is it possible that the signal number we're getting called for is above
PG_NSIG? That'd explain why the source value is something fairly random?
ISTM that we should add an Assert() to wrapper_handler() that ensures that the
signal arg is below PG_NSIG.
Might also be worth trying to run without valgrind but with address and
undefined behaviour sanitizers enabled. I don't currently have access to an
armv8 machine that's not busy doing other stuff...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-07 16:36 Nathan Bossart <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 1 reply; 13+ messages in thread
From: Nathan Bossart @ 2025-03-07 16:36 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tomas Vondra <[email protected]>; pgsql-hackers
On Fri, Mar 07, 2025 at 11:32:28AM -0500, Andres Freund wrote:
> Is it possible that the signal number we're getting called for is above
> PG_NSIG? That'd explain why the source value is something fairly random?
>
> ISTM that we should add an Assert() to wrapper_handler() that ensures that the
> signal arg is below PG_NSIG.
We have such an assertion in pqsignal() before we install wrapper_handler
for anything. Is there another way it could be getting called with a
different signo?
--
nathan
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-07 16:41 Andres Freund <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Andres Freund @ 2025-03-07 16:41 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Tomas Vondra <[email protected]>; pgsql-hackers
Hi,
On 2025-03-07 10:36:35 -0600, Nathan Bossart wrote:
> On Fri, Mar 07, 2025 at 11:32:28AM -0500, Andres Freund wrote:
> > Is it possible that the signal number we're getting called for is above
> > PG_NSIG? That'd explain why the source value is something fairly random?
> >
> > ISTM that we should add an Assert() to wrapper_handler() that ensures that the
> > signal arg is below PG_NSIG.
>
> We have such an assertion in pqsignal() before we install wrapper_handler
> for anything. Is there another way it could be getting called with a
> different signo?
Who the hell knows :).
One potential way would be that we got SIGNAL_ARGS wrong for the platform and
are interpreting some random thing as the signal number. Or something went
wrong in the windows signal emulation code. Or ...
It seems cheap insurance to add it both places.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-07 16:52 Nathan Bossart <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Bossart @ 2025-03-07 16:52 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tomas Vondra <[email protected]>; pgsql-hackers
On Fri, Mar 07, 2025 at 11:41:38AM -0500, Andres Freund wrote:
> On 2025-03-07 10:36:35 -0600, Nathan Bossart wrote:
>> On Fri, Mar 07, 2025 at 11:32:28AM -0500, Andres Freund wrote:
>> > Is it possible that the signal number we're getting called for is above
>> > PG_NSIG? That'd explain why the source value is something fairly random?
>> >
>> > ISTM that we should add an Assert() to wrapper_handler() that ensures that the
>> > signal arg is below PG_NSIG.
>>
>> We have such an assertion in pqsignal() before we install wrapper_handler
>> for anything. Is there another way it could be getting called with a
>> different signo?
>
> Who the hell knows :).
>
> One potential way would be that we got SIGNAL_ARGS wrong for the platform and
> are interpreting some random thing as the signal number. Or something went
> wrong in the windows signal emulation code. Or ...
>
> It seems cheap insurance to add it both places.
Good enough for me. I'll commit/back-patch to v17 the attached soon.
--
nathan
diff --git a/src/port/pqsignal.c b/src/port/pqsignal.c
index 5dd8b76bae8..79b50486175 100644
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -87,6 +87,8 @@ wrapper_handler(SIGNAL_ARGS)
{
int save_errno = errno;
+ Assert(postgres_signal_arg < PG_NSIG);
+
#ifndef FRONTEND
/*
Attachments:
[text/plain] assert.patch (293B, ../../Z8skOiYFFNzbPVED@nathan/2-assert.patch)
download | inline diff:
diff --git a/src/port/pqsignal.c b/src/port/pqsignal.c
index 5dd8b76bae8..79b50486175 100644
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -87,6 +87,8 @@ wrapper_handler(SIGNAL_ARGS)
{
int save_errno = errno;
+ Assert(postgres_signal_arg < PG_NSIG);
+
#ifndef FRONTEND
/*
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-07 20:38 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Bossart @ 2025-03-07 20:38 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tomas Vondra <[email protected]>; pgsql-hackers
On Fri, Mar 07, 2025 at 10:52:10AM -0600, Nathan Bossart wrote:
> Good enough for me. I'll commit/back-patch to v17 the attached soon.
On second thought, since the signal number is a signed integer, I think we
also ought to check that it's > 0. I'm running the attached patch through
the CI tests to make sure that's correct for the common platforms. If that
looks good, I'm planning to commit it.
--
nathan
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-07 21:27 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Nathan Bossart @ 2025-03-07 21:27 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tomas Vondra <[email protected]>; pgsql-hackers
On Fri, Mar 07, 2025 at 02:38:47PM -0600, Nathan Bossart wrote:
> If that looks good, I'm planning to commit it.
And committed.
--
nathan
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-08 20:38 Tomas Vondra <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 1 reply; 13+ messages in thread
From: Tomas Vondra @ 2025-03-08 20:38 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Nathan Bossart <[email protected]>
On 3/7/25 17:32, Andres Freund wrote:
> Hi,
>
> On 2025-03-07 00:03:47 +0100, Tomas Vondra wrote:
>> while running check-world on 64-bit arm (rpi5 with Debian 12.9), I got a
>> couple reports like this:
>>
>> ==64550== Use of uninitialised value of size 8
>> ==64550== at 0xA62FE0: wrapper_handler (pqsignal.c:107)
>> ==64550== by 0x580BB9E7: ??? (in
>> /usr/libexec/valgrind/memcheck-arm64-linux)
>> ==64550== Uninitialised value was created by a stack allocation
>> ==64550== at 0x4F94660: strcoll_l (strcoll_l.c:258)
>> ==64550==
>> {
>> <insert_a_suppression_name_here>
>> Memcheck:Value8
>> fun:wrapper_handler
>> obj:/usr/libexec/valgrind/memcheck-arm64-linux
>> }
>> **64550** Valgrind detected 1 error(s) during execution of "ANALYZE
>> mcv_lists;"
>
>> The exact command varies, I don't think it's necessarily about analyze
>> or extended stats.
>
> Do you have a few other examples from where it was triggered?
>
> Is the source of the uninitialized value always strcoll_l?
>
I've seen a couple reports, but only a single one had info about source
of the allocation (and that was strcoll).
> Can you reliably reproduce it in certain scenarios or is it probabilistic in
> some form?
>
I believe it's probabilistic, I certainly don't know how to trigger or
reproduce it.
> Do you know what signal was delivered (I think that could be detected using
> valgrinds --vgdb)?
>
No idea.
>
>> The line the report refers to is this:
>>
>> (*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
>>
>> so I guess it can't be about postgres_signal_arg (as that's an int). But
>> that leaves just pqsignal_handlers, and why would that be uninitialized?
>
> Is it possible that the signal number we're getting called for is above
> PG_NSIG? That'd explain why the source value is something fairly random?
>
No idea.
> ISTM that we should add an Assert() to wrapper_handler() that ensures that the
> signal arg is below PG_NSIG.
>
No idea.
>
> Might also be worth trying to run without valgrind but with address and
> undefined behaviour sanitizers enabled. I don't currently have access to an
> armv8 machine that's not busy doing other stuff...
>
I've restarted check-world with valgrind on my rpi5 machines, with
current master. I can try running other stuff once that finishes in a
couple hours.
regards
--
Tomas Vondra
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-08 22:48 Tomas Vondra <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tomas Vondra @ 2025-03-08 22:48 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Nathan Bossart <[email protected]>
On 3/8/25 21:38, Tomas Vondra wrote:
>
> I've restarted check-world with valgrind on my rpi5 machines, with
> current master. I can try running other stuff once that finishes in a
> couple hours.
>
Shortly after restarting this I got three more reports - all of them are
related to strcoll_l. This is on c472a18296e4, i.e. with the asserts
added in this thread etc. But none of those seem to fail.
regards
--
Tomas Vondra
Attachments:
[text/x-log] valgrind.log (1.5K, ../../[email protected]/2-valgrind.log)
download | inline:
==189168== Conditional jump or move depends on uninitialised value(s)
==189168== at 0xA683CC: wrapper_handler (pqsignal.c:90)
==189168== by 0x580BB9E7: ??? (in /usr/libexec/valgrind/memcheck-arm64-linux)
==189168== Uninitialised value was created by a stack allocation
==189168== at 0x4F94660: strcoll_l (strcoll_l.c:258)
==189168==
{
<insert_a_suppression_name_here>
Memcheck:Cond
fun:wrapper_handler
obj:/usr/libexec/valgrind/memcheck-arm64-linux
}
==189168== Conditional jump or move depends on uninitialised value(s)
==189168== at 0xA683F0: wrapper_handler (pqsignal.c:91)
==189168== by 0x580BB9E7: ??? (in /usr/libexec/valgrind/memcheck-arm64-linux)
==189168== Uninitialised value was created by a stack allocation
==189168== at 0x4F94660: strcoll_l (strcoll_l.c:258)
==189168==
{
<insert_a_suppression_name_here>
Memcheck:Cond
fun:wrapper_handler
obj:/usr/libexec/valgrind/memcheck-arm64-linux
}
==189168== Use of uninitialised value of size 8
==189168== at 0xA684D4: wrapper_handler (pqsignal.c:110)
==189168== by 0x580BB9E7: ??? (in /usr/libexec/valgrind/memcheck-arm64-linux)
==189168== Uninitialised value was created by a stack allocation
==189168== at 0x4F94660: strcoll_l (strcoll_l.c:258)
==189168==
{
<insert_a_suppression_name_here>
Memcheck:Value8
fun:wrapper_handler
obj:/usr/libexec/valgrind/memcheck-arm64-linux
}
**189168** Valgrind detected 3 error(s) during execution of "ANALYZE functional_dependencies;"
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-09 02:16 Nathan Bossart <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Bossart @ 2025-03-09 02:16 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers
On Sat, Mar 08, 2025 at 11:48:22PM +0100, Tomas Vondra wrote:
> Shortly after restarting this I got three more reports - all of them are
> related to strcoll_l. This is on c472a18296e4, i.e. with the asserts
> added in this thread etc. But none of those seem to fail.
> ==189168== at 0xA683CC: wrapper_handler (pqsignal.c:90)
> ==189168== at 0xA683F0: wrapper_handler (pqsignal.c:91)
> ==189168== at 0xA684D4: wrapper_handler (pqsignal.c:110)
This appears to refer to the following lines:
Assert(postgres_signal_arg > 0);
Assert(postgres_signal_arg < PG_NSIG);
(*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
The common ingredient seems to be postgres_signal_arg. I haven't found
anything else that seems helpful.
--
nathan
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: strange valgrind reports about wrapper_handler on 64-bit arm
@ 2025-03-09 17:11 Tomas Vondra <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Tomas Vondra @ 2025-03-09 17:11 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers
On 3/9/25 03:16, Nathan Bossart wrote:
> On Sat, Mar 08, 2025 at 11:48:22PM +0100, Tomas Vondra wrote:
>> Shortly after restarting this I got three more reports - all of them are
>> related to strcoll_l. This is on c472a18296e4, i.e. with the asserts
>> added in this thread etc. But none of those seem to fail.
>
>> ==189168== at 0xA683CC: wrapper_handler (pqsignal.c:90)
>> ==189168== at 0xA683F0: wrapper_handler (pqsignal.c:91)
>> ==189168== at 0xA684D4: wrapper_handler (pqsignal.c:110)
>
> This appears to refer to the following lines:
>
> Assert(postgres_signal_arg > 0);
> Assert(postgres_signal_arg < PG_NSIG);
> (*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
>
> The common ingredient seems to be postgres_signal_arg. I haven't found
> anything else that seems helpful.
>
Yeah, it's a bit weird. I got another report on the 64-bit rpi5 machine
(except for the command it's exactly the same), and then also this
report on the 32-bit machine:
---------------------------------------------------------------
==3482== Use of uninitialised value of size 4
==3482== at 0x991498: wrapper_handler (pqsignal.c:113)
==3482== by 0xFFFFFFFF: ???
==3482== Uninitialised value was created by a heap allocation
==3482== at 0x940EB8: palloc (mcxt.c:1341)
==3482== by 0x980037: initStringInfoInternal (stringinfo.c:45)
==3482== by 0x980103: initStringInfo (stringinfo.c:99)
==3482== by 0x7194B7: ReadArrayStr (arrayfuncs.c:613)
==3482== by 0x718803: array_in (arrayfuncs.c:289)
==3482== by 0x90024F: InputFunctionCallSafe (fmgr.c:1607)
==3482== by 0x2E346B: CopyFromTextLikeOneRow (copyfromparse.c:1029)
==3482== by 0x2E304B: CopyFromTextOneRow (copyfromparse.c:919)
==3482== by 0x2E2EEF: NextCopyFrom (copyfromparse.c:890)
==3482== by 0x2DF5C7: CopyFrom (copyfrom.c:1149)
==3482== by 0x2DAE33: DoCopy (copy.c:306)
==3482== by 0x6CC38F: standard_ProcessUtility (utility.c:738)
==3482== by 0x4B654C3: pgss_ProcessUtility (pg_stat_statements.c:1179)
==3482== by 0x6CBBF3: ProcessUtility (utility.c:519)
==3482== by 0x6CA24B: PortalRunUtility (pquery.c:1184)
==3482== by 0x6CA537: PortalRunMulti (pquery.c:1348)
==3482== by 0x6C9837: PortalRun (pquery.c:819)
==3482== by 0x6C1BEB: exec_simple_query (postgres.c:1272)
==3482== by 0x6C74FF: PostgresMain (postgres.c:4693)
==3482== by 0x6BD297: BackendMain (backend_startup.c:107)
==3482==
{
<insert_a_suppression_name_here>
Memcheck:Value4
fun:wrapper_handler
obj:*
}
**3482** Valgrind detected 1 error(s) during execution of "COPY
array_op_test FROM
'/home/debian/postgres/src/test/regress/data/array.data';"
---------------------------------------------------------------
This all seems very strange ... I'm starting to wonder if maybe this is
a valgrind issue. Both machines have valgrind 3.19, I'll try with a
custom build of 3.24 (the latest release).
regards
--
Tomas Vondra
^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2025-03-09 17:11 UTC | newest]
Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 03:11 [PATCH v6 3/7] Propagate changes to indisclustered to child/parents Justin Pryzby <[email protected]>
2025-03-06 23:03 strange valgrind reports about wrapper_handler on 64-bit arm Tomas Vondra <[email protected]>
2025-03-07 16:15 ` Re: strange valgrind reports about wrapper_handler on 64-bit arm Nathan Bossart <[email protected]>
2025-03-07 16:32 ` Re: strange valgrind reports about wrapper_handler on 64-bit arm Andres Freund <[email protected]>
2025-03-07 16:36 ` Re: strange valgrind reports about wrapper_handler on 64-bit arm Nathan Bossart <[email protected]>
2025-03-07 16:41 ` Re: strange valgrind reports about wrapper_handler on 64-bit arm Andres Freund <[email protected]>
2025-03-07 16:52 ` Re: strange valgrind reports about wrapper_handler on 64-bit arm Nathan Bossart <[email protected]>
2025-03-07 20:38 ` Re: strange valgrind reports about wrapper_handler on 64-bit arm Nathan Bossart <[email protected]>
2025-03-07 21:27 ` Re: strange valgrind reports about wrapper_handler on 64-bit arm Nathan Bossart <[email protected]>
2025-03-08 20:38 ` Re: strange valgrind reports about wrapper_handler on 64-bit arm Tomas Vondra <[email protected]>
2025-03-08 22:48 ` Re: strange valgrind reports about wrapper_handler on 64-bit arm Tomas Vondra <[email protected]>
2025-03-09 02:16 ` Re: strange valgrind reports about wrapper_handler on 64-bit arm Nathan Bossart <[email protected]>
2025-03-09 17:11 ` Re: strange valgrind reports about wrapper_handler on 64-bit arm Tomas Vondra <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox