Horton

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

Justin Pryzby <pryzby@telsasoft.com>

View thread View on postgr.es
From: Justin Pryzby <pryzby@telsasoft.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, Magnus Hagander <magnus@hagander.net>, pgsql-hackers@postgresql.org, Lukas Fittl <lukas@fittl.com>, Thomas Munro <thomas.munro@gmail.com>
Date: 2021-11-26T21:16:36Z
Lists: pgsql-hackers 
On Wed, Nov 24, 2021 at 07:15:59PM -0600, Justin Pryzby wrote:
> There's extraneous blank lines in these functions:
> 
> +pgstat_sum_io_path_ops
> +pgstat_report_live_backend_io_path_ops
> +pgstat_recv_resetsharedcounter
> +GetIOPathDesc
> +StrategyRejectBuffer

+ an extra blank line pgstat_reset_shared_counters.

In 0005:

monitoring.sgml says that the columns in pg_stat_buffers are integers, but
they're actually bigint.

+       tupstore = tuplestore_begin_heap(true, false, work_mem);

You're passing a constant randomAccess=true to tuplestore_begin_heap ;)

+Datum all_values[NROWS][COLUMN_LENGTH];

If you were to allocate this as an array, I think it could actually be 3-D:
Datum all_values[BACKEND_NUM_TYPES-1][IOPATH_NUM_TYPES][COLUMN_LENGTH];

But I don't know if this is portable across postgres' supported platforms; I
haven't seen any place which allocates a multidimensional array on the stack,
nor passes one to a function:

+static inline Datum *
+get_pg_stat_buffers_row(Datum all_values[NROWS][COLUMN_LENGTH], BackendType backend_type, IOPath io_path)

Maybe the allocation half is okay (I think it's ~3kB), but it seems easier to
palloc the required amount than to research compiler behavior.

That function is only used as a one-line helper, and doesn't use
multidimensional array access anyway:

+       return all_values[(backend_type - 1) * IOPATH_NUM_TYPES + io_path];

I think it'd be better as a macro, like (I think)
#define ROW(backend_type, io_path) all_values[NROWS*(backend_type-1)+io_path]

Maybe it should take the column type as a 3 arg.

The enum with COLUMN_LENGTH should be named.

Or maybe it should be removed, and the enum names moved to comments, like:

+                       /* backend_type */
+                       values[val++] = backend_type_desc;

+                       /* io_path */
+                       values[val++] = CStringGetTextDatum(GetIOPathDesc(io_path)); 

+                       /* allocs */
+                       values[val++] += io_ops->allocs - resets->allocs;
...

*Note the use of += and not =.

Also:
src/include/miscadmin.h:#define BACKEND_NUM_TYPES (B_LOGGER + 1)

I think it's wrong to say NUM_TYPES = B_LOGGER + 1 (which would suggest using
lessthan-or-equal instead of lessthan as you are).

Since the valid backend types start at 1 , the "count" of backend types is
currently B_LOGGER (13) - not 14.  I think you should remove the "+1" here.
Then NROWS (if it continued to exist at all) wouldn't need to subtract one.

-- 
Justin

Commits

  1. Stabilize pg_stat_io writes test

  2. Fix flakey pg_stat_io test

  3. Suppress more compiler warnings in new pgstats code.

  4. Suppress compiler warnings in new pgstats code.

  5. Add tests for pg_stat_io

  6. Create regress_tblspc in test_setup

  7. Add pg_stat_io view, providing more detailed IO statistics

  8. pgstat: Track more detailed relation IO statistics

  9. pgstat: Infrastructure for more detailed IO statistics

  10. doc: Fix some issues in logical replication section

  11. Manual cleanup and pgindent of pgstat and bufmgr related code

  12. Have the planner consider Incremental Sort for DISTINCT

  13. Use actual backend IDs in pg_stat_get_backend_idset() and friends.

  14. Remove redundant call to pgstat_report_wal()

  15. Add BackendType for standalone backends

  16. Initialize backend status reporting during bootstrap.