public inbox for [email protected]  

help / color / mirror / Atom feed

From: Dagfinn Ilmari Mannsåker <[email protected]>
To: vignesh C <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Implement generalized sub routine find_in_log for tap test
Date: Thu, 25 May 2023 18:34:20 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CALDaNm0YSiLpjCmajwLfidQrFOrLNKPQir7s__PeVvh9U3uoTQ@mail.gmail.com>
References: <CALDaNm0YSiLpjCmajwLfidQrFOrLNKPQir7s__PeVvh9U3uoTQ@mail.gmail.com>

vignesh C <[email protected]> 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.

> +	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;
    }

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.

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.

- ilmari

view thread (5+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Implement generalized sub routine find_in_log for tap test
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox