Обсуждение: custom postgres launcher for tests

On Tue, 11 Feb 2020 at 19:33, Ivan Taranov <i.taranov@postgrespro.ru> wrote:
>
> This patch allow to use custom postgres launcher for tests (tap®ress)
> by setting  environment variable PGLAUNCHER.

I thought I saw a related patch to this that proposed to add a pg_ctl
argument. Was that you too? I can't find it at the moment.

In any case I *very definitely* want the ability to wrap the
'postgres' executable we launch via some wrapper command. I currently
do this with a horrid shellscript hack that uses next-on-path lookups
and a wrapper 'postgres' executable. But because of how
find_other_exec works this is rather far from optimal so I'm a fan of
a cleaner, built-in alternative.

I was initially going to object to using an env-var. But if someone
controls your environment they can already LD_PRELOAD you or PATH-hack
into doing whatever they want, so it's not a security concern. And
you're right that getting a pg_ctl command line or the like into all
the places where we'd want it deep inside TAP tests etc is just too
much hassle otherwise.

I haven't reviewed the code yet, but for the idea a strong +1000 or so.

-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise

On Fri, Feb 21, 2020 at 4:49 AM Craig Ringer <craig@2ndquadrant.com> wrote:

> I thought I saw a related patch to this that proposed to add a pg_ctl
> argument. Was that you too? I can't find it at the moment.

This very simple two-line patch for src/test/perl/PostgresNode.pm code,
it set `pg_ctl -p <path>`  argument, and one-line patch for
src/test/regress/pg_regress.c it spawn postgres-launcher directly.

This routine usable only for tap tests with used
PostgresNode::get_new_node/start/restart, and for regress tests.

Perhaps the name TEST_PGLAUNCHER is more correct for this env-var.

>into doing whatever they want, so it's not a security concern

>I currently do this with a horrid shellscript hack that uses next-on-path
>lookups and a wrapper 'postgres' executable

Its not security problem, because this kit only for developer, commonly - for
iteratively build and run concrete tests.

For more complexy replacing need patch for pg_ctl, or postgres wrapper, or
replacing postgres bin  and other ways...

Thanks for the response!

On Fri, 21 Feb 2020 at 17:05, Ivan N. Taranov <i.taranov@postgrespro.ru> wrote:
>
> On Fri, Feb 21, 2020 at 4:49 AM Craig Ringer <craig@2ndquadrant.com> wrote:
>
> > I thought I saw a related patch to this that proposed to add a pg_ctl
> > argument. Was that you too? I can't find it at the moment.

I've had it on my TODO forever but I don't think it was me who posted
a patch. I honestly can't even remember. Too much going on at once.

> This routine usable only for tap tests with used
> PostgresNode::get_new_node/start/restart, and for regress tests.
>
> Perhaps the name TEST_PGLAUNCHER is more correct for this env-var.
>
> >into doing whatever they want, so it's not a security concern
>
> >I currently do this with a horrid shellscript hack that uses next-on-path
> >lookups and a wrapper 'postgres' executable
>
> Its not security problem, because this kit only for developer, commonly - for
> iteratively build and run concrete tests.
>
> For more complexy replacing need patch for pg_ctl, or postgres wrapper, or
> replacing postgres bin  and other ways...

If we support a wrapper we should support it for all pg_ctl usage IMO.
Even if it's intended just for testing. Because the scope of "testing"
extends very far outside "in-core TAP and pg_regress tests". Testing
needs include extensions running their own tests under valgrind or
similar tools, tests simulating clustered environments using ansible
or other automation tools, and more.

So I'd rather stick with your original PGLAUNCHER proposal. I think
all the tools we care about already invoke postgres via pg_ctl, and
any that don't should probably be taught to.

(I wish pg_ctl had a --no-daemon, --foreground or --no-detach mode
though, to help with this.)

For the sake of others with similar needs I attach my current
wrapper/launcher script. To use it you have to:

mkdir pglauncher
cd pglauncher
cp $the_script postgres
chmod a+x postgres
ln -s postgres initdb
ln -s postgres pg_ctl
cd ..

Then ensure the bin directory for your target postgres is first on the
PATH and run with something like:

POSTGRES_SRC=/path/to/your/srcdir

PATH=${PWD}/pglauncher/:$PATH \
  VG_LOG="$(mktemp -p . -d valgrind-log-XXXXXX)/%n-%p.log"
  VG_ARGS="--verbose --leak-check=full
--show-leak-kinds=definite,possible --track-origins=yes
--read-var-info=yes --show-error-list=yes --num-callers=30
--malloc-fill=8f --free-fill=9f
--suppressions=$POSTGRES_SRC/src/tools/valgrind.supp
--gen-suppressions=all" \
  pg_ctl blah blah

(BTW, we should install "valgrind.supp" when we install the optional
bits and pieces like the src/test/perl modules, pg_regress, and so on,
so it's available for extensions that run their own valgrind scans.)

-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise

> If we support a wrapper we should support it for all pg_ctl usage IMO.

As i understand it  - you propose to patch pg_ctl.c & regress.c instead of
PostgresNode.pm & regress.c?

This is a deeper invasion to  pg_ctl. There will be a conflict between the
environment variable and the pg_ctl -p parameter, and possibly induced
bugs.

I suggest microscopic changes for iterative  recompilation/test/debug
without installation.

> For the sake of others with similar needs I attach my current
> wrapper/launcher script. To use it you have to:

IMHO, the method what you proposed (wrapper/launcher) - is more suitable for
complex testing.

I agree that the my proposed way  is incomplete.

> If we support a wrapper we should support it for all pg_ctl usage IMO.

As i understand it  - you propose to patch pg_ctl.c & regress.c instead of
PostgresNode.pm & regress.c?

This is a deeper invasion to  pg_ctl. There will be a conflict between the
environment variable and the pg_ctl -p parameter, and possibly induced
bugs.

I suggest microscopic changes for iterative  recompilation/test/debug
without installation.

> For the sake of others with similar needs I attach my current
> wrapper/launcher script. To use it you have to:

IMHO, the method what you proposed (wrapper/launcher) - is more suitable for
complex testing.

I agree that the my proposed way  is incomplete.