Daniel Farina <dan...@heroku.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.
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
-- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers