Re: Using AF_UNIX sockets always for tests on Windows

2024-02-01 Thread vignesh C
On Sun, 21 Jan 2024 at 18:01, vignesh C  wrote:
>
> On Wed, 22 Mar 2023 at 09:59, Thomas Munro  wrote:
> >
> > Thanks Tom, Andres, Juan José, Andrew for your feedback.  I agree that
> > it's a better "OS harmonisation" outcome if we can choose both ways on
> > both OSes.  I will leave the 0003 patch aside for now due to lack of
> > time, but here's a rebase of the first two patches.  Since this is
> > really just more cleanup-obsolete-stuff background work, I'm going to
> > move it to the next CF.
> >
> > 0001 -- Teach mkdtemp() to care about permissions on Windows.
> > Reviewed by Juan José, who confirmed my blind-coded understanding and
> > showed an alternative version but didn't suggest doing it that way.
> > It's a little confusing that NULL "attributes" means something
> > different from NULL "descriptor", so I figured I should highlight that
> > difference more clearly with some new comments.  I guess one question
> > is "why should we expect the calling process to have the default
> > access token?"
> >
> > 0002 -- Use AF_UNIX for pg_regress.  This one removes a couple of
> > hundred Windows-only lines that set up SSPI stuff, and some comments
> > about a signal handling hazard that -- as far as I can tell by reading
> > manuals -- was never really true.
> >
> > 0003 -- TAP testing adjustments needs some more work based on feedback
> > already given, not included in this revision.
>
> The patch does not apply anymore:
> patch -p1 < v3-0002-Always-use-AF_UNIX-sockets-in-pg_regress-on-Windo.patch
> patching file src/test/regress/pg_regress.c
> ...
> Hunk #6 FAILED at 781.
> ...
> 1 out of 10 hunks FAILED -- saving rejects to file
> src/test/regress/pg_regress.c.rej

With no update to the thread and the patch not applying I'm marking
this as returned with feedback. Please feel free to resubmit to the
next CF when there is a new version of the patch.

Regards,
Vignesh




Re: Using AF_UNIX sockets always for tests on Windows

2024-01-21 Thread vignesh C
On Wed, 22 Mar 2023 at 09:59, Thomas Munro  wrote:
>
> Thanks Tom, Andres, Juan José, Andrew for your feedback.  I agree that
> it's a better "OS harmonisation" outcome if we can choose both ways on
> both OSes.  I will leave the 0003 patch aside for now due to lack of
> time, but here's a rebase of the first two patches.  Since this is
> really just more cleanup-obsolete-stuff background work, I'm going to
> move it to the next CF.
>
> 0001 -- Teach mkdtemp() to care about permissions on Windows.
> Reviewed by Juan José, who confirmed my blind-coded understanding and
> showed an alternative version but didn't suggest doing it that way.
> It's a little confusing that NULL "attributes" means something
> different from NULL "descriptor", so I figured I should highlight that
> difference more clearly with some new comments.  I guess one question
> is "why should we expect the calling process to have the default
> access token?"
>
> 0002 -- Use AF_UNIX for pg_regress.  This one removes a couple of
> hundred Windows-only lines that set up SSPI stuff, and some comments
> about a signal handling hazard that -- as far as I can tell by reading
> manuals -- was never really true.
>
> 0003 -- TAP testing adjustments needs some more work based on feedback
> already given, not included in this revision.

The patch does not apply anymore:
patch -p1 < v3-0002-Always-use-AF_UNIX-sockets-in-pg_regress-on-Windo.patch
patching file src/test/regress/pg_regress.c
...
Hunk #6 FAILED at 781.
...
1 out of 10 hunks FAILED -- saving rejects to file
src/test/regress/pg_regress.c.rej

Regards,
Vignesh




Re: Using AF_UNIX sockets always for tests on Windows

