Обсуждение: [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/YyPd9unV14SX2bLF@paquier.xyz
Вложения
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
Вложения
> On 27 Jun 2023, at 07:47, Michael Paquier <michael@paquier.xyz> 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
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
Andrew Dunstan <andrew@dunslane.net> 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
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! --Jacob
Вложения
On 2023-06-27 Tu 11:54, Dagfinn Ilmari Mannsåker wrote:
Andrew Dunstan <andrew@dunslane.net> 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
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.
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
Вложения
On Wed, Jun 28, 2023 at 5:41 PM Michael Paquier <michael@paquier.xyz> 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
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-7a744ea45fef@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
Вложения
> On 30 Jun 2023, at 09:09, Michael Paquier <michael@paquier.xyz> 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-7a744ea45fef@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
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
Вложения
On Sun, Jul 2, 2023 at 6:17 PM Michael Paquier <michael@paquier.xyz> wrote: > Adjusted that as well, on top of an extra comment. Thanks all! --Jacob