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 <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0sMgOvuhpb2P=KSJOjgjC6AfUu+GYcu9mHar-y_Xtd=Pg@mail.gmail.com
Backpatch-through: 19

diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 9c82081f4c59f2cba1a18fda05f3775b2e4cb934..5926c0c8b9ced30cd500a19ac8f3674b7ae2e11f 100644 (file)
--- 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 cb73ec1f8e4b3ebe32d82bad2ddd441873fc82b9..14aeb7102916f768d7719b484dcbf181294a7c9e 100644 (file)
--- 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 34d474be604dfd0dbfbbb8d3662a6bcdcf725016..a39ada0b67c8397aca378b4eaf38ddb38dfea304 100644 (file)
--- 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;
 }
 
 /*