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

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.


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com