Add -k/--link option to pg_combinebackup

One concern that I have about this --link mode, which Euler Taveira
also got concerned about: the fact that it can invalidate the original
backups if the user modifies or starts the synthetic backup without
moving it to another file system or machine.

At the moment, pg_combinebackup issues a warning about that problem as
soon as the execution of the tool is about to finish. There is also a
warning in the docs. But some users don't pay proper attention to the
output and/or docs.

I wonder if we have some way, like pg_upgrade does (rename a global
file in the original cluster), so we could increase the protection
against that issue?

On Wed, Jan 15, 2025 at 12:42 PM Israel Barth Rubio
<barthisrael@gmail.com> wrote:

With the current implementation of pg_combinebackup, we have a few
copy methods: --clone, --copy and --copy-file-range. By using either of
them, it implicates an actual file copy in the file system, i.e. among
other things, disk usage.

While discussing with some people, e.g. Robert Haas and Martín Marqués,
about possible ways to improve pg_combinebackup performance and/or
reduce disk usage taken by the synthetic backup, there was a thought
of using hard links.

I'm submitting a patch in this thread which introduces the -k/--link
option to pg_combinebackup, making it similar to the options exposed
by pg_upgrade.

In general, +1, although I think that the patch needs polishing in a
bunch of areas.

Originally, I thought what we wanted was something like a --in-place
option to pg_combinebackup, allowing the output directory to be the
same as one of the input directories. However, I now think that what
this patch does is likely better, and this patch is a lot simpler to
write than that one would be. With the --in-place option, if I'm
combine backup1 and backup2, I must write either "pg_combinebackup
backup1 backup2 -o backup1" or "pg_combinebackup backup1 backup2 -o
backup2". In either case, I can skip whole-file copies from one of the
two source directories -- whichever one happens to be the output
directory. However, if I write "pg_combinebackup --link backup1
backup2 -o result", I can skip *all* whole-file copies from *every*
input directory, which seems a lot nicer.

Furthermore, if all the input directories are staging directories,
basically copies of original backups stored elsewhere, then the fact
that those directories get trashed is of no concern to me. In fact,
they don't even get trashed if pg_combinebackup is interrupted partway
through, because I can just remove the output directory and recreate
it and everything is fine. With an --in-place option, that would be
trickier.

Regarding the patch itself, I think we need to rethink the test case
changes in particular. As written, the patch won't do any testing of
link mode unless the user sets PG_TEST_PG_COMBINEBACKUP_MODE=--link;
and if they do set that option, then we won't test anything else.
Also, even with that environment variable set, we'll only test --link
mode a bit ... accidentally. The patch doesn't really do anything to
make sure that link mode actually does what it's intended to do. It
only adapts the existing tests not to fail. I think it would be better
to decide that you're not supposed to set
PG_TEST_PG_COMBINEBACKUP_MODE=--link, and then write some new test,
not depending on PG_TEST_PG_COMBINEBACKUP_MODE, that specifically
verifies that link mode behaves as expected.

After all, link mode is noticeably different from the other copy
modes. Those should all produce equivalent results, or fail outright,
we suppose. This produces a different user-visible result, so we
probably ought to test that we get that result. For example, we might
check that certain files end up with a link count of 1 or 2, as
appropriate.

Does link() work on Windows?

I'm not sure what to do about the issue that using --link trashes the
input cluster if you subsequently start the database or otherwise
modify any hard-linked files. Keep in mind that, for command-line
arguments other than the first, these are incremental backups, and you
already can't run postgres on those directories. Only the first input
directory, which is a full backup not an incremental, is a potential
target for an unexpected database start. I'm tentatively inclined to
think we shouldn't modify the input directories and just emit a
warning like:

warning: --link mode was used; any modifications to the output
directory may destructively modify input directories

...but maybe that's not the right idea. Happy to hear other opinions.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hello Robert,

In general, +1, although I think that the patch needs polishing in a
bunch of areas.

Thanks for your review!

