Re: Stronger safeguard for archive recovery not to miss data

On 2021/04/04 11:58, osumi.takamichi@fujitsu.com wrote:
>> IMO it's better to comment why this server restart is necessary.
>> As far as I understand correctly, this is necessary to ensure the WAL file
>> containing the record about the change of wal_level (to minimal) is archived,
>> so that the subsequent archive recovery will be able to replay it.
> OK, added some comments. Further, I felt the way I wrote this part was not good at all and self-evident
> and developers who read this test would feel uneasy about that point.
> So, a little bit fixed that test so that we can get clearer conviction for wal archive.

LGTM. Thanks for updating the patch!

Attached is the updated version of the patch. I applied the following changes.
Could you review this version? Barring any objection, I'm thinking to
commit this.

+sub check_wal_level
+{
+    my ($target_wal_level, $explanation) = @_;
+
+    is( $node->safe_psql(
+            'postgres', q{show wal_level}),
+        $target_wal_level,
+        $explanation);

Do we really need this test? This test doesn't seem to be directly related
to the test purpose. It seems to be testing the behavior of the PostgresNode
functions. So I removed this from the patch.

+# This protection should apply to recovery mode
+my $another_node = get_new_node('another_node');

The same test is performed twice with different settings. So isn't it better
to merge the code for both tests into one common function, to simplify
the code? I did that.

I also applied some cosmetic changes.


>>> By the way, when I build postgres with this patch and enable-coverage
>>> option, the results of RT becomes unstable. Does someone know the
>> reason ?
>>> When it fails, I get stderr like below
>>
>> I have no idea about this. Does this happen even without the patch?
> Unfortunately, no. I get this only with --enable-coverage and with my patch,
> althought regression tests have passed with this patch.
> OSS HEAD doesn't produce the stderr even with --enable-coverage.

Could you check whether the latest patch still causes this issue or not?
If it still causes, could you check which part (the change of xlog.c or
the addition of regression test) caused the issue?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION