From 0892319c65e7d7458ee8fb902618491aaa0e4734 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 15 Jul 2026 10:35:19 +0900 Subject: [PATCH] Rework pgstat_write_statsfile() in combination with to_serialized_data Contrary to the from_serialized_data callback used by the pgstats reads at startup, the to_serialized_data callback used for the pgstats writes matched with pgstat_write_statsfile(), by not returning a boolean status, expecting a ferror() failure to deal with the discard of the stats file should an error happen while writing the stats. This was slightly confusing designed this way. Things are changed in this commit with: - to_serialized_data now returns a boolean status on a write failure. pgstat_write_statsfile() detects that and switches to failure mode instead of continuing to process the entries to write, speeding up the shutdown. - pgstat_write_statsfile() now uses STATS_DISCARD if a failure happens, to let the registered callbacks directly know that something is wrong, and that things need to be cleaned up. This gives a better error path detection for custom stats kinds. For example, they do not have to rely solely on the expectation of an ferror() for an auxiliary file. This new set of behaviors matches with what is already done in pgstat_read_statsfile() for the finish() callback (DISCARD on failure, READ on success) and the from_serialized_data with a status returned. Author: Sami Imseih Discussion: https://postgr.es/m/CAA5RZ0sMgOvuhpb2P=KSJOjgjC6AfUu+GYcu9mHar-y_Xtd=Pg@mail.gmail.com Backpatch-through: 19 --- src/backend/utils/activity/pgstat.c | 34 +++++++++++++++---- src/include/utils/pgstat_internal.h | 5 +-- .../test_custom_stats/test_custom_var_stats.c | 29 ++++++++++------ 3 files changed, 49 insertions(+), 19 deletions(-) diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index 9c82081f4c5..5926c0c8b9c 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -1623,6 +1623,7 @@ pgstat_write_statsfile(void) const char *statfile = PGSTAT_STAT_PERMANENT_FILENAME; dshash_seq_status hstat; PgStatShared_HashEntry *ps; + PgStat_StatsFileOp status = STATS_WRITE; pgstat_assert_is_up(); @@ -1751,8 +1752,12 @@ pgstat_write_statsfile(void) pgstat_get_entry_len(ps->key.kind)); /* Write more data for the entry, if required */ - if (kind_info->to_serialized_data) - kind_info->to_serialized_data(&ps->key, shstats, fpout); + if (kind_info->to_serialized_data && + !kind_info->to_serialized_data(&ps->key, shstats, fpout)) + { + status = STATS_DISCARD; + break; + } } dshash_seq_term(&hstat); @@ -1763,7 +1768,17 @@ pgstat_write_statsfile(void) */ fputc(PGSTAT_FILE_ENTRY_END, fpout); - if (ferror(fpout)) + if (status == STATS_DISCARD) + { + /* + * A to_serialized_data callback failed. DEBUG2 because the callback + * already logged the reason. + */ + elog(DEBUG2, "discarding temporary statistics file \"%s\"", tmpfile); + FreeFile(fpout); + unlink(tmpfile); + } + else if (ferror(fpout)) { ereport(LOG, (errcode_for_file_access(), @@ -1771,6 +1786,7 @@ pgstat_write_statsfile(void) tmpfile))); FreeFile(fpout); unlink(tmpfile); + status = STATS_DISCARD; } else if (FreeFile(fpout) < 0) { @@ -1779,11 +1795,13 @@ pgstat_write_statsfile(void) errmsg("could not close temporary statistics file \"%s\": %m", tmpfile))); unlink(tmpfile); + status = STATS_DISCARD; } else if (durable_rename(tmpfile, statfile, LOG) < 0) { /* durable_rename already emitted log message */ unlink(tmpfile); + status = STATS_DISCARD; } /* Finish callbacks, if required */ @@ -1792,7 +1810,7 @@ pgstat_write_statsfile(void) const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); if (kind_info && kind_info->finish) - kind_info->finish(STATS_WRITE); + kind_info->finish(status); } } @@ -1815,6 +1833,7 @@ pgstat_read_statsfile(void) FILE *fpin; int32 format_id; bool found; + PgStat_StatsFileOp status = STATS_READ; const char *statfile = PGSTAT_STAT_PERMANENT_FILENAME; PgStat_ShmemControl *shmem = pgStatLocal.shmem; @@ -1840,7 +1859,8 @@ pgstat_read_statsfile(void) errmsg("could not open statistics file \"%s\": %m", statfile))); pgstat_reset_after_failure(); - return; + status = STATS_DISCARD; + goto finish; } /* @@ -2098,13 +2118,14 @@ done: elog(DEBUG2, "removing permanent stats file \"%s\"", statfile); unlink(statfile); +finish: /* Finish callbacks, if required */ for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++) { const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); if (kind_info && kind_info->finish) - kind_info->finish(STATS_READ); + kind_info->finish(status); } return; @@ -2114,6 +2135,7 @@ error: (errmsg("corrupted statistics file \"%s\"", statfile))); pgstat_reset_after_failure(); + status = STATS_DISCARD; goto done; } diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index cb73ec1f8e4..14aeb710291 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -322,7 +322,8 @@ typedef struct PgStat_KindInfo * an entry, in the stats file or optionally in a different file. * Optional. * - * to_serialized_data: write auxiliary data for an entry. + * to_serialized_data: write auxiliary data for an entry. Returns true on + * success, false on write error. * * from_serialized_data: read auxiliary data for an entry. Returns true * on success, false on read error. @@ -332,7 +333,7 @@ typedef struct PgStat_KindInfo * just written or read. "header" is a pointer to the stats data; it may * be modified only in from_serialized_data to reconstruct an entry. */ - void (*to_serialized_data) (const PgStat_HashKey *key, + bool (*to_serialized_data) (const PgStat_HashKey *key, const PgStatShared_Common *header, FILE *statfile); bool (*from_serialized_data) (const PgStat_HashKey *key, 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 34d474be604..a39ada0b67c 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 @@ -26,7 +26,7 @@ PG_MODULE_MAGIC_EXT( ); /* Local helpers for stats file I/O */ -#define write_chunk(fpout, ptr, len) ((void) fwrite(ptr, len, 1, fpout)) +#define write_chunk(fpout, ptr, len) (fwrite(ptr, len, 1, fpout) == 1) #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)) @@ -94,7 +94,7 @@ static bool test_custom_stats_var_flush_pending_cb(PgStat_EntryRef *entry_ref, bool nowait); /* Serialization callback: write auxiliary entry data */ -static void test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key, +static bool test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key, const PgStatShared_Common *header, FILE *statfile); @@ -193,7 +193,7 @@ test_custom_stats_var_flush_pending_cb(PgStat_EntryRef *entry_ref, bool nowait) * - The length of the description. * - The description data itself. */ -static void +static bool test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key, const PgStatShared_Common *header, FILE *statfile) @@ -208,7 +208,8 @@ 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. */ - write_chunk_s(statfile, &magic_number); + if (!write_chunk_s(statfile, &magic_number)) + return false; /* Open statistics file for writing. */ if (!fd_description) @@ -220,7 +221,7 @@ test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key, (errcode_for_file_access(), errmsg("could not open statistics file \"%s\" for writing: %m", TEST_CUSTOM_AUX_DATA_DESC))); - return; + return false; } /* Initialize offset for secondary statistics file. */ @@ -228,14 +229,16 @@ test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key, } /* Write offset to the main data file */ - write_chunk_s(statfile, &fd_description_offset); + if (!write_chunk_s(statfile, &fd_description_offset)) + return false; /* * 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. */ - write_chunk_s(fd_description, (PgStat_HashKey *) key); + if (!write_chunk_s(fd_description, (PgStat_HashKey *) key)) + return false; fd_description_offset += sizeof(PgStat_HashKey); if (!custom_stats_description_dsa) @@ -246,9 +249,10 @@ test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key, { /* length to description file */ len = 0; - write_chunk_s(fd_description, &len); + if (!write_chunk_s(fd_description, &len)) + return false; fd_description_offset += sizeof(size_t); - return; + return true; } /* @@ -258,14 +262,17 @@ 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; - write_chunk_s(fd_description, &len); - write_chunk(fd_description, description, len); + if (!write_chunk_s(fd_description, &len)) + return false; + if (!write_chunk(fd_description, description, len)) + return false; /* * Update offset for next entry, counting for the length (size_t) of the * description and the description contents. */ fd_description_offset += len + sizeof(size_t); + return true; } /* -- 2.47.3