Originally, I thought what we wanted was something like a --in-place
option to pg_combinebackup, allowing the output directory to be the
same as one of the input directories. However, I now think that what
this patch does is likely better, and this patch is a lot simpler to
write than that one would be. With the --in-place option, if I'm
combine backup1 and backup2, I must write either "pg_combinebackup
backup1 backup2 -o backup1" or "pg_combinebackup backup1 backup2 -o
backup2". In either case, I can skip whole-file copies from one of the
two source directories -- whichever one happens to be the output
directory. However, if I write "pg_combinebackup --link backup1
backup2 -o result", I can skip *all* whole-file copies from *every*
input directory, which seems a lot nicer.

Furthermore, if all the input directories are staging directories,
basically copies of original backups stored elsewhere, then the fact
that those directories get trashed is of no concern to me. In fact,
they don't even get trashed if pg_combinebackup is interrupted partway
through, because I can just remove the output directory and recreate
it and everything is fine. With an --in-place option, that would be
trickier.

I see! I thought of not reinventing the wheel while writing, so I replayed
in pg_combinebackup the same behavior implemented in pg_upgrade.
And turns out the outcomes sounded convincing.

Regarding the patch itself, I think we need to rethink the test case
changes in particular. As written, the patch won't do any testing of
link mode unless the user sets PG_TEST_PG_COMBINEBACKUP_MODE=--link;
and if they do set that option, then we won't test anything else.
Also, even with that environment variable set, we'll only test --link
mode a bit ... accidentally. The patch doesn't really do anything to
make sure that link mode actually does what it's intended to do. It
only adapts the existing tests not to fail. I think it would be better
to decide that you're not supposed to set
PG_TEST_PG_COMBINEBACKUP_MODE=--link, and then write some new test,
not depending on PG_TEST_PG_COMBINEBACKUP_MODE, that specifically
verifies that link mode behaves as expected.

After all, link mode is noticeably different from the other copy
modes. Those should all produce equivalent results, or fail outright,
we suppose. This produces a different user-visible result, so we
probably ought to test that we get that result. For example, we might
check that certain files end up with a link count of 1 or 2, as
appropriate.

That makes sense to me too. I'm submitting a new patch which includes
a new file 010_links.pl. That test takes care of generating full and
incremental,
backups, then combines them with --link mode and ensures that the hard link
count for all files under PGDATA is as expected based on the operations that
were applied in the cluster.

I kept the "support" for PG_TEST_PG_COMBINEBACKUP_MODE=--link
around, just in case someone wants to run that and verify those outcomes
with
the --link mode. At least now we might have a proper test for checking
--link,
which always runs independently of the env variable.

Does link() work on Windows?

link is a function available only in Linux. The equivalent function in
Windows is
CreateHardLink.

However, in Postgres link works in either of the operating systems because
we
implement a link function, which wraps the call to CreateHardLink. That's
coded
in the file src/port/win32link.c

I'm not sure what to do about the issue that using --link trashes the
input cluster if you subsequently start the database or otherwise
modify any hard-linked files. Keep in mind that, for command-line
arguments other than the first, these are incremental backups, and you
already can't run postgres on those directories. Only the first input
directory, which is a full backup not an incremental, is a potential
target for an unexpected database start. I'm tentatively inclined to
think we shouldn't modify the input directories and just emit a
warning like:

warning: --link mode was used; any modifications to the output
directory may destructively modify input directories

The previous patch already had a warning. But I enjoyed your message,
so I slightly changed the warning and the docs in the new version of the
patch.

Best regards,
Israel.

Some more review:

+        Use hard links instead of copying files to the synthetic backup.
+        Reconstruction of the synthetic backup might be faster (no file copying
)
+        and use less disk space.

I think it would be good to add a caveat at the end of this sentence:
and use less disk space, but care must be taken when using the output
directory, because any modifications to that directory (for example,
starting the server) can also affect the input directories. Likewise,
changes to the input directories (for example, starting the server on
the full backup) could affect the output directory. Thus, this option
is best used when the input directories are only copies that will be
removed after <application>pg_combienbackup</application> has
completed.

-        which can result in near-instantaneous copying of the data files.
+        which can result in near-instantaneous copying of the data
files, giving
+        the speed advantages of <option>-k</option>/<option>--link</option>
+        while leaving the input backups untouched.

I don't think we need this hunk.

+   When <application>pg_combinebackup</application> is run with
+   <option>-k</option>/<option>--link</option>, the output synthetic backup may
+   share several files with the input backups because of the hard links it
+   creates. Any changes performed on files that were linked between any of the
+   input backups and the synthetic backup, will be reflected on both places.

