From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
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 02:09:38 |
Message-ID: | CAB7nPqTSM8ydS=nyW_rb3nE=sEQgrhAtXKEWGejPnehvWRsjjg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sun, Aug 21, 2016 at 3:02 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> A bigger issue here is that it seems fundamentally wrong for initdb to be
>> including libpq, because it surely is never meant to be communicating
>> with a running postmaster. Not sure what to do about that. We could
>> consider moving pqexpbuffer out of libpq into fe_utils, but I wonder
>> whether that would break any third-party code. We've never advertised
>> pqexpbuffer.h as a supported API of libpq, but it's probably handy enough
>> that people use it anyway. I suppose we could duplicate it in fe_utils
>> and libpq, though that's a tad ugly. Thoughts?
>
> I looked into this and soon found that fe_utils/string_utils.o has
> dependencies on libpq that are much wider than just pqexpbuffer :-(.
> It might be a project to think about sorting that out sometime, but it
> looks like it would be an awful lot of work just to avoid having initdb
> depend on libpq.so. So I now think this objection is impractical.
> I'll just annotate the makefile entry to say that initdb itself doesn't
> use libpq.
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.
>> Another perhaps-only-cosmetic issue is that now initdb prints quotes
>> whether they are needed or not. I find this output pretty ugly:
>> ...
>> That's not really the fault of this patch perhaps. Maybe we could adjust
>> appendShellString so it doesn't add quotes if they are clearly
>> unnecessary.
>
> I still think this is worth fixing, but it's a simple modification.
> Will take care of it.
>
> This item is listed in the commitfest as a bug fix, but given the lack of
> previous complaints, and the fact that the printed command isn't really
> meant to be blindly copied-and-pasted anyway (you're at least meant to
> replace "logfile" with something), I do not think it needs back-patching.
Agreed on that. Only first-time users do a copy-paste of the command
anyway, so the impact is very narrow.
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? I think that you saw the error for initdb because
that's the first one to be built in src/bin/, and I think that Ryan
used -lpq in his patch because the same pattern is used everywhere
else.
It seems to me as well that submake-libpq and submake-libpgfeutils
should be listed in initdb's Makefile when building the binary.
Am I missing something? Please see the patch attached to see what I mean.
Thinking harder about that, it may be better to even add a variable in
Makefile.global.in for utilities in need of fe_utils, like that for
example:
libpg_feutils = -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
Thoughts?
--
Michael
Attachment | Content-Type | Size |
---|---|---|
bin-make-cleanup.patch | text/x-patch | 3.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2016-08-22 02:17:47 | Re: Should we cacheline align PGXACT? |
Previous Message | Robert Haas | 2016-08-22 02:05:43 | Re: synchronous_commit = remote_flush |