Обсуждение: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)

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

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

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

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

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

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