Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?) - Mailing list pgsql-hackers

Note that 001 fails to compile without 002:

../src/backend/storage/buffer/bufmgr.c:1257:43: error: ‘from_ring’ undeclared (first use in this function)
 1257 |       StrategyRejectBuffer(strategy, buf, from_ring))

My "warnings" script informed me about these gripes from MSVC:

[03:42:30.607] c:\cirrus>call sh -c 'if grep ": warning " build.txt; then exit 1; fi; exit 0' 
[03:42:30.749] c:\cirrus\src\backend\storage\buffer\freelist.c(699) : warning C4715: 'IOContextForStrategy': not all
controlpaths return a value
 
[03:42:30.749] c:\cirrus\src\backend\utils\activity\pgstat_io_ops.c(190) : warning C4715: 'pgstat_io_context_desc': not
allcontrol paths return a value
 
[03:42:30.749] c:\cirrus\src\backend\utils\activity\pgstat_io_ops.c(204) : warning C4715: 'pgstat_io_object_desc': not
allcontrol paths return a value
 
[03:42:30.749] c:\cirrus\src\backend\utils\activity\pgstat_io_ops.c(226) : warning C4715: 'pgstat_io_op_desc': not all
controlpaths return a value
 
[03:42:30.749] c:\cirrus\src\backend\utils\adt\pgstatfuncs.c(1816) : warning C4715: 'pgstat_io_op_get_index': not all
controlpaths return a value
 

In the docs table, you say things like:
| io_context vacuum refers to the IO operations incurred while vacuuming and analyzing.

..but it's a bit unclear (maybe due to the way the docs are rendered).
I think it may be more clear to say "when <io_context> is
<vacuum>, ..."

| acquiring the equivalent number of shared buffers

I don't think "equivelent" fits here, since it's actually acquiring a
different number of buffers.

There's a missing period before " The difference is"

The sentence beginning "read plus extended for backend_types" is difficult to
parse due to having a bulleted list in its middle.

There aren't many references to "IOOps", which is good, because I
started to read it as "I oops".

+        * Flush IO Operations statistics now. pgstat_report_stat() will flush IO
+        * Operation stats, however this will not be called after an entire

=> I think that's intended to say *until* after ?

+ * Functions to assert that invalid IO Operation counters are zero.

=> There's a missing newline above this comment.

+       Assert(counters->evictions == 0 && counters->extends == 0 &&
+                       counters->fsyncs == 0 && counters->reads == 0 && counters->reuses
+                       == 0 && counters->writes == 0);

=> It'd be more readable and also maybe help debugging if these were separate
assertions.  I wondered in the past if that should be a general policy
for all assertions.

+pgstat_io_op_stats_collected(BackendType bktype)
+{
+       return bktype != B_INVALID && bktype != B_ARCHIVER && bktype != B_LOGGER &&
+               bktype != B_WAL_RECEIVER && bktype != B_WAL_WRITER;

Similar: I'd prefer to see this as 5 "ifs" or a "switch" to return
false, else return true.  But YMMV.

+                * CREATE TEMPORRARY TABLE AS ...

=> typo: temporary

+       if (strategy_io_context  && io_op == IOOP_FSYNC)

=> Extra space.

pgstat_count_io_op() has a superflous newline before "}".

I think there may be a problem/deficiency with hint bits:

|postgres=# DROP TABLE u2; CREATE TABLE u2 AS SELECT generate_series(1,999999)a; SELECT pg_stat_reset_shared('io');
explain(analyze,buffers) SELECT * FROM u2;
 
|...
| Seq Scan on u2  (cost=0.00..15708.75 rows=1128375 width=4) (actual time=0.111..458.239 rows=999999 loops=1)
|   Buffers: shared hit=2048 read=2377 dirtied=2377 written=2345

|postgres=# SELECT COUNT(1), relname, COUNT(1) FILTER(WHERE isdirty) FROM pg_buffercache b LEFT JOIN pg_class c ON
pg_relation_filenode(c.oid)=b.relfilenodeGROUP BY 2 ORDER BY 1 DESC LIMIT 11;
 
| count |             relname             | count
|-------+---------------------------------+-------
| 13619 |                                 |     0
|  2080 | u2                              |  2080
|   104 | pg_attribute                    |     4
|    71 | pg_statistic                    |     1
|    51 | pg_class                        |     1

It says that SELECT caused 2377 buffers to be dirtied, of which 2080 are
associated with the new table in pg_buffercache.

|postgres=# SELECT * FROM pg_stat_io WHERE backend_type!~'autovac|archiver|logger|standalone|startup|^wal|background
worker'or true ORDER BY 2;
 
|    backend_type     | io_context  |   io_object   | read | written | extended | op_bytes | evicted | reused |
files_synced|          stats_reset
 
|...
| client backend      | bulkread    | relation      | 2377 |    2345 |          |     8192 |       0 |   2345 |
    | 2022-11-22 22:32:33.044552-06
 

I think it's a known behavior that hint bits do not use the strategy
ring buffer.  For BAS_BULKREAD, ring_size = 256kB (32, 8kB pages), but
there's 2080 dirty pages in the buffercache (~16MB).

But the IO view says that 2345 of the pages were "reused", which seems
misleading to me.  Maybe that just follows from the behavior and the view is
fine.  If the view is fine, maybe this case should still be specifically
mentioned in the docs.

-- 
Justin