> I don't think it does, because I think those options are anyway > blocked on Windows. See the comment block that says /* Check that the > platform supports the requested copy method. */
Yeah, you are right. I could swear I ran `pg_combinebackup --clone` on a Windows VM, and instead of erroring out it instead ran with CopyFile. But that's not the case, it errors out (as expected). > So, this still fails for me, in a manner quite similar to before. The > first call to check_data_file() is for > /Users/robert.haas/pgsql-meson/testrun/pg_combinebackup/010_links/data/t_010_links_restore_data/pgdata/base/5/16384 > but @data_file_segments ends up zero-length. Therefore, $last_segment > is undef when we access it at the bottom of check_data_file(). I think > the problem is here: > > my $basename = (split /\./, $full_path)[0]; > > This code assumes that no path name component other than the filename > at the end contains a period, but in my case that's not true, because > my home directory on this particular computer is /Users/robert.haas. Right, well spotted. The test code had bad assumptions. Those assumptions should be in the file name, not in the full path, thus that broke in your env. > But I'm wondering why you are even using readdir() here in the first > place. You could just test -f "16384"; if it exists test -f "16384.1"; > if that exists test -f "16384.2" and keep going until you reach a file > that does not exist. Not only would this avoid reading the entire > directory contents for every call to this function, but it would also > generate the list of files in sorted order, which would let you throw > away natural_sort(). That makes total sense. In the v6 version I threw out the natural_sort and started using the approach you described, which is much more efficient and less error prone. > Overall, I feel like this could just be significantly simpler. For > instance, I don't quite see the need to have both test_1 and test_2. > If we just have one table and all the segments but the last have 2 > hard links while the last have 1, isn't that enough? What test > coverage do we get by adding the second relation? All of the segments > of test_1 behave just like the non-final segments of test_2, so why > test both? If you go down to 1 table, you can simplify a bunch of > things here. I think that isn't enough because it makes assumptions on the environment -- in this case that assumes the Linux autoconf environment from Cirrus CI. The test is creating a couple of tables with ~264KB. If you have a very small segment size, like we have in the autoconf job of Cirrus CI, then that's enough to create more than a single file, in such a way we are able to verify segments with 2 or 1 hard links. However, when running through other jobs, like Windows meson, it uses the default segment size of 1GB, and in that case we would have a single segment with 1 hard link if we had a single table. With that in mind, and taking into consideration that local builds from devs might use the default segment size, having a couple of tables to test different purposes, i.e. test_1 for testing an "unmodified table" scenario, and test_2 for testing a "modified table" scenario, looks more appropriate to me. For now I'm keeping the patch as it was in that sense, i.e. with a check_data_file function that is called both for test_1 and test_2. I added a comment about that in the test file. I can send a new patch version if that still sounds like an overkill (having two test tables). > Why does the insert add 10 new rows instead of just 1? Doesn't that > introduce a significant risk that with some choice of segment size > those inserts will be spread across two segments rather than one, > leading to the test failing? That was a leftover from some manual attempts on my side before sending the patch, sorry! I started with a simple INSERT, then tried a INSERT with a SELECT on a range, and missed reverting before sending the patch. The v6 version of the patch contains the simple INSERT version, which adds only one tuple to the test_2 table, and is safer as your pointed out. Best regards, Israel.
v6-0001-pg_combinebackup-add-support-for-hard-links.patch
Description: Binary data
