On Thu, 25 May 2023 at 23:04, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
>
> vignesh C <vignesh21@gmail.com> writes:
>
> > Hi,
> >
> > The recovery tap test has 4 implementations of find_in_log sub routine
> > for various uses, I felt we can generalize these and have a single
> > function for the same. The attached patch is an attempt to have a
> > generalized sub routine find_in_log which can be used by all of them.
> > Thoughts?
>
> +1 on factoring out this common code. Just a few comments on the implementation.
>
>
> > diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
> > index a27fac83d2..5c9b2f6c03 100644
> > --- a/src/test/perl/PostgreSQL/Test/Utils.pm
> > +++ b/src/test/perl/PostgreSQL/Test/Utils.pm
> > @@ -67,6 +67,7 @@ our @EXPORT = qw(
> > slurp_file
> > append_to_file
> > string_replace_file
> > + find_in_log
> > check_mode_recursive
> > chmod_recursive
> > check_pg_config
> > @@ -579,6 +580,28 @@ sub string_replace_file
> >
> > =pod
> >
> > +
> > +=item find_in_log(node, pattern, offset)
> > +
> > +Find pattern in logfile of node after offset byte.
> > +
> > +=cut
> > +
> > +sub find_in_log
> > +{
> > + my ($node, $pattern, $offset) = @_;
> > +
> > + $offset = 0 unless defined $offset;
> > + my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile);
>
> Since this function is in the same package, there's no need to qualify
> it with the full name. I know the callers you copied it from did, but
> they wouldn't have had to either, since it's exported by default (in the
> @EXPORT array above), unless the use statement has an explicit argument
> list that excludes it.
I have moved this function to Cluster.pm file now, since it is moved,
I had to qualify the name with the full name.
> > + return 0 if (length($log) <= 0 || length($log) <= $offset);
> > +
> > + $log = substr($log, $offset);
>
> Also, the existing callers don't seem to have got the memo that
> slurp_file() takes an optinal offset parameter, which will cause it to
> seek to that postion before slurping the file, which is more efficient
> than reading the whole file in and substr-ing it. There's not much
> point in the length checks either, since regex-matching against an empty
> string is very cheap (and if the provide pattern can match the empty
> string the whole function call is rather pointless).
>
> > + return $log =~ m/$pattern/;
> > +}
>
> All in all, it could be simplified to:
>
> sub find_in_log {
> my ($node, $pattern, $offset) = @_;
>
> return slurp_file($node->logfile, $offset) =~ $pattern;
> }
Modified in similar lines
> However, none of the other functions in ::Utils know anything about node
> objects, which makes me think it should be a method on the node itself
> (i.e. in PostgreSQL::Test::Cluster) instead. Also, I think log_contains
> would be a better name, since it just returns a boolean. The name
> find_in_log makes me think it would return the log lines matching the
> pattern, or the position of the match in the file.
Modified
> In that case, the slurp_file() call would have to be fully qualified,
> since ::Cluster uses an empty import list to avoid polluting the method
> namespace with imported functions.
Modified.
Thanks for the comments, the attached v2 version patch has the changes
for the same.
Regards,
Vignesh