Обсуждение: remove ancient pre-dlopen dynloader code
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. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
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