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
havea 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
pathsin 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
secondon 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
ofthe 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
participantthat's writing something. We also should discuss whether that's actually sane behaviour.
Greetings,
Andres Freund
