Hi Ayush, Heikki,
I tested v2 on a real local-mode rewind -- primary, standby promoted, old
primary rewound from it -- forcing the branch with an LD_PRELOAD shim that
makes lstat() return ENOENT for one chosen name, since the readdir/lstat
race isn't reproducible by hand. Three things worth adding.
1. On master the consequence is worse than an uninitialized read: the entry
can be dropped silently.
The same injection in two places, on master:
a file in the source root
pg_rewind: error: could not open directory ".../vanishme_test":
Not a directory
exit 1
a file in source global/
pg_rewind: Done!
exit 0
The second one is the bad case. That file exists in the source, and a
control run without the injection copies it to the target; with the
injection it is simply absent from the rewound target and pg_rewind reports
success. Which of the two you get depends on whether the garbage st_mode
happens to satisfy S_ISREG/S_ISDIR/S_ISLNK -- in the first run it looked
like a directory, so recurse_dir() descended into a path that isn't there
and reported a problem unrelated to the real one. So this is not only
undefined in theory: one of its outcomes is an incomplete rewind that exits
0.
v2 gives the same message in both places, and exits 1:
pg_rewind: error: could not stat file ".../global/vanishme2": No
such file or directory
2. The premise behind the change is enforced, not just expected.
sanityChecks() already refuses to run unless the target -- and, for
--source-pgdata, the source -- has DB_SHUTDOWNED or DB_SHUTDOWNED_IN_RECOVERY
in its control file, and it runs well before either traversal. A running
source therefore cannot reach recurse_dir() at all, which is a bit stronger
than what the new comment claims, and might be worth pointing at from there.
The one gap is the one that check's own comment already admits -- "This
doesn't guard against someone starting the cluster concurrently" -- and that
is precisely the case where the new pg_fatal() fires. Failing loudly is the
right answer there.
3. The new error cannot leave a half-rewound target.
Both traversals run before anything is written to the target; the first
modification is remove_target() in the apply loop, with createBackupLabel()
after it. Confirmed rather than assumed: with the error injected I compared
the target against a copy taken before the run, and diff -r reports no
difference, so the rewind can just be retried.
make check in src/bin/pg_rewind passes with v2. For what it's worth,
nothing else in pg_rewind reads a struct stat after tolerating a failed
stat: the only other ENOENT is remove_target_file()'s missing_ok, which is
deliberate and doesn't inspect anything afterwards.
I found nothing to change in the code -- as far as I can tell v2 is ready to
commit. Two things for the committer rather than for the patch:
- The commit message explains the bug as "reading an uninitialized struct
stat", which undersells it. The observable outcome in my second run was a
rewind that exits 0 with a source file missing from the target. That seems
worth saying, not least because it bears on whether this is back-patch
material -- I don't have a strong view there, since the race needs
something removing files from a data directory pg_rewind has already
verified is shut down, which is not an everyday event.
- The new comment could point at sanityChecks() for its "never over a running
server" claim, since that is where it is actually enforced.
Regards,
Rui
