Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
On Sun, Jul 2, 2023 at 6:17 PM Michael Paquier wrote: > Adjusted that as well, on top of an extra comment. Thanks all! --Jacob
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
On Fri, Jun 30, 2023 at 09:42:13AM +0200, Daniel Gustafsson wrote: > Agreed, I'd prefer all branches to work the same for this. Thanks, done this way across all the branches, then. > Reading the patch, only one thing stood out: > > -variable PG_TEST_NOCLEAN is set, data directories will be retained > -regardless of test status. > +variable PG_TEST_NOCLEAN is set, those directories will be retained > > I would've written "the data directories" instead of "those directories" here. Adjusted that as well, on top of an extra comment. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
> On 30 Jun 2023, at 09:09, Michael Paquier wrote: > > On Thu, Jun 29, 2023 at 09:05:59AM -0700, Jacob Champion wrote: >> Sounds good -- 0002 can be ignored as needed, then. (Or I can resend a >> v3 for CI purposes, if you'd like.) > > I am assuming that this is 0001 posted here: > https://www.postgresql.org/message-id/8be3d35a-9608-b1d5-e5e6-7a744ea45...@timescale.com > > And that looks OK to me. This is something I'd rather backpatch down > to v11 on usability ground for developers. Any comments or objections > about that? Agreed, I'd prefer all branches to work the same for this. Reading the patch, only one thing stood out: -variable PG_TEST_NOCLEAN is set, data directories will be retained -regardless of test status. +variable PG_TEST_NOCLEAN is set, those directories will be retained I would've written "the data directories" instead of "those directories" here. -- Daniel Gustafsson
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
On Thu, Jun 29, 2023 at 09:05:59AM -0700, Jacob Champion wrote: > Sounds good -- 0002 can be ignored as needed, then. (Or I can resend a > v3 for CI purposes, if you'd like.) I am assuming that this is 0001 posted here: https://www.postgresql.org/message-id/8be3d35a-9608-b1d5-e5e6-7a744ea45...@timescale.com And that looks OK to me. This is something I'd rather backpatch down to v11 on usability ground for developers. Any comments or objections about that? -- Michael signature.asc Description: PGP signature
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
On Wed, Jun 28, 2023 at 5:41 PM Michael Paquier wrote: > Agreed. I am not sure that this is worth changing to have > boolean-like checks. Hence, I would also to keep the patch that > checks if the environment variable is defined to enforce the behavior, > without checking for a specific value. Sounds good -- 0002 can be ignored as needed, then. (Or I can resend a v3 for CI purposes, if you'd like.) --Jacob
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
On Wed, Jun 28, 2023 at 10:45:02AM +0200, Peter Eisentraut wrote: > Right, the usual style is just to check whether an environment variable is > set to something, not what it is. > > Also note that in general not all environment variables are processed by > Perl, so I would avoid encoding Perl semantics about what is "true" or > whatever into it. Agreed. I am not sure that this is worth changing to have boolean-like checks. Hence, I would also to keep the patch that checks if the environment variable is defined to enforce the behavior, without checking for a specific value. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
On 27.06.23 17:54, Dagfinn Ilmari Mannsåker wrote:
However, the docs
(https://www.postgresql.org/docs/16/regress-tap.html#REGRESS-TAP-VARS)
say "If the environment variable PG_TEST_NOCLEAN is set", not "is set to
a true value", and the existing test in PostgreSQL::Test::Cluster's END
block is:
# skip clean if we are requested to retain the basedir
next if defined $ENV{'PG_TEST_NOCLEAN'};
So the original `not defined` test is consistent with that.
Right, the usual style is just to check whether an environment variable
is set to something, not what it is.
Also note that in general not all environment variables are processed by
Perl, so I would avoid encoding Perl semantics about what is "true" or
whatever into it.
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
On 2023-06-27 Tu 11:54, Dagfinn Ilmari Mannsåker wrote:
Andrew Dunstan writes:
On 2023-06-26 Mo 19:55, Jacob Champion wrote:
Hello,
I was running the test_pg_dump extension suite, and I got annoyed that
I couldn't keep it from deleting its dump artifacts after a successful
run. Here's a patch to make use of PG_TEST_NOCLEAN (which currently
covers the test cluster's base directory) with the Test::Utils
tempdirs too.
(Looks like this idea was also discussed last year [1]; let me know if
I missed any more recent suggestions.)
- CLEANUP => 1);
+ CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});
This doesn't look quite right. If PG_TEST_CLEAN had a value of 0 we
would still do the cleanup. I would probably use something like:
CLEANUP => $ENV{'PG_TEST_NOCLEAN'} // 1
i.e. if it's not defined at all or has a value of undef, do the cleanup,
otherwise use the value.
If the environment varible were used as a boolean, it should be
CLEANUP => not $ENV{PG_TEST_NOCLEAN}
since `not undef` returns true with no warning, and the senses of the
two flags are inverted.
However, the docs
(https://www.postgresql.org/docs/16/regress-tap.html#REGRESS-TAP-VARS)
say "If the environment variable PG_TEST_NOCLEAN is set", not "is set to
a true value", and the existing test in PostgreSQL::Test::Cluster's END
block is:
# skip clean if we are requested to retain the basedir
next if defined $ENV{'PG_TEST_NOCLEAN'};
So the original `not defined` test is consistent with that.
ok, but ...
I think it's unwise to encourage setting environment variables without
values. Some years ago I had to work around some ugly warnings in
buildfarm logs by removing one such. I guess in the end it's a minor
issue, but if someone actually sets it to 0 it would seem to me like a
POLA violation still to skip the cleanup.
cheers
andew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
On 6/26/23 22:47, Michael Paquier wrote:
> src/test/perl/README and regress.sgml both describe what
> PG_TEST_NOCLEAN does, and it seems to me that these should be updated
> to tell that temporary files are not removed on top of the data
> folders?
I've added a couple of quick lines to the docs in v2; see what you think.
On 6/26/23 23:10, Daniel Gustafsson wrote:
> I think it simply got lost in that thread which had a lot of moving
> parts as it was.
I'll make sure to register it for the CF. :D
On 6/27/23 08:20, Andrew Dunstan wrote:
> This doesn't look quite right. If PG_TEST_CLEAN had a value of 0 we
would still do the cleanup.
That's how it currently works for the data directories, but Dagfinn beat
me to the punch:
On 6/27/23 08:54, Dagfinn Ilmari Mannsåker wrote:
> However, the docs
> (https://www.postgresql.org/docs/16/regress-tap.html#REGRESS-TAP-VARS)
> say "If the environment variable PG_TEST_NOCLEAN is set", not "is set to
> a true value", and the existing test in PostgreSQL::Test::Cluster's END
> block is:
>
> # skip clean if we are requested to retain the basedir
> next if defined $ENV{'PG_TEST_NOCLEAN'};
>
> So the original `not defined` test is consistent with that.
Right. The second patch in v2 now changes that behavior across the
board, so we handle false values. I'm ambivalent on changing the wording
of the docs, but I can do that too if needed. (I'm pretty used to the
phrase "setting an environment variable" implying some sort of
true/false handling, when the envvar is a boolean toggle.)
Thanks all!
--JacobFrom 2f403265509b66879d2d3396a9da4a6ba0dded6d Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Fri, 23 Jun 2023 13:30:20 -0700
Subject: [PATCH v2 1/2] Test::Utils: honor PG_TEST_NOCLEAN for tempdirs
---
doc/src/sgml/regress.sgml | 2 ++
src/test/perl/PostgreSQL/Test/Utils.pm | 6 +++---
src/test/perl/README | 5 +++--
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 88a43b8961..675db86e4d 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -818,6 +818,8 @@ make check PROVE_TESTS='t/001_test1.pl t/003_test3.pl'
PG_TEST_NOCLEAN=1 make -C src/bin/pg_dump check
+This environment variable also prevents the test's temporary directories
+from being removed.
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index a27fac83d2..e5a08d7b1a 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -272,7 +272,7 @@ sub all_tests_passing
Securely create a temporary directory inside C<$tmp_check>, like C,
and return its name. The directory will be removed automatically at the
-end of the tests.
+end of the tests, unless the PG_TEST_NOCLEAN envvar is provided.
If C is given, the new directory is templated as C<${prefix}_>.
Otherwise the template is C.
@@ -286,7 +286,7 @@ sub tempdir
return File::Temp::tempdir(
$prefix . '_',
DIR => $tmp_check,
- CLEANUP => 1);
+ CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});
}
=pod
@@ -301,7 +301,7 @@ name, to avoid path length issues.
sub tempdir_short
{
- return File::Temp::tempdir(CLEANUP => 1);
+ return File::Temp::tempdir(CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});
}
=pod
diff --git a/src/test/perl/README b/src/test/perl/README
index 6ddee42a10..43f6431f6e 100644
--- a/src/test/perl/README
+++ b/src/test/perl/README
@@ -31,8 +31,9 @@ some lesser number of seconds.
Data directories will also be left behind for analysis when a test fails;
they are named according to the test filename. But if the environment
-variable PG_TEST_NOCLEAN is set, data directories will be retained
-regardless of test status.
+variable PG_TEST_NOCLEAN is set, those directories will be retained
+regardless of test status. This environment variable also prevents the
+test's temporary directories from being removed.
Writing tests
--
2.25.1
From 42299731884b939a90495d04fe3e8411d0a2a97d Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Tue, 27 Jun 2023 08:43:19 -0700
Subject: [PATCH v2 2/2] test/perl: clean up when PG_TEST_NOCLEAN=0
Prior to this patch, setting PG_TEST_NOCLEAN=0 (or PG_TEST_NOCLEAN=)
still would have skipped the cleanup step.
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 2 +-
src/test/perl/PostgreSQL/Test/Utils.pm | 5 ++---
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 5e161dbee6..f48e935437 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1637,7 +1637,7 @@ END
$node->teardown_node(fail_ok => 1);
# skip clean if we are requested to retain the basedir
- next if defined $ENV{'PG_TEST_NOCLEAN'};
+ next if $ENV{PG_TEST_NOCLEAN};
# clean basedir on
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
Andrew Dunstan writes:
> On 2023-06-26 Mo 19:55, Jacob Champion wrote:
>> Hello,
>>
>> I was running the test_pg_dump extension suite, and I got annoyed that
>> I couldn't keep it from deleting its dump artifacts after a successful
>> run. Here's a patch to make use of PG_TEST_NOCLEAN (which currently
>> covers the test cluster's base directory) with the Test::Utils
>> tempdirs too.
>>
>> (Looks like this idea was also discussed last year [1]; let me know if
>> I missed any more recent suggestions.)
>
>
> - CLEANUP => 1);
> + CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});
>
>
> This doesn't look quite right. If PG_TEST_CLEAN had a value of 0 we
> would still do the cleanup. I would probably use something like:
>
> CLEANUP => $ENV{'PG_TEST_NOCLEAN'} // 1
>
> i.e. if it's not defined at all or has a value of undef, do the cleanup,
> otherwise use the value.
If the environment varible were used as a boolean, it should be
CLEANUP => not $ENV{PG_TEST_NOCLEAN}
since `not undef` returns true with no warning, and the senses of the
two flags are inverted.
However, the docs
(https://www.postgresql.org/docs/16/regress-tap.html#REGRESS-TAP-VARS)
say "If the environment variable PG_TEST_NOCLEAN is set", not "is set to
a true value", and the existing test in PostgreSQL::Test::Cluster's END
block is:
# skip clean if we are requested to retain the basedir
next if defined $ENV{'PG_TEST_NOCLEAN'};
So the original `not defined` test is consistent with that.
Tangentially, even though the above line contradicts it, the general
perl style is to not unnecessarily quote hash keys or words before `=>`:
~/src/postgresql $ rg -P -t perl '\{\s*\w+\s*\}' | wc -l
1662
~/src/postgresql $ rg -P -t perl '\{\s*(["'\''])\w+\1\s*\}' | wc -l
155
~/src/postgresql $ rg -P -t perl '\w+\s*=>' | wc -l
3842
~/src/postgresql $ rg -P -t perl '(["'\''])\w+\1\s*=>' | wc -l
310
- ilmari
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
On 2023-06-26 Mo 19:55, Jacob Champion wrote:
Hello,
I was running the test_pg_dump extension suite, and I got annoyed that
I couldn't keep it from deleting its dump artifacts after a successful
run. Here's a patch to make use of PG_TEST_NOCLEAN (which currently
covers the test cluster's base directory) with the Test::Utils
tempdirs too.
(Looks like this idea was also discussed last year [1]; let me know if
I missed any more recent suggestions.)
- CLEANUP => 1);
+ CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});
This doesn't look quite right. If PG_TEST_CLEAN had a value of 0 we
would still do the cleanup. I would probably use something like:
CLEANUP => $ENV{'PG_TEST_NOCLEAN'} // 1
i.e. if it's not defined at all or has a value of undef, do the cleanup,
otherwise use the value.
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
> On 27 Jun 2023, at 07:47, Michael Paquier wrote: > > On Mon, Jun 26, 2023 at 04:55:47PM -0700, Jacob Champion wrote: >> I was running the test_pg_dump extension suite, and I got annoyed that >> I couldn't keep it from deleting its dump artifacts after a successful >> run. Here's a patch to make use of PG_TEST_NOCLEAN (which currently >> covers the test cluster's base directory) with the Test::Utils >> tempdirs too. > > I am still +1 in doing that. > >> (Looks like this idea was also discussed last year [1]; let me know if >> I missed any more recent suggestions.) +1. I think it simply got lost in that thread which had a lot of moving parts as it was. -- Daniel Gustafsson
Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs
On Mon, Jun 26, 2023 at 04:55:47PM -0700, Jacob Champion wrote: > I was running the test_pg_dump extension suite, and I got annoyed that > I couldn't keep it from deleting its dump artifacts after a successful > run. Here's a patch to make use of PG_TEST_NOCLEAN (which currently > covers the test cluster's base directory) with the Test::Utils > tempdirs too. I am still +1 in doing that. > (Looks like this idea was also discussed last year [1]; let me know if > I missed any more recent suggestions.) I don't recall any specific suggestions related to that, but perhaps it got mentioned somewhere else. src/test/perl/README and regress.sgml both describe what PG_TEST_NOCLEAN does, and it seems to me that these should be updated to tell that temporary files are not removed on top of the data folders? -- Michael signature.asc Description: PGP signature
[PATCH] Honor PG_TEST_NOCLEAN for tempdirs
Hello,
I was running the test_pg_dump extension suite, and I got annoyed that
I couldn't keep it from deleting its dump artifacts after a successful
run. Here's a patch to make use of PG_TEST_NOCLEAN (which currently
covers the test cluster's base directory) with the Test::Utils
tempdirs too.
(Looks like this idea was also discussed last year [1]; let me know if
I missed any more recent suggestions.)
Thanks,
--Jacob
[1] https://www.postgresql.org/message-id/yypd9unv14sx2...@paquier.xyz
From 4e00947b66edc83ab66a06aa8a9f4c1591a2734c Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Fri, 23 Jun 2023 13:30:20 -0700
Subject: [PATCH] Test::Utils: honor PG_TEST_NOCLEAN for tempdirs
---
src/test/perl/PostgreSQL/Test/Utils.pm | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index a27fac83d2..e5a08d7b1a 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -272,7 +272,7 @@ sub all_tests_passing
Securely create a temporary directory inside C<$tmp_check>, like C,
and return its name. The directory will be removed automatically at the
-end of the tests.
+end of the tests, unless the PG_TEST_NOCLEAN envvar is provided.
If C is given, the new directory is templated as C<${prefix}_>.
Otherwise the template is C.
@@ -286,7 +286,7 @@ sub tempdir
return File::Temp::tempdir(
$prefix . '_',
DIR => $tmp_check,
- CLEANUP => 1);
+ CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});
}
=pod
@@ -301,7 +301,7 @@ name, to avoid path length issues.
sub tempdir_short
{
- return File::Temp::tempdir(CLEANUP => 1);
+ return File::Temp::tempdir(CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});
}
=pod
--
2.25.1
