From: Amit Kapila <[email protected]>
> 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
