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

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)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