Re: Implement generalized sub routine find_in_log for tap test

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