pgsql: Provide a TLS init hook

Andrew Dunstan <andrew@dunslane.net> writes:

Provide a TLS init hook

Buildfarm's not terribly happy --- I suspect that the makefile for
the new test module is failing to link in libopenssl explicitly.
Some platforms are more forgiving of that than others.

regards, tom lane

I wrote:

Buildfarm's not terribly happy --- I suspect that the makefile for
the new test module is failing to link in libopenssl explicitly.

Concretely, I see that contrib/sslinfo has

SHLIB_LINK += $(filter -lssl -lcrypto -lssleay32 -leay32, $(LIBS))

which you probably need to crib here. There might be some analogous
magic in the MSVC files, too.

regards, tom lane

I wrote:

Concretely, I see that contrib/sslinfo has
SHLIB_LINK += $(filter -lssl -lcrypto -lssleay32 -leay32, $(LIBS))

I verified that that fixes things on macOS and pushed it, along with
a couple other minor fixes.

However, I'm quite desperately unhappy that the new test module
does this:

$node->append_conf('postgresql.conf', "listen_addresses = 'localhost'");

That's opening a security hole. Note that we do *not* run src/test/ssl
by default, and it has a README warning people not to run it on multiuser
systems. It seems 100% unacceptable for this test to fire up a similarly
insecure server without so much as a by-your-leave.

I don't actually see why we need the localhost port at all --- it doesn't
look like this test ever attempts to connect to the server. So couldn't
we just drop that?

regards, tom lane

On 3/25/20 7:44 PM, Tom Lane wrote:

I wrote:

Concretely, I see that contrib/sslinfo has
SHLIB_LINK += $(filter -lssl -lcrypto -lssleay32 -leay32, $(LIBS))

I verified that that fixes things on macOS and pushed it, along with
a couple other minor fixes.

Thanks.

However, I'm quite desperately unhappy that the new test module
does this:

$node->append_conf('postgresql.conf', "listen_addresses = 'localhost'");

That's opening a security hole. Note that we do *not* run src/test/ssl
by default, and it has a README warning people not to run it on multiuser
systems. It seems 100% unacceptable for this test to fire up a similarly
insecure server without so much as a by-your-leave.

I don't actually see why we need the localhost port at all --- it doesn't
look like this test ever attempts to connect to the server. So couldn't
we just drop that?

Seems reasonable. I just tested that and it seems quite happy, so I'll
make the change.

cheers

andrew

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 3/25/20 7:44 PM, Tom Lane wrote:

I don't actually see why we need the localhost port at all --- it doesn't
look like this test ever attempts to connect to the server. So couldn't
we just drop that?

Seems reasonable. I just tested that and it seems quite happy, so I'll
make the change.

Cool, thanks.

jacana has just exposed a different problem: it's not configured
--with-openssl, but the buildfarm script is trying to run this
new test module anyway. I'm confused about the reason.
"make installcheck" in src/test/modules does the right thing,
but seemingly that client is doing something different?

regards, tom lane

On 3/25/20 9:28 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 3/25/20 7:44 PM, Tom Lane wrote:

I don't actually see why we need the localhost port at all --- it doesn't
look like this test ever attempts to connect to the server. So couldn't
we just drop that?

Seems reasonable. I just tested that and it seems quite happy, so I'll
make the change.

Cool, thanks.

jacana has just exposed a different problem: it's not configured
--with-openssl, but the buildfarm script is trying to run this
new test module anyway. I'm confused about the reason.
"make installcheck" in src/test/modules does the right thing,
but seemingly that client is doing something different?

Ugh. I have put in place a hack to clear the error on jacana. Yes, the
client does something different so we can run each module separately.
Trawling through the output and files for one test on its own is hard
enough, I don't want to aggregate them.

cheers

andrew

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 3/25/20 9:28 PM, Tom Lane wrote:

jacana has just exposed a different problem: it's not configured
--with-openssl, but the buildfarm script is trying to run this
new test module anyway. I'm confused about the reason.
"make installcheck" in src/test/modules does the right thing,
but seemingly that client is doing something different?

Ugh. I have put in place a hack to clear the error on jacana. Yes, the
client does something different so we can run each module separately.
Trawling through the output and files for one test on its own is hard
enough, I don't want to aggregate them.

