Thread: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
From
Ranier Vilela
Date:
Hi,
Per Coverity.
It seems to me that some recent commit has failed to properly initialize a structure,
in extended_stats.c, when is passed to heap_copytuple.
regards,
Ranier Vilela
Attachment
Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
From
Justin Pryzby
Date:
On Sun, Apr 11, 2021 at 03:38:10PM -0300, Ranier Vilela wrote:
> Per Coverity.
>
> It seems to me that some recent commit has failed to properly initialize a
> structure, in extended_stats.c, when is passed to heap_copytuple.
I think you're right. You can look in the commit history to find the relevant
commit and copy the committer.
I think it's cleanest to write:
|HeapTupleData tmptup = {0};
--
Justin
Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
From
Ranier Vilela
Date:
Hi Justin, sorry for the delay.
Nothing against it, but I looked for similar codes and this is the usual way to initialize HeapTupleData.
Perhaps InvalidOid makes a difference.
regards,
Ranier Vilela
Em dom., 11 de abr. de 2021 às 16:25, Justin Pryzby <pryzby@telsasoft.com> escreveu:
On Sun, Apr 11, 2021 at 03:38:10PM -0300, Ranier Vilela wrote:
> Per Coverity.
>
> It seems to me that some recent commit has failed to properly initialize a
> structure, in extended_stats.c, when is passed to heap_copytuple.
I think you're right. You can look in the commit history to find the relevant
commit and copy the committer.
I think it's cleanest to write:
|HeapTupleData tmptup = {0};
--
Justin
Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
From
Michael Paquier
Date:
On Sun, Apr 11, 2021 at 07:42:20PM -0300, Ranier Vilela wrote:
> Em dom., 11 de abr. de 2021 às 16:25, Justin Pryzby <pryzby@telsasoft.com>
> escreveu:
>> I think you're right. You can look in the commit history to find the
>> relevant
>> commit and copy the committer.
In this case that's a4d75c8, for Tomas.
>> I think it's cleanest to write:
>> |HeapTupleData tmptup = {0};
I agree that this would be cleaner.
While on it, if you could not top-post..
--
Michael
Attachment
Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
From
Tom Lane
Date:
Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Apr 11, 2021 at 07:42:20PM -0300, Ranier Vilela wrote:
>> Em dom., 11 de abr. de 2021 às 16:25, Justin Pryzby <pryzby@telsasoft.com>
>>> I think it's cleanest to write:
>>> |HeapTupleData tmptup = {0};
> I agree that this would be cleaner.
It would be wrong, though, or at least not have the same effect.
ItemPointerSetInvalid does not set the target to all-zeroes.
(Regardless of that detail, it's generally best to accomplish
objective X in the same way that existing code does. Deciding
that you have a better way is often wrong, and even if you
are right, you should then submit a patch to change all the
existing cases.)
regards, tom lane
Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
From
Ranier Vilela
Date:
Em seg., 12 de abr. de 2021 às 03:04, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Apr 11, 2021 at 07:42:20PM -0300, Ranier Vilela wrote:
>> Em dom., 11 de abr. de 2021 às 16:25, Justin Pryzby <pryzby@telsasoft.com>
>>> I think it's cleanest to write:
>>> |HeapTupleData tmptup = {0};
> I agree that this would be cleaner.
It would be wrong, though, or at least not have the same effect.
I think that you speak about fill pointers with 0 is not the same as fill pointers with NULL.
ItemPointerSetInvalid does not set the target to all-zeroes.
ItemPointerSetInvalid set or not set the target to all-zeroes?
(Regardless of that detail, it's generally best to accomplish
objective X in the same way that existing code does. Deciding
that you have a better way is often wrong, and even if you
are right, you should then submit a patch to change all the
existing cases.)
I was confused here, does the patch follow the pattern and fix the problem or not?
regards,
Ranier Vilela
Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
From
Tomas Vondra
Date:
On 4/12/21 6:55 PM, Ranier Vilela wrote:
>
>
> Em seg., 12 de abr. de 2021 às 03:04, Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>> escreveu:
>
> Michael Paquier <michael@paquier.xyz <mailto:michael@paquier.xyz>>
> writes:
> > On Sun, Apr 11, 2021 at 07:42:20PM -0300, Ranier Vilela wrote:
> >> Em dom., 11 de abr. de 2021 às 16:25, Justin Pryzby
> <pryzby@telsasoft.com <mailto:pryzby@telsasoft.com>>
> >>> I think it's cleanest to write:
> >>> |HeapTupleData tmptup = {0};
>
> > I agree that this would be cleaner.
>
> It would be wrong, though, or at least not have the same effect.
>
> I think that you speak about fill pointers with 0 is not the same as
> fill pointers with NULL.
>
>
> ItemPointerSetInvalid does not set the target to all-zeroes.
>
> ItemPointerSetInvalid set or not set the target to all-zeroes?
>
Not sure what exactly are you asking about? What Tom said is that if you
do 'struct = {0}' it sets all fields to 0, but we only want/need to set
the t_self/t_tableOid fields to 0.
>
> (Regardless of that detail, it's generally best to accomplish
> objective X in the same way that existing code does. Deciding
> that you have a better way is often wrong, and even if you
> are right, you should then submit a patch to change all the
> existing cases.)
>
> I was confused here, does the patch follow the pattern and fix the
> problem or not?
>
I believe it does, and it's doing it in the same way as most other
similar places.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
From
Tom Lane
Date:
Ranier Vilela <ranier.vf@gmail.com> writes:
> Em seg., 12 de abr. de 2021 às 03:04, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
>> It would be wrong, though, or at least not have the same effect.
> I think that you speak about fill pointers with 0 is not the same as fill
> pointers with NULL.
No, I mean that InvalidBlockNumber isn't 0.
> I was confused here, does the patch follow the pattern and fix the problem
> or not?
Your patch seems fine. Justin's proposed improvement isn't.
(I'm not real sure whether there's any *actual* bug here --- would we
really be looking at either ctid or tableoid of this temporary tuple?
But it's probably best to ensure that they're valid anyway.)
regards, tom lane
Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
From
Tomas Vondra
Date:
On 4/12/21 7:04 PM, Tom Lane wrote: > Ranier Vilela <ranier.vf@gmail.com> writes: >> Em seg., 12 de abr. de 2021 às 03:04, Tom Lane <tgl@sss.pgh.pa.us> escreveu: >>> It would be wrong, though, or at least not have the same effect. > >> I think that you speak about fill pointers with 0 is not the same as fill >> pointers with NULL. > > No, I mean that InvalidBlockNumber isn't 0. > >> I was confused here, does the patch follow the pattern and fix the problem >> or not? > > Your patch seems fine. Justin's proposed improvement isn't. > Pushed. > (I'm not real sure whether there's any *actual* bug here --- would we > really be looking at either ctid or tableoid of this temporary tuple? > But it's probably best to ensure that they're valid anyway.)> Yeah, the tuple is only built so that we can pass it to the various selectivity estimators. I don't think anything will be actually looking at those fields, but initializing them seems easy enough. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company