I don't like the wording of this for a couple of reasons. First,
"several" means more than 1 and less than all, but we really have no
idea: it could be none, one, some, many, or all. Second, we want to be
a little careful not to define what a hard link means here. Also,
these notes are a bit far from the documentation of --link, so
somebody might miss them. I suggest that we can probably just leave
out the notes you've added here if we add something like what I
suggested above to the documentation of --link itself.

+               pg_log_warning_hint("If the input directories are not staging "
+                                                       "directories,
it is recommended to move the output"
+                                                       "directory to
another file system or machine "
+                                                       "before
performing changes to the files and/or "
+                                                       "starting the cluster");

I don't think "staging directories" is a sufficiently well-known and
unambiguous term that we should be using it here. Also, "move the
output directory" seems like fuzzy wording. You can really only move
files around within a filesystem; otherwise, you're making a copy and
deleting the original. I think we can just drop this hint entirely.
The primary warning message seems sufficient to me.

I'm still not a fan of the changes to 010_links.pl; let's drop those.

cfbot is fairly unhappy with your patch. See
http://cfbot.cputube.org/israel-barth.html or
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F5508 --
I haven't looked into what is going wrong here, and there may well be
more than one thing, but nothing can be committed until this is all
green.

--
Robert Haas
EDB: http://www.enterprisedb.com

Oh, one more thing:

+        If a backup manifest is not available or does not contain checksum of
+        the right type, file cloning will be used to copy the file, but the
+        file will be also read block-by-block for the checksum calculation.

s/file cloning will be used to copy the file/hard links will still be created/

--
Robert Haas
EDB: http://www.enterprisedb.com

Thanks for the review, Robert!
I applied all of your suggestions.

I'm still not a fan of the changes to 010_links.pl; let's drop those.

As discussed through a side chat, 010_links.pl is the new test file
which ensures the hard links are created as expected in the output
directory.

I've removed the changes that the v2 patch had in the other test files,
which were the ones you were suggesting to drop here.

cfbot is fairly unhappy with your patch. See
http://cfbot.cputube.org/israel-barth.html or
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F5508 --
I haven't looked into what is going wrong here, and there may well be
more than one thing, but nothing can be committed until this is all
green.

There are some failures on compiler warnings and Windows jobs that
seem unrelated with this patch.

Besides that, there were failures when executing `010_links.pl` in the
Linux autoconf jobs. As discussed in a side chat with Euler and Robert,
and pointed out by Robert, Cirrus CI uses a very small
`--with-segsize-blocks`. My tests in the V2 patch were assuming 1GB
segments, so they were failing in Cirrus CI.

The patch V3, which I'm attaching in this reply, implements some logic
to handle any segment size.

Best regards,
Israel.

On Fri, 21 Feb 2025 at 03:50, Israel Barth Rubio <barthisrael@gmail.com> wrote:

There are some failures on compiler warnings and Windows jobs that
seem unrelated with this patch.

Besides that, there were failures when executing `010_links.pl` in the
Linux autoconf jobs. As discussed in a side chat with Euler and Robert,
and pointed out by Robert, Cirrus CI uses a very small
`--with-segsize-blocks`. My tests in the V2 patch were assuming 1GB
segments, so they were failing in Cirrus CI.

The patch V3, which I'm attaching in this reply, implements some logic
to handle any segment size.

Few comments:
1) The new file 010_links.pl added should be included to meson.build also:
'tests': [
't/010_pg_basebackup.pl',
't/011_in_place_tablespace.pl',
't/020_pg_receivewal.pl',
't/030_pg_recvlogical.pl',
't/040_pg_createsubscriber.pl',
],

2) Since it is a new file, "Copyright (c) 2021-2025" should be
"Copyright (c) 2025":
+++ b/src/bin/pg_combinebackup/t/010_links.pl
@@ -0,0 +1,212 @@
+# Copyright (c) 2021-2025, PostgreSQL Global Development Group
+#
+# This test aims to validate that hard links are created as expected in the
+# output directory, when running pg_combinebackup with --link mode.

