Date: Mon, 29 Jan 2024 16:32:06 +0900 (JST)
Message-Id: <20240129.163206.738744107103873975.horikyota.ntt@gmail.com>
To: hlinnaka@iki.fi
Cc: masao.fujii@gmail.com, andres@anarazel.de, michael@paquier.xyz,
 pgsql-hackers@lists.postgresql.org
Subject: Re: Network failure may prevent promotion
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <ea7b8012-b739-436e-afe4-be0b2f69b304@iki.fi>
References: 
 <CAHGQGwGSQratfsnAwAH6kygmdfv7u+S3UzcwbwB66YzoWZiFaQ@mail.gmail.com>
	<20240123.172410.1596193222420636986.horikyota.ntt@gmail.com>
	<ea7b8012-b739-436e-afe4-be0b2f69b304@iki.fi>
User-Agent: Mew version 6.8 on Emacs 27.2
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Mon_Jan_29_16_32_06_2024_387)--"
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20240129.163206.738744107103873975.horikyota.ntt%40gmail.com>
Precedence: bulk

----Next_Part(Mon_Jan_29_16_32_06_2024_387)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Thank you fixing the issue.

At Tue, 23 Jan 2024 11:43:43 +0200, Heikki Linnakangas <hlinnaka@iki.fi> wrote i
n 
> There's an existing AmWalReceiverProcess() macro too. Let's use that.

Mmm. I sought an Is* function becuase "IsLogicalWorker()" is placed on
the previous line. Our convention regarding those functions (macros)
and variables seems inconsistent. However, I can't say for sure that
we should unify all of them.

> (See also
> https://www.postgresql.org/message-id/f3ecd4cb-85ee-4e54-8278-5fabfb3a4ed0%40iki.fi
> for refactoring in this area)
> 
> Here's a patch set summarizing the changes so far. They should be
> squashed, but I kept them separate for now to help with review:
> 
> 1. revert the revert of 728f86fec6.
> 2. your walrcv_shutdown_deblocking_v2-2.patch
> 3. Also replace libpqrcv_PQexec() and libpqrcv_PQgetResult() with the
> wrappers from libpq-be-fe-helpers.h

Both replacements look fine. I didn't find another instance of similar
code.

> 4. Replace IsWalReceiver() with AmWalReceiverProcess()

Just look fine.

> >> - pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request
> >> - shutdown */
> >> + pqsignal(SIGTERM, WalRcvShutdownSignalHandler); /* request shutdown
> >> */
> >>
> >> Can't we just use die(), instead?
> > There was a comment explaining the problems associated with exiting
> > within a signal handler;
> > - * Currently, only SIGTERM is of interest.  We can't just exit(1) within
> > - * the
> > - * SIGTERM signal handler, because the signal might arrive in the middle
> > - * of
> > - * some critical operation, like while we're holding a spinlock.
> > - * Instead, the
> > And I think we should keep the considerations it suggests. The patch
> > removes the comment itself, but it does so because it implements our
> > standard process exit procedure, which incorporates points suggested
> > by the now-removed comment.
> 
> die() doesn't call exit(1). Unless DoingCommandRead is set, but it
> never is in the walreceiver. It looks just like the new
> WalRcvShutdownSignalHandler() function. Am I missing something?

Ugh.. Doesn't the name 'die()' suggest exit()?
I agree that die() can be used instad.

> Hmm, but doesn't bgworker_die() have that problem with exit(1)ing in
> the signal handler?

I noticed that but ignored for this time.

> I also wonder if we should replace SignalHandlerForShutdownRequest()
> completely with die(), in all processes? The difference is that
> SignalHandlerForShutdownRequest() uses ShutdownRequestPending, while
> die() uses ProcDiePending && InterruptPending to indicate that the
> signal was received. Or do some of the processes want to check for
> ShutdownRequestPending only at specific places, and don't want to get
> terminated at the any random CHECK_FOR_INTERRUPTS()?

At least, pg_log_backend_memory_context(<chkpt_pid>) causes a call to
ProcessInterrupts via "ereport(LOG_SERVER_ONLY" which can lead to an
exit due to ProcDiePending. In this regard, checkpointer clearly
requires the distinction.

Rather than merely consolidating the notification variables and
striving to annihilate CFI calls in the execution path, I
believe we need a shutdown mechanism that CFI doesn't react
to. However, as for the method to achieve this, whether we should keep
the notification variables separate as they are now, or whether it
would be better to introduce a variable that causes CFI to ignore
ProcDiePending, is a matter I think is open to discussion.

Attached patches are the rebased version of v3 (0003 is updated) and
additional 0005 that makes use of die() instead of walreceiver's
custom function.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

----Next_Part(Mon_Jan_29_16_32_06_2024_387)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="v4-0001-Revert-Revert-libpqwalreceiver-Convert-to-libpq-b.patch"