Fix incorrect const qualification for tbm_add_tuples() and itemptr_to_uint64()

Started by Chao Li6 months ago2 messageshackers
Jump to latest
#1Chao Li
li.evan.chao@gmail.com

Hi Hackers,

I noticed a wrong const qualification:
```
void
tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bool recheck)
```

This "const" only protects "tids" itself from updating, which is
meaningless. I believe the real intention should be protecting the content
"tids" pointing to from updating.

This behavior can be easily proved by the compiler. If we add a line of
fake code in the function:
```
tids[0].ip_posid = 0;
```

With current "const ItemPointer tids", the compiler won't report any
problem. If we change to "const ItemPointerData *tids", the compiler will
raise an error due to the assignment to read-only variable.

I searched over the source tree, and found only one more occurrence in
itemptr_to_uint64(), so I fixed it as well.

Also, as I am touching tbm_add_tuples(), I did a tiny change that moved the
loop variable "i" into "for". Peter Eisentraut just did the same change in
formatting.c [1]https://git.postgresql.org/cgit/postgresql.git/commit/?id=03fbb0814c5015ab79e670ab97bb6a3349269e4b.

[1]: https://git.postgresql.org/cgit/postgresql.git/commit/?id=03fbb0814c5015ab79e670ab97bb6a3349269e4b
https://git.postgresql.org/cgit/postgresql.git/commit/?id=03fbb0814c5015ab79e670ab97bb6a3349269e4b

Best regards,
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/

Attachments:

v1-0001-Fix-incorrect-const-qualification-for-tbm_add_tup.patchapplication/octet-stream; name=v1-0001-Fix-incorrect-const-qualification-for-tbm_add_tup.patchDownload+4-6
#2Peter Eisentraut
peter_e@gmx.net
In reply to: Chao Li (#1)
Re: Fix incorrect const qualification for tbm_add_tuples() and itemptr_to_uint64()

On 29.10.25 04:11, Chao Li wrote:

I noticed a wrong const qualification:
```
void
tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
  bool recheck)
```

This "const" only protects "tids" itself from updating, which is
meaningless. I believe the real intention should be protecting the
content "tids" pointing to from updating.

This behavior can be easily proved by the compiler. If we add a line of
fake code in the function:
```
tids[0].ip_posid = 0;
```

With current "const ItemPointer tids", the compiler won't report any
problem. If we change to "const ItemPointerData *tids", the compiler
will raise an error due to the assignment to read-only variable.

I searched over the source tree, and found only one more occurrence in
itemptr_to_uint64(), so I fixed it as well.

I have committed this, and I also found a few more similarly confused
cases across the tree, which I also fixed.

Also, as I am touching tbm_add_tuples(), I did a tiny change that moved
the loop variable "i" into "for". Peter Eisentraut just did the same
change in formatting.c [1].

I don't know, let's leave unrelated changes for a separate patch.