2023-03-21 Thread Thomas Munro
Thanks Tom, Andres, Juan José, Andrew for your feedback.  I agree that
it's a better "OS harmonisation" outcome if we can choose both ways on
both OSes.  I will leave the 0003 patch aside for now due to lack of
time, but here's a rebase of the first two patches.  Since this is
really just more cleanup-obsolete-stuff background work, I'm going to
move it to the next CF.

0001 -- Teach mkdtemp() to care about permissions on Windows.
Reviewed by Juan José, who confirmed my blind-coded understanding and
showed an alternative version but didn't suggest doing it that way.
It's a little confusing that NULL "attributes" means something
different from NULL "descriptor", so I figured I should highlight that
difference more clearly with some new comments.  I guess one question
is "why should we expect the calling process to have the default
access token?"

0002 -- Use AF_UNIX for pg_regress.  This one removes a couple of
hundred Windows-only lines that set up SSPI stuff, and some comments
about a signal handling hazard that -- as far as I can tell by reading
manuals -- was never really true.

0003 -- TAP testing adjustments needs some more work based on feedback
already given, not included in this revision.
From e7fac7a15ed0eda6516e7fa0917c06e005341b00 Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Wed, 7 Sep 2022 07:35:11 +1200
Subject: [PATCH v3 1/2] Make mkdtemp() more secure on Windows.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Our POSIX mkdtemp() implementation in src/port/mkdtemp.c code would
create directories with default permissions on Windows.  Fix, using the
native Windows API instead of mkdir().

This function is currently used by pg_regress's make_temp_sockdir().

Reviewed-by: Juan José Santamaría Flecha 
Discussion: https://postgr.es/m/CA%2BhUKGK30uLx9dpgkYwomgH0WVLUHytkChDgf3iUM2zp0pf_nA%40mail.gmail.com
---
 src/port/mkdtemp.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/src/port/mkdtemp.c b/src/port/mkdtemp.c
index 4578e8384c..9d3c4fce71 100644
--- a/src/port/mkdtemp.c
+++ b/src/port/mkdtemp.c
@@ -187,8 +187,35 @@ GETTEMP(char *path, int *doopen, int domkdir)
 		}
 		else if (domkdir)
 		{
+#ifdef WIN32
+			/*
+			 * Plain mkdir(path, 0700) would ignore the mode argument, so
+			 * we'll use the native Windows API to create the directory.  By
+			 * setting lpSecurityDescriptor to NULL, we get "the default
+			 * security descriptor associated with the access token of the
+			 * calling process.  [...]  By default, the default DACL in the
+			 * access token of a process allows access only to the user
+			 * represented by the access token."
+			 *
+			 * Note that a NULL lpSecurityDescriptor is not the same as a NULL
+			 * lpSecurityAttributes argument.  The latter would mean that the
+			 * ACL is inherited from the parent directory, which would
+			 * probably work out the same if it's the TMP directory, but by a
+			 * different route.
+			 */
+			SECURITY_ATTRIBUTES sa = {
+.nLength = sizeof(SECURITY_ATTRIBUTES),
+.lpSecurityDescriptor = NULL,
+.bInheritHandle = false
+			};
+
+			if (CreateDirectory(path, ))
+return 1;
+			_dosmaperr(GetLastError());
+#else
 			if (mkdir(path, 0700) >= 0)
 return 1;
+#endif
 			if (errno != EEXIST)
 return 0;
 		}
-- 
2.39.2

From 43ae66dc965d807dded8d434b7a1ea0b3f12e986 Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Fri, 19 Aug 2022 11:28:38 +1200
Subject: [PATCH v3 2/2] Always use AF_UNIX sockets in pg_regress on Windows.

Since we can now rely on AF_UNIX sockets working on supported Windows
versions (10+), we can remove some Windows-only code for setting up
secure TCP in pg_regress.

