I wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> Both of these, as attached up thread.
>> Simon's patch - dropallforks.v1.patch
>> Jeff's patch - DropRelFileNodeBuffers_unlock_v1.patch
>> (needs a little tidyup)
> OK, will take a look.
I didn't like dropallforks.v1.patch at all as presented, for several
reasons:
* Introducing an AllForks notation that only works in some contexts is
a foot-gun of large caliber. This concern is not academic: you broke
dropping of temp relations entirely, in the patch as presented, because
for temp rels DropRelFileNodeBuffers would hand off to
DropRelFileNodeLocalBuffers and the latter had not been taught about
AllForks.
* Since we have found out in this thread that the inner loop of
DropRelFileNodeBuffers is performance-critical for the cases we're
dealing with, it seems inappropriate to me to make its tests more
complex. We want simpler, and we can have simpler given that the
relation-drop case cares neither about fork nor block number.
* The patch modified behavior of XLogDropRelation, which has not been
shown to be performance-critical, and probably can't be because the
hashtable it searches should never be all that large. It certainly
doesn't matter to the speed of normal execution.
I thought it would be a lot safer and probably a little bit quicker
if we just split DropRelFileNodeBuffers into two routines, one for
the specific-fork case and one for the all-forks case; and then the
same for its main caller smgrdounlink. So I modified the patch along
those lines and committed it.
As committed, the smgrdounlinkfork case is actually dead code; it's
never called from anywhere. I left it in place just in case we want
it someday.
regards, tom lane
