From 91ed0e69a7df00be147548973a8172322998b528 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Sun, 22 Mar 2026 06:53:34 -0400 Subject: [PATCH v5 4/5] Fix get_archive_wal_entry to handle cur_file transitions reliably. As noted by Tom Lane, get_archive_wal_entry() uses cur_file in an unsafe way: a single read_archive_file() call can trigger multiple astreamer callbacks when compression is effective, causing cur_file to change several times (entry -> NULL -> new entry) within one call. The old code captured cur_file before the read and checked for changes after, but this missed intermediate transitions. This could cause spill-file handles to leak or data to not be flushed when the streamer finishes one segment and starts another within the same read. Restructure the spill logic to explicitly track the entry being spilled (spill_entry) separately from cur_file, and detect transitions at the top of each loop iteration. Also ensure spill file handles are closed on both success and error paths. Discussion: https://postgr.es/m/2178517.1774064942@sss.pgh.pa.us --- src/bin/pg_waldump/archive_waldump.c | 117 ++++++++++++++++----------- 1 file changed, 69 insertions(+), 48 deletions(-) diff --git a/src/bin/pg_waldump/archive_waldump.c b/src/bin/pg_waldump/archive_waldump.c index 3fce2183099..93ed856c674 100644 --- a/src/bin/pg_waldump/archive_waldump.c +++ b/src/bin/pg_waldump/archive_waldump.c @@ -463,11 +463,18 @@ free_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo) * found. If the archive streamer is reading a WAL file from the archive that * is not currently needed, that data is spilled to a temporary file for later * retrieval. + * + * Because a single read_archive_file() call may trigger multiple astreamer + * callbacks (especially when data compresses well), cur_file can change + * several times within one call: from one entry to NULL (member trailer), + * and then to a new entry (next member header). The spill logic below + * handles this by flushing and closing per-entry state whenever we detect + * that the streamer has moved on. */ static ArchivedWALFile * get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo) { - ArchivedWALFile *entry = NULL; + ArchivedWALFile *spill_entry = NULL; FILE *write_fp = NULL; /* @@ -477,6 +484,8 @@ get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo) */ while (1) { + ArchivedWALFile *entry; + /* * Search hash table. * @@ -488,64 +497,76 @@ get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo) entry = ArchivedWAL_lookup(privateInfo->archive_wal_htab, fname); if (entry != NULL) + { + /* Close any open spill file before returning. */ + if (write_fp != NULL) + fclose(write_fp); return entry; - - /* - * Capture the current entry before calling read_archive_file(), - * because cur_file may advance to a new segment during streaming. We - * hold this reference so we can flush any remaining buffer data and - * close the write handle once we detect that cur_file has moved on. - */ - entry = privateInfo->cur_file; - - /* - * Fetch more data either when no current file is being tracked or - * when its buffer has been fully flushed to the temporary file. - */ - if (entry == NULL || entry->buf->len == 0) - { - if (!read_archive_file(privateInfo, READ_CHUNK_SIZE)) - break; /* archive file ended */ } /* - * Archive streamer is reading a non-WAL file or an irrelevant WAL - * file. - */ - if (entry == NULL) - continue; - - /* - * The streamer is producing a WAL segment that isn't the one asked - * for; it must be arriving out of order. Spill its data to disk so - * it can be read back when needed. - */ - Assert(strcmp(fname, entry->fname) != 0); - - /* Create a temporary file if one does not already exist */ - if (!entry->spilled) - { - write_fp = prepare_tmp_write(entry->fname, privateInfo); - entry->spilled = true; - } - - /* Flush data from the buffer to the file */ - perform_tmp_write(entry->fname, entry->buf, write_fp); - resetStringInfo(entry->buf); - - /* - * If cur_file changed since we captured entry above, the archive - * streamer has finished this segment and moved on. Close its spill - * file handle so data is flushed to disk before the next segment - * starts writing to a different handle. + * If the streamer has moved on to a different entry than the one we + * were spilling, flush any remaining data for the old entry and close + * its spill file. */ - if (entry != privateInfo->cur_file && write_fp != NULL) + if (spill_entry != NULL && spill_entry != privateInfo->cur_file) { + if (spill_entry->buf->len > 0) + { + perform_tmp_write(spill_entry->fname, spill_entry->buf, + write_fp); + resetStringInfo(spill_entry->buf); + } fclose(write_fp); write_fp = NULL; + spill_entry = NULL; } + + /* + * If no WAL file is currently being streamed (cur_file is NULL), or + * the current spill entry's buffer has been fully flushed, we need + * more data from the archive. + */ + if (privateInfo->cur_file == NULL || + (spill_entry != NULL && spill_entry->buf->len == 0)) + { + if (!read_archive_file(privateInfo, READ_CHUNK_SIZE)) + break; /* archive fully exhausted */ + continue; /* re-check hash table and cur_file */ + } + + /* + * cur_file points to a WAL segment that isn't the one asked for; it + * must be arriving out of order. Spill its data to disk so it can be + * read back when needed. + */ + spill_entry = privateInfo->cur_file; + Assert(strcmp(fname, spill_entry->fname) != 0); + + /* Create a temporary file if one does not already exist */ + if (!spill_entry->spilled) + { + write_fp = prepare_tmp_write(spill_entry->fname, privateInfo); + spill_entry->spilled = true; + } + + /* Flush data from the buffer to the file */ + perform_tmp_write(spill_entry->fname, spill_entry->buf, write_fp); + resetStringInfo(spill_entry->buf); + + /* + * Read more data from the archive. This may add data to the current + * spill_entry's buffer, advance cur_file to a new entry, or set + * cur_file to NULL (member trailer). + */ + if (!read_archive_file(privateInfo, READ_CHUNK_SIZE)) + break; /* archive fully exhausted */ } + /* Close any open spill file before erroring out. */ + if (write_fp != NULL) + fclose(write_fp); + /* Requested WAL segment not found */ pg_fatal("could not find WAL \"%s\" in archive \"%s\"", fname, privateInfo->archive_name); -- 2.43.0