Re: Strange coding in _mdfd_openseg()

On Thu, Apr 4, 2019 at 4:16 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> At Wed, 3 Apr 2019 13:47:46 -0700, Andres Freund <andres@anarazel.de> wrote in
<20190403204746.2yumq7c2mirmodsg@alap3.anarazel.de>
> > Yea, I totally agree it's weird. I'm not sure if I'd go for an assertion
> > of equality, or just invert the >= (which I agree I probably just
> > screwed up and didn't notice when reviewing the patch because it looked
> > close enough to correct and it didn't have a measurable effect).
>
> I looked there and agreed. _mdfd_openseg is always called just to
> add one new segment after the last opened segment by the two
> callers. So I think == is better.

Thanks.  Some other little things I noticed while poking around in this area:

Callers of _mdgetseg(EXTENSION_RETURN_NULL) expect errno to be set if
it returns NULL, and it expects the same of
mdopen(EXTERNSION_RETURN_NULL), and yet the latter does:

    fd = PathNameOpenFile(path, O_RDWR | PG_BINARY);

    if (fd < 0)
    {
        if ((behavior & EXTENSION_RETURN_NULL) &&
            FILE_POSSIBLY_DELETED(errno))
        {
            pfree(path);
            return NULL;
        }

1.  I guess that needs save_errno treatment to protect it from being
clobbered by pfree()?
2.  It'd be nice if function documentation explicitly said which
functions set errno on error (and perhaps which syscalls).
3.  Why does some code use "file < 0" and other code "file <= 0" to
detect errors from fd.c functions that return File?

-- 
Thomas Munro
https://enterprisedb.com