On Fri, Nov 17, 2017 at 1:55 PM, Andres Freund <andres@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'reusing 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
