Обсуждение: Mark ItemPointer parameters as const in tuple/table lock functions

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.

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.