Horton

Thread

Commits

  1. Remove unneeded casts of BufferGetPage() result

  1. Remove unneeded cast in heap_xlog_lock.

    Kirill Reshke <reshkekirill@gmail.com> — 2025-08-21T14:10:43Z

    Hi!

I was looking at how PostgreSQL handles VM map bits, when I noticed $subj.
PFA small refactoring patch.

-- 
Best regards,
Kirill Reshke

  2. Re: Remove unneeded cast in heap_xlog_lock.

    Chao Li <li.evan.chao@gmail.com> — 2025-08-22T00:44:17Z

    
> On Aug 21, 2025, at 22:10, Kirill Reshke <reshkekirill@gmail.com> wrote:
> 
> Hi!
> 
> I was looking at how PostgreSQL handles VM map bits, when I noticed $subj.
> PFA small refactoring patch.
> 
> -- 
> Best regards,
> Kirill Reshke
> <v1-0001-Remove-unneeded-cast-in-heap_xlog_lock.patch>



LGTM. BufferGetPage() returns a “Page” type, and the receiving variable “page” is of “Page” type as well, so the cast is not needed.

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

  3. Re: Remove unneeded cast in heap_xlog_lock.

    Richard Guo <guofenglinux@gmail.com> — 2025-08-22T01:41:15Z

    On Fri, Aug 22, 2025 at 9:44 AM Chao Li <li.evan.chao@gmail.com> wrote:
> On Aug 21, 2025, at 22:10, Kirill Reshke <reshkekirill@gmail.com> wrote:
> I was looking at how PostgreSQL handles VM map bits, when I noticed $subj.
> PFA small refactoring patch.

> LGTM. BufferGetPage() returns a “Page” type, and the receiving variable “page” is of “Page” type as well, so the cast is not needed.

If you run 'git grep', you'll find a lot more instances where the
result of BufferGetPage() is explicitly cast to Page.

git grep -rn "(Page) BufferGetPage" | wc -l
69

Although these casts are unnecessary for sure, I'm not sure if it's
worth making the code changes to fix them.

Thanks
Richard

  4. Re: Remove unneeded cast in heap_xlog_lock.

    Michael Paquier <michael@paquier.xyz> — 2025-08-22T05:23:22Z

    On Fri, Aug 22, 2025 at 10:41:15AM +0900, Richard Guo wrote:
> Although these casts are unnecessary for sure, I'm not sure if it's
> worth making the code changes to fix them.

That's sort of the point.  This is not code that needs to be fixed,
because it's not broken.
--
Michael

  5. Re: Remove unneeded cast in heap_xlog_lock.

    Kirill Reshke <reshkekirill@gmail.com> — 2025-08-22T05:36:30Z

    On Fri, 22 Aug 2025 at 06:41, Richard Guo <guofenglinux@gmail.com> wrote:
>
> If you run 'git grep', you'll find a lot more instances where the
> result of BufferGetPage() is explicitly cast to Page.
>
> git grep -rn "(Page) BufferGetPage" | wc -l
> 69
>
> Although these casts are unnecessary for sure, I'm not sure if it's
> worth making the code changes to fix them.

I can see your point. But I can see that there is a great amount of
commits in HEAD (every now and then), which are just pure refactoring
and standardizing things.
I am uncertain about the delineation between when we make changes and
when we refrain from doing so.

I do not insist on this modification. I just spotted two completely
same codes in [0] & [1], which only differ in BufferGetPage cast. And
I merely tried to do something with it.

v2 attached with all 69 casts removed, but I see there is a little
chance of this committed.

[0] https://github.com/postgres/postgres/blob/13b935c/src/backend/access/heap/heapam_xlog.c#L1007-L1050
[1] https://github.com/postgres/postgres/blob/13b935c/src/backend/access/heap/heapam_xlog.c#L1083-L1121

-- 
Best regards,
Kirill Reshke

  6. Re: Remove unneeded cast in heap_xlog_lock.

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-08-22T09:59:31Z

    On 2025-Aug-22, Kirill Reshke wrote:

> I am uncertain about the delineation between when we make changes and
> when we refrain from doing so.

I think this is natural work after 9c727360bcc7, before which
BufferGetPage() was a macro and strangely enough had its own cast
embedded.  As I understand, the less casts we have, the better.  There's
some other standardization work going on to remove unnecessary casts
elsewhere, so I'm not sure why we wouldn't do this.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El que vive para el futuro es un iluso, y el que vive para el pasado,
un imbécil" (Luis Adler, "Los tripulantes de la noche")

  7. Re: Remove unneeded cast in heap_xlog_lock.

    Chao Li <li.evan.chao@gmail.com> — 2025-08-22T10:25:38Z

    
> On Aug 22, 2025, at 13:36, Kirill Reshke <reshkekirill@gmail.com> wrote:
> 
> 
> I do not insist on this modification. I just spotted two completely
> same codes in [0] & [1], which only differ in BufferGetPage cast. And
> I merely tried to do something with it.
> 
> v2 attached with all 69 casts removed, but I see there is a little
> chance of this committed.
> 
> [0] https://github.com/postgres/postgres/blob/13b935c/src/backend/access/heap/heapam_xlog.c#L1007-L1050
> [1] https://github.com/postgres/postgres/blob/13b935c/src/backend/access/heap/heapam_xlog.c#L1083-L1121
> 
> -- 
> Best regards,
> Kirill Reshke
> <v2-0001-Remove-unneeded-cast-in-BufferGetPage.patch>


As there are only 76 occurrences, I think using one commit to clean up the unnecessary cast is worthwhile.

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

  8. Re: Remove unneeded cast in heap_xlog_lock.

    Richard Guo <guofenglinux@gmail.com> — 2025-08-22T14:01:41Z

    On Fri, Aug 22, 2025 at 6:59 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2025-Aug-22, Kirill Reshke wrote:
> > I am uncertain about the delineation between when we make changes and
> > when we refrain from doing so.

> I think this is natural work after 9c727360bcc7, before which
> BufferGetPage() was a macro and strangely enough had its own cast
> embedded.  As I understand, the less casts we have, the better.  There's
> some other standardization work going on to remove unnecessary casts
> elsewhere, so I'm not sure why we wouldn't do this.

I don't have a strong opinion on whether we should do this cleanup or
not.  I'm a bit concerned about the code churn, given that there are
69 instances spread across 22 files.  But maybe I'm worrying over
nothing, as we've done similar cleanups before to remove unnecessary
casts.

Thanks
Richard

  9. Re: Remove unneeded cast in heap_xlog_lock.

    Peter Eisentraut <peter@eisentraut.org> — 2025-08-23T14:57:53Z

    On 22.08.25 11:59, Álvaro Herrera wrote:
> On 2025-Aug-22, Kirill Reshke wrote:
> 
>> I am uncertain about the delineation between when we make changes and
>> when we refrain from doing so.
> 
> I think this is natural work after 9c727360bcc7, before which
> BufferGetPage() was a macro and strangely enough had its own cast
> embedded.  As I understand, the less casts we have, the better.  There's
> some other standardization work going on to remove unnecessary casts
> elsewhere, so I'm not sure why we wouldn't do this.

In the very first code import, BufferGetPage() was a regular function 
that returned Page.  (I suppose it was then turned into a macro, and 
then back into an inline function.)  Even in that first code import, 
some callers cast the return to (Page), and some not.  So I suppose this 
style just crept in for some random and ancient reason and then got 
copied around inconsistently.  We should clean it up.  Casts are bad.

  10. Re: Remove unneeded cast in heap_xlog_lock.

    Kirill Reshke <reshkekirill@gmail.com> — 2025-08-28T08:08:50Z

    On Sat, 23 Aug 2025 at 19:57, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 22.08.25 11:59, Álvaro Herrera wrote:
> > On 2025-Aug-22, Kirill Reshke wrote:
> >
> >> I am uncertain about the delineation between when we make changes and
> >> when we refrain from doing so.
> >
> > I think this is natural work after 9c727360bcc7, before which
> > BufferGetPage() was a macro and strangely enough had its own cast
> > embedded.  As I understand, the less casts we have, the better.  There's
> > some other standardization work going on to remove unnecessary casts
> > elsewhere, so I'm not sure why we wouldn't do this.
>
> In the very first code import, BufferGetPage() was a regular function
> that returned Page.  (I suppose it was then turned into a macro, and
> then back into an inline function.)  Even in that first code import,
> some callers cast the return to (Page), and some not.  So I suppose this
> style just crept in for some random and ancient reason and then got
> copied around inconsistently.

Thank you for clarifications.

> We should clean it up.  Casts are bad.

I created CF [0] for this.

[0] https://commitfest.postgresql.org/patch/6006/


-- 
Best regards,
Kirill Reshke

  11. Re: Remove unneeded cast in heap_xlog_lock.

    Peter Eisentraut <peter@eisentraut.org> — 2025-08-29T05:23:27Z

    On 28.08.25 10:08, Kirill Reshke wrote:
> On Sat, 23 Aug 2025 at 19:57, Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> On 22.08.25 11:59, Álvaro Herrera wrote:
>>> On 2025-Aug-22, Kirill Reshke wrote:
>>>
>>>> I am uncertain about the delineation between when we make changes and
>>>> when we refrain from doing so.
>>>
>>> I think this is natural work after 9c727360bcc7, before which
>>> BufferGetPage() was a macro and strangely enough had its own cast
>>> embedded.  As I understand, the less casts we have, the better.  There's
>>> some other standardization work going on to remove unnecessary casts
>>> elsewhere, so I'm not sure why we wouldn't do this.
>>
>> In the very first code import, BufferGetPage() was a regular function
>> that returned Page.  (I suppose it was then turned into a macro, and
>> then back into an inline function.)  Even in that first code import,
>> some callers cast the return to (Page), and some not.  So I suppose this
>> style just crept in for some random and ancient reason and then got
>> copied around inconsistently.
> 
> Thank you for clarifications.
> 
>> We should clean it up.  Casts are bad.
> 
> I created CF [0] for this.
> 
> [0] https://commitfest.postgresql.org/patch/6006/

committed