Well, I'm confused, because my own critters are running this as part
of a single make-installcheck-in-src/test/modules step, eg

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&amp;dt=2020-03-26%2002%3A09%3A08&amp;stg=testmodules-install-check-C

Why is jacana doing it differently?

regards, tom lane

On 3/26/20 9:50 AM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 3/25/20 9:28 PM, Tom Lane wrote:

jacana has just exposed a different problem: it's not configured
--with-openssl, but the buildfarm script is trying to run this
new test module anyway. I'm confused about the reason.
"make installcheck" in src/test/modules does the right thing,
but seemingly that client is doing something different?

Ugh. I have put in place a hack to clear the error on jacana. Yes, the
client does something different so we can run each module separately.
Trawling through the output and files for one test on its own is hard
enough, I don't want to aggregate them.

Well, I'm confused, because my own critters are running this as part
of a single make-installcheck-in-src/test/modules step, eg

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&amp;dt=2020-03-26%2002%3A09%3A08&amp;stg=testmodules-install-check-C

Why is jacana doing it differently?

longfin is also running it (first) here
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&amp;dt=2020-03-26%2014%3A39%3A51&amp;stg=ssl_passphrase_callback-check

That's where jacana failed.

I don't think this belongs in installcheck, we should add
'NO_INSTALLCHECK = 1' to the Makefile.

cheers

andrew

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 3/26/20 9:50 AM, Tom Lane wrote:

Why is jacana doing it differently?

longfin is also running it (first) here
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&amp;dt=2020-03-26%2014%3A39%3A51&amp;stg=ssl_passphrase_callback-check

Oh, I missed that. Isn't that pretty duplicative of the
testmodules-install phase?

I don't think this belongs in installcheck, we should add
'NO_INSTALLCHECK = 1' to the Makefile.

Why? The other src/test/modules/ modules with TAP tests do not
specify that, with the exception of commit_ts which has a solid
doesnt-work-in-the-default-configuration excuse.

regards, tom lane

On 3/26/20 11:31 AM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 3/26/20 9:50 AM, Tom Lane wrote:

Why is jacana doing it differently?

longfin is also running it (first) here
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&amp;dt=2020-03-26%2014%3A39%3A51&amp;stg=ssl_passphrase_callback-check

Oh, I missed that. Isn't that pretty duplicative of the
testmodules-install phase?

Yes, but see below

I don't think this belongs in installcheck, we should add
'NO_INSTALLCHECK = 1' to the Makefile.

Why? The other src/test/modules/ modules with TAP tests do not
specify that, with the exception of commit_ts which has a solid
doesnt-work-in-the-default-configuration excuse.

That seems wrong, installcheck should be testing against an installed
instance, and the TAP tests don't. Moreover, from the buildfarm's POV
it's completely wrong, as we call the installcheck targets multiple
times, once for each configured locale. See one of the animals that
tests multiple locales (e.g. crake or prion)

src/test is a mess, TBH, and I have spent quite some time trying to get
it so that we test everything but without duplication, clearly without
complete success.

cheers

andrew

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 3/26/20 11:31 AM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

I don't think this belongs in installcheck, we should add
'NO_INSTALLCHECK = 1' to the Makefile.

Why? The other src/test/modules/ modules with TAP tests do not
specify that, with the exception of commit_ts which has a solid
doesnt-work-in-the-default-configuration excuse.

That seems wrong, installcheck should be testing against an installed
instance, and the TAP tests don't.

So? We clearly document that for the TAP tests, "make installcheck"
means "use the installed executables, but run a new instance" [1]https://www.postgresql.org/docs/devel/regress-tap.html.

Moreover, from the buildfarm's POV
it's completely wrong, as we call the installcheck targets multiple
times, once for each configured locale. See one of the animals that
tests multiple locales (e.g. crake or prion)

Yeah. That's productive if you think the tests might be
locale-sensitive. I doubt that any of the ones under src/test/modules/
actually are at the moment, so maybe this is a waste of buildfarm effort.
But I don't think that it's the place of the Makefiles to dictate such
policy, and especially not for them to do so by breaking the ability to
use "make installcheck" at all.

regards, tom lane

[1]: https://www.postgresql.org/docs/devel/regress-tap.html

[discussion from -committers]

On 3/26/20 4:31 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 3/26/20 11:31 AM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

