Re: [HACKERS] PATCH: Keep one postmaster monitoring pipe per process

At Sun, 2 Sep 2018 07:04:19 +1200, Thomas Munro <thomas.munro@enterprisedb.com> wrote in <CAEepm=0=PkSXQ5oNU8BhY1DEzxw_EspsU14D_zAPnj+fBAjxFQ@mail.gmail.com>
> > > # Is it intentional that the patch doesn't touch pgstat.c?
> >
> > Yes.  pgstat.c still uses WL_POSTMASTER_DEATH because it does
> > something special: it calls pgstat_write_statsfiles() before it exits.

Mmm. Exactly..

> Rebased.

Thank you for the new version.

===
In sysloger.c, cur_flags is (just set but) no longer used.

===
In latch.c,

- The parentheses around the symbols don't seem to be needed.
|           (wakeEvents & (WL_EXIT_ON_PM_DEATH)) != 0 ||
|           (wakeEvents & (WL_POSTMASTER_DEATH)) != 0);

- The following assertion looks contradicting to the comment.
|    /* Postmaster-managed callers must handle postmaster death somehow. */
|    Assert(!IsUnderPostmaster ||
|           (wakeEvents & (WL_EXIT_ON_PM_DEATH)) != 0 ||
|           (wakeEvents & (WL_POSTMASTER_DEATH)) != 0);

  (Maybe Assert(IsUnderPost && (wakeEv & (WL_EXI | WL_PO)) != 0);)

  And don't we need a description about this restriction in the
  function comment?

- I think it may be better that the followings had parentheses
  around '&' expressions.

|    if (wakeEvents & WL_POSTMASTER_DEATH && IsUnderPostmaster)
|    if (wakeEvents & WL_EXIT_ON_PM_DEATH && IsUnderPostmaster)


===
All the caller sites of WaitLatch, WaitLatchOrSocket and
WaitEventSetWait are covered by this patch and all them look
fine.


bgworker.c, pgstat.c, be-secure-openssl.c, be-secure.c:
  Not modified on purpose. WL_EXIT_POSTMASTER_DEATH is not proper
  to use there.

pgarch.c, syncrep.c, walsender.c:
  Removed redundant check of postmaster death.

syslogger.c:
  Left as it doesn't exit at postmaster death on purpose.  Uses
  reusable wait event set.

walrceiver.c:
  Adds new bailing out point in WalRcvWaitForStartPosition and it
  seems reasonable.

shm_mq.c:
  Adds PMdie bail out in shm_mq_send/receive_bytes, wait_internal.
  It looks fine.

postgres_fdw/connection.c:
  Adds pm_die bailout while getting result. This seems to be a bug fix.
  I'm fine with it included in this patch.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center