From: | Andrew Dunstan <andrew(at)dunslane(dot)net> |
---|---|
To: | Dagfinn Ilmari Mannsåker <ilmari(at)ilmari(dot)org> |
Cc: | Jacob Champion <jchampion(at)timescale(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs |
Date: | 2023-06-27 18:45:26 |
Message-ID: | 44575677-27f5-ae40-32da-a8c93166659e@dunslane.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2023-06-27 Tu 11:54, Dagfinn Ilmari Mannsåker wrote:
> Andrew Dunstan<andrew(at)dunslane(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Ranier Vilela | 2023-06-27 18:55:23 | Re: POC, WIP: OR-clause support for indexes |
Previous Message | Andres Freund | 2023-06-27 18:17:46 | Re: Add GUC to tune glibc's malloc implementation. |