Thread: pgsql: Remove BufFile's isTemp flag.
Remove BufFile's isTemp flag. The isTemp flag controls whether buffile.c chops BufFile data up into 1GB segments on disk. Since it was badly named and always true, get rid of it. Author: Thomas Munro (based on suggestion by Peter Geoghegan) Reviewed-By: Peter Geoghegan, Andres Freund Discussion: https://postgr.es/m/CAH2-Wz%3D%2B9Rfqh5UdvdW9rGezdhrMGGH-JL1X9FXXVZdeeGeOJA%40mail.gmail.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/11e264517dff7a911d9e6494de86049cab42cde3 Modified Files -------------- src/backend/storage/file/buffile.c | 43 ++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 25 deletions(-)
Andres Freund <andres@anarazel.de> writes:
> Remove BufFile's isTemp flag.
> The isTemp flag controls whether buffile.c chops BufFile data up into
> 1GB segments on disk. Since it was badly named and always true, get
> rid of it.
[ squint... ] That used to have an actual purpose connected to
transaction-abort cleanup, IIRC. It disturbs me that this seems
to have been lost.
regards, tom lane
On 2017-11-16 21:58:14 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Remove BufFile's isTemp flag. > > The isTemp flag controls whether buffile.c chops BufFile data up into > > 1GB segments on disk. Since it was badly named and always true, get > > rid of it. > > [ squint... ] That used to have an actual purpose connected to > transaction-abort cleanup, IIRC. It disturbs me that this seems > to have been 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. Greetings, Andres Freund
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.
I'm tired for today, but will take a closer look tomorrow. I do not
think the mechanism was created without a purpose ...
regards, tom lane
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
On Fri, Nov 17, 2017 at 8:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > 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. +1 > Or maybe we should revert 11e264517. It doesn't seem to be buying > much to make up for the loss of flexibility. Thomas wrote code that makes it possible to extend individual BufFiles with other BufFiles across backends. This code will be used by parallel CREATE INDEX, though it is something included in recent versions of his parallel hash join patchset. This process happens at a higher level than buffile.c, and should get the tricky details of resource management right. I think it's likely that this will be committed for v11. -- Peter Geoghegan
Hi, On 2017-11-17 11:23:54 -0500, Tom Lane wrote: > 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. That sounds reasonable. > 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. Thomas? > Or maybe we should revert 11e264517. It doesn't seem to be buying > much to make up for the loss of flexibility. There's a bunch of work adding new functionality to buffile.c pending. And having code paths that have been dead for 10+ years around and maintaining them in working order doesn't seem like a good use of time. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2017-11-17 11:23:54 -0500, Tom Lane wrote:
>> Or maybe we should revert 11e264517. It doesn't seem to be buying
>> much to make up for the loss of flexibility.
> There's a bunch of work adding new functionality to buffile.c
> pending. And having code paths that have been dead for 10+ years around
> and maintaining them in working order doesn't seem like a good use of
> time.
OK, if there's active work going on in the area, that's a good reason
to simplify. But let's get rid of all vestiges of the old non-temp
semantics.
regards, tom lane
On Sat, Nov 18, 2017 at 6:58 AM, Andres Freund <andres@anarazel.de> wrote: > On 2017-11-17 11:23:54 -0500, Tom Lane wrote: >> 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. > > That sounds reasonable. > > >> 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. > > Thomas? Here's a patch that does those things. I'm slightly surprised by the renaming suggestion though, because it means that an extension that uses BufFile will need to know how to select the v10 and v11 function name as appropriate. Would you backpatch redirect support for the new name to older versions? -- Thomas Munro http://www.enterprisedb.com
Attachment
Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> On 2017-11-17 11:23:54 -0500, Tom Lane wrote:
>>> 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.
> Here's a patch that does those things. I'm slightly surprised by the
> renaming suggestion though, because it means that an extension that
> uses BufFile will need to know how to select the v10 and v11 function
> name as appropriate. Would you backpatch redirect support for the new
> name to older versions?
No, but if you're concerned about it, we could maintain API compatibility
for extensions with something like
#define BufFileCreateTemp(interXact) BufFileCreate(interXact)
Typically we expect extensions to provide such workarounds for cases that
concern them ... but since this change is purely cosmetic, maybe it should
be treated differently.
regards, tom lane
On 2017-11-19 17:00:36 -0500, Tom Lane wrote: > Thomas Munro <thomas.munro@enterprisedb.com> writes: > >> On 2017-11-17 11:23:54 -0500, Tom Lane wrote: > >>> 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. > > > Here's a patch that does those things. I'm slightly surprised by the > > renaming suggestion though, because it means that an extension that > > uses BufFile will need to know how to select the v10 and v11 function > > name as appropriate. Would you backpatch redirect support for the new > > name to older versions? > > No, but if you're concerned about it, we could maintain API compatibility > for extensions with something like > > #define BufFileCreateTemp(interXact) BufFileCreate(interXact) I don't really see a point in doing this renaming in the first place. It's not like the Temp suffix has become inaccurate. I'd perhaps not add it in the green field, but I don't see a need to change an existing function name. If anything it seems confusing because you'd miss something when trivially searching the history / comparing branches. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2017-11-19 17:00:36 -0500, Tom Lane wrote:
>> No, but if you're concerned about it, we could maintain API compatibility
>> for extensions with something like
>> #define BufFileCreateTemp(interXact) BufFileCreate(interXact)
> I don't really see a point in doing this renaming in the first
> place. It's not like the Temp suffix has become inaccurate. I'd perhaps
> not add it in the green field, but I don't see a need to change an
> existing function name. If anything it seems confusing because you'd
> miss something when trivially searching the history / comparing
> branches.
Well, that's a fair point about history, but the reason I no longer
want the Temp suffix is that it implies that there's such a thing as
a non-temp BufFile. I think that's misleading if we've cut off any
vestige of support for it.
Anybody else have an opinion?
regards, tom lane
Andres Freund <andres@anarazel.de> writes:
> I don't really see a point in doing this renaming in the first
> place. It's not like the Temp suffix has become inaccurate. I'd perhaps
> not add it in the green field, but I don't see a need to change an
> existing function name. If anything it seems confusing because you'd
> miss something when trivially searching the history / comparing
> branches.
It seems that the vote is 2-1 against renaming that function, so I've
committed Thomas' patch without that and with some additional
comment-smithing.
regards, tom lane