Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix async.c to not register any SLRU stats counts in the postmaster.

  2. Use proper GetDatum function in pg_stat_get_slru().

  3. Initialize SLRU stats entries to zero.

  4. Remove pg_xact from pg_stat_reset_slru docs

  5. Remove pg_xact entry from SLRU stats

  6. Track SLRU page hits in SimpleLruReadPage_ReadOnly

  7. Fix typo in SLRU stats documentation

  8. Collect statistics about SLRU caches

  9. Simplify and rename some GUC variables, per various recent discussions:

  1. SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-01-19T14:37:07Z

    Hi,

One of the stats I occasionally wanted to know are stats for the SLRU
stats (we have couple of those - clog, subtrans, ...). So here is a WIP
version of a patch adding that.

The implementation is fairly simple - the slru code updates counters in
local memory, and then sends them to the collector at the end of the
transaction (similarly to table/func stats). The collector stores it
similarly to global stats. And the collected stats are accessible
through pg_stat_slru.

The main issue is that we have multiple SLRU caches, and it seems handy
to have separate stats for each. OTOH the number of SLRU stats is not
fixed, so e.g. extensions might define their own SLRU caches. But
handing dynamic number of SLRU caches seems a bit hard (we'd need to
assign some sort of unique IDs etc.) so what I did was define a fixed
number of SLRU types

     typedef enum SlruType
     {
         SLRU_CLOG,
         SLRU_COMMIT_TS,
         SLRU_MULTIXACT_OFFSET,
         SLRU_MULTIXACT_MEMBER,
         SLRU_SUBTRANS,
         SLRU_ASYNC,
         SLRU_OLDSERXID,
         SLRU_OTHER
     } SlruType;

with one group of counters for each group. The last type (SLRU_OTHER) is
used to store stats for all SLRUs that are not predefined. It wouldn't
be that difficult to store dynamic number of SLRUs, but I'm not sure how
to solve issues with identifying SLRUs etc. And there are probably very
few extensions adding custom SLRU anyway.

The one thing missing from the patch is a way to reset the SLRU stats,
similarly to how we can reset bgwriter stats.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  2. RE: SLRU statistics

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2020-01-20T01:04:33Z

    From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
> One of the stats I occasionally wanted to know are stats for the SLRU
> stats (we have couple of those - clog, subtrans, ...). So here is a WIP
> version of a patch adding that.

How can users take advantage of this information?  I think we also need the ability to set the size of SLRU buffers.  (I want to be freed from the concern about the buffer shortage by setting the buffer size to its maximum.  For example, CLOG would be only 1 GB.)


Regards
Takayuki Tsunakawa

  3. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-01-20T16:37:54Z

    On Mon, Jan 20, 2020 at 01:04:33AM +0000, tsunakawa.takay@fujitsu.com wrote:
>From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
>> One of the stats I occasionally wanted to know are stats for the SLRU
>> stats (we have couple of those - clog, subtrans, ...). So here is a WIP
>> version of a patch adding that.
>
>How can users take advantage of this information?  I think we also need
>the ability to set the size of SLRU buffers.  (I want to be freed from
>the concern about the buffer shortage by setting the buffer size to its
>maximum.  For example, CLOG would be only 1 GB.)
>

You're right the users can't really take advantage of this - my primary
motivation was providing a feedback for devs, benchmarking etc. That
might have been done with DEBUG messages or something, but this seems
more convenient.

I think it's unclear how desirable / necessary it is to allow users to
tweak those caches. I don't think we should have a GUC for everything,
but maybe there's some sort of heuristics to determine the size. The
assumption is we actually find practical workloads where the size of
these SLRUs is a performance issue.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  4. Re: SLRU statistics

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-01-20T18:01:36Z

    On 2020-Jan-20, Tomas Vondra wrote:

> On Mon, Jan 20, 2020 at 01:04:33AM +0000, tsunakawa.takay@fujitsu.com wrote:
> > From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
> > > One of the stats I occasionally wanted to know are stats for the SLRU
> > > stats (we have couple of those - clog, subtrans, ...). So here is a WIP
> > > version of a patch adding that.
> > 
> > How can users take advantage of this information?  I think we also need
> > the ability to set the size of SLRU buffers.  (I want to be freed from
> > the concern about the buffer shortage by setting the buffer size to its
> > maximum.  For example, CLOG would be only 1 GB.)
> 
> You're right the users can't really take advantage of this - my primary
> motivation was providing a feedback for devs, benchmarking etc. That
> might have been done with DEBUG messages or something, but this seems
> more convenient.

I think the stats are definitely needed if we keep the current code.
I've researched some specific problems in this code, such as the need
for more subtrans SLRU buffers; IIRC it was pretty painful to figure out
what the problem was without counters, and it'd have been trivial with
them.

> I think it's unclear how desirable / necessary it is to allow users to
> tweak those caches. I don't think we should have a GUC for everything,
> but maybe there's some sort of heuristics to determine the size. The
> assumption is we actually find practical workloads where the size of
> these SLRUs is a performance issue.

I expect we'll eventually realize the need for changes in this area.
Either configurability in the buffer pool sizes, or moving them to be
part of shared_buffers (IIRC Thomas Munro had a patch for this.)
Example: SLRUs like pg_commit and pg_subtrans have higher buffer
consumption as the range of open transactions increases; for many users
this is not a concern and they can live with the default values.

