Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Move tar detection and compression logic to common.
- c8a350a43982 19 (unreleased) landed
-
pg_verifybackup: Enable WAL parsing for tar-format backups
- b3cf461b3cf9 19 (unreleased) landed
-
pg_waldump: Add support for reading WAL from tar archives
- b15c1513984e 19 (unreleased) landed
-
pg_waldump: Preparatory refactoring for tar archive WAL decoding.
- f8a0cd267170 19 (unreleased) landed
-
pg_verifybackup: Verify tar-format backups.
- 8dfd31290279 18.0 cited
-
pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-08-07T14:17:08Z
Hi All, Attaching patch to support a new feature that let pg_waldump decode WAL files directly from a tar archive. This was worked to address a limitation in pg_verifybackup[1], which couldn't parse WAL files from tar-formatted backups. The implementation will align with pg_waldump's existing xlogreader design, which uses three callback functions to manage WAL segments: open, read, and close. For tar archives, however, the approach will be simpler. Instead of using separate callbacks for opening and closing, the tar archive will be opened once at the start and closed explicitly at the end. The core logic will be in the WAL page reading callback. When xlogreader requests a new WAL page, this callback will be invoked. It will then call the archive streamer routine to read the WAL data from the tar archive into a buffer. This data will then be copied into xlogreader's own buffer, completing the read. Essentially, this is plumbing work: the new code will be responsible for getting WAL data from the tar archive and feeding it to the existing xlogreader. All other WAL page and record decoding logic, which is already robust within xlogreader, will be reused as is. This feature is being implemented in a series of patches as: - Refactoring: The first few patches (0001-0004) are dedicated to refactoring and minor code changes. - 005: This patch introduces the core functionality for pg_waldump to read WAL from a tar archive using the same archive streamer (fe_utils/astreamer.h) used in pg_verifybackup. This version requires WAL files in the archive to be in sequential order. - 006: This patch removes the sequential order restriction. If pg_waldump encounters an out-of-order WAL file, it writes the file to a temporary directory. The utility will then continue decoding and read from this temporary location later. - 007 and onwards: These patches will update pg_verifybackup to remove the restriction on WAL parsing for tar-formatted backups. 008 patch renames the "--wal-directory" switch to "--wal-path" to make it more generic, allowing it accepts a directory path or a tar archive path. ----------------------------------- Known Issues & Status: ----------------------------------- - Timeline Switching: The current implementation in patch 006 does not correctly handle timeline switching. This is a known issue, especially when a timeline change occurs on a WAL file that has been written to a temporary location. - Testing: Local regression tests on CentOS and macOS M4 are passing. However, some tests on macOS Sonoma (specifically 008_untar.pl and 010_client_untar.pl) are failing in the GitHub workflow with a "WAL parsing failed for timeline 1" error. This issue is currently being investigated. Please take a look at the attached patch and let me know your thoughts. This is an initial version, and I am making incremental improvements to address known issues and limitations. 1] https://git.postgresql.org/pg/commitdiff/8dfd3129027969fdd2d9d294220c867d2efd84aa -- Regards, Amul Sul EDB: http://www.enterprisedb.com
-
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-08-25T12:28:44Z
On Thu, Aug 7, 2025 at 7:47 PM Amul Sul <sulamul@gmail.com> wrote: > [....] > ----------------------------------- > Known Issues & Status: > ----------------------------------- > - Timeline Switching: The current implementation in patch 006 does not > correctly handle timeline switching. This is a known issue, especially > when a timeline change occurs on a WAL file that has been written to a > temporary location. > This is still pending and will be addressed in the next version. Therefore, patch 0006 remains marked as WIP. > - Testing: Local regression tests on CentOS and macOS M4 are passing. > However, some tests on macOS Sonoma (specifically 008_untar.pl and > 010_client_untar.pl) are failing in the GitHub workflow with a "WAL > parsing failed for timeline 1" error. This issue is currently being > investigated. > This has been fixed in the attached version; all GitHub workflow tests are now fine. Regards, Amul
-
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-08-26T11:52:20Z
On Mon, Aug 25, 2025 at 5:58 PM Amul Sul <sulamul@gmail.com> wrote: > > On Thu, Aug 7, 2025 at 7:47 PM Amul Sul <sulamul@gmail.com> wrote: > > [....] > > ----------------------------------- > > Known Issues & Status: > > ----------------------------------- > > - Timeline Switching: The current implementation in patch 006 does not > > correctly handle timeline switching. This is a known issue, especially > > when a timeline change occurs on a WAL file that has been written to a > > temporary location. > > > > This is still pending and will be addressed in the next version. > Therefore, patch 0006 remains marked as WIP. > After testing pg_waldump, I have realised that my previous understanding of its timeline handling was incorrect. I had mistakenly assumed by reading xlogreader code that it would use the same timeline-switching logic found in xlogreader, without first verifying this behavior. In testing, I found that pg_waldump does not follow timeline switches. Instead, it expects all WAL files to be from a single timeline, which is either specified by the user or determined from the starting segment or default 1. This is a positive finding, as it means we don't need to make significant changes to align pg_waldump's current behavior. The attached patches are now complete and no longer works in progress -- read for review. Additionally, I've dropped patch v2-0004 because it is no longer necessary. The primary patches that implement the proposed feature are now 0004 and 0005 in the attached set. Regards, Amul
-
Re: pg_waldump: support decoding of WAL inside tarfile
Jakub Wartak <jakub.wartak@enterprisedb.com> — 2025-09-08T13:37:02Z
On Tue, Aug 26, 2025 at 1:53 PM Amul Sul <sulamul@gmail.com> wrote: > [..patch] Hi Amul! 0001: LGTM, maybe I would just slightly enhance the commit message ("This is in preparation for adding a second source file to this directory.") -- maye bit a bit more verbose or use a message from 0002? 0002: LGTM 0003: LGTM Tested here (after partial patch apply, and test suite did work fine). 0004: a. Why should it be necessary to provide startLSN (-s) ? Couldn't it autodetect the first WAL (tar file) inside and just use that with some info message? $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar pg_waldump: error: no start WAL location given b. Why would it like to open "blah" dir if I wanted that "blah" segment from the archive? Shouldn't it tell that it was looking in the archive and couldn find it inside? $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar blah pg_waldump: error: could not open file "blah": Not a directory c. It doesnt work when using SEGSTART, but it's there: $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar 000000010000000000000059 pg_waldump: error: could not open file "000000010000000000000059": Not a directory $ tar tf /tmp/base/pg_wal.tar | head -1 000000010000000000000059 d. I've later noticed that follow-up patches seem to use the -s switch and there it seems to work OK. The above SEGSTART issue was not detected, probably because tests need to be extended cover of segment name rather than just --start LSN (see test_pg_waldump): $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar --stats -s 0/59000358 pg_waldump: first record is after 0/59000358, at 0/590003E8, skipping over 144 bytes WAL statistics between 0/590003E8 and 0/61000000: [..] e. Code around`if (walpath == NULL && directory != NULL)` needs some comments. f. Code around `if (fname != NULL && is_tar_file(fname, &compression))` , so if fname is WAL segment here (00000001000000000000005A) and we do check again if that has been tar-ed (is_tar_file())? Why? g. Just a question: the commit message says `Note that this patch requires that the WAL files within the archive be in sequential order; an error will be reported otherwise`. I'm wondering if such occurrences are known to be happening in the wild? Or is it just an assumption that if someone would modify the tar somehow? (either way we could just add a reason why we need to handle such a case if we know -- is manual alternation the only source of such state?). For the record, I've tested crafting custom archives with out of sequence WAL archives and the code seems to work (it was done using: tar --append -f pg_wal.tar --format=ustar ..) h. Anyway, in case of typo/wrong LSN, 0004 emits wrong error message I think: $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar --stats -s 0/50000358 pg_waldump: error: WAL files are not archived in sequential order pg_waldump: detail: Expecting segment number 80 but found 89. it's just that the 50000358 LSN above is below the minimal LSN present in the WAL segments (first segment is 000000010000000000000059 there, i've just intentionally provided a bad value 50.. as a typo and it causes the wrong message). Now it might not be an issue as with 0005 patch the same test behaves OK (`pg_waldump: error: could not find a valid record after 0/50000358`). It is just relevant if this would be committed not all at once. i. If I give wrong --timeline=999 to pg_waldump it fails with misleading error message: could not read WAL data from "pg_wal.tar" archive: read -1 of 8192 0005: a. I'm wondering if we shouldn't log (to stderr?) some kind of notification message (just once) that non-sequential WAL files were discovered and that pg_waldump is starting to write to $somewhere as it may be causing bigger I/O than anticipated when running the command. This can easily help when troubleshooting why it is not fast, and also having set TMPDIR to usually /tmp can be slow or too small. b. IMHO member_prepare_tmp_write() / get_tmp_wal_file_path() with TMPDIR can be prone to symlink attack. Consider setting TMPDIR=/tmp . We are writing to e.g. /tmp/<WALsegment>.waldump.tmp in 0004 , but that path is completely guessable. If an attacker prepares some symlinks and links those to some other places, I think the code will happily open and overwrite the contents of the rogue symlink. I think using mkstemp(3)/tmpfile(3) would be a safer choice if TMPDIR needs to be in play. Consider that pg_waldump can be run as root (there's no mechanism preventing it from being used that way). c. IMHO that unlink() might be not guaranteed to always remove files, as in case of any trouble and exit() , those files might be left over. I think we need some atexit() handlers. This can be triggered with combo of options of nonsequential files in tar + wrong LSN given: $ tar tf pg_wal.tar 00000001000000000000005A 00000001000000000000005B 00000001000000000000005C [..] 000000010000000000000060 000000010000000000000059 <-- out of order, appended last $ ls -lh 0* ls: cannot access '0*': No such file or directory $ /usr/pgsql19/bin/pg_waldump --path=/tmp/ble/pg_wal.tar --stats -s 0/10000358 #wrong LSN pg_waldump: error: could not find a valid record after 0/10000358 $ ls -lh 0* -rw------- 1 postgres postgres 16M Sep 8 14:44 000000010000000000000059.waldump.tmp -rw------- 1 postgres postgres 16M Sep 8 14:44 00000001000000000000005A.waldump.tmp [..] 0006: LGTM 0007: a. Commit message says `Future patches to pg_waldump will enable it to decode WAL directly` , but those pg_waldump are earlier patches, right? b. pg_verifybackup should print some info with --progress that it is spawning pg_waldump (pg_verifybackup --progress mode does not display anything related to verifing WALs, but it could) c. I'm wondering, but pg_waldump seems to be not complaining if --end=LSN is made into such a future that it doesn't exist. E.g. If the latest WAL segment is 60 (with end LSN 0/60A77A59), but I run pg_waldump `--end=0/7000000` , it will return code 0 and nothing on stderr. So how sure are we that the necessary WAL segments (as per backup_manifest) are actually inside the tar? It's supposed to be verified, but it isn't for this use case? Same happens if craft special tar and remove just one WAL segment from pg_wal.tar (simulate missing WAL segment), but ask the pg_verifybackup/pg_waldump to verify it to exact last LSN sequence, e.g.: $ /usr/pgsql19/bin/pg_waldump --quiet --path=/tmp/missing/pg_wal.tar --timeline=1 --start=0/59000028 --end=0/60A77A58 && echo OK # but it is not OK OK $ /usr/pgsql19/bin/pg_waldump --stats --path=/tmp/missing/pg_wal.tar --timeline=1 --start=0/59000028 --end=0/60A77A58 WAL statistics between 0/59000028 and 0/5CFFFFD0: # <-- 0/5C LSN maximum detected [..] Notice it has read till 0/5C (but I've asked till 0/60), because I've removed 0D: $ tar tf /tmp/missing/pg_wal.tar| grep ^0 000000010000000000000059 00000001000000000000005A 00000001000000000000005B 00000001000000000000005C 00000001000000000000005E <-- missing 5D Yet it reported no errors. 0008: LGTM Another open question I have is this: shouldn't backup_manifest come with CRC checksum for the archived WALs? Or does that guarantee that backup_manifest WAL-Ranges are present in pg_wal.tar is good enough because individual WAL files are CRC-protected itself? -J. -
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-09-12T10:55:28Z
On Mon, Sep 8, 2025 at 7:07 PM Jakub Wartak <jakub.wartak@enterprisedb.com> wrote: > > On Tue, Aug 26, 2025 at 1:53 PM Amul Sul <sulamul@gmail.com> wrote: > > > [..patch] > > Hi Amul! > Thanks for your review. I'm replying to a few of your comments now, but for the rest, I need to think about them. I'm kind of in agreement with some of them for the fix, but I won't be able to spend time on that next week due to official travel. I'll try to get back as soon as possible after that. > a. Why should it be necessary to provide startLSN (-s) ? Couldn't > it autodetect the first WAL (tar file) inside and just use that with > some info message? > $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar > pg_waldump: error: no start WAL location given > There are two reasons. First, existing pg_waldump --path=some_directory would result in the same error. Second, it would force us to re-read the archive twice just to locate the first WAL segment, which is inefficient. > c. It doesnt work when using SEGSTART, but it's there: > $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar > 000000010000000000000059 > pg_waldump: error: could not open file "000000010000000000000059": > Not a directory > $ tar tf /tmp/base/pg_wal.tar | head -1 > 000000010000000000000059 > I don't believe this is the correct use case. The WAL files are inside a tar archive, and the requirement is to use a starting LSN and a timeline (if not the default). > d. I've later noticed that follow-up patches seem to use the > -s switch and there it seems to work OK. The above SEGSTART issue was > not detected, probably because tests need to be extended cover of > segment name rather than just --start LSN (see test_pg_waldump): > $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar --stats > -s 0/59000358 > pg_waldump: first record is after 0/59000358, at 0/590003E8, > skipping over 144 bytes > WAL statistics between 0/590003E8 and 0/61000000: > [..] > Hope previous reasoning makes sense to you. > e. Code around`if (walpath == NULL && directory != NULL)` needs > some comments. > I think this is an existing one. > f. Code around `if (fname != NULL && is_tar_file(fname, > &compression))` , so if fname is WAL segment here > (00000001000000000000005A) and we do check again if that has been > tar-ed (is_tar_file())? Why? > Again, how? > g. Just a question: the commit message says `Note that this patch > requires that the WAL files within the archive be in sequential order; > an error will be reported otherwise`. I'm wondering if such > occurrences are known to be happening in the wild? Or is it just an > assumption that if someone would modify the tar somehow? (either way > we could just add a reason why we need to handle such a case if we > know -- is manual alternation the only source of such state?). For the > record, I've tested crafting custom archives with out of sequence WAL > archives and the code seems to work (it was done using: tar --append > -f pg_wal.tar --format=ustar ..) > This is an almost nonexistent occurrence. While pg_basebackup archives WAL files in sequential order, we don't have an explicit code to enforce that order within it. Furthermore, since we can't control how external tools might handle the files, this extra precaution is necessary. > Another open question I have is this: shouldn't backup_manifest come > with CRC checksum for the archived WALs? Or does that guarantee that > backup_manifest WAL-Ranges are present in pg_wal.tar is good enough > because individual WAL files are CRC-protected itself? > I don't know, I have to check pg_verifybackup. Regards, Amul
-
Re: pg_waldump: support decoding of WAL inside tarfile
Robert Haas <robertmhaas@gmail.com> — 2025-09-12T18:28:15Z
Here are some review comments on v3-0004: In general, I think this looks pretty nice, but I think it needs more cleanup and polishing. There doesn't seem to be any reason for astreamer_waldump_content_new() to take an astreamer *next argument. If you look at astreamer.h, you'll see that some astreamer_BLAH_new() functions take such an argument, and others don't. The ones that do forward their input to another astreamer; the ones that don't, like astreamer_plain_writer_new(), send it somewhere else. AFAICT, this astreamer is never going to send its output to another astreamer, so there's no reason for this argument. I'm also a little confused by the choice of the name astreamer_waldump_content_new(). I would have thought this would be something like astreamer_waldump_new() or astreamer_xlogreader_new(). The word "content" doesn't seem to me to be adding much here, and it invites confusion with the "content" callback. I think you can merge setup_astreamer() into init_tar_archive_reader(). The only other caller is verify_tar_archive(), but that does exactly the same additional steps as init_tar_archive_reader(), as far as I can see. The return statement for astreamer_wal_read is really odd: + return (count - nbytes) ? (count - nbytes) : -1; Since 0 is false in C, this is equivalent to: count != nbytes ? count - nbytes : -1, but it's a strange way to write it. What makes it even stranger is that it seems as though the intention here is to count the number of bytes read, but you do that by taking the number of bytes requested (count) and subtracting the number of bytes we didn't manage to read (nbytes); and then you just up and return -1 instead of 0 whenever the answer would have been zero. This is all lacking in comments and seems a bit more confusing than it needs to be. So my suggestions are: 1. Consider redefining nbytes to be the number of bytes that you have read instead of the number of bytes you haven't read. So the loop in this function would be while (nbytes < count) instead of while (nbytes > 0). 2. If you need to map 0 to -1, consider having the caller do this instead of putting that inside this function. 3. Add a comment saying what the return value is supposed to be". If you do both 1 and 2, then the return statement can just say "return nbytes;" and the comment can say "Returns the number of bytes successfully read." I would suggest changing the name of the variable from "readBuff" to "readBuf". There are no existing uses of readBuff in the code base. I think this comment also needs improvement: + /* + * Ignore existing data if the required target page has not yet been + * read. + */ + if (recptr >= endPtr) + { + len = 0; + + /* Reset the buffer */ + resetStringInfo(astreamer_buf); + } This comment is problematic for a few reasons. First, we're not ignoring the existing data: we're throwing it out. Second, the comment doesn't say why we're doing what we're doing, only that we're doing it. Here's my guess at the actual explanation -- please correct me if I'm wrong: "pg_waldump never reads the same WAL bytes more than once, so if we're now being asked for data beyond the end of what we've already read, that means none of the data we currently have in the buffer will ever be consulted again. So, we can discard the existing buffer contents and start over." By the way, if this explanation is correct, it might be nice to add an assertion someplace that verifies it, like asserting that we're always reading from an LSN greater than or equal to (or exactly equal to?) the LSN immediately following the last data we read. In general, I wonder whether there's a way to make the separation of concerns between astreamer_wal_read() and TarWALDumpReadPage() cleaner. Right now, the latter is basically a stub, but I'm not sure that is the best thing here. I already mentioned one example of how to do this: make the responsibility for 0 => -1 translation the job of TarWALDumpReadPage() rather than astreamer_wal_read(). But I think there might be a little more we can do. In particular, I wonder whether we could say that astreamer_wal_read() is only responsible for filling the buffer, and the caller, TarWALDumpReadPage() in this case, needs to empty it. That seems like it might produce a cleaner separation of duties. Another thing that isn't so nice right now is that verify_tar_archive() has to open and close the archive only for init_tar_archive_reader() to be called to reopen it again just moments later. It would be nicer to open the file just once and then keep it open. Here again, I wonder if the separation of duties could be a bit cleaner. Is there a real need to pass XLogDumpPrivate to astreamer_wal_read or astreamer_archive_read? The only things that they need are archive_fd, archive_name, archive_streamer, archive_streamer_buf, and archive_streamer_read_ptr. In other words, they really don't care about any of the *existing* things that are in XLogDumpPrivate. This makes me wonder whether we should actually try to make this new astreamer completely independent of xlogreader. In other words, instead of calling it astreamer_waldump() or astreamer_xlogreader() as I proposed above, maybe it could be a completely generic astreamer, say astreamer_stringinfo_new(StringInfo *buf) that just appends to the buffer. That would require also moving the stuff out of astreamer_wal_read() that knows about XLogRecPtr, but why does that function need to know about XLogRecPtr? Couldn't the caller figure out that part and just tell this function how many bytes are needed? -- Robert Haas EDB: http://www.enterprisedb.com -
Re: pg_waldump: support decoding of WAL inside tarfile
Robert Haas <robertmhaas@gmail.com> — 2025-09-12T20:27:07Z
On Fri, Sep 12, 2025 at 2:28 PM Robert Haas <robertmhaas@gmail.com> wrote: > Is there a real need to pass XLogDumpPrivate to astreamer_wal_read or > astreamer_archive_read? The only things that they need are archive_fd, > archive_name, archive_streamer, archive_streamer_buf, and > archive_streamer_read_ptr. In other words, they really don't care > about any of the *existing* things that are in XLogDumpPrivate. This > makes me wonder whether we should actually try to make this new > astreamer completely independent of xlogreader. In other words, > instead of calling it astreamer_waldump() or astreamer_xlogreader() as > I proposed above, maybe it could be a completely generic astreamer, > say astreamer_stringinfo_new(StringInfo *buf) that just appends to the > buffer. That would require also moving the stuff out of > astreamer_wal_read() that knows about XLogRecPtr, but why does that > function need to know about XLogRecPtr? Couldn't the caller figure out > that part and just tell this function how many bytes are needed? Hmm, on further thought, I think this was a silly idea. Part of the intended function of this astreamer is to make sure we're only reading WAL files from the archive, and eventually reordering them if required, so obviously something completely generic isn't going to work. Maybe there's a way to make this look a little cleaner and tidier but this isn't it... -- Robert Haas EDB: http://www.enterprisedb.com
-
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-09-25T08:18:13Z
On Fri, Sep 12, 2025 at 4:25 PM Amul Sul <sulamul@gmail.com> wrote: > > On Mon, Sep 8, 2025 at 7:07 PM Jakub Wartak > <jakub.wartak@enterprisedb.com> wrote: > > > > On Tue, Aug 26, 2025 at 1:53 PM Amul Sul <sulamul@gmail.com> wrote: > > > > > [..patch] > > > > Hi Amul! > > > > Thanks for your review. I'm replying to a few of your comments now, > but for the rest, I need to think about them. I'm kind of in agreement > with some of them for the fix, but I won't be able to spend time on > that next week due to official travel. I'll try to get back as soon as > possible after that. > Reverting on rest of review comments: > 0001: LGTM, maybe I would just slightly enhance the commit message > ("This is in preparation for adding a second source file to this > directory.") -- maye bit a bit more verbose or use a message from > 0002? Done. > b. Why would it like to open "blah" dir if I wanted that "blah" > segment from the archive? Shouldn't it tell that it was looking in the > archive and couldn find it inside? > $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar blah > pg_waldump: error: could not open file "blah": Not a directory Now, an error will be thrown if any additional command-line arguments are provided when an archive is specified, similar to how existing extra arguments are handled. > i. If I give wrong --timeline=999 to pg_waldump it fails with > misleading error message: could not read WAL data from "pg_wal.tar" > archive: read -1 of 8192 Now., added a much better error message for that case. > a. I'm wondering if we shouldn't log (to stderr?) some kind of > notification message (just once) that non-sequential WAL files were > discovered and that pg_waldump is starting to write to $somewhere as > it may be causing bigger I/O than anticipated when running the > command. This can easily help when troubleshooting why it is not fast, > and also having set TMPDIR to usually /tmp can be slow or too small. Now, emitting info messages, but I'm not sure whether we should have info or debug. > b. IMHO member_prepare_tmp_write() / get_tmp_wal_file_path() with > TMPDIR can be prone to symlink attack. Consider setting TMPDIR=/tmp . > We are writing to e.g. /tmp/<WALsegment>.waldump.tmp in 0004 , but > that path is completely guessable. If an attacker prepares some > symlinks and links those to some other places, I think the code will > happily open and overwrite the contents of the rogue symlink. I think > using mkstemp(3)/tmpfile(3) would be a safer choice if TMPDIR needs to > be in play. Consider that pg_waldump can be run as root (there's no > mechanism preventing it from being used that way). I am not sure what the worst-case scenario would be or what a good alternative is. > c. IMHO that unlink() might be not guaranteed to always remove > files, as in case of any trouble and exit() , those files might be > left over. I think we need some atexit() handlers. This can be > triggered with combo of options of nonsequential files in tar + wrong > LSN given: Done. > 0007: > a. Commit message says `Future patches to pg_waldump will enable > it to decode WAL directly` , but those pg_waldump are earlier patches, > right? Right, fixed. > b. pg_verifybackup should print some info with --progress that it > is spawning pg_waldump (pg_verifybackup --progress mode does not > display anything related to verifing WALs, but it could) If we decide to do that, it could be a separate project, IMHO. > c. I'm wondering, but pg_waldump seems to be not complaining if > --end=LSN is made into such a future that it doesn't exist. The behavior will be kept as if a directory was provided with a start and end LSN. Thanks again for the review. I'll post the new patches in my next reply. Regards, Amul -
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-09-25T08:24:51Z
On Fri, Sep 12, 2025 at 11:58 PM Robert Haas <robertmhaas@gmail.com> wrote: > > Here are some review comments on v3-0004: > Thanks for the review. My replies are below. > There doesn't seem to be any reason for > astreamer_waldump_content_new() to take an astreamer *next argument. > If you look at astreamer.h, you'll see that some astreamer_BLAH_new() > functions take such an argument, and others don't. The ones that do > forward their input to another astreamer; the ones that don't, like > astreamer_plain_writer_new(), send it somewhere else. AFAICT, this > astreamer is never going to send its output to another astreamer, so > there's no reason for this argument. > Done. > I'm also a little confused by the choice of the name > astreamer_waldump_content_new(). I would have thought this would be > something like astreamer_waldump_new() or astreamer_xlogreader_new(). > The word "content" doesn't seem to me to be adding much here, and it > invites confusion with the "content" callback. > Done -- renamed to astreamer_waldump_new(). > I think you can merge setup_astreamer() into > init_tar_archive_reader(). The only other caller is > verify_tar_archive(), but that does exactly the same additional steps > as init_tar_archive_reader(), as far as I can see. > Done. > The return statement for astreamer_wal_read is really odd: > > + return (count - nbytes) ? (count - nbytes) : -1; > Agreed, that's a bit odd. This seems to be leftover code from the experimental patch. The astreamer_wal_read() function should behave like WALRead(): it should either successfully read all the requested bytes or throw an error. Corrected in the attached version. > > I would suggest changing the name of the variable from "readBuff" to > "readBuf". There are no existing uses of readBuff in the code base. > The existing WALDumpReadPage() function has a "readBuff" argument, and I've used it that way for consistency. > I think this comment also needs improvement: > > + /* > + * Ignore existing data if the required target page > has not yet been > + * read. > + */ > + if (recptr >= endPtr) > + { > + len = 0; > + > + /* Reset the buffer */ > + resetStringInfo(astreamer_buf); > + } > > This comment is problematic for a few reasons. First, we're not > ignoring the existing data: we're throwing it out. Second, the comment > doesn't say why we're doing what we're doing, only that we're doing > it. Here's my guess at the actual explanation -- please correct me if > I'm wrong: "pg_waldump never reads the same WAL bytes more than once, > so if we're now being asked for data beyond the end of what we've > already read, that means none of the data we currently have in the > buffer will ever be consulted again. So, we can discard the existing > buffer contents and start over." By the way, if this explanation is > correct, it might be nice to add an assertion someplace that verifies > it, like asserting that we're always reading from an LSN greater than > or equal to (or exactly equal to?) the LSN immediately following the > last data we read. > Updated the comment. The similar assertion exists right before copying to the readBuff. > > Another thing that isn't so nice right now is that > verify_tar_archive() has to open and close the archive only for > init_tar_archive_reader() to be called to reopen it again just moments > later. It would be nicer to open the file just once and then keep it > open. Here again, I wonder if the separation of duties could be a bit > cleaner. > Prefer to keep those separate, assuming that reopening the file won't cause any significant harm. Let me know if you think otherwise. Attached the updated version, kindly have a look. Regards, Amul -
Re: pg_waldump: support decoding of WAL inside tarfile
Robert Haas <robertmhaas@gmail.com> — 2025-09-29T15:15:31Z
On Thu, Sep 25, 2025 at 4:25 AM Amul Sul <sulamul@gmail.com> wrote: > > Another thing that isn't so nice right now is that > > verify_tar_archive() has to open and close the archive only for > > init_tar_archive_reader() to be called to reopen it again just moments > > later. It would be nicer to open the file just once and then keep it > > open. Here again, I wonder if the separation of duties could be a bit > > cleaner. > > Prefer to keep those separate, assuming that reopening the file won't > cause any significant harm. Let me know if you think otherwise. Well, I guess I'd like to know why we can't do better. I'm not really worried about performance, but reopening the file means that you can never make it work with reading from a pipe. -- Robert Haas EDB: http://www.enterprisedb.com
-
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-09-29T16:17:10Z
On Mon, Sep 29, 2025 at 8:45 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Thu, Sep 25, 2025 at 4:25 AM Amul Sul <sulamul@gmail.com> wrote: > > > Another thing that isn't so nice right now is that > > > verify_tar_archive() has to open and close the archive only for > > > init_tar_archive_reader() to be called to reopen it again just moments > > > later. It would be nicer to open the file just once and then keep it > > > open. Here again, I wonder if the separation of duties could be a bit > > > cleaner. > > > > Prefer to keep those separate, assuming that reopening the file won't > > cause any significant harm. Let me know if you think otherwise. > > Well, I guess I'd like to know why we can't do better. I'm not really > worried about performance, but reopening the file means that you can > never make it work with reading from a pipe. I have some skepticism regarding the extra coding that might be introduced, as performance is not my primary concern here. If we aim to keep the file open only once, that logic should be implemented before calling verify_tar_archive(), not inside it. Implementing the open and close logic within verify_tar_archive() and free_tar_archive_reader() would create a confusing and scattered pattern, especially since these separate operations require only two lines of code each (open and close if it's a tar file). My second, concern is that after verify_tar_archive(), we might need to reset the file reader offset to the beginning. While reusing the buffered data from the first iteration is technically possible, that only works if the desired start LSN is at the absolute beginning of the archive, or later in the sequence, which cannot be reliably guaranteed. Therefore, for simplicity and avoid the complexity of managing that offset reset code, I am thinking of a simpler approach. Regards, Amul
-
Re: pg_waldump: support decoding of WAL inside tarfile
Robert Haas <robertmhaas@gmail.com> — 2025-10-10T18:01:59Z
On Mon, Sep 29, 2025 at 12:17 PM Amul Sul <sulamul@gmail.com> wrote: > While reusing the buffered data > from the first iteration is technically possible, that only works if > the desired start LSN is at the absolute beginning of the archive, or > later in the sequence, which cannot be reliably guaranteed. I spent a bunch of time studying this code today and I think that the problem you're talking about here is evidence of a design problem with astreamer_wal_read() and some of the other code in astreamer_waldump.c. Your code calls astreamer_wal_read() when it wants to peek at the first xlog block to determine the WAL segment size, and it also calls astreamer_wal_read() when it wants read WAL sequentially beginning at the start LSN and continuing until it reaches the end LSN. However, these two cases have very different requirements. verify_tar_archive(), which is misleadingly named and really exists to determine the WAL segment size, just wants to read the first xlog block that physically appears in the archive. Every xlog block will have the same WAL segment size, so it does not matter which one we read. On the other hand, TarWALDumpReadPage wants to read WAL in sequential order. In other words, one call to astreamer_wal_read() really wants to read a block without any block reordering, and the other call wants to read a block with block reordering. To me, it looks like the problem here is that the block reordering functionality should live on top of the astreamer, not inside of it. Imagine that astreamer just spits out the bytes in the order in which they physically appear in the archive, and then there's another component that consumes and reorders those bytes. So, you read data and push it into the astreamer until the number of bytes in the output buffer is at least XLOG_BLCKSZ, and then from there you extract the WAL segment size. Then, you call XLogReaderAllocate() and enter the main loop. The reordering logic lives inside of TarWALDumpReadPage(). Each time it gets data from the astreamer's buffer, it either returns it to the caller if it's in order or buffers it using temporary files if not. I found it's actually quite easy to write a patch that avoids reopening the file. Here it is, on top of your v4: diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c index 2c42df46d43..c4346a5e211 100644 --- a/src/bin/pg_waldump/pg_waldump.c +++ b/src/bin/pg_waldump/pg_waldump.c @@ -368,17 +368,8 @@ init_tar_archive_reader(XLogDumpPrivate *private, const char *waldir, XLogRecPtr startptr, XLogRecPtr endptr, pg_compress_algorithm compression) { - int fd; astreamer *streamer; - /* Open tar archive and store its file descriptor */ - fd = open_file_in_directory(waldir, private->archive_name); - - if (fd < 0) - pg_fatal("could not open file \"%s\"", private->archive_name); - - private->archive_fd = fd; - /* * Create an appropriate chain of archive streamers for reading the given * tar archive. @@ -1416,12 +1407,22 @@ main(int argc, char **argv) /* we have everything we need, start reading */ if (is_tar) { + /* Open tar archive and store its file descriptor */ + private.archive_fd = + open_file_in_directory(waldir, private.archive_name); + if (private.archive_fd < 0) + pg_fatal("could not open file \"%s\"", private.archive_name); + /* Verify that the archive contains valid WAL files */ waldir = waldir ? pg_strdup(waldir) : pg_strdup("."); init_tar_archive_reader(&private, waldir, InvalidXLogRecPtr, InvalidXLogRecPtr, compression); verify_tar_archive(&private); - free_tar_archive_reader(&private); + astreamer_free(private.archive_streamer); + + if (lseek(private.archive_fd, 0, SEEK_SET) != 0) + pg_log_error("could not seek in file \"%s\": %m", + private.archive_name); /* Set up for reading tar file */ init_tar_archive_reader(&private, waldir, private.startptr, Of course, this is not really what we want to do: it avoids reopening the file, but because we can't back up the archive streamer once it's been created, we have to lseek back to the beginning of the file. But notice how silly this looks: with this patch, we free the archive reader and immediately create a new archive reader that is exactly the same in every way except that we call astreamer_waldump_new(startptr, endptr, private) instead of astreamer_waldump_new(InvalidXLogRecPtr, InvalidXLogRecPtr, private). We could arrange to update the original archive streamer with new values of startSegNo and endSegNo after verify_tar_archive(), but that's still not quite good enough, because we might have already made some decisions on what to do with the data that we read that it's too late to reverse. But, what that means is that the astreamer_waldump machinery is not smart enough to read one block of data without making irreversible decisions from which we can't recover without recreating the entire object. I think we can, and should, try to do better. It's also worth noting that the unfortunate layering doesn't just require us to read the first block of the file: it also complicates the code in various places. The fact that astreamer_wal_read() needs a special case for XLogRecPtrIsInvalid(recptr) is a direct result of this problem, and the READ_ANY_WAL() macro and both the places that test it are also direct results of this problem. In other words, I'm arguing that astreamer_wal_read() is incorrectly defined, and that error creates ugliness in the code both above and below astreamer_wal_read(). While I'm on the topic of astreamer_wal_read(), here are a few other problems I noticed: * The return value is not documented, and it seems to always be count, in which case it might as well return void. The caller already has the value they passed for count. * It seems like it would be more appropriate to assert that endPtr >= len and just set startPtr = endPtr - len. I don't see how len > endPtr can ever happen, and I bet bad things will happen if it does. * "pg_waldump never ask the same" -> "pg_waldump never asks for the same" Also, this is absolutely not OK with me: /* Fetch more data */ if (astreamer_archive_read(privateInfo) == 0) { char fname[MAXFNAMELEN]; XLogSegNo segno; XLByteToSeg(targetPagePtr, segno, WalSegSz); an XLogFileName(fname, privateInfo->timeline, segno, WalSegSz); pg_fatal("could not find file \"%s\" in \"%s\" archive", fname, privateInfo->archive_name); } astreamer_archive_read() will return 0 if we reach the end of the tarfile, so this is saying that if we reach the end of the tar file without finding the range of bytes for which we're looking, the explanation must be that the relevant WAL file is missing from the archive. But that is way too much action at a distance. I was able to easily construct a counterexample by copying the first 81920 bytes of a valid WAL file and then doing this: [robert.haas pgsql-meson]$ tar tf pg_wal.tar 000000010000000000000005 [robert.haas pgsql-meson]$ pg_waldump -s 0/050008D8 -e 0/05FFED98 pg_wal.tar >/dev/null pg_waldump: error: could not find file "000000010000000000000005" in "pg_wal.tar" archive Without the redirection to /dev/null, what happened was that pg_waldump printed out a bunch of records from 000000010000000000000005 and then said that 000000010000000000000005 could not be found, which is obviously silly. But the fact that I found a specific counterexample here isn't even really the point. The point is that there's a big gap between what we actually know at this point (which is that we've read the whole input file) and what the message is claiming (which is that the reason must be that the file is missing from the archive). Even if the counterexample above didn't exist and that really were the only way for that to happen as of today, that's very fragile. Maybe some future code change will make it so that there's a second reason that could happen. How would somebody realize that they had created a second condition by means of which this code could be reached? If they did realize it, how would they get the correct error to be reported? I'm not quite sure how this should be fixed, but I strongly suspect that the error report here needs to move closer to the code that is doing the file reordering. Aside from the possibility of the file being missing and the possibility of the file being too short, a third possibility is that targetPagePtr retreats between one call and the next. That really shouldn't happen, but there are no asserts here verifying that it doesn't. I also don't like the fact that one call to astreamer_archive_read() checks the return value (but only whether it's zero, the specific return value apparently doesn't matter, so why doesn't it return bool?) and the other doesn't. That kind of coding pattern is very rarely correct. The code says: /* Continue reading from the open WAL segment, if any */ if (state->seg.ws_file >= 0) { /* * To prevent a race condition where the archive streamer is still * exporting a file that we are trying to read, we invoke the streamer * to ensure enough data is available. */ if (private->curSegNo == state->seg.ws_segno) astreamer_archive_read(private); return WALDumpReadPage(state, targetPagePtr, reqLen, targetPtr, readBuff); } But it's unclear why this should be good enough to ensure that enough data is available. astreamer_archive_read() might read zero bytes and return 0, so this doesn't really guarantee anything at all. On the other hand, even if astereamer_archive_read() returns a non-zero value, it's only going to read READ_CHUNK_SIZE bytes from the underlying file, so if more than that needs to be read in order for us to have enough data, we won't. I think it's very hard to imagine a situation in which you can call astreamer_archive_read() without using some loop. That's what astreamer_wal_read() does: it calls astreamer_archive_read() until it either returns 0 -- in which case we know we've failed -- or until we have enough data. Here we just hope that calling it once is enough, and that checking for errors is unimportant. I also don't understand the reference to a race condition, because there's only one process with one thread here, I believe, so what would be racing against? Another thing I noticed is that astreamer_archive_read() makes reference to decrypting, but there's no cryptography involved in any of this. -- Robert Haas EDB: http://www.enterprisedb.com -
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-10-16T11:48:31Z
On Fri, Oct 10, 2025 at 11:32 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Mon, Sep 29, 2025 at 12:17 PM Amul Sul <sulamul@gmail.com> wrote: > > While reusing the buffered data > > from the first iteration is technically possible, that only works if > > the desired start LSN is at the absolute beginning of the archive, or > > later in the sequence, which cannot be reliably guaranteed. > > I spent a bunch of time studying this code today and I think that the > problem you're talking about here is evidence of a design problem with > astreamer_wal_read() and some of the other code in > astreamer_waldump.c. Your code calls astreamer_wal_read() when it > wants to peek at the first xlog block to determine the WAL segment > size, and it also calls astreamer_wal_read() when it wants read WAL > sequentially beginning at the start LSN and continuing until it > reaches the end LSN. However, these two cases have very different > requirements. verify_tar_archive(), which is misleadingly named and > really exists to determine the WAL segment size, just wants to read > the first xlog block that physically appears in the archive. Every > xlog block will have the same WAL segment size, so it does not matter > which one we read. On the other hand, TarWALDumpReadPage wants to read > WAL in sequential order. In other words, one call to > astreamer_wal_read() really wants to read a block without any block > reordering, and the other call wants to read a block with block > reordering. > > To me, it looks like the problem here is that the block reordering > functionality should live on top of the astreamer, not inside of it. > Imagine that astreamer just spits out the bytes in the order in which > they physically appear in the archive, and then there's another > component that consumes and reorders those bytes. So, you read data > and push it into the astreamer until the number of bytes in the output > buffer is at least XLOG_BLCKSZ, and then from there you extract the > WAL segment size. Then, you call XLogReaderAllocate() and enter the > main loop. The reordering logic lives inside of TarWALDumpReadPage(). > Each time it gets data from the astreamer's buffer, it either returns > it to the caller if it's in order or buffers it using temporary files > if not. > I initially considered implementing the reordering logic outside of astreamer when we first discussed this project, but the implementation could get complicated -- or at least feel hacky. Let me explain why: astreamer reads the archive in fixed-size chunks (here it is 128KB). Sometimes, a single read can contain data from two WAL files -- specifically, the tail end of one file and the start of the next -- because of how they’re physically stored in the archive. astreamer knows where one file ends and another begins through tags like ASTREAMER_MEMBER_HEADER, ASTREAMER_MEMBER_CONTENTS, and ASTREAMER_MEMBER_TRAILER. However, it can’t pause mid-chunk to hold data from the next file once the previous one ends and for the caller; it pushes the entire chunk it has read to the target buffer. So, if we put the reordering logic outside the streamer, we’d sometimes be receiving buffers containing mixed data from two WAL files. The caller would then need to correctly identify WAL file boundaries within those buffers. This would require passing extra metadata -- like segment numbers for the WAL files in the buffer, plus start and end offsets of those segments within the buffer. While not impossible, it feels a bit hacky and I'm unsure if that’s the best approach. > I found it's actually quite easy to write a patch that avoids > reopening the file. Here it is, on top of your v4: > > diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c > index 2c42df46d43..c4346a5e211 100644 > --- a/src/bin/pg_waldump/pg_waldump.c > +++ b/src/bin/pg_waldump/pg_waldump.c > @@ -368,17 +368,8 @@ init_tar_archive_reader(XLogDumpPrivate *private, > const char *waldir, > XLogRecPtr startptr, XLogRecPtr endptr, > pg_compress_algorithm compression) > { > - int fd; > astreamer *streamer; > > - /* Open tar archive and store its file descriptor */ > - fd = open_file_in_directory(waldir, private->archive_name); > - > - if (fd < 0) > - pg_fatal("could not open file \"%s\"", private->archive_name); > - > - private->archive_fd = fd; > - > /* > * Create an appropriate chain of archive streamers for reading the given > * tar archive. > @@ -1416,12 +1407,22 @@ main(int argc, char **argv) > /* we have everything we need, start reading */ > if (is_tar) > { > + /* Open tar archive and store its file descriptor */ > + private.archive_fd = > + open_file_in_directory(waldir, private.archive_name); > + if (private.archive_fd < 0) > + pg_fatal("could not open file \"%s\"", private.archive_name); > + > /* Verify that the archive contains valid WAL files */ > waldir = waldir ? pg_strdup(waldir) : pg_strdup("."); > init_tar_archive_reader(&private, waldir, InvalidXLogRecPtr, > InvalidXLogRecPtr, compression); > verify_tar_archive(&private); > - free_tar_archive_reader(&private); > + astreamer_free(private.archive_streamer); > + > + if (lseek(private.archive_fd, 0, SEEK_SET) != 0) > + pg_log_error("could not seek in file \"%s\": %m", > + private.archive_name); > > /* Set up for reading tar file */ > init_tar_archive_reader(&private, waldir, private.startptr, > > Of course, this is not really what we want to do: it avoids reopening > the file, but because we can't back up the archive streamer once it's > been created, we have to lseek back to the beginning of the file. But > notice how silly this looks: with this patch, we free the archive > reader and immediately create a new archive reader that is exactly the > same in every way except that we call astreamer_waldump_new(startptr, > endptr, private) instead of astreamer_waldump_new(InvalidXLogRecPtr, > InvalidXLogRecPtr, private). We could arrange to update the original > archive streamer with new values of startSegNo and endSegNo after > verify_tar_archive(), but that's still not quite good enough, because > we might have already made some decisions on what to do with the data > that we read that it's too late to reverse. But, what that means is > that the astreamer_waldump machinery is not smart enough to read one > block of data without making irreversible decisions from which we > can't recover without recreating the entire object. I think we can, > and should, try to do better. > Agreed. > It's also worth noting that the unfortunate layering doesn't just > require us to read the first block of the file: it also complicates > the code in various places. The fact that astreamer_wal_read() needs a > special case for XLogRecPtrIsInvalid(recptr) is a direct result of > this problem, and the READ_ANY_WAL() macro and both the places that > test it are also direct results of this problem. In other words, I'm > arguing that astreamer_wal_read() is incorrectly defined, and that > error creates ugliness in the code both above and below > astreamer_wal_read(). > > While I'm on the topic of astreamer_wal_read(), here are a few other > problems I noticed: > > * The return value is not documented, and it seems to always be count, > in which case it might as well return void. The caller already has the > value they passed for count. The caller will be xlogreader, and I believe we shouldn't change that. For the same reason, WALDumpReadPage() also returns the same. > * It seems like it would be more appropriate to assert that endPtr >= > len and just set startPtr = endPtr - len. I don't see how len > endPtr > can ever happen, and I bet bad things will happen if it does. > * "pg_waldump never ask the same" -> "pg_waldump never asks for the same" > Ok. > Also, this is absolutely not OK with me: > > /* Fetch more data */ > if (astreamer_archive_read(privateInfo) == 0) > { > char fname[MAXFNAMELEN]; > XLogSegNo segno; > > XLByteToSeg(targetPagePtr, segno, WalSegSz); an > XLogFileName(fname, > privateInfo->timeline, segno, WalSegSz); > > pg_fatal("could not find file \"%s\" > in \"%s\" archive", > fname, > privateInfo->archive_name); > } > > astreamer_archive_read() will return 0 if we reach the end of the > tarfile, so this is saying that if we reach the end of the tar file > without finding the range of bytes for which we're looking, the > explanation must be that the relevant WAL file is missing from the > archive. But that is way too much action at a distance. I was able to > easily construct a counterexample by copying the first 81920 bytes of > a valid WAL file and then doing this: > > [robert.haas pgsql-meson]$ tar tf pg_wal.tar > 000000010000000000000005 > [robert.haas pgsql-meson]$ pg_waldump -s 0/050008D8 -e 0/05FFED98 > pg_wal.tar >/dev/null > pg_waldump: error: could not find file "000000010000000000000005" in > "pg_wal.tar" archive > > Without the redirection to /dev/null, what happened was that > pg_waldump printed out a bunch of records from > 000000010000000000000005 and then said that 000000010000000000000005 > could not be found, which is obviously silly. But the fact that I > found a specific counterexample here isn't even really the point. The > point is that there's a big gap between what we actually know at this > point (which is that we've read the whole input file) and what the > message is claiming (which is that the reason must be that the file is > missing from the archive). Even if the counterexample above didn't > exist and that really were the only way for that to happen as of > today, that's very fragile. Maybe some future code change will make it > so that there's a second reason that could happen. How would somebody > realize that they had created a second condition by means of which > this code could be reached? If they did realize it, how would they get > the correct error to be reported? > Agreed, I'll think about this. > > /* Continue reading from the open WAL segment, if any */ > if (state->seg.ws_file >= 0) > { > /* > * To prevent a race condition where the archive streamer is still > * exporting a file that we are trying to read, we invoke the streamer > * to ensure enough data is available. > */ > if (private->curSegNo == state->seg.ws_segno) > astreamer_archive_read(private); > > return WALDumpReadPage(state, targetPagePtr, reqLen, targetPtr, > readBuff); > } > > But it's unclear why this should be good enough to ensure that enough > data is available. astreamer_archive_read() might read zero bytes and > return 0, so this doesn't really guarantee anything at all. On the > other hand, even if astereamer_archive_read() returns a non-zero > value, it's only going to read READ_CHUNK_SIZE bytes from the > underlying file, so if more than that needs to be read in order for us > to have enough data, we won't. I think it's very hard to imagine a > situation in which you can call astreamer_archive_read() without using > some loop. The loop isn't needed because the caller always requests 8KB of data, while READ_CHUNK_SIZE is 128KB. It’s assumed that the astreamer has already created the file with some initial data. For example, if only a few bytes have been written so far, when we reach TarWALDumpReadPage(), it detects that we’re reading the same file that the astreamer is still writing to and hasn’t finished. It then request to appends 128KB of data by calling astreamer_archive_read, even though we only need 8KB at a time. This process repeats each time the next 8KBchunk is requested: astreamer_archive_read() appends another 128KB,and continues until the file has been fully read and written. > That's what astreamer_wal_read() does: it calls > astreamer_archive_read() until it either returns 0 -- in which case we > know we've failed -- or until we have enough data. Here we just hope > that calling it once is enough, and that checking for errors is > unimportant. I also don't understand the reference to a race > condition, because there's only one process with one thread here, I > believe, so what would be racing against? > In the case where the astreamer is exporting a file to disk but hasn’t finished writing it, and we call TarWALDumpReadPage() to request block(s) from that WAL file, we can read only up to the existing blocks in the file. Since the file is incomplete, reading may fail later. To handle this, astreamer_archive_read() is invoked to append more data -- usually more than the requested amount, as explained earlier. That is the race condition I am trying to handle. Now, regarding the concern of astreamer_archive_read() returning zero without reading or appending any data: this can happen only if the WAL is shorter than expected -- an incomplete. In that case, WALDumpReadPage() will raise the appropriate error, we don't have to check at that point, I think. > Another thing I noticed is that astreamer_archive_read() makes > reference to decrypting, but there's no cryptography involved in any > of this. > I think that was a typo -- I meant decompression. Regards, Amul -
Re: pg_waldump: support decoding of WAL inside tarfile
Robert Haas <robertmhaas@gmail.com> — 2025-10-20T14:34:50Z
On Thu, Oct 16, 2025 at 7:49 AM Amul Sul <sulamul@gmail.com> wrote: > astreamer reads the archive in fixed-size chunks (here it is 128KB). > Sometimes, a single read can contain data from two WAL files -- > specifically, the tail end of one file and the start of the next -- > because of how they’re physically stored in the archive. astreamer > knows where one file ends and another begins through tags like > ASTREAMER_MEMBER_HEADER, ASTREAMER_MEMBER_CONTENTS, and > ASTREAMER_MEMBER_TRAILER. However, it can’t pause mid-chunk to hold > data from the next file once the previous one ends and for the caller; > it pushes the entire chunk it has read to the target buffer. Right, this makes sense. > So, if we put the reordering logic outside the streamer, we’d > sometimes be receiving buffers containing mixed data from two WAL > files. The caller would then need to correctly identify WAL file > boundaries within those buffers. This would require passing extra > metadata -- like segment numbers for the WAL files in the buffer, plus > start and end offsets of those segments within the buffer. While not > impossible, it feels a bit hacky and I'm unsure if that’s the best > approach. I agree that we need that kind of metadata, but I don't see why our need for it depends on where we do the reordering. That is, if we do the reordering above the astreamer layer, we need to keep track of the origin of each chunk of WAL bytes, and if we do the reordering within the astreamer layer, we still need to keep track of the origin of the WAL bytes. Doing the ordering properly requires that tracking, but it doesn't say anything about where that tracking has to be performed. I think it might be better if we didn't write to the astreamer's buffer at all. For example, suppose we create a struct that looks approximately like this: struct ChunkOfDecodedWAL { XLogSegNo segno; // could also be XLogRecPtr start_lsn or char *walfilename or whatever StringInfoData buffer; char *spillfilename; // or whatever we use to identify the temporary files bool already_removed; // potentially other metadata }; Then, create a hash table and key it on the segno whatever. Have the astreamer write to the hash table: when it gets a chunk of WAL, it looks up or creates the relevant hash table entry and appends the data to the buffer. At any convenient point in the code, you can decide to write the data from the buffer to a spill file, after which you resetStringInfo() on the buffer and populate the spill file name. When you've used up the data, you remove the spill file and set the already_removed flag. I think this could also help with the error reporting stuff. When you get to the end of the file, you'll know all the files you saw and how much data you read from each of them. So you could possibly do something like ERROR: LSN %08X/%08X not found in archive "\%s\" DETAIL: WAL segment %s is not present in the archive -or DETAIL: WAL segment %s was expected to be %u bytes, but was only %u bytes -or- DETAIL: whatever else can go wrong The point is that every file you've ever seen has a hash table entry, and in that hash table entry you can store everything about that file that you need to know, whether that's the file data, the disk file that contains the file data, the fact that we already threw the data away, or any other fact that you can imagine wanting to know. Said differently, the astreamer buffer is not really a great place to write data. It exists because when we're just forwarding data from one astreamer to the next, we will often need to buffer a small amount of data to avoid terrible performance. However, it's only there to be used when we don't have something better. I don't think any astreamer that is intended to be the last one in the chain currently writes to the buffer -- they write to the output file, or whatever, because using an in-memory buffer as your final output destination is not a real good plan. > > While I'm on the topic of astreamer_wal_read(), here are a few other > > problems I noticed: > > > > * The return value is not documented, and it seems to always be count, > > in which case it might as well return void. The caller already has the > > value they passed for count. > > The caller will be xlogreader, and I believe we shouldn't change that. > For the same reason, WALDumpReadPage() also returns the same. OK, but then you can make that clear via a brief comment. > The loop isn't needed because the caller always requests 8KB of data, > while READ_CHUNK_SIZE is 128KB. It’s assumed that the astreamer has > already created the file with some initial data. For example, if only > a few bytes have been written so far, when we reach > TarWALDumpReadPage(), it detects that we’re reading the same file > that the astreamer is still writing to and hasn’t finished. It then request to > appends 128KB of data by calling astreamer_archive_read, even though we > only need 8KB at a time. This process repeats each time the next 8KBchunk is > requested: astreamer_archive_read() appends another 128KB,and continues until > the file has been fully read and written. Sure, but you don't know how much data is going to come out the other end of the astreamer pipeline. Since the data is (possibly) compressed, you expect at least as many bytes to emerge from the output end as you add to the input end, but it's not a good idea to rely on assumptions like that. Sometimes compressors end up making the data slightly larger instead of smaller. It's unlikely that the effect would be so dramatic that adding 128kB to one end of the pipeline would make less than 8kB emerge from the other end, but it's not a good idea to rely on assumptions like that. Not that this is a real thing, but imagine that the compressed file had something in the middle of it that behaved like a comment in C code, i.e. it didn't generate any output. > In the case where the astreamer is exporting a file to disk but hasn’t > finished writing it, and we call TarWALDumpReadPage() to request > block(s) from that WAL file, we can read only up to the existing > blocks in the file. Since the file is incomplete, reading may fail > later. To handle this, astreamer_archive_read() is invoked to append > more data -- usually more than the requested amount, as explained > earlier. That is the race condition I am trying to handle. That's not what a race condition is: https://en.wikipedia.org/wiki/Race_condition > Now, regarding the concern of astreamer_archive_read() returning zero > without reading or appending any data: this can happen only if the WAL > is shorter than expected -- an incomplete. In that case, > WALDumpReadPage() will raise the appropriate error, we don't have to > check at that point, I think. I'm not going to accept that kind of justification -- it is too fragile to assume that you don't need to check for an error because it "can't happen". Sometimes that is reasonable, but there is quite a lot of action-at-a-distance here, so it does not feel safe. > > Another thing I noticed is that astreamer_archive_read() makes > > reference to decrypting, but there's no cryptography involved in any > > of this. > > I think that was a typo -- I meant decompression. I figured as much, but it still needs fixing. -- Robert Haas EDB: http://www.enterprisedb.com -
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-11-06T09:03:05Z
On Mon, Oct 20, 2025 at 8:05 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Thu, Oct 16, 2025 at 7:49 AM Amul Sul <sulamul@gmail.com> wrote: > > > So, if we put the reordering logic outside the streamer, we’d > > sometimes be receiving buffers containing mixed data from two WAL > > files. The caller would then need to correctly identify WAL file > > boundaries within those buffers. This would require passing extra > > metadata -- like segment numbers for the WAL files in the buffer, plus > > start and end offsets of those segments within the buffer. While not > > impossible, it feels a bit hacky and I'm unsure if that’s the best > > approach. > > I agree that we need that kind of metadata, but I don't see why our > need for it depends on where we do the reordering. That is, if we do > the reordering above the astreamer layer, we need to keep track of the > origin of each chunk of WAL bytes, and if we do the reordering within > the astreamer layer, we still need to keep track of the origin of the > WAL bytes. Doing the ordering properly requires that tracking, but it > doesn't say anything about where that tracking has to be performed. > > I think it might be better if we didn't write to the astreamer's > buffer at all. For example, suppose we create a struct that looks > approximately like this: > > struct ChunkOfDecodedWAL > { > XLogSegNo segno; // could also be XLogRecPtr start_lsn or char > *walfilename or whatever > StringInfoData buffer; > char *spillfilename; // or whatever we use to identify the temporary files > bool already_removed; > // potentially other metadata > }; > > Then, create a hash table and key it on the segno whatever. Have the > astreamer write to the hash table: when it gets a chunk of WAL, it > looks up or creates the relevant hash table entry and appends the data > to the buffer. At any convenient point in the code, you can decide to > write the data from the buffer to a spill file, after which you > resetStringInfo() on the buffer and populate the spill file name. When > you've used up the data, you remove the spill file and set the > already_removed flag. > > I think this could also help with the error reporting stuff. When you > get to the end of the file, you'll know all the files you saw and how > much data you read from each of them. So you could possibly do > something like > > ERROR: LSN %08X/%08X not found in archive "\%s\" > DETAIL: WAL segment %s is not present in the archive > -or > DETAIL: WAL segment %s was expected to be %u bytes, but was only %u bytes > -or- > DETAIL: whatever else can go wrong > > The point is that every file you've ever seen has a hash table entry, > and in that hash table entry you can store everything about that file > that you need to know, whether that's the file data, the disk file > that contains the file data, the fact that we already threw the data > away, or any other fact that you can imagine wanting to know. > > Said differently, the astreamer buffer is not really a great place to > write data. It exists because when we're just forwarding data from one > astreamer to the next, we will often need to buffer a small amount of > data to avoid terrible performance. However, it's only there to be > used when we don't have something better. I don't think any astreamer > that is intended to be the last one in the chain currently writes to > the buffer -- they write to the output file, or whatever, because > using an in-memory buffer as your final output destination is not a > real good plan. > Make sense, I implemented this approach in the attached version, but with a different structure name and a slightly different error message. In the error output using the WAL file name instead of the LSN. This is because the LSN at that point may differ from the user-provided one (it might have been adjusted to the start of a WAL page by xlogreader). This follows the same style used in the routine that reads the WAL file. The LSN values (user provided) are only used in error messages generated at the very beginning, specifically in the main() function of pg_waldump. I have also restructured the code by moving most of the tar file reading logic out of pg_waldump.c into astreamer_waldump.c, which has now been renamed to archive_waldump.c. Kindly have a look at the attached version. Thank you ! Regards, Amul -
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-11-17T04:50:48Z
On Thu, Nov 6, 2025 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote: > > On Mon, Oct 20, 2025 at 8:05 PM Robert Haas <robertmhaas@gmail.com> wrote: > > > > On Thu, Oct 16, 2025 at 7:49 AM Amul Sul <sulamul@gmail.com> wrote: > > [....] > Kindly have a look at the attached version. Thank you ! > Attached is the rebased version against the latest master head (e76defbcf09). Regards, Amul
-
Re: pg_waldump: support decoding of WAL inside tarfile
Jakub Wartak <jakub.wartak@enterprisedb.com> — 2025-11-19T08:20:14Z
On Mon, Nov 17, 2025 at 5:51 AM Amul Sul <sulamul@gmail.com> wrote: > > On Thu, Nov 6, 2025 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote: > > > > On Mon, Oct 20, 2025 at 8:05 PM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > On Thu, Oct 16, 2025 at 7:49 AM Amul Sul <sulamul@gmail.com> wrote: > > > [....] > > Kindly have a look at the attached version. Thank you ! > > > > Attached is the rebased version against the latest master head (e76defbcf09). Hi Amul, thanks for working on this. I haven't really looked at the source code deeply (I trust Robert eyes much more than mine on this one), just skimmed a little bit: 1. As stated earlier, get_tmp_walseg_path() is still vulnerable (it uses predictable path that could be used by attacker in $TMPDIR) 2. On the usability front: a. If you do `pg_waldump --path pg_wal.tar -s 0/31000000` it will dump a lot of WAL records and then print final: pg_waldump: error: could not find file "000000010000000000000034" in archive However, with `pg_waldump --path pg_wal.tar -s 0/31000000 --stats=record` (not passing '-e') it will simply bailout without printing stats and with error: pg_waldump: error: could not find file "000000010000000000000034" in archive IMHO, it could print stats if it was capable of getting at least 1 WAL record. 3. The most critical issue for me was the initial lack of error pass-through from pg_waldump (when used with WALs in tar) to the pg_verifybackup. Now it works fine, so thanks for this: a. pg_waldump is capable of discovering missing WALs as requested and throwing proper return code (good) $ /usr/pgsql19/bin/pg_waldump --path pg_wal.tar -s 0/31005F70 -e 0/343D2650 -q pg_waldump: error: could not find file "000000010000000000000034" in archive $ echo $? 1 $ b. pg_verifybackup now also complains properly with missing WAL inside tar $ tar --delete -f pg_wal.tar 000000010000000000000032 # simulate loss of file $ tar -tf pg_wal.tar 000000010000000000000031 archive_status/000000010000000000000031.done archive_status/000000010000000000000032.done 000000010000000000000033 $ grep Start-LSN backup_manifest { "Timeline": 1, "Start-LSN": "0/31005F70", "End-LSN": "0/333D2650" } $ /usr/pgsql19/bin/pg_verifybackup -P /tmp/basebackup/ 791372/791372 kB (100%) verified pg_waldump: error: could not find file "000000010000000000000032" in archive pg_verifybackup: error: WAL parsing failed for timeline 1 $ echo $? 1 $ -J. -
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-11-21T11:44:26Z
On Wed, Nov 19, 2025 at 1:50 PM Jakub Wartak <jakub.wartak@enterprisedb.com> wrote: > > On Mon, Nov 17, 2025 at 5:51 AM Amul Sul <sulamul@gmail.com> wrote: > > > > On Thu, Nov 6, 2025 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote: > > > > > > On Mon, Oct 20, 2025 at 8:05 PM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > On Thu, Oct 16, 2025 at 7:49 AM Amul Sul <sulamul@gmail.com> wrote: > > > > [....] > > > Kindly have a look at the attached version. Thank you ! > > > > > > > Attached is the rebased version against the latest master head (e76defbcf09). > > Hi Amul, thanks for working on this. I haven't really looked at the > source code deeply (I trust Robert eyes much more than mine on this > one), just skimmed a little bit: > > 1. As stated earlier, get_tmp_walseg_path() is still vulnerable (it > uses predictable path that could be used by attacker in $TMPDIR) > Yeah, I haven't done anything regarding this since I am unsure of what should be done and what the risks involved are. I am thinking of taking Robert's opinion on this. > 2. On the usability front: > > a. If you do `pg_waldump --path pg_wal.tar -s 0/31000000` it will dump > a lot of WAL records and then print final: > pg_waldump: error: could not find file "000000010000000000000034" in archive > > However, with `pg_waldump --path pg_wal.tar -s 0/31000000 > --stats=record` (not passing '-e') it will simply bailout without > printing stats and with error: > pg_waldump: error: could not find file "000000010000000000000034" in archive > > IMHO, it could print stats if it was capable of getting at least 1 WAL record. > The similar behavior in the current pg_waldump when using the --path option with a WAL directory and a starting LSN. E.g: $ pg_waldump -s 0/04FE36E0 --path=/tmp/backup/tmp/ --stats=record pg_waldump: first record is after 0/04FE36E0, at 0/04FE3F90, skipping over 2224 bytes pg_waldump: error: could not find file "000000010000000000000009": No such file or directory > 3. The most critical issue for me was the initial lack of error > pass-through from pg_waldump (when used with WALs in tar) to the > pg_verifybackup. Now it works fine, so thanks for this: > Thanks, that was exactly the intention -- to complete pg_verifybackup for tar-formatted backup verification. Regards, Amul
-
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-11-21T12:16:24Z
On Mon, Nov 17, 2025 at 10:20 AM Amul Sul <sulamul@gmail.com> wrote: > > On Thu, Nov 6, 2025 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote: > > > > On Mon, Oct 20, 2025 at 8:05 PM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > On Thu, Oct 16, 2025 at 7:49 AM Amul Sul <sulamul@gmail.com> wrote: > > > [....] > > Kindly have a look at the attached version. Thank you ! > > > Attached is the updated version. I have fixed an assertion failure that can occasionally occur with a partial WAL page read. Regards, Amul
-
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-11-25T06:37:32Z
On Fri, Nov 21, 2025 at 5:14 PM Amul Sul <sulamul@gmail.com> wrote: > > On Wed, Nov 19, 2025 at 1:50 PM Jakub Wartak > <jakub.wartak@enterprisedb.com> wrote: > > > > On Mon, Nov 17, 2025 at 5:51 AM Amul Sul <sulamul@gmail.com> wrote: > > > > > > On Thu, Nov 6, 2025 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote: > > > > > > > > On Mon, Oct 20, 2025 at 8:05 PM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > > > On Thu, Oct 16, 2025 at 7:49 AM Amul Sul <sulamul@gmail.com> wrote: > > > > > [....] > > > > Kindly have a look at the attached version. Thank you ! > > > > > > > > > > Attached is the rebased version against the latest master head (e76defbcf09). > > > > Hi Amul, thanks for working on this. I haven't really looked at the > > source code deeply (I trust Robert eyes much more than mine on this > > one), just skimmed a little bit: > > > > 1. As stated earlier, get_tmp_walseg_path() is still vulnerable (it > > uses predictable path that could be used by attacker in $TMPDIR) > > > > Yeah, I haven't done anything regarding this since I am unsure of what > should be done and what the risks involved are. I am thinking of > taking Robert's opinion on this. > Per offline discussion with Robert and Jakub, I have updated the patch to use mkdtemp() as suggested, which is already available in the tree for similar purposes. Thanks ! Regards, Amul
-
Re: pg_waldump: support decoding of WAL inside tarfile
Chao Li <li.evan.chao@gmail.com> — 2025-11-25T08:50:29Z
Hi Amul, I reviewed the patch and got some comments: > On Nov 25, 2025, at 14:37, Amul Sul <sulamul@gmail.com> wrote: > > > Regards, > Amul > <v8-0001-Refactor-pg_waldump-Move-some-declarations-to-new.patch><v8-0002-Refactor-pg_waldump-Separate-logic-used-to-calcul.patch><v8-0003-Refactor-pg_waldump-Restructure-TAP-tests.patch><v8-0004-pg_waldump-Add-support-for-archived-WAL-decoding.patch><v8-0005-pg_waldump-Remove-the-restriction-on-the-order-of.patch><v8-0006-pg_verifybackup-Delay-default-WAL-directory-prepa.patch><v8-0007-pg_verifybackup-Rename-the-wal-directory-switch-t.patch><v8-0008-pg_verifybackup-enabled-WAL-parsing-for-tar-forma.patch> 1 - 0001 - pg_waldump.h ``` + * pg_waldump.h - decode and display WAL + * + * Copyright (c) 2013-2025, PostgreSQL Global Development Group ``` This header file is brand new, so copyright year should be only 2025. 2 - 0001 - pg_waldump.c ``` -static int WalSegSz; +int WalSegSz = DEFAULT_XLOG_SEG_SIZE; ``` 0001 claims a refactoring, but if you initialize WalSegSz with DEFAULT_XLOG_SEG_SIZE, then the behavior is changing, this change is no longer a pure refactor. I would suggest leave WalSegSz uninitiated (compiler will set 0 to it), then no behavior change, so that 0001 stays a self-contained pure refactor. The other nit thing is that, as “static” is removed, now “WalSegSz” is placed in middle of two static variables, which looks not good. If I were making the code change, I would have moved WalSegSz to after all static variables. 3 - 0002 ``` @@ -383,21 +406,11 @@ WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetPtr, char *readBuff) { XLogDumpPrivate *private = state->private_data; - int count = XLOG_BLCKSZ; + int count = required_read_len(private, targetPagePtr, reqLen); WALReadError errinfo; - if (XLogRecPtrIsValid(private->endptr)) - { - if (targetPagePtr + XLOG_BLCKSZ <= private->endptr) - count = XLOG_BLCKSZ; - else if (targetPagePtr + reqLen <= private->endptr) - count = private->endptr - targetPagePtr; - else - { - private->endptr_reached = true; - return -1; - } - } + if (private->endptr_reached) + return -1; ``` This change introduces a logic hole. In old code, it sets private->endptr_reached = true; and return -1. In the code code, count and private->endptr_reached assignments are wrapped into required_read_len(). However, required_read_len() doesn’t check if private->endptr_reached has already been true, so that the logic hole is that, if private->endptr_reached is already true when calling required_read_len(), and required_read_len() returns a positive count, if (private->endptr_reached) will also be satisfied and return -1 from the function. So, to be safe, we should check “if (count < 0) return -1”. 4 - 0002 ``` +/* Returns the size in bytes of the data to be read. */ +static inline int +required_read_len(XLogDumpPrivate *private, XLogRecPtr targetPagePtr, + int reqLen) +{ ``` The function comment is too simple. It doesn’t cover the case where -1 is returned. 5 - 0003 ``` +my @scenario = ( + { + 'path' => $node->data_dir + }); -@lines = test_pg_waldump('--limit' => 6); -is(@lines, 6, 'limit option observed'); +for my $scenario (@scenario) +{ ``` "my @scenario” should be "my @scenarios”, so that for line become "for my $scenario (@scenarios)”, a little bit clearer. 6 - 0003 ``` + SKIP: + { ``` Why SKIP label is defined here? A SKIP label usually follows a skip statement, for example: in bin/pg_ctl/t/001_start_stop.pl ``` SKIP: { skip "unix-style permissions not supported on Windows", 2 if ($windows_os); ok(-f $logFileName); ok(check_mode_recursive("$tempdir/data", 0700, 0600)); } ``` 7 - 0004 - Makefile ``` $(WIN32RES) \ compat.o \ pg_waldump.o \ + archive_waldump.o \ rmgrdesc.o \ xlogreader.o \ xlogstats.o ``` Obviously the list was in alphabetical order, so archive_waldump.o should be placed before compat.o. 8 - 0004 ``` +/* + * pg_waldump's XLogReaderRoutine->page_read callback to support dumping WAL + * files from tar archives. + */ +static int +TarWALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, + XLogRecPtr targetPtr, char *readBuff) +{ + XLogDumpPrivate *private = state->private_data; + int count = required_read_len(private, targetPagePtr, reqLen); ``` Looking the page_read’s spec: ``` /* * Data input callback * * This callback shall read at least reqLen valid bytes of the xlog page * starting at targetPagePtr, and store them in readBuf. The callback * shall return the number of bytes read (never more than XLOG_BLCKSZ), or * -1 on failure. The callback shall sleep, if necessary, to wait for the * requested bytes to become available. The callback will not be invoked * again for the same page unless more than the returned number of bytes * are needed. * * targetRecPtr is the position of the WAL record we're reading. Usually * it is equal to targetPagePtr + reqLen, but sometimes xlogreader needs * to read and verify the page or segment header, before it reads the * actual WAL record it's interested in. In that case, targetRecPtr can * be used to determine which timeline to read the page from. * * The callback shall set ->seg.ws_tli to the TLI of the file the page was * read from. */ XLogPageReadCB page_read; ``` It says that page_read must read reqLen bytes, otherwise it should wait for more bytes. However, TarWALDumpReadPage just calculate how many bytes can read and only read that long, which breaks the protocol. Is it a problem? 9 - 0004 ``` +/* + * Create an astreamer that can read WAL from tar file. + */ +static astreamer * +astreamer_waldump_new(XLogDumpPrivate *privateInfo) +{ + astreamer_waldump *streamer; + + streamer = palloc0(sizeof(astreamer_waldump)); + *((const astreamer_ops **) &streamer->base.bbs_ops) = + &astreamer_waldump_ops; + + streamer->privateInfo = privateInfo; + + return &streamer->base; +} ``` This function allocates memory for streamer but only returns &streamer->base, so memory of streamer is leaked. Also, in the function comment, “from tar file” => “from a tar file”. 10 - 0004 ``` + * End-of-stream processing for a astreamer_waldump stream. ``` Nit typo: a => an 11 - 0004 ``` + if (!IsValidWalSegSize(WalSegSz)) + { + pg_log_error(ngettext("invalid WAL segment size in WAL file from archive \"%s\" (%d byte)", + "invalid WAL segment size in WAL file from archive \"%s\" (%d bytes)", + WalSegSz), + privateInfo->archive_name, WalSegSz); + pg_log_error_detail("The WAL segment size must be a power of two between 1 MB and 1 GB."); + exit(1); + } ``` Why don’t pg_fatal()? 12 - 0005 ``` + /* Create a temporary file if one does not already exist */ + if (!entry->tmpseg_exists) + { + write_fp = prepare_tmp_write(entry->segno); + entry->tmpseg_exists = true; + } + + /* Flush data from the buffer to the file */ + perform_tmp_write(entry->segno, &entry->buf, write_fp); + resetStringInfo(&entry->buf); + + /* + * The change in the current segment entry indicates that the reading + * of this file has ended. + */ + if (entry != privateInfo->cur_wal && write_fp != NULL) + { + fclose(write_fp); + write_fp = NULL; + } ``` When entry->tmpseg_exists is true, then write_fp will not be initialized, but there should be a check to make sure write_fp is not NULL before perform_tmp_write(). Also, if write_fp != NULL, should we anyway close the file without considering entry != privateInfo->cur_wal? Otherwise write_fp may be left open. 13 - 0005 ``` + * Use the directory specified by the TEMDIR environment variable. If it’s ``` Typo: TEMDIR => TMPDIR 14 - 0005 ``` + * Set up a temporary directory to temporarily store WAL segments. ``` temporary and temporarily are redundant. No comment for 0007. 15 - 0007 I wonder why we need to manually po files? This is the first time I see a patch including po file changes. 16 - 0008 ``` + { + pg_log_error("wal archive not found"); + pg_log_error_hint("Specify the correct path using the option -w/--wal-path." + "Or you must use -n/--no-parse-wal when verifying a tar-format backup."); + exit(1); + } ``` “wal” should be “WAL”. In the hint message, there should be a white space between the two sentences. Again, why not pg_fatal(). Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: pg_waldump: support decoding of WAL inside tarfile
amul sul <sulamul@gmail.com> — 2025-11-26T06:02:04Z
On Tue, Nov 25, 2025 at 2:21 PM Chao Li <li.evan.chao@gmail.com> wrote: > > Hi Amul, > > I reviewed the patch and got some comments: > Thanks for the review. Replying inline below. > 1 - 0001 - pg_waldump.h > ``` > + * pg_waldump.h - decode and display WAL > + * > + * Copyright (c) 2013-2025, PostgreSQL Global Development Group > ``` > > This header file is brand new, so copyright year should be only 2025. > Fixed in the attached version. > 2 - 0001 - pg_waldump.c > ``` > -static int WalSegSz; > +int WalSegSz = DEFAULT_XLOG_SEG_SIZE; > ``` > > 0001 claims a refactoring, but if you initialize WalSegSz with DEFAULT_XLOG_SEG_SIZE, then the behavior is changing, this change is no longer a pure refactor. > > I would suggest leave WalSegSz uninitiated (compiler will set 0 to it), then no behavior change, so that 0001 stays a self-contained pure refactor. > Agreed. > The other nit thing is that, as “static” is removed, now “WalSegSz” is placed in middle of two static variables, which looks not good. If I were making the code change, I would have moved WalSegSz to after all static variables. > I placed it before the static declaration. > 3 - 0002 > ``` > @@ -383,21 +406,11 @@ WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, > XLogRecPtr targetPtr, char *readBuff) > { > XLogDumpPrivate *private = state->private_data; > - int count = XLOG_BLCKSZ; > + int count = required_read_len(private, targetPagePtr, reqLen); > WALReadError errinfo; > > - if (XLogRecPtrIsValid(private->endptr)) > - { > - if (targetPagePtr + XLOG_BLCKSZ <= private->endptr) > - count = XLOG_BLCKSZ; > - else if (targetPagePtr + reqLen <= private->endptr) > - count = private->endptr - targetPagePtr; > - else > - { > - private->endptr_reached = true; > - return -1; > - } > - } > + if (private->endptr_reached) > + return -1; > ``` > > This change introduces a logic hole. In old code, it sets private->endptr_reached = true; and return -1. In the code code, count and private->endptr_reached assignments are wrapped into required_read_len(). However, required_read_len() doesn’t check if private->endptr_reached has already been true, so that the logic hole is that, if private->endptr_reached is already true when calling required_read_len(), and required_read_len() returns a positive count, if (private->endptr_reached) will also be satisfied and return -1 from the function. > > So, to be safe, we should check “if (count < 0) return -1”. > I do not really understand the logical hole where the behaviour is the same as the previous, but I like the idea of checking endptr_reached. This is quite unlikely to be true, but it looks like good practice to check that flag before setting it. Did it that way in the attached version. > 4 - 0002 > ``` > +/* Returns the size in bytes of the data to be read. */ > +static inline int > +required_read_len(XLogDumpPrivate *private, XLogRecPtr targetPagePtr, > + int reqLen) > +{ > ``` > > The function comment is too simple. It doesn’t cover the case where -1 is returned. > Okay. > 5 - 0003 > ``` > +my @scenario = ( > + { > + 'path' => $node->data_dir > + }); > > -@lines = test_pg_waldump('--limit' => 6); > -is(@lines, 6, 'limit option observed'); > +for my $scenario (@scenario) > +{ > ``` > > "my @scenario” should be "my @scenarios”, so that for line become "for my $scenario (@scenarios)”, a little bit clearer. > Done. > 6 - 0003 > ``` > + SKIP: > + { > ``` > > Why SKIP label is defined here? A SKIP label usually follows a skip statement, for example: in bin/pg_ctl/t/001_start_stop.pl > ``` > SKIP: > { > skip "unix-style permissions not supported on Windows", 2 > if ($windows_os); > > ok(-f $logFileName); > ok(check_mode_recursive("$tempdir/data", 0700, 0600)); > } > ``` > Yeah, I knew that, but that is needed in the next patch where I wanted to avoid a large diff when introducing SKIP and the associated indentation. This patch is not expected to be committed independently, and I have added a note in the commit message for the same. > 7 - 0004 - Makefile > ``` > $(WIN32RES) \ > compat.o \ > pg_waldump.o \ > + archive_waldump.o \ > rmgrdesc.o \ > xlogreader.o \ > xlogstats.o > ``` > > Obviously the list was in alphabetical order, so archive_waldump.o should be placed before compat.o. > Done. > 8 - 0004 > ``` > +/* > + * pg_waldump's XLogReaderRoutine->page_read callback to support dumping WAL > + * files from tar archives. > + */ > +static int > +TarWALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, > + XLogRecPtr targetPtr, char *readBuff) > +{ > + XLogDumpPrivate *private = state->private_data; > + int count = required_read_len(private, targetPagePtr, reqLen); > ``` > > Looking the page_read’s spec: > ``` > /* > * Data input callback > * > * This callback shall read at least reqLen valid bytes of the xlog page > * starting at targetPagePtr, and store them in readBuf. The callback > * shall return the number of bytes read (never more than XLOG_BLCKSZ), or > * -1 on failure. The callback shall sleep, if necessary, to wait for the > * requested bytes to become available. The callback will not be invoked > * again for the same page unless more than the returned number of bytes > * are needed. > * > * targetRecPtr is the position of the WAL record we're reading. Usually > * it is equal to targetPagePtr + reqLen, but sometimes xlogreader needs > * to read and verify the page or segment header, before it reads the > * actual WAL record it's interested in. In that case, targetRecPtr can > * be used to determine which timeline to read the page from. > * > * The callback shall set ->seg.ws_tli to the TLI of the file the page was > * read from. > */ > XLogPageReadCB page_read; > ``` > > It says that page_read must read reqLen bytes, otherwise it should wait for more bytes. > > However, just calculate how many bytes can read and only read that long, which breaks the protocol. Is it a problem? > The behaviour is the same as the routine used to read the bare WAL file. I don't think there will be any problem for the pg_waldump. > 9 - 0004 > ``` > +/* > + * Create an astreamer that can read WAL from tar file. > + */ > +static astreamer * > +astreamer_waldump_new(XLogDumpPrivate *privateInfo) > +{ > + astreamer_waldump *streamer; > + > + streamer = palloc0(sizeof(astreamer_waldump)); > + *((const astreamer_ops **) &streamer->base.bbs_ops) = > + &astreamer_waldump_ops; > + > + streamer->privateInfo = privateInfo; > + > + return &streamer->base; > +} > ``` > > This function allocates memory for streamer but only returns &streamer->base, so memory of streamer is leaked. > May I know why you think there would be a memory leak? I believe the address of the structure is the same as the address of its first member, base. I am returning base because the goal is to return a generic astreamer type, which is the standard approach used in other archive streamer code. > Also, in the function comment, “from tar file” => “from a tar file”. > > 10 - 0004 > ``` > + * End-of-stream processing for a astreamer_waldump stream. > ``` > > Nit typo: a => an > Done. > 11 - 0004 > ``` > + if (!IsValidWalSegSize(WalSegSz)) > + { > + pg_log_error(ngettext("invalid WAL segment size in WAL file from archive \"%s\" (%d byte)", > + "invalid WAL segment size in WAL file from archive \"%s\" (%d bytes)", > + WalSegSz), > + privateInfo->archive_name, WalSegSz); > + pg_log_error_detail("The WAL segment size must be a power of two between 1 MB and 1 GB."); > + exit(1); > + } > ``` > > Why don’t pg_fatal()? > This is how we do when we need to emit error details as well. > 12 - 0005 > ``` > + /* Create a temporary file if one does not already exist */ > + if (!entry->tmpseg_exists) > + { > + write_fp = prepare_tmp_write(entry->segno); > + entry->tmpseg_exists = true; > + } > + > + /* Flush data from the buffer to the file */ > + perform_tmp_write(entry->segno, &entry->buf, write_fp); > + resetStringInfo(&entry->buf); > + > + /* > + * The change in the current segment entry indicates that the reading > + * of this file has ended. > + */ > + if (entry != privateInfo->cur_wal && write_fp != NULL) > + { > + fclose(write_fp); > + write_fp = NULL; > + } > ``` > > When entry->tmpseg_exists is true, then write_fp will not be initialized, but there should be a check to make sure write_fp is not NULL before perform_tmp_write(). > perform_tmp_write() has assert for the same. > Also, if write_fp != NULL, should we anyway close the file without considering entry != privateInfo->cur_wal? Otherwise write_fp may be left open. > We read the WAL from the tar file in chunks, and those same chunks are written to the temporary file within the loop. If we close the temporary file now, we will have to open it again later for the next chunk write. Could you elaborate on a scenario where you believe this file might be left open unintentionally? > 13 - 0005 > ``` > + * Use the directory specified by the TEMDIR environment variable. If it’s > ``` > > Typo: TEMDIR => TMPDIR > Done. > 14 - 0005 > ``` > + * Set up a temporary directory to temporarily store WAL segments. > ``` > > temporary and temporarily are redundant. > I believe that is grammatically correct and clear. > No comment for 0007. > > 15 - 0007 > > I wonder why we need to manually po files? This is the first time I see a patch including po file changes. > Okay, I included that initially to ensure the PO file update wasn't overlooked during commit. I have removed it to minimize the diff and added the note in the patch commit message. > 16 - 0008 > ``` > + { > + pg_log_error("wal archive not found"); > + pg_log_error_hint("Specify the correct path using the option -w/--wal-path." > + "Or you must use -n/--no-parse-wal when verifying a tar-format backup."); > + exit(1); > + } > ``` > > “wal” should be “WAL”. > > In the hint message, there should be a white space between the two sentences. > Done. Thanks again for your review comments; they are quite helpful. Kindly take a look at the attached version. Regards, Amul -
Re: pg_waldump: support decoding of WAL inside tarfile
Chao Li <li.evan.chao@gmail.com> — 2025-11-26T07:23:39Z
> On Nov 26, 2025, at 14:02, Amul Sul <sulamul@gmail.com> wrote: > >> 9 - 0004 >> ``` >> +/* >> + * Create an astreamer that can read WAL from tar file. >> + */ >> +static astreamer * >> +astreamer_waldump_new(XLogDumpPrivate *privateInfo) >> +{ >> + astreamer_waldump *streamer; >> + >> + streamer = palloc0(sizeof(astreamer_waldump)); >> + *((const astreamer_ops **) &streamer->base.bbs_ops) = >> + &astreamer_waldump_ops; >> + >> + streamer->privateInfo = privateInfo; >> + >> + return &streamer->base; >> +} >> ``` >> >> This function allocates memory for streamer but only returns &streamer->base, so memory of streamer is leaked. >> > > May I know why you think there would be a memory leak? I believe the > address of the structure is the same as the address of its first > member, base. I am returning base because the goal is to return a > generic astreamer type, which is the standard approach used in other > archive streamer code. Ah… Got it. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/