3) Since it is a single statement, no need of braces here:
+       /* Warn about the possibility of compromising the backups,
when link mode */
+       if (opt.copy_method == COPY_METHOD_LINK)
+       {
+               pg_log_warning("--link mode was used; any
modifications to the output "
+                                          "directory may
destructively modify input directories");
+       }

Regards,
Vignesh

On Thu, Feb 27, 2025 at 4:50 AM vignesh C <vignesh21@gmail.com> wrote:

Few comments:
1) The new file 010_links.pl added should be included to meson.build also:
'tests': [
't/010_pg_basebackup.pl',
't/011_in_place_tablespace.pl',
't/020_pg_receivewal.pl',
't/030_pg_recvlogical.pl',
't/040_pg_createsubscriber.pl',
],

Also give it a different number than any existing file -- if we
already have an 010 in that directory then make this one something
else. 012, 050, or whatever makes most sense.

--
Robert Haas
EDB: http://www.enterprisedb.com

Thanks for the new round of reviews!

1) The new file 010_links.pl added should be included to meson.build also

Added 010_links.pl to src/bin/pg_combinebackup/meson.build.

2) Since it is a new file, "Copyright (c) 2021-2025" should be
"Copyright (c) 2025"

Done!

3) Since it is a single statement, no need of braces here

I missed removing the braces along with the warning hint in
the previous version. Adjusted.

Also give it a different number than any existing file -- if we
already have an 010 in that directory then make this one something
else. 012, 050, or whatever makes most sense.

Oh, t/010_links.pl was not conflicting with anything.
The snippet that Vignesh quoted in his reply was from the
meson.build file of pg_basebackup instead of from
pg_combinebackup.

Attaching the new version of the patch in this reply.

I had to slightly change 010_links.pl and copy_file.c to
properly handle Windows, as the meson tests on
Windows had complaints with the code of v3:

* copy_file.c was forcing CopyFile on Windows regardless
of the option chosen by the user. Now, to make that behavior
backward compatible, I'm only forcing CopyFile on Windows
when the copy method is not --link. That allows my patch to
work, and doesn't change the previous behavior.
* Had to normalize paths when performing string matching in
the test that verifies the hard link count on files.

Best regards,
Israel.

On Mon, Mar 3, 2025 at 9:00 AM Israel Barth Rubio <barthisrael@gmail.com> wrote:

2) Since it is a new file, "Copyright (c) 2021-2025" should be
"Copyright (c) 2025"

Done!

For the record, this proposed change is not a project policy, AIUI. I
don't care very much what we do here, but -1 for kibitzing the range
of years people put in the copyright header.

Attaching the new version of the patch in this reply.

I had to slightly change 010_links.pl and copy_file.c to
properly handle Windows, as the meson tests on
Windows had complaints with the code of v3:

* copy_file.c was forcing CopyFile on Windows regardless
of the option chosen by the user. Now, to make that behavior
backward compatible, I'm only forcing CopyFile on Windows
when the copy method is not --link. That allows my patch to
work, and doesn't change the previous behavior.
* Had to normalize paths when performing string matching in
the test that verifies the hard link count on files.

When I looked at the code for this test, the questions that sprang to
mind for me were:

1. Is this really going to be stable?
2. Is this going to be too expensive?

With regard to the second question, why does this test need to create
three tables with a million rows each instead of some small number of
rows? If you need to fill multiple blocks, consider making the rows
wider instead of inserting such a large number.

With regard to the first question, I'm going to say that the answer is
"no" because when I run this test locally, I get this:

<lots of output omitted>
[12:26:07.979](0.000s) ok 973 - File
'/Users/robert.haas/pgsql-meson/testrun/pg_combinebackup/010_links/data/t_010_links_restore_data/pgdata/base/5/2664'
has 2 hard links
[12:26:07.980](0.000s) ok 974 - File
'/Users/robert.haas/pgsql-meson/testrun/pg_combinebackup/010_links/data/t_010_links_restore_data/pgdata/base/5/1249_vm'
has 2 hard links
[12:26:07.980](0.000s) ok 975 - File
'/Users/robert.haas/pgsql-meson/testrun/pg_combinebackup/010_links/data/t_010_links_restore_data/pgdata/base/5/2836'
has 2 hard links
[12:26:07.980](0.000s) ok 976 - File
'/Users/robert.haas/pgsql-meson/testrun/pg_combinebackup/010_links/data/t_010_links_restore_data/pgdata/base/5/2663'
has 2 hard links
[12:26:07.980](0.000s) ok 977 - File
'/Users/robert.haas/pgsql-meson/testrun/pg_combinebackup/010_links/data/t_010_links_restore_data/pgdata/base/5/6237'
has 2 hard links
Use of uninitialized value $file in stat at
/Users/robert.haas/pgsql/src/bin/pg_combinebackup/t/010_links.pl line
226.