(I think when pg_commit (née pg_clog) buffers were increased, we should
have increased pg_subtrans buffers to match.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  5. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-01-20T18:45:46Z

    On Mon, Jan 20, 2020 at 03:01:36PM -0300, Alvaro Herrera wrote:
>On 2020-Jan-20, Tomas Vondra wrote:
>
>> On Mon, Jan 20, 2020 at 01:04:33AM +0000, tsunakawa.takay@fujitsu.com wrote:
>> > From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
>> > > One of the stats I occasionally wanted to know are stats for the SLRU
>> > > stats (we have couple of those - clog, subtrans, ...). So here is a WIP
>> > > version of a patch adding that.
>> >
>> > How can users take advantage of this information?  I think we also need
>> > the ability to set the size of SLRU buffers.  (I want to be freed from
>> > the concern about the buffer shortage by setting the buffer size to its
>> > maximum.  For example, CLOG would be only 1 GB.)
>>
>> You're right the users can't really take advantage of this - my primary
>> motivation was providing a feedback for devs, benchmarking etc. That
>> might have been done with DEBUG messages or something, but this seems
>> more convenient.
>
>I think the stats are definitely needed if we keep the current code.
>I've researched some specific problems in this code, such as the need
>for more subtrans SLRU buffers; IIRC it was pretty painful to figure out
>what the problem was without counters, and it'd have been trivial with
>them.
>

Right. Improving our ability to monitor/measure things is the goal of
this patch.

>> I think it's unclear how desirable / necessary it is to allow users to
>> tweak those caches. I don't think we should have a GUC for everything,
>> but maybe there's some sort of heuristics to determine the size. The
>> assumption is we actually find practical workloads where the size of
>> these SLRUs is a performance issue.
>
>I expect we'll eventually realize the need for changes in this area.
>Either configurability in the buffer pool sizes, or moving them to be
>part of shared_buffers (IIRC Thomas Munro had a patch for this.)
>Example: SLRUs like pg_commit and pg_subtrans have higher buffer
>consumption as the range of open transactions increases; for many users
>this is not a concern and they can live with the default values.
>
>(I think when pg_commit (née pg_clog) buffers were increased, we should
>have increased pg_subtrans buffers to match.)
>

Quite possibly, yes. All I'm saying is that it's not something I intend
to address with this patch. It's quite possible the solutions will be
different for each SLRU, and that will require more research.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. RE: SLRU statistics

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2020-01-21T06:24:29Z

    From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
> You're right the users can't really take advantage of this - my primary
> motivation was providing a feedback for devs, benchmarking etc. That
> might have been done with DEBUG messages or something, but this seems
> more convenient.

Understood.  I'm in favor of adding performance information even if it doesn't make sense for users (like other DBMSs sometimes do.)  One concern is that all the PostgreSQL performance statistics have been useful so far for tuning in some way, and this may become the first exception.  Do we describe the SLRU stats view in the manual, or hide it only for PG devs and support staff?


> I think it's unclear how desirable / necessary it is to allow users to
> tweak those caches. I don't think we should have a GUC for everything,
> but maybe there's some sort of heuristics to determine the size. The
> assumption is we actually find practical workloads where the size of
> these SLRUs is a performance issue.

I understood that the new performance statistics are expected to reveal what SLRUs need to be tunable and/or implemented with a different mechanism like shared buffers.


Regards
Takayuki Tsunakawa

  7. Re: SLRU statistics

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-01-21T08:09:33Z

    On Tue, 21 Jan 2020 at 01:38, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>
> On Mon, Jan 20, 2020 at 01:04:33AM +0000, tsunakawa.takay@fujitsu.com wrote:
> >From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
> >> One of the stats I occasionally wanted to know are stats for the SLRU
> >> stats (we have couple of those - clog, subtrans, ...). So here is a WIP
> >> version of a patch adding that.

+1

> >
> >How can users take advantage of this information?  I think we also need
> >the ability to set the size of SLRU buffers.  (I want to be freed from
> >the concern about the buffer shortage by setting the buffer size to its
> >maximum.  For example, CLOG would be only 1 GB.)
> >
>
> You're right the users can't really take advantage of this - my primary
> motivation was providing a feedback for devs, benchmarking etc. That
> might have been done with DEBUG messages or something, but this seems
> more convenient.

I've not tested the performance impact but perhaps we might want to
disable these counter by default and controlled by a GUC. And similar
to buffer statistics it might be better to inline
pgstat_count_slru_page_xxx function for better performance.

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  8. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-01-21T13:56:14Z

    On Tue, Jan 21, 2020 at 05:09:33PM +0900, Masahiko Sawada wrote:
>On Tue, 21 Jan 2020 at 01:38, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>>
>> On Mon, Jan 20, 2020 at 01:04:33AM +0000, tsunakawa.takay@fujitsu.com wrote:
>> >From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
>> >> One of the stats I occasionally wanted to know are stats for the SLRU
>> >> stats (we have couple of those - clog, subtrans, ...). So here is a WIP
>> >> version of a patch adding that.
>
>+1
>
>> >
>> >How can users take advantage of this information?  I think we also need
>> >the ability to set the size of SLRU buffers.  (I want to be freed from
>> >the concern about the buffer shortage by setting the buffer size to its
>> >maximum.  For example, CLOG would be only 1 GB.)
>> >
>>
>> You're right the users can't really take advantage of this - my primary
>> motivation was providing a feedback for devs, benchmarking etc. That
>> might have been done with DEBUG messages or something, but this seems
>> more convenient.
>
>I've not tested the performance impact but perhaps we might want to
>disable these counter by default and controlled by a GUC. And similar
>to buffer statistics it might be better to inline
>pgstat_count_slru_page_xxx function for better performance.
>

Hmmm, yeah. Inlining seems like a good idea, and maybe we should have
something like track_slru GUC.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-01-21T13:59:51Z

    On Tue, Jan 21, 2020 at 06:24:29AM +0000, tsunakawa.takay@fujitsu.com
wrote:
>From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
>> You're right the users can't really take advantage of this - my
>> primary motivation was providing a feedback for devs, benchmarking
>> etc. That might have been done with DEBUG messages or something, but
>> this seems more convenient.
>
>Understood.  I'm in favor of adding performance information even if it
>doesn't make sense for users (like other DBMSs sometimes do.)  One
>concern is that all the PostgreSQL performance statistics have been
>useful so far for tuning in some way, and this may become the first
>exception.  Do we describe the SLRU stats view in the manual, or hide
>it only for PG devs and support staff?
>

Yes, the pg_stat_slru view should be described in a manual. That's
missing from the patch.

>
>> I think it's unclear how desirable / necessary it is to allow users
>> to tweak those caches. I don't think we should have a GUC for
>> everything, but maybe there's some sort of heuristics to determine
>> the size. The assumption is we actually find practical workloads
>> where the size of these SLRUs is a performance issue.
>
>I understood that the new performance statistics are expected to reveal
>what SLRUs need to be tunable and/or implemented with a different
>mechanism like shared buffers.
>

Right. It's certainly meant to provide information for further tuning.
I'm just saying it's targeted more at developers, at least initially.
Maybe we'll end up with GUCs, maybe we'll choose other approaches for
some SLRUs. I don't have an opinion on that yet.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  10. Re: SLRU statistics

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-02-28T23:19:18Z

    On 2020-Jan-21, Tomas Vondra wrote:

> On Tue, Jan 21, 2020 at 05:09:33PM +0900, Masahiko Sawada wrote:

> > I've not tested the performance impact but perhaps we might want to
> > disable these counter by default and controlled by a GUC. And similar
> > to buffer statistics it might be better to inline
> > pgstat_count_slru_page_xxx function for better performance.
> 
> Hmmm, yeah. Inlining seems like a good idea, and maybe we should have
> something like track_slru GUC.

I disagree with adding a GUC.  If a performance impact can be measured
let's turn the functions to static inline, as already proposed.  My
guess is that pgstat_count_slru_page_hit() is the only candidate for
that; all the other paths involve I/O or lock acquisition or even WAL
generation, so the impact won't be measurable anyhow.  We removed
track-enabling GUCs years ago.

BTW, this comment:
			/* update the stats counter of pages found in shared buffers */

is not strictly true, because we don't use what we normally call "shared
buffers"  for SLRUs.

Patch applies cleanly.  I suggest to move the page_miss() call until
after SlruRecentlyUsed(), for consistency with the other case.

I find SlruType pretty odd, and the accompanying "if" list in
pg_stat_get_slru() correspondingly so.  Would it be possible to have
each SLRU enumerate itself somehow?  Maybe add the name in SlruCtlData
and query that, somehow.  (I don't think we have an array of SlruCtlData
anywhere though, so this might be a useless idea.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-02-29T12:24:41Z

    On Fri, Feb 28, 2020 at 08:19:18PM -0300, Alvaro Herrera wrote:
>On 2020-Jan-21, Tomas Vondra wrote:
>
>> On Tue, Jan 21, 2020 at 05:09:33PM +0900, Masahiko Sawada wrote:
>
>> > I've not tested the performance impact but perhaps we might want to
>> > disable these counter by default and controlled by a GUC. And similar
>> > to buffer statistics it might be better to inline
>> > pgstat_count_slru_page_xxx function for better performance.
>>
>> Hmmm, yeah. Inlining seems like a good idea, and maybe we should have
>> something like track_slru GUC.
>
>I disagree with adding a GUC.  If a performance impact can be measured
>let's turn the functions to static inline, as already proposed.  My
>guess is that pgstat_count_slru_page_hit() is the only candidate for
>that; all the other paths involve I/O or lock acquisition or even WAL
>generation, so the impact won't be measurable anyhow.  We removed
>track-enabling GUCs years ago.
>

Did we actually remove track-enabling GUCs? I think we still have

  - track_activities
  - track_counts
  - track_io_timing
  - track_functions

But maybe I'm missing something?

That being said, I'm not sure we need to add a GUC. I'll do some
measurements and we'll see. Maybe the statis inline will me enough.

>BTW, this comment:
>			/* update the stats counter of pages found in shared buffers */
>
>is not strictly true, because we don't use what we normally call "shared
>buffers"  for SLRUs.
>

Oh, right. Will fix.

>Patch applies cleanly.  I suggest to move the page_miss() call until
>after SlruRecentlyUsed(), for consistency with the other case.
>

OK.

>I find SlruType pretty odd, and the accompanying "if" list in
>pg_stat_get_slru() correspondingly so.  Would it be possible to have
>each SLRU enumerate itself somehow?  Maybe add the name in SlruCtlData
>and query that, somehow.  (I don't think we have an array of SlruCtlData
>anywhere though, so this might be a useless idea.)
>

Well, maybe. We could have a system to register SLRUs dynamically, but
the trick here is that by having a fixed predefined number of SLRUs
simplifies serialization in pgstat.c and so on. I don't think the "if"
branches in pg_stat_get_slru() are particularly ugly, but maybe we could
replace the enum with a registry of structs, something like rmgrlist.h.
It seems like an overkill to me, though.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  12. Re: SLRU statistics

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-02-29T14:44:26Z

    On 2020-Feb-29, Tomas Vondra wrote:

> Did we actually remove track-enabling GUCs? I think we still have
> 
>  - track_activities
>  - track_counts
>  - track_io_timing
>  - track_functions
> 
> But maybe I'm missing something?

Hm I remembered we removed the one for row-level stats
(track_row_stats), but what we really did is merge it with block-level
stats (track_block_stats) into track_counts -- commit 48f7e6439568. 
Funnily enough, if you disable that autovacuum won't work, so I'm not
sure it's a very useful tunable.  And it definitely has more overhead
than what this new GUC would have.

> > I find SlruType pretty odd, and the accompanying "if" list in
> > pg_stat_get_slru() correspondingly so.  Would it be possible to have
> > each SLRU enumerate itself somehow?  Maybe add the name in SlruCtlData
> > and query that, somehow.  (I don't think we have an array of SlruCtlData
> > anywhere though, so this might be a useless idea.)
> 
> Well, maybe. We could have a system to register SLRUs dynamically, but
> the trick here is that by having a fixed predefined number of SLRUs
> simplifies serialization in pgstat.c and so on. I don't think the "if"
> branches in pg_stat_get_slru() are particularly ugly, but maybe we could
> replace the enum with a registry of structs, something like rmgrlist.h.
> It seems like an overkill to me, though.

Yeah, maybe we don't have to fix that now.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  13. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-02-29T20:55:18Z

    On Sat, Feb 29, 2020 at 11:44:26AM -0300, Alvaro Herrera wrote:
>On 2020-Feb-29, Tomas Vondra wrote:
>
>> Did we actually remove track-enabling GUCs? I think we still have
>>
>>  - track_activities
>>  - track_counts
>>  - track_io_timing
>>  - track_functions
>>
>> But maybe I'm missing something?
>
>Hm I remembered we removed the one for row-level stats
>(track_row_stats), but what we really did is merge it with block-level
>stats (track_block_stats) into track_counts -- commit 48f7e6439568.
>Funnily enough, if you disable that autovacuum won't work, so I'm not
>sure it's a very useful tunable.  And it definitely has more overhead
>than what this new GUC would have.
>

OK

>> > I find SlruType pretty odd, and the accompanying "if" list in
>> > pg_stat_get_slru() correspondingly so.  Would it be possible to have
>> > each SLRU enumerate itself somehow?  Maybe add the name in SlruCtlData
>> > and query that, somehow.  (I don't think we have an array of SlruCtlData
>> > anywhere though, so this might be a useless idea.)
>>
>> Well, maybe. We could have a system to register SLRUs dynamically, but
>> the trick here is that by having a fixed predefined number of SLRUs
>> simplifies serialization in pgstat.c and so on. I don't think the "if"
>> branches in pg_stat_get_slru() are particularly ugly, but maybe we could
>> replace the enum with a registry of structs, something like rmgrlist.h.
>> It seems like an overkill to me, though.
>
>Yeah, maybe we don't have to fix that now.
>

IMO the current solution is sufficient for the purpose. I guess we could
just stick a name into the SlruCtlData (and remove SlruType entirely),
and use that to identify the stats entries. That might be enough, and in
fact we already have that - SimpleLruInit gets a name parameter and
copies that to the lwlock_tranche_name.

One of the main reasons why I opted to use the enum is that it makes
tracking, lookup and serialization pretty trivial - it's just an index
lookup, etc. But maybe it wouldn't be much more complex with the name, 
considering the name length is limited by SLRU_MAX_NAME_LENGTH. And we
probably don't expect many entries, so we could keep them in a simple
list, or maybe a simplehash.

I'm not sure what to do with data for SLRUs that might have disappeared
after a restart (e.g. because someone removed an extension). Until now
those would be in the all in the "other" entry.


The attached v2 fixes the issues in your first message:

- I moved the page_miss() call after SlruRecentlyUsed(), but then I
   realized it's entirely duplicate with the page_read() update done in
   SlruPhysicalReadPage(). I removed the call from SlruPhysicalReadPage()
   and renamed page_miss to page_read - that's more consistent with
   shared buffers stats, which also have buffers_hit and buffer_read.

- I've also implemented the reset. I ended up adding a new option to
   pg_stat_reset_shared, which always resets all SLRU entries. We track
   the reset timestamp for each SLRU entry, but the value is always the
   same. I admit this is a bit weird - I did it like this because (a) I'm
   not sure how to identify the individual entries and (b) the SLRU is
   shared, so pg_stat_reset_shared seems kinda natural.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  14. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-03-27T00:41:11Z

    Hi,

Attached is v3 of the patch with one big change and various small ones.

The main change is that it gets rid of the SlruType enum and the new
field in SlruCtlData. Instead, the patch now uses the name passed to
SimpleLruInit (which is then stored as LWLock tranche name).

The counters are still stored in a fixed-sized array, and there's a
simple name/index mapping. We don't have any registry of stable SLRU
IDs, so I can't think of anything better, and I think this is good
enough for now.

The other change is that I got rid of the io_error counter. We don't
have that for shared buffers etc. either, anyway.

I've also renamed the colunms from "pages" to "blks" to make it
consistent with other similar stats (blks_hit, blks_read). I've
renamed the fields to "blks_written" and "blks_zeroed".

And finally, I've added the view to monitoring.sgml.

Barring objections, I'll get this committed in the next few days, after
reviewing the comments a bit.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  15. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-03-28T00:26:30Z

    Hi,

here is a bit improved version of the patch - I've been annoyed by how
the resetting works (per-entry timestamp, but resetting all entries) so
I've added a new function pg_stat_reset_slru() that allows resetting
either all entries or just one entry (identified by name). So

     SELECT pg_stat_reset_slru('clog');

resets just "clog" SLRU counters, while

     SELECT pg_stat_reset_slru(NULL);

resets all entries.

I've also done a bit of benchmarking, to see if this has measurable
impact (in which case it might deserve a new GUC), and I think it's not
measurable. I've used a tiny unlogged table (single row).

     CREATE UNLOGGED TABLE t (a int);
     INSERT INTO t VALUES (1);

and then short pgbench runs with a single client, updatint the row. I've
been unable to measure any regression, it's all well within 1% so noise.
But perhaps there's some other benchmark that I should do?


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  16. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-04-02T00:41:36Z

    Hi,

I've pushed this after some minor cleanup and improvements.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  17. RE: SLRU statistics

    Shinoda, Noriyoshi <noriyoshi.shinoda@hpe.com> — 2020-04-02T02:04:10Z

    Hi, 

Thank you for developing great features.
The attached patch is a small fix to the committed documentation for the data type name of blks_hit column.

Best regards,
Noriyoshi Shinoda

-----Original Message-----
From: Tomas Vondra [mailto:tomas.vondra@2ndquadrant.com] 
Sent: Thursday, April 2, 2020 9:42 AM
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; tsunakawa.takay@fujitsu.com; pgsql-hackers@postgresql.org
Subject: Re: SLRU statistics

Hi,

I've pushed this after some minor cleanup and improvements.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com 
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  18. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-04-02T12:29:33Z

    On Thu, Apr 02, 2020 at 02:04:10AM +0000, Shinoda, Noriyoshi (PN Japan A&PS Delivery) wrote:
>Hi,
>
>Thank you for developing great features.
>The attached patch is a small fix to the committed documentation for the data type name of blks_hit column.
>

Thank you for the patch, pushed.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  19. Re: SLRU statistics

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2020-04-07T11:31:37Z

    Hello Tomas,

On Thu, Apr 2, 2020 at 5:59 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> Thank you for the patch, pushed.
>
In SimpleLruReadPage_ReadOnly, we first try to find the SLRU page in
shared buffer under shared lock, then conditionally visit
SimpleLruReadPage if reading is necessary. IMHO, we should update
hit_count if we can find the buffer in SimpleLruReadPage_ReadOnly
directly. Am I missing something?

Attached a patch for the same.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  20. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-04-07T17:29:06Z

    On Tue, Apr 07, 2020 at 05:01:37PM +0530, Kuntal Ghosh wrote:
>Hello Tomas,
>
>On Thu, Apr 2, 2020 at 5:59 PM Tomas Vondra
><tomas.vondra@2ndquadrant.com> wrote:
>> Thank you for the patch, pushed.
>>
>In SimpleLruReadPage_ReadOnly, we first try to find the SLRU page in
>shared buffer under shared lock, then conditionally visit
>SimpleLruReadPage if reading is necessary. IMHO, we should update
>hit_count if we can find the buffer in SimpleLruReadPage_ReadOnly
>directly. Am I missing something?
>
>Attached a patch for the same.
>

Yes, I think that's correct - without this we fail to account for
(possibly) a quite significant number of hits. Thanks for the report,
I'll get this pushed later today.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  21. Re: SLRU statistics

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-04-30T18:02:59Z

    
On 2020/04/02 9:41, Tomas Vondra wrote:
> Hi,
> 
> I've pushed this after some minor cleanup and improvements.

+static char *slru_names[] = {"async", "clog", "commit_timestamp",
+							  "multixact_offset", "multixact_member",
+							  "oldserxid", "pg_xact", "subtrans",
+							  "other" /* has to be last */};

When I tried pg_stat_slru, I found that it returns a row for "pg_xact".
But since there is no "pg_xact" slru ("clog" slru exists instead),
"pg_xact" should be removed? Patch attached.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  22. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-04-30T18:19:01Z

    On Fri, May 01, 2020 at 03:02:59AM +0900, Fujii Masao wrote:
>
>
>On 2020/04/02 9:41, Tomas Vondra wrote:
>>Hi,
>>
>>I've pushed this after some minor cleanup and improvements.
>
>+static char *slru_names[] = {"async", "clog", "commit_timestamp",
>+							  "multixact_offset", "multixact_member",
>+							  "oldserxid", "pg_xact", "subtrans",
>+							  "other" /* has to be last */};
>
>When I tried pg_stat_slru, I found that it returns a row for "pg_xact".
>But since there is no "pg_xact" slru ("clog" slru exists instead),
>"pg_xact" should be removed? Patch attached.
>

Yeah, I think I got confused and accidentally added both "clog" and
"pg_xact". I'll get "pg_xact" removed.

>Regards,
>
>-- 
>Fujii Masao
>Advanced Computing Technology Center
>Research and Development Headquarters
>NTT DATA CORPORATION

>diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
>index 6562cc400b..ba6d8d2123 100644
>--- a/doc/src/sgml/monitoring.sgml
>+++ b/doc/src/sgml/monitoring.sgml
>@@ -3483,7 +3483,7 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
>        predefined list (<literal>async</literal>, <literal>clog</literal>,
>        <literal>commit_timestamp</literal>, <literal>multixact_offset</literal>,
>        <literal>multixact_member</literal>, <literal>oldserxid</literal>,
>-       <literal>pg_xact</literal>, <literal>subtrans</literal> and
>+       <literal>subtrans</literal> and
>        <literal>other</literal>) resets counters for only that entry.
>        Names not included in this list are treated as <literal>other</literal>.
>       </entry>
>diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
>index 50eea2e8a8..2ba3858d31 100644
>--- a/src/backend/postmaster/pgstat.c
>+++ b/src/backend/postmaster/pgstat.c
>@@ -152,7 +152,7 @@ PgStat_MsgBgWriter BgWriterStats;
>  */
> static char *slru_names[] = {"async", "clog", "commit_timestamp",
> 							  "multixact_offset", "multixact_member",
>-							  "oldserxid", "pg_xact", "subtrans",
>+							  "oldserxid", "subtrans",
> 							  "other" /* has to be last */};
>
> /* number of elemenents of slru_name array */


-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  23. Re: SLRU statistics

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-05-01T02:49:51Z

    
On 2020/05/01 3:19, Tomas Vondra wrote:
> On Fri, May 01, 2020 at 03:02:59AM +0900, Fujii Masao wrote:
>>
>>
>> On 2020/04/02 9:41, Tomas Vondra wrote:
>>> Hi,
>>>
>>> I've pushed this after some minor cleanup and improvements.
>>
>> +static char *slru_names[] = {"async", "clog", "commit_timestamp",
>> +                              "multixact_offset", "multixact_member",
>> +                              "oldserxid", "pg_xact", "subtrans",
>> +                              "other" /* has to be last */};
>>
>> When I tried pg_stat_slru, I found that it returns a row for "pg_xact".
>> But since there is no "pg_xact" slru ("clog" slru exists instead),
>> "pg_xact" should be removed? Patch attached.
>>
> 
> Yeah, I think I got confused and accidentally added both "clog" and
> "pg_xact". I'll get "pg_xact" removed.

Thanks!

Another thing I found is; pgstat_send_slru() should be called also by
other processes than backend? For example, since clog data is flushed
basically by checkpointer, checkpointer seems to need to send slru stats.
Otherwise, pg_stat_slru.flushes would not be updated.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  24. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-05-02T00:08:18Z

    On Fri, May 01, 2020 at 11:49:51AM +0900, Fujii Masao wrote:
>
>
>On 2020/05/01 3:19, Tomas Vondra wrote:
>>On Fri, May 01, 2020 at 03:02:59AM +0900, Fujii Masao wrote:
>>>
>>>
>>>On 2020/04/02 9:41, Tomas Vondra wrote:
>>>>Hi,
>>>>
>>>>I've pushed this after some minor cleanup and improvements.
>>>
>>>+static char *slru_names[] = {"async", "clog", "commit_timestamp",
>>>+                              "multixact_offset", "multixact_member",
>>>+                              "oldserxid", "pg_xact", "subtrans",
>>>+                              "other" /* has to be last */};
>>>
>>>When I tried pg_stat_slru, I found that it returns a row for "pg_xact".
>>>But since there is no "pg_xact" slru ("clog" slru exists instead),
>>>"pg_xact" should be removed? Patch attached.
>>>
>>
>>Yeah, I think I got confused and accidentally added both "clog" and
>>"pg_xact". I'll get "pg_xact" removed.
>
>Thanks!
>

OK, pushed. Thanks!

>Another thing I found is; pgstat_send_slru() should be called also by
>other processes than backend? For example, since clog data is flushed
>basically by checkpointer, checkpointer seems to need to send slru stats.
>Otherwise, pg_stat_slru.flushes would not be updated.
>

Hmmm, that's a good point. If I understand the issue correctly, the
checkpointer accumulates the stats but never really sends them because
it never calls pgstat_report_stat/pgstat_send_slru. That's only called
from PostgresMain, but not from CheckpointerMain.

I think we could simply add pgstat_send_slru() right after the existing
call in CheckpointerMain, right?

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  25. Re: SLRU statistics

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-05-02T06:56:07Z

    
On 2020/05/02 9:08, Tomas Vondra wrote:
> On Fri, May 01, 2020 at 11:49:51AM +0900, Fujii Masao wrote:
>>
>>
>> On 2020/05/01 3:19, Tomas Vondra wrote:
>>> On Fri, May 01, 2020 at 03:02:59AM +0900, Fujii Masao wrote:
>>>>
>>>>
>>>> On 2020/04/02 9:41, Tomas Vondra wrote:
>>>>> Hi,
>>>>>
>>>>> I've pushed this after some minor cleanup and improvements.
>>>>
>>>> +static char *slru_names[] = {"async", "clog", "commit_timestamp",
>>>> +                              "multixact_offset", "multixact_member",
>>>> +                              "oldserxid", "pg_xact", "subtrans",
>>>> +                              "other" /* has to be last */};
>>>>
>>>> When I tried pg_stat_slru, I found that it returns a row for "pg_xact".
>>>> But since there is no "pg_xact" slru ("clog" slru exists instead),
>>>> "pg_xact" should be removed? Patch attached.
>>>>
>>>
>>> Yeah, I think I got confused and accidentally added both "clog" and
>>> "pg_xact". I'll get "pg_xact" removed.
>>
>> Thanks!
>>
> 
> OK, pushed. Thanks!

Thanks a lot!

But, like the patch that I attached in the previous email does,
"pg_xact" should be removed from the description of pg_stat_reset_slru()
in monitoring.sgml.

>> Another thing I found is; pgstat_send_slru() should be called also by
>> other processes than backend? For example, since clog data is flushed
>> basically by checkpointer, checkpointer seems to need to send slru stats.
>> Otherwise, pg_stat_slru.flushes would not be updated.
>>
> 
> Hmmm, that's a good point. If I understand the issue correctly, the
> checkpointer accumulates the stats but never really sends them because
> it never calls pgstat_report_stat/pgstat_send_slru. That's only called
> from PostgresMain, but not from CheckpointerMain.

Yes.

> I think we could simply add pgstat_send_slru() right after the existing
> call in CheckpointerMain, right?

Checkpointer sends off activity statistics to the stats collector in
two places, by calling pgstat_send_bgwriter(). What about calling
pgstat_send_slru() just after pgstat_send_bgwriter()?

In previous email, I mentioned checkpointer just as an example.
So probably we need to investigate what process should send slru stats,
other than checkpointer. I guess that at least autovacuum worker,
logical replication walsender and parallel query worker (maybe this has
been already covered by parallel query some mechanisms. Sorry I've
not checked that) would need to send its slru stats.

Atsushi-san reported another issue in pg_stat_slru.
You're planning to work on that?
https://postgr.es/m/CACZ0uYFe16pjZxQYaTn53mspyM7dgMPYL3DJLjjPw69GMCC2Ow@mail.gmail.com

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  26. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-05-02T10:55:00Z

    On Sat, May 02, 2020 at 03:56:07PM +0900, Fujii Masao wrote:
>
>
>On 2020/05/02 9:08, Tomas Vondra wrote:
>>On Fri, May 01, 2020 at 11:49:51AM +0900, Fujii Masao wrote:
>>>
>>>
>>>On 2020/05/01 3:19, Tomas Vondra wrote:
>>>>On Fri, May 01, 2020 at 03:02:59AM +0900, Fujii Masao wrote:
>>>>>
>>>>>
>>>>>On 2020/04/02 9:41, Tomas Vondra wrote:
>>>>>>Hi,
>>>>>>
>>>>>>I've pushed this after some minor cleanup and improvements.
>>>>>
>>>>>+static char *slru_names[] = {"async", "clog", "commit_timestamp",
>>>>>+                              "multixact_offset", "multixact_member",
>>>>>+                              "oldserxid", "pg_xact", "subtrans",
>>>>>+                              "other" /* has to be last */};
>>>>>
>>>>>When I tried pg_stat_slru, I found that it returns a row for "pg_xact".
>>>>>But since there is no "pg_xact" slru ("clog" slru exists instead),
>>>>>"pg_xact" should be removed? Patch attached.
>>>>>
>>>>
>>>>Yeah, I think I got confused and accidentally added both "clog" and
>>>>"pg_xact". I'll get "pg_xact" removed.
>>>
>>>Thanks!
>>>
>>
>>OK, pushed. Thanks!
>
>Thanks a lot!
>
>But, like the patch that I attached in the previous email does,
>"pg_xact" should be removed from the description of pg_stat_reset_slru()
>in monitoring.sgml.
>

Whooops. My bad, will fix.

>>>Another thing I found is; pgstat_send_slru() should be called also by
>>>other processes than backend? For example, since clog data is flushed
>>>basically by checkpointer, checkpointer seems to need to send slru stats.
>>>Otherwise, pg_stat_slru.flushes would not be updated.
>>>
>>
>>Hmmm, that's a good point. If I understand the issue correctly, the
>>checkpointer accumulates the stats but never really sends them because
>>it never calls pgstat_report_stat/pgstat_send_slru. That's only called
>>from PostgresMain, but not from CheckpointerMain.
>
>Yes.
>
>>I think we could simply add pgstat_send_slru() right after the existing
>>call in CheckpointerMain, right?
>
>Checkpointer sends off activity statistics to the stats collector in
>two places, by calling pgstat_send_bgwriter(). What about calling
>pgstat_send_slru() just after pgstat_send_bgwriter()?
>

Yep, that's what I proposed.

>In previous email, I mentioned checkpointer just as an example.
>So probably we need to investigate what process should send slru stats,
>other than checkpointer. I guess that at least autovacuum worker,
>logical replication walsender and parallel query worker (maybe this has
>been already covered by parallel query some mechanisms. Sorry I've
>not checked that) would need to send its slru stats.
>

Probably. Do you have any other process type in mind?

>Atsushi-san reported another issue in pg_stat_slru.
>You're planning to work on that?
>https://postgr.es/m/CACZ0uYFe16pjZxQYaTn53mspyM7dgMPYL3DJLjjPw69GMCC2Ow@mail.gmail.com
>

Yes, I'll investigate.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  27. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-05-02T16:59:05Z

    On Sat, May 02, 2020 at 12:55:00PM +0200, Tomas Vondra wrote:
>On Sat, May 02, 2020 at 03:56:07PM +0900, Fujii Masao wrote:
>>
>> ...
>
>>>>Another thing I found is; pgstat_send_slru() should be called also by
>>>>other processes than backend? For example, since clog data is flushed
>>>>basically by checkpointer, checkpointer seems to need to send slru stats.
>>>>Otherwise, pg_stat_slru.flushes would not be updated.
>>>>
>>>
>>>Hmmm, that's a good point. If I understand the issue correctly, the
>>>checkpointer accumulates the stats but never really sends them because
>>>it never calls pgstat_report_stat/pgstat_send_slru. That's only called
>>>from PostgresMain, but not from CheckpointerMain.
>>
>>Yes.
>>
>>>I think we could simply add pgstat_send_slru() right after the existing
>>>call in CheckpointerMain, right?
>>
>>Checkpointer sends off activity statistics to the stats collector in
>>two places, by calling pgstat_send_bgwriter(). What about calling
>>pgstat_send_slru() just after pgstat_send_bgwriter()?
>>
>
>Yep, that's what I proposed.
>
>>In previous email, I mentioned checkpointer just as an example.
>>So probably we need to investigate what process should send slru stats,
>>other than checkpointer. I guess that at least autovacuum worker,
>>logical replication walsender and parallel query worker (maybe this has
>>been already covered by parallel query some mechanisms. Sorry I've
>>not checked that) would need to send its slru stats.
>>
>
>Probably. Do you have any other process type in mind?
>

I've looked at places calling pgstat_send_* functions, and I found
thsese places:

src/backend/postmaster/bgwriter.c

- AFAIK it merely writes out dirty shared buffers, so likely irrelevant.

src/backend/postmaster/checkpointer.c

- This is what we're already discussing here.

src/backend/postmaster/pgarch.c

- Seems irrelevant.


I'm a bit puzzled why we're not sending any stats from walsender, which
I suppose could do various stuff during logical decoding.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  28. Re: SLRU statistics

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-05-07T04:47:40Z

    
On 2020/05/03 1:59, Tomas Vondra wrote:
> On Sat, May 02, 2020 at 12:55:00PM +0200, Tomas Vondra wrote:
>> On Sat, May 02, 2020 at 03:56:07PM +0900, Fujii Masao wrote:
>>>
>>> ...
>>
>>>>> Another thing I found is; pgstat_send_slru() should be called also by
>>>>> other processes than backend? For example, since clog data is flushed
>>>>> basically by checkpointer, checkpointer seems to need to send slru stats.
>>>>> Otherwise, pg_stat_slru.flushes would not be updated.
>>>>>
>>>>
>>>> Hmmm, that's a good point. If I understand the issue correctly, the
>>>> checkpointer accumulates the stats but never really sends them because
>>>> it never calls pgstat_report_stat/pgstat_send_slru. That's only called
>>>> from PostgresMain, but not from CheckpointerMain.
>>>
>>> Yes.
>>>
>>>> I think we could simply add pgstat_send_slru() right after the existing
>>>> call in CheckpointerMain, right?
>>>
>>> Checkpointer sends off activity statistics to the stats collector in
>>> two places, by calling pgstat_send_bgwriter(). What about calling
>>> pgstat_send_slru() just after pgstat_send_bgwriter()?
>>>
>>
>> Yep, that's what I proposed.
>>
>>> In previous email, I mentioned checkpointer just as an example.
>>> So probably we need to investigate what process should send slru stats,
>>> other than checkpointer. I guess that at least autovacuum worker,
>>> logical replication walsender and parallel query worker (maybe this has
>>> been already covered by parallel query some mechanisms. Sorry I've
>>> not checked that) would need to send its slru stats.
>>>
>>
>> Probably. Do you have any other process type in mind?

No. For now what I'm in mind are just checkpointer, autovacuum worker,
logical replication walsender and parallel query worker. Seems logical
replication worker and syncer have sent slru stats via pgstat_report_stat().

> I've looked at places calling pgstat_send_* functions, and I found
> thsese places:
> 
> src/backend/postmaster/bgwriter.c
> 
> - AFAIK it merely writes out dirty shared buffers, so likely irrelevant.
> 
> src/backend/postmaster/checkpointer.c
> 
> - This is what we're already discussing here.
> 
> src/backend/postmaster/pgarch.c
> 
> - Seems irrelevant.
> 
> 
> I'm a bit puzzled why we're not sending any stats from walsender, which
> I suppose could do various stuff during logical decoding.

Not sure why, but that seems an oversight...


Also I found another minor issue; SLRUStats has not been initialized to 0
and which could update the counters unexpectedly. Attached patch fixes
this issue.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  29. Re: SLRU statistics

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-05-13T07:10:30Z

    
On 2020/05/07 13:47, Fujii Masao wrote:
> 
> 
> On 2020/05/03 1:59, Tomas Vondra wrote:
>> On Sat, May 02, 2020 at 12:55:00PM +0200, Tomas Vondra wrote:
>>> On Sat, May 02, 2020 at 03:56:07PM +0900, Fujii Masao wrote:
>>>>
>>>> ...
>>>
>>>>>> Another thing I found is; pgstat_send_slru() should be called also by
>>>>>> other processes than backend? For example, since clog data is flushed
>>>>>> basically by checkpointer, checkpointer seems to need to send slru stats.
>>>>>> Otherwise, pg_stat_slru.flushes would not be updated.
>>>>>>
>>>>>
>>>>> Hmmm, that's a good point. If I understand the issue correctly, the
>>>>> checkpointer accumulates the stats but never really sends them because
>>>>> it never calls pgstat_report_stat/pgstat_send_slru. That's only called
>>>>> from PostgresMain, but not from CheckpointerMain.
>>>>
>>>> Yes.
>>>>
>>>>> I think we could simply add pgstat_send_slru() right after the existing
>>>>> call in CheckpointerMain, right?
>>>>
>>>> Checkpointer sends off activity statistics to the stats collector in
>>>> two places, by calling pgstat_send_bgwriter(). What about calling
>>>> pgstat_send_slru() just after pgstat_send_bgwriter()?
>>>>
>>>
>>> Yep, that's what I proposed.
>>>
>>>> In previous email, I mentioned checkpointer just as an example.
>>>> So probably we need to investigate what process should send slru stats,
>>>> other than checkpointer. I guess that at least autovacuum worker,
>>>> logical replication walsender and parallel query worker (maybe this has
>>>> been already covered by parallel query some mechanisms. Sorry I've
>>>> not checked that) would need to send its slru stats.
>>>>
>>>
>>> Probably. Do you have any other process type in mind?
> 
> No. For now what I'm in mind are just checkpointer, autovacuum worker,
> logical replication walsender and parallel query worker. Seems logical
> replication worker and syncer have sent slru stats via pgstat_report_stat().
> 
>> I've looked at places calling pgstat_send_* functions, and I found
>> thsese places:
>>
>> src/backend/postmaster/bgwriter.c
>>
>> - AFAIK it merely writes out dirty shared buffers, so likely irrelevant.
>>
>> src/backend/postmaster/checkpointer.c
>>
>> - This is what we're already discussing here.
>>
>> src/backend/postmaster/pgarch.c
>>
>> - Seems irrelevant.
>>
>>
>> I'm a bit puzzled why we're not sending any stats from walsender, which
>> I suppose could do various stuff during logical decoding.
> 
> Not sure why, but that seems an oversight...
> 
> 
> Also I found another minor issue; SLRUStats has not been initialized to 0
> and which could update the counters unexpectedly. Attached patch fixes
> this issue.

This is minor issue, but basically it's better to fix that before
v13 beta1 release. So barring any objection, I will commit the patch.

+		values[8] = Int64GetDatum(stat.stat_reset_timestamp);

Also I found another small issue: pg_stat_get_slru() returns the timestamp
when pg_stat_slru was reset by using Int64GetDatum(). This works maybe
because the timestamp is also int64. But TimestampTzGetDatum() should
be used here, instead. Patch attached. Thought?

Regards,


-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  30. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-05-13T08:21:59Z

    On Wed, May 13, 2020 at 04:10:30PM +0900, Fujii Masao wrote:
>
>
>On 2020/05/07 13:47, Fujii Masao wrote:
>>
>>
>>On 2020/05/03 1:59, Tomas Vondra wrote:
>>>On Sat, May 02, 2020 at 12:55:00PM +0200, Tomas Vondra wrote:
>>>>On Sat, May 02, 2020 at 03:56:07PM +0900, Fujii Masao wrote:
>>>>>
>>>>>...
>>>>
>>>>>>>Another thing I found is; pgstat_send_slru() should be called also by
>>>>>>>other processes than backend? For example, since clog data is flushed
>>>>>>>basically by checkpointer, checkpointer seems to need to send slru stats.
>>>>>>>Otherwise, pg_stat_slru.flushes would not be updated.
>>>>>>>
>>>>>>
>>>>>>Hmmm, that's a good point. If I understand the issue correctly, the
>>>>>>checkpointer accumulates the stats but never really sends them because
>>>>>>it never calls pgstat_report_stat/pgstat_send_slru. That's only called
>>>>>>from PostgresMain, but not from CheckpointerMain.
>>>>>
>>>>>Yes.
>>>>>
>>>>>>I think we could simply add pgstat_send_slru() right after the existing
>>>>>>call in CheckpointerMain, right?
>>>>>
>>>>>Checkpointer sends off activity statistics to the stats collector in
>>>>>two places, by calling pgstat_send_bgwriter(). What about calling
>>>>>pgstat_send_slru() just after pgstat_send_bgwriter()?
>>>>>
>>>>
>>>>Yep, that's what I proposed.
>>>>
>>>>>In previous email, I mentioned checkpointer just as an example.
>>>>>So probably we need to investigate what process should send slru stats,
>>>>>other than checkpointer. I guess that at least autovacuum worker,
>>>>>logical replication walsender and parallel query worker (maybe this has
>>>>>been already covered by parallel query some mechanisms. Sorry I've
>>>>>not checked that) would need to send its slru stats.
>>>>>
>>>>
>>>>Probably. Do you have any other process type in mind?
>>
>>No. For now what I'm in mind are just checkpointer, autovacuum worker,
>>logical replication walsender and parallel query worker. Seems logical
>>replication worker and syncer have sent slru stats via pgstat_report_stat().
>>
>>>I've looked at places calling pgstat_send_* functions, and I found
>>>thsese places:
>>>
>>>src/backend/postmaster/bgwriter.c
>>>
>>>- AFAIK it merely writes out dirty shared buffers, so likely irrelevant.
>>>
>>>src/backend/postmaster/checkpointer.c
>>>
>>>- This is what we're already discussing here.
>>>
>>>src/backend/postmaster/pgarch.c
>>>
>>>- Seems irrelevant.
>>>
>>>
>>>I'm a bit puzzled why we're not sending any stats from walsender, which
>>>I suppose could do various stuff during logical decoding.
>>
>>Not sure why, but that seems an oversight...
>>
>>
>>Also I found another minor issue; SLRUStats has not been initialized to 0
>>and which could update the counters unexpectedly. Attached patch fixes
>>this issue.
>
>This is minor issue, but basically it's better to fix that before
>v13 beta1 release. So barring any objection, I will commit the patch.
>
>+		values[8] = Int64GetDatum(stat.stat_reset_timestamp);
>
>Also I found another small issue: pg_stat_get_slru() returns the timestamp
>when pg_stat_slru was reset by using Int64GetDatum(). This works maybe
>because the timestamp is also int64. But TimestampTzGetDatum() should
>be used here, instead. Patch attached. Thought?
>

I agree with both fixes.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  31. Re: SLRU statistics

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-05-13T13:22:12Z

    
On 2020/05/13 17:21, Tomas Vondra wrote:
> On Wed, May 13, 2020 at 04:10:30PM +0900, Fujii Masao wrote:
>>
>>
>> On 2020/05/07 13:47, Fujii Masao wrote:
>>>
>>>
>>> On 2020/05/03 1:59, Tomas Vondra wrote:
>>>> On Sat, May 02, 2020 at 12:55:00PM +0200, Tomas Vondra wrote:
>>>>> On Sat, May 02, 2020 at 03:56:07PM +0900, Fujii Masao wrote:
>>>>>>
>>>>>> ...
>>>>>
>>>>>>>> Another thing I found is; pgstat_send_slru() should be called also by
>>>>>>>> other processes than backend? For example, since clog data is flushed
>>>>>>>> basically by checkpointer, checkpointer seems to need to send slru stats.
>>>>>>>> Otherwise, pg_stat_slru.flushes would not be updated.
>>>>>>>>
>>>>>>>
>>>>>>> Hmmm, that's a good point. If I understand the issue correctly, the
>>>>>>> checkpointer accumulates the stats but never really sends them because
>>>>>>> it never calls pgstat_report_stat/pgstat_send_slru. That's only called
>>>>>>> from PostgresMain, but not from CheckpointerMain.
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>> I think we could simply add pgstat_send_slru() right after the existing
>>>>>>> call in CheckpointerMain, right?
>>>>>>
>>>>>> Checkpointer sends off activity statistics to the stats collector in
>>>>>> two places, by calling pgstat_send_bgwriter(). What about calling
>>>>>> pgstat_send_slru() just after pgstat_send_bgwriter()?
>>>>>>
>>>>>
>>>>> Yep, that's what I proposed.
>>>>>
>>>>>> In previous email, I mentioned checkpointer just as an example.
>>>>>> So probably we need to investigate what process should send slru stats,
>>>>>> other than checkpointer. I guess that at least autovacuum worker,
>>>>>> logical replication walsender and parallel query worker (maybe this has
>>>>>> been already covered by parallel query some mechanisms. Sorry I've
>>>>>> not checked that) would need to send its slru stats.
>>>>>>
>>>>>
>>>>> Probably. Do you have any other process type in mind?
>>>
>>> No. For now what I'm in mind are just checkpointer, autovacuum worker,
>>> logical replication walsender and parallel query worker. Seems logical
>>> replication worker and syncer have sent slru stats via pgstat_report_stat().
>>>
>>>> I've looked at places calling pgstat_send_* functions, and I found
>>>> thsese places:
>>>>
>>>> src/backend/postmaster/bgwriter.c
>>>>
>>>> - AFAIK it merely writes out dirty shared buffers, so likely irrelevant.
>>>>
>>>> src/backend/postmaster/checkpointer.c
>>>>
>>>> - This is what we're already discussing here.
>>>>
>>>> src/backend/postmaster/pgarch.c
>>>>
>>>> - Seems irrelevant.
>>>>
>>>>
>>>> I'm a bit puzzled why we're not sending any stats from walsender, which
>>>> I suppose could do various stuff during logical decoding.
>>>
>>> Not sure why, but that seems an oversight...
>>>
>>>
>>> Also I found another minor issue; SLRUStats has not been initialized to 0
>>> and which could update the counters unexpectedly. Attached patch fixes
>>> this issue.
>>
>> This is minor issue, but basically it's better to fix that before
>> v13 beta1 release. So barring any objection, I will commit the patch.
>>
>> +        values[8] = Int64GetDatum(stat.stat_reset_timestamp);
>>
>> Also I found another small issue: pg_stat_get_slru() returns the timestamp
>> when pg_stat_slru was reset by using Int64GetDatum(). This works maybe
>> because the timestamp is also int64. But TimestampTzGetDatum() should
>> be used here, instead. Patch attached. Thought?
>>
> 
> I agree with both fixes.

Pushed both. Thanks!

Regards,


-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  32. Re: SLRU statistics

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-13T14:26:39Z

    Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> On 2020/05/13 17:21, Tomas Vondra wrote:
>> On Wed, May 13, 2020 at 04:10:30PM +0900, Fujii Masao wrote:
>>> Also I found another minor issue; SLRUStats has not been initialized to 0
>>> and which could update the counters unexpectedly. Attached patch fixes
>>> this issue.

> Pushed both. Thanks!

Why is that necessary?  A static variable is defined by C to start off
as zeroes.

			regards, tom lane

  33. Re: SLRU statistics

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-05-13T14:46:30Z

    
On 2020/05/13 23:26, Tom Lane wrote:
> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
>> On 2020/05/13 17:21, Tomas Vondra wrote:
>>> On Wed, May 13, 2020 at 04:10:30PM +0900, Fujii Masao wrote:
>>>> Also I found another minor issue; SLRUStats has not been initialized to 0
>>>> and which could update the counters unexpectedly. Attached patch fixes
>>>> this issue.
> 
>> Pushed both. Thanks!
> 
> Why is that necessary?  A static variable is defined by C to start off
> as zeroes.

Because SLRUStats is not a static variable. No?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  34. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-05-13T14:57:39Z

    On Wed, May 13, 2020 at 10:26:39AM -0400, Tom Lane wrote:
>Fujii Masao <masao.fujii@oss.nttdata.com> writes:
>> On 2020/05/13 17:21, Tomas Vondra wrote:
>>> On Wed, May 13, 2020 at 04:10:30PM +0900, Fujii Masao wrote:
>>>> Also I found another minor issue; SLRUStats has not been initialized to 0
>>>> and which could update the counters unexpectedly. Attached patch fixes
>>>> this issue.
>
>> Pushed both. Thanks!
>
>Why is that necessary?  A static variable is defined by C to start off
>as zeroes.
>

But is it a static variable? It's not declared as 'static' but maybe we
can assume it inits to zeroes anyway? I see we do that for
BgWriterStats.



regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  35. Re: SLRU statistics

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-13T15:01:47Z

    Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On Wed, May 13, 2020 at 10:26:39AM -0400, Tom Lane wrote:
>> Why is that necessary?  A static variable is defined by C to start off
>> as zeroes.

> But is it a static variable? It's not declared as 'static' but maybe we
> can assume it inits to zeroes anyway? I see we do that for
> BgWriterStats.

Sorry, by "static" I meant "statically allocated", not "private to
this module".  I'm sure the C standard has some more precise terminology
for this distinction, but I forget what it is.

			regards, tom lane

  36. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-05-13T15:23:39Z

    On Wed, May 13, 2020 at 11:01:47AM -0400, Tom Lane wrote:
>Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> On Wed, May 13, 2020 at 10:26:39AM -0400, Tom Lane wrote:
>>> Why is that necessary?  A static variable is defined by C to start off
>>> as zeroes.
>
>> But is it a static variable? It's not declared as 'static' but maybe we
>> can assume it inits to zeroes anyway? I see we do that for
>> BgWriterStats.
>
>Sorry, by "static" I meant "statically allocated", not "private to
>this module".  I'm sure the C standard has some more precise terminology
>for this distinction, but I forget what it is.
>

Ah, I see. I'm no expert in reading C standard (or any other standard),
but a quick google search yielded this section of C99 standard:

-------------------------------------------------------------------------
If an object that has static storage duration is not initialized
explicitly, then:

- if it has pointer type, it is initialized to a null pointer;

- if it has arithmetic type, it is initialized to (positive or unsigned)
   zero;

- if it is an aggregate, every member is initialized (recursively)
   according to these rules;

- if it is au nion, the first named member is initialized (recursively)
   according to these rules
-------------------------------------------------------------------------

I assume the SLRU variable counts as aggregate, with members having
arithmetic types. In which case it really should be initialized to 0.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  37. Re: SLRU statistics

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-05-13T15:28:46Z

    On Wed, May 13, 2020 at 11:46:30PM +0900, Fujii Masao wrote:
>
>
>On 2020/05/13 23:26, Tom Lane wrote:
>>Fujii Masao <masao.fujii@oss.nttdata.com> writes:
>>>On 2020/05/13 17:21, Tomas Vondra wrote:
>>>>On Wed, May 13, 2020 at 04:10:30PM +0900, Fujii Masao wrote:
>>>>>Also I found another minor issue; SLRUStats has not been initialized to 0
>>>>>and which could update the counters unexpectedly. Attached patch fixes
>>>>>this issue.
>>
>>>Pushed both. Thanks!
>>
>>Why is that necessary?  A static variable is defined by C to start off
>>as zeroes.
>
>Because SLRUStats is not a static variable. No?
>

I think it counts as a variable with "static storage duration" per 6.7.8
(para 10), see [1]. I wasn't aware of this either, but it probably means
the memset is unnecessary.

Also, it seems a bit strange/confusing to handle this differently from
BgWriterStats. And that worked fine without the init for years ...


[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  38. Re: SLRU statistics

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-13T15:38:15Z

    Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> I think it counts as a variable with "static storage duration" per 6.7.8
> (para 10), see [1]. I wasn't aware of this either, but it probably means
> the memset is unnecessary.
> Also, it seems a bit strange/confusing to handle this differently from
> BgWriterStats. And that worked fine without the init for years ...

Yeah, exactly.

There might be merit in memsetting it if we thought that it could have
become nonzero in the postmaster during a previous shmem cycle-of-life.
But the postmaster really shouldn't be accumulating such counts; and
if it is, then we have a bigger problem, because child processes would
be inheriting those counts via fork.

I think this change is unnecessary and should be reverted to avoid
future confusion about whether somehow it is necessary.

			regards, tom lane

  39. Re: SLRU statistics

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-05-13T16:06:12Z

    
On 2020/05/14 0:38, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> I think it counts as a variable with "static storage duration" per 6.7.8
>> (para 10), see [1]. I wasn't aware of this either, but it probably means
>> the memset is unnecessary.
>> Also, it seems a bit strange/confusing to handle this differently from
>> BgWriterStats. And that worked fine without the init for years ...
> 
> Yeah, exactly.
> 
> There might be merit in memsetting it if we thought that it could have
> become nonzero in the postmaster during a previous shmem cycle-of-life.
> But the postmaster really shouldn't be accumulating such counts; and
> if it is, then we have a bigger problem, because child processes would
> be inheriting those counts via fork.

In my previous test, I thought I observed that the counters are already
updated at the beginning of some processes. So I thought that
the counters need to be initialized. Sorry, that's my fault...

So I tried the similar test again and found that postmaster seems to be
able to increment the counters unless I'm missing something.
For example,

     frame #2: 0x000000010d93845f postgres`pgstat_count_slru_page_zeroed(ctl=0x000000010de27320) at pgstat.c:6739:2
     frame #3: 0x000000010d5922ba postgres`SimpleLruZeroPage(ctl=0x000000010de27320, pageno=0) at slru.c:290:2
     frame #4: 0x000000010d6b9ae2 postgres`AsyncShmemInit at async.c:568:12
     frame #5: 0x000000010d9da9a6 postgres`CreateSharedMemoryAndSemaphores at ipci.c:265:2
     frame #6: 0x000000010d93f679 postgres`reset_shared at postmaster.c:2664:2
     frame #7: 0x000000010d93d253 postgres`PostmasterMain(argc=3, argv=0x00007fad56402e00) at postmaster.c:1008:2

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  40. Re: SLRU statistics

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-05-13T16:14:06Z

    On 2020-May-14, Fujii Masao wrote:

> So I tried the similar test again and found that postmaster seems to be
> able to increment the counters unless I'm missing something.
> For example,
> 
>     frame #2: 0x000000010d93845f postgres`pgstat_count_slru_page_zeroed(ctl=0x000000010de27320) at pgstat.c:6739:2
>     frame #3: 0x000000010d5922ba postgres`SimpleLruZeroPage(ctl=0x000000010de27320, pageno=0) at slru.c:290:2
>     frame #4: 0x000000010d6b9ae2 postgres`AsyncShmemInit at async.c:568:12
>     frame #5: 0x000000010d9da9a6 postgres`CreateSharedMemoryAndSemaphores at ipci.c:265:2
>     frame #6: 0x000000010d93f679 postgres`reset_shared at postmaster.c:2664:2
>     frame #7: 0x000000010d93d253 postgres`PostmasterMain(argc=3, argv=0x00007fad56402e00) at postmaster.c:1008:2

Umm.  I have the feeling that we'd rather avoid these updates in
postmaster, per our general rule that postmaster should not touch shared
memory.  However, it might be that it's okay in this case, as it only
happens just as shmem is being "created", so other processes have not
yet had any time to mess things up.  (IIRC only the Async module is
doing that.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  41. Re: SLRU statistics

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-05-13T16:41:57Z

    
On 2020/05/14 1:14, Alvaro Herrera wrote:
> On 2020-May-14, Fujii Masao wrote:
> 
>> So I tried the similar test again and found that postmaster seems to be
>> able to increment the counters unless I'm missing something.
>> For example,
>>
>>      frame #2: 0x000000010d93845f postgres`pgstat_count_slru_page_zeroed(ctl=0x000000010de27320) at pgstat.c:6739:2
>>      frame #3: 0x000000010d5922ba postgres`SimpleLruZeroPage(ctl=0x000000010de27320, pageno=0) at slru.c:290:2
>>      frame #4: 0x000000010d6b9ae2 postgres`AsyncShmemInit at async.c:568:12
>>      frame #5: 0x000000010d9da9a6 postgres`CreateSharedMemoryAndSemaphores at ipci.c:265:2
>>      frame #6: 0x000000010d93f679 postgres`reset_shared at postmaster.c:2664:2
>>      frame #7: 0x000000010d93d253 postgres`PostmasterMain(argc=3, argv=0x00007fad56402e00) at postmaster.c:1008:2
> 
> Umm.  I have the feeling that we'd rather avoid these updates in
> postmaster, per our general rule that postmaster should not touch shared
> memory.  However, it might be that it's okay in this case, as it only
> happens just as shmem is being "created", so other processes have not
> yet had any time to mess things up.

But since the counter that postmaster incremented is propagated to
child processes via fork, it should be zeroed at postmaster or the
beginning of child process? Otherwise that counter always starts
with non-zero in child process.

> (IIRC only the Async module is
> doing that.)

Yes, as far as I do the test.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  42. Re: SLRU statistics

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-13T16:55:50Z

    Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> But since the counter that postmaster incremented is propagated to
> child processes via fork, it should be zeroed at postmaster or the
> beginning of child process? Otherwise that counter always starts
> with non-zero in child process.

Yes, if the postmaster is incrementing these counts then we would
have to reset them at the start of each child process.  I share
Alvaro's feeling that that's bad and we don't want to do it.

>> (IIRC only the Async module is doing that.)

Hm, maybe we can fix that.

			regards, tom lane

  43. Re: SLRU statistics

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-13T17:44:44Z

    I wrote:
>>> (IIRC only the Async module is doing that.)

> Hm, maybe we can fix that.

Yeah, it's quite easy to make async.c postpone its first write to the
async SLRU.  This seems like a win all around, because many installations
don't use NOTIFY and so will never need to do that work at all.  In
installations that do use notify, this costs an extra instruction or
two per NOTIFY, but that's down in the noise.

I got through check-world with the assertion shown that we are not
counting any SLRU operations in the postmaster.  Don't know if we
want to commit that or not --- any thoughts?

			regards, tom lane

  44. Re: SLRU statistics

    Ranier Vilela <ranier.vf@gmail.com> — 2020-05-13T20:08:39Z

    Em qua., 13 de mai. de 2020 às 11:46, Fujii Masao <
masao.fujii@oss.nttdata.com> escreveu:

>
>
> On 2020/05/13 23:26, Tom Lane wrote:
> > Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> >> On 2020/05/13 17:21, Tomas Vondra wrote:
> >>> On Wed, May 13, 2020 at 04:10:30PM +0900, Fujii Masao wrote:
> >>>> Also I found another minor issue; SLRUStats has not been initialized
> to 0
> >>>> and which could update the counters unexpectedly. Attached patch fixes
> >>>> this issue.
> >
> >> Pushed both. Thanks!
> >
> > Why is that necessary?  A static variable is defined by C to start off
> > as zeroes.
>
> Because SLRUStats is not a static variable. No?
>
IMHO, BgWriterStats  have the same problem, shouldn't the same be done?

/* Initialize BgWriterStats to zero */
MemSet(&BgWriterStats, 0, sizeof(BgWriterStats));

/* Initialize SLRU statistics to zero */
memset(&SLRUStats, 0, sizeof(SLRUStats));

regards,
Ranier Vilela

  45. Re: SLRU statistics

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-05-14T02:18:53Z

    
On 2020/05/14 2:44, Tom Lane wrote:
> I wrote:
>>>> (IIRC only the Async module is doing that.)
> 
>> Hm, maybe we can fix that.
> 
> Yeah, it's quite easy to make async.c postpone its first write to the
> async SLRU.  This seems like a win all around, because many installations
> don't use NOTIFY and so will never need to do that work at all.  In
> installations that do use notify, this costs an extra instruction or
> two per NOTIFY, but that's down in the noise.

Looks good to me. Thanks for the patch!

> I got through check-world with the assertion shown that we are not
> counting any SLRU operations in the postmaster.  Don't know if we
> want to commit that or not --- any thoughts?

+1 to add this assertion because basically it's not good thing
to access to SLRU at postmaster and we may want to fix that if found.
At least if we get rid of the SLRUStats initialization code,
IMO it's better to add this assertion and ensure that postmaster
doesn't update the SLRU stats counters.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  46. Re: SLRU statistics

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-14T02:24:48Z

    Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> On 2020/05/14 2:44, Tom Lane wrote:
>> I got through check-world with the assertion shown that we are not
>> counting any SLRU operations in the postmaster.  Don't know if we
>> want to commit that or not --- any thoughts?

> +1 to add this assertion because basically it's not good thing
> to access to SLRU at postmaster and we may want to fix that if found.
> At least if we get rid of the SLRUStats initialization code,
> IMO it's better to add this assertion and ensure that postmaster
> doesn't update the SLRU stats counters.

Seems reasonable --- I'll include it.

It might be nice to have similar assertions protecting BgWriterStats.
But given that we've made that public to be hacked on directly by several
different modules, I'm not sure that there's any simple way to do that.

			regards, tom lane

  47. Re: SLRU statistics

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-05-14T06:27:37Z

    
On 2020/05/07 13:47, Fujii Masao wrote:
> 
> 
> On 2020/05/03 1:59, Tomas Vondra wrote:
>> On Sat, May 02, 2020 at 12:55:00PM +0200, Tomas Vondra wrote:
>>> On Sat, May 02, 2020 at 03:56:07PM +0900, Fujii Masao wrote:
>>>>
>>>> ...
>>>
>>>>>> Another thing I found is; pgstat_send_slru() should be called also by
>>>>>> other processes than backend? For example, since clog data is flushed
>>>>>> basically by checkpointer, checkpointer seems to need to send slru stats.
>>>>>> Otherwise, pg_stat_slru.flushes would not be updated.
>>>>>>
>>>>>
>>>>> Hmmm, that's a good point. If I understand the issue correctly, the
>>>>> checkpointer accumulates the stats but never really sends them because
>>>>> it never calls pgstat_report_stat/pgstat_send_slru. That's only called
>>>>> from PostgresMain, but not from CheckpointerMain.
>>>>
>>>> Yes.
>>>>
>>>>> I think we could simply add pgstat_send_slru() right after the existing
>>>>> call in CheckpointerMain, right?
>>>>
>>>> Checkpointer sends off activity statistics to the stats collector in
>>>> two places, by calling pgstat_send_bgwriter(). What about calling
>>>> pgstat_send_slru() just after pgstat_send_bgwriter()?
>>>>
>>>
>>> Yep, that's what I proposed.
>>>
>>>> In previous email, I mentioned checkpointer just as an example.
>>>> So probably we need to investigate what process should send slru stats,
>>>> other than checkpointer. I guess that at least autovacuum worker,
>>>> logical replication walsender and parallel query worker (maybe this has
>>>> been already covered by parallel query some mechanisms. Sorry I've
>>>> not checked that) would need to send its slru stats.
>>>>
>>>
>>> Probably. Do you have any other process type in mind?
> 
> No. For now what I'm in mind are just checkpointer, autovacuum worker,
> logical replication walsender and parallel query worker. Seems logical
> replication worker and syncer have sent slru stats via pgstat_report_stat().

Let me go back to this topic. As far as I read the code again, logical
walsender reports the stats at the exit via pgstat_beshutdown_hook()
process-exit callback. But it doesn't report the stats while it's running.
This is not the problem only for SLRU stats. We would need to consider
how to handle the stats by logical walsender, separately from SLRU stats.

Autovacuum worker reports the stats at the exit via pgstat_beshutdown_hook(),
too. Unlike logical walsender, autovacuum worker is not the process that
basically keeps running during the service. It exits after it does vacuum or
analyze. So it's not bad to report the stats only at the exit, in autovacuum
worker case. There is no need to add extra code for SLRU stats report by
autovacuum worker.

Parallel worker is in the same situation as autovacuum worker. Its lifetime
is basically short and its stats is reported at the exit via
pgstat_beshutdown_hook().

pgstat_beshutdown_hook() reports the stats only when MyDatabaseId is valid.
Checkpointer calls pgstat_beshutdown_hook() at the exit, but doesn't report
the stats because its MyDatabaseId is invalid. Also it doesn't report the SLRU
stats while it's running. As we discussed upthread, we need to make
checkpointer call pgstat_send_slru() just after pgstat_send_bgwriter().

However even if we do this, the stats updated during the last checkpointer's
activity (e.g., shutdown checkpoint) seems not reported because
pgstat_beshutdown_hook() doesn't report the stats in checkpointer case.
Do we need to address this issue? If yes, we would need to change
pgstat_beshutdown_hook() or register another checkpointer-exit callback
that sends the stats. Thought?

Startup process is in the same situation as checkpointer process. It reports
the stats neither at the exit nor whle it's running. But, like logical
walsender, this seems not the problem only for SLRU stats. We would need to
consider how to handle the stats by startup process, separately from SLRU
stats.

Therefore what we can do right now seems to make checkpointer report the SLRU
stats while it's running. Other issues need more time to investigate...
Thought?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  48. Re: SLRU statistics

    Robert Haas <robertmhaas@gmail.com> — 2020-05-14T18:52:06Z

    On Thu, May 14, 2020 at 2:27 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> Therefore what we can do right now seems to make checkpointer report the SLRU
> stats while it's running. Other issues need more time to investigate...
> Thought?

I'm confused by why SLRU statistics are reported by messages sent to
the stats collector rather than by just directly updating shared
memory. For database or table statistics there can be any number of
objects and we can't know in advance how many there will be, so we
can't set aside shared memory for the stats in advance. For SLRUs,
there's no such problem. Just having the individual backends
periodically merge their accumulated backend-local counters into the
shared counters seems like it would be way simpler and more
performant.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  49. Re: SLRU statistics

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-14T19:45:26Z

    Robert Haas <robertmhaas@gmail.com> writes:
> I'm confused by why SLRU statistics are reported by messages sent to
> the stats collector rather than by just directly updating shared
> memory.

It would be better to consider that as an aspect of the WIP stats
collector redesign, rather than inventing a bespoke mechanism for
SLRU stats that's outside the stats collector (and, no doubt,
would have its own set of bugs).  We don't need to invent even more
pathways for this sort of data.

			regards, tom lane