From 5ad01853f38a2b78ba66be71f4bc886812f7c369 Mon Sep 17 00:00:00 2001 From: Sami Imseih Date: Mon, 13 Jul 2026 14:22:43 -0500 Subject: [PATCH v1 1/2] Revert "Rename routines for write/read of pgstats file" This reverts commit ed823da1289. The published API (pgstat_write_chunk/pgstat_read_chunk) had an inconsistent contract: write returned void and deferred error checking to ferror(), while read returned bool with inline error checking. Rather than fixing the inconsistency by documenting or changing the API, make these functions internal again. Extensions that need file I/O in their serialization callbacks can use fwrite/fread directly and manage their own error handling. Update test_custom_var_stats to use local inline helpers wrapping fwrite/fread instead of the removed API. --- src/backend/utils/activity/pgstat.c | 55 ++++++++++--------- src/include/utils/pgstat_internal.h | 5 -- .../test_custom_stats/test_custom_var_stats.c | 28 ++++++---- 3 files changed, 46 insertions(+), 42 deletions(-) diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index 8b2a5ec3675..debcc87af50 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -1619,15 +1619,18 @@ pgstat_assert_is_up(void) * ------------------------------------------------------------ */ -/* helper for pgstat_write_statsfile() */ -void -pgstat_write_chunk(FILE *fpout, void *ptr, size_t len) +#define write_chunk_s(fpout, ptr) write_chunk(fpout, ptr, sizeof(*ptr)) +#define read_chunk_s(fpin, ptr) read_chunk(fpin, ptr, sizeof(*ptr)) + +/* helpers for pgstat_write_statsfile() */ +static void +write_chunk(FILE *fpout, void *ptr, size_t len) { int rc; rc = fwrite(ptr, len, 1, fpout); - /* We check for errors with ferror() when done writing the stats. */ + /* we'll check for errors with ferror once at the end */ (void) rc; } @@ -1672,7 +1675,7 @@ pgstat_write_statsfile(void) * Write the file header --- currently just a format ID. */ format_id = PGSTAT_FILE_FORMAT_ID; - pgstat_write_chunk_s(fpout, &format_id); + write_chunk_s(fpout, &format_id); /* Write various stats structs for fixed number of objects */ for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++) @@ -1697,8 +1700,8 @@ pgstat_write_statsfile(void) ptr = pgStatLocal.snapshot.custom_data[kind - PGSTAT_KIND_CUSTOM_MIN]; fputc(PGSTAT_FILE_ENTRY_FIXED, fpout); - pgstat_write_chunk_s(fpout, &kind); - pgstat_write_chunk(fpout, ptr, info->shared_data_len); + write_chunk_s(fpout, &kind); + write_chunk(fpout, ptr, info->shared_data_len); } /* @@ -1752,7 +1755,7 @@ pgstat_write_statsfile(void) { /* normal stats entry, identified by PgStat_HashKey */ fputc(PGSTAT_FILE_ENTRY_HASH, fpout); - pgstat_write_chunk_s(fpout, &ps->key); + write_chunk_s(fpout, &ps->key); } else { @@ -1762,14 +1765,14 @@ pgstat_write_statsfile(void) kind_info->to_serialized_name(&ps->key, shstats, &name); fputc(PGSTAT_FILE_ENTRY_NAME, fpout); - pgstat_write_chunk_s(fpout, &ps->key.kind); - pgstat_write_chunk_s(fpout, &name); + write_chunk_s(fpout, &ps->key.kind); + write_chunk_s(fpout, &name); } /* Write except the header part of the entry */ - pgstat_write_chunk(fpout, - pgstat_get_entry_data(ps->key.kind, shstats), - pgstat_get_entry_len(ps->key.kind)); + write_chunk(fpout, + pgstat_get_entry_data(ps->key.kind, shstats), + pgstat_get_entry_len(ps->key.kind)); /* Write more data for the entry, if required */ if (kind_info->to_serialized_data) @@ -1780,7 +1783,7 @@ pgstat_write_statsfile(void) /* * No more output to be done. Close the temp file and replace the old * pgstat.stat with it. The ferror() check replaces testing for error - * after each individual fputc or fwrite (in pgstat_write_chunk()) above. + * after each individual fputc or fwrite (in write_chunk()) above. */ fputc(PGSTAT_FILE_ENTRY_END, fpout); @@ -1817,9 +1820,9 @@ pgstat_write_statsfile(void) } } -/* helper for pgstat_read_statsfile() */ -bool -pgstat_read_chunk(FILE *fpin, void *ptr, size_t len) +/* helpers for pgstat_read_statsfile() */ +static bool +read_chunk(FILE *fpin, void *ptr, size_t len) { return fread(ptr, 1, len, fpin) == len; } @@ -1867,7 +1870,7 @@ pgstat_read_statsfile(void) /* * Verify it's of the expected format. */ - if (!pgstat_read_chunk_s(fpin, &format_id)) + if (!read_chunk_s(fpin, &format_id)) { elog(WARNING, "could not read format ID"); goto error; @@ -1897,7 +1900,7 @@ pgstat_read_statsfile(void) char *ptr; /* entry for fixed-numbered stats */ - if (!pgstat_read_chunk_s(fpin, &kind)) + if (!read_chunk_s(fpin, &kind)) { elog(WARNING, "could not read stats kind for entry of type %c", t); goto error; @@ -1937,7 +1940,7 @@ pgstat_read_statsfile(void) info->shared_data_off; } - if (!pgstat_read_chunk(fpin, ptr, info->shared_data_len)) + if (!read_chunk(fpin, ptr, info->shared_data_len)) { elog(WARNING, "could not read data of stats kind %u for entry of type %c with size %u", kind, t, info->shared_data_len); @@ -1959,7 +1962,7 @@ pgstat_read_statsfile(void) if (t == PGSTAT_FILE_ENTRY_HASH) { /* normal stats entry, identified by PgStat_HashKey */ - if (!pgstat_read_chunk_s(fpin, &key)) + if (!read_chunk_s(fpin, &key)) { elog(WARNING, "could not read key for entry of type %c", t); goto error; @@ -1988,12 +1991,12 @@ pgstat_read_statsfile(void) PgStat_Kind kind; NameData name; - if (!pgstat_read_chunk_s(fpin, &kind)) + if (!read_chunk_s(fpin, &kind)) { elog(WARNING, "could not read stats kind for entry of type %c", t); goto error; } - if (!pgstat_read_chunk_s(fpin, &name)) + if (!read_chunk_s(fpin, &name)) { elog(WARNING, "could not read name of stats kind %u for entry of type %c", kind, t); @@ -2068,9 +2071,9 @@ pgstat_read_statsfile(void) key.objid, t); } - if (!pgstat_read_chunk(fpin, - pgstat_get_entry_data(key.kind, header), - pgstat_get_entry_len(key.kind))) + if (!read_chunk(fpin, + pgstat_get_entry_data(key.kind, header), + pgstat_get_entry_len(key.kind))) { elog(WARNING, "could not read data for entry %u/%u/%" PRIu64 " of type %c", key.kind, key.dboid, diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index b3dc3ff7d8b..02022f19b81 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -886,11 +886,6 @@ extern PGDLLIMPORT bool pgstat_report_fixed; /* Backend-local stats state */ extern PGDLLIMPORT PgStat_LocalState pgStatLocal; -/* Helper functions for reading and writing of on-disk stats file */ -extern void pgstat_write_chunk(FILE *fpout, void *ptr, size_t len); -extern bool pgstat_read_chunk(FILE *fpin, void *ptr, size_t len); -#define pgstat_read_chunk_s(fpin, ptr) pgstat_read_chunk(fpin, ptr, sizeof(*ptr)) -#define pgstat_write_chunk_s(fpout, ptr) pgstat_write_chunk(fpout, ptr, sizeof(*ptr)) /* * Implementation of inline functions declared above. diff --git a/src/test/modules/test_custom_stats/test_custom_var_stats.c b/src/test/modules/test_custom_stats/test_custom_var_stats.c index 024dae85a45..34d474be604 100644 --- a/src/test/modules/test_custom_stats/test_custom_var_stats.c +++ b/src/test/modules/test_custom_stats/test_custom_var_stats.c @@ -25,6 +25,12 @@ PG_MODULE_MAGIC_EXT( .version = PG_VERSION ); +/* Local helpers for stats file I/O */ +#define write_chunk(fpout, ptr, len) ((void) fwrite(ptr, len, 1, fpout)) +#define write_chunk_s(fpout, ptr) write_chunk(fpout, ptr, sizeof(*ptr)) +#define read_chunk(fpin, ptr, len) (fread(ptr, 1, len, fpin) == (len)) +#define read_chunk_s(fpin, ptr) read_chunk(fpin, ptr, sizeof(*ptr)) + #define TEST_CUSTOM_VAR_MAGIC_NUMBER (0xBEEFBEEF) /*-------------------------------------------------------------------------- @@ -202,7 +208,7 @@ test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key, * First mark the main file with a magic number, keeping a trace that some * auxiliary data will exist in the secondary statistics file. */ - pgstat_write_chunk_s(statfile, &magic_number); + write_chunk_s(statfile, &magic_number); /* Open statistics file for writing. */ if (!fd_description) @@ -222,14 +228,14 @@ test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key, } /* Write offset to the main data file */ - pgstat_write_chunk_s(statfile, &fd_description_offset); + write_chunk_s(statfile, &fd_description_offset); /* * First write the entry key to the secondary statistics file. This will * be cross-checked with the key read from main stats file at loading * time. */ - pgstat_write_chunk_s(fd_description, (PgStat_HashKey *) key); + write_chunk_s(fd_description, (PgStat_HashKey *) key); fd_description_offset += sizeof(PgStat_HashKey); if (!custom_stats_description_dsa) @@ -240,7 +246,7 @@ test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key, { /* length to description file */ len = 0; - pgstat_write_chunk_s(fd_description, &len); + write_chunk_s(fd_description, &len); fd_description_offset += sizeof(size_t); return; } @@ -252,8 +258,8 @@ test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key, description = dsa_get_address(custom_stats_description_dsa, entry->description); len = strlen(description) + 1; - pgstat_write_chunk_s(fd_description, &len); - pgstat_write_chunk(fd_description, description, len); + write_chunk_s(fd_description, &len); + write_chunk(fd_description, description, len); /* * Update offset for next entry, counting for the length (size_t) of the @@ -287,7 +293,7 @@ test_custom_stats_var_from_serialized_data(const PgStat_HashKey *key, PgStat_HashKey file_key; /* Check the magic number first, in the main file. */ - if (!pgstat_read_chunk_s(statfile, &magic_number)) + if (!read_chunk_s(statfile, &magic_number)) { elog(WARNING, "failed to read magic number from statistics file"); return false; @@ -304,7 +310,7 @@ test_custom_stats_var_from_serialized_data(const PgStat_HashKey *key, * Read the offset from the main stats file, to be able to read the * auxiliary data from the secondary statistics file. */ - if (!pgstat_read_chunk_s(statfile, &offset)) + if (!read_chunk_s(statfile, &offset)) { elog(WARNING, "failed to read metadata offset from statistics file"); return false; @@ -335,7 +341,7 @@ test_custom_stats_var_from_serialized_data(const PgStat_HashKey *key, } /* Read the hash key from the secondary statistics file */ - if (!pgstat_read_chunk_s(fd_description, &file_key)) + if (!read_chunk_s(fd_description, &file_key)) { elog(WARNING, "failed to read hash key from file"); return false; @@ -355,7 +361,7 @@ test_custom_stats_var_from_serialized_data(const PgStat_HashKey *key, entry = (PgStatShared_CustomVarEntry *) header; /* Read the description length and its data */ - if (!pgstat_read_chunk_s(fd_description, &len)) + if (!read_chunk_s(fd_description, &len)) { elog(WARNING, "failed to read metadata length from statistics file"); return false; @@ -379,7 +385,7 @@ test_custom_stats_var_from_serialized_data(const PgStat_HashKey *key, } buffer = palloc(len); - if (!pgstat_read_chunk(fd_description, buffer, len)) + if (!read_chunk(fd_description, buffer, len)) { pfree(buffer); elog(WARNING, "failed to read description from file"); -- 2.50.1 (Apple Git-155)