Horton

Re: Making pg_rewind faster

John H <johnhyvr@gmail.com>

View thread
From: John H <johnhyvr@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: wenhui qiu <qiuwenhuifx@gmail.com>, Japin Li <japinli@hotmail.com>, Michael Paquier <michael@paquier.xyz>, Andres Freund <andres@anarazel.de>, Alexander Korotkov <aekorotkov@gmail.com>, Justin Kwan <justinpkwan@outlook.com>, Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers <pgsql-hackers@postgresql.org>, vignesh <vignesh@cloudflare.com>, vignesh ravichandran <admin@viggy28.dev>, "hlinnaka@iki.fi" <hlinnaka@iki.fi>, "jkwan@cloudflare.com" <jkwan@cloudflare.com>
Date: 2025-10-09T17:56:48Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_rewind: Skip copy of WAL segments generated before point of divergence

  2. pg_rewind: Extend code detecting relation files to work with WAL files

  3. Split TESTDIR into TESTLOGDIR and TESTDATADIR

Attachments

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