From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: pgsql-committers@postgresql.org
Subject: Re: pgsql: Remove BufFile's isTemp flag.
In-reply-to: <24110.1510888651@sss.pgh.pa.us>
References: <E1eFVyl-0008J1-RO@gemulon.postgresql.org>
 <23339.1510887494@sss.pgh.pa.us>
 <20171117031149.hslkxis732hkjuog@alap3.anarazel.de>
 <24110.1510888651@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Thu, 16 Nov 2017 22:17:31 -0500"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <32215.1510935834.1@sss.pgh.pa.us>
Date: Fri, 17 Nov 2017 11:23:54 -0500
Message-ID: <32216.1510935834@sss.pgh.pa.us>
Precedence: bulk

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2017-11-16 21:58:14 -0500, Tom Lane wrote:
>>> [ squint... ]  That used to have an actual purpose connected to
>>> transaction-abort cleanup, IIRC.  It disturbs me that this seems
>>> to have been lost.

>> I've not found any such use, searching through buffile.c's history. I
>> don't quite see how that flag could've been related to abort cleanup
>> stuff?  There's been another unused caller of makeBufFile, namely
>> BufFileCreate, that has been #ifdef'ed out for ages (perhaps we
>> should've removed that with this commit or a long time ago).  Other than
>> that there seems to not have been any other caller setting that flag
>> differently since you created the file in db3c4c3a2d980dcd.

OK, after looking through the history, the reason for isTemp = false
is indeed to allow BufFileCreate() to maintain its old semantics,
wherein you could attach a BufFile to an already-existing, possibly
non-temp file.  There have not been any core callers of BufFileCreate()
in a long time (maybe not since that commit, in fact), but I imagine
I left it alone for fear that extensions might be using it.  I see though
that Bruce ifdef'd it out in 20ad43b5, so there aren't any extensions
using it either.

We should flat-out remove the function, since this change makes it
impossible to resurrect with its old semantics.  I wonder whether
we should then rename BufFileCreateTemp to just BufFileCreate,
since it's no longer possible to have a BufFile that isn't temp.
I suspect that some attention to the comments might be needed too.

Or maybe we should revert 11e264517.  It doesn't seem to be buying
much to make up for the loss of flexibility.

			regards, tom lane