On Fri, Jun 11, 2021 at 08:31:30PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> AFAICS, none of the other callers of sub reload are doing anything
> special to wait for it to take effect. Not sure why this one
> would need more protection. If we do need a fix for that, likely
> sub reload itself had better do it.
Perhaps.
>> +my $logfile = slurp_file($node_subscriber->logfile());
>> +ok( $logfile =~
>> + qr/logical replication did not find row to be updated in replication target relation "tab_full_pk"/,
>> + 'update target row is missing');
>> I think that it would be better here to grab the log location before
>> running the queries that will be matched with the logs, then pass down
>> the location to slurp_file().
>
> This coding is also stolen verbatim from other test scripts.
> What is your concern about it exactly?
My main concern are future changes in this test suite that could cause
more queries to generate logs that overlap with this sequence of
tests, falsifying what this test is checking.
> It's not like the
> tests have such varying amounts of log output that an unexpected
> log rotation would be likely. (Does the default TAP setup even
> allow log rotation at all? I doubt it.)
The default TAP configuration does not use log rotation, because it
causes complications in the test code as it makes necessary wait
phases to make sure that the logs have rotated. That's also more
costly than just getting the log position, run the queries and
slurp()'ing the file from the previous position.
--
Michael