Cleaning up historical portability baggage

Thomas Munro <thomas.munro@gmail.com> writes:

I wonder how much dead code for ancient operating systems we could now
drop.

+1, it seems like this is the cycle for some housecleaning.

* prairiedog, macOS 10.4 (vintage system most likely to cause problems)

FWIW, I am expecting to retire prairiedog once the meson stuff drops.
macOS 10.4 is incapable of running ninja (for lack of <spawn.h>).
While I could keep it working for awhile with the autoconf build system,
I'm not sure I see the point. The hardware is still chugging along,
but I think it'd be more useful to run up-to-date NetBSD or the like
on it.

Having said that, I'll be happy to try out this patch series on
that platform and see if it burps.

regards, tom lane

I wrote:

Having said that, I'll be happy to try out this patch series on
that platform and see if it burps.

HEAD + patches 0001-0006 seems fine on prairiedog's host.
Builds clean (or as clean as HEAD does anyway), passes make check.
I did not trouble with check-world.

(I haven't actually read the patches, so this isn't a review,
just a quick smoke-test.)

regards, tom lane

On Sat, 9 Jul 2022 at 21:46, Thomas Munro <thomas.munro@gmail.com> wrote:

Hello,

I wonder how much dead code for ancient operating systems we could now
drop.

0002-Remove-dead-getrusage-replacement-code.patch

I thought the getrusage replacement code was for Windows. Does
getrusage on Windows actually do anything useful?

More generally I think there is a question about whether some of these
things are "supported" in only a minimal way to satisfy standards but
maybe not in a way that we actually want to use. Getrusage might exist
on Windows but not actually report the metrics we need, reentrant
library functions may be implemented by simply locking instead of
actually avoiding static storage, etc.

--
greg

(Reading the patch it seems both those points are already addressed)

On Sat, Jul 9, 2022 at 9:46 PM Thomas Munro <thomas.munro@gmail.com> wrote:

The pwritev/preadv functions are unfortunately not standardised by
POSIX (I dunno why, it's the obvious combination of the p* and *v
functions) despite every OS in the list having them except for Solaris
and old macOS. Oh well.

I don't think that 0001 is buying us a whole lot, really. I prefer the
style where we have PG-specific functions that behave differently on
different platforms to the one where we call something that looks like
a native OS function call on all platforms but on some of them it is
secretly invoking a replacement implementation in src/port. The
problem with the latter is it looks like you're using something that's
universally supported and works the same way everywhere, but you're
really not. If it were up to me, we'd have more pg_whatever() that
calls whatever() on non-Windows and something else on Windows, rather
than going in the direction that this patch takes us.

I like all of the other patches. Reducing the number of configure
tests that we need seems like a really good idea.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Jul 12, 2022 at 4:46 AM Robert Haas <robertmhaas@gmail.com> wrote:

I don't think that 0001 is buying us a whole lot, really. I prefer the
style where we have PG-specific functions that behave differently on
different platforms to the one where we call something that looks like
a native OS function call on all platforms but on some of them it is
secretly invoking a replacement implementation in src/port. The
problem with the latter is it looks like you're using something that's
universally supported and works the same way everywhere, but you're
really not. If it were up to me, we'd have more pg_whatever() that
calls whatever() on non-Windows and something else on Windows, rather
than going in the direction that this patch takes us.

Hmm, but that's not what we're doing in general. For example, on
Windows we're redirecting open() to a replacement function of our own,
we're not using "pg_open()" in our code. That's not an example based
on AC_REPLACE_FUNCS, but there are plenty of those too. Isn't this
quite well established?

AFAIK we generally only use pg_whatever() when there's a good reason,
such as an incompatibility, a complication or a different abstraction
that you want to highlight to a reader. The reason here was
temporary: we couldn't implement standard pread/pwrite perfectly on
ancient HP-UX, but we *can* implement it on Windows, so the reason is
gone.

