Hi all, On Mon, Sep 7, 2026 at 3:35 PM Ayush Tiwari <[email protected]> wrote: > > Hi. > > On Mon, 6 Jul 2026 at 18:49, Heikki Linnakangas <[email protected]> wrote: >> >> On 28/05/2026 09:01, Ayush Tiwari wrote: >> > >> > Does this interpretation make sense, or is there a reason we should >> > continue >> > processing the entry after ENOENT? >> >> So, the full comment is: >> >> > /* >> > * File doesn't exist anymore. This is ok, if the new primary >> > * is running and the file was just removed. If it was a data >> > * file, there should be a WAL record of the removal. If it >> > * was something else, it couldn't have been anyway. >> > * >> > * TODO: But complain if we're processing the target dir! >> >> That explanation doesn't make a whole lot of sense to me. We don't >> support running pg_rewind on a source directory while the source server >> is running. We do support the "connection" mode on a running server, but >> that doesn't use this function. (And it's not clear what the "couldn't >> have been anyway" means here. Mea culpa, I wrote that comment) >> >> >> And it would indeed be nice to implement that TODO. But AFAICS we should >> just always throw an error here. It's not OK if a file goes missing in >> the target dir, and we don't expect it in the source dir either, because >> the server shouldn't be running. > > > Ahh, yes you are right. I'd only been looking at the uninitialized > fst.st_mode read (lstat() fails, but the code still inspects st_mode), > and continue was the minimal way to avoid it. > > In supported paths, recurse_dir() only runs on a stopped local source > data directory or the target data directory; the running-source case > goes through libpq_traverse_files(). So a vanished file isn't expected > there, and erroring would be the better fix. > > v2 attached: it just pg_fatal()s on lstat() failure, dropping the ENOENT > special case and the TODO. >
Thank you for the updated patch. I reviewed and tested the v2 patch. The patch changes the handling of lstat() failures in recurse_dir() and instead of ignoring an ENOENT and continuing the traversal, the patch now reports the failure as a fatal error. This looks correct to me since recurse_dir() is used for stopped data directories, while the running source-server case uses a different traversal path. I also tested the specific failure scenario by creating a real timeline divergence between the source and target clusters. I used an LD_PRELOAD shim to force lstat() to return ENOENT for a selected file under the source global directory. With the patch, pg_rewind correctly stopped with: pg_rewind: error: could not stat file "/tmp/pg_rewind_test/source/global/vanishme2": No such file or directory and exited with status 1 I also verified that the target cluster was not modified after the failed rewind. The existing target-side table was still present, while the source-only table was not introduced. Based on the above testing, the patch behaves as expected and correctly handles an unexpectedly vanished file during traversal. The patch looks good to me. Regards Solai
