Thread: Inconsistencies around defining FRONTEND
Hi, This started at https://postgr.es/m/20220817215317.poeofidf7o7dy6hy%40awork3.anarazel.de Peter made a good point about -DFRONTED not being defined symmetrically between meson and autoconf builds, which made me look at where we define it. And I think we ought to clean this up independ of the meson patch. On 2022-08-17 14:53:17 -0700, Andres Freund wrote: > On 2022-08-17 15:50:23 +0200, Peter Eisentraut wrote: > > - -DFRONTEND is used somewhat differently from the makefiles. For > > example, meson sets -DFRONTEND for pg_controldata, but the > > makefiles don't. Conversely, the makefiles set -DFRONTEND for > > ecpglib, but meson does not. This should be checked again to make > > sure it all matches up. > > Yes, should sync that up. > > FWIW, meson does add -DFRONTEND for ecpglib. There were a few places that did > add it twice, I'll push a cleanup of that in a bit. Yikes, the situation in HEAD is quite the mess. Several .c files set FRONTEND themselves, so they can include postgres.h, instead of postgres_fe.h: $ git grep '#define.*FRONTEND' upstream/master ':^src/include/postgres_fe.h' upstream/master:src/bin/pg_controldata/pg_controldata.c:#define FRONTEND 1 upstream/master:src/bin/pg_resetwal/pg_resetwal.c:#define FRONTEND 1 upstream/master:src/bin/pg_waldump/compat.c:#define FRONTEND 1 upstream/master:src/bin/pg_waldump/pg_waldump.c:#define FRONTEND 1 upstream/master:src/bin/pg_waldump/rmgrdesc.c:#define FRONTEND 1 Yet, most of them also define FRONTEND in both make and msvc buildsystem. $ git grep -E "(D|AddDefine\(')FRONTEND" upstream/master src/bin/ src/tools/msvc/ upstream/master:src/bin/initdb/Makefile:override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) -I$(top_srcdir)/src/timezone $(CPPFLAGS) upstream/master:src/bin/pg_rewind/Makefile:override CPPFLAGS := -I$(libpq_srcdir) -DFRONTEND $(CPPFLAGS) upstream/master:src/bin/pg_waldump/Makefile:override CPPFLAGS := -DFRONTEND $(CPPFLAGS) upstream/master:src/tools/msvc/Mkvcbuild.pm: $libpgport->AddDefine('FRONTEND'); upstream/master:src/tools/msvc/Mkvcbuild.pm: $libpgcommon->AddDefine('FRONTEND'); upstream/master:src/tools/msvc/Mkvcbuild.pm: $libpgfeutils->AddDefine('FRONTEND'); upstream/master:src/tools/msvc/Mkvcbuild.pm: $libpq->AddDefine('FRONTEND'); upstream/master:src/tools/msvc/Mkvcbuild.pm: $pgtypes->AddDefine('FRONTEND'); upstream/master:src/tools/msvc/Mkvcbuild.pm: $libecpg->AddDefine('FRONTEND'); upstream/master:src/tools/msvc/Mkvcbuild.pm: $libecpgcompat->AddDefine('FRONTEND'); upstream/master:src/tools/msvc/Mkvcbuild.pm: $pgrewind->AddDefine('FRONTEND'); upstream/master:src/tools/msvc/Mkvcbuild.pm: $pg_waldump->AddDefine('FRONTEND') That's largely because they also build files from src/backend, which obviously contain no #define FRONTEND. The -DFRONTENDs for the various ecpg libraries don't seem necessary anymore. That looks to be a leftover from 7143b3e8213, before that ecpg had copies of various pgport libraries. Same with libpq, also looks to be obsoleted by 7143b3e8213. I don't think we need FRONTEND in initdb - looks like that stopped being required with af1a949109d. Unfortunately, the remaining uses of FRONTEND are required. That's: - pg_controldata, via #define - pg_resetwal, via #define - pg_rewind, via -DFRONTEND, due to xlogreader.c - pg_waldump, via #define and -DFRONTEND, due to xlogreader.c, xlogstats.c, rmgrdesc/*desc.c I'm kind of wondering if we should add xlogreader.c, xlogstat.c, *desc to fe_utils, instead of building them in various places. That'd leave us only with #define FRONTENDs for cases that do need to include postgres.h themselves, which seems a lot more palatable. Greetings, Andres Freund
Attachment
Hi, On 2022-08-20 12:45:50 -0700, Andres Freund wrote: > The -DFRONTENDs for the various ecpg libraries don't seem necessary > anymore. That looks to be a leftover from 7143b3e8213, before that ecpg had > copies of various pgport libraries. > > Same with libpq, also looks to be obsoleted by 7143b3e8213. > > I don't think we need FRONTEND in initdb - looks like that stopped being > required with af1a949109d. I think the patches for this are fairly obvious, and survived CI without an issue [1], so the src/tools/msvc bits work too. So I'm planning to push them fairly soon. But the remaining "issues" don't have an obvious solutions and not addressed by these patches: > Unfortunately, the remaining uses of FRONTEND are required. That's: > - pg_controldata, via #define > - pg_resetwal, via #define > - pg_rewind, via -DFRONTEND, due to xlogreader.c > - pg_waldump, via #define and -DFRONTEND, due to xlogreader.c, xlogstats.c, rmgrdesc/*desc.c > > I'm kind of wondering if we should add xlogreader.c, xlogstat.c, *desc to > fe_utils, instead of building them in various places. That'd leave us only > with #define FRONTENDs for cases that do need to include postgres.h > themselves, which seems a lot more palatable. Greetings, Andres Freund [1] https://cirrus-ci.com/build/4648937721167872
Hi, On 2022-08-22 08:48:34 -0700, Andres Freund wrote: > On 2022-08-20 12:45:50 -0700, Andres Freund wrote: > > The -DFRONTENDs for the various ecpg libraries don't seem necessary > > anymore. That looks to be a leftover from 7143b3e8213, before that ecpg had > > copies of various pgport libraries. > > > > Same with libpq, also looks to be obsoleted by 7143b3e8213. > > > > I don't think we need FRONTEND in initdb - looks like that stopped being > > required with af1a949109d. > > I think the patches for this are fairly obvious, and survived CI without an > issue [1], so the src/tools/msvc bits work too. So I'm planning to push them > fairly soon. Done. - Andres
On Sat, Aug 20, 2022 at 3:46 PM Andres Freund <andres@anarazel.de> wrote: > Unfortunately, the remaining uses of FRONTEND are required. That's: > - pg_controldata, via #define > - pg_resetwal, via #define > - pg_rewind, via -DFRONTEND, due to xlogreader.c > - pg_waldump, via #define and -DFRONTEND, due to xlogreader.c, xlogstats.c, rmgrdesc/*desc.c Actually, I think we could fix these pretty easily too. See attached. This has been annoying me for a while. I hope we can agree on a way to clean it up. -- Robert Haas EDB: http://www.enterprisedb.com
Attachment
Robert Haas <robertmhaas@gmail.com> writes:
> Actually, I think we could fix these pretty easily too. See attached.
Hmm, do these headers still pass headerscheck/cpluspluscheck?
I might quibble a bit with the exact placement of the #ifndef FRONTEND
tests, but overall this looks pretty plausible.
regards, tom lane
On Tue, Aug 23, 2022 at 5:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > Actually, I think we could fix these pretty easily too. See attached. > > Hmm, do these headers still pass headerscheck/cpluspluscheck? I didn't check before sending the patch, but now I ran it locally, and I did get failures from both, but they all seem to be unrelated. Mainly, it's sad that I don't have Python.h, but I didn't configure with python, so whatever. > I might quibble a bit with the exact placement of the #ifndef FRONTEND > tests, but overall this looks pretty plausible. Yep, that's arguable. In particular, should the redo functions also be protected by #ifdef FRONTEND? I'd be more than thrilled if you wanted to adjust this to taste and apply it, barring objections from others of course. -- Robert Haas EDB: http://www.enterprisedb.com
Hi, On 2022-08-23 17:24:30 -0400, Robert Haas wrote: > On Sat, Aug 20, 2022 at 3:46 PM Andres Freund <andres@anarazel.de> wrote: > > Unfortunately, the remaining uses of FRONTEND are required. That's: > > - pg_controldata, via #define > > - pg_resetwal, via #define > > - pg_rewind, via -DFRONTEND, due to xlogreader.c > > - pg_waldump, via #define and -DFRONTEND, due to xlogreader.c, xlogstats.c, rmgrdesc/*desc.c > > Actually, I think we could fix these pretty easily too. I just meant that they're not already obsolete ;) > See attached. Just to make sure I understand - you're just trying to get rid of the #define frontends, not the -DFRONTENDs passed in from the Makefile? Because afaics we still need those, correct? Greetings, Andres Freund
On Tue, Aug 23, 2022 at 7:24 PM Andres Freund <andres@anarazel.de> wrote: > Just to make sure I understand - you're just trying to get rid of the #define > frontends, not the -DFRONTENDs passed in from the Makefile? Because afaics we > still need those, correct? Oh, yeah, this only fixes the #define ones. But maybe fixing the other ones with a similar approach would be possible? I really don't see why we should tolerate having #define FRONTEND in more than once place. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 23, 2022 at 7:24 PM Andres Freund <andres@anarazel.de> wrote:
>> Just to make sure I understand - you're just trying to get rid of the #define
>> frontends, not the -DFRONTENDs passed in from the Makefile? Because afaics we
>> still need those, correct?
> Oh, yeah, this only fixes the #define ones. But maybe fixing the other
> ones with a similar approach would be possible?
> I really don't see why we should tolerate having #define FRONTEND in
> more than once place.
src/port and src/common really need to do it like that (ie pass in
the -D switch) so that the identical source file can be built
both ways. Maybe we could get rid of -DFRONTEND in other places,
like pg_rewind and pg_waldump.
regards, tom lane
Hi, On 2022-08-23 19:50:00 -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Tue, Aug 23, 2022 at 7:24 PM Andres Freund <andres@anarazel.de> wrote: > >> Just to make sure I understand - you're just trying to get rid of the #define > >> frontends, not the -DFRONTENDs passed in from the Makefile? Because afaics we > >> still need those, correct? > > > Oh, yeah, this only fixes the #define ones. But maybe fixing the other > > ones with a similar approach would be possible? > > > I really don't see why we should tolerate having #define FRONTEND in > > more than once place. > > src/port and src/common really need to do it like that (ie pass in > the -D switch) so that the identical source file can be built > both ways. Maybe we could get rid of -DFRONTEND in other places, > like pg_rewind and pg_waldump. We could, if we make xlogreader.c and the rmgrdesc routines built as part of src/common. I don't really see how otherwise. Greetings, Andres Freund
On Tue, Aug 23, 2022 at 9:55 PM Andres Freund <andres@anarazel.de> wrote: > We could, if we make xlogreader.c and the rmgrdesc routines built as part of > src/common. I don't really see how otherwise. After a little bit of study, I agree. It looks to me like -DFRONTEND can be removed from src/fe_utils/Makefile and probably also src/common/unicode/Makefile without changing anything else, because the C files in those directories seem to be frontend-only and they already include "postgres_fe.h". I think we should go ahead and do that, and also apply the patch I posted yesterday with whatever bikeshedding seems appropriate. It doesn't really seem like we have a plausible alternative to the current system for src/common or src/port. pg_rewind and pg_waldump seem to need the xlogreader code moved to src/common, as Andres proposes. I'm not volunteering to tackle that right now but I think it might be a good thing to do sometime. -- Robert Haas EDB: http://www.enterprisedb.com
Hi, On 2022-08-24 10:40:01 -0400, Robert Haas wrote: > pg_rewind and pg_waldump seem to need the xlogreader code moved to > src/common, as Andres proposes. I'm not volunteering to tackle that > right now but I think it might be a good thing to do sometime. The easier way would be to just keep their current method of building, but do it as part of src/common. Greetings, Andres Freund