Обсуждение: Re: some Page/PageData const stuff

This has been committed.

On 09.12.24 16:44, Peter Eisentraut wrote:
> In [0] I wrote:
> 
> """
> I was fiddling a bit with making some Page-related APIs const-proof, 
> which might involve changing something like "Page p" to "const PageData 
> *p", but I was surprised that a type PageData exists but it's an 
> unrelated type local to generic_xlog.c.
> 
> This patch renames that type to a more specific name 
> [GenericXLogPageData].  This makes room for possibly adding another 
> PageData type with the earlier meaning, but that's not done here.
> 
> """
> 
> [0]: https://www.postgresql.org/message-id/flat/001d457e-c118-4219-8132- 
> e1846c2ae3c9%40eisentraut.org
> 
> This is now the follow-up that adds back PageData with the earlier 
> meaning and updates a few of the Page-related APIs to be const-proof. 
> That is all pretty straightforward, except one inline function that had 
> to be changed back to a macro, because it is used in a way that 
> sometimes it takes const and returns const and sometimes takes non-const 
> and returns non-const.  (We might be able to do that kind of thing 
> better with C23 in N years. ;-) )
> 
> Just a thought, I've been thinking it might be neat if PageData were 
> actually defined something like this:
> 
> typedef struct PageData
> {
>      union
>      {
>          PageHeaderData phdr;
>          PGAlignedBlock data;
>      };
> } PageData;
> 
> Then you could write all those (PageHeader) casts in a more elegant way, 
> and you don't get to randomly mix char * and Page, which has very weak 
> type safety.  But this currently totally breaks, because many places 
> assume you can do char-based pointer arithmetic with Page values.  So 
> this would need further analysis.

Hi,

On 2025-01-20 15:01:08 +0100, Peter Eisentraut wrote:
> This has been committed.

I don't like the const markings in PageGetItem():


/*
 * PageGetItem
 *        Retrieves an item on the given page.
 *
 * Note:
 *        This does not change the status of any of the resources passed.
 *        The semantics may change in the future.
 */
static inline void *
PageGetItem(const PageData *page, const ItemIdData *itemId)
{
    Assert(page);
    Assert(ItemIdHasStorage(itemId));

    return (void *) (((const char *) page) + ItemIdGetOffset(itemId));
}

The const for PageData seems like a lie to me, because we cast it away. And
indeed, we often then use the returned value to set hint bits etc.

Greetings,

Andres Freund

On 02.01.26 20:17, Andres Freund wrote:
> On 2025-01-20 15:01:08 +0100, Peter Eisentraut wrote:
>> This has been committed.
> 
> I don't like the const markings in PageGetItem():
> 
> 
> /*
>   * PageGetItem
>   *        Retrieves an item on the given page.
>   *
>   * Note:
>   *        This does not change the status of any of the resources passed.
>   *        The semantics may change in the future.
>   */
> static inline void *
> PageGetItem(const PageData *page, const ItemIdData *itemId)
> {
>     Assert(page);
>     Assert(ItemIdHasStorage(itemId));
> 
>     return (void *) (((const char *) page) + ItemIdGetOffset(itemId));
> }
> 
> The const for PageData seems like a lie to me, because we cast it away. And
> indeed, we often then use the returned value to set hint bits etc.

I agree this is incorrect.  Since no callers appear to rely on the const 
qualification of the argument, the easiest solution would be to just 
remove it.  See attached patch.

(In the future, this might be a candidate for a qualifier-preserving 
polymorphic function like the C23 string functions, but that's for 
another day.)

Hi,

On 2026-01-03 18:05:19 +0100, Peter Eisentraut wrote:
> On 02.01.26 20:17, Andres Freund wrote:
> > On 2025-01-20 15:01:08 +0100, Peter Eisentraut wrote:
> > > This has been committed.
> > 
> > I don't like the const markings in PageGetItem():
> > 
> > 
> > /*
> >   * PageGetItem
> >   *        Retrieves an item on the given page.
> >   *
> >   * Note:
> >   *        This does not change the status of any of the resources passed.
> >   *        The semantics may change in the future.
> >   */
> > static inline void *
> > PageGetItem(const PageData *page, const ItemIdData *itemId)
> > {
> >     Assert(page);
> >     Assert(ItemIdHasStorage(itemId));
> > 
> >     return (void *) (((const char *) page) + ItemIdGetOffset(itemId));
> > }
> > 
> > The const for PageData seems like a lie to me, because we cast it away. And
> > indeed, we often then use the returned value to set hint bits etc.
> 
> I agree this is incorrect.  Since no callers appear to rely on the const
> qualification of the argument, the easiest solution would be to just remove
> it.  See attached patch.

LGTM


> (In the future, this might be a candidate for a qualifier-preserving
> polymorphic function like the C23 string functions, but that's for another
> day.)

Makes sense. I doubt this is the place I would start with that though, the
amount of effort that'd take around all the functions dealing with heap tuples
seems too large...

Greetings,

Andres Freund

On 03.01.26 18:23, Andres Freund wrote:
> On 2026-01-03 18:05:19 +0100, Peter Eisentraut wrote:
>> On 02.01.26 20:17, Andres Freund wrote:
>>> On 2025-01-20 15:01:08 +0100, Peter Eisentraut wrote:
>>>> This has been committed.
>>>
>>> I don't like the const markings in PageGetItem():
>>>
>>>
>>> /*
>>>    * PageGetItem
>>>    *        Retrieves an item on the given page.
>>>    *
>>>    * Note:
>>>    *        This does not change the status of any of the resources passed.
>>>    *        The semantics may change in the future.
>>>    */
>>> static inline void *
>>> PageGetItem(const PageData *page, const ItemIdData *itemId)
>>> {
>>>     Assert(page);
>>>     Assert(ItemIdHasStorage(itemId));
>>>
>>>     return (void *) (((const char *) page) + ItemIdGetOffset(itemId));
>>> }
>>>
>>> The const for PageData seems like a lie to me, because we cast it away. And
>>> indeed, we often then use the returned value to set hint bits etc.
>>
>> I agree this is incorrect.  Since no callers appear to rely on the const
>> qualification of the argument, the easiest solution would be to just remove
>> it.  See attached patch.
> 
> LGTM

committed