Horton

Thread

Commits

  1. Make PG_TEST_NOCLEAN work for temporary directories in TAP tests

  1. [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Jacob Champion <jchampion@timescale.com> — 2023-06-26T23:55:47Z

    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

  2. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Michael Paquier <michael@paquier.xyz> — 2023-06-27T05:47:08Z

    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

  3. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Daniel Gustafsson <daniel@yesql.se> — 2023-06-27T06:10:42Z

    > 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

  4. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Andrew Dunstan <andrew@dunslane.net> — 2023-06-27T15:20:20Z

    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

  5. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2023-06-27T15:54:20Z

    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

  6. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Jacob Champion <jchampion@timescale.com> — 2023-06-27T16:47:09Z

    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

  7. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Andrew Dunstan <andrew@dunslane.net> — 2023-06-27T18:45:26Z

    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

  8. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Peter Eisentraut <peter@eisentraut.org> — 2023-06-28T08:45:02Z

    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.

  9. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Michael Paquier <michael@paquier.xyz> — 2023-06-29T00:40:53Z

    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

  10. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Jacob Champion <jchampion@timescale.com> — 2023-06-29T16:05:59Z

    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

  11. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Michael Paquier <michael@paquier.xyz> — 2023-06-30T07:09:03Z

    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

  12. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Daniel Gustafsson <daniel@yesql.se> — 2023-06-30T07:42:13Z

    > 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

  13. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Michael Paquier <michael@paquier.xyz> — 2023-07-03T01:17:24Z

    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

  14. Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

    Jacob Champion <jchampion@timescale.com> — 2023-07-05T19:55:16Z

    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