Hi Robert,
Thanks for taking a look.
On Thu, Sep 25, 2025 at 11:35 AM Robert Haas <robertmhaas@gmail.com> wrote:
> ...
> integrated. Let's rename the isRelDataFile() to getFileContentType()
> and put all the logic in that function, including appropriate tests of
> the path name. Second, in decide_file_action(), I think we should
> check that entry->target_size == entry->source_size and return
> FILE_ACTION_COPY if not. This case really shouldn't happen, but it's
> very cheap to check and might offer a bit of protection in some weird
> scenario.
Updated the patch to reflect that.
> I am not a huge fan of the way that the patch stuffs last_common_segno
> into a global variable that is then accessed by
> ...
I forgot why I bothered with a global instead since it was straightforward
to actually pass in the argument. Updated.
> It does not appear to me that the test case tests what it purports to
> test, and I don't think that what it purports to test is the right
> thing anyway. It claims that it is testing that a certain WAL file
> (which it erroneously calls a "WAL file entry") is not copied to the
> target, but I see no logic to actually check this.
> ...
That's a fair point. The test does:
1. Write some data -> WAL 1
2. SELECT pg_switch_wal() -> pg_current_wal_lsn() is at WAL 2
3. CHECKPOINT; -> Makes WAL 2 the last common checkpoint
Then it checks that "WAL 1 not copied over" was logged which isn't
the best since if it was logged elsewhere then it would still pass.
> ...
> first. If that's correct, I don't quite know what a test case here can
> usefully verify, since the only difference would be performance, but
> maybe I'm misunderstanding something?
I updated the test to run stat and get the modification time of the common
WAL segment before and after pg_rewind and verify it is the same.
In filemap.c in decide_wal_file_action, if you comment out
> return FILE_ACTION_NONE;
the test fails.
>
> As an administrative note, the naming convention for the patches
> posted to the thread is not consistent and not correct. Use "git
> format-patch -v$VERSION" to generate patches. In a name like
> ...
Fixed patch formatting.
> ...
> writing it. I see that a number of different people have posted
> versions of the patch; it would be nice if "Author:" or
> "Co-authored-by:" tags were added to the commit message to reflect
I added Justin as the Co-Author in the commit message but I realized
in their original patch they had "Author: Justin Kwan, Vignesh Ravichandran".
Andres also had the latest patch series before mine so I'll leave it up to
the committer how they want to handle this.
Thanks,
--
John Hsu - Amazon Web Services
