v16 (also rebased) attached On Fri, Nov 26, 2021 at 4:16 PM Justin Pryzby <pry...@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
v16-0006-Remove-superfluous-bgwriter-stats.patch
Description: Binary data
v16-0004-Add-buffers-to-pgstat_reset_shared_counters.patch
Description: Binary data
v16-0007-small-comment-correction.patch
Description: Binary data
v16-0005-Add-system-view-tracking-IO-ops-per-backend-type.patch
Description: Binary data
v16-0003-Send-IO-operations-to-stats-collector.patch
Description: Binary data
v16-0002-Add-IO-operation-counters-to-PgBackendStatus.patch
Description: Binary data
v16-0001-Read-only-atomic-backend-write-function.patch
Description: Binary data
