On Fri, Nov 17, 2017 at 1:55 PM, Andres Freund <and...@anarazel.de> wrote: > 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.
+1 > - 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. Isn't that just because only one backend is supposed to delete the file, but they all must close it and do temp_file_limit accounting? Sorry if I missed something (my explanation seems too obvious to be correct). > - 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. This is already the documented behavior of temp_file_limit, fwiw. Another question for Thomas: Is it okay that routines like BufFileOpenShared() introduce new palloc()s (not repalloc()s) to buffile.c, given that struct BufFile already contains this?: /* * resowner is the ResourceOwner to use for underlying temp files. (We * don't need to remember the memory context we're using explicitly, * because after creation we only repalloc our arrays larger.) */ ResourceOwner resowner; Maybe we need to remember the original caller's memory context, too? Either that, or the contract/comments for memory contexts need to be revised. -- Peter Geoghegan
