Re: smgrzeroextend clarification

Hi,

On 2023-05-16 20:40:01 +0200, Peter Eisentraut wrote:
> On 10.05.23 20:10, Andres Freund wrote:
> > > So if you want to understand what smgrzeroextend() does, you need to
> > > mentally combine the documentation of three different functions. Could we
> > > write documentation for each function that stands on its own? And document
> > > the function arguments, like what does blocknum and nblocks mean?
> > I guess it couldn't hurt. But if we go down that route, we basically need to
> > rewrite all the function headers in smgr.c, I think.
>
> I took a stab at this, going through the function comments in smgr.c and
> md.c and try to make some things easier to follow.
>
> - Took at out the weird leading tabs in the older comments.

I hate those. I wonder if we could devise a regex that'd remove them
tree-wide, instead of ending up doing it very slowly one by one - I've
e.g. removed them from a bunch of pgstat files. Reflowing the comments after
is probably the most painful part.

FWIW, I'd do that in a separate commit, and then add that commit to
.git-blame-ignore-revs. Otherwise blaming gets unnecessarily painful. Also
makes it easier to see the actual content changes...

> - Rephrased some comments so that smgr.c is more like an API documentation
> and md.c documents what that particular instance of that API does.
>
> - Move the *extend and *zeroextend functions to a more sensible place among
> all the functions. Especially since *write and *extend are very similar, it
> makes sense to have them close together. This way, it's also easier to
> follow "this function is like that function" comments.

For me the prior location made a bit more sense - we should always extend the
file before writing or reading the relevant blocks. But I don't really care.

> - Also moved mdwriteback(), which was in some completely odd place.
>
> - Added comments for function arguments that were previously not documented.
>
> - Reworded the comments for *extend and *zeroextend to make more sense
> relative to each other.
>
> - I left this for smgrzeroextend():
>
> FIXME: why both blocknum and nblocks

I guess you're suggesting that we would do an lstat() to figure out the size
instead? Or use some cached size? That'd not be trivial to add - and it just
seems unrelated to smgzerorextend(), it's just as true for smgrextend().

> Like, what happens when blocknum is not the block right after the last
> existing block?

The same as with smgrextend().

> Do you get an implicit POSIX hole between the end of the file and the
> specified block and then an explicit hole for the next nblocks?

I don't know what you mean with an explicit hole? The whole point of
posix_fallocate() is that it actually allocates blocks - I don't really see
how such a space could be described as a hole? My understanding of the
"implicit POSIX hole" semantics is that the point precisely is that there is
*no* space allocated for the region.

> We should be clear here what the designer of this API intended.
>
>
> The name smgrzeroextend is not great. The "zero" isn't what distinguishes
> it from smgrextend.

I think it's a quite distinguishing feature - you can't provide initial block
contents, which you can with smgrextend() (and we largely do). And using
fallocate() is only possible because we know that we're intending to read
zeros.

Greetings,

Andres Freund