Re: Another review of URI for libpq, v7 submission

Daniel Farina <daniel(at)heroku(dot)com> writes:

> I reviewed this and so far have not found any serious problems,
> although as is par for the course it contains some of the fiddly bits
> involved in any string manipulations in C. I made a few edits -- none
> strictly necessary for correctness -- that the original author is free
> audit and/or include[0].

Thank you for the review, Daniel!

Apparently, I was on drugs when I've submitted v7, as it still contains
the bug for which to fix I was forced to move parts of the code back
into the main parser routine...

> I did put in some defensive programming choices (instead of if/else
> if/elseif/else raise an error, even if the latter is allegedly
> impossible) that I think are a good idea.

Yes, this is a good idea, I'll incorporate them in the patch. However,
this one doesn't work:
https://github.com/fdr/postgres/commit/4fad90fb243d9266b1003cfbcf8397f67269fad3

Neither '@' or '/' are mandatory in the URI anywhere after "scheme://",
so we can't just say it "should never happen." Running the regression
test with this commit applied highlights the problem.

> One thing I found puzzling was that in the latest revision the tests
> appeared to be broken for me: all "@" signs were translated to "(at)".
> Is that mangling applied by the archives, or something?

This is definitely mangling by the lists. I can see this has happened
to people before, e.g.:
http://archives.postgresql.org/pgsql-hackers/2010-01/msg00938.php

But that discussion didn't seem to lead to a solution for the problem.

I wonder if there's any evidence as to that mangling the email addresses
helps to reduce spam at all? I mean replacing "(at)" back to "@" and
"(dot)" to "." is piece of cake for a spam crawler.

What I see though, is that most of the message-id URLs are broken by the
mangling. Also I don't think everyone should just tolerate that it
messes with the attachments. Should we all suddenly use
application/octet-stream or application/gzip instead of text/plain?

> The test suite neatly tries to copy pg_regress's general "make
> installcheck" style, but it likes to use my username as the database
> rather than the standard "regression" as seen by pg_regress. It is
> nice that a place to test connection strings and such is there, where
> there was none before.

Oh, I don't see why we can't use "regression" too.

While testing this I've also noticed that there was some funny behavior
when you left out the final slash after hostname, e.g.:
"postgres://localhost/" vs. "postgres://localhost". It turned out in
the former case we were setting dbname conninfo parameter to an empty
string and in the latter one we didn't set it at all. The difference is
that when we do set it, the default dbname is derived from username, but
when we don't--$PGDATABASE is used. I've fixed this, so now both URIs
work the same way (both do not set dbname.)

It also appeared to me that with the current code, a wide range of
funny-looking URIs are considered valid, e.g.:

postgres://user@
postgres://@host
postgres://host:/

and, taking approach to the extreme:

postgres://:@:

This specifies empty user, password, host and port, and looks really
funny. I've added (actually revived) some checks to forbid setting
empty URI parts where it doesn't make much sense.

Finally, attached is v8. Hopefully I didn't mess things up too much.

--
Regards,
Alex