Horton

Re: Backporting BackgroundPsql

Daniel Gustafsson <daniel@yesql.se>

View thread View on postgr.es
From: Daniel Gustafsson <daniel@yesql.se>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>, Heikki Linnakangas <hlinnaka@iki.fi>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Andrew Dunstan <andrew@dunslane.net>, Melanie Plageman <melanieplageman@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Mikael Kjellström <mikael.kjellstrom@gmail.com>
Date: 2024-06-25T20:47:11Z
Lists: pgsql-hackers 
> On 25 Jun 2024, at 16:26, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Andres Freund <andres@anarazel.de> writes:
>> On 2024-06-25 13:26:23 +0300, Heikki Linnakangas wrote:
>>> 1. Write the new test differently on backbranches. Before 664d757531, the
>>> test needs to work a lot harder to use the background psql session, calling
>>> pump() etc. That's doable, but as noted in the discussion that led to
>>> 664d757531, it's laborious and error-prone.
>>> 
>>> 2. Backport commit 664d757531. This might break out-of-tree perl tests that
>>> use the background_psql() function. I don't know if any such tests exist,
>>> and they would need to be changed for v17 anyway, so that seems acceptable.
>>> Anyone aware of any extensions using the perl test modules?
>>> 
>>> 3. Backport commit 664d757531, but keep the existing background_psql()
>>> function unchanged. Add a different constructor to get the v17-style
>>> BackgroundPsql session, something like "$node->background_psql_new()".
> 
>> Yes, I've wished for this a couple times. I think 2 or 3 would be reasonable.
>> I think 1) often just leads to either tests not being written or being
>> fragile...
> 
> I'd vote for (2).  (3) is just leaving a foot-gun for people to
> hurt themselves with.

I agree with this, if we're backporting we should opt for 2.  The only out of
tree user of background_psql() that I could find was check_pgactivity but they
seem to have vendored an old copy of our lib rather than use anything in a our
tree so we should be fine there.

Before pulling any triggers I think https://commitfest.postgresql.org/48/4959/
should be considered, since Tom found some flaws in the current code around how
timers and timeouts are used.

However, since Andrew is actively aiming to replace all of this shortly, should
we wait a see where that lands to avoid having to backport another library
change?

--
Daniel Gustafsson

Commits

  1. Fix missing installation/uninstallation rules for BackgroundPsql.pm

  2. Backport BackgroundPsql perl test module

  3. Add missing uninstallation rule for BackgroundPsql.pm

  4. Fix missing installation rules for BackgroundPsql.pm

  5. Skip \password TAP test on old IPC::Run versions

  6. Test SCRAM iteration changes with psql \password

  7. Refactor background psql TAP functions