These particular pg_ prefixes have only been in our tree for a few
years and I was hoping to boot them out again before they stick, like
"Size". I like using standard interfaces where possible for the very
basic stuff, to de-weird our stuff.

I like all of the other patches. Reducing the number of configure
tests that we need seems like a really good idea.

Thanks for looking. Yeah, we could also be a little more aggressive
about removing configure tests, in the cases where it's just Windows
vs !Windows. "HAVE_XXX" tests that are always true on POSIX systems
at the level we require would then be unnecessary.

On 12.07.22 03:10, Thomas Munro wrote:

AFAIK we generally only use pg_whatever() when there's a good reason,
such as an incompatibility, a complication or a different abstraction
that you want to highlight to a reader. The reason here was
temporary: we couldn't implement standard pread/pwrite perfectly on
ancient HP-UX, but we*can* implement it on Windows, so the reason is
gone.

These particular pg_ prefixes have only been in our tree for a few
years and I was hoping to boot them out again before they stick, like
"Size". I like using standard interfaces where possible for the very
basic stuff, to de-weird our stuff.

I agree. That's been the established approach.

On Mon, Jul 11, 2022 at 9:11 PM Thomas Munro <thomas.munro@gmail.com> wrote:

Hmm, but that's not what we're doing in general. For example, on
Windows we're redirecting open() to a replacement function of our own,
we're not using "pg_open()" in our code. That's not an example based
on AC_REPLACE_FUNCS, but there are plenty of those too. Isn't this
quite well established?

Yes. I just don't care for it.

Sounds like I'm in the minority, though.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jul 11, 2022 at 9:11 PM Thomas Munro <thomas.munro@gmail.com> wrote:

Hmm, but that's not what we're doing in general. For example, on
Windows we're redirecting open() to a replacement function of our own,
we're not using "pg_open()" in our code. That's not an example based
on AC_REPLACE_FUNCS, but there are plenty of those too. Isn't this
quite well established?

Yes. I just don't care for it.
Sounds like I'm in the minority, though.

I concur with your point that it's not great to use the standard name
for a function that doesn't have exactly the standard semantics.
But if it does, using a nonstandard name is not better. It's just one
more thing that readers of our code have to learn about.

Note that "exactly" only needs to mean "satisfies all the promises
made by POSIX". If some caller is depending on behavior details not
specified in the standard, that's the caller's bug not the wrapper
function's. Otherwise, yeah, we couldn't ever be sure whether a
wrapper function is close enough.

regards, tom lane

Hi,

On 2022-07-12 08:01:40 -0400, Robert Haas wrote:

On Mon, Jul 11, 2022 at 9:11 PM Thomas Munro <thomas.munro@gmail.com> wrote:

Hmm, but that's not what we're doing in general. For example, on
Windows we're redirecting open() to a replacement function of our own,
we're not using "pg_open()" in our code. That's not an example based
on AC_REPLACE_FUNCS, but there are plenty of those too. Isn't this
quite well established?

Yes. I just don't care for it.

Sounds like I'm in the minority, though.

I agree with you, at least largely.

Redefining functions, be it by linking in something or by redefining function
names via macros, is a mess. There's places where we then have to undefine
some of these things to be able to include external headers etc. Some
functions are only replaced in backends, others in frontend too. It makes it
hard to know what exactly the assumed set of platform primitives is. Etc.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

Redefining functions, be it by linking in something or by redefining function
names via macros, is a mess. There's places where we then have to undefine
some of these things to be able to include external headers etc. Some
functions are only replaced in backends, others in frontend too. It makes it
hard to know what exactly the assumed set of platform primitives is. Etc.

In the cases at hand, we aren't doing that, are we? The replacement
function is only used on platforms that lack the relevant POSIX function,
so it's hard to argue that we're replacing anything.

regards, tom lane

I have committed the first few:

* "Remove dead getrusage replacement code."
* "Remove dead handling for pre-POSIX sigwait()."
* "Remove dead getpwuid_r replacement code."

Here are some more, a couple of which I posted before but I've now
gone a bit further with them in terms of removing configure checks
etc:

* "Remove dlopen configure probe."
* "Remove configure probe and extra tests for getrlimit."
* "Remove configure probe for shm_open."
* "Remove configure probe for setsid."
* "Remove configure probes for readlink, and dead code and docs."
* "Remove configure probe for symlink, and dead code."
* "Remove configure probe for link."
* "Remove dead replacement code for clock_gettime()."
* "Remove configure probes for poll and poll.h."
* "Remove dead setenv, unsetenv replacement code."
* "Remove dead pread and pwrite replacement code."
* "Simplify replacement code for preadv and pwritev."
* "Remove --disable-thread-safety."

Some of these depend on SUSv2 options (not just "base"), but we
already do that (fsync, ...) and they're all features that are by now
ubiquitous, which means the fallback code is untested and the probes
are pointless.

<archeology-mode>I'd guess the last system we ran on that didn't have
symlinks would have been SVr3-based SCO, HP-UX, DG/UX etc from the
1980s, since they were invented in 4.2BSD in 1983 and adopted by SVr4
in 1988. The RLIMIT_OFILE stuff seems to be referring to 1BSD or 2BSD
on a PDP, whereas RLIMIT_NOFILE was already used in 4.3BSD in 1986,
which I'd have guessed would be the oldest OS POSTGRES ever actually
ran on, so that must have been cargo culting from something older?</>

The clock_gettime() one only becomes committable once prairiedog's
host switched to NetBSD, so that'll be committed at the same time as
the fdatasync one from a nearby thread.

The setenv/unsetenv one levels up to SUSv3 (= POSIX issue 6, 2001).
That'd be the first time we don't point at SUSv2 (= POSIX issue 5,
199x) to justify a change like this.

I expect there to be further clean-up after the removal of
--disable-thread-safety.

Thomas Munro <thomas.munro@gmail.com> writes:

Some of these depend on SUSv2 options (not just "base"), but we
already do that (fsync, ...) and they're all features that are by now
ubiquitous, which means the fallback code is untested and the probes
are pointless.

Reading this, it occurred to me that it'd be interesting to scrape
all of the latest configure results from the buildfarm, and see which
tests actually produce more than one answer among the set of tested
platforms. Those that don't could be targets for further simplification,
or else an indicator that we'd better go find some more animals.

Before I go off and do that, though, I wonder if you already did.

regards, tom lane

On Sun, Jul 24, 2022 at 11:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@gmail.com> writes:

Some of these depend on SUSv2 options (not just "base"), but we
already do that (fsync, ...) and they're all features that are by now
ubiquitous, which means the fallback code is untested and the probes
are pointless.

Reading this, it occurred to me that it'd be interesting to scrape
all of the latest configure results from the buildfarm, and see which
tests actually produce more than one answer among the set of tested
platforms. Those that don't could be targets for further simplification,
or else an indicator that we'd better go find some more animals.

Before I go off and do that, though, I wonder if you already did.

Yeah, here are the macros I scraped yesterday, considering the latest
results from machines that did something in the past week.

Thomas Munro <thomas.munro@gmail.com> writes:

Here are some more, a couple of which I posted before but I've now
gone a bit further with them in terms of removing configure checks
etc:

After looking through these briefly, I'm pretty concerned about
whether this won't break our Cygwin build in significant ways.
For example, lorikeet reports "HAVE_SETSID 1", a condition that
you want to replace with !WIN32. The question here is whether
or not WIN32 is defined in a Cygwin build. I see some places
in our code that believe it is not, but others that believe that
it is --- and the former ones are mostly like
#if defined(__CYGWIN__) || defined(WIN32)
which means they wouldn't actually fail if they are wrong about that.

More generally, I'm not exactly convinced that changes like
this are a readability improvement:

-#ifdef HAVE_SETSID
+#ifndef WIN32

I'd rather not have the code cluttered with a sea of
indistinguishable "#ifndef WIN32" tests when some of them could be
more specific and more mnemonic. So I think we'd be better off
leaving that as-is. I don't mind nuking the configure-time test
and hard-wiring "#define HAVE_SETSID 1" somewhere, but changing
the code's #if tests doesn't seem to bring any advantage.

Specific to 0001, I don't especially like what you did to
src/port/dlopen.c. The original intent (and reality until
not so long ago) was that that would be a container for
various dlopen replacements. Well, okay, maybe there will
never be any more besides Windows, but I think then we should
either rename the file to (say) win32dlopen.c or move it to
src/backend/port/win32. Likewise for link.c in 0007 and
pread.c et al in 0011. (But 0010 is fine, because the
replacement code is already handled that way.)

OTOH, 0012 seems to immediately change pread.c et al back
to NOT being Windows-only, though it's hard to tell for
sure because the configure support seems all wrong.
I'm quite confused by those two patches ... are they really
correct?

regards, tom lane

On Sun, Jul 24, 2022 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

After looking through these briefly, I'm pretty concerned about
whether this won't break our Cygwin build in significant ways.
For example, lorikeet reports "HAVE_SETSID 1", a condition that
you want to replace with !WIN32. The question here is whether
or not WIN32 is defined in a Cygwin build. I see some places
in our code that believe it is not, but others that believe that
it is --- and the former ones are mostly like
#if defined(__CYGWIN__) || defined(WIN32)
which means they wouldn't actually fail if they are wrong about that.

I spent a large chunk of today figuring out how to build PostgreSQL
under Cygwin/GCC on CI. My method for answering this question was to
put the following on the end of 192 .c files that contain the pattern
/#if.*WIN32/:

+
+#if defined(WIN32) && defined(__CYGWIN__)
+#pragma message "contradiction"
+#endif

Only one of them printed that message: dirmod.c. The reason is that
it goes out of its way to include Windows headers:

#if defined(WIN32) || defined(__CYGWIN__)
#ifndef __CYGWIN__
#include <winioctl.h>
#else
#include <windows.h>
#include <w32api/winioctl.h>
#endif
#endif

The chain <windows.h> -> <windef.h> -> <minwindef.h> leads to WIN32 here:

https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/include/minwindef.h#L15

I'm left wondering if we should de-confuse matters by ripping out all
the checks and comments that assume that this problem is more
widespread, and then stick a big notice about it in dirmod.c, to
contain this Jekyll/Hide situation safely inside about 8 feet of
concrete.

I'll respond to your other complaints with new patches tomorrow.

On 2022-07-25 Mo 10:35, Thomas Munro wrote:

On Sun, Jul 24, 2022 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

After looking through these briefly, I'm pretty concerned about
whether this won't break our Cygwin build in significant ways.
For example, lorikeet reports "HAVE_SETSID 1", a condition that
you want to replace with !WIN32. The question here is whether
or not WIN32 is defined in a Cygwin build. I see some places
in our code that believe it is not, but others that believe that
it is --- and the former ones are mostly like
#if defined(__CYGWIN__) || defined(WIN32)
which means they wouldn't actually fail if they are wrong about that.

I spent a large chunk of today figuring out how to build PostgreSQL
under Cygwin/GCC on CI. My method for answering this question was to
put the following on the end of 192 .c files that contain the pattern
/#if.*WIN32/:

+
+#if defined(WIN32) && defined(__CYGWIN__)
+#pragma message "contradiction"
+#endif

Only one of them printed that message: dirmod.c. The reason is that
it goes out of its way to include Windows headers:

#if defined(WIN32) || defined(__CYGWIN__)
#ifndef __CYGWIN__
#include <winioctl.h>
#else
#include <windows.h>
#include <w32api/winioctl.h>
#endif
#endif

The chain <windows.h> -> <windef.h> -> <minwindef.h> leads to WIN32 here:

https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/include/minwindef.h#L15

I'm left wondering if we should de-confuse matters by ripping out all
the checks and comments that assume that this problem is more
widespread, and then stick a big notice about it in dirmod.c, to
contain this Jekyll/Hide situation safely inside about 8 feet of
concrete.

Clearly it's something we've been aware of before, port.h has:

* Note: Some CYGWIN includes might #define WIN32.
 */
#if defined(WIN32) && !defined(__CYGWIN__)
#include "port/win32_port.h"
#endif

I can test any patches you want on lorikeet.

cheers

andrew

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

On Sun, Jul 24, 2022 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@gmail.com> writes:

Here are some more, a couple of which I posted before but I've now
gone a bit further with them in terms of removing configure checks
etc:

After looking through these briefly, I'm pretty concerned about
whether this won't break our Cygwin build in significant ways.
For example, lorikeet reports "HAVE_SETSID 1", a condition that
you want to replace with !WIN32. The question here is whether
or not WIN32 is defined in a Cygwin build. ...

No, it should not be unless someone screws up and leaks <windows.h>
into a header when WIN32 isn't already defined. I've done some
analysis and testing of that, and proposed to nail it down a bit and
remove the confusion created by the inconsistent macro tests, over at
[1]: /messages/by-id/CA+hUKG+e13wK0PBX5Z63CCwWm7MfRQuwBRabM_3aKWSko2AUww@mail.gmail.com

More generally, I'm not exactly convinced that changes like
this are a readability improvement:

-#ifdef HAVE_SETSID
+#ifndef WIN32

I'd rather not have the code cluttered with a sea of
indistinguishable "#ifndef WIN32" tests when some of them could be
more specific and more mnemonic. So I think we'd be better off
leaving that as-is. I don't mind nuking the configure-time test
and hard-wiring "#define HAVE_SETSID 1" somewhere, but changing
the code's #if tests doesn't seem to bring any advantage.

OK, in this version of the patch series I did this:

1. If it's something that only Unix has, and for Windows we do
nothing or skip a feature, then I've now hard-wired the macro as you
suggested. I put that in port.h. I agree that's a little easier to
grok than no-context !defined(WIN32). Examples: HAVE_SETSID,
HAVE_SHM_OPEN.

2. If it's something that Unix has and we supply a Windows
replacements, and we just can't cope without that function, then I
didn't bother with a vestigial macro. There generally weren't tests
for such things already (mostly stuff auto-generated by
AC_REPLACE_FUNCS). Example: HAVE_LINK.

3. In the special case of symlink() and readlink(), I defined the
macros on Unix even though we also have replacements on Windows.
(Previously we effectively did that for one but not the other...) My
idea here is that, wherever we are OK using our pseudo-symlinks made
from junction points (ie for tablespaces), then we should just go
ahead and use them without testing. But in just a couple of places
where fully compliant symlinks are clearly expected (ie non-directory
or relative path, eg tz file or executable symlinks), then the tests
can still be used. See also commit message. Does this make sense?

(I also propose to supply S_ISLNK and lstat() for Windows and make
usage of that stuff unconditional, but I put that in another
thread[2]/messages/by-id/CA+hUKGLfOOeyZpm5ByVcAt7x5Pn-=xGRNCvgiUPVVzjFLtnY0w@mail.gmail.com, as that's new code, and this thread is just about ripping
old dead stuff out.)

4. If it's something that already had very obvious Windows and Unix
code paths, then I didn't bother with a HAVE_XXX macro, because I
think it'd be more confusing than just #ifdef WIN32 ...windows stuff
... #else ...unix stuff... #endif. Example: HAVE_CLOCK_GETTIME.

Specific to 0001, I don't especially like what you did to
src/port/dlopen.c. The original intent (and reality until
not so long ago) was that that would be a container for
various dlopen replacements. Well, okay, maybe there will
never be any more besides Windows, but I think then we should
either rename the file to (say) win32dlopen.c or move it to
src/backend/port/win32. Likewise for link.c in 0007 and
pread.c et al in 0011. (But 0010 is fine, because the
replacement code is already handled that way.)