Previously, we thought the socket cleanup code was unsafe, so we made
Unix-domain sockets an option with a "use-at-your-own-risk" note.  On
closer inspection, the concerns about signal handlers don't seem to
apply here.  (initdb.c has similar concerns but needs separate
investigation.)

Previously, commit f6dc6dd5 secured temporary installations using TCP/IP
on Windows, while commit be76a6d3 used file system permissions for Unix.
Now that our src/port/mkdtemp.c file creates non-world-accessible
directories on Windows, we can just do the same on Windows.

Note that this doesn't affect the TAP tests, which continue to use
TCP/IP on Windows by default.

Discussion: https://postgr.es/m/CA%2BhUKGK30uLx9dpgkYwomgH0WVLUHytkChDgf3iUM2zp0pf_nA%40mail.gmail.com
---
 src/test/regress/pg_regress.c | 274 --
 1 file changed, 32 insertions(+), 242 deletions(-)

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 7b23cc80dc..9c3251ca3b 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -55,6 +55,20 @@ char	   *host_platform = HOST_TUPLE;
 static char *shellprog = SHELLPROG;
 #endif
 
+/*
+ * The name of the environment variable that controls where we put temporary
+ * files, to override 

Re: Using AF_UNIX sockets always for tests on Windows

2023-01-16 Thread Juan José Santamaría Flecha
Hello,

On Fri, Dec 2, 2022 at 1:03 AM Thomas Munro  wrote:

>
> 1.  Teach mkdtemp() to make a non-world-accessible directory.  This is
> required to be able to make a socket that other processes can't
> connect to, to match the paranoia level used on Unix.  This was
> written just by reading documentation, because I am not a Windows
> user, so I would be grateful for a second opinion and/or testing from
> a Windows hacker, which would involve testing with two different
> users.  The idea is that Windows' mkdir() is completely ignoring the
> permissions (we can see in the mingw headers that it literally throws
> away the mode argument), so we shouldn't use that, but native
> CreateDirectory() when given a pointer to a SECURITY_ATTRIBUTES with
> lpSecurityDesciptor set to NULL should only allow the current user to
> access the object (directory).  Does this really work, and would it be
> better to create some more explicit private-keep-out
> SECURITY_ATTRIBUTE, and how would that look?
>

A directory created with a NULL SECURITY_ATTRIBUTES inherits the ACL from
its parent directory [1]. In this case, its parent is the designated
temporary location, which already should have a limited access.

You can create an explicit DACL for that directory, PFA a patch for so.
This is just an example, not something that I'm proposing as a committable
alternative.

I'm fairly sure that filesystem permissions must be enough to stop
> another OS user from connecting, because it's clear from documentation
> that AF_UNIX works on Windows by opening the file and reading some
> magic "reparse" data from inside it, so if you can't see into the
> containing directory, you can't do it.  This patch is the one the rest
> are standing on, because the tests should match Unix in their level of
> security.
>

Yes, this is correct.

>
> Only the first patch is modified, but I'm including all of them so they go
through the cfbot.

[1]
https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea


Regards,

Juan José Santamaría Flecha


v2-0004-Doc-Abstract-AF_UNIX-sockets-don-t-work-on-Windows-a.patch
Description: Binary data


v2-0003-WIP-Stop-using-TCP-in-TAP-tests-on-Windows.patch
Description: Binary data


v2-0002-WIP-Always-use-Unix-domain-sockets-in-pg_regress-on-.patch
Description: Binary data


v2-0001-WIP-Make-mkdtemp-more-secure-on-Windows.patch
Description: Binary data


Re: Using AF_UNIX sockets always for tests on Windows

2023-01-05 Thread Andrew Dunstan


On 2023-01-04 We 07:13, vignesh C wrote:
> On Fri, 2 Dec 2022 at 18:08, Andrew Dunstan  wrote:
>>
>> On 2022-12-01 Th 21:10, Andres Freund wrote:
>>> Hi,
>>>
>>> On 2022-12-01 20:56:18 -0500, Tom Lane wrote:
 Andres Freund  writes:
> On 2022-12-01 20:30:36 -0500, Tom Lane wrote:
>> If we remove that, won't we have a whole lot of code that's not
>> tested at all on any platform, ie all the TCP-socket code?
> There's some coverage via the auth and ssl tests. But I agree it's an
> issue. But to me the fix for that seems to be to add a dedicated test for
> that, rather than relying on windows to test our socket code - that's 
> quite a
> few separate code paths from the tcp support of other platforms.
 IMO that's not the best way forward, because you'll always have
 nagging questions about whether a single-purpose test covers
 everything that needs coverage.
>>> Still seems better than not having any coverage in our development
>>> environments...
>>>
>>>
 I think the best place to be in would be to be able to run the whole test
 suite using either TCP or UNIX sockets, on any platform (with stuff like 
 the
 SSL test overriding the choice as needed).
>>> I agree that that's useful. But it seems somewhat independent from the
>>> majority of the proposed changes. To be able to test force-tcp-everywhere we
>>> don't need e.g.  code for setting sspi auth in pg_regress etc - it's afaik
>>> just needed so there's a secure way of running tests at all on windows.
>>>
>>> I think 0003 should be "trimmed" to only change the default for
>>> $use_unix_sockets on windows and to remove PG_TEST_USE_UNIX_SOCKETS. Whoever
>>> wants to, can then add a new environment variable to force tap tests to use
>>> tcp.
>>>
>> Not sure if it's useful here, but a few months ago I prepared patches to
>> remove the config-auth option of pg_regress, which struck me as more
>> than odd, and replace it with a perl module. I didn't get around to
>> finishing them, but the patches as of then are attached.
>>
>> I agree that having some switch that says "run everything with TCP" or
>> "run (almost) everything with Unix sockets" would be good.
> The patch does not apply on top of HEAD as in [1], please post a rebased 
> patch:
> === Applying patches on top of PostgreSQL commit ID
> bf03cfd162176d543da79f9398131abc251ddbb9 ===
> === applying patch
> ./0001-Do-config_auth-in-perl-code-for-TAP-tests-and-vcregr.patch
> patching file contrib/basebackup_to_shell/t/001_basic.pl
> Hunk #1 FAILED at 21.
> 1 out of 1 hunk FAILED -- saving rejects to file
> contrib/basebackup_to_shell/t/001_basic.pl.rej
> patching file src/bin/pg_basebackup/t/010_pg_basebackup.pl
> Hunk #1 FAILED at 29.
> 1 out of 1 hunk FAILED -- saving rejects to file
> src/bin/pg_basebackup/t/010_pg_basebackup.pl.rej
> Hunk #3 FAILED at 461.
> 1 out of 3 hunks FAILED -- saving rejects to file
> src/test/perl/PostgreSQL/Test/Cluster.pm.rej
>
> [1] - http://cfbot.cputube.org/patch_41_4033.log
>

What I posted was not intended as a replacement for Thomas' patches, or
indeed meant as a CF item at all.

So really we're waiting on Thomas to post a response to Tom's and
Andres' comments upthread.


cheers


andrew

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





Re: Using AF_UNIX sockets always for tests on Windows

2023-01-04 Thread vignesh C
On Fri, 2 Dec 2022 at 18:08, Andrew Dunstan  wrote:
>
>
> On 2022-12-01 Th 21:10, Andres Freund wrote:
> > Hi,
> >
> > On 2022-12-01 20:56:18 -0500, Tom Lane wrote:
> >> Andres Freund  writes:
> >>> On 2022-12-01 20:30:36 -0500, Tom Lane wrote:
>  If we remove that, won't we have a whole lot of code that's not
>  tested at all on any platform, ie all the TCP-socket code?
> >>> There's some coverage via the auth and ssl tests. But I agree it's an
> >>> issue. But to me the fix for that seems to be to add a dedicated test for
> >>> that, rather than relying on windows to test our socket code - that's 
> >>> quite a
> >>> few separate code paths from the tcp support of other platforms.
> >> IMO that's not the best way forward, because you'll always have
> >> nagging questions about whether a single-purpose test covers
> >> everything that needs coverage.
> > Still seems better than not having any coverage in our development
> > environments...
> >
> >
> >> I think the best place to be in would be to be able to run the whole test
> >> suite using either TCP or UNIX sockets, on any platform (with stuff like 
> >> the
> >> SSL test overriding the choice as needed).
> > I agree that that's useful. But it seems somewhat independent from the
> > majority of the proposed changes. To be able to test force-tcp-everywhere we
> > don't need e.g.  code for setting sspi auth in pg_regress etc - it's afaik
> > just needed so there's a secure way of running tests at all on windows.
> >
> > I think 0003 should be "trimmed" to only change the default for
> > $use_unix_sockets on windows and to remove PG_TEST_USE_UNIX_SOCKETS. Whoever
> > wants to, can then add a new environment variable to force tap tests to use
> > tcp.
> >
>
> Not sure if it's useful here, but a few months ago I prepared patches to
> remove the config-auth option of pg_regress, which struck me as more
> than odd, and replace it with a perl module. I didn't get around to
> finishing them, but the patches as of then are attached.
>
> I agree that having some switch that says "run everything with TCP" or
> "run (almost) everything with Unix sockets" would be good.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
bf03cfd162176d543da79f9398131abc251ddbb9 ===
=== applying patch
./0001-Do-config_auth-in-perl-code-for-TAP-tests-and-vcregr.patch
patching file contrib/basebackup_to_shell/t/001_basic.pl
Hunk #1 FAILED at 21.
1 out of 1 hunk FAILED -- saving rejects to file
contrib/basebackup_to_shell/t/001_basic.pl.rej
patching file src/bin/pg_basebackup/t/010_pg_basebackup.pl
Hunk #1 FAILED at 29.
1 out of 1 hunk FAILED -- saving rejects to file
src/bin/pg_basebackup/t/010_pg_basebackup.pl.rej
Hunk #3 FAILED at 461.
1 out of 3 hunks FAILED -- saving rejects to file
src/test/perl/PostgreSQL/Test/Cluster.pm.rej

[1] - http://cfbot.cputube.org/patch_41_4033.log

Regards,
Vignesh




Re: Using AF_UNIX sockets always for tests on Windows

2022-12-02 Thread Andrew Dunstan

On 2022-12-01 Th 21:10, Andres Freund wrote:
> Hi,
>
> On 2022-12-01 20:56:18 -0500, Tom Lane wrote:
>> Andres Freund  writes:
>>> On 2022-12-01 20:30:36 -0500, Tom Lane wrote:
 If we remove that, won't we have a whole lot of code that's not
 tested at all on any platform, ie all the TCP-socket code?
>>> There's some coverage via the auth and ssl tests. But I agree it's an
>>> issue. But to me the fix for that seems to be to add a dedicated test for
>>> that, rather than relying on windows to test our socket code - that's quite 
>>> a
>>> few separate code paths from the tcp support of other platforms.
>> IMO that's not the best way forward, because you'll always have
>> nagging questions about whether a single-purpose test covers
>> everything that needs coverage.
> Still seems better than not having any coverage in our development
> environments...
>
>
>> I think the best place to be in would be to be able to run the whole test
>> suite using either TCP or UNIX sockets, on any platform (with stuff like the
>> SSL test overriding the choice as needed).
> I agree that that's useful. But it seems somewhat independent from the
> majority of the proposed changes. To be able to test force-tcp-everywhere we
> don't need e.g.  code for setting sspi auth in pg_regress etc - it's afaik
> just needed so there's a secure way of running tests at all on windows.
>
> I think 0003 should be "trimmed" to only change the default for
> $use_unix_sockets on windows and to remove PG_TEST_USE_UNIX_SOCKETS. Whoever
> wants to, can then add a new environment variable to force tap tests to use
> tcp.
>

Not sure if it's useful here, but a few months ago I prepared patches to
remove the config-auth option of pg_regress, which struck me as more
than odd, and replace it with a perl module. I didn't get around to
finishing them, but the patches as of then are attached.

I agree that having some switch that says "run everything with TCP" or
"run (almost) everything with Unix sockets" would be good.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com
From 5c7fad30aa2db2d47d0cc3869f132ca9d14a8814 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan 
Date: Tue, 26 Apr 2022 10:49:26 -0400
Subject: [PATCH 1/2] Do config_auth in perl code for TAP tests and
 vcregress.pl.

That means there is no longer any need to call pg_regress --config-auth
to set up Windows authentication. Instead a simple perl function call
does the work.
---
 contrib/basebackup_to_shell/t/001_basic.pl   |   2 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl |   2 +-
 src/bin/pg_ctl/t/001_start_stop.pl   |   4 +-
 src/bin/pg_dump/t/010_dump_connstr.pl|  16 ++-
 src/bin/pg_rewind/t/RewindTest.pm|   2 +-
 src/test/perl/PostgreSQL/Test/Cluster.pm |   7 +-
 src/test/perl/PostgreSQL/Test/ConfigAuth.pm  | 115 +++
 src/test/recovery/t/001_stream_rep.pl|   2 +-
 src/tools/msvc/vcregress.pl  |   8 +-
 9 files changed, 135 insertions(+), 23 deletions(-)
 create mode 100644 src/test/perl/PostgreSQL/Test/ConfigAuth.pm

diff --git a/contrib/basebackup_to_shell/t/001_basic.pl b/contrib/basebackup_to_shell/t/001_basic.pl
index 350d42079a..b5de23b48f 100644
--- a/contrib/basebackup_to_shell/t/001_basic.pl
+++ b/contrib/basebackup_to_shell/t/001_basic.pl
@@ -21,7 +21,7 @@ my $node = PostgreSQL::Test::Cluster->new('primary');
 # Make sure pg_hba.conf is set up to allow connections from backupuser.
 # This is only needed on Windows machines that don't use UNIX sockets.
 $node->init('allows_streaming' => 1,
-			'auth_extra' => [ '--create-role', 'backupuser' ]);
+			'auth_extra' => [ extra_roles => 'backupuser' ]);
 
 $node->append_conf('postgresql.conf',
    "shared_preload_libraries = 'basebackup_to_shell'");
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 056fcf3597..a0f4fd7b92 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -29,7 +29,7 @@ umask(0077);
 
 # Initialize node without replication settings
 $node->init(extra => ['--data-checksums'],
-			auth_extra => [ '--create-role', 'backupuser' ]);
+			auth_extra => [ extra_roles => 'backupuser' ]);
 $node->start;
 my $pgdata = $node->data_dir;
 
diff --git a/src/bin/pg_ctl/t/001_start_stop.pl b/src/bin/pg_ctl/t/001_start_stop.pl
index fdffd76d99..c92b06ad85 100644
--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -4,6 +4,7 @@
 use strict;
 use warnings;
 
+use PostgreSQL::Test::ConfigAuth;
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
@@ -20,8 +21,7 @@ command_exit_is([ 'pg_ctl', 'start', '-D', "$tempdir/nonexistent" ],
 
 command_ok([ 'pg_ctl', 'initdb', '-D', "$tempdir/data", '-o', '-N' ],
 	'pg_ctl initdb');
-command_ok([ $ENV{PG_REGRESS}, '--config-auth', "$tempdir/data" ],
-	'configure authentication');
+config_auth("$tempdir/data");
 my 

Re: Using AF_UNIX sockets always for tests on Windows

2022-12-01 Thread Andres Freund
Hi,

On 2022-12-01 20:56:18 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2022-12-01 20:30:36 -0500, Tom Lane wrote:
> >> If we remove that, won't we have a whole lot of code that's not
> >> tested at all on any platform, ie all the TCP-socket code?
> 
> > There's some coverage via the auth and ssl tests. But I agree it's an
> > issue. But to me the fix for that seems to be to add a dedicated test for
> > that, rather than relying on windows to test our socket code - that's quite 
> > a
> > few separate code paths from the tcp support of other platforms.
> 
> IMO that's not the best way forward, because you'll always have
> nagging questions about whether a single-purpose test covers
> everything that needs coverage.

Still seems better than not having any coverage in our development
environments...


> I think the best place to be in would be to be able to run the whole test
> suite using either TCP or UNIX sockets, on any platform (with stuff like the
> SSL test overriding the choice as needed).

I agree that that's useful. But it seems somewhat independent from the
majority of the proposed changes. To be able to test force-tcp-everywhere we
don't need e.g.  code for setting sspi auth in pg_regress etc - it's afaik
just needed so there's a secure way of running tests at all on windows.

I think 0003 should be "trimmed" to only change the default for
$use_unix_sockets on windows and to remove PG_TEST_USE_UNIX_SOCKETS. Whoever
wants to, can then add a new environment variable to force tap tests to use
tcp.

Greetings,

Andres Freund




Re: Using AF_UNIX sockets always for tests on Windows

2022-12-01 Thread Tom Lane
Andres Freund  writes:
> On 2022-12-01 20:30:36 -0500, Tom Lane wrote:
>> If we remove that, won't we have a whole lot of code that's not
>> tested at all on any platform, ie all the TCP-socket code?

> There's some coverage via the auth and ssl tests. But I agree it's an
> issue. But to me the fix for that seems to be to add a dedicated test for
> that, rather than relying on windows to test our socket code - that's quite a
> few separate code paths from the tcp support of other platforms.

IMO that's not the best way forward, because you'll always have
nagging questions about whether a single-purpose test covers
everything that needs coverage.  I think the best place to be in
would be to be able to run the whole test suite using either TCP or
UNIX sockets, on any platform (with stuff like the SSL test
overriding the choice as needed).  I doubt ripping out the existing
Windows-only support for TCP-based testing is a good step in that
direction.

regards, tom lane




Re: Using AF_UNIX sockets always for tests on Windows

2022-12-01 Thread Andres Freund
Hi,

On 2022-12-01 20:30:36 -0500, Tom Lane wrote:
> Thomas Munro  writes:
> > Commit f5580882 established that all supported computers have AF_UNIX.
> > One of the follow-up consequences that was left unfinished is that we
> > could simplify our test harness code to make it the same on all
> > platforms.  Currently we have hundreds of lines of C and perl to use
> > secure TCP connections instead for the benefit of defunct Windows
> > versions.  Here's a patch set for that.
> 
> If we remove that, won't we have a whole lot of code that's not
> tested at all on any platform, ie all the TCP-socket code?

There's some coverage via the auth and ssl tests. But I agree it's an
issue. But to me the fix for that seems to be to add a dedicated test for
that, rather than relying on windows to test our socket code - that's quite a
few separate code paths from the tcp support of other platforms.

Greetings,

Andres Freund




Re: Using AF_UNIX sockets always for tests on Windows

2022-12-01 Thread Tom Lane
Thomas Munro  writes:
> Commit f5580882 established that all supported computers have AF_UNIX.
> One of the follow-up consequences that was left unfinished is that we
> could simplify our test harness code to make it the same on all
> platforms.  Currently we have hundreds of lines of C and perl to use
> secure TCP connections instead for the benefit of defunct Windows
> versions.  Here's a patch set for that.

If we remove that, won't we have a whole lot of code that's not
tested at all on any platform, ie all the TCP-socket code?

regards, tom lane