On Mon, 6 Jul 2026 22:34:59 +0300, Heikki Linnakangas <[email protected]> wrote: > On 29/06/2026 16:19, Thomas Munro wrote: > > Hi, > > > > ECPG's auto_mem_destructor() doesn't seem quite right: > > > > 1. POSIX thread-specific keys are reset to NULL before their at-exit > > destructors are called, so its call to ECPGfree_auto_mem() gets NULL > > from get_auto_allocs(), so nothing much happens. It should use the > > value passed to the destructor. > > > > 2. ECPGfree_auto_mem() doesn't seem to be the right thing to do > > anyway, because the user is expected to free heap objects allocated by > > the library, for example where > > src/interfaces/ecpg/test/thread/alloc.pgc does this: > > > > char **r = NULL; > > ... > > for (i = 1; i <= REPEATS; ++i) > > { > > EXEC SQL SELECT relname INTO :r FROM pg_class WHERE relname = > > 'pg_class'; > > free(r); > > r = NULL; > > } > > > > With a fix for only problem #1 in place, the thread-exit destructor > > double-frees "r" from the final loop. I *think* what is wanted here > > is ecpg_clear_auto_mem(), to free just the list structure and not the > > values themselves. Draft patch like that attached. > > > > It still leaks on Windows, but that's a known issue and I have a fix > > for that as part of a larger refactoring of thread-related stuff, more > > on that shortly. This looked like a bug to report separately first. > > Good catch!
The v2 patch looks right to me. I applied it on top of current master, built it clean, and ran the ecpg suite: all 66 tests pass. To confirm the hazard is real I also tried the #1-only version (use the passed value but keep freeing the payload), and thread/alloc aborts on a double-free, as you'd expect; v2 is clean. > I wonder if we should refrain from doing the "set_auto_allocs(NULL);" > call from the destructor. I guess it's harmless, but it feels a bit > wrong to me. Like in the attached. Agreed. The slot is already NULL by the time the destructor runs, so re-clearing it buys nothing, and storing into a TLS key from inside its own destructor is a wart worth avoiding. Keeping set_auto_allocs(NULL) in ecpg_clear_auto_mem(), where the thread is still live, and leaving it out of the destructor seems like the right split. The one thing worth spelling out in the commit message is why freeing the node but not the pointer is correct: the auto_mem node is library bookkeeping, but ownership of the pointed-to value was handed to the caller when the statement succeeded. That captures the bug, and it is what separates this path from ECPGfree_auto_mem(). > > Hmm, I wonder why ecpg_raise() frees auto-allocated values for all > > connections just because one connection raised an error. > > I didn't look at that > > - Heikki Earlier in the thread Thom Brown wrote: > So would it just be a matter of moving ecpg_clear_auto_mem() out of > ecpg_do_prologue() and put it into ecpg_do_epilogue() ... I think that is the better model, but it can't fully retire the destructor. SELECT ... INTO transfers ownership and could be cleared eagerly, but GET DESCRIPTOR ... INTO :array leaves caller-owned arrays on the list between statements by design, so a thread that exits before the bulk-free still needs node cleanup. I'd keep the back-patch limited to the destructor fix and treat the epilogue move as a separate master-only change. On your off-list ordering question, whether this can go in as-is or the ecpg_raise() issue has to come first: as-is, I think. The patch is self-contained. It changes only the thread-exit destructor and the node-versus-value split, and touches neither ecpg_raise() nor where ecpg_clear_auto_mem() is called. It is a back-patchable fix for a double-free in valid .pgc code, with no dependency on the ecpg_raise() rework in either direction. It unblocks the rest of the series. The ecpg_raise() over-free stays a separate master-only change, and as noted above it would not let us remove the destructor anyway, so there is no reason to gate the leak fix on it. On the Windows replacement mentioned upthread: the reason the destructor never runs there is that TlsAlloc has no thread-exit callback. The drop-in that does is FlsAlloc, whose fiber-local storage callback fires on thread and fiber exit and is handed the stored value as its argument, which is exactly the contract this patch now relies on. For a concrete reference, I maintain a small portable thread-exit cleanup primitive built on this, __os_thread_atexit(fn, arg): https://github.com/gburd/libxtc/blob/main/src/os/os_tls.c#L69-L129 It exists because a runtime has to free per-thread state on any thread that touches it, including host/carrier threads it did not create, which is the same spot ECPG is in with application threads. It is one lazily-created key whose destructor walks a per-thread LIFO list and frees each node after calling its fn. Two details line up with this thread. The destructor uses the value the OS passes in rather than re-reading the slot, which is problem #1 here. And it frees only its own list nodes, leaving each fn to decide the payload's fate, which is the node-versus-value split v2 introduces. The POSIX build uses pthread_key_create and the Windows build maps the same key onto FlsAlloc, so one mechanism covers both with no Windows leak. It is not something Postgres would depend on, just prior art showing the shape in this patch ports cleanly, Windows included. -greg
