pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedsilence compiler warning in brin.c
8+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2022-06-01 dd1c8dd101 dd672be38d 37290f7f5a c051180147 60ca2e8418 4e46726156 Silence compiler warnings from some older compilers.
* silence compiler warning in brin.c
@ 2022-06-01 16:35 Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Nathan Bossart @ 2022-06-01 16:35 UTC (permalink / raw)
To: pgsql-hackers
Hi hackers,
I'm seeing a compiler warning in brin.c with an older version of gcc.
Specifically, it seems worried that a variable might not be initialized.
AFAICT there is no real risk, so I've attached a small patch to silence the
warning.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
silence_compiler_warning.patch (text/x-diff, 426B) download
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 52f171772d..5b564f9507 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1010,7 +1010,7 @@ brin_summarize_range(PG_FUNCTION_ARGS)
Relation heapRel;
Oid save_userid;
int save_sec_context;
- int save_nestlevel;
+ int save_nestlevel = -1;
double numSummarized = 0;
if (RecoveryInProgress())
* Re: silence compiler warning in brin.c
@ 2022-06-01 16:46 Zhihong Yu <zyu@yugabyte.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Zhihong Yu @ 2022-06-01 16:46 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
On Wed, Jun 1, 2022 at 9:35 AM Nathan Bossart <nathandbossart@gmail.com>
wrote:
> Hi hackers,
>
> I'm seeing a compiler warning in brin.c with an older version of gcc.
> Specifically, it seems worried that a variable might not be initialized.
> AFAICT there is no real risk, so I've attached a small patch to silence the
> warning.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
Hi,
It seems the variable can be initialized to the value of GUCNestLevel since
later in the func:
/* Roll back any GUC changes executed by index functions */
AtEOXact_GUC(false, save_nestlevel);
Cheers
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: silence compiler warning in brin.c
@ 2022-06-01 17:06 Tom Lane <tgl@sss.pgh.pa.us>
parent: Zhihong Yu <zyu@yugabyte.com>
0 siblings, 2 replies; 8+ messages in thread
From: Tom Lane @ 2022-06-01 17:06 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
Zhihong Yu <zyu@yugabyte.com> writes:
> On Wed, Jun 1, 2022 at 9:35 AM Nathan Bossart <nathandbossart@gmail.com>
> wrote:
>> I'm seeing a compiler warning in brin.c with an older version of gcc.
>> Specifically, it seems worried that a variable might not be initialized.
>> AFAICT there is no real risk, so I've attached a small patch to silence the
>> warning.
Yeah, I noticed the other day that a couple of older buildfarm members
(curculio, gaur) are grousing about this too. We don't really have a
hard-n-fast rule about how old a compiler needs to be before we stop
worrying about its notions about uninitialized variables, but these are
kind of old. Still, since this is the only such warning from these
animals, I'm inclined to silence it.
> It seems the variable can be initialized to the value of GUCNestLevel since
> later in the func:
> /* Roll back any GUC changes executed by index functions */
> AtEOXact_GUC(false, save_nestlevel);
That seems pretty inappropriate. If, thanks to some future thinko,
control were able to reach the AtEOXact_GUC call despite not having
called NewGUCNestLevel, we'd want that to fail. It looks like
AtEOXact_GUC asserts nestLevel > 0, so that either 0 or -1 would
do as an "invalid" value; I'd lean a bit to using 0.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: silence compiler warning in brin.c
@ 2022-06-01 17:38 Nathan Bossart <nathandbossart@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 8+ messages in thread
From: Nathan Bossart @ 2022-06-01 17:38 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Zhihong Yu <zyu@yugabyte.com>; pgsql-hackers
On Wed, Jun 01, 2022 at 01:06:21PM -0400, Tom Lane wrote:
> Zhihong Yu <zyu@yugabyte.com> writes:
>> It seems the variable can be initialized to the value of GUCNestLevel since
>> later in the func:
>> /* Roll back any GUC changes executed by index functions */
>> AtEOXact_GUC(false, save_nestlevel);
>
> That seems pretty inappropriate. If, thanks to some future thinko,
> control were able to reach the AtEOXact_GUC call despite not having
> called NewGUCNestLevel, we'd want that to fail.
+1
> It looks like
> AtEOXact_GUC asserts nestLevel > 0, so that either 0 or -1 would
> do as an "invalid" value; I'd lean a bit to using 0.
I only chose -1 to follow a117ceb's example in amcheck. I have no
preference.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: silence compiler warning in brin.c
@ 2022-06-01 17:55 Tom Lane <tgl@sss.pgh.pa.us>
parent: Zhihong Yu <zyu@yugabyte.com>
0 siblings, 0 replies; 8+ messages in thread
From: Tom Lane @ 2022-06-01 17:55 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
Zhihong Yu <zyu@yugabyte.com> writes:
> Hi,
> if (heapRel == NULL || heapoid != IndexGetRelation(indexoid, false))
> ereport(ERROR,
> I wonder why the above check is not placed in the else block:
> else
> heapRel = NULL;
Because we don't want to throw that error until we've exhausted the
possibilities for throwing other errors.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: silence compiler warning in brin.c
@ 2022-06-01 17:58 Zhihong Yu <zyu@yugabyte.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 8+ messages in thread
From: Zhihong Yu @ 2022-06-01 17:58 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
On Wed, Jun 1, 2022 at 10:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Zhihong Yu <zyu@yugabyte.com> writes:
> > On Wed, Jun 1, 2022 at 9:35 AM Nathan Bossart <nathandbossart@gmail.com>
> > wrote:
> >> I'm seeing a compiler warning in brin.c with an older version of gcc.
> >> Specifically, it seems worried that a variable might not be initialized.
> >> AFAICT there is no real risk, so I've attached a small patch to silence
> the
> >> warning.
>
> Yeah, I noticed the other day that a couple of older buildfarm members
> (curculio, gaur) are grousing about this too. We don't really have a
> hard-n-fast rule about how old a compiler needs to be before we stop
> worrying about its notions about uninitialized variables, but these are
> kind of old. Still, since this is the only such warning from these
> animals, I'm inclined to silence it.
>
> > It seems the variable can be initialized to the value of GUCNestLevel
> since
> > later in the func:
> > /* Roll back any GUC changes executed by index functions */
> > AtEOXact_GUC(false, save_nestlevel);
>
> That seems pretty inappropriate. If, thanks to some future thinko,
> control were able to reach the AtEOXact_GUC call despite not having
> called NewGUCNestLevel, we'd want that to fail. It looks like
> AtEOXact_GUC asserts nestLevel > 0, so that either 0 or -1 would
> do as an "invalid" value; I'd lean a bit to using 0.
>
> regards, tom lane
>
Hi,
if (heapRel == NULL || heapoid != IndexGetRelation(indexoid, false))
ereport(ERROR,
I wonder why the above check is not placed in the else block:
else
heapRel = NULL;
because heapRel is not modified between the else and the above check.
If the check is placed in the else block, we can potentially save the call
to index_open().
Cheers
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: silence compiler warning in brin.c
@ 2022-06-01 21:08 Tom Lane <tgl@sss.pgh.pa.us>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2022-06-01 21:08 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Zhihong Yu <zyu@yugabyte.com>; pgsql-hackers
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Wed, Jun 01, 2022 at 01:06:21PM -0400, Tom Lane wrote:
>> It looks like
>> AtEOXact_GUC asserts nestLevel > 0, so that either 0 or -1 would
>> do as an "invalid" value; I'd lean a bit to using 0.
> I only chose -1 to follow a117ceb's example in amcheck. I have no
> preference.
Hmm, if we're following amcheck's example it should be more like this:
inline patch 1: src/backend/access/brin/brin.c (+6/-0 lines) download inline-1.patch
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 52f171772d..0de1441dc6 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1051,7 +1051,13 @@ brin_summarize_range(PG_FUNCTION_ARGS)
save_nestlevel = NewGUCNestLevel();
}
else
+ {
heapRel = NULL;
+ /* Set these just to suppress "uninitialized variable" warnings */
+ save_userid = InvalidOid;
+ save_sec_context = -1;
+ save_nestlevel = -1;
+ }
indexRel = index_open(indexoid, ShareUpdateExclusiveLock);
* Re: silence compiler warning in brin.c
@ 2022-06-01 21:30 Nathan Bossart <nathandbossart@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 8+ messages in thread
From: Nathan Bossart @ 2022-06-01 21:30 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Zhihong Yu <zyu@yugabyte.com>; pgsql-hackers
On Wed, Jun 01, 2022 at 05:08:03PM -0400, Tom Lane wrote:
> Hmm, if we're following amcheck's example it should be more like this:
>
> diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
> index 52f171772d..0de1441dc6 100644
> --- a/src/backend/access/brin/brin.c
> +++ b/src/backend/access/brin/brin.c
> @@ -1051,7 +1051,13 @@ brin_summarize_range(PG_FUNCTION_ARGS)
> save_nestlevel = NewGUCNestLevel();
> }
> else
> + {
> heapRel = NULL;
> + /* Set these just to suppress "uninitialized variable" warnings */
> + save_userid = InvalidOid;
> + save_sec_context = -1;
> + save_nestlevel = -1;
> + }
>
> indexRel = index_open(indexoid, ShareUpdateExclusiveLock);
>
> I like this better anyway since the fact that the other two variables
> aren't warned about seems like an implementation artifact.
Yeah, that is better. It's not clear why the other variables aren't
subject to the same warnings, so we might as well cover our bases.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2022-06-01 21:30 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-06-01 16:35 silence compiler warning in brin.c Nathan Bossart <nathandbossart@gmail.com>
2022-06-01 16:46 ` Zhihong Yu <zyu@yugabyte.com>
2022-06-01 17:06 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-06-01 17:38 ` Nathan Bossart <nathandbossart@gmail.com>
2022-06-01 21:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-06-01 21:30 ` Nathan Bossart <nathandbossart@gmail.com>
2022-06-01 17:58 ` Zhihong Yu <zyu@yugabyte.com>
2022-06-01 17:55 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox