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,
althoughthis may be different for each person's taste, it's sometimes not easy to understand when a function call with
true/falseappears. One such example is find_XXX(some_args, true/false), where the true/false represents missing_ok.
Anotherexample 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
functionname 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
