Re: Strange coding in _mdfd_openseg()

At Fri, 5 Apr 2019 18:44:15 +1300, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote in <CA+hUKGKa-OKiNEsWOs+SWugpSE-C7MebejK-dDipaoS17BkRNw(at)mail(dot)gmail(dot)com>
> On Thu, Apr 4, 2019 at 4:16 PM Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > At Wed, 3 Apr 2019 13:47:46 -0700, Andres Freund <andres(at)anarazel(dot)de> wrote in <20190403204746(dot)2yumq7c2mirmodsg(at)alap3(dot)anarazel(dot)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

Only mdsyncfiletag seems expecting that and it is documented. But
_mdfd_getseg is not documented as the same. mdopen also is not.

> 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()?

If both elog() and free() don't change errno, we don't need to do
that at least for AllocSetFree, and is seems to be the same for
other allocators. I think it is better to guarantee (and
document) that errno does not change by pfree(), rather than to
protect in the caller side.a

> 2. It'd be nice if function documentation explicitly said which
> functions set errno on error (and perhaps which syscalls).

I agree about errno. I'm not sure about syscall (names?).

> 3. Why does some code use "file < 0" and other code "file <= 0" to
> detect errors from fd.c functions that return File?

That seems just a thinko, or difference of how to think about
invalid (or impossible) values. Vfd == 0 is invalid and
impossible, so file <=0 and < 0 are effectively the
equivalents. I think we should treat 0 as error rather than
sucess. I don't think it worth to do Assert(file != 0).

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center