Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
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
Attachments:
fix_uninitialized_var_extend_stats.patchapplication/octet-stream; name=fix_uninitialized_var_extend_stats.patchDownload
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index 4674168ff8..4b15b117b6 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -2420,6 +2420,8 @@ statext_expressions_load(Oid stxoid, int idx)
/* Build a temporary HeapTuple control structure */
tmptup.t_len = HeapTupleHeaderGetDatumLength(td);
+ ItemPointerSetInvalid(&(tmptup.t_self));
+ tmptup.t_tableOid = InvalidOid;
tmptup.t_data = td;
tup = heap_copytuple(&tmptup);
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
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:
Show quoted text
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
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
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 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
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