I'm not sure whether that "Use of uninitialized value" message at the
end is killing the script at that point or whether it's just a
warning, but it should be cleaned up either way. For the rest, I'm not
a fan of testing every single file in the data directory like this. It
seems sufficient to me to test two files, one that you expect to
definitely be modified and one that you expect to definitely be not
modified. That assumes that you can guarantee that the file definitely
wasn't modified, which I'm not quite sure about. The test doesn't seem
to disable autovacuum, so what would keep autovacuum from sometimes
processing that table after the full backup and before the incremental
backup, and sometimes not? But there's something else wrong here, too,
because even when I adjusted the test to disable autovacuum, it still
failed in the same way as shown above.

Also, project style is uncuddled braces and lines less than 80 where
possible. So don't do this:

for my $test_3_segment (@test_3_segments) {

The curly brace belongs on the next line. Similarly this should be
three separate lines:

} else {

Regarding long lines, some of these cases are easy to fix:

my $query = "SELECT pg_relation_filepath(oid) FROM pg_class WHERE
relname = '%s';";

This could be fixed by writing my $query = <<EOM;

my $pg_attribute_path = $primary->safe_psql('postgres',
sprintf($query, 'pg_attribute'));

This could be fixed by moving the sprintf() down a line and indenting
it under 'postgres'.

Attached is a small delta patch to fix a few other issues. It does the
following:

* adds a serial comma to pg_combinebackup.sgml. This isn't absolutely
mandatory but we usually prefer this style.

* puts the new -k option in proper alphabetical order in several places

* changes the test in copy_file() to test for == COPY_METHOD_COPY
instead of != COPY_METHOD_COPYFILE and updates the comment to reflect
what I believe the actual reasoning to be

--
Robert Haas
EDB: http://www.enterprisedb.com

With regard to the second question, why does this test need to create
three tables with a million rows each instead of some small number of
rows? If you need to fill multiple blocks, consider making the rows
wider instead of inserting such a large number.

Makes sense! Changed that to use tables with wider rows, but less
rows.

With regard to the first question, I'm going to say that the answer is
"no" because when I run this test locally, I get this:

Use of uninitialized value $file in stat at
/Users/robert.haas/pgsql/src/bin/pg_combinebackup/t/010_links.pl line
226.

I'm not sure whether that "Use of uninitialized value" message at the
end is killing the script at that point or whether it's just a
warning, but it should be cleaned up either way.

That's unexpected. It seems somehow the function was called with a
"null" value as the argument.

For the rest, I'm not
a fan of testing every single file in the data directory like this. It
seems sufficient to me to test two files, one that you expect to
definitely be modified and one that you expect to definitely be not
modified. That assumes that you can guarantee that the file definitely
wasn't modified, which I'm not quite sure about. The test doesn't seem
to disable autovacuum, so what would keep autovacuum from sometimes
processing that table after the full backup and before the incremental
backup, and sometimes not? But there's something else wrong here, too,
because even when I adjusted the test to disable autovacuum, it still
failed in the same way as shown above.

Right, maybe the previous test was trying to do much more than it
should.

I've refactored the test to focus only on the user tables that we create
and use in the test.

I've also added `autovacuum = off` to the configuration, just in case.

Also, project style is uncuddled braces and lines less than 80 where
possible. So don't do this:

for my $test_3_segment (@test_3_segments) {

The curly brace belongs on the next line. Similarly this should be
three separate lines:

} else {

Regarding long lines, some of these cases are easy to fix:

my $query = "SELECT pg_relation_filepath(oid) FROM pg_class WHERE
relname = '%s';";

This could be fixed by writing my $query = <<EOM;

my $pg_attribute_path = $primary->safe_psql('postgres',
sprintf($query, 'pg_attribute'));

This could be fixed by moving the sprintf() down a line and indenting
it under 'postgres'.

Oh, I thought the styling guide from the Postgres wiki would apply to
the .c/.h files from the source code. Not sure why I got to that conclusion,
but I applied the styling guide to the perl files in the new version of the
patch.

Attached is a small delta patch to fix a few other issues. It does the
following:

* adds a serial comma to pg_combinebackup.sgml. This isn't absolutely
mandatory but we usually prefer this style.

* puts the new -k option in proper alphabetical order in several places

* changes the test in copy_file() to test for == COPY_METHOD_COPY
instead of != COPY_METHOD_COPYFILE and updates the comment to reflect
what I believe the actual reasoning to be

Thanks for the patch with the suggestions.
About the last one, related to the copy method, my first thought was to do
something like you did (in the sense of using == instead of !=). However, I
was concerned that I would change the previous behavior. But I do prefer
how it stands in your suggestion, thanks!

I'm attaching the v5 of the patch now.

Best regards,
Israel.

On Tue, Mar 4, 2025 at 7:07 AM Israel Barth Rubio <barthisrael@gmail.com> wrote:

About the last one, related to the copy method, my first thought was to do
something like you did (in the sense of using == instead of !=). However, I
was concerned that I would change the previous behavior. But I do prefer
how it stands in your suggestion, thanks!

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. */

I'm attaching the v5 of the patch now.

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.

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().

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.

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?

--
Robert Haas
EDB: http://www.enterprisedb.com

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.

On Wed, Mar 5, 2025 at 9:47 AM Israel Barth Rubio <barthisrael@gmail.com> wrote:

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.

Cool. Here is v7, with minor changes. I rewrote the commit message,
renamed the test case to 010_hardlink.pl, and adjusted some of the
comments in the test case.

I'm happy with this now, so barring objections or complaints from CI,
I plan to commit it soon.

--
Robert Haas
EDB: http://www.enterprisedb.com

I'm happy with this now, so barring objections or complaints from CI,
I plan to commit it soon.

Great, thank you!

On Thu, Mar 6, 2025 at 6:18 AM Israel Barth Rubio <barthisrael@gmail.com> wrote:

I'm happy with this now, so barring objections or complaints from CI,
I plan to commit it soon.

Great, thank you!

"Soon" ended up being somewhat later than planned, because I've been
out of town, but done now.

Now, to see if the buildfarm explodes...

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Mar 17, 2025 at 02:10:14PM -0400, Robert Haas wrote:

Now, to see if the buildfarm explodes...

I think koel might complain. pgindent on my machine yields the following:

diff --git a/src/bin/pg_combinebackup/copy_file.c b/src/bin/pg_combinebackup/copy_file.c
index 97ecda5a66d..b0c94f6ee31 100644
--- a/src/bin/pg_combinebackup/copy_file.c
+++ b/src/bin/pg_combinebackup/copy_file.c
@@ -72,9 +72,10 @@ copy_file(const char *src, const char *dst,
     }

 #ifdef WIN32
+
     /*
-     * We have no specific switch to enable CopyFile on Windows, because
-     * it's supported (as far as we know) on all Windows machines. So,
+     * We have no specific switch to enable CopyFile on Windows, because it's
+     * supported (as far as we know) on all Windows machines. So,
      * automatically enable it unless some other strategy was selected.
      */
     if (copy_method == COPY_METHOD_COPY)

--
nathan

On Mon, Mar 17, 2025 at 2:17 PM Nathan Bossart <nathandbossart@gmail.com> wrote:

I think koel might complain. pgindent on my machine yields the following:

Indeed, it did. I still think it's a mistake to have testing for this
in the buildfarm and not in 'meson test' or CI.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2025-03-17 Mo 4:07 PM, Robert Haas wrote:

On Mon, Mar 17, 2025 at 2:17 PM Nathan Bossart <nathandbossart@gmail.com> wrote:

I think koel might complain. pgindent on my machine yields the following:

Indeed, it did. I still think it's a mistake to have testing for this
in the buildfarm and not in 'meson test' or CI.

Maybe we could have a TAP test module that would run it but only if you
have "code-indent" in your PG_TEST_EXTRA.

cheers

andre

--
Andrew Dunstan
EDB: https://www.enterprisedb.com