RE: [Patch] Optimize dropping of relation buffers using dlist

From: Amit Kapila <amit.kapila16@gmail.com>
> I agree with the above two points.

Thank you.  I'm relieved to know I didn't misunderstand.


> > * Then, add a new function, say, smgrnblocks_cached() that simply returns
> the cached block count, and DropRelFileNodeBuffers() uses it instead of
> smgrnblocks().
> >
> 
> I am not sure if it worth adding a new function for this. Why not simply add a
> boolean variable in smgrnblocks for this?


One reason is that adding an argument requires modification of existing call sites (10 + a few).  Another is that, although this may be different for each person's taste, it's sometimes not easy to understand when a function call with true/false appears.  One such example is find_XXX(some_args, true/false), where the true/false represents missing_ok.  Another example is as follows.  I often wonder "what's the meaning of this false, and that true?"

    if (!InstallXLogFileSegment(&destsegno, tmppath, false, 0, false))
        elog(ERROR, "InstallXLogFileSegment should not have failed");

Fortunately, the new function is very short and doesn't duplicate much code.  The function is a simple getter and the function name can convey the meaning straight (if the name is good.)


> BTW, AFAICS, the latest patch
> doesn't have code to address this point.

Kirk-san, can you address this?  I don't mind much if you add an argument or a new function.



Regards
Takayuki Tsunakawa