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

v16 (also rebased) attached

On Fri, Nov 26, 2021 at 4:16 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> 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.

Fixed

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

Fixed

>
> +       tupstore = tuplestore_begin_heap(true, false, work_mem);
>
> You're passing a constant randomAccess=true to tuplestore_begin_heap ;)

Fixed

>
> +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];

I've changed this to a 3D array as you suggested and removed the NROWS
macro.

> 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.

I think passing it to the function is okay. The parameter type would be
adjusted from an array to a pointer.
I am not sure if the allocation on the stack in the body of
pg_stat_get_buffers is too large. (left as is for now)

> 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];

with your suggested changes to a 3D array, it now does use multidimensional
array access

> 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]

If I am understanding the idea of the macro, it would change the call
site from this:

+Datum *values = get_pg_stat_buffers_row(all_values,
beentry->st_backendType, io_path);

+values[COLUMN_ALLOCS] += pg_atomic_read_u64(&io_ops->allocs);
+values[COLUMN_FSYNCS] += pg_atomic_read_u64(&io_ops->fsyncs);

to this:

+Datum *row =  ROW(beentry->st_backendType, io_path);

+row[COLUMN_ALLOCS] += pg_atomic_read_u64(&io_ops->allocs);
+row[COLUMN_FSYNCS] += pg_atomic_read_u64(&io_ops->fsyncs);

I usually prefer functions to macros, but I am fine with changing it.
(I did not change it in this version)
I have changed all the local variables from "values" to "row" which
I think is a bit clearer.

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

If I am understanding this idea, the call site would look like this now:
+CELL(beentry->st_backendType, io_path, COLUMN_FSYNCS) +=
pg_atomic_read_u64(&io_ops->fsyncs);
+CELL(beentry->st_backendType, io_path, COLUMN_ALLOCS) +=
pg_atomic_read_u64(&io_ops->allocs);

I don't like this as much. Since this code is inside of a loop, it kind
of makes sense to me that you get a row at the top of the loop and then
fill in all the cells in the row using that "row" variable.

> The enum with COLUMN_LENGTH should be named.

I only use the values in it, so it didn't need a name.

> 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;
> ...

I find it easier to understand with it in code instead of as a comment.

> *Note the use of += and not =.

Thanks for seeing this. I have changed this (to use +=).

> 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.

I think what I currently have is technically correct because I start at
1 when I am using it as a loop condition. I do waste a spot in the
arrays I allocate with BACKEND_NUM_TYPES size.

I was hesitant to make the value of BACKEND_NUM_TYPES == B_LOGGER
because it seems kind of weird to have it have the same value as the
B_LOGGER enum.

I am open to changing it. (I didn't change it in this v16).

- Melanie