Re: Per-thread leak in ECPG's memory.c

On Tue, 30 Jun 2026 at 03:17, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>
> On Tue, Jun 30, 2026 at 1:21 PM Thom Brown <thom(at)linux(dot)com> wrote:
> > On Tue, 30 Jun 2026 at 00:24, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> > > EXEC SQL AT con1 SELECT datname INTO :anything FROM pg_database;
> > > EXEC SQL AT con2 ... something that reaches ecpg_raise() ...
> > >
> > > /* Why should "anything" not be accessible, and mine to free(), here? */
> >
> > I see, yeah, that does seem wrong. 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() so that it cleans up at the end of the statement
> > instead of the start of the next one?
>
> Oh, that sounds plausible and simple! Have I understood the
> lifetime/ownership rule correctly here?: after a successful statement,
> ownership of auto-allocated objects is transferred to the caller, and
> the fact that we currently don't "clear" (forget, disown) the list
> until the next statement is just an implementation detail and we could
> indeed make it "eager". In contrast, ECPGfree_auto_mem() is only used
> to clean up partially allocated variables on failure, since then the
> caller doesn't own anything. If that all makes sense, then perhaps
> the auto-mem list could always be NULL between ECPG statements,
> meaning that ownership of "anything" would already have been
> transferred to the caller (cleared) in this example. And then perhaps
> we wouldn't even need a thread-exit destructor here? On the other
> hand, that'd be a subtle semantic change that could upset an
> application. So perhaps this would have to be a well-signalled
> master-only change, if it makes sense to do it? I haven't actually
> tried any of that or written a test (feel free if you're interested in
> looking at this), I'm just trying to confirm my understanding of the
> design.
>
> This is independent of the destructor issue I started with, which I
> assume we'd want to back-patch. It would be nice if we could do
> something like the above in master and that leads to deleting the
> destructor though (we still need thread-exit cleanup for other ECPG
> things though, and need a new system for that that also works on
> Windows, more patches soon..., so it's not a big deal either way).

AFAICT, ownership transfers to the caller on success, and the lazy
clearing at next statement is just timing. But I think
ECPGfree_auto_mem() isn't only internal failure-cleanup. It's exported
and called by application code as the bulk-free for GET DESCRIPTOR ...
INTO :array results, which the caller does own.

I'm looking at dynalloc.pgc, line 83, and those arrays look like
they're auto-allocated in descriptor.c:

Line 437 onward, for the data array:

/* allocate storage if needed */
if (arrsize == 0 && *(void **) var == NULL)
{
void *mem = ecpg_auto_alloc(offset * ntuples, lineno);

if (!mem)
{
va_end(args);
return false;
}
*(void **) var = mem;
var = mem;
}

And there's another one at line 557 onward for the indicator array

So maybe it really does both cleanup on failure, and free descriptor
arrays that the caller owns. The change I semi-proposed would mean the
list would be NULL between statements, which holds for SELECT-INTO,
although not for descriptors. I don't think the destructor fully goes
away. A thread that does GET DESCRIPTOR and exits before bulk-freeing
still leaves nodes on the list. So thread-exit node cleanup is still
needed for the descriptor case, which matches what you said about
needing a new Windows-capable mechanism anyway.

Thom