Horton

Re: shared-memory based stats collector

Kyotaro Horiguchi <horikyota.ntt@gmail.com>

View thread View on postgr.es
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
To: masao.fujii@oss.nttdata.com
Cc: andres@anarazel.de, gkokolatos@protonmail.com, pgsql-hackers@lists.postgresql.org
Date: 2021-03-15T08:49:36Z
Lists: pgsql-hackers 
At Fri, 12 Mar 2021 23:33:05 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2021/03/12 17:24, Kyotaro Horiguchi wrote:
> > At Fri, 12 Mar 2021 15:13:15 +0900, Fujii Masao
> > <masao.fujii@oss.nttdata.com> wrote in
> >> On 2021/03/12 13:49, Kyotaro Horiguchi wrote:
> >>> I noticed that I accidentally removed the launch-suppression feature
> >>> that is to avoid frequent relaunching.  That mechanism is needed on
> >>> the postmaster side. I added PgArchIsSuppressed() to do the same check
> >>> with the old pgarch_start() and make it called as a part of
> >>> PgArchStartupAllowed().
> >>
> >> You're right! At least for me the function name PgArchIsSuppressed()
> >> sounds not good to me. What about something like PgArchCanRestart()?
> > The reason for the name was some positive-meaning names came up with
> > me are confusing with PgArchStartupAllowed().  The name
> > PgArchCanRestart suggests that it's usable only when
> > restarting. However, the function requires to be called also when the
> > first launch since last_pgarch_start_time needs to be updated every
> > time archiver is launched.
> > Anyway in the attached the name is changed wihtout changing its usage.
> 
> Thanks! If we come up with better name, let's rename the function
> later.

Ok.

> > # I don't like it uses "can" as "allowed" so much. The archiver
> > # actually can restart but is just inhibited to restart.
> > 
> >> This is not fault of this patch. But last_pgarch_start_time should be
> >> initialized with zero?
> > Right. I noticed that but forgot to fix it.
> > 
> >> +	if ((curtime - last_pgarch_start_time) < PGARCH_RESTART_INTERVAL)
> >> +		return true;
> >>
> >> Why did you remove the cast to unsigned int there?
> > The cast converts small negative values to large numbers, the code
> > looks like intending to allow archiver launched when curtime goes
> > behind than last_pgarch_start_time. That is the case the on-memory
> > data is corrupt. I'm not sure it's worth worrying and in the first
> > place if we want to care of the case we should explicitly compare the
> > operands of the subtraction.  I did that in the attached.
> 
> That's an idea. But the similar calculation using that cast is used in
> other places (e.g., in pgarch_MainLoop()), so I'm thinking that it's
> better not to change that...

Mmm. I'm fine with it:( (:p)

> > And the last_pgarch_start_time is accessed only in the function. I
> > moved it to inside the function.
> 
> OK.
> 
> 
> > 
> >> +	/*
> >> + * Advertise our latch that backends can use to wake us up while
> >> we're
> >> +	 * sleeping.
> >> +	 */
> >> +	PgArch->pgprocno = MyProc->pgprocno;
> >>
> >> The comment should be updated?
> > Hmm. What is advertised is our pgprocno.. Fixed.
> 
> OK.
> 
> Thanks for updating the patch! I applied some minor changes into your
> patch.
> Attached is the updated version of the patch. I'm thinking to commit
> this version.

Thanks for committing this!  I'm very happy to see this reduces the
size of this patchset.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Commits

  1. Improve comment about dropped entries in pgstat.c

  2. Fix temporary memory leak in system table index scans

  3. pgstat: set timestamps of fixed-numbered stats after a crash.

  4. pgstat: Update docs to match the shared memory stats reality.

  5. pgstat: Hide instability in stats.spec with -DCATCACHE_FORCE_RELEASE.

  6. pgstat: add/extend tests for resetting various kinds of stats.

  7. Add minimal tests for recovery conflict handling.

  8. pgstat: test stats interactions with physical replication.

  9. pgstat: add tests for handling of restarts, including crashes.

  10. pgstat: add tests for transaction behaviour, 2PC, function stats.

  11. pgstat: add pg_stat_have_stats() test helper.

  12. pgstat: add pg_stat_force_next_flush(), use it to simplify tests.

  13. pgstat: move pgstat.c to utils/activity.

  14. pgstat: store statistics in shared memory.

  15. pgstat: remove stats_temp_directory.

  16. pgstat: rename STATS_COLLECTOR GUC group to STATS_CUMULATIVE.

  17. pgstat: revise replication slot API in preparation for shared memory stats.

  18. pgstat: scaffolding for transactional stats creation / drop.

  19. pgstat: introduce PgStat_Kind enum.

  20. pgstat: prepare APIs used by pgstatfuncs for shared memory stats.

  21. pgstat: add pgstat_copy_relation_stats().

  22. pgstat: rename some pgstat_send_* functions to pgstat_report_*.

  23. pgstat: stats collector references in comments.

  24. pgstat: move transactional code into pgstat_xact.c.

  25. dsm: allow use in single user mode.

  26. dshash: revise sequential scan support.

  27. pgstat: remove some superflous comments from pgstat.h.

  28. pgstat: reorder pgstat.[ch] contents.

  29. pgstat: split different types of stats into separate files.

  30. pgstat: introduce pgstat_relation_should_count().

  31. pgstat: separate "xact level" handling out of relation specific functions.

  32. pgstat: rename pgstat_initstats() to pgstat_relation_init().

  33. pgstat: split out WAL handling from pgstat_{initialize,report_stat}.

  34. pgstat: run pgindent on pgstat.c/h.

  35. pgstat: split relation, database handling out of pgstat_report_stat().

  36. Move code around in StartupXLOG().

  37. pgstat: Prepare to use mechanism for truncated rels also for droppped rels.

  38. pgstat: Split out relation stats handling from AtEO[Sub]Xact_PgStat() etc.

  39. pgstat: Schedule per-backend pgstat shutdown via before_shmem_exit().

  40. Schedule ShutdownXLOG() in single user mode using before_shmem_exit().

  41. Make parallel worker shutdown complete entirely via before_shmem_exit().

  42. pgstat: Bring up pgstat in BaseInit() to fix uninitialized use of pgstat by AV.

  43. pgstat: split reporting/fetching of bgwriter and checkpointer stats.

  44. Split backend status and progress related functionality out of pgstat.c.

  45. Split wait event related code from pgstat.[ch] into wait_event.[ch].

  46. Make archiver process an auxiliary process.

  47. Force to send remaining WAL stats to the stats collector at walwriter exit.

  48. Add pg_stat_database counters for sessions and session time

  49. Collect statistics about SLRU caches

  50. Don't run atexit callbacks in quickdie signal handlers.

  51. Create a "fast path" for acquiring weak relation locks.