Horton

Thread

Commits

  1. Remove bogus const qualifier on PageGetItem() argument

  2. Add some more use of Page/PageData rather than char *

  3. Add const qualifiers to bufpage.h

  4. Add PageData C type

  1. some Page/PageData const stuff

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-09T15:44:14Z

    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.

  2. Re: some Page/PageData const stuff

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-20T14:01:08Z

    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.

  3. Re: some Page/PageData const stuff

    Andres Freund <andres@anarazel.de> — 2026-01-02T19:17:39Z

    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

  4. Re: some Page/PageData const stuff

    Peter Eisentraut <peter@eisentraut.org> — 2026-01-03T17:05:19Z

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

  5. Re: some Page/PageData const stuff

    Andres Freund <andres@anarazel.de> — 2026-01-03T17:23:01Z

    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

  6. Re: some Page/PageData const stuff

    Peter Eisentraut <peter@eisentraut.org> — 2026-01-04T15:10:30Z

    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