Re: timeouts in PostgresNode::psql

Hi all

While writing some tests I noticed that in newer IPC::Run or Perl
versions (haven't dug extensively to find out which), perl appends the
location to the extension, so 'ne' doesn't match the passed exception
string.

Pattern-match the exception string to handle this.

Bugfix, should be applied to 9.6 and master.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Amended patch attached after a few Perl-related comments I got on
private mail. Instead of

$exc_save !~ /^$timeout_exception.*/

I've updated to:

$exc_save !~ /^\Q$timeout_exception\E/

i.e. don't do an unnecessary wildcard match at the end, and disable
metachar interpretation in the substituted range.

Still needs applying to pg9.6 and pg10.

On Mon, Feb 27, 2017 at 11:28 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> Amended patch attached after a few Perl-related comments I got on
> private mail.

Out of curiosity, what are they?

> Instead of
>
> $exc_save !~ /^$timeout_exception.*/
>
> I've updated to:
>
> $exc_save !~ /^\Q$timeout_exception\E/
>
> i.e. don't do an unnecessary wildcard match at the end, and disable
> metachar interpretation in the substituted range.
>
> Still needs applying to pg9.6 and pg10.

I did not understand at first what you meant, but after looking at the
commit message of the patch things are clear:
Newer Perl or IPC::Run versions default to appending the filename to string
exceptions, e.g. the exception
psql timed out
is thrown as
psql timed out at /usr/share/perl5/vendor_perl/IPC/Run.pm line 2961.

And that's good to know, I can see as well that the patch is on the CF app:
https://commitfest.postgresql.org/13/
And that it has been marked as ready for committer.
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Mon, Feb 27, 2017 at 11:28 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>
>> Still needs applying to pg9.6 and pg10.
>
> I did not understand at first what you meant, but after looking at the
> commit message of the patch things are clear:
> Newer Perl or IPC::Run versions default to appending the filename to string
> exceptions, e.g. the exception
> psql timed out
> is thrown as
> psql timed out at /usr/share/perl5/vendor_perl/IPC/Run.pm line 2961.
>
> And that's good to know, I can see as well that the patch is on the CF app:
> https://commitfest.postgresql.org/13/
> And that it has been marked as ready for committer.

That was me reviewing it, but the email to the list bounced, because the
commitfest app (or the python email.message module, depending on who you
feel like blaming) doesn't handle non-ASCII characters in structured
headers correctly. In my case, it generated the header

From: =?utf-8?q?Dagfinn_Ilmari_Manns=C3=A5ker_=3Cilmari=40ilmari=2Eorg=3E?=\n\n

when it should have been

From: =?utf-8?q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari(at)ilmari(dot)org>

Anyway, my rewview was as follows:

Tested by copying the timeout example from the PostgresNode docs into a
test file and adding an assertion that the variable passed to the
'timed_out' parameter gets set. Without the patch the test script dies
when the timeout occurs, with the patch the test passes.

PostgresNode itself could do with some tests (nothing in the actual
tests seems to use the timeout functionality), but that's for another
patch.

--
"I use RMS as a guide in the same way that a boat captain would use
a lighthouse. It's good to know where it is, but you generally
don't want to find yourself in the same spot." - Tollef Fog Heen

Michael Paquier wrote:
> On Mon, Feb 27, 2017 at 11:28 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> > Instead of
> >
> > $exc_save !~ /^$timeout_exception.*/
> >
> > I've updated to:
> >
> > $exc_save !~ /^\Q$timeout_exception\E/
> >
> > i.e. don't do an unnecessary wildcard match at the end, and disable
> > metachar interpretation in the substituted range.
> >
> > Still needs applying to pg9.6 and pg10.
>
> I did not understand at first what you meant, but after looking at the
> commit message of the patch things are clear:
> Newer Perl or IPC::Run versions default to appending the filename to string
> exceptions, e.g. the exception
> psql timed out
> is thrown as
> psql timed out at /usr/share/perl5/vendor_perl/IPC/Run.pm line 2961.

Hmm, I think this is really a bugfix that we should backpatch all the
way back to where we introduced PostgresNode.

Lately I've been wondering about backpatching the whole TAP test
infrastructure, all the way back. As we notice bugs, it's really useful
to use newly added tests in all branches; but currently PostgresNode
doesn't work with old branches, particularly since the '-w' switch was
removed from pg_ctl invokations in PostgresNode->start and ->restart
methods -- (the test just fail without any indication of what is going
on).

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 02/28/2017 07:39 AM, Alvaro Herrera wrote:
> Lately I've been wondering about backpatching the whole TAP test
> infrastructure, all the way back. As we notice bugs, it's really useful
> to use newly added tests in all branches; but currently PostgresNode
> doesn't work with old branches, particularly since the '-w' switch was
> removed from pg_ctl invokations in PostgresNode->start and ->restart
> methods -- (the test just fail without any indication of what is going
> on).
>

+1

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 28 February 2017 at 20:39, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:

> Lately I've been wondering about backpatching the whole TAP test
> infrastructure, all the way back. As we notice bugs, it's really useful
> to use newly added tests in all branches; but currently PostgresNode
> doesn't work with old branches, particularly since the '-w' switch was
> removed from pg_ctl invokations in PostgresNode->start and ->restart
> methods -- (the test just fail without any indication of what is going
> on).

Yeah. I actually did prepare a backpatch of PostgresNode to 9.5 (with
room to go back further), and it's pretty simple if you don't care
about breaking all existing TAP tests ;)

The irritating bit is that the various TAP stuff introduced earlier
for src/bin and so on uses its own node management plus some helpers
in TestLib. Those helpers went away when we introduced PostgresNode
and they aren't easily emulated because the tests make assumptions
about the directory structure that don't fit well with PostgresNode's
way of doing things.

It's fixable, it just wasn't worth the hassle for what I needed given
that I saw the backport as having minimal chance of getting into core.

I've pushed what I did to
https://github.com/2ndQuadrant/postgres/tree/dev/backport-96-tap-to-95
in case anyone finds it useful.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2/26/17 21:28, Craig Ringer wrote:
> Amended patch attached after a few Perl-related comments I got on
> private mail. Instead of
>
> $exc_save !~ /^$timeout_exception.*/
>
> I've updated to:
>
> $exc_save !~ /^\Q$timeout_exception\E/
>
> i.e. don't do an unnecessary wildcard match at the end, and disable
> metachar interpretation in the substituted range.
>
> Still needs applying to pg9.6 and pg10.

committed to master and 9.6

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2 March 2017 at 03:19, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> On 2/26/17 21:28, Craig Ringer wrote:
>> Amended patch attached after a few Perl-related comments I got on
>> private mail. Instead of
>>
>> $exc_save !~ /^$timeout_exception.*/
>>
>> I've updated to:
>>
>> $exc_save !~ /^\Q$timeout_exception\E/
>>
>> i.e. don't do an unnecessary wildcard match at the end, and disable
>> metachar interpretation in the substituted range.
>>
>> Still needs applying to pg9.6 and pg10.
>
> committed to master and 9.6

Thanks.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services