agora inbox for [email protected]
help / color / mirror / Atom feed[PATCH v8 7/8] Preserve indisclustered on children of clustered, partitioned indexes
16+ messages / 8 participants
[nested] [flat]
* [PATCH v8 7/8] Preserve indisclustered on children of clustered, partitioned indexes
@ 2020-10-07 01:40 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Justin Pryzby @ 2020-10-07 01:40 UTC (permalink / raw)
Note, this takes a parentIndex, but that wasn't previously used ...
UpdateIndexRelation(Oid indexoid, Oid heapoid, Oid parentIndexId,
---
src/backend/catalog/index.c | 2 +-
src/test/regress/expected/cluster.out | 12 ++++++++++++
src/test/regress/sql/cluster.sql | 4 ++++
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1514937748..08fdf02ebc 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -604,7 +604,7 @@ UpdateIndexRelation(Oid indexoid,
values[Anum_pg_index_indisprimary - 1] = BoolGetDatum(primary);
values[Anum_pg_index_indisexclusion - 1] = BoolGetDatum(isexclusion);
values[Anum_pg_index_indimmediate - 1] = BoolGetDatum(immediate);
- values[Anum_pg_index_indisclustered - 1] = BoolGetDatum(false);
+ values[Anum_pg_index_indisclustered - 1] = BoolGetDatum(OidIsValid(parentIndexId) && get_index_isclustered(parentIndexId));
values[Anum_pg_index_indisvalid - 1] = BoolGetDatum(isvalid);
values[Anum_pg_index_indcheckxmin - 1] = BoolGetDatum(false);
values[Anum_pg_index_indisready - 1] = BoolGetDatum(isready);
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index e7f0889743..a3943c13f5 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -581,6 +581,18 @@ Indexes:
"clstrpart_idx" btree (a)
Number of partitions: 4 (Use \d+ to list them.)
+-- Check that new children inherit clustered mark
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+CREATE TABLE clstrpart4 PARTITION OF clstrpart FOR VALUES FROM (30)TO(40);
+\d clstrpart4
+ Table "public.clstrpart4"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart FOR VALUES FROM (30) TO (40)
+Indexes:
+ "clstrpart4_a_idx" btree (a) CLUSTER
+
-- 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 3c8085c69e..22628e90ca 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -244,6 +244,10 @@ ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
CREATE TABLE clstrpart5 (LIKE clstrpart INCLUDING INDEXES);
ALTER TABLE clstrpart ATTACH PARTITION clstrpart5 FOR VALUES FROM (40)TO(50);
\d clstrpart
+-- Check that new children inherit clustered mark
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+CREATE TABLE clstrpart4 PARTITION OF clstrpart FOR VALUES FROM (30)TO(40);
+\d clstrpart4
-- Test CLUSTER with external tuplesorting
--
2.17.0
--O5XBE6gyVG5Rl6Rj
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v8-0008-pg_dump-partitioned-index-depend-on-its-partition.patch"
^ permalink raw reply [nested|flat] 16+ messages in thread
* Debian 12 gcc warning
@ 2023-08-28 19:37 Bruce Momjian <[email protected]>
2023-08-28 22:30 ` Re: Debian 12 gcc warning Michael Paquier <[email protected]>
2023-08-28 23:55 ` Re: Debian 12 gcc warning David Rowley <[email protected]>
2023-08-29 05:17 ` Re: Debian 12 gcc warning Tristan Partin <[email protected]>
0 siblings, 3 replies; 16+ messages in thread
From: Bruce Momjian @ 2023-08-28 19:37 UTC (permalink / raw)
To: pgsql-hackers
On Debian 12, gcc version 12.2.0 (Debian 12.2.0-14) generates a warning
on PG 13 to current, but only with -O1 optimization level, and not at
-O0/-O2/-O3:
clauses.c: In function ‘recheck_cast_function_args’:
clauses.c:4293:19: warning: ‘actual_arg_types’ may be used uninitialized [-Wmaybe-uninitialized]
4293 | rettype = enforce_generic_type_consistency(actual_arg_types,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4294 | declared_arg_types,
| ~~~~~~~~~~~~~~~~~~~
4295 | nargs,
| ~~~~~~
4296 | funcform->prorettype,
| ~~~~~~~~~~~~~~~~~~~~~
4297 | false);
| ~~~~~~
In file included from clauses.c:45:
../../../../src/include/parser/parse_coerce.h:82:17: note: by argument 1 of type ‘const Oid *’ {aka ‘const unsigned int *’} to ‘enforce_generic_type_consistency’ declared here
82 | extern Oid enforce_generic_type_consistency(const Oid *actual_arg_types,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
clauses.c:4279:33: note: ‘actual_arg_types’ declared here
4279 | Oid actual_arg_types[FUNC_MAX_ARGS];
| ^~~~~~~~~~~~~~~~
The code is:
static void
recheck_cast_function_args(List *args, Oid result_type,
Oid *proargtypes, int pronargs,
HeapTuple func_tuple)
{
Form_pg_proc funcform = (Form_pg_proc) GETSTRUCT(func_tuple);
int nargs;
Oid actual_arg_types[FUNC_MAX_ARGS];
Oid declared_arg_types[FUNC_MAX_ARGS];
Oid rettype;
ListCell *lc;
if (list_length(args) > FUNC_MAX_ARGS)
elog(ERROR, "too many function arguments");
nargs = 0;
foreach(lc, args)
{
actual_arg_types[nargs++] = exprType((Node *) lfirst(lc));
}
Assert(nargs == pronargs);
memcpy(declared_arg_types, proargtypes, pronargs * sizeof(Oid));
--> rettype = enforce_generic_type_consistency(actual_arg_types,
declared_arg_types,
nargs,
funcform->prorettype,
false);
/* let's just check we got the same answer as the parser did ... */
I don't see a clean way of avoiding the warning except by initializing
the array, which seems wasteful.
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
@ 2023-08-28 22:30 ` Michael Paquier <[email protected]>
2023-08-28 23:10 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2023-08-28 22:30 UTC (permalink / raw)
To: Bruce Momjian <[email protected]>; +Cc: pgsql-hackers
On Mon, Aug 28, 2023 at 03:37:20PM -0400, Bruce Momjian wrote:
> I don't see a clean way of avoiding the warning except by initializing
> the array, which seems wasteful.
Or just initialize the array with a {0}?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../ZO0f9%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 22:30 ` Re: Debian 12 gcc warning Michael Paquier <[email protected]>
@ 2023-08-28 23:10 ` Bruce Momjian <[email protected]>
2023-08-29 01:00 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Bruce Momjian @ 2023-08-28 23:10 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers
On Tue, Aug 29, 2023 at 07:30:15AM +0900, Michael Paquier wrote:
> On Mon, Aug 28, 2023 at 03:37:20PM -0400, Bruce Momjian wrote:
> > I don't see a clean way of avoiding the warning except by initializing
> > the array, which seems wasteful.
>
> Or just initialize the array with a {0}?
Uh, doesn't that set all elements to zero? See:
https://stackoverflow.com/questions/2589749/how-to-initialize-array-to-0-in-c
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 22:30 ` Re: Debian 12 gcc warning Michael Paquier <[email protected]>
2023-08-28 23:10 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
@ 2023-08-29 01:00 ` Bruce Momjian <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Bruce Momjian @ 2023-08-29 01:00 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers
On Mon, Aug 28, 2023 at 07:10:38PM -0400, Bruce Momjian wrote:
> On Tue, Aug 29, 2023 at 07:30:15AM +0900, Michael Paquier wrote:
> > On Mon, Aug 28, 2023 at 03:37:20PM -0400, Bruce Momjian wrote:
> > > I don't see a clean way of avoiding the warning except by initializing
> > > the array, which seems wasteful.
> >
> > Or just initialize the array with a {0}?
>
> Uh, doesn't that set all elements to zero? See:
>
> https://stackoverflow.com/questions/2589749/how-to-initialize-array-to-0-in-c
FYI, that does stop the warning.
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
@ 2023-08-28 23:55 ` David Rowley <[email protected]>
2023-08-29 00:44 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-29 03:26 ` Re: Debian 12 gcc warning John Naylor <[email protected]>
2 siblings, 2 replies; 16+ messages in thread
From: David Rowley @ 2023-08-28 23:55 UTC (permalink / raw)
To: Bruce Momjian <[email protected]>; +Cc: pgsql-hackers
On Tue, 29 Aug 2023 at 07:37, Bruce Momjian <[email protected]> wrote:
> nargs = 0;
> foreach(lc, args)
> {
> actual_arg_types[nargs++] = exprType((Node *) lfirst(lc));
> }
Does it still produce the warning if you form the above more like?
nargs = list_length(args);
for (int i = 0; i < nargs; i++)
actual_arg_types[i] = exprType((Node *) list_nth(args, i));
I'm just not sure if it's unable to figure out if at least nargs
elements is set or if it won't be happy until all 100 elements are
set.
David
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 23:55 ` Re: Debian 12 gcc warning David Rowley <[email protected]>
@ 2023-08-29 00:44 ` Bruce Momjian <[email protected]>
1 sibling, 0 replies; 16+ messages in thread
From: Bruce Momjian @ 2023-08-29 00:44 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: pgsql-hackers
On Tue, Aug 29, 2023 at 11:55:48AM +1200, David Rowley wrote:
> On Tue, 29 Aug 2023 at 07:37, Bruce Momjian <[email protected]> wrote:
> > nargs = 0;
> > foreach(lc, args)
> > {
> > actual_arg_types[nargs++] = exprType((Node *) lfirst(lc));
> > }
>
> Does it still produce the warning if you form the above more like?
>
> nargs = list_length(args);
> for (int i = 0; i < nargs; i++)
> actual_arg_types[i] = exprType((Node *) list_nth(args, i));
>
> I'm just not sure if it's unable to figure out if at least nargs
> elements is set or if it won't be happy until all 100 elements are
> set.
I applied the attached patch but got the same warning:
clauses.c: In function ‘recheck_cast_function_args’:
clauses.c:4297:19: warning: ‘actual_arg_types’ may be used uninitialized [-Wmaybe-uninitialized]
4297 | rettype = enforce_generic_type_consistency(actual_arg_types,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4298 | declared_arg_types,
| ~~~~~~~~~~~~~~~~~~~
4299 | nargs,
| ~~~~~~
4300 | funcform->prorettype,
| ~~~~~~~~~~~~~~~~~~~~~
4301 | false);
| ~~~~~~
In file included from clauses.c:45:
../../../../src/include/parser/parse_coerce.h:82:17: note: by argument 1 of type ‘const Oid *’ {aka ‘const unsigned int *’} to ‘enforce_generic_type_consistency’ declared here
82 | extern Oid enforce_generic_type_consistency(const Oid *actual_arg_types,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
clauses.c:4279:33: note: ‘actual_arg_types’ declared here
4279 | Oid actual_arg_types[FUNC_MAX_ARGS];
| ^~~~~~~~~~~~~~~~
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
Attachments:
[text/x-diff] warning.diff (1.0K, ../../ZO0%[email protected]/2-warning.diff)
download | inline diff:
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index da258968b8..6cf020acef 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4279,15 +4279,19 @@ recheck_cast_function_args(List *args, Oid result_type,
Oid actual_arg_types[FUNC_MAX_ARGS];
Oid declared_arg_types[FUNC_MAX_ARGS];
Oid rettype;
- ListCell *lc;
+// ListCell *lc;
if (list_length(args) > FUNC_MAX_ARGS)
elog(ERROR, "too many function arguments");
- nargs = 0;
- foreach(lc, args)
- {
- actual_arg_types[nargs++] = exprType((Node *) lfirst(lc));
- }
+// nargs = 0;
+// foreach(lc, args)
+// {
+// actual_arg_types[nargs++] = exprType((Node *) lfirst(lc));
+// }
+ nargs = list_length(args);
+ for (int i = 0; i < nargs; i++)
+ actual_arg_types[i] = exprType((Node *) list_nth(args, i));
+
Assert(nargs == pronargs);
memcpy(declared_arg_types, proargtypes, pronargs * sizeof(Oid));
rettype = enforce_generic_type_consistency(actual_arg_types,
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 23:55 ` Re: Debian 12 gcc warning David Rowley <[email protected]>
@ 2023-08-29 03:26 ` John Naylor <[email protected]>
2023-08-29 13:27 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
1 sibling, 1 reply; 16+ messages in thread
From: John Naylor @ 2023-08-29 03:26 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Bruce Momjian <[email protected]>; pgsql-hackers
On Tue, Aug 29, 2023 at 6:56 AM David Rowley <[email protected]> wrote:
>
> I'm just not sure if it's unable to figure out if at least nargs
> elements is set or if it won't be happy until all 100 elements are
> set.
It looks like the former, since I can silence it on gcc 13 / -O1 by doing:
/* keep compiler quiet */
actual_arg_types[0] = InvalidOid;
--
John Naylor
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 23:55 ` Re: Debian 12 gcc warning David Rowley <[email protected]>
2023-08-29 03:26 ` Re: Debian 12 gcc warning John Naylor <[email protected]>
@ 2023-08-29 13:27 ` Bruce Momjian <[email protected]>
2023-08-29 14:18 ` Re: Debian 12 gcc warning Tom Lane <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Bruce Momjian @ 2023-08-29 13:27 UTC (permalink / raw)
To: John Naylor <[email protected]>; +Cc: David Rowley <[email protected]>; pgsql-hackers
On Tue, Aug 29, 2023 at 10:26:27AM +0700, John Naylor wrote:
>
> On Tue, Aug 29, 2023 at 6:56 AM David Rowley <[email protected]> wrote:
> >
> > I'm just not sure if it's unable to figure out if at least nargs
> > elements is set or if it won't be happy until all 100 elements are
> > set.
>
> It looks like the former, since I can silence it on gcc 13 / -O1 by doing:
>
> /* keep compiler quiet */
> actual_arg_types[0] = InvalidOid;
Agreed, that fixes it for me too. In fact, assigning to only element 99 or
200 also prevents the warning, and considering the array is defined for
100 elements, the fact is accepts 200 isn't a good thing. Patch attached.
I think the question is whether we add this to silence a common compiler
but non-default optimization level. It is the only such case in our
source code right now.
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
Attachments:
[text/x-diff] warning.diff (557B, ../../[email protected]/2-warning.diff)
download | inline diff:
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index da258968b8..f4a1d1049c 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4284,6 +4284,10 @@ recheck_cast_function_args(List *args, Oid result_type,
if (list_length(args) > FUNC_MAX_ARGS)
elog(ERROR, "too many function arguments");
nargs = 0;
+
+ /* Silence gcc 12 compiler at -O1. */
+ actual_arg_types[0] = InvalidOid;
+
foreach(lc, args)
{
actual_arg_types[nargs++] = exprType((Node *) lfirst(lc));
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 23:55 ` Re: Debian 12 gcc warning David Rowley <[email protected]>
2023-08-29 03:26 ` Re: Debian 12 gcc warning John Naylor <[email protected]>
2023-08-29 13:27 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
@ 2023-08-29 14:18 ` Tom Lane <[email protected]>
2023-08-30 02:52 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Tom Lane @ 2023-08-29 14:18 UTC (permalink / raw)
To: Bruce Momjian <[email protected]>; +Cc: John Naylor <[email protected]>; David Rowley <[email protected]>; pgsql-hackers
Bruce Momjian <[email protected]> writes:
> On Tue, Aug 29, 2023 at 10:26:27AM +0700, John Naylor wrote:
>> It looks like the former, since I can silence it on gcc 13 / -O1 by doing:
>> /* keep compiler quiet */
>> actual_arg_types[0] = InvalidOid;
> Agreed, that fixes it for me too. In fact, assigning to only element 99 or
> 200 also prevents the warning, and considering the array is defined for
> 100 elements, the fact is accepts 200 isn't a good thing. Patch attached.
That seems like a pretty clear compiler bug, particularly since it just
appears in this one version. Rather than contorting our code, I'd
suggest filing a gcc bug.
regards, tom lane
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 23:55 ` Re: Debian 12 gcc warning David Rowley <[email protected]>
2023-08-29 03:26 ` Re: Debian 12 gcc warning John Naylor <[email protected]>
2023-08-29 13:27 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-29 14:18 ` Re: Debian 12 gcc warning Tom Lane <[email protected]>
@ 2023-08-30 02:52 ` Bruce Momjian <[email protected]>
2023-08-30 03:30 ` Re: Debian 12 gcc warning Tom Lane <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Bruce Momjian @ 2023-08-30 02:52 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: John Naylor <[email protected]>; David Rowley <[email protected]>; pgsql-hackers
On Tue, Aug 29, 2023 at 10:18:36AM -0400, Tom Lane wrote:
> Bruce Momjian <[email protected]> writes:
> > On Tue, Aug 29, 2023 at 10:26:27AM +0700, John Naylor wrote:
> >> It looks like the former, since I can silence it on gcc 13 / -O1 by doing:
> >> /* keep compiler quiet */
> >> actual_arg_types[0] = InvalidOid;
>
> > Agreed, that fixes it for me too. In fact, assigning to only element 99 or
> > 200 also prevents the warning, and considering the array is defined for
> > 100 elements, the fact is accepts 200 isn't a good thing. Patch attached.
>
> That seems like a pretty clear compiler bug, particularly since it just
> appears in this one version. Rather than contorting our code, I'd
> suggest filing a gcc bug.
I assume I have to create a test case to report this to the gcc team. I
tried to create such a test case on gcc 12 but it doesn't generate the
warning. Attached is my attempt. Any ideas? I assume we can't just
tell them to download our software and compile it.
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
Attachments:
[application/zip] test_gcc_12.2.0-O1.zip (6.4K, ../../ZO6u1hySX4lBI%[email protected]/2-test_gcc_12.2.0-O1.zip)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 23:55 ` Re: Debian 12 gcc warning David Rowley <[email protected]>
2023-08-29 03:26 ` Re: Debian 12 gcc warning John Naylor <[email protected]>
2023-08-29 13:27 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-29 14:18 ` Re: Debian 12 gcc warning Tom Lane <[email protected]>
2023-08-30 02:52 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
@ 2023-08-30 03:30 ` Tom Lane <[email protected]>
2023-08-30 15:16 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Tom Lane @ 2023-08-30 03:30 UTC (permalink / raw)
To: Bruce Momjian <[email protected]>; +Cc: John Naylor <[email protected]>; David Rowley <[email protected]>; pgsql-hackers
Bruce Momjian <[email protected]> writes:
> On Tue, Aug 29, 2023 at 10:18:36AM -0400, Tom Lane wrote:
>> That seems like a pretty clear compiler bug, particularly since it just
>> appears in this one version. Rather than contorting our code, I'd
>> suggest filing a gcc bug.
> I assume I have to create a test case to report this to the gcc team. I
> tried to create such a test case on gcc 12 but it doesn't generate the
> warning. Attached is my attempt. Any ideas? I assume we can't just
> tell them to download our software and compile it.
IIRC, they'll accept preprocessed compiler input for the specific file;
you don't need to provide a complete source tree. Per
https://gcc.gnu.org/bugs/
Please include all of the following items, the first three of which can be obtained from the output of gcc -v:
the exact version of GCC;
the system type;
the options given when GCC was configured/built;
the complete command line that triggers the bug;
the compiler output (error messages, warnings, etc.); and
the preprocessed file (*.i*) that triggers the bug, generated by adding -save-temps to the complete compilation command, or, in the case of a bug report for the GNAT front end, a complete set of source files (see below).
Obviously, if you can trim the input it's good, but it doesn't
have to be a minimal reproducer.
regards, tom lane
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 23:55 ` Re: Debian 12 gcc warning David Rowley <[email protected]>
2023-08-29 03:26 ` Re: Debian 12 gcc warning John Naylor <[email protected]>
2023-08-29 13:27 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-29 14:18 ` Re: Debian 12 gcc warning Tom Lane <[email protected]>
2023-08-30 02:52 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-30 03:30 ` Re: Debian 12 gcc warning Tom Lane <[email protected]>
@ 2023-08-30 15:16 ` Bruce Momjian <[email protected]>
2023-08-30 15:34 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Bruce Momjian @ 2023-08-30 15:16 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: John Naylor <[email protected]>; David Rowley <[email protected]>; pgsql-hackers
On Tue, Aug 29, 2023 at 11:30:06PM -0400, Tom Lane wrote:
> Bruce Momjian <[email protected]> writes:
> > On Tue, Aug 29, 2023 at 10:18:36AM -0400, Tom Lane wrote:
> >> That seems like a pretty clear compiler bug, particularly since it just
> >> appears in this one version. Rather than contorting our code, I'd
> >> suggest filing a gcc bug.
>
> > I assume I have to create a test case to report this to the gcc team. I
> > tried to create such a test case on gcc 12 but it doesn't generate the
> > warning. Attached is my attempt. Any ideas? I assume we can't just
> > tell them to download our software and compile it.
>
> IIRC, they'll accept preprocessed compiler input for the specific file;
> you don't need to provide a complete source tree. Per
> https://gcc.gnu.org/bugs/
>
> Please include all of the following items, the first three of which can be obtained from the output of gcc -v:
>
> the exact version of GCC;
> the system type;
> the options given when GCC was configured/built;
> the complete command line that triggers the bug;
> the compiler output (error messages, warnings, etc.); and
> the preprocessed file (*.i*) that triggers the bug, generated by adding -save-temps to the complete compilation command, or, in the case of a bug report for the GNAT front end, a complete set of source files (see below).
>
> Obviously, if you can trim the input it's good, but it doesn't
> have to be a minimal reproducer.
Bug submitted, thanks for th preprocessed file tip.
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 23:55 ` Re: Debian 12 gcc warning David Rowley <[email protected]>
2023-08-29 03:26 ` Re: Debian 12 gcc warning John Naylor <[email protected]>
2023-08-29 13:27 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-29 14:18 ` Re: Debian 12 gcc warning Tom Lane <[email protected]>
2023-08-30 02:52 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-30 03:30 ` Re: Debian 12 gcc warning Tom Lane <[email protected]>
2023-08-30 15:16 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
@ 2023-08-30 15:34 ` Bruce Momjian <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Bruce Momjian @ 2023-08-30 15:34 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: John Naylor <[email protected]>; David Rowley <[email protected]>; pgsql-hackers
On Wed, Aug 30, 2023 at 11:16:48AM -0400, Bruce Momjian wrote:
> On Tue, Aug 29, 2023 at 11:30:06PM -0400, Tom Lane wrote:
> > Bruce Momjian <[email protected]> writes:
> > > On Tue, Aug 29, 2023 at 10:18:36AM -0400, Tom Lane wrote:
> > >> That seems like a pretty clear compiler bug, particularly since it just
> > >> appears in this one version. Rather than contorting our code, I'd
> > >> suggest filing a gcc bug.
> >
> > > I assume I have to create a test case to report this to the gcc team. I
> > > tried to create such a test case on gcc 12 but it doesn't generate the
> > > warning. Attached is my attempt. Any ideas? I assume we can't just
> > > tell them to download our software and compile it.
> >
> > IIRC, they'll accept preprocessed compiler input for the specific file;
> > you don't need to provide a complete source tree. Per
> > https://gcc.gnu.org/bugs/
> >
> > Please include all of the following items, the first three of which can be obtained from the output of gcc -v:
> >
> > the exact version of GCC;
> > the system type;
> > the options given when GCC was configured/built;
> > the complete command line that triggers the bug;
> > the compiler output (error messages, warnings, etc.); and
> > the preprocessed file (*.i*) that triggers the bug, generated by adding -save-temps to the complete compilation command, or, in the case of a bug report for the GNAT front end, a complete set of source files (see below).
> >
> > Obviously, if you can trim the input it's good, but it doesn't
> > have to be a minimal reproducer.
>
> Bug submitted, thanks for th preprocessed file tip.
Good news, I was able to prevent the bug by causing compiling of
clauses.c to use -O2 by adding this to src/Makefile.custom:
clauses.o : CFLAGS+=-O2
Here is my submitted bug report:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111240
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Debian 12 gcc warning
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
@ 2023-08-29 05:17 ` Tristan Partin <[email protected]>
2 siblings, 0 replies; 16+ messages in thread
From: Tristan Partin @ 2023-08-29 05:17 UTC (permalink / raw)
To: Bruce Momjian <[email protected]>; +Cc: pgsql-hackers
On Mon Aug 28, 2023 at 2:37 PM CDT, Bruce Momjian wrote:
> I don't see a clean way of avoiding the warning except by initializing
> the array, which seems wasteful.
For what it's worth, we recently committed a patch[0] that initialized
an array due to a similar warning being generated on Fedora 38 (gcc
(GCC) 13.2.1 20230728 (Red Hat 13.2.1-1)).
[0]: https://github.com/postgres/postgres/commit/4a8fef0d733965c1a1836022f8a42ab1e83a721f
--
Tristan Partin
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v9 04/10] lwlock: Invert meaning of LW_FLAG_RELEASE_OK
@ 2026-01-06 01:40 Andres Freund <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Andres Freund @ 2026-01-06 01:40 UTC (permalink / raw)
Instead of setting a flag whenever a lock release is supposed to wake up
waiters - the majority of the time - set a flag whenever wakeups are
inhibited. The motivation for this that in an upcoming commit, buffer content
locks are implemented separately from lwlocks, and for buffer content locks it
is useful to be able to reset all buffer flags when a buffer invalidated,
alternatively we would have to set the release-ok flag when making a buffer
valid. It seems good to keep the implementation of lwlocks and buffer content
locks as similar as reasonably possible.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/storage/lmgr/lwlock.c | 42 +++++++++++++++----------------
1 file changed, 20 insertions(+), 22 deletions(-)
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 6a9f86d5025..148309cc186 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -92,7 +92,7 @@
#define LW_FLAG_HAS_WAITERS ((uint32) 1 << 31)
-#define LW_FLAG_RELEASE_OK ((uint32) 1 << 30)
+#define LW_FLAG_WAKE_IN_PROGRESS ((uint32) 1 << 30)
#define LW_FLAG_LOCKED ((uint32) 1 << 29)
#define LW_FLAG_BITS 3
#define LW_FLAG_MASK (((1<<LW_FLAG_BITS)-1)<<(32-LW_FLAG_BITS))
@@ -246,14 +246,14 @@ PRINT_LWDEBUG(const char *where, LWLock *lock, LWLockMode mode)
ereport(LOG,
(errhidestmt(true),
errhidecontext(true),
- errmsg_internal("%d: %s(%s %p): excl %u shared %u haswaiters %u waiters %u rOK %d",
+ errmsg_internal("%d: %s(%s %p): excl %u shared %u haswaiters %u waiters %u waking %d",
MyProcPid,
where, T_NAME(lock), lock,
(state & LW_VAL_EXCLUSIVE) != 0,
state & LW_SHARED_MASK,
(state & LW_FLAG_HAS_WAITERS) != 0,
pg_atomic_read_u32(&lock->nwaiters),
- (state & LW_FLAG_RELEASE_OK) != 0)));
+ (state & LW_FLAG_WAKE_IN_PROGRESS) != 0)));
}
}
@@ -700,7 +700,7 @@ LWLockInitialize(LWLock *lock, int tranche_id)
/* verify the tranche_id is valid */
(void) GetLWTrancheName(tranche_id);
- pg_atomic_init_u32(&lock->state, LW_FLAG_RELEASE_OK);
+ pg_atomic_init_u32(&lock->state, 0);
#ifdef LOCK_DEBUG
pg_atomic_init_u32(&lock->nwaiters, 0);
#endif
@@ -929,15 +929,13 @@ LWLockWaitListUnlock(LWLock *lock)
static void
LWLockWakeup(LWLock *lock)
{
- bool new_release_ok;
+ bool new_release_in_progress = false;
bool wokeup_somebody = false;
proclist_head wakeup;
proclist_mutable_iter iter;
proclist_init(&wakeup);
- new_release_ok = true;
-
/* lock wait list while collecting backends to wake up */
LWLockWaitListLock(lock);
@@ -958,7 +956,7 @@ LWLockWakeup(LWLock *lock)
* that are just waiting for the lock to become free don't retry
* automatically.
*/
- new_release_ok = false;
+ new_release_in_progress = true;
/*
* Don't wakeup (further) exclusive locks.
@@ -997,10 +995,10 @@ LWLockWakeup(LWLock *lock)
/* compute desired flags */
- if (new_release_ok)
- desired_state |= LW_FLAG_RELEASE_OK;
+ if (new_release_in_progress)
+ desired_state |= LW_FLAG_WAKE_IN_PROGRESS;
else
- desired_state &= ~LW_FLAG_RELEASE_OK;
+ desired_state &= ~LW_FLAG_WAKE_IN_PROGRESS;
if (proclist_is_empty(&lock->waiters))
desired_state &= ~LW_FLAG_HAS_WAITERS;
@@ -1131,10 +1129,10 @@ LWLockDequeueSelf(LWLock *lock)
*/
/*
- * Reset RELEASE_OK flag if somebody woke us before we removed
- * ourselves - they'll have set it to false.
+ * Clear LW_FLAG_WAKE_IN_PROGRESS if somebody woke us before we
+ * removed ourselves - they'll have set it.
*/
- pg_atomic_fetch_or_u32(&lock->state, LW_FLAG_RELEASE_OK);
+ pg_atomic_fetch_and_u32(&lock->state, ~LW_FLAG_WAKE_IN_PROGRESS);
/*
* Now wait for the scheduled wakeup, otherwise our ->lwWaiting would
@@ -1301,7 +1299,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
}
/* Retrying, allow LWLockRelease to release waiters again. */
- pg_atomic_fetch_or_u32(&lock->state, LW_FLAG_RELEASE_OK);
+ pg_atomic_fetch_and_u32(&lock->state, ~LW_FLAG_WAKE_IN_PROGRESS);
#ifdef LOCK_DEBUG
{
@@ -1636,10 +1634,10 @@ LWLockWaitForVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 oldval,
LWLockQueueSelf(lock, LW_WAIT_UNTIL_FREE);
/*
- * Set RELEASE_OK flag, to make sure we get woken up as soon as the
- * lock is released.
+ * Clear LW_FLAG_WAKE_IN_PROGRESS flag, to make sure we get woken up
+ * as soon as the lock is released.
*/
- pg_atomic_fetch_or_u32(&lock->state, LW_FLAG_RELEASE_OK);
+ pg_atomic_fetch_and_u32(&lock->state, ~LW_FLAG_WAKE_IN_PROGRESS);
/*
* We're now guaranteed to be woken up if necessary. Recheck the lock
@@ -1852,11 +1850,11 @@ LWLockReleaseInternal(LWLock *lock, LWLockMode mode)
TRACE_POSTGRESQL_LWLOCK_RELEASE(T_NAME(lock));
/*
- * We're still waiting for backends to get scheduled, don't wake them up
- * again.
+ * Check if we're still waiting for backends to get scheduled, if so,
+ * don't wake them up again.
*/
- if ((oldstate & (LW_FLAG_HAS_WAITERS | LW_FLAG_RELEASE_OK)) ==
- (LW_FLAG_HAS_WAITERS | LW_FLAG_RELEASE_OK) &&
+ if ((oldstate & LW_FLAG_HAS_WAITERS) &&
+ !(oldstate & LW_FLAG_WAKE_IN_PROGRESS) &&
(oldstate & LW_LOCK_MASK) == 0)
check_waiters = true;
else
--
2.48.1.76.g4e746b1a31.dirty
--rkiyqpij3ajqn7ww
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0005-bufmgr-Make-definitions-related-to-buffer-descrip.patch"
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2026-01-06 01:40 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 01:40 [PATCH v8 7/8] Preserve indisclustered on children of clustered, partitioned indexes Justin Pryzby <[email protected]>
2023-08-28 19:37 Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 22:30 ` Re: Debian 12 gcc warning Michael Paquier <[email protected]>
2023-08-28 23:10 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-29 01:00 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-28 23:55 ` Re: Debian 12 gcc warning David Rowley <[email protected]>
2023-08-29 00:44 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-29 03:26 ` Re: Debian 12 gcc warning John Naylor <[email protected]>
2023-08-29 13:27 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-29 14:18 ` Re: Debian 12 gcc warning Tom Lane <[email protected]>
2023-08-30 02:52 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-30 03:30 ` Re: Debian 12 gcc warning Tom Lane <[email protected]>
2023-08-30 15:16 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-30 15:34 ` Re: Debian 12 gcc warning Bruce Momjian <[email protected]>
2023-08-29 05:17 ` Re: Debian 12 gcc warning Tristan Partin <[email protected]>
2026-01-06 01:40 [PATCH v9 04/10] lwlock: Invert meaning of LW_FLAG_RELEASE_OK Andres Freund <[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