MIME-Version: 1.0
References: <CALDaNm0YSiLpjCmajwLfidQrFOrLNKPQir7s__PeVvh9U3uoTQ@mail.gmail.com>
 <87cz2otiw3.fsf@wibble.ilmari.org> <ZG/jlbT0eIME+MFW@paquier.xyz>
 <CALDaNm1sDD-MCW6zrxHCC7Ka5AzO-L4ejxeeTMtXPPN+F4vz2Q@mail.gmail.com>
 <c44dfd51-96fe-d3aa-8f8a-07946d052fdc@dunslane.net> <CALDaNm3SWpFDTs1oRRbOAfYJNuSPMCLu4O3mLnU-tKoSFgGckQ@mail.gmail.com>
 <ZHu852YlGAk7AVk0@paquier.xyz>
In-Reply-To: <ZHu852YlGAk7AVk0@paquier.xyz>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 6 Jun 2023 08:05:49 +0530
Message-ID: <CAA4eK1Kbyo-L3nfMnZbusgv_a7jejxcm1gsDBJ2VH5KnK1rkTw@mail.gmail.com>
Subject: Re: Implement generalized sub routine find_in_log for tap test
To: Michael Paquier <michael@paquier.xyz>
Cc: vignesh C <vignesh21@gmail.com>, Andrew Dunstan <andrew@dunslane.net>, 
	=?UTF-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1Kbyo-L3nfMnZbusgv_a7jejxcm1gsDBJ2VH5KnK1rkTw%40mail.gmail.com>
Precedence: bulk

On Sun, Jun 4, 2023 at 3:51=E2=80=AFAM Michael Paquier <michael@paquier.xyz=
> wrote:
>
> On Mon, May 29, 2023 at 07:49:52AM +0530, vignesh C wrote:
> > Thanks for the comment, the attached v3 version patch has the changes
> > for the same.
>
> -if (find_in_log(
> -               $node, $log_offset,
> -               qr/peer authentication is not supported on this platform/=
))
> +if ($node->log_contains(
> +               qr/peer authentication is not supported on this platform/=
),
> +               $log_offset,)
>
> This looks like a typo to me, the log offset is eaten.
>
> Except of that, I am on board with log_contains().
>
> There are two things that bugged me with the refactoring related to
> connect_ok and connect_fails:
> - check_connect_log_contents() is a name too complicated.  While
> looking at that I have settled to a simpler log_check(), as we could
> perfectly use that for something else than connections as long as we
> want to check multiple patterns at once.
> - The refactoring of the documentation for the routines of Cluster.pm
> became incorrect.  For example, the patch does not list anymore
> log_like and log_unlike for connect_ok.
>
> With all that in mind I have hacked a few adjustments in a 0003,
> though I agree with the separation between 0001 and 0002.
>
> 033_replay_tsp_drops and 019_replslot_limit are not new to v16, but
> 003_peer.pl and 035_standby_logical_decoding.pl, making the number of
> places where find_in_log() exists twice as much.  So I would be
> tempted to refactor these tests in v16.  Perhaps anybody from the RMT
> could comment?  We've usually been quite flexible with the tests even
> in beta.
>

Personally, I don't see any problem to do this refactoring for v16.
However, sometimes, we do decide to backpatch refactoring in tests to
avoid backpatch effort. I am not completely sure if that is the case
here.

--=20
With Regards,
Amit Kapila.