Horton

Thread

Commits

  1. Mark ItemPointer arguments as const in tuple/table lock functions

  1. Mark ItemPointer parameters as const in tuple/table lock functions

    Chao Li <li.evan.chao@gmail.com> — 2025-08-27T08:57:31Z

    Hi Hackers,

This is a pure refactor patch.

The functions LockTuple, ConditionalLockTuple, UnlockTuple, and
XactLockTableWait take an ItemPointer parameter named 'tid'. Since these
functions do not modify the tuple identifier, the parameter should be
declared as const to better convey intent and allow the compiler to enforce
immutability.

With this patch, build still passes, and "make check" also passes.

Best regards,

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

  2. Re: Mark ItemPointer parameters as const in tuple/table lock functions

    Peter Eisentraut <peter@eisentraut.org> — 2025-08-27T09:24:18Z

    On 27.08.25 10:57, Chao Li wrote:
> This is a pure refactor patch.
> 
> The functions LockTuple, ConditionalLockTuple, UnlockTuple, and 
> XactLockTableWait take an ItemPointer parameter named 'tid'. Since these 
> functions do not modify the tuple identifier, the parameter should be 
> declared as const to better convey intent and allow the compiler to 
> enforce immutability.
> 
> With this patch, build still passes, and "make check" also passes.

I think this is a misunderstanding:

-LockTuple(Relation relation, ItemPointer tid, LOCKMODE lockmode)
+LockTuple(Relation relation, const ItemPointer tid, LOCKMODE lockmode)

This means that the pointer variable "tid" itself is constant, which is 
probably not what you wanted.  This would prevent changes to tid itself, 
like if the function did

     tid = NULL;

What you actually want would be something like

-LockTuple(Relation relation, ItemPointer tid, LOCKMODE lockmode)
+LockTuple(Relation relation, const ItemPointerData *tid, LOCKMODE lockmode)

so that the qualifier applies to what is pointed to.

See for example the function ItemPointerGetBlockNumber() that 
LockTuple() calls for this style.

This style of having Foo be a type alias for pointer-to-FooData is an 
ancient Postgres coding convention that does not map well to modern C 
that has an emphasis on judicious use of qualifiers and attributes, and 
so if this abstraction gets in the way, we sometimes crack it open, like 
in the case of ItemPointerGetBlockNumber() etc.

  3. Re: Mark ItemPointer parameters as const in tuple/table lock functions

    Chao Li <li.evan.chao@gmail.com> — 2025-08-28T02:27:58Z

    
> On Aug 27, 2025, at 17:24, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> This style of having Foo be a type alias for pointer-to-FooData is an ancient Postgres coding convention that does not map well to modern C that has an emphasis on judicious use of qualifiers and attributes, and so if this abstraction gets in the way, we sometimes crack it open, like in the case of ItemPointerGetBlockNumber() etc.

You are right, we want to protect the stuff that “tid” points to instead of “tid” itself:

     *tid = something; // should hit compile error
     tid = something; // ok

Also, thanks for telling the history. I have updated the patch to use “const ItemPointerData *” in the same way as ItemPointerGetBlockNumber().

Attached is the v2 patch.

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



  4. Re: Mark ItemPointer parameters as const in tuple/table lock functions

    Chao Li <li.evan.chao@gmail.com> — 2025-08-28T02:30:13Z

    Mail Archive always misses attachments sent by Apple Mail. Resending from
the Gmail web client.

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


Chao Li <li.evan.chao@gmail.com> 于2025年8月28日周四 10:28写道：

>
>
> On Aug 27, 2025, at 17:24, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> This style of having Foo be a type alias for pointer-to-FooData is an
> ancient Postgres coding convention that does not map well to modern C that
> has an emphasis on judicious use of qualifiers and attributes, and so if
> this abstraction gets in the way, we sometimes crack it open, like in the
> case of ItemPointerGetBlockNumber() etc.
>
>
> You are right, we want to protect the stuff that “tid” points to instead
> of “tid” itself:
>
>      *tid = something; // should hit compile error
>      tid = something; // ok
>
> Also, thanks for telling the history. I have updated the patch to use
> “const ItemPointerData *” in the same way as ItemPointerGetBlockNumber().
>
> Attached is the v2 patch.
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
>

  5. Re: Mark ItemPointer parameters as const in tuple/table lock functions

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

    On 28.08.25 04:27, Chao Li wrote:
>> On Aug 27, 2025, at 17:24, Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> This style of having Foo be a type alias for pointer-to-FooData is an 
>> ancient Postgres coding convention that does not map well to modern C 
>> that has an emphasis on judicious use of qualifiers and attributes, 
>> and so if this abstraction gets in the way, we sometimes crack it 
>> open, like in the case of ItemPointerGetBlockNumber() etc.
> 
> You are right, we want to protect the stuff that “tid” points to instead 
> of “tid” itself:
> 
>       *tid = something; // should hit compile error
>       tid = something; // ok
> 
> Also, thanks for telling the history. I have updated the patch to use 
> “const ItemPointerData *” in the same way as ItemPointerGetBlockNumber().
> 
> Attached is the v2 patch.

This patch still causes a compiler warning:

../src/backend/storage/lmgr/lmgr.c: In function 'XactLockTableWait':
../src/backend/storage/lmgr/lmgr.c:681:27: error: assignment discards 
'const' qualifier from pointer target type [-Werror=discarded-qualifiers]

I have fixed that and committed your patch.

  6. Re: Mark ItemPointer parameters as const in tuple/table lock functions

    Chao Li <li.evan.chao@gmail.com> — 2025-08-29T06:23:50Z

    Hi Peter,

Thank you so much for your help.

> On Aug 29, 2025, at 13:42, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> This patch still causes a compiler warning:
> 
> ../src/backend/storage/lmgr/lmgr.c: In function 'XactLockTableWait':
> ../src/backend/storage/lmgr/lmgr.c:681:27: error: assignment discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
> 
> I have fixed that and committed your patch.
> 

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