Re: Strange coding in _mdfd_openseg()

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: thomas(dot)munro(at)gmail(dot)com
Cc: andres(at)anarazel(dot)de, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Strange coding in _mdfd_openseg()
Date: 2019-04-08 06:34:33
Message-ID: 20190408.153433.177814321.horiguchi.kyotaro@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2019-04-08 06:37:03 Re: clean up pg_checksums.sgml
Previous Message Michael Paquier 2019-04-08 06:17:25 Re: pg_rewind vs superuser