Re: POLA violation with \c service=

On Fri, Feb 20, 2015 at 05:55:20PM -0300, Alvaro Herrera wrote:
> Pavel Stehule wrote:
> > 2015-02-20 8:22 GMT+01:00 David Fetter <david(at)fetter(dot)org>:
> >
> > > On Fri, Feb 20, 2015 at 07:10:29AM +0100, Pavel Stehule wrote:
> > > > Hi
> > > >
> > > > I am happy with doc changes now.
> > > >
> > > > When I test last patch, I found sigfault bug, because host =
> > > > PQhost(o_conn); returns NULL. I fexed it - please, see patch 007
> > > >
> > > > If you are agree with fix, I'll mark this patch as ready for commit.
> > >
> > > Thanks for fixing the bug. Let's go with this.
> > >
> >
> > marked as "ready for commit"
>
> Gave this patch a look. In general it looks pretty good, but there is
> one troublesome point: it duplicates two functions from libpq into psql,
> including the URI designators. This doesn't look very nice.

My thinking behind this was that the patch is a bug fix and intended
to be back-patched, so I wanted to mess with as little infrastructure
as possible. A new version of libpq seems like a very big ask for
such a case. You'll recall that the original problem was that

\c service=foo

only worked accidentally for some pretty narrow use cases and broke
without much of a clue for the rest. It turned out that the general
problem was that options given to psql on the command line were not
even remotely equivalent to \c, even though they were documented to
be.

> I thought about just creating a new src/common (say connstring.c) to
> host those two functions and the URI designators, but then on closer
> look I noticed that libpq's facilities for URI parsing become
> severed: two very small functions become part of libpgcommon, while
> the more complex parts remain in libpq.

Right.

> On the other hand, if we see that psql needs this functionality, isn't
> this a clue that other client programs might find it useful too?
> (Honestly I'm not completely sure about this point -- other opinions?)

It might well.

> I see three[four] ways forward from here:
>
> 1. export this functionality in libpq as one or two new functions. This
> would need proper docs, exports.txt, etc.

That sounds like a great thing going forward.

> 2. export it in libpgcommon. If we choose this option we should
> probably rename those functions, as in the attached patch.

I'm not super attached to the names.

> 3. accept the patch as is, i.e. duplicate the libq-internal functions in
> psql.

Again, my chief concern was to produce a back-patchable bug fix. The
internal functions could be in the going-backward versions, and the
shared ones in the going-forward (9.5+) versions.

> [4. reject the whole thing]
>
> I lean towards (2) myself, but what do others think?

Obviously, I'm biased, as the original impulse to fix this bug was
mine.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate