[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

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:
/messages/by-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:
/messages/by-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