Re: [HACKERS] Parallel Hash take II

Hi,

On 2017-11-14 01:30:30 +1300, Thomas Munro wrote:
> New patch attached.

(I've commit some of the preliminary work)

Looking at 0005-Add-infrastructure-for-sharing-temporary-files-betwe.patch:
- The created path/filenames seem really redundant:
base/pgsql_tmp/pgsql_tmp11160.9.sharedfileset.d/pgsql_tmp.o3of8.p0.0

Including pgsql_tmp no less than three times seems a bit absurd.

I'm quite inclined to just remove all but the first.

- There seems to be a moment where could leak temporary file
directories:
File
SharedFileSetCreate(SharedFileSet *fileset, const char *name)
{
char path[MAXPGPATH];
File file;

SharedFilePath(path, fileset, name);
file = PathNameCreateTemporaryFile(path, false);

/* If we failed, see if we need to create the directory on demand. */
if (file <= 0)
{
char tempdirpath[MAXPGPATH];
char filesetpath[MAXPGPATH];
Oid tablespace = ChooseTablespace(fileset, name);

TempTablespacePath(tempdirpath, tablespace);
SharedFileSetPath(filesetpath, fileset, tablespace);
PathNameCreateTemporaryDir(tempdirpath, filesetpath);
file = PathNameCreateTemporaryFile(path, true);
}

return file;
}

The resowner handling is done in PathNameCreateTemporaryFile(). But if
we fail after creating the directory we'll not have a resowner for
that. That's probably not too bad.

- related to the last point, I'm kinda wondering why we need sub-fileset
resowners? Given we're dealing with error paths in resowners I'm not
quite seeing the point - we're not going to want to roll back
sub-parts of of a fileset, no?

- If we want to keep these resowners, shouldn't we unregister them in
PathNameDeleteTemporaryFile?

- PathNameCreateTemporaryFile() and OpenTemporaryFile() now overlap
quite a bit. Can't we rejigger things to base the second on the first?
At the very least the comments need to work out the difference more
closely.

- It's not clear to me why it's correct to have the vfdP->fdstate & FD_TEMPORARY
handling in FileClose() be independent of the file being deleted. At
the very least there needs to be a comment explaining why we chose
that behaviour.

- I think we need to document somehwere that the temp_file_limit in a
shared file set applies independently for each participant that's
writing something. We also should discuss whether that's actually
sane behaviour.

Greetings,

Andres Freund