On 11/05/2023 00:00, Jehan-Guillaume de Rorthais wrote:
> On Wed, 10 May 2023 15:11:20 +1200
> Thomas Munro <thomas.munro@gmail.com> wrote:
>> The reason I didn't do this earlier is that sharedtuplestore.c
>> continues the pre-existing tradition where each parallel process
>> counts what it writes against its own temp_file_limit. At the time I
>> thought I'd need to have one process unlink all the files, but if a
>> process were to unlink files that it didn't create, that accounting
>> system would break. Without some new kind of shared temp_file_limit
>> mechanism that doesn't currently exist, per-process counters could go
>> negative, creating free money. In the attached patch, I realised
>> something that I'd missed before: there is a safe point for each
>> backend to unlink just the files that it created, and there is no way
>> for a process that created files not to reach that point.
>
> Indeed.
>
> For what it worth, from my new and non-experienced understanding of the
> parallel mechanism, waiting for all workers to reach
> WAIT_EVENT_HASH_GROW_BATCHES_REPARTITION, after re-dispatching old batches in
> new ones, seems like a safe place to instruct each workers to clean their old
> temp files.
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.
--
Heikki Linnakangas
Neon (https://neon.tech)