On Wed, Mar 06, 2019 at 07:24:22PM -0800, Noah Misch wrote:
> On Mon, Mar 04, 2019 at 06:04:20PM +0900, Kyotaro HORIGUCHI wrote:
> > PGSharedMemoryCreate changed to choose a usable shmem id using
> > the IpcMemoryAnalyze(). But some of the statuses from
> > IpcMemoryAnalyze() is concealed by failure of
> > PGSharedMemoryAttach() and ignored silently opposed to what the
> > code is intending to do.
>
> SHMSTATE_FOREIGN is ignored silently. The patch modifies the
> PGSharedMemoryAttach() header comment to direct callers to treat
> PGSharedMemoryAttach() failure like SHMSTATE_FOREIGN. I don't think the code
> had an unintentional outcome due to the situation you describe. Perhaps I
> could have made the situation clearer.
I think v3, attached, avoids this appearance of unintended behavior.
> > (By the way SHMSTATE_EEXISTS seems
> > suggesting oppsite thing from EEXIST, which would be confusing.)
>
> Good catch. Is SHMSTATE_ENOENT better?
I did s/SHMSTATE_EEXISTS/SHMSTATE_ENOENT/.
> > PGSharedMemoryCreate() repeats shmat/shmdt twice in every
> > iteration. It won't harm so much but it would be better if we
> > could get rid of that.
>
> I'll try to achieve that and see if the code turns out cleaner.
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?