On Wed, Sep 27, 2023 at 11:42 PM Heikki Linnakangas <hlinn...@iki.fi> wrote:
>
> Looks good to me too at a quick glance. There's this one "XXX free"
> comment though:
>
> > for (int i = 1; i < old_nbatch; ++i)
> > {
> > ParallelHashJoinBatch *shared =
> > NthParallelHashJoinBatch(old_batches, i);
> > SharedTuplestoreAccessor *accessor;
> >
> > accessor = sts_attach(ParallelHashJoinBatchInner(shared),
> >
> > ParallelWorkerNumber + 1,
> > &pstate->fileset);
> > sts_dispose(accessor);
> > /* XXX free */
> > }
>
> I think that's referring to the fact that sts_dispose() doesn't free the
> 'accessor', or any of the buffers etc. that it contains. That's a
> pre-existing problem, though: ExecParallelHashRepartitionRest() already
> leaks the SharedTuplestoreAccessor structs and their buffers etc. of the
> old batches. I'm a little surprised there isn't aready an sts_free()
> function.
>
> Another thought is that it's a bit silly to have to call sts_attach()
> just to delete the files. Maybe sts_dispose() should take the same three
> arguments that sts_attach() does, instead.
>
> So that freeing would be nice to tidy up, although the amount of memory
> leaked is tiny so might not be worth it, and it's a pre-existing issue.
> I'm marking this as Ready for Committer.
(I thought I'd go around and nudge CF entries where both author and
reviewer are committers.)
Hi Thomas, do you have any additional thoughts on the above?
