Обсуждение: Let's stop with the retail rebuilds of src/port/ files already

I'm getting tired of having to make fixes like ce4887bd0.  I think
we should rearrange things so that src/port/ and src/common/ compile
all their files a third time using shared-library-friendly switches,
put them into new .a files, and have libpq and the ecpg libraries
just include those libraries instead of what they're doing now.

This would result in compiling some of the port+common files uselessly,
since they'd never actually get pulled in by any shared library.
But I think we're approaching the point where we might have a net savings
of build time anyway, due to not having to compile the same files multiple
times in different subdirectories.  And it'd sure be a savings of
developer brain-cells and sanity.

Maybe use the extension "_shlib" (vs "_srv") for these .o and .a files.

Thoughts?

            regards, tom lane

Hi,

On 2018-09-26 19:10:40 -0400, Tom Lane wrote:
> I'm getting tired of having to make fixes like ce4887bd0.  I think
> we should rearrange things so that src/port/ and src/common/ compile
> all their files a third time using shared-library-friendly switches,
> put them into new .a files, and have libpq and the ecpg libraries
> just include those libraries instead of what they're doing now.

> This would result in compiling some of the port+common files uselessly,
> since they'd never actually get pulled in by any shared library.
> But I think we're approaching the point where we might have a net savings
> of build time anyway, due to not having to compile the same files multiple
> times in different subdirectories.  And it'd sure be a savings of
> developer brain-cells and sanity.

+1

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2018-09-26 19:10:40 -0400, Tom Lane wrote:
>> I'm getting tired of having to make fixes like ce4887bd0.  I think
>> we should rearrange things so that src/port/ and src/common/ compile
>> all their files a third time using shared-library-friendly switches,
>> put them into new .a files, and have libpq and the ecpg libraries
>> just include those libraries instead of what they're doing now.

> +1

Here's a partial patch for that: it adds the third build variant
to src/port/ and teaches libpq to use it.  We'd want to likewise
modify src/common/ and fix up other callers such as ecpg, but this
seems to be enough to test whether the idea works or not.

I've tried this on Linux, macOS and HPUX and it seems to work in
those cases, but I'm not foolish enough to imagine that that's
exhaustive.

What I think would make sense is to push this and see what the
buildfarm thinks of it.  If there are unfixable problems then
we won't have wasted time fleshing out the concept.  Otherwise,
I'll do the remaining pieces.

            regards, tom lane

diff --git a/src/interfaces/libpq/.gitignore b/src/interfaces/libpq/.gitignore
index ce1576e..8885e91 100644
*** a/src/interfaces/libpq/.gitignore
--- b/src/interfaces/libpq/.gitignore
***************
*** 1,27 ****
  /exports.list
  /libpq.rc
  # .c files that are symlinked in from elsewhere
- /chklocale.c
- /crypt.c
- /erand48.c
- /getaddrinfo.c
- /getpeereid.c
- /inet_aton.c
- /inet_net_ntop.c
- /noblock.c
- /open.c
- /system.c
- /pgsleep.c
- /pg_strong_random.c
- /pgstrcasecmp.c
- /pqsignal.c
- /snprintf.c
- /strerror.c
- /strlcpy.c
- /strnlen.c
- /thread.c
- /win32error.c
- /win32setlocale.c
  /ip.c
  /md5.c
  /base64.c
--- 1,6 ----
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index ef8abaf..769c58b 100644
*** a/src/interfaces/libpq/Makefile
--- b/src/interfaces/libpq/Makefile
*************** ifneq ($(PORTNAME), win32)
*** 24,50 ****
  override CFLAGS += $(PTHREAD_CFLAGS)
  endif

- # Need to recompile any external C files because we need
- # all object files to use the same compile flags as libpq; some
- # platforms require special flags.
- LIBS := $(LIBS:-lpgport=)
-
  # We can't use Makefile variables here because the MSVC build system scrapes
  # OBJS from this file.
  OBJS=    fe-auth.o fe-auth-scram.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-lobj.o \
      fe-protocol2.o fe-protocol3.o pqexpbuffer.o fe-secure.o \
      libpq-events.o
- # libpgport C files we always use
- OBJS += chklocale.o inet_net_ntop.o noblock.o pgstrcasecmp.o pqsignal.o \
-     snprintf.o strerror.o thread.o
- # libpgport C files that are needed if identified by configure
- OBJS += $(filter crypt.o getaddrinfo.o getpeereid.o inet_aton.o open.o system.o strlcpy.o strnlen.o win32error.o
win32setlocale.o,$(LIBOBJS)) 
-
- ifeq ($(enable_strong_random), yes)
- OBJS += pg_strong_random.o
- else
- OBJS += erand48.o
- endif

  # src/backend/utils/mb
  OBJS += encnames.o wchar.o
--- 24,34 ----
*************** override shlib = cyg$(NAME)$(DLSUFFIX)
*** 62,69 ****
  endif

  ifeq ($(PORTNAME), win32)
! # pgsleep.o is from libpgport
! OBJS += pgsleep.o win32.o libpqrc.o

  libpqrc.o: libpq.rc
      $(WINDRES) -i $< -o $@
--- 46,52 ----
  endif

  ifeq ($(PORTNAME), win32)
! OBJS += win32.o libpqrc.o

  libpqrc.o: libpq.rc
      $(WINDRES) -i $< -o $@
*************** endif
*** 76,86 ****

  # Add libraries that libpq depends (or might depend) on into the
  # shared library link.  (The order in which you list them here doesn't
! # matter.)
  ifneq ($(PORTNAME), win32)
! SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket
-lnsl-lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS) 
  else
! SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi32 -lssl -lsocket -lnsl -lresolv
-lintl-lm $(PTHREAD_LIBS), $(LIBS)) $(LDAP_LIBS_FE) 
  endif
  ifeq ($(PORTNAME), win32)
  SHLIB_LINK += -lshell32 -lws2_32 -lsecur32 $(filter -leay32 -lssleay32 -lcomerr32 -lkrb5_32, $(LIBS))
--- 59,70 ----

  # Add libraries that libpq depends (or might depend) on into the
  # shared library link.  (The order in which you list them here doesn't
! # matter.)  Note that we filter out -lpgport from LIBS and instead
! # insert -lpgport_shlib, to get port files that are built correctly.
  ifneq ($(PORTNAME), win32)
! SHLIB_LINK += -lpgport_shlib $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi
-lssl-lsocket -lnsl -lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS) 
  else
! SHLIB_LINK += -lpgport_shlib $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi32 -lssl -lsocket
-lnsl-lresolv -lintl -lm $(PTHREAD_LIBS), $(LIBS)) $(LDAP_LIBS_FE) 
  endif
  ifeq ($(PORTNAME), win32)
  SHLIB_LINK += -lshell32 -lws2_32 -lsecur32 $(filter -leay32 -lssleay32 -lcomerr32 -lkrb5_32, $(LIBS))
*************** SHLIB_EXPORTS = exports.txt
*** 90,111 ****

  all: all-lib

  # Shared library stuff
  include $(top_srcdir)/src/Makefile.shlib
  backend_src = $(top_srcdir)/src/backend


! # We use several libpgport and backend modules verbatim, but since we need
  # to compile with appropriate options to build a shared lib, we can't
! # necessarily use the same object files built for libpgport and the backend.
  # Instead, symlink the source files in here and build our own object files.
- # For some libpgport modules, this only happens if configure decides
- # the module is needed (see filter hack in OBJS, above).
  # When you add a file here, remember to add it in the "clean" target below.

- chklocale.c crypt.c erand48.c getaddrinfo.c getpeereid.c inet_aton.c inet_net_ntop.c noblock.c open.c system.c
pgsleep.cpg_strong_random.c pgstrcasecmp.c pqsignal.c snprintf.c strerror.c strlcpy.c strnlen.c thread.c win32error.c
win32setlocale.c:% : $(top_srcdir)/src/port/% 
-     rm -f $@ && $(LN_S) $< .
-
  ip.c md5.c base64.c link-canary.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c: % :
$(top_srcdir)/src/common/%
      rm -f $@ && $(LN_S) $< .

--- 74,92 ----

  all: all-lib

+ all-lib: | submake-libpgport
+
  # Shared library stuff
  include $(top_srcdir)/src/Makefile.shlib
  backend_src = $(top_srcdir)/src/backend


! # We use a few backend modules verbatim, but since we need
  # to compile with appropriate options to build a shared lib, we can't
! # use the same object files built for the backend.
  # Instead, symlink the source files in here and build our own object files.
  # When you add a file here, remember to add it in the "clean" target below.

  ip.c md5.c base64.c link-canary.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c: % :
$(top_srcdir)/src/common/%
      rm -f $@ && $(LN_S) $< .

*************** libpq.rc libpq-dist.rc: libpq.rc.in
*** 123,128 ****
--- 104,110 ----
  # installations and is only updated by distprep.)
  libpq.rc: $(top_builddir)/src/Makefile.global

+ # Make dependencies on pg_config_paths.h visible, too.
  fe-connect.o: fe-connect.c $(top_builddir)/src/port/pg_config_paths.h
  fe-misc.o: fe-misc.c $(top_builddir)/src/port/pg_config_paths.h

*************** clean distclean: clean-lib
*** 154,161 ****
      rm -f $(OBJS) pthread.h libpq.rc
  # Might be left over from a Win32 client-only build
      rm -f pg_config_paths.h
! # Remove files we (may have) symlinked in from src/port and other places
!     rm -f chklocale.c crypt.c erand48.c getaddrinfo.c getpeereid.c inet_aton.c inet_net_ntop.c noblock.c open.c
system.cpgsleep.c pg_strong_random.c pgstrcasecmp.c pqsignal.c snprintf.c strerror.c strlcpy.c strnlen.c thread.c
win32error.cwin32setlocale.c 
      rm -f ip.c md5.c base64.c link-canary.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c
      rm -f encnames.c wchar.c

--- 136,142 ----
      rm -f $(OBJS) pthread.h libpq.rc
  # Might be left over from a Win32 client-only build
      rm -f pg_config_paths.h
! # Remove files we (may have) symlinked in from src/common and other places
      rm -f ip.c md5.c base64.c link-canary.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c
      rm -f encnames.c wchar.c

diff --git a/src/port/.gitignore b/src/port/.gitignore
index 53a4032..2037b7d 100644
*** a/src/port/.gitignore
--- b/src/port/.gitignore
***************
*** 1,3 ****
--- 1,4 ----
  /libpgport.a
+ /libpgport_shlib.a
  /libpgport_srv.a
  /pg_config_paths.h
diff --git a/src/port/Makefile b/src/port/Makefile
index a2ee8e2..ec62a31 100644
*** a/src/port/Makefile
--- b/src/port/Makefile
***************
*** 1,18 ****
  #-------------------------------------------------------------------------
  #
  # Makefile
! #    Makefile for the port-specific subsystem of the backend
  #
! # These files are used in other directories for portability on systems
! # with broken/missing library files, and for common code sharing.
  #
! # This makefile generates two outputs:
  #
  #    libpgport.a - contains object files with FRONTEND defined,
! #        for use by client application and libraries
  #
  #    libpgport_srv.a - contains object files without FRONTEND defined,
! #        for use only by the backend binaries
  #
  # LIBOBJS is set by configure (via Makefile.global) to be the list of object
  # files that are conditionally needed as determined by configure's probing.
--- 1,23 ----
  #-------------------------------------------------------------------------
  #
  # Makefile
! #    Makefile for src/port
  #
! # These files are used by the Postgres backend, and also by frontend
! # programs.  Primarily, they are meant to provide portability on systems
! # with broken/missing library files.
  #
! # This makefile generates three outputs:
  #
  #    libpgport.a - contains object files with FRONTEND defined,
! #        for use by client applications
! #
! #    libpgport_shlib.a - contains object files with FRONTEND defined,
! #        built suitably for use in shared libraries; for use
! #        by libpq and other frontend libraries
  #
  #    libpgport_srv.a - contains object files without FRONTEND defined,
! #        for use only by the backend
  #
  # LIBOBJS is set by configure (via Makefile.global) to be the list of object
  # files that are conditionally needed as determined by configure's probing.
*************** ifeq ($(enable_strong_random), yes)
*** 40,51 ****
  OBJS += pg_strong_random.o
  endif

! # foo_srv.o and foo.o are both built from foo.c, but only foo.o has -DFRONTEND
  OBJS_SRV = $(OBJS:%.o=%_srv.o)

! all: libpgport.a libpgport_srv.a

  # libpgport is needed by some contrib
  install: all installdirs
      $(INSTALL_STLIB) libpgport.a '$(DESTDIR)$(libdir)/libpgport.a'

--- 45,59 ----
  OBJS += pg_strong_random.o
  endif

! # libpgport.a, libpgport_shlib.a, and libpgport_srv.a contain the same files
! # foo.o, foo_shlib.o, and foo_srv.o are all built from foo.c
! OBJS_SHLIB = $(OBJS:%.o=%_shlib.o)
  OBJS_SRV = $(OBJS:%.o=%_srv.o)

! all: libpgport.a libpgport_shlib.a libpgport_srv.a

  # libpgport is needed by some contrib
+ # currently we don't install libpgport_shlib.a, maybe we should?
  install: all installdirs
      $(INSTALL_STLIB) libpgport.a '$(DESTDIR)$(libdir)/libpgport.a'

*************** libpgport.a: $(OBJS)
*** 59,76 ****
      rm -f $@
      $(AR) $(AROPT) $@ $^

! # thread.o needs PTHREAD_CFLAGS (but thread_srv.o does not)
  thread.o: CFLAGS+=$(PTHREAD_CFLAGS)

! # pg_crc32c_sse42.o and its _srv.o version need CFLAGS_SSE42
  pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_SSE42)
  pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_SSE42)

! # pg_crc32c_armv8.o and its _srv.o version need CFLAGS_ARMV8_CRC32C
  pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C)
  pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C)

  #
  # Server versions of object files
  #

--- 67,104 ----
      rm -f $@
      $(AR) $(AROPT) $@ $^

! # thread.o and thread_shlib.o need PTHREAD_CFLAGS (but thread_srv.o does not)
  thread.o: CFLAGS+=$(PTHREAD_CFLAGS)
+ thread_shlib.o: CFLAGS+=$(PTHREAD_CFLAGS)

! # all versions of pg_crc32c_sse42.o need CFLAGS_SSE42
  pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_SSE42)
+ pg_crc32c_sse42_shlib.o: CFLAGS+=$(CFLAGS_SSE42)
  pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_SSE42)

! # all versions of pg_crc32c_armv8.o need CFLAGS_ARMV8_CRC32C
  pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C)
+ pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C)
  pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C)

  #
+ # Shared library versions of object files
+ #
+
+ libpgport_shlib.a: $(OBJS_SHLIB)
+     rm -f $@
+     $(AR) $(AROPT) $@ $^
+
+ # Because this uses its own compilation rule, it doesn't use the
+ # dependency tracking logic from Makefile.global.  To make sure that
+ # dependency tracking works anyway for the *_shlib.o files, depend on
+ # their *.o siblings as well, which do have proper dependencies.  It's
+ # a hack that might fail someday if there is a *_shlib.o without a
+ # corresponding *.o, but there seems little reason for that.
+ %_shlib.o: %.c %.o
+     $(CC) $(CFLAGS) $(CFLAGS_SL) $(CPPFLAGS) -c $< -o $@
+
+ #
  # Server versions of object files
  #

*************** libpgport_srv.a: $(OBJS_SRV)
*** 92,97 ****
--- 120,127 ----

  path.o: path.c pg_config_paths.h

+ path_shlib.o: path.c pg_config_paths.h
+
  path_srv.o: path.c pg_config_paths.h

  # We create a separate file rather than put these in pg_config.h
*************** pg_config_paths.h: $(top_builddir)/src/M
*** 112,115 ****
      echo "#define MANDIR \"$(mandir)\"" >>$@

  clean distclean maintainer-clean:
!     rm -f libpgport.a libpgport_srv.a $(OBJS) $(OBJS_SRV) pg_config_paths.h
--- 142,146 ----
      echo "#define MANDIR \"$(mandir)\"" >>$@

  clean distclean maintainer-clean:
!     rm -f libpgport.a libpgport_shlib.a libpgport_srv.a
!     rm -f $(OBJS) $(OBJS_SHLIB) $(OBJS_SRV) pg_config_paths.h

Hi,

On September 26, 2018 9:03:05 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Andres Freund <andres@anarazel.de> writes:
>> On 2018-09-26 19:10:40 -0400, Tom Lane wrote:
>>> I'm getting tired of having to make fixes like ce4887bd0.  I think
>>> we should rearrange things so that src/port/ and src/common/ compile
>>> all their files a third time using shared-library-friendly switches,
>>> put them into new .a files, and have libpq and the ecpg libraries
>>> just include those libraries instead of what they're doing now.
>
>> +1
>
>Here's a partial patch for that: it adds the third build variant
>to src/port/ and teaches libpq to use it.  We'd want to likewise
>modify src/common/ and fix up other callers such as ecpg, but this
>seems to be enough to test whether the idea works or not.
>
>I've tried this on Linux, macOS and HPUX and it seems to work in
>those cases, but I'm not foolish enough to imagine that that's
>exhaustive.
>
>What I think would make sense is to push this and see what the
>buildfarm thinks of it.  If there are unfixable problems then
>we won't have wasted time fleshing out the concept.  Otherwise,
>I'll do the remaining pieces.

Sounds reasonable to me.

Medium-long term I think we should consider trying to reduce the duplication tho.  Once we provide an elog and error
handlingwrapper, we really should be able to reduce duplication (code and build) a fair bit.  But that should be
tackledseparately. 

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Wed, Sep 26, 2018 at 09:24:48PM -0700, Andres Freund wrote:
> On September 26, 2018 9:03:05 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What I think would make sense is to push this and see what the
>> buildfarm thinks of it.  If there are unfixable problems then
>> we won't have wasted time fleshing out the concept.  Otherwise,
>> I'll do the remaining pieces.
>
> Sounds reasonable to me.

 # libpgport is needed by some contrib
+# currently we don't install libpgport_shlib.a, maybe we should?

Likely you should as this could be used directly by out-of-core things.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
>> On September 26, 2018 9:03:05 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>  # libpgport is needed by some contrib
>>> +# currently we don't install libpgport_shlib.a, maybe we should?

> Likely you should as this could be used directly by out-of-core things.

Maybe, but what things exactly?  Extension modules don't need it, as
they just call the versions built into the core backend.

            regards, tom lane

I wrote:
> Here's a partial patch for that: it adds the third build variant
> to src/port/ and teaches libpq to use it.  We'd want to likewise
> modify src/common/ and fix up other callers such as ecpg, but this
> seems to be enough to test whether the idea works or not.
> ...
> What I think would make sense is to push this and see what the
> buildfarm thinks of it.  If there are unfixable problems then
> we won't have wasted time fleshing out the concept.  Otherwise,
> I'll do the remaining pieces.

Well, the buildfarm did turn up one problem: on really old macOS
(cf prairiedog) the libpq link step fails with

ld: symbols names listed in -exported_symbols_list: exports.list not in linked objects
_pqsignal

Apparently, with that linker, the exported symbols list is resolved
against just what is found in the listed *.o files, not anything pulled
in from a library file.

Now, the question that raises in my mind is why is libpq.so exporting
pqsignal() at all?  Probably there was once a reason for it, but nowadays
I would think that any client program using pqsignal() should get it
from -lpgport, not from libpq.  Having more than one place to get it from
seems more likely to create issues than solve them.  And we certainly
do not document it as a function available from libpq.

So my recommendation is to remove pqsignal from libpq's exports.txt.
I've verified that prairiedog builds happily with that change,
confirming my expectation that all consumers of the symbol can get it
from someplace else.

Now, if we go forward with that solution, there will be issues with
some other things that libpq exports without having defined itself:

src/backend/utils/mb/wchar.c:
pg_utf_mblen
src/backend/utils/mb/encnames.c:
pg_encoding_to_char
pg_char_to_encoding
pg_valid_server_encoding
pg_valid_server_encoding_id

Now, I'd already had my eye on those two files, because after applying a
similar fix for src/common/, those two files would be the only ones that
libpq needs to symlink from somewhere else.

What I was thinking of proposing was to move those two files out of the
backend and into src/common/, thereby normalizing their status as
modules available in both frontend and backend, and removing the need
for a special build rule for them in libpq.  (initdb could be simplified
too.)  Per this discovery, we'd need to also remove these symbols from
libpq's exports list, meaning that clients *must* get them from -lpgcommon
not from libpq.

