Обсуждение: [PATCH] Free memory allocated by waitonlock_error_callback()

Hi,

Currently waitonlock_error_callback() allocates memory in ErrorContext
and doesn't explicitly free it. Valgrind is not happy about it and
generates multiple reports like this:

```
2,048 bytes in 2 blocks are definitely lost in loss record 1,097 of 1,165
   at 0x999539: palloc (mcxt.c:1389)
   by 0x9DCBFD: initStringInfoInternal (stringinfo.c:45)
   by 0x9DCC9D: initStringInfo (stringinfo.c:99)
   by 0x73A807: waitonlock_error_callback (lock.c:2027)
   by 0x94E1DE: errfinish (elog.c:510)
   by 0x750394: ProcSleep (proc.c:1614)
   by 0x73A720: WaitOnLock (lock.c:1979)
   by 0x7391C2: LockAcquireExtended (lock.c:1221)
   by 0x738360: LockAcquire (lock.c:814)
   by 0x741064: VirtualXactLock (lock.c:4844)
   by 0x3E9D93: WaitForOlderSnapshots (indexcmds.c:492)
   by 0x3F155B: ReindexRelationConcurrently (indexcmds.c:4216)
```

I propose adding pfree().

waitonlock_error_callback() was added in:

```
commit f727b63e810724c7187f38b2580b2915bdbc3c9c
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Fri Aug 29 15:43:34 2025 -0400

    Provide error context when an error is thrown within WaitOnLock().
```

-- 
Best regards,
Aleksander Alekseev

On 9/19/25 7:16 PM, Aleksander Alekseev wrote:
> Hi,
> 
> Currently waitonlock_error_callback() allocates memory in ErrorContext
> and doesn't explicitly free it. Valgrind is not happy about it and
> generates multiple reports like this:
> 
> ```
> 2,048 bytes in 2 blocks are definitely lost in loss record 1,097 of 1,165
>     at 0x999539: palloc (mcxt.c:1389)
>     by 0x9DCBFD: initStringInfoInternal (stringinfo.c:45)
>     by 0x9DCC9D: initStringInfo (stringinfo.c:99)
>     by 0x73A807: waitonlock_error_callback (lock.c:2027)
>     by 0x94E1DE: errfinish (elog.c:510)
>     by 0x750394: ProcSleep (proc.c:1614)
>     by 0x73A720: WaitOnLock (lock.c:1979)
>     by 0x7391C2: LockAcquireExtended (lock.c:1221)
>     by 0x738360: LockAcquire (lock.c:814)
>     by 0x741064: VirtualXactLock (lock.c:4844)
>     by 0x3E9D93: WaitForOlderSnapshots (indexcmds.c:492)
>     by 0x3F155B: ReindexRelationConcurrently (indexcmds.c:4216)
> ```
> 
> I propose adding pfree().
> 
> waitonlock_error_callback() was added in:
> 
> ```
> commit f727b63e810724c7187f38b2580b2915bdbc3c9c
> Author: Tom Lane <tgl@sss.pgh.pa.us>
> Date:   Fri Aug 29 15:43:34 2025 -0400
> 
>      Provide error context when an error is thrown within WaitOnLock().
> ```
> 

LGTM

Improve the completeness of the code.

On Thu, Oct 23, 2025 at 11:18:09AM +0800, Quan Zongliang wrote:
> On 9/19/25 7:16 PM, Aleksander Alekseev wrote:
>> Currently waitonlock_error_callback() allocates memory in ErrorContext
>> and doesn't explicitly free it. Valgrind is not happy about it and
>> generates multiple reports like this:
>
> Improve the completeness of the code.

errfinish() calls MemoryContextReset() on ErrorContext so as any leaks
like the one you are cleaning up are taken care of.  Still, what you
are suggesting is simple enough and silences a bit valgrind, so agreed
about the addition of this pfree().

Let's see if somebody objects to that.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Oct 23, 2025 at 11:18:09AM +0800, Quan Zongliang wrote:
>> Improve the completeness of the code.

> errfinish() calls MemoryContextReset() on ErrorContext so as any leaks
> like the one you are cleaning up are taken care of.  Still, what you
> are suggesting is simple enough and silences a bit valgrind, so agreed
> about the addition of this pfree().

> Let's see if somebody objects to that.

Yeah, I object.  Didn't 89d57c1fb already solve this in a more
general way?  If it's not fixed by that, why not?

            regards, tom lane

Hi Tom,

> Yeah, I object.  Didn't 89d57c1fb already solve this in a more
> general way?  If it's not fixed by that, why not?

I can confirm that 89d57c1fb fixed the issue under question, thanks!
Marking the patch as Withdrawn.

-- 
Best regards,
Aleksander Alekseev