Re: Add -k/--link option to pg_combinebackup

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