Re: ssl tests aren't concurrency safe due to get_free_port()

On 05.05.23 00:33, Andrew Dunstan wrote:
> 
> On 2023-05-04 Th 02:40, Peter Eisentraut wrote:
>> On 25.04.23 12:27, Peter Eisentraut wrote:
>>> On 20.11.22 16:10, Andrew Dunstan wrote:
>>>>
>>>> On 2022-11-19 Sa 15:16, Andres Freund wrote:
>>>>> Hi,
>>>>>
>>>>> On 2022-11-19 10:56:33 -0500, Andrew Dunstan wrote:
>>>>>>> Perhaps we should just export a directory in configure instead of 
>>>>>>> this
>>>>>>> guessing game?
>>>>>> I think the obvious candidate would be to export top_builddir from
>>>>>> src/Makefile.global. That would remove the need to infer it from
>>>>>> TESTDATADIR.
>>>>> I think that'd be good. I'd perhaps rename it in the process so it's
>>>>> exported uppercase, but whatever...
>>>>>
>>>>
>>>> OK, pushed with a little more tweaking. I didn't upcase top_builddir
>>>> because the existing prove_installcheck recipes already export it and I
>>>> wanted to stay consistent with those.
>>>>
>>>> If it works ok I will backpatch in couple of days.
>>>
>>> These patches have affected pgxs-using extensions that have their own 
>>> TAP tests.
>>>
>>> The portlock directory is created at
>>>
>>>      my $build_dir = $ENV{top_builddir}
>>>        || $PostgreSQL::Test::Utils::tmp_check ;
>>>      $portdir ||= "$build_dir/portlock";
>>>
>>> but for a pgxs user, top_builddir points into the installation tree, 
>>> specifically at $prefix/lib/pgxs/.
>>>
>>> So when running "make installcheck" for an extension, we either won't 
>>> have write access to that directory, or if we do, then it's still not 
>>> good to write into the installation tree during a test suite.
>>>
>>> A possible fix is
>>>
>>> diff --git a/src/Makefile.global.in b/src/Makefile.global.in
>>> index 5dacc4d838..c493d1a60c 100644
>>> --- a/src/Makefile.global.in
>>> +++ b/src/Makefile.global.in
>>> @@ -464,7 +464,7 @@ rm -rf '$(CURDIR)'/tmp_check && \
>>>   $(MKDIR_P) '$(CURDIR)'/tmp_check && \
>>>   cd $(srcdir) && \
>>>      TESTDIR='$(CURDIR)' PATH="$(bindir):$(CURDIR):$$PATH" \
>>> -   PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' \
>>> +   PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)' \
>>>      PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' \
>>>      $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if 
>>> $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
>>>   endef
>>
>> Better idea: Remove the top_builddir assignment altogether.  I traced 
>> the history of this, and it seems like it was just dragged around with 
>> various other changes and doesn't have a purpose of its own.
>>
>> The only effect of the current code (top_builddir='$(top_builddir)') 
>> is to export top_builddir as an environment variable.  And the only 
>> Perl test code that reads that environment variable is the code that 
>> makes the portlock directory, which is exactly what we don't want.  So 
>> just removing that seems to be the right solution.
> 
> Yeah, that should be OK in the pgxs case.

I have committed this to all active branches.