I don't think this belongs in installcheck, we should add
'NO_INSTALLCHECK = 1' to the Makefile.

Why? The other src/test/modules/ modules with TAP tests do not
specify that, with the exception of commit_ts which has a solid
doesnt-work-in-the-default-configuration excuse.

That seems wrong, installcheck should be testing against an installed
instance, and the TAP tests don't.

So? We clearly document that for the TAP tests, "make installcheck"
means "use the installed executables, but run a new instance" [1].

I think we were probably a bit shortsighted about that. But what's done
is done. I wonder if there is a simple way we could turn it off for the
buildfarm?

Moreover, from the buildfarm's POV
it's completely wrong, as we call the installcheck targets multiple
times, once for each configured locale. See one of the animals that
tests multiple locales (e.g. crake or prion)

Yeah. That's productive if you think the tests might be
locale-sensitive. I doubt that any of the ones under src/test/modules/
actually are at the moment, so maybe this is a waste of buildfarm effort.
But I don't think that it's the place of the Makefiles to dictate such
policy, and especially not for them to do so by breaking the ability to
use "make installcheck" at all.

regards, tom lane

[1] https://www.postgresql.org/docs/devel/regress-tap.html

Right now the explicit TAP test code in the buildfarm knows how to collect all the relevant output. The installcheck code doesn't know about that for TAP tests.

I get that developers want to be able to run tests in a small number of commands, but for the buildfarm I generally favor more disaggregated tests. That way if test X fails it's much easier to focus on the problem. (related note: I'm working on breaking up the log text blobs which will also help focussing on the right area).

Maybe we need to take the discussion to -hackers.

cheers

andrew

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

[discussion from -committers]

On 3/26/20 4:31 PM, Tom Lane wrote:

So? We clearly document that for the TAP tests, "make installcheck"
means "use the installed executables, but run a new instance" [1].

I think we were probably a bit shortsighted about that. But what's done
is done. I wonder if there is a simple way we could turn it off for the
buildfarm?

I think it was entirely intentional. I use "installcheck" all the time
to save the cost of repeatedly building an install tree. If anything,
it's more important to be able to do that when running a specific
subdirectory's tests than when testing the whole tree, because the
overhead would be worse in proportion. So sprinkling NO_INSTALLCHECK
liberally would make me sad. (In fact, I wonder if we should treat
that as only disabling traditional-framework tests not TAP tests.
The problem of tests requiring atypical configurations doesn't apply
to TAP tests.)

Right now the explicit TAP test code in the buildfarm knows how to collect all the relevant output. The installcheck code doesn't know about that for TAP tests.

It seems like what the buildfarm would like is a way to invoke TAP tests
and traditional-framework tests separately, so that it could apply special
tooling to the former. I'd have no objection to making that possible.

Alternatively, maybe you could just dig through the tree after-the-fact
looking for tmp_check subdirectories, and capturing their contents?

A separate issue is whether or not it's worth running all the
src/test/modules/ tests over again for multiple locales. ISTM the
answer is mostly "no", but I grant that some of them might be locale
sensitive. Maybe we could mark the ones that are in their Makefiles?
Get the buildfarm to look for "LOCALE_SENSITIVE = 1" or the like.
Right now, the modules/ tests don't run long enough for this to be super
important, but we might be more worried about their cost in future.

regards, tom lane

On 3/27/20 11:09 AM, Tom Lane wrote:

Right now the explicit TAP test code in the buildfarm knows how to collect all the relevant output. The installcheck code doesn't know about that for TAP tests.

It seems like what the buildfarm would like is a way to invoke TAP tests
and traditional-framework tests separately, so that it could apply special
tooling to the former. I'd have no objection to making that possible.

Exactly. I'll look into that, but I'm open to any ideas people have.

cheers

andrew

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 3/27/20 11:09 AM, Tom Lane wrote:

It seems like what the buildfarm would like is a way to invoke TAP tests
and traditional-framework tests separately, so that it could apply special
tooling to the former. I'd have no objection to making that possible.

Exactly. I'll look into that, but I'm open to any ideas people have.

With the makefile infrastructure, the first thing that comes to mind
is to support something like

make [install]check SKIP_TAP_TESTS=1

make [install]check SKIP_TRADITIONAL_TESTS=1

Don't know what to do in the MSVC world.

regards, tom lane