PG12, PGXS and linking pgfeutils
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).
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
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
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
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
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.oSo 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
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.oSo 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
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.cSeveral 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
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
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
Attachments:
get-rid-of-pg_rewind-logging-files.patchtext/x-diff; charset=us-ascii; name=get-rid-of-pg_rewind-logging-files.patchDownload+74-116
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
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