Horton

Thread

Commits

  1. Move logging.h and logging.c from src/fe_utils/ to src/common/.

  2. Remove pg_rewind's private logging.h/logging.c files.

  3. Unified logging system for command-line programs

  1. PG12, PGXS and linking pgfeutils

    Ian Barwick <ian.barwick@2ndquadrant.com> — 2019-05-07T05:24:19Z

    Hi

Commit cc8d4151 [*] introduced a dependency between some functions in
libpgcommon and libpgfeutils, which is not reflected in the linker options
provided when building an external program using PGXS, e.g. attempting to
build the attached (trivial) example results in:

     $ PATH=$PG_HEAD:$PATH USE_PGXS=1 make
     gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3 -fno-omit-frame-pointer -I. -I./ -I/home/ibarwick/devel/builds/HEAD/include/postgresql/server -I/home/ibarwick/devel/builds/HEAD/include/postgresql/internal  -D_GNU_SOURCE   -c -o pgxs-test.o pgxs-test.c
     gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3 -fno-omit-frame-pointer pgxs-test.o  -L/home/ibarwick/devel/builds/HEAD/lib   -Wl,--as-needed -Wl,-rpath,'/home/ibarwick/devel/builds/HEAD/lib',--enable-new-dtags  -L/home/ibarwick/devel/builds/HEAD/lib -lpgcommon -lpgport -L/home/ibarwick/devel/builds/HEAD/lib -lpq -lpgcommon -lpgport -lpthread -lssl -lcrypto -lgssapi_krb5 -lz -lreadline -lrt -lcrypt -ldl -lm  -o pgxs-test
     /home/ibarwick/devel/builds/HEAD/lib/libpgcommon.a(pgfnames.o): In function `pgfnames':
     /home/ibarwick/devel/postgresql/src/common/pgfnames.c:48: undefined reference to `__pg_log_level'
     /home/ibarwick/devel/postgresql/src/common/pgfnames.c:48: undefined reference to `pg_log_generic'
     /home/ibarwick/devel/postgresql/src/common/pgfnames.c:69: undefined reference to `__pg_log_level'
     /home/ibarwick/devel/postgresql/src/common/pgfnames.c:69: undefined reference to `pg_log_generic'
     /home/ibarwick/devel/postgresql/src/common/pgfnames.c:74: undefined reference to `__pg_log_level'
     /home/ibarwick/devel/postgresql/src/common/pgfnames.c:74: undefined reference to `pg_log_generic'
     collect2: error: ld returned 1 exit status
     make: *** [pgxs-test] Error 1

which is a regression compared to PG11 and earlier.

Workaround/possible fix is to include "pgfeutils" in the "libpq_pgport" definition, i.e.:

     *** a/src/Makefile.global.in
     --- b/src/Makefile.global.in
     *************** libpq = -L$(libpq_builddir) -lpq
     *** 561,567 ****
       # on client link lines, since that also appears in $(LIBS).
       # libpq_pgport_shlib is the same idea, but for use in client shared libraries.
       ifdef PGXS
     ! libpq_pgport = -L$(libdir) -lpgcommon -lpgport $(libpq)
       libpq_pgport_shlib = -L$(libdir) -lpgcommon_shlib -lpgport_shlib $(libpq)
       else
       libpq_pgport = -L$(top_builddir)/src/common -lpgcommon -L$(top_builddir)/src/port -lpgport $(libpq)
     --- 561,567 ----
       # on client link lines, since that also appears in $(LIBS).
       # libpq_pgport_shlib is the same idea, but for use in client shared libraries.
       ifdef PGXS
     ! libpq_pgport = -L$(libdir) -lpgcommon -lpgport -lpgfeutils $(libpq)
       libpq_pgport_shlib = -L$(libdir) -lpgcommon_shlib -lpgport_shlib $(libpq)
       else
       libpq_pgport = -L$(top_builddir)/src/common -lpgcommon -L$(top_builddir)/src/port -lpgport $(libpq)

I presume a similar modification may need to be added to the following lines in
that section but haven't had a chance to look in detail yet (and may be barking
up the wrong tree entirely of course).

[*] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=cc8d41511721d25d557fc02a46c053c0a602fed


Regards


Ian Barwick

-- 
  Ian Barwick                   https://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  2. Re: PG12, PGXS and linking pgfeutils

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-07T13:46:07Z

    Ian Barwick <ian.barwick@2ndquadrant.com> writes:
> Commit cc8d4151 [*] introduced a dependency between some functions in
> libpgcommon and libpgfeutils,

This seems rather seriously broken.  I do not think the answer is to
create a global dependency on libpgfeutils.

			regards, tom lane

  3. Re: PG12, PGXS and linking pgfeutils

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-05-07T19:29:52Z

    On Tue, May 07, 2019 at 09:46:07AM -0400, Tom Lane wrote:
>Ian Barwick <ian.barwick@2ndquadrant.com> writes:
>> Commit cc8d4151 [*] introduced a dependency between some functions in
>> libpgcommon and libpgfeutils,
>
>This seems rather seriously broken.  I do not think the answer is to
>create a global dependency on libpgfeutils.
>

Yeah. I've added it to the open items.

regards

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

  4. Re: PG12, PGXS and linking pgfeutils

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-09T17:20:53Z

    I wrote:
> Ian Barwick <ian.barwick@2ndquadrant.com> writes:
>> Commit cc8d4151 [*] introduced a dependency between some functions in
>> libpgcommon and libpgfeutils,

> This seems rather seriously broken.  I do not think the answer is to
> create a global dependency on libpgfeutils.

Or, to be clearer: fe_utils has had dependencies on libpgcommon since
its inception.  What we are seeing here is that libpgcommon has now
grown some dependencies on libpgfeutils.  That can't be allowed to
stand.  We'd be better off giving up on the separation between those
libraries than having circular dependencies between them.

I'm not especially on board with the idea of moving FE-specific error
handling code into libpgcommon, as that breaks the concept that
src/common/ is broadly for code that can work in either frontend or
backend contexts.  However, we already have a few violations of that
rule: common/Makefile already has

# A few files are currently only built for frontend, not server
OBJS_FRONTEND = $(OBJS_COMMON) fe_memutils.o file_utils.o restricted_token.o

So maybe the answer is to move these logging support functions into
src/common, in a file that's only built for frontend.

			regards, tom lane

  5. Re: PG12, PGXS and linking pgfeutils

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-09T17:39:11Z

    On 2019-May-09, Tom Lane wrote:

> I'm not especially on board with the idea of moving FE-specific error
> handling code into libpgcommon, as that breaks the concept that
> src/common/ is broadly for code that can work in either frontend or
> backend contexts.  However, we already have a few violations of that
> rule: common/Makefile already has
> 
> # A few files are currently only built for frontend, not server
> OBJS_FRONTEND = $(OBJS_COMMON) fe_memutils.o file_utils.o restricted_token.o
> 
> So maybe the answer is to move these logging support functions into
> src/common, in a file that's only built for frontend.

I wonder if a better solution isn't to move the file_utils stuff to
fe_utils.  Half of it is frontend-specific.  The only one that should be
shared to backend seems to be fsync_fname ... but instead of sharing it,
we have a second copy in fd.c.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: PG12, PGXS and linking pgfeutils

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-09T17:47:12Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-May-09, Tom Lane wrote:
>> I'm not especially on board with the idea of moving FE-specific error
>> handling code into libpgcommon, as that breaks the concept that
>> src/common/ is broadly for code that can work in either frontend or
>> backend contexts.  However, we already have a few violations of that
>> rule: common/Makefile already has
>> 
>> # A few files are currently only built for frontend, not server
>> OBJS_FRONTEND = $(OBJS_COMMON) fe_memutils.o file_utils.o restricted_token.o
>> 
>> So maybe the answer is to move these logging support functions into
>> src/common, in a file that's only built for frontend.

> I wonder if a better solution isn't to move the file_utils stuff to
> fe_utils.  Half of it is frontend-specific.  The only one that should be
> shared to backend seems to be fsync_fname ... but instead of sharing it,
> we have a second copy in fd.c.

Hm, if file_utils is the only thing in common/ that uses this, and we
expect that to remain true, that would fix the issue.  But ...

The thing I was looking at was mainly fe_memutils, which is justifiably
here on the grounds that it provides backend-like palloc support and
thereby eases the task of making other common/ modules work in both
contexts.  If we built elog/ereport emulations on top of Peter's logging
functions, there'd be a very clear case for having that in common/.
Peter didn't do that for v12, but I hope we get there at some point.

			regards, tom lane

  7. Re: PG12, PGXS and linking pgfeutils

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-13T19:33:17Z

    I wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> I wonder if a better solution isn't to move the file_utils stuff to
>> fe_utils.  Half of it is frontend-specific.  The only one that should be
>> shared to backend seems to be fsync_fname ... but instead of sharing it,
>> we have a second copy in fd.c.

> Hm, if file_utils is the only thing in common/ that uses this, and we
> expect that to remain true, that would fix the issue.  But ...

Thumbing through commit cc8d41511, I see that it already touched
five common/ modules

diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
diff --git a/src/common/pgfnames.c b/src/common/pgfnames.c
diff --git a/src/common/restricted_token.c b/src/common/restricted_token.c
diff --git a/src/common/rmtree.c b/src/common/rmtree.c

Several of those have substantial backend components, so moving them
to fe_utils is a nonstarter.  I think moving fe_utils/logging.[hc] to
common/ is definitely the way to get out of this problem.


I started working on a patch to do that, and soon noticed that there
are pre-existing files logging.[hc] in src/bin/pg_rewind/.  This seems
like a Bad Thing, in fact the #includes in pg_rewind/ are already a
little confused due to this.  I think we should either rename those
two pg_rewind files to something else, or rename the generic ones,
perhaps to "fe_logging.[hc]".  The latter could be done nearly
trivially as part of the movement patch, but on cosmetic grounds
I'd be more inclined to do the former instead.  Thoughts?

			regards, tom lane

  8. Re: PG12, PGXS and linking pgfeutils

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-13T19:58:51Z

    On 2019-May-13, Tom Lane wrote:

> I started working on a patch to do that, and soon noticed that there
> are pre-existing files logging.[hc] in src/bin/pg_rewind/.  This seems
> like a Bad Thing, in fact the #includes in pg_rewind/ are already a
> little confused due to this.  I think we should either rename those
> two pg_rewind files to something else, or rename the generic ones,
> perhaps to "fe_logging.[hc]".  The latter could be done nearly
> trivially as part of the movement patch, but on cosmetic grounds
> I'd be more inclined to do the former instead.  Thoughts?

I'd rename both :-)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: PG12, PGXS and linking pgfeutils

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-13T22:51:54Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-May-13, Tom Lane wrote:
>> I started working on a patch to do that, and soon noticed that there
>> are pre-existing files logging.[hc] in src/bin/pg_rewind/.  This seems
>> like a Bad Thing, in fact the #includes in pg_rewind/ are already a
>> little confused due to this.  I think we should either rename those
>> two pg_rewind files to something else, or rename the generic ones,
>> perhaps to "fe_logging.[hc]".  The latter could be done nearly
>> trivially as part of the movement patch, but on cosmetic grounds
>> I'd be more inclined to do the former instead.  Thoughts?

> I'd rename both :-)

On closer inspection, there's so little left in pg_rewind's logging.h/.c
(one function and a couple of global variables) that the better answer
is probably just to move those objects somewhere else and nuke the
separate files altogether.  As attached.

			regards, tom lane

  10. Re: PG12, PGXS and linking pgfeutils

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-14T18:38:58Z

    I wrote:
> I think moving fe_utils/logging.[hc] to
> common/ is definitely the way to get out of this problem.

I've pushed that, so Ian's problem should be gone as of HEAD.

			regards, tom lane

  11. Re: PG12, PGXS and linking pgfeutils

    Ian Barwick <ian.barwick@2ndquadrant.com> — 2019-05-14T23:50:46Z

    On 5/15/19 3:38 AM, Tom Lane wrote:
> I wrote:
>> I think moving fe_utils/logging.[hc] to
>> common/ is definitely the way to get out of this problem.
> 
> I've pushed that, so Ian's problem should be gone as of HEAD.

Thanks, that resolves the issue!


Regards

Ian Barwick


-- 
  Ian Barwick                   https://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services