On Thu, Mar 10, 2022 at 09:30:57PM +0000, Imseih (AWS), Sami wrote: > Attached v4 which includes accounting for the hash size on startup, removal > of the no longer needed comment in pgstatfuncs.c and a change in both > code/docs to only reset the indexes_total to 0 when failsafe is triggered.
Thanks for the new patch set. +/* + * Structs for tracking shared Progress information + * amongst worker ( and leader ) processes of a vacuum. + */ nitpick: Can we remove the extra spaces in the parentheses? + if (entry != NULL) + values[PGSTAT_NUM_PROGRESS_COMMON + PROGRESS_VACUUM_INDEXES_COMPLETED] = entry->indexes_processed; What does it mean if there isn't an entry in the map? Is this actually expected, or should we ERROR instead? + /* vacuum worker progress hash table */ + max_table_size = GetMaxBackends(); + size = add_size(size, hash_estimate_size(max_table_size, + sizeof(VacProgressEntry))); I think the number of entries should be shared between VacuumWorkerProgressShmemInit() and VacuumWorkerProgressShmemSize(). Otherwise, we might update one and not the other. + /* Call the command specific function to override datum values */ + if (pg_strcasecmp(cmd, "VACUUM") == 0) + set_vaccum_worker_progress(values); I think we should elaborate a bit more in this comment. It's difficult to follow what this is doing without referencing the comment above set_vacuum_worker_progress(). IMO the patches are in decent shape, and this should likely be marked as ready-for-committer in the near future. Before doing so, I think we should check that Sawada-san is okay with moving the deeper infrastructure changes to a separate threaḋ. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
