pgsql: Revert "Get rid of the dedicated latch for signaling the startup
Revert "Get rid of the dedicated latch for signaling the startup process". Revert ac22929a26, as well as the followup fix 113d3591b8. Because it broke the assumption that the startup process waiting for the recovery conflict on buffer pin should be waken up only by buffer unpin or the timeout enabled in ResolveRecoveryConflictWithBufferPin(). It caused, for example, SIGHUP signal handler or walreceiver process to wake that startup process up unnecessarily frequently. Additionally, add the comments about why that dedicated latch that the reverted patch tried to get rid of should not be removed. Thanks to Kyotaro Horiguchi for the discussion. Author: Fujii Masao Discussion: https://postgr.es/m/[email protected] Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/00f690a239932e477f25120d19b08aacdc30deb7 Modified Files -- src/backend/access/transam/xlog.c | 40 --- src/backend/postmaster/startup.c | 24 ++- src/backend/storage/ipc/standby.c | 9 - 3 files changed, 52 insertions(+), 21 deletions(-)
Re: pgsql: Get rid of the dedicated latch for signaling the startup process
On 2020/11/05 23:32, Fujii Masao wrote:
On 2020/11/05 6:02, Fujii Masao wrote:
On 2020/11/05 5:36, Heikki Linnakangas wrote:
On 04/11/2020 15:17, Heikki Linnakangas wrote:
On 04/11/2020 14:03, Fujii Masao wrote:
Or ISTM that WakeupRecovery() should set the latch only when the latch
has not been reset to NULL yet.
Got to be careful with race conditions, if the latch is set to NULL at
the same time that WakeupRecovery() is called.
I don't think commit 113d3591b8 got this quite right:
void
WakeupRecovery(void)
{
if (XLogCtl->recoveryWakeupLatch)
SetLatch(XLogCtl->recoveryWakeupLatch);
}
If XLogCtl->recoveryWakeupLatch is set to NULL between the if and the SetLatch,
you'll still get a segfault. That's highly unlikely to happen in practice because
the compiler will optimize that into a single load instruction, but could happen
with -O0. I think you'd need to do the access only once, using a volatile pointer,
to make that safe.
On second thought, since fetching the latch pointer might not be atomic,
maybe we need to use spinlock like WalRcvForceReply() does. But using
spinlock in every calls of WakeupRecovery() might cause performance
overhead. So I'm thinking to use spinlock only when it's necessary, i.e.,
when the latch may be reset to NULL concurrently. Attached is the POC
patch implementing that. Thought?
Previously I added this patch to next CommitFest. But I reverted the commit
ac22929a26 and 113d3591b8 because those changes have other issue. So this
patch is no longer necessary, and I dropped it from next CommitFest.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
pgsql: pg_stat_statements: Track time at which all statistics were last
pg_stat_statements: Track time at which all statistics were last reset. This commit adds "stats_reset" column into the pg_stat_statements_info view. This column indicates the time at which all statistics in the pg_stat_statements view were last reset. Per discussion, this commit also changes pg_stat_statements_info code so that "dealloc" column is reset at the same time as "stats_reset" is reset, i.e., whenever all pg_stat_statements entries are removed, for the sake of consistency. Previously "dealloc" was reset only when pg_stat_statements_reset(0, 0, 0) is called and was not reset when pg_stat_statements_reset() with non-zero value argument discards all entries. This was confusing. Author: Naoki Nakamichi, Yuki Seino Reviewed-by: Yuki Seino, Kyotaro Horiguchi, Li Japin, Fujii Masao Discussion: https://postgr.es/m/[email protected] Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/2e0fedf0362cc964c4dae42258455b6391051e70 Modified Files -- .../pg_stat_statements--1.8--1.9.sql | 5 ++- contrib/pg_stat_statements/pg_stat_statements.c| 46 -- doc/src/sgml/pgstatstatements.sgml | 18 +++-- 3 files changed, 53 insertions(+), 16 deletions(-)
