remove ancient pre-dlopen dynloader code

Hi,

On 2018-08-09 14:29:08 +0200, Peter Eisentraut wrote:

The non-dlopen dynloader code for several operating systems is in some
cases decades obsolete, and I have had some doubts that it would even
compile anymore. Attached are patches for each operating system
removing the obsolete code, with references to when it became obsolete.

Cool, I encountered those files a couple times when grepping for
things. +1 for the removal.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-08-09 14:29:08 +0200, Peter Eisentraut wrote:

The non-dlopen dynloader code for several operating systems is in some
cases decades obsolete, and I have had some doubts that it would even
compile anymore. Attached are patches for each operating system
removing the obsolete code, with references to when it became obsolete.

Cool, I encountered those files a couple times when grepping for
things. +1 for the removal.

LGTM, too.

regards, tom lane

On 2018-08-09 10:03:43 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-08-09 14:29:08 +0200, Peter Eisentraut wrote:

The non-dlopen dynloader code for several operating systems is in some
cases decades obsolete, and I have had some doubts that it would even
compile anymore. Attached are patches for each operating system
removing the obsolete code, with references to when it became obsolete.

Cool, I encountered those files a couple times when grepping for
things. +1 for the removal.

LGTM, too.

This now generates a super nitpicky warning on at at least some linux +
clang configurations. I use -Weverything plus a lot of -Wno-*, and this
change added:

dynloader.c:7:4: warning: ISO C requires a translation unit to contain at least one declaration [-Wempty-translation-unit]
*/
^
1 warning generated.

I'll probably just neuter the warning, but I wanted to nevertheless
raise the "issue".

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

This now generates a super nitpicky warning on at at least some linux +
clang configurations. I use -Weverything plus a lot of -Wno-*, and this
change added:
dynloader.c:7:4: warning: ISO C requires a translation unit to contain at least one declaration [-Wempty-translation-unit]

We've been seeing that (or equivalents) on other platforms for years,
if not decades. I can't get too excited about it really.

The lazy man's way to get rid of it would be to put something like
"int bogus = 0;" in the empty dynloader.c files. Better would be
to not have the empty .c files at all, but I'm not sure how much
we'd have to contort the Makefiles to support that.

regards, tom lane

On 2018-08-16 09:22:14 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

This now generates a super nitpicky warning on at at least some linux +
clang configurations. I use -Weverything plus a lot of -Wno-*, and this
change added:
dynloader.c:7:4: warning: ISO C requires a translation unit to contain at least one declaration [-Wempty-translation-unit]

We've been seeing that (or equivalents) on other platforms for years,
if not decades. I can't get too excited about it really.

Yea, me neither.

The lazy man's way to get rid of it would be to put something like
"int bogus = 0;" in the empty dynloader.c files. Better would be
to not have the empty .c files at all, but I'm not sure how much
we'd have to contort the Makefiles to support that.

If I had my druthers, we'd just remove all that configure magic for
selecting these files and just use ifdefs. Personally I find it
occasionally that they're linked into place, rather than built under
their original name.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-08-16 09:22:14 -0400, Tom Lane wrote:

The lazy man's way to get rid of it would be to put something like
"int bogus = 0;" in the empty dynloader.c files. Better would be
to not have the empty .c files at all, but I'm not sure how much
we'd have to contort the Makefiles to support that.

If I had my druthers, we'd just remove all that configure magic for
selecting these files and just use ifdefs. Personally I find it
occasionally that they're linked into place, rather than built under
their original name.

