pgsql: Add 'basebackup_to_shell' contrib module.

Hi,

On 2022-03-15 17:33:12 +0000, Robert Haas wrote:

Add 'basebackup_to_shell' contrib module.

As a demonstration of the sort of thing that can be done by adding a
custom backup target, this defines a 'shell' target which executes a
command defined by the system administrator. The command is executed
once for each tar archive generate by the backup and once for the
backup manifest, if any. Each time the command is executed, it
receives the contents of th file for which it is executed via standard
input.

The configured command can use %f to refer to the name of the archive
(e.g. base.tar, $TABLESPACE_OID.tar, backup_manifest) and %d to refer
to the target detail (pg_basebackup --target shell:DETAIL). A target
detail is required if %d appears in the configured command and
forbidden if it does not.

Patch by me, reviewed by Abhijit Menon-Sen.

Modified Files
--------------
contrib/Makefile | 1 +
contrib/basebackup_to_shell/Makefile | 19 +
contrib/basebackup_to_shell/basebackup_to_shell.c | 419 ++++++++++++++++++++++
doc/src/sgml/basebackup-to-shell.sgml | 69 ++++
doc/src/sgml/contrib.sgml | 1 +
doc/src/sgml/filelist.sgml | 1 +
6 files changed, 510 insertions(+)

Seems like this ought to have at least some basic test to make sure it
actually works / keeps working?

Greetings,

Andres Freund

On Tue, Mar 15, 2022 at 3:04 PM Andres Freund <andres@anarazel.de> wrote:

Seems like this ought to have at least some basic test to make sure it
actually works / keeps working?

Wouldn't hurt, although it may be a little bit tricky to getting it
work portably. I'll try to take a look at it.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Mar 17, 2022 at 11:52 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Mar 15, 2022 at 3:04 PM Andres Freund <andres@anarazel.de> wrote:

Seems like this ought to have at least some basic test to make sure it
actually works / keeps working?

Wouldn't hurt, although it may be a little bit tricky to getting it
work portably. I'll try to take a look at it.

Here is a basic test. I am unable to verify whether it works on Windows.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On March 25, 2022 9:22:09 AM PDT, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 17, 2022 at 11:52 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Mar 15, 2022 at 3:04 PM Andres Freund <andres@anarazel.de> wrote:

Seems like this ought to have at least some basic test to make sure it
actually works / keeps working?

Wouldn't hurt, although it may be a little bit tricky to getting it
work portably. I'll try to take a look at it.

Here is a basic test. I am unable to verify whether it works on Windows.

Create a CF entry for it, or enable CI on a github repo?

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Fri, Mar 25, 2022 at 12:36 PM Andres Freund <andres@anarazel.de> wrote:

Create a CF entry for it, or enable CI on a github repo?

I created a CF entry for it. Then I had to try to Google around to
find the URL from the cfbot, because it's not even linked from
commitfest.postgresql.org for some reason. #blamemagnus

I don't think that the Windows CI is running the TAP tests for
contrib. At least, I can't find any indication of it in the output. So
it doesn't really help to assess how portable this test is, unless I'm
missing something.

I looked through the Linux output. It looks to me like that does run
the TAP tests for contrib. Unfortunately, the output is not in order
and is also not labelled, so it's hard to tell what output goes with
what contrib module. I named my test 001_basic.pl, but there are 12 of
those already. I see that 13 copies of 001_basic.pl seem to have
passed CI on Linux, so I guess the test ran and passed there. It seems
like it would be an awfully good idea to mention the subdirectory name
before each dump of output.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 3/25/22 13:52, Robert Haas wrote:

On Fri, Mar 25, 2022 at 12:36 PM Andres Freund <andres@anarazel.de> wrote:

Create a CF entry for it, or enable CI on a github repo?

I created a CF entry for it. Then I had to try to Google around to
find the URL from the cfbot, because it's not even linked from
commitfest.postgresql.org for some reason. #blamemagnus

I don't think that the Windows CI is running the TAP tests for
contrib. At least, I can't find any indication of it in the output. So
it doesn't really help to assess how portable this test is, unless I'm
missing something.

I looked through the Linux output. It looks to me like that does run
the TAP tests for contrib. Unfortunately, the output is not in order
and is also not labelled, so it's hard to tell what output goes with
what contrib module. I named my test 001_basic.pl, but there are 12 of
those already. I see that 13 copies of 001_basic.pl seem to have
passed CI on Linux, so I guess the test ran and passed there. It seems
like it would be an awfully good idea to mention the subdirectory name
before each dump of output.

Duplication of TAP test names has long been something that's annoyed me.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sat, Mar 26, 2022 at 6:52 AM Robert Haas <robertmhaas@gmail.com> wrote:

I don't think that the Windows CI is running the TAP tests for
contrib. At least, I can't find any indication of it in the output. So
it doesn't really help to assess how portable this test is, unless I'm
missing something.

