Thread: Re: pgsql: Add 'basebackup_to_shell' contrib module.
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
Attachment
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
Attachment
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 status255 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&dt=2022-03-26%2000%3A20%3A30&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&dt=2022-03-26%2018%3A46%3A28&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
Attachment
> 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.
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
Attachment
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.
Attachment
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
Attachment
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.log2>&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.log2>&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.log2>&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.log2>&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.log2>&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.log2>&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
Attachment
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
authenticationfailed 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
authenticationfailed 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 authenticationfailed 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: http://postgr.es/m/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: > http://postgr.es/m/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: >> http://postgr.es/m/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
Attachment
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 http://postgr.es/m/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 > http://postgr.es/m/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 >> http://postgr.es/m/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