From: Noah Misch <noah@leadboat.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: 9erthalion6@gmail.com <9erthalion6@gmail.com>
Cc: sfrost@snowman.net <sfrost@snowman.net>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
Date: Sun, 31 Mar 2019 15:42:33 -0700
Message-ID: <20190331224233.GC891537@rfd.leadboat.com> (raw)
In-Reply-To: <GrdLgAdUK9FdyZg8VIcTDKVOkys122ZINEb3CjjoySfGj2KyPiMKTh1zqtRp0TAD7FJ27G-OBB3eplxIB5GhcQH5o8zzGZfp0MuJaXJxVxk=@yesql.se>
References: <CA+q6zcV4r+oS27G-nYqonG+q7442BOA350s3jeRtA-Oqx4vDWg@mail.gmail.com>
<20181130065140.GB2774301@rfd.leadboat.com>
<20181203004106.GA2860387@rfd.leadboat.com>
<20190304.180420.139624607.horiguchi.kyotaro@lab.ntt.co.jp>
<20190307032422.GB1797215@rfd.leadboat.com>
<20190309071610.GA2039121@rfd.leadboat.com>
<GrdLgAdUK9FdyZg8VIcTDKVOkys122ZINEb3CjjoySfGj2KyPiMKTh1zqtRp0TAD7FJ27G-OBB3eplxIB5GhcQH5o8zzGZfp0MuJaXJxVxk=@yesql.se>
On Fri, Mar 29, 2019 at 09:53:51AM +0000, Daniel Gustafsson wrote:
> On Saturday, March 9, 2019 8:16 AM, Noah Misch <noah@leadboat.com> wrote:
> > I renamed IpcMemoryAnalyze() to PGSharedMemoryAttach() and deleted the old
> > function of that name. Now, this function never calls shmdt(); the caller is
> > responsible for that. I do like things better this way. What do you think?
>
> I think it makes for a good API for the caller to be responsible, but it does
> warrant a comment on the function to explicitly state that.
The name "PGSharedMemoryAttach" makes that fact sufficiently obvious, I think.
> A few other small comments:
>
> + state = PGSharedMemoryAttach((IpcMemoryId) id2, &memAddress);
> + if (memAddress)
> + shmdt(memAddress);
>
> This seems like a case where it would be useful to log a shmdt() error or do
> an Assert() around the success of the operation perhaps?
I'll add the same elog(LOG) we have at other shmdt() sites. I can't think of
a site where we Assert() about the results of a system call. While shmdt()
might be a justified exception, elog(LOG) seems reasonable.
> + * Loop till we find a free IPC key. Trust CreateDataDirLockFile() to
> + * ensure no more than one postmaster per data directory can enter this
> + * loop simultaneously. (CreateDataDirLockFile() does not ensure that,
> + * but prefer fixing it over coping here.)
>
> This comment make it seem like there is a fix to CreateLockFile() missing to
> his patch, is that correct? If so, do you have an idea for that patch?
That comment refers to
https://postgr.es/m/flat/20120803145635.GE9683%40tornado.leadboat.com> Switching this to Ready for Committer since I can't see anything but tiny things.
Thanks.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, daniel@yesql.se, horiguchi.kyotaro@lab.ntt.co.jp, 9erthalion6@gmail.com, sfrost@snowman.net
Subject: Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
In-Reply-To: <20190331224233.GC891537@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox