Horton

Thread

Commits

  1. Don't define FRONTEND for ecpg libraries

  2. Don't define FRONTEND for initdb

  3. Don't define FRONTEND for libpq

  1. Inconsistencies around defining FRONTEND

    Andres Freund <andres@anarazel.de> — 2022-08-20T19:45:50Z

    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

  2. Re: Inconsistencies around defining FRONTEND

    Andres Freund <andres@anarazel.de> — 2022-08-22T15:48:34Z

    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

  3. Re: Inconsistencies around defining FRONTEND

    Andres Freund <andres@anarazel.de> — 2022-08-23T03:42:33Z

    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

  4. Re: Inconsistencies around defining FRONTEND

    Robert Haas <robertmhaas@gmail.com> — 2022-08-23T21:24:30Z

    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

  5. Re: Inconsistencies around defining FRONTEND

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-23T21:56:23Z

    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

  6. Re: Inconsistencies around defining FRONTEND

    Robert Haas <robertmhaas@gmail.com> — 2022-08-23T22:58:50Z

    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

  7. Re: Inconsistencies around defining FRONTEND

    Andres Freund <andres@anarazel.de> — 2022-08-23T23:24:45Z

    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

  8. Re: Inconsistencies around defining FRONTEND

    Robert Haas <robertmhaas@gmail.com> — 2022-08-23T23:37:00Z

    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

  9. Re: Inconsistencies around defining FRONTEND

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-23T23:50:00Z

    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

  10. Re: Inconsistencies around defining FRONTEND

    Andres Freund <andres@anarazel.de> — 2022-08-24T01:55:44Z

    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

  11. Re: Inconsistencies around defining FRONTEND

    Robert Haas <robertmhaas@gmail.com> — 2022-08-24T14:40:01Z

    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

  12. Re: Inconsistencies around defining FRONTEND

    Andres Freund <andres@anarazel.de> — 2022-08-24T15:10:07Z

    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