Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1q2sde-00006d-H4 for pgsql-hackers@arkaria.postgresql.org; Sat, 27 May 2023 12:03:14 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1q2scd-00080z-BY for pgsql-hackers@arkaria.postgresql.org; Sat, 27 May 2023 12:02:11 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1q2scd-00080o-2O for pgsql-hackers@lists.postgresql.org; Sat, 27 May 2023 12:02:11 +0000 Received: from relay1-d.mail.gandi.net ([217.70.183.193]) by magus.postgresql.org with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1q2scU-002VVY-L2 for pgsql-hackers@lists.postgresql.org; Sat, 27 May 2023 12:02:09 +0000 Received: by mail.gandi.net (Postfix) with ESMTPSA id AD84C240003; Sat, 27 May 2023 12:01:59 +0000 (UTC) Content-Type: multipart/alternative; boundary="------------1TvyH5Ym8aVCSuGD7ByL4hqz" Message-ID: Date: Sat, 27 May 2023 08:01:58 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 Subject: Re: Implement generalized sub routine find_in_log for tap test Content-Language: en-US To: vignesh C , Michael Paquier Cc: =?UTF-8?Q?Dagfinn_Ilmari_Manns=c3=a5ker?= , PostgreSQL Hackers References: <87cz2otiw3.fsf@wibble.ilmari.org> From: Andrew Dunstan In-Reply-To: List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This is a multi-part message in MIME format. --------------1TvyH5Ym8aVCSuGD7ByL4hqz Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 2023-05-26 Fr 20:35, vignesh C wrote: > On Fri, 26 May 2023 at 04:09, Michael Paquier wrote: >> On Thu, May 25, 2023 at 06:34:20PM +0100, Dagfinn Ilmari Mannsåker wrote: >>> 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. >> Hmm. connect_ok() and connect_fails() in Cluster.pm have a similar >> log comparison logic, feeding from the offset of a log file. Couldn't >> you use the same code across the board for everything? Note that this >> stuff is parameterized so as it is possible to check if patterns match >> or do not match, for multiple patterns. It seems to me that we could >> use the new log finding routine there as well, so how about extending >> it a bit more? You would need, at least: >> - One parameter for log entries matching. >> - One parameter for log entries not matching. > I felt adding these to log_contains was making the function slightly > complex with multiple checks. I was not able to make it simple with > the approach I tried. How about having a common function > check_connect_log_contents which has the common log contents check for > connect_ok and connect_fails function like the v2-0002 patch attached. +    $offset = 0 unless defined $offset; This is unnecessary, as slurp_file() handles it appropriately, and in fact doing this is slightly inefficient, as it will cause slurp_file to do a redundant seek. FYI there's a simpler way to say it if we wanted to:     $offset //= 0; cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com --------------1TvyH5Ym8aVCSuGD7ByL4hqz Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit


On 2023-05-26 Fr 20:35, vignesh C wrote:
On Fri, 26 May 2023 at 04:09, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, May 25, 2023 at 06:34:20PM +0100, Dagfinn Ilmari Mannsåker wrote:

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.

Hmm.  connect_ok() and connect_fails() in Cluster.pm have a similar
log comparison logic, feeding from the offset of a log file.  Couldn't
you use the same code across the board for everything?  Note that this
stuff is parameterized so as it is possible to check if patterns match
or do not match, for multiple patterns.  It seems to me that we could
use the new log finding routine there as well, so how about extending
it a bit more?  You would need, at least:
- One parameter for log entries matching.
- One parameter for log entries not matching.

I felt adding these to log_contains was making the function slightly
complex with multiple checks. I was not able to make it simple with
the approach I tried. How about having a common function
check_connect_log_contents which has the common log contents check for
connect_ok and connect_fails function like the v2-0002 patch attached.


+    $offset = 0 unless defined $offset;


This is unnecessary, as slurp_file() handles it appropriately, and in fact doing this is slightly inefficient, as it will cause slurp_file to do a redundant seek.

FYI there's a simpler way to say it if we wanted to:

    $offset //= 0;


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com
--------------1TvyH5Ym8aVCSuGD7ByL4hqz--