There's a small chance that this'd break third-party clients that
are using these symbols out of libpq.  We've never documented them
as being available, but somebody might be using them anyway.
If that does happen, it could be repaired by linking against -lpgcommon
along with libpq, but it'd possibly still make people unhappy.

Comments?

            regards, tom lane

On 09/27/2018 03:48 PM, Tom Lane wrote:
> I wrote:
>> Here's a partial patch for that: it adds the third build variant
>> to src/port/ and teaches libpq to use it.  We'd want to likewise
>> modify src/common/ and fix up other callers such as ecpg, but this
>> seems to be enough to test whether the idea works or not.
>> ...
>> What I think would make sense is to push this and see what the
>> buildfarm thinks of it.  If there are unfixable problems then
>> we won't have wasted time fleshing out the concept.  Otherwise,
>> I'll do the remaining pieces.
> Well, the buildfarm did turn up one problem: on really old macOS
> (cf prairiedog) the libpq link step fails with
>
> ld: symbols names listed in -exported_symbols_list: exports.list not in linked objects
> _pqsignal
>
> Apparently, with that linker, the exported symbols list is resolved
> against just what is found in the listed *.o files, not anything pulled
> in from a library file.
>
> Now, the question that raises in my mind is why is libpq.so exporting
> pqsignal() at all?  Probably there was once a reason for it, but nowadays
> I would think that any client program using pqsignal() should get it
> from -lpgport, not from libpq.  Having more than one place to get it from
> seems more likely to create issues than solve them.  And we certainly
> do not document it as a function available from libpq.
>
> So my recommendation is to remove pqsignal from libpq's exports.txt.
> I've verified that prairiedog builds happily with that change,
> confirming my expectation that all consumers of the symbol can get it
> from someplace else.
>
> Now, if we go forward with that solution, there will be issues with
> some other things that libpq exports without having defined itself:
>
> src/backend/utils/mb/wchar.c:
> pg_utf_mblen
> src/backend/utils/mb/encnames.c:
> pg_encoding_to_char
> pg_char_to_encoding
> pg_valid_server_encoding
> pg_valid_server_encoding_id
>
> Now, I'd already had my eye on those two files, because after applying a
> similar fix for src/common/, those two files would be the only ones that
> libpq needs to symlink from somewhere else.
>
> What I was thinking of proposing was to move those two files out of the
> backend and into src/common/, thereby normalizing their status as
> modules available in both frontend and backend, and removing the need
> for a special build rule for them in libpq.  (initdb could be simplified
> too.)  Per this discovery, we'd need to also remove these symbols from
> libpq's exports list, meaning that clients *must* get them from -lpgcommon
> not from libpq.
>
> There's a small chance that this'd break third-party clients that
> are using these symbols out of libpq.  We've never documented them
> as being available, but somebody might be using them anyway.
> If that does happen, it could be repaired by linking against -lpgcommon
> along with libpq, but it'd possibly still make people unhappy.
>


Seems a small enough price to pay.

cheers

andrew


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

I wrote:
> Now, if we go forward with that solution, there will be issues with
> some other things that libpq exports without having defined itself:
> src/backend/utils/mb/wchar.c:
> pg_utf_mblen
> src/backend/utils/mb/encnames.c:
> pg_encoding_to_char
> pg_char_to_encoding
> pg_valid_server_encoding
> pg_valid_server_encoding_id
> What I was thinking of proposing was to move those two files out of the
> backend and into src/common/, thereby normalizing their status as
> modules available in both frontend and backend, and removing the need
> for a special build rule for them in libpq.  (initdb could be simplified
> too.)  Per this discovery, we'd need to also remove these symbols from
> libpq's exports list, meaning that clients *must* get them from -lpgcommon
> not from libpq.

After further study I've concluded that moving those two files would
be more neatnik-ism than is justified.  While it'd get rid of the
symlink-a-source-file technique in libpq, there'd still be other
occurrences of that in our tree, so the actual cleanup benefit seems
pretty limited.  And while I'm prepared to believe that nobody outside PG
uses pqsignal() or should do so, it's a little harder to make that case
for the encnames.c functions; so the risk of causing problems seems
noticeably greater.

Accordingly, I cleaned up the usage of the existing src/common/ files
but didn't move anything around.  I plan to stop here unless the
buildfarm shows more issues.

            regards, tom lane