On 16.06.2011 15:07, Peter Geoghegan wrote:
> I had another quick look-over this patch, and realised that I made a
> minor mistake:
>
> +void
> +ReleasePostmasterDeathWatchHandle(void)
> +{
> + /* MyProcPid won't have been set yet */
> + Assert(PostmasterPid != getpid());
> + /* Please don't ask twice */
> + Assert(postmaster_alive_fds[POSTMASTER_FD_OWN] != -1);
> + /* Release parent's ownership fd - only postmaster should hold it */
> + if (close(postmaster_alive_fds[ POSTMASTER_FD_OWN]))
> + {
> + ereport(FATAL,
> + (errcode_for_socket_access(),
> + errmsg("Failed to close file descriptor associated with
> Postmaster death in child process %d", MyProcPid)));
> + }
> + postmaster_alive_fds[POSTMASTER_FD_OWN] = -1;
> +}
> +
>
> MyProcPid is used in this errmsg, and as noted in the first comment,
> it isn't expected to be initialised when
> ReleasePostmasterDeathWatchHandle() is called. Therefore, MyProcPid
> should be replaced with a call to getpid(), just as it is for
> Assert(PostmasterPid != getpid()).
>
> I suppose that you could take the view that MyProcPid ought to be
> initialised before the function is called, but I thought this was the
> least worst way. Better to do it this way than to touch all the
> different ways in which MyProcPid might be initialised, I suspect.
Hmm, I'm not sure having the pid in that error message is too useful in
the first place. The process was just spawned, and it will die at that
error. When you try to debug that sort of error, what you would compare
the pid with? And you can include the pid in log_line_prefix if it turns
out to be useful after all.
PS. error messages should begin with lower-case letter.
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com