Thread: [HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Mon, Feb 27, 2017 at 11:28 AM, Craig Ringer <craig@2ndquadrant.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 outis thrown as psql timed out at /usr/share/perl5/vendor_perl/IPC/Run.pm line2961. 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
Re: [HACKERS] timeouts in PostgresNode::psql
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
Michael Paquier <michael.paquier@gmail.com> writes: > On Mon, Feb 27, 2017 at 11:28 AM, Craig Ringer <craig@2ndquadrant.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: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>\n\n when it should have been From: Dagfinn Ilmari Mannsåker <ilmari@ilmari.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 usea lighthouse. It's good to know where it is, but yougenerallydon'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@2ndquadrant.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@2ndquadrant.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.eisentraut@2ndquadrant.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