On 01/04/2022 12:00, Daniel Gustafsson wrote:
I took another look at this patch, and I think it's ready to go in, it clearly
fixes a bug that isn't too hard to hit in production settings. To ensure we
don't break this I've added a testcase which pipes the pg_rewind --verbose
output to a file it's asked to copy, which then guarantees that the file is
growing in size during the operation without need for synchronizing two
processes with IPC::Run (it also passes on Windows in the CI setup).
One small comment on the patch:
+ snprintf(srcpath, sizeof(srcpath), "%s/%s", datadir, path);
This should IMO check the returnvalue of snprintf to ensure it wasn't
truncated. While the risk is exceedingly small, a truncated filename might
match another existing filename and the error not getting caught. There is
another instance just like this one in open_target_file() to which I think we
should apply the same belts-and-suspenders treatment. I've fixed this in the
attached version which also have had a pg_indent run on top of a fresh rebase.
+ if (len >= sizeof(dstpath))
+ pg_fatal("filepath buffer too small"); /* shouldn't
happen */
Makes sense. I would remove the "shouldn't happen"; it's not very hard
to make it happen, you just need a very long target datadir path. And
rephrase the error message as "datadir path too long".
One typo in the commit message: s/update/updates/.
Thanks!
- Heikki