Yeah :-( vcregress.pl doesn't yet have an easy way to run around and
find contrib modules with tap tests and run them, for the CI script to
call. (I think there was a patch somewhere? I've been bitten by the
lack of this recently...)

In case it's helpful, here's how to run a specific contrib module's
TAP test by explicitly adding it. That'll run once I post this email,
but I already ran in it my own github account and it looks like this:

https://cirrus-ci.com/task/5637156969381888
https://api.cirrus-ci.com/v1/artifact/task/5637156969381888/log/contrib/basebackup_to_shell/tmp_check/log/regress_log_001_basic
https://api.cirrus-ci.com/v1/artifact/task/5637156969381888/log/contrib/basebackup_to_shell/tmp_check/log/001_basic_primary.log

Hi,

On 2022-03-25 13:52:11 -0400, Robert Haas wrote:

On Fri, Mar 25, 2022 at 12:36 PM Andres Freund <andres@anarazel.de> wrote:

Create a CF entry for it, or enable CI on a github repo?

I created a CF entry for it. Then I had to try to Google around to
find the URL from the cfbot, because it's not even linked from
commitfest.postgresql.org for some reason. #blamemagnus

Yea, we really need to improve on that. I think Thomas has some hope of
improving things after the release...

I don't think that the Windows CI is running the TAP tests for
contrib. At least, I can't find any indication of it in the output. So
it doesn't really help to assess how portable this test is, unless I'm
missing something.

Yea. It's really unfortunate how vcregress.pl makes it hard to run all
tests. And we're kind of stuck finding a way forward. It's easy enough to work
around for individual tests by just adding them to the test file (like Thomas
did nearby), but clearly that doesn't scale. Andrew wasn't happy with
additional vcregress commands. The fact that vcregress doesn't run tests in
parallel makes things take forever. And so it goes on.

I looked through the Linux output. It looks to me like that does run
the TAP tests for contrib. Unfortunately, the output is not in order
and is also not labelled, so it's hard to tell what output goes with
what contrib module. I named my test 001_basic.pl, but there are 12 of
those already. I see that 13 copies of 001_basic.pl seem to have
passed CI on Linux, so I guess the test ran and passed there. It seems
like it would be an awfully good idea to mention the subdirectory name
before each dump of output.

Yea, the current output is *awful*.

FWIW, the way it's hard to run tests the same way across platforms, the crappy
output etc was one of the motivations behind the meson effort. If you just
compare the output from both *nix and windows runs today with the meson
output, it's imo night and day:

https://cirrus-ci.com/task/5869668815601664?logs=check_world#L67

That's a recent run where I'd not properly mirrored 7c51b7f7cc0, leading to a
failure on windows. Though it'd be more intersting to see a run with a
failure.

If one wants one can also see the test output of individual tests (it's always
logged to a file). But I typically find that not useful for a 'general' test
run, too much output. In that case there's a nice list of failed tests at the
end:

Summary of Failures:

144/219 postgresql:tap+vacuumlo / vacuumlo/t/001_basic.pl ERROR 0.48s (exit status 255 or signal 127 SIGinvalid)

Ok: 218
Expected Fail: 0
Fail: 1
Unexpected Pass: 0
Skipped: 0
Timeout: 0

Greetings,

Andres Freund

On Sat, Mar 26, 2022 at 9:55 AM Thomas Munro <thomas.munro@gmail.com> wrote:

https://api.cirrus-ci.com/v1/artifact/task/5637156969381888/log/contrib/basebackup_to_shell/tmp_check/log/regress_log_001_basic

This line doesn't look too healthy:

pg_basebackup: error: backup failed: ERROR: shell command "type con >
"C:cirruscontrib asebackup_to_shell mp_check mp_test_tch3\base.tar""
failed

I guess it's an escaping problem around \ characters.

On Fri, Mar 25, 2022 at 02:27:07PM -0700, Andres Freund wrote:

On 2022-03-25 13:52:11 -0400, Robert Haas wrote:

On Fri, Mar 25, 2022 at 12:36 PM Andres Freund <andres@anarazel.de> wrote:

Create a CF entry for it, or enable CI on a github repo?

I created a CF entry for it. Then I had to try to Google around to
find the URL from the cfbot, because it's not even linked from
commitfest.postgresql.org for some reason. #blamemagnus

I see it here (and in cfbot), although I'm not sure how you created a new
patch for the active CF, and not for the next CF.
https://commitfest.postgresql.org/37/

I don't think that the Windows CI is running the TAP tests for
contrib. At least, I can't find any indication of it in the output. So
it doesn't really help to assess how portable this test is, unless I'm
missing something.

Yea. It's really unfortunate how vcregress.pl makes it hard to run all
tests. And we're kind of stuck finding a way forward. It's easy enough to work
around for individual tests by just adding them to the test file (like Thomas
did nearby), but clearly that doesn't scale. Andrew wasn't happy with
additional vcregress commands. The fact that vcregress doesn't run tests in
parallel makes things take forever. And so it goes on.

I have a patch to add alltaptests target to vcregress. But I don't recall
hearing any objection to new targets until now.

https://github.com/justinpryzby/postgres/runs/5174877506

On Fri, Mar 25, 2022 at 4:09 PM Andrew Dunstan <andrew@dunslane.net> wrote:

Duplication of TAP test names has long been something that's annoyed me.

Well, I think that's unwarranted. Many years ago, people discovered
that it was annoying if you had to distinguish files solely based on
name, and so they invented directories and pathnames. That was a good
call. Displaying that information in the buildfarm output would be a
good call, too.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2022-03-26 14:40:06 -0400, Robert Haas wrote:

Well, I think that's unwarranted. Many years ago, people discovered
that it was annoying if you had to distinguish files solely based on
name, and so they invented directories and pathnames. That was a good
call.

Yea. I have no problem naming tests the same way, particularly if they do
similar things. But we should show the path.

Displaying that information in the buildfarm output would be a good call,
too.

I would find it very useful locally when running the tests too. A very simple
approach would be to invoke prove with absolute paths to the tests. But that's
not particularly pretty. But unless we change the directory that prove is run
in away from the directory that contains t/ (there's a thread about that, but
more to do), I don't think we can do better on an individual test basis?

We could just make prove_[install]check echo the $(subdir) it's about to run
tests for? Certainly looks better to me:

make -j48 -Otarget -s -C src/bin/ check NO_TEMP_INSTALL=1
...
=== tap tests in src/bin/pg_resetwal ===
t/001_basic.pl ...... ok
t/002_corrupted.pl .. ok
All tests successful.
Files=2, Tests=18, 3 wallclock secs ( 0.01 usr 0.01 sys + 2.39 cusr 0.31 csys = 2.72 CPU)
Result: PASS
=== tap tests in src/bin/pg_checksums ===
t/001_basic.pl .... ok
t/002_actions.pl .. ok
All tests successful.
Files=2, Tests=74, 4 wallclock secs ( 0.02 usr 0.01 sys + 1.57 cusr 0.42 csys = 2.02 CPU)
Result: PASS
=== tap tests in src/bin/psql ===
t/001_basic.pl ........... ok
t/010_tab_completion.pl .. ok
t/020_cancel.pl .......... ok
All tests successful.
Files=3, Tests=125, 6 wallclock secs ( 0.03 usr 0.00 sys + 3.65 cusr 0.56 csys = 4.24 CPU)
Result: PASS
...

Greetings,

Andres Freund

On Sat, Mar 26, 2022 at 3:35 PM Andres Freund <andres@anarazel.de> wrote:

=== tap tests in src/bin/pg_resetwal ===
t/001_basic.pl ...... ok
t/002_corrupted.pl .. ok
All tests successful.
Files=2, Tests=18, 3 wallclock secs ( 0.01 usr 0.01 sys + 2.39 cusr 0.31 csys = 2.72 CPU)
Result: PASS
=== tap tests in src/bin/pg_checksums ===
t/001_basic.pl .... ok
t/002_actions.pl .. ok
All tests successful.
Files=2, Tests=74, 4 wallclock secs ( 0.02 usr 0.01 sys + 1.57 cusr 0.42 csys = 2.02 CPU)
Result: PASS

Yeah, this certainly seems like an improvement to me.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 26 Mar 2022, at 21:03, Robert Haas <robertmhaas@gmail.com> wrote:

On Sat, Mar 26, 2022 at 3:35 PM Andres Freund <andres@anarazel.de> wrote:

=== tap tests in src/bin/pg_resetwal ===
t/001_basic.pl ...... ok
t/002_corrupted.pl .. ok
All tests successful.
Files=2, Tests=18, 3 wallclock secs ( 0.01 usr 0.01 sys + 2.39 cusr 0.31 csys = 2.72 CPU)
Result: PASS
=== tap tests in src/bin/pg_checksums ===
t/001_basic.pl .... ok
t/002_actions.pl .. ok
All tests successful.
Files=2, Tests=74, 4 wallclock secs ( 0.02 usr 0.01 sys + 1.57 cusr 0.42 csys = 2.02 CPU)
Result: PASS

Yeah, this certainly seems like an improvement to me.

+1, that's clearly better.

--
Daniel Gustafsson https://vmware.com/

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Mar 26, 2022 at 3:35 PM Andres Freund <andres@anarazel.de> wrote:

=== tap tests in src/bin/pg_resetwal ===
t/001_basic.pl ...... ok
t/002_corrupted.pl .. ok
All tests successful.

Yeah, this certainly seems like an improvement to me.

+1, but will it help for CI or buildfarm cases?

regards, tom lane

Hi,

On 2022-03-26 16:24:32 -0400, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Mar 26, 2022 at 3:35 PM Andres Freund <andres@anarazel.de> wrote:

=== tap tests in src/bin/pg_resetwal ===
t/001_basic.pl ...... ok
t/002_corrupted.pl .. ok
All tests successful.

Yeah, this certainly seems like an improvement to me.

Do we want to do the same of regress and isolation tests? They're mostly a bit
easier to place, but it's still a memory retention game. Using the above
format for all looks a tad weird, due to pg_regress' output having kinda
similar markers.

...
======================
All 22 tests passed.
======================

=== regress tests in contrib/ltree_plpython" ===
============== creating temporary instance ==============
============== initializing database system ==============
============== starting postmaster ==============
running on port 51696 with PID 3905518
============== creating database "contrib_regression" ==============
CREATE DATABASE
ALTER DATABASE
============== installing ltree ==============
CREATE EXTENSION
============== running regression test queries ==============
test ltree_plpython ... ok 51 ms
============== shutting down postmaster ==============
============== removing temporary instance ==============
...

Could just use a different character. +++ doesn't look bad:
+++ tap tests in contrib/test_decoding +++
t/001_repl_stats.pl .. ok
All tests successful.
Files=1, Tests=2,  3 wallclock secs ( 0.02 usr  0.00 sys +  1.74 cusr  0.28 csys =  2.04 CPU)
Result: PASS

Would we want to do this in all branches? I'd vote for yes, but ...

Prototype patch attached. I looked through the uses of
pg_(isolation_)?regress_(install)?check'
and didn't find any that'd have a problem with turning the invocation into a
multi-command one.

+1, but will it help for CI

Yes, it should make it considerably better (for everything but windows, but
that outputs separators already).

or buildfarm cases?

Probably not much, because that largely runs tests serially with "stage" names
corresponding to the test. And when it runs multiple tests in a row it adds
something similar to the above, e.g.:
=========== Module pg_stat_statements check =============
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=peripatus&amp;dt=2022-03-26%2000%3A20%3A30&amp;stg=misc-check

But I think it'll still be a tad better when it runs a single test:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=snapper&amp;dt=2022-03-26%2018%3A46%3A28&amp;stg=subscription-check

Might make it more realistic to make -s, at least to run tests? The reams of
output like:
gmake -C ../../../../src/test/regress pg_regress
gmake[1]: Entering directory '/home/pgbuildfarm/buildroot/HEAD/pgsql.build/src/test/regress'
gmake -C ../../../src/port all
gmake[2]: Entering directory '/home/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port'
gmake[2]: Nothing to be done for 'all'.
gmake[2]: Leaving directory '/home/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port'
gmake -C ../../../src/common all
gmake[2]: Entering directory '/home/pgbuildfarm/buildroot/HEAD/pgsql.build/src/common'
gmake[2]: Nothing to be done for 'all'.

are quite clutter-y.

Greetings,

Andres Freund

On 26 Mar 2022, at 21:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:

+1, but will it help for CI or buildfarm cases?

Isn't it both, but mostly for CI since the buildfarm already prints the path
when dumping the logfile. Below is a random example snippet from the buildfarm
where it's fairly easy to see 001_basic.pl being the pg_test_fsync test:

/bin/prove -I ../../../src/test/perl/ -I . --timer t/*.pl
[20:31:18] t/001_basic.pl .. ok 224 ms ( 0.00 usr 0.01 sys + 0.18 cusr 0.01 csys = 0.20 CPU)
[20:31:18]
All tests successful.
Files=1, Tests=12, 0 wallclock secs ( 0.05 usr 0.02 sys + 0.18 cusr 0.01 csys = 0.26 CPU)
Result: PASS

================== pgsql.build/src/bin/pg_test_fsync/tmp_check/log/regress_log_001_basic ===================
..

--
Daniel Gustafsson https://vmware.com/

On Sat, Mar 26, 2022 at 4:14 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

I see it here (and in cfbot), although I'm not sure how you created a new
patch for the active CF, and not for the next CF.

Anyone who has ever been a CF manager has this power, it seems. I did
it myself once, by accident, and got told off by the active CF
manager.

Thomas Munro <thomas.munro@gmail.com> writes:

On Sat, Mar 26, 2022 at 4:14 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

I see it here (and in cfbot), although I'm not sure how you created a new
patch for the active CF, and not for the next CF.

Anyone who has ever been a CF manager has this power, it seems. I did
it myself once, by accident, and got told off by the active CF
manager.

I'm not sure what the policy is for that. I have done it myself,
although I've never been a CF manager, so maybe it was granted
to all committers?

regards, tom lane

On Sun, Mar 27, 2022 at 12:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@gmail.com> writes:

On Sat, Mar 26, 2022 at 4:14 PM Justin Pryzby <pryzby@telsasoft.com>

wrote:

I see it here (and in cfbot), although I'm not sure how you created a

new

patch for the active CF, and not for the next CF.

Anyone who has ever been a CF manager has this power, it seems. I did
it myself once, by accident, and got told off by the active CF
manager.

I'm not sure what the policy is for that. I have done it myself,
although I've never been a CF manager, so maybe it was granted
to all committers?

It is not. In fact, you have some strange half-between power that is only
you and those pginfra members that are *not* developers in it... I've made
you a "full cf manager" now so it's at least consistent :)

And yes, the way it works now is once a cf manager always a cf manager. We
haven't had enough of them that it's been something worth considering yet.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On Fri, Mar 25, 2022 at 5:52 PM Thomas Munro <thomas.munro@gmail.com> wrote:

On Sat, Mar 26, 2022 at 9:55 AM Thomas Munro <thomas.munro@gmail.com> wrote:

https://api.cirrus-ci.com/v1/artifact/task/5637156969381888/log/contrib/basebackup_to_shell/tmp_check/log/regress_log_001_basic

This line doesn't look too healthy:

pg_basebackup: error: backup failed: ERROR: shell command "type con >
"C:cirruscontrib asebackup_to_shell mp_check mp_test_tch3\base.tar""
failed

I guess it's an escaping problem around \ characters.

Oh, right. I didn't copy the usual incantation as completely as I
should have done.

Here's a new version, hopefully rectifying that deficiency. I also add
a second patch here, documenting basebackup_to_shell.required_role,
because Joe Conway observed elsewhere that I forgot to do that.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Mar 30, 2022 at 3:08 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Mar 25, 2022 at 5:52 PM Thomas Munro <thomas.munro@gmail.com> wrote:

On Sat, Mar 26, 2022 at 9:55 AM Thomas Munro <thomas.munro@gmail.com> wrote:
This line doesn't look too healthy:

pg_basebackup: error: backup failed: ERROR: shell command "type con >
"C:cirruscontrib asebackup_to_shell mp_check mp_test_tch3\base.tar""
failed

I guess it's an escaping problem around \ characters.

Oh, right. I didn't copy the usual incantation as completely as I
should have done.

Here's a new version, hopefully rectifying that deficiency. I also add
a second patch here, documenting basebackup_to_shell.required_role,
because Joe Conway observed elsewhere that I forgot to do that.

Here are your patches again, plus that kludge to make the CI run your
TAP test on Windows.

On Wed, Mar 30, 2022 at 8:30 AM Thomas Munro <thomas.munro@gmail.com> wrote:

On Wed, Mar 30, 2022 at 3:08 AM Robert Haas <robertmhaas@gmail.com> wrote:

Here's a new version, hopefully rectifying that deficiency. I also add
a second patch here, documenting basebackup_to_shell.required_role,
because Joe Conway observed elsewhere that I forgot to do that.

Here are your patches again, plus that kludge to make the CI run your
TAP test on Windows.

It failed:

https://cirrus-ci.com/task/5567070686412800
https://api.cirrus-ci.com/v1/artifact/task/5567070686412800/log/contrib/basebackup_to_shell/tmp_check/log/001_basic_primary.log
https://api.cirrus-ci.com/v1/artifact/task/5567070686412800/log/contrib/basebackup_to_shell/tmp_check/log/regress_log_001_basic

On Tue, Mar 29, 2022 at 4:36 PM Thomas Munro <thomas.munro@gmail.com> wrote:

It failed:

https://cirrus-ci.com/task/5567070686412800
https://api.cirrus-ci.com/v1/artifact/task/5567070686412800/log/contrib/basebackup_to_shell/tmp_check/log/001_basic_primary.log
https://api.cirrus-ci.com/v1/artifact/task/5567070686412800/log/contrib/basebackup_to_shell/tmp_check/log/regress_log_001_basic

Hmm. The log here is not very informative. It just says that the first
time we tried to use the 'shell' target, it timed out. I suppose the
most obvious explanation for that is that the shell command we
executed timed out:

qq{type con > "$escaped_backup_path\\\\%f"}

But why should that be so? Does 'type con' not respond to EOF? I don't
see how that can be the case. Is our implementation of pclose broken?
If so, then I think COPY TO/FROM PROGRAM would be broken on Windows.

Any ideas?

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2022-03-29 17:19:44 -0400, Robert Haas wrote:

On Tue, Mar 29, 2022 at 4:36 PM Thomas Munro <thomas.munro@gmail.com> wrote:

It failed:

https://cirrus-ci.com/task/5567070686412800
https://api.cirrus-ci.com/v1/artifact/task/5567070686412800/log/contrib/basebackup_to_shell/tmp_check/log/001_basic_primary.log
https://api.cirrus-ci.com/v1/artifact/task/5567070686412800/log/contrib/basebackup_to_shell/tmp_check/log/regress_log_001_basic

Hmm. The log here is not very informative. It just says that the first
time we tried to use the 'shell' target, it timed out. I suppose the
most obvious explanation for that is that the shell command we
executed timed out:

qq{type con > "$escaped_backup_path\\\\%f"}

But why should that be so? Does 'type con' not respond to EOF?

This is trying to write stdin into a file? I think the problem may be that con
doesn't represent stdin, it it's console input. I think consoles are a
separate thing from stdin on windows - you can have console input, even while
stdin is coming from a file or such.

Didn't immediate find a reference to a cat equivalent. Maybe just gzip the
file? That can read from stdin across platforms afaict.

Greetings,

Andres Freund

On 3/29/22 17:19, Robert Haas wrote:

On Tue, Mar 29, 2022 at 4:36 PM Thomas Munro <thomas.munro@gmail.com> wrote:

It failed:

https://cirrus-ci.com/task/5567070686412800
https://api.cirrus-ci.com/v1/artifact/task/5567070686412800/log/contrib/basebackup_to_shell/tmp_check/log/001_basic_primary.log
https://api.cirrus-ci.com/v1/artifact/task/5567070686412800/log/contrib/basebackup_to_shell/tmp_check/log/regress_log_001_basic

Hmm. The log here is not very informative. It just says that the first
time we tried to use the 'shell' target, it timed out. I suppose the
most obvious explanation for that is that the shell command we
executed timed out:

qq{type con > "$escaped_backup_path\\\\%f"}

But why should that be so? Does 'type con' not respond to EOF? I don't
see how that can be the case. Is our implementation of pclose broken?
If so, then I think COPY TO/FROM PROGRAM would be broken on Windows.

AIUI 'type con' is not the equivalent of Unix cat, especially w.r.t.
stdin. It's going to try to read from the console, not from stdin. It's
more the equivalent of 'cat /dev/tty'. So it's not at all surprising
that it hangs. I don't know of a Windows builtin that is equivalent to cat.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, Mar 30, 2022 at 11:25 AM Andres Freund <andres@anarazel.de> wrote:

Didn't immediate find a reference to a cat equivalent. Maybe just gzip the
file? That can read from stdin across platforms afaict.

. o O ( gzip | gzip -d )

On 3/29/22 20:48, Thomas Munro wrote:

On Wed, Mar 30, 2022 at 11:25 AM Andres Freund <andres@anarazel.de> wrote:

Didn't immediate find a reference to a cat equivalent. Maybe just gzip the
file? That can read from stdin across platforms afaict.

. o O ( gzip | gzip -d )

Triple bleah. If we have to do that at least we should probably use
`gzip --fast`

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, Mar 30, 2022 at 7:01 AM Andrew Dunstan <andrew@dunslane.net> wrote:

Triple bleah. If we have to do that at least we should probably use
`gzip --fast`

I'm not sure it's going to make enough difference to get fussed about,
but sure. Here's a new series, adjusted to use 'gzip' instead of 'cat'
and 'type'.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2022-03-26 13:51:35 -0700, Andres Freund wrote:

Prototype patch attached.

Because I forgot about cfbot when attaching the patch, cfbot actually ran with
it under this thread's cf entry. It does look like an improvement to me:
https://cirrus-ci.com/task/6397292629458944?logs=test_world#L900

We certainly can do better, but it's sufficiently better than what we have
right now. So I'd like to commit it?

Would we want to do this in all branches? I'd vote for yes, but ...

Unless somebody speaks in favor of doing this across branches, I'd just go for
HEAD.

Greetings,

Andres Freund

On 2022-03-30 08:53:43 -0400, Robert Haas wrote:

Here's a new series, adjusted to use 'gzip' instead of 'cat' and 'type'.

Seems to have done the trick: https://cirrus-ci.com/task/6474955838717952?logs=test_contrib_basebackup_to_shell#L1

# Reconfigure to restrict access and require a detail.
$shell_command =
$PostgreSQL::Test::Utils::windows_os
? qq{$gzip --fast > "$escaped_backup_path\\\\%d.%f.gz"}
: qq{$gzip --fast > "$escaped_backup_path/%d.%f.gz"};

I don't think the branch is needed anymore, forward slashes should work for
output redirection.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

Because I forgot about cfbot when attaching the patch, cfbot actually ran with
it under this thread's cf entry. It does look like an improvement to me:
https://cirrus-ci.com/task/6397292629458944?logs=test_world#L900
We certainly can do better, but it's sufficiently better than what we have
right now. So I'd like to commit it?

No objection here.

Would we want to do this in all branches? I'd vote for yes, but ...

Unless somebody speaks in favor of doing this across branches, I'd just go for
HEAD.

+1 for HEAD only, especially if we think we might change it some more
later. It seems possible this might break somebody's tooling if we
drop it into minor releases.

One refinement that comes to mind as I look at the patch is to distinguish
between "check" and "installcheck". Not sure that's worthwhile, but not
sure it isn't, either.

regards, tom lane

On Wed, Mar 30, 2022 at 12:30 PM Andres Freund <andres@anarazel.de> wrote:

# Reconfigure to restrict access and require a detail.
$shell_command =
$PostgreSQL::Test::Utils::windows_os
? qq{$gzip --fast > "$escaped_backup_path\\\\%d.%f.gz"}
: qq{$gzip --fast > "$escaped_backup_path/%d.%f.gz"};

I don't think the branch is needed anymore, forward slashes should work for
output redirection.

We have similar things in src/test/perl/PostgreSQL/Test/Cluster.pm. Do
you think those can also be removed? I'm not sure it's the place of
this patch to introduce a mix of styles.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2022-03-30 12:34:34 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Unless somebody speaks in favor of doing this across branches, I'd just go for
HEAD.

+1 for HEAD only, especially if we think we might change it some more
later. It seems possible this might break somebody's tooling if we
drop it into minor releases.

Yea. I certainly have written scripts that parse check-world output - they
didn't break, but...

One refinement that comes to mind as I look at the patch is to distinguish
between "check" and "installcheck". Not sure that's worthwhile, but not
sure it isn't, either.

As it's just about "free" to do so, I see no reason not to go for showing that
difference. How about:

echo "+++ (tap|regress|isolation) [install-]check in $(subdir) +++" && \

I see no reason to distinguish the PGXS / non-PGXs tap installcheck cases?

Random aside: Am I the only one bothered by a bunch of places in
Makefile.global.in quoting like
$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
and
rm -rf '$(CURDIR)'/tmp_check &&
etc
yielding commands like:
make -C '.' DESTDIR='/home/andres/build/postgres/dev-assert/vpath'/tmp_install install >'/home/andres/build/postgres/dev-assert/vpath'/tmp_install/log/install.log 2>&1
and
rm -rf '/home/andres/build/postgres/dev-assert/vpath/contrib/test_decoding'/tmp_check &

Greetings,

Andres Freund

Hi,

On 2022-03-30 12:42:50 -0400, Robert Haas wrote:

On Wed, Mar 30, 2022 at 12:30 PM Andres Freund <andres@anarazel.de> wrote:

# Reconfigure to restrict access and require a detail.
$shell_command =
$PostgreSQL::Test::Utils::windows_os
? qq{$gzip --fast > "$escaped_backup_path\\\\%d.%f.gz"}
: qq{$gzip --fast > "$escaped_backup_path/%d.%f.gz"};

I don't think the branch is needed anymore, forward slashes should work for
output redirection.

We have similar things in src/test/perl/PostgreSQL/Test/Cluster.pm.

There are some commandline utilities (including copy) where backward slashes
in arguments are necessary, to separate options from paths :/. Those are the
extent of backslash use in Cluster.pm that I could see quickly.

I'm not sure it's the place of this patch to introduce a mix of styles.

Fair enough. I found it a bit grating to read in the test, that's why I
mentioned it...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-03-30 12:34:34 -0400, Tom Lane wrote:

One refinement that comes to mind as I look at the patch is to distinguish
between "check" and "installcheck". Not sure that's worthwhile, but not
sure it isn't, either.

As it's just about "free" to do so, I see no reason not to go for showing that
difference. How about:

echo "+++ (tap|regress|isolation) [install-]check in $(subdir) +++" && \

WFM.

I see no reason to distinguish the PGXS / non-PGXs tap installcheck cases?

Agreed.

Random aside: Am I the only one bothered by a bunch of places in
Makefile.global.in quoting like
$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
and
rm -rf '$(CURDIR)'/tmp_check &&
etc

Don't we need that to handle, say, build paths with spaces in them?
Admittedly we're probably not completely clean for such paths,
but that's not an excuse to break the places that do it right.

(I occasionally think about setting up a BF animal configured
like that, but haven't tried yet.)

regards, tom lane

On Wed, Mar 30, 2022 at 12:54 PM Andres Freund <andres@anarazel.de> wrote:

There are some commandline utilities (including copy) where backward slashes
in arguments are necessary, to separate options from paths :/. Those are the
extent of backslash use in Cluster.pm that I could see quickly.

I just copied this logic from that file:

$path =~ s{\\}{\\\\}g if ($PostgreSQL::Test::Utils::windows_os);
my $copy_command =
$PostgreSQL::Test::Utils::windows_os
? qq{copy "$path\\\\%f" "%p"}
: qq{cp "$path/%f" "%p"};

In the first version of the patch I neglected the first of those lines
and it broke, so the s{\\}{\\\\}g thing is definitely needed. It's
possible that / would be as good as \\\\ in the command text itself,
but it doesn't seem worth getting excited about. It'd be best if any
unnecessary garbage of this sort got cleaned up by someone who has a
test environment locally, rather than me testing by sending emails to
a mailing list which Thomas then downloads into a sandbox and executes
which you then send me links to what broke on the mailing list and I
try again.

Fair enough. I found it a bit grating to read in the test, that's why I
mentioned it...

I'm going to go ahead and commit this test script later on this
afternoon unless there are vigorous objections real soon now, and then
if somebody wants to improve it, great!

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On March 30, 2022 9:58:26 AM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

Random aside: Am I the only one bothered by a bunch of places in
Makefile.global.in quoting like
$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
and
rm -rf '$(CURDIR)'/tmp_check &&
etc

Don't we need that to handle, say, build paths with spaces in them?

My concern is about the quote in the middle of the path, not about quoting at all... I.e. the ' should be after /tmp_install, not before.

Admittedly we're probably not completely clean for such paths,
but that's not an excuse to break the places that do it right.

(I occasionally think about setting up a BF animal configured
like that, but haven't tried yet.)

That might be a fun exercise. Not so much for the build aspect, but to make sure our tools handle it.

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Wed, Mar 30, 2022 at 1:11 PM Andres Freund <andres@anarazel.de> wrote:

On March 30, 2022 9:58:26 AM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

Random aside: Am I the only one bothered by a bunch of places in
Makefile.global.in quoting like
$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
and
rm -rf '$(CURDIR)'/tmp_check &&
etc

Don't we need that to handle, say, build paths with spaces in them?

My concern is about the quote in the middle of the path, not about quoting at all... I.e. the ' should be after /tmp_install, not before.

Makes no difference. We know that the string /tmp_install contains no
shell metacharacters, so why does it need to be in quotes? I would've
probably written it the way it is here, rather than what you are
proposing.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Mar 31, 2022 at 5:23 AM Andres Freund <andres@anarazel.de> wrote:

On 2022-03-26 13:51:35 -0700, Andres Freund wrote:

Prototype patch attached.

Because I forgot about cfbot when attaching the patch, cfbot actually ran with
it under this thread's cf entry. It does look like an improvement to me:
https://cirrus-ci.com/task/6397292629458944?logs=test_world#L900

We certainly can do better, but it's sufficiently better than what we have
right now. So I'd like to commit it?

Nice, this will save a lot of time scrolling around trying to figure
out what broke.

Would we want to do this in all branches? I'd vote for yes, but ...

Unless somebody speaks in favor of doing this across branches, I'd just go for
HEAD.

I don't see any reason not to do it on all branches. If anyone is
machine-processing the output and cares about format changes they will
be happy about the improvement.

Hi,

On 2022-03-30 13:16:47 -0400, Robert Haas wrote:

On Wed, Mar 30, 2022 at 1:11 PM Andres Freund <andres@anarazel.de> wrote:

On March 30, 2022 9:58:26 AM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

Random aside: Am I the only one bothered by a bunch of places in
Makefile.global.in quoting like
$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
and
rm -rf '$(CURDIR)'/tmp_check &&
etc

Don't we need that to handle, say, build paths with spaces in them?

My concern is about the quote in the middle of the path, not about quoting at all... I.e. the ' should be after /tmp_install, not before.

Makes no difference. We know that the string /tmp_install contains no
shell metacharacters, so why does it need to be in quotes? I would've
probably written it the way it is here, rather than what you are
proposing.

It looks ugly, and it can't be copy-pasted as easily. Seems I'm alone on this,
so I'll leave it be...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-03-30 13:16:47 -0400, Robert Haas wrote:

On Wed, Mar 30, 2022 at 1:11 PM Andres Freund <andres@anarazel.de> wrote:

My concern is about the quote in the middle of the path, not about quoting at all... I.e. the ' should be after /tmp_install, not before.

Makes no difference. We know that the string /tmp_install contains no
shell metacharacters, so why does it need to be in quotes? I would've
probably written it the way it is here, rather than what you are
proposing.

It looks ugly, and it can't be copy-pasted as easily. Seems I'm alone on this,
so I'll leave it be...

FWIW, I agree with Andres that I'd probably have put the quote
at the end. But Robert is right that it's functionally equivalent;
so I doubt it's worth changing.

regards, tom lane

Robert Haas <robertmhaas@gmail.com> writes:

I'm going to go ahead and commit this test script later on this
afternoon unless there are vigorous objections real soon now, and then
if somebody wants to improve it, great!

I see you did that, but the CF entry is still open?

regards, tom lane

On Wed, Mar 30, 2022 at 4:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I'm going to go ahead and commit this test script later on this
afternoon unless there are vigorous objections real soon now, and then
if somebody wants to improve it, great!

I see you did that, but the CF entry is still open?

Fixed.

--
Robert Haas
EDB: http://www.enterprisedb.com

I think we should give this module a .gitignore file. Patch attached.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:

I think we should give this module a .gitignore file. Patch attached.

Indeed, before somebody accidentally commits the cruft that
check-world is leaving around. Pushed.

regards, tom lane

So ... none of the Windows buildfarm members actually like this
test script. They're all showing failures along the lines of

not ok 2 - fails if basebackup_to_shell.command is not set: matches

# Failed test 'fails if basebackup_to_shell.command is not set: matches'
# at t/001_basic.pl line 38.
# 'pg_basebackup: error: connection to server at "127.0.0.1", port 55358 failed: FATAL: SSPI authentication failed for user "backupuser"
# '
# doesn't match '(?^:shell command for backup is not configured)'

Does the CI setup not account for this issue?

regards, tom lane

Hi,

On 2022-03-30 22:07:48 -0400, Tom Lane wrote:

So ... none of the Windows buildfarm members actually like this
test script. They're all showing failures along the lines of

not ok 2 - fails if basebackup_to_shell.command is not set: matches

# Failed test 'fails if basebackup_to_shell.command is not set: matches'
# at t/001_basic.pl line 38.
# 'pg_basebackup: error: connection to server at "127.0.0.1", port 55358 failed: FATAL: SSPI authentication failed for user "backupuser"
# '
# doesn't match '(?^:shell command for backup is not configured)'

Does the CI setup not account for this issue?

On windows CI sets
# Avoids port conflicts between concurrent tap test runs
PG_TEST_USE_UNIX_SOCKETS: 1

because I've otherwise seen a lot of spurious tap test failures - Cluster.pm
get_free_port() is racy, as it admits:
XXX A port available now may become unavailable by the time we start
the desired service.

The only alternative is to not use parallelism when running tap tests, but
that makes test runs even slower - and windows is already the bottleneck for
cfbot.

I assume SSPI doesn't work over unix sockets? Oh. Maybe it's not even that -
we only enable it when not using unix sockets:

# Internal method to set up trusted pg_hba.conf for replication. Not
# documented because you shouldn't use it, it's called automatically if needed.
sub set_replication_conf
{
my ($self) = @_;
my $pgdata = $self->data_dir;

$self->host eq $test_pghost
or croak "set_replication_conf only works with the default host";

open my $hba, '>>', "$pgdata/pg_hba.conf";
print $hba "\n# Allow replication (set up by PostgreSQL::Test::Cluster.pm)\n";
if ($PostgreSQL::Test::Utils::windows_os && !$PostgreSQL::Test::Utils::use_unix_sockets)
{
print $hba
"host replication all $test_localhost/32 sspi include_realm=1 map=regress\n";
}
close $hba;
return;
}

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-03-30 22:07:48 -0400, Tom Lane wrote:

So ... none of the Windows buildfarm members actually like this
test script.

On windows CI sets
# Avoids port conflicts between concurrent tap test runs
PG_TEST_USE_UNIX_SOCKETS: 1

Ok ...

I assume SSPI doesn't work over unix sockets? Oh. Maybe it's not even that -
we only enable it when not using unix sockets:

Duh. But can it work over unix sockets?

regards, tom lane

Hi,

On 2022-03-31 00:08:00 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I assume SSPI doesn't work over unix sockets? Oh. Maybe it's not even that -
we only enable it when not using unix sockets:

Duh. But can it work over unix sockets?

I wonder if we should go the other way and use unix sockets by default in the
tests. Even if CI windows could be made to use SSPI, it'd still be further
away from the environment most of us write tests in.

Afaics the only reason we use SSPI is to secure the tests, because they run
over tcp by default. But since we have unix socket support for windows now,
that shouldn't really be needed anymore.

The only animal that might not be new enough for it is hamerkop. I don't
really understand when windows features end up in which release.

Looking at 8f3ec75de40 it seems we just assume unix sockets are available, we
don't have a version / feature test (win32.h just defines
HAVE_STRUCT_SOCKADDR_UN).

Greetings,

Andres Freund

On 31.03.22 07:25, Andres Freund wrote:

Looking at 8f3ec75de40 it seems we just assume unix sockets are available, we
don't have a version / feature test (win32.h just defines
HAVE_STRUCT_SOCKADDR_UN).

I think you have to handle that dynamically at run time, a bit like
IPv6: The build environment might provide symbols, structs, etc., but
the kernel might return an error when you try to create a socket.

On 3/30/22 22:07, Tom Lane wrote:

So ... none of the Windows buildfarm members actually like this
test script. They're all showing failures along the lines of

not ok 2 - fails if basebackup_to_shell.command is not set: matches

# Failed test 'fails if basebackup_to_shell.command is not set: matches'
# at t/001_basic.pl line 38.
# 'pg_basebackup: error: connection to server at "127.0.0.1", port 55358 failed: FATAL: SSPI authentication failed for user "backupuser"
# '
# doesn't match '(?^:shell command for backup is not configured)'

Does the CI setup not account for this issue?

I have configured fairywren and drongo to use Unix sockets., and they
have turned green Here are the settings I'm using in the config's
build_env section:

                PG_TEST_USE_UNIX_SOCKETS => 1,
                PG_REGRESS_SOCK_DIR =>
'C:/Users/pgrunner/AppData/Local/Temp',

We should probably fix the test though, so it doesn't require Unix
sockets. It should be possible, although I haven't looked yet to see how.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, Mar 31, 2022 at 10:52 AM Andrew Dunstan <andrew@dunslane.net> wrote:

I have configured fairywren and drongo to use Unix sockets., and they
have turned green Here are the settings I'm using in the config's
build_env section:

PG_TEST_USE_UNIX_SOCKETS => 1,
PG_REGRESS_SOCK_DIR =>
'C:/Users/pgrunner/AppData/Local/Temp',

We should probably fix the test though, so it doesn't require Unix
sockets. It should be possible, although I haven't looked yet to see how.

Our mutual colleague Neha Sharma pointed out this email message to me:

/messages/by-id/106926.1643842376@sss.pgh.pa.us

I actually don't understand why using pg_regress --auth-extra would
fix it, or what that option does, or why we're even running pg_regress
at all in PostgreSQL::Test::Cluster::init. I think it might be to fix
this exact issue, but there's no SGML documentation for pg_regress,
and the output of pg_regress -h isn't really clear enough to
understand what's going on here.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Mar 31, 2022 at 10:52 AM Andrew Dunstan <andrew@dunslane.net> wrote:

We should probably fix the test though, so it doesn't require Unix
sockets. It should be possible, although I haven't looked yet to see how.

Our mutual colleague Neha Sharma pointed out this email message to me:
/messages/by-id/106926.1643842376@sss.pgh.pa.us

Ah, right.

I actually don't understand why using pg_regress --auth-extra would
fix it, or what that option does, or why we're even running pg_regress
at all in PostgreSQL::Test::Cluster::init. I think it might be to fix
this exact issue, but there's no SGML documentation for pg_regress,

I'm not volunteering to fix that, but this comment in pg_regress.c
is probably adequately illuminating:

* Rewrite pg_hba.conf and pg_ident.conf to use SSPI authentication. Permit
* the current OS user to authenticate as the bootstrap superuser and as any
* user named in a --create-role option.

This script is creating users manually rather than letting the TAP
infrastructure do it, which is an antipattern.

regards, tom lane

On 2022-Mar-30, Andres Freund wrote:

It looks ugly, and it can't be copy-pasted as easily. Seems I'm alone on this,
so I'll leave it be...

I'm bothered by that quote-in-the-middle occassionally as well (requires
more clicks to paste).

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On Thu, Mar 31, 2022 at 11:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm not volunteering to fix that, but this comment in pg_regress.c
is probably adequately illuminating:

* Rewrite pg_hba.conf and pg_ident.conf to use SSPI authentication. Permit
* the current OS user to authenticate as the bootstrap superuser and as any
* user named in a --create-role option.

This script is creating users manually rather than letting the TAP
infrastructure do it, which is an antipattern.

Well, first, I don't really think it's great if you have to try to
figure out what a tool does by reading the comments in the source
code. I grant that it's a step above trying to interpret the source
code itself, but it's still not great. Second, I think your diagnosis
of the problem is slightly incorrect, because your comment seems to
imply that this change ought to work:

diff --git a/contrib/basebackup_to_shell/t/001_basic.pl
b/contrib/basebackup_to_shell/t/001_basic.pl
index 57534b62c8..1fc0d9ab15 100644
--- a/contrib/basebackup_to_shell/t/001_basic.pl
+++ b/contrib/basebackup_to_shell/t/001_basic.pl
@@ -17,11 +17,11 @@ if (!defined $gzip || $gzip eq '')
 }

 my $node = PostgreSQL::Test::Cluster->new('primary');
-$node->init('allows_streaming' => 1);
+$node->init('allows_streaming' => 1, auth_extra => [ '--create-role',
'backupuser' ]);
 $node->append_conf('postgresql.conf',
                    "shared_preload_libraries = 'basebackup_to_shell'");
 $node->start;
-$node->safe_psql('postgres', 'CREATE USER backupuser REPLICATION');
+#$node->safe_psql('postgres', 'CREATE USER backupuser REPLICATION');
 $node->safe_psql('postgres', 'CREATE ROLE trustworthy');

# For nearly all pg_basebackup invocations some options should be specified,

But it doesn't -- with that change, the test fails on Linux,
complaining that the backupuser user does not exist. That's because
--create-role doesn't actually create a role at all, and in fact
absolutely couldn't, because the server isn't even started at the
point where we're running pg_regress. I think we need to both tell
pg_regress to "create the role" and also actually create it. Which is
maybe not a great sign that everything here is totally clear and
comprehensible...

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2022-03-31 11:32:15 -0400, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:
* Rewrite pg_hba.conf and pg_ident.conf to use SSPI authentication. Permit
* the current OS user to authenticate as the bootstrap superuser and as any
* user named in a --create-role option.

This script is creating users manually rather than letting the TAP
infrastructure do it, which is an antipattern.

Seems like Cluster.pm should have a helper for creating roles, which then
would use --create-role internally. So there's at least something to find when
looking through Cluster.pm...

Greetings,

Andres Freund

On 3/31/22 11:32, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Mar 31, 2022 at 10:52 AM Andrew Dunstan <andrew@dunslane.net> wrote:

We should probably fix the test though, so it doesn't require Unix
sockets. It should be possible, although I haven't looked yet to see how.

Our mutual colleague Neha Sharma pointed out this email message to me:
/messages/by-id/106926.1643842376@sss.pgh.pa.us

Yep, that's kinda what I was expecting.

I actually don't understand why using pg_regress --auth-extra would
fix it, or what that option does, or why we're even running pg_regress
at all in PostgreSQL::Test::Cluster::init. I think it might be to fix
this exact issue, but there's no SGML documentation for pg_regress,

I really don't know why this stuff is in pg_regress at all. It seems
rather odd to me and it's annoyed me for a while. But that's a fight for
another day.

I'm not volunteering to fix that, but this comment in pg_regress.c
is probably adequately illuminating:

* Rewrite pg_hba.conf and pg_ident.conf to use SSPI authentication. Permit
* the current OS user to authenticate as the bootstrap superuser and as any
* user named in a --create-role option.

This script is creating users manually rather than letting the TAP
infrastructure do it, which is an antipattern.

Yeah, I think the fix is as simple as the attached.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, Mar 31, 2022 at 12:25 PM Andrew Dunstan <andrew@dunslane.net> wrote:

Yeah, I think the fix is as simple as the attached.

Well, that does not work because you added an extra parenthesis which
makes Perl barf. If you fix that, then the test does not pass because,
as I just explained to Tom, the flag we call --create-role doesn't
create a role:

error running SQL: 'psql:<stdin>:1: ERROR: role "backupuser" does not exist'

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Mar 31, 2022 at 12:25 PM Andrew Dunstan <andrew@dunslane.net> wrote:

Yeah, I think the fix is as simple as the attached.

Well, that does not work because you added an extra parenthesis which
makes Perl barf. If you fix that, then the test does not pass because,
as I just explained to Tom, the flag we call --create-role doesn't
create a role:

error running SQL: 'psql:<stdin>:1: ERROR: role "backupuser" does not exist'

On looking closer, the combination of --config-auth and --create-role
*only* fixes the config files for SSPI, it doesn't expect the server
to be running.

I agree that the documentation of this is nonexistent and the design
is probably questionable, but I'm not volunteering to fix either.
If you are, step right up. In the meantime, I believe (without
having tested) that the correct incantation is to use auth_extra
but *also* create the user further down.

regards, tom lane

On Thu, Mar 31, 2022 at 12:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I agree that the documentation of this is nonexistent and the design
is probably questionable, but I'm not volunteering to fix either.
If you are, step right up. In the meantime, I believe (without
having tested) that the correct incantation is to use auth_extra
but *also* create the user further down.

I agree. That's exactly what I said in
/messages/by-id/CA+TgmoasOhqLR=TSYmHd4TyX-qnfwtde_u19ZphKunpSCkh_iw@mail.gmail.com
...

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Mar 31, 2022 at 12:56 PM Robert Haas <robertmhaas@gmail.com> wrote:

I agree. That's exactly what I said in
/messages/by-id/CA+TgmoasOhqLR=TSYmHd4TyX-qnfwtde_u19ZphKunpSCkh_iw@mail.gmail.com
...

OK, so I pushed a commit adding that incantation to the test script,
and also a comment explaining why it's there. Possibly we ought to go
add similar comments to other places where this incantation is used,
or find a way to make this all a bit more self-documenting, but that
doesn't necessarily need to be done today.

The buildfarm does look rather green at the moment, though, so I'm not
sure how I know whether this "worked".

--
Robert Haas
EDB: http://www.enterprisedb.com

On 3/31/22 14:12, Robert Haas wrote:

On Thu, Mar 31, 2022 at 12:56 PM Robert Haas <robertmhaas@gmail.com> wrote:

I agree. That's exactly what I said in
/messages/by-id/CA+TgmoasOhqLR=TSYmHd4TyX-qnfwtde_u19ZphKunpSCkh_iw@mail.gmail.com
...

OK, so I pushed a commit adding that incantation to the test script,
and also a comment explaining why it's there. Possibly we ought to go
add similar comments to other places where this incantation is used,
or find a way to make this all a bit more self-documenting, but that
doesn't necessarily need to be done today.

The buildfarm does look rather green at the moment, though, so I'm not
sure how I know whether this "worked".

You should know when jacana reports next (in the next hour or three), as
it's not set up for Unix sockets.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 3/31/22 12:45, Tom Lane wrote:

On looking closer, the combination of --config-auth and --create-role
*only* fixes the config files for SSPI, it doesn't expect the server
to be running.

I agree that the documentation of this is nonexistent and the design
is probably questionable, but I'm not volunteering to fix either.
If you are, step right up. In the meantime, I believe (without
having tested) that the correct incantation is to use auth_extra
but *also* create the user further down.

I will take fixing it as a TODO. But not until after feature freeze.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 2022-03-30 12:58:26 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2022-03-30 12:34:34 -0400, Tom Lane wrote:

One refinement that comes to mind as I look at the patch is to distinguish
between "check" and "installcheck". Not sure that's worthwhile, but not
sure it isn't, either.

As it's just about "free" to do so, I see no reason not to go for showing that
difference. How about:

echo "+++ (tap|regress|isolation) [install-]check in $(subdir) +++" && \

WFM.

Pushed like that.

Greetings,

Andres Freund