Date: Sun, 31 Mar 2019 15:42:33 -0700
From: Noah Misch <noah@leadboat.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>,
	"9erthalion6@gmail.com" <9erthalion6@gmail.com>,
	"sfrost@snowman.net" <sfrost@snowman.net>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
Message-ID: <20190331224233.GC891537@rfd.leadboat.com>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <GrdLgAdUK9FdyZg8VIcTDKVOkys122ZINEb3CjjoySfGj2KyPiMKTh1zqtRp0TAD7FJ27G-OBB3eplxIB5GhcQH5o8zzGZfp0MuJaXJxVxk=@yesql.se>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

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.