Обсуждение: Remove unneeded cast in heap_xlog_lock.

Hi!

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

-- 
Best regards,
Kirill Reshke

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
isnot 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

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

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

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")

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

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.

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

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