Even if we all agreed that was an improvement (which I'm not sure of),
it wouldn't fix this problem would it? On affected platforms, the
file would still be empty after preprocessing.

regards, tom lane

On 2018-08-16 10:07:04 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-08-16 09:22:14 -0400, Tom Lane wrote:

The lazy man's way to get rid of it would be to put something like
"int bogus = 0;" in the empty dynloader.c files. Better would be
to not have the empty .c files at all, but I'm not sure how much
we'd have to contort the Makefiles to support that.

If I had my druthers, we'd just remove all that configure magic for
selecting these files and just use ifdefs. Personally I find it
occasionally that they're linked into place, rather than built under
their original name.

Even if we all agreed that was an improvement (which I'm not sure of),
it wouldn't fix this problem would it? On affected platforms, the
file would still be empty after preprocessing.

Well, that depends on what you put into that file, it seems
realistically combinable with a bunch of non-conditional code...

Anyway, I'm not planning to do something here right now besides putting
-Wno-empty-translation-unit into my scripts, I just wanted to make sure
people are aware that we hit this now.

Greetings,

Andres Freund

On 16/08/2018 16:10, Andres Freund wrote:

If I had my druthers, we'd just remove all that configure magic for
selecting these files and just use ifdefs. Personally I find it
occasionally that they're linked into place, rather than built under
their original name.

Even if we all agreed that was an improvement (which I'm not sure of),
it wouldn't fix this problem would it? On affected platforms, the
file would still be empty after preprocessing.

Well, that depends on what you put into that file, it seems
realistically combinable with a bunch of non-conditional code...

How about this: We only have two nonstandard dlopen() implementations
left: Windows and (old) HP-UX. We move those into src/port/dlopen.c and
treat it like a regular libpgport member. That gets rid of all those
duplicative empty per-platform files.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

How about this: We only have two nonstandard dlopen() implementations
left: Windows and (old) HP-UX. We move those into src/port/dlopen.c and
treat it like a regular libpgport member. That gets rid of all those
duplicative empty per-platform files.

+1. I eyeballed the patch quickly and it looks sane, but I didn't
try to test it. Being a lazy person, I don't want to test it manually,
but I'll be happy to queue up a gaur run as soon as you push it
(and if by some chance that shows a problem, I'll fix it).

regards, tom lane

On 2018-08-31 10:52:18 +0200, Peter Eisentraut wrote:

How about this: We only have two nonstandard dlopen() implementations
left: Windows and (old) HP-UX. We move those into src/port/dlopen.c and
treat it like a regular libpgport member. That gets rid of all those
duplicative empty per-platform files.

Great! Quickly skimmed the patch and it looks good. I don't quite know
why you moved the implementation to src/port rather than
src/backend/port, but either is fine with me... Long term the former
probably is better.

Greetings,

Andres Freund

On 31/08/2018 10:52, Peter Eisentraut wrote:

On 16/08/2018 16:10, Andres Freund wrote:

If I had my druthers, we'd just remove all that configure magic for
selecting these files and just use ifdefs. Personally I find it
occasionally that they're linked into place, rather than built under
their original name.

Even if we all agreed that was an improvement (which I'm not sure of),
it wouldn't fix this problem would it? On affected platforms, the
file would still be empty after preprocessing.

Well, that depends on what you put into that file, it seems
realistically combinable with a bunch of non-conditional code...

How about this: We only have two nonstandard dlopen() implementations
left: Windows and (old) HP-UX. We move those into src/port/dlopen.c and
treat it like a regular libpgport member. That gets rid of all those
duplicative empty per-platform files.

Updated patch. It needed some adjustments for Windows, per Appveyor,

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 01/09/2018 06:51, Peter Eisentraut wrote:

How about this: We only have two nonstandard dlopen() implementations
left: Windows and (old) HP-UX. We move those into src/port/dlopen.c and
treat it like a regular libpgport member. That gets rid of all those
duplicative empty per-platform files.

Updated patch. It needed some adjustments for Windows, per Appveyor,

I'm going to use this thread for a moment to work out some details with
the cfbot.

The v2 patch I sent previously was created using git format-patch with
default settings. This detected a rename:

rename src/{backend/port/dynloader/win32.c => port/dlopen.c} (51%)

which is fair enough. However, whatever method the cfbot uses to apply
patches fails to handle that. The tree that is sent for testing by
Appveyor still contains a full src/backend/port/dynloader/win32.c and no
src/port/dlopen.c, which expectedly makes the build fail. (It also
still contains src/backend/port/dynloader/otherplatform.c, but empty, so
the patching doesn't remove empty files, which is another but minor
problem.)

The v3 patch attached here was made with git format-patch --no-renames.
Let's see how that works out.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 06/09/2018 10:16, Peter Eisentraut wrote:

The v3 patch attached here was made with git format-patch --no-renames.
Let's see how that works out.

That worked, and the patch has been committed.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 06/09/2018 10:16, Peter Eisentraut wrote:

The v3 patch attached here was made with git format-patch --no-renames.
Let's see how that works out.

That worked, and the patch has been committed.

Sure enough, gaur's not happy. I'll take a look in a bit.

regards, tom lane

On Thu, Sep 6, 2018 at 1:16 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

I'm going to use this thread for a moment to work out some details with
the cfbot.

The v2 patch I sent previously was created using git format-patch with
default settings. This detected a rename:

rename src/{backend/port/dynloader/win32.c => port/dlopen.c} (51%)

which is fair enough. However, whatever method the cfbot uses to apply
patches fails to handle that.

Interesting. Its version of "patch" doesn't understand that. I am
not sure if other versions of patch do. Currently cfbot doesn't use
git am because not everyone is posting patches created with
format-patch, and I thought good old patch could handle basically
anything. I wasn't aware of this quirk. I'll see if there is some
way I can convince patch to respect renames, or I should try to apply
with git first and then fall back to patch only if that fails.

--
Thomas Munro
http://www.enterprisedb.com