> On 20 Dec 2024, at 11:01, Ashutosh Bapat <ashutosh.bapat....@gmail.com> wrote:
> On Wed, Dec 18, 2024 at 7:39 PM Daniel Gustafsson <dan...@yesql.se> wrote:
>>
>>> On 18 Dec 2024, at 12:28, Ashutosh Bapat <ashutosh.bapat....@gmail.com>
>>> wrote:
>> + if ( $ENV{PG_TEST_EXTRA}
>> + && $ENV{PG_TEST_EXTRA} =~ /\bregress_dump_test\b/)
>> Should this also test that $oldnode and $newnode have matching pg_version to
>> keep this from running in a cross-version upgrade test? While it can be
>> argued
>> that running this in a cross-version upgrade is breaking it and getting to
>> keep
>> both pieces, it's also not ideal to run a resource intensive test we know
>> will
>> fail. (It can't be done at this exact callsite, just picked to illustrate.)
>
> You already wrote it in parenthesis. At the exact callsite $oldnode
> and $newnode can not be of different versions. In fact newnode is yet
> to be created at this point. But $oldnode has the same version as the
> server run from the code. In a cross-version upgrade this test will
> not be executed. I am confused as to what this comment is about.
Sure, it can't be checked until $newnode is created, but it seems like a cheap
test to ensure it's not executed as part of someones cross-version tests.
>> + my $format_spec = substr($format, 0, 1);
>> This doesn't seem great for readability, how about storing the formats and
>> specfiers in an array of Perl hashes which can be iterated over with
>> descriptive names, like $format{'name'} and $format{'spec'}?
>
> Instead of an array of hashes, I used a single hash with format
> description as key and format spec as value. Hope that's acceptable.
LGTM.
--
Daniel Gustafsson
