Re: Patch: initdb: "'" for QUOTE_PATH (non-windows)

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Ryan Murphy <ryanfmurphy(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Patch: initdb: "'" for QUOTE_PATH (non-windows)
Date: 2016-08-22 11:45:46
Message-ID: 24287.1471866346@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Sun, Aug 21, 2016 at 3:02 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I looked into this and soon found that fe_utils/string_utils.o has
>> dependencies on libpq that are much wider than just pqexpbuffer :-(.

> pqexpbuffer.c is an independent piece of facility, so we could move it
> to src/common and leverage the dependency a bit, and have libpq link
> to the source file itself at build phase. The real problem is the call
> to PQmblen in psqlscan.l... And this, I am not sure how this could be
> refactored cleanly.

I see all of these as libpq dependencies in string_utils.o:

U PQclientEncoding
U PQescapeStringConn
U PQmblen
U PQserverVersion

Maybe we could split that file into two parts (libpq-dependent and not)
but it would be pretty arbitrary.

> And actually, I had a look at the build failure that you reported in
> 3855(dot)1471713949(at)sss(dot)pgh(dot)pa(dot)us(dot) While that was because of a copy of
> libpq.so that I had in my own LD_LIBRARY_PATH, shouldn't all the other
> frontend utilities depending on fe_utils also use $(libpq_pgport)
> instead of -lpq?

All the rest of them already have that, because their link commands
look like, eg for psql,

LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq

psql: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) $(OBJS) $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $(at)$(X)
^^^^^^^^^^^^^^^^^^^^^^^^^^

The extra reference to -lpq just makes sure that references to libpq from
libpgfeutils get resolved properly. (And yeah, there are some platforms
that need that :-(.) We don't need an extra copy of the -L flag.

This is all pretty messy, not least because of the way libpq_pgport
is set up; as Makefile.global notes,

# ... This does cause duplicate -lpgport's to appear
# on client link lines.

Likely it would be better to refactor all of this so we get just one
reference to libpq and one reference to libpgport, but that'd require
a more thoroughgoing cleanup than you have here. (Now that I think
about it, adding -L/-l to LDFLAGS is pretty duff coding style to
begin with --- we should be adding those things to LIBS, or at least
putting them just before LIBS in the command lines.)

You're right that I missed the desirability of invoking
submake-libpq and submake-libpgfeutils in initdb's build.
Will fix that.

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2016-08-22 11:55:40 Re: Patch: initdb: "'" for QUOTE_PATH (non-windows)
Previous Message Heikki Linnakangas 2016-08-22 10:46:08 Race condition in GetOldestActiveTransactionId()