Horton

Thread

Commits

  1. Initialize t_self and t_tableOid in statext_expressions_load

  1. Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2021-04-11T18:38:10Z

    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

  2. Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)

    Justin Pryzby <pryzby@telsasoft.com> — 2021-04-11T19:25:12Z

    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

  3. Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2021-04-11T22:42:20Z

    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
>

  4. Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)

    Michael Paquier <michael@paquier.xyz> — 2021-04-12T05:07:30Z

    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

  5. Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-12T06:04:13Z

    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

  6. Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2021-04-12T16:55:13Z

    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

  7. Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-04-12T17:03:11Z

    
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

  8. Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-12T17:04:24Z

    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

  9. Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)

    Justin Pryzby <pryzby@telsasoft.com> — 2021-04-12T17:05:53Z

    On Mon, Apr 12, 2021 at 01:55:13PM -0300, Ranier Vilela wrote:
> 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?

I think Tom means that it does:
BlockIdSet(&((pointer)->ip_blkid), InvalidBlockNumber),
(pointer)->ip_posid = InvalidOffsetNumber

but it's not zero, as I thought:

src/include/storage/block.h:#define InvalidBlockNumber          ((BlockNumber) 0xFFFFFFFF)

> > (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.)

FYI, I'd gotten the idea from here:

$ git grep 'HeapTupleData.*='
src/backend/executor/execTuples.c:      HeapTupleData tuple = {0};

-- 
Justin

  10. Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-04-13T22:55:55Z

    
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