On 11/05/2023 00:00, Jehan-Guillaume de Rorthais wrote:
On Wed, 10 May 2023 15:11:20 +1200
Thomas Munro <thomas.mu...@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)
