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

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