pgsql: Add 'basebackup_to_shell' contrib module.
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.
Discussion: /messages/by-id/CA+TgmoaqvdT-u3nt+_kkZ7bgDAyqDB0i-+XOMmr5JN2Rd37hxw@mail.gmail.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/c6306db24bd913375f99494e38ab315befe44e11
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(+)
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
Attachments:
0001-Tests.patchapplication/octet-stream; name=0001-Tests.patchDownload+114-1
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. #blamemagnusI 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:
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.
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: PASSYeah, 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: PASSWould 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
Attachments:
make-test-subdir.patchtext/x-diff; charset=us-asciiDownload+7-0
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