Agreed on the file names win32dlopen.c, win32link.c, win32pread.c,
win32pwrite.c, and done.

Another characteristic of other Windows-only replacement code is that
it's called pgwin32_THING and then a macro replaces THING() with
pgwin32_THING(). I guess I should do that too, for consistency, and
move relevant declarations into win32_port.h? Done.

There are clearly many other candidates for X.c -> win32X.c renamings
by the same only-for-Windows argument, but I haven't touched those (at
least dirent.c, dirmod.c, gettimeofday.c, kill.c, open.c, system.c).

I'll also include the fdatasync configure change here (moved from
another thread). Now it also renames fdatasync.c -> win32datasync.c.
Erm, but I didn't add the pgwin32_ prefix to the function name,
because it shares a function declaration with macOS in c.h.

OTOH, 0012 seems to immediately change pread.c et al back
to NOT being Windows-only, though it's hard to tell for
sure because the configure support seems all wrong.
I'm quite confused by those two patches ... are they really
correct?

The 0012 patch (now 0011 in v2) is about the variants with -v on the
end. The patches are as I intended. I've now put a longer
explanation into the commit message, but here's a short recap:

pread()/pwrite() replacements (without 'v' for vector) are now needed
only by Windows. HP-UX < 11 was the last Unix system I knew of
without these functions. That makes sense, as I think they were
related to the final POSIX threading push (multi-threaded programs
want to be able to skip file position side-effects), which HP-UX 10.x
predated slightly. Gaur's retirement unblocked this cleanup.

preadv()/pwritev() replacements (with 'v' for vector) are needed by
Solaris, macOS < 11 and Windows, and will likely be required for a
long time, because these functions still haven't been standardised.
My goal is to make our replacement code side-effect free, thread-safe,
in line with the de facto standard/convention seen on Linux, *BSD,
macOS, AIX, illumos.

Note that I have some better vector I/O code for Windows to propose a
bit later, so the real effect of this choice will be to drop true
vector I/O on Solaris, until such time as they get around to providing
the modern interface that almost every other Unix managed to agree on.

[1]: /messages/by-id/CA+hUKG+e13wK0PBX5Z63CCwWm7MfRQuwBRabM_3aKWSko2AUww@mail.gmail.com
[2]: /messages/by-id/CA+hUKGLfOOeyZpm5ByVcAt7x5Pn-=xGRNCvgiUPVVzjFLtnY0w@mail.gmail.com

Sorry, I know this is really tedious stuff, but I found a couple more
cleanup opportunities nearby:

For dlopen(), we don't have to worry about old Unix systems without
RTLD_NOW and RTLD_GLOBAL macros anymore. They're in SUSv2 and present
on all our BF Unixes, so that's some more configure probes that we can
remove. Also, we might as well move the declarations for everything
relating to dlopen into win32_port.h.

(Why some WIN32 things are done in port.h while others are done in
win32_port.h is something I don't grok; probably depends whether there
were ever non-Windows systems that needed something... I might propose
to tidy that up some more, later...)

For setenv()/unsetenv(), I removed the declarations from port.h. Only
the ones in win32_port.h are needed now.

I fixed a couple of places where I'd renamed a file but forgotten to
update that IDENTIFICATION section from the CVS days, and a stray 2021
copyright year.

It'd be good to find a new home for pg_get_user_name() and
pg_get_user_home_dir(), which really shouldn't be left in the now
bogusly named src/port/thread.c. Any suggestions?

Was it a mistake to add pgwin32_ prefixes to some Windows replacement
functions? It seems a little arbitrary that we do that sometimes even
though we don't need to. Perhaps we should only do that kind of thing
when we want to avoid name-clash with a real Windows function that
we're replacing?

I'd like to push these soon, if there are no further objections. If
you prefer, I could hold back on the two that will break prairiedog
until you give the word, namely clock_gettime() and fdatasync().