Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-04-11 Thread Alvaro Herrera

Excerpts from Alvaro Herrera's message of lun abr 09 16:41:50 -0300 2012:

 There are three minor things that need to be changed for this to be
 committable:

Committed this patch after some more editorialization; in particular the
test was rewritten so that instead of trying to connect, it uses
PQconninfoParse to figure out how the URI is parsed, which makes a
lot more sense.  Also some other changes to the accepted URI, in
particular so that username, pwd, and port are possible to be specified
when using unix-domain sockets.

Now that it is a done deal I'm sure people will start complaining how
bad the documentation change was; please keep the flames up.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-04-09 Thread Alvaro Herrera
Excerpts from Peter Eisentraut's message of vie abr 06 03:09:10 -0300 2012:
 On fre, 2012-04-06 at 00:25 -0300, Alvaro Herrera wrote:
  Some moments of radical thinking later, I became unhappy with the fact
  that the conninfo stuff and parameter keywords are all crammed in the
  PQconnectdbParams description.  This feels wrong to me, even more so
  after we expand it even more to add URIs to the mix.  I think it's
  better to create a separate sect1 (which I've entitled Connection
  Strings) which explains the conninfo and URI formats as well as
  accepted keywords.  The new section is referenced from the multiple
  places that need it, without having to point to PQconnectdbParams.
 
 Yes, it should be split out.  But the libpq chapter already has too many
 tiny sect1s.  I think it should be a sect2 under Database Connection
 Control.

Thanks, that seems a good idea.  I have tweaked things slightly and it
looks pretty decent to me.  Wording improvements are welcome.  The file
in its entirety can be seen here:
https://github.com/alvherre/postgres/blob/uri/doc/src/sgml/libpq.sgml
The new bits start at line 1224.  I also attach the HTML output for easy
reading.  (I wonder if it's going to be visible in the archives).

There are three minor things that need to be changed for this to be
committable:

1. it depends on strtok_r which is likely to be a problem in MSVC++ and
perhaps older Unix platforms as well.

2. The ssl=true trick being converted into sslmode=require doesn't work
if the URI specifies them uri-encoded, which seems bogus.

3. if an unknown keyword is uri-encoded, the error message displays it
still uri-encoded.  Seems to me it'd be better to uri-decode it before
throwing error.

Alexander says he's going to work on these and then I'll finally commit it.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Title: Database Connection Control Functions

PostgreSQL 9.2devel DocumentationPrevUpChapter 31. libpq - C LibraryNext31.1. Database Connection Control Functions   The following functions deal with making a connection to a
   PostgreSQL backend server.  An
   application program can have several backend connections open at
   one time.  (One reason to do that is to access more than one
   database.)  Each connection is represented by a
   PGconn object, which
   is obtained from the function PQconnectdb,
   PQconnectdbParams, or
   PQsetdbLogin.  Note that these functions will always
   return a non-null object pointer, unless perhaps there is too
   little memory even to allocate the PGconn object.
   The PQstatus function should be called to check
   the return value for a successful connection before queries are sent
   via the connection object.

   Warning On Unix, forking a process with open libpq connections can lead to
 unpredictable results because the parent and child processes share
 the same sockets and operating system resources.  For this reason,
 such usage is not recommended, though doing an exec from
 the child process to load a new executable is safe.


   Note:  On Windows, there is a way to improve performance if a single
 database connection is repeatedly started and shutdown.  Internally,
 libpq calls WSAStartup() and WSACleanup() for connection startup
 and shutdown, respectively.  WSAStartup() increments an internal
 Windows library reference count which is decremented by WSACleanup().
 When the reference count is just one, calling WSACleanup() frees
 all resources and all DLLs are unloaded.  This is an expensive
 operation.  To avoid this, an application can manually call
 WSAStartup() so resources will not be freed when the last database
 connection is closed.


   PQconnectdbParams   Makes a new connection to the database server.

PGconn *PQconnectdbParams(const char * const *keywords,
  const char * const *values,
  int expand_dbname);
 This function opens a new database connection using the parameters taken
   from two NULL-terminated arrays. The first,
   keywords, is defined as an array of strings, each one
   being a key word. The second, values, gives the value
   for each key word. Unlike PQsetdbLogin below, the parameter
   set can be extended without changing the function signature, so use of
   this function (or its nonblocking analogs PQconnectStartParams
   and PQconnectPoll) is preferred for new application
   programming.
 The currently recognized parameter key words are listed in
   Section 31.1.1.
 When expand_dbname is non-zero, the
   dbname key word value is allowed to be recognized
   as a connection string. More details on the possible formats appear in 
   Section 31.1.2.
 The passed arrays can be empty to use all default 

Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-04-06 Thread Peter Eisentraut
On fre, 2012-04-06 at 00:25 -0300, Alvaro Herrera wrote:
 Some moments of radical thinking later, I became unhappy with the fact
 that the conninfo stuff and parameter keywords are all crammed in the
 PQconnectdbParams description.  This feels wrong to me, even more so
 after we expand it even more to add URIs to the mix.  I think it's
 better to create a separate sect1 (which I've entitled Connection
 Strings) which explains the conninfo and URI formats as well as
 accepted keywords.  The new section is referenced from the multiple
 places that need it, without having to point to PQconnectdbParams.

Yes, it should be split out.  But the libpq chapter already has too many
tiny sect1s.  I think it should be a sect2 under Database Connection
Control.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-04-05 Thread Peter Eisentraut
On ons, 2012-03-28 at 01:49 +0300, Alex wrote:
 Attached is a gzipped v10, complete with the above fixes and fixes to
 bugs found by Heikki.  Documentation and code comments are also
 finally updated.

The compiler warning is still there:

fe-connect.c: In function ‘conninfo_parse’:
fe-connect.c:4122:20: error: unused variable ‘option’ [-Werror=unused-variable]



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-04-05 Thread Alvaro Herrera
Excerpts from Peter Eisentraut's message of jue abr 05 13:52:13 -0300 2012:
 On ons, 2012-03-28 at 01:49 +0300, Alex wrote:
  Attached is a gzipped v10, complete with the above fixes and fixes to
  bugs found by Heikki.  Documentation and code comments are also
  finally updated.
 
 The compiler warning is still there:
 
 fe-connect.c: In function ‘conninfo_parse’:
 fe-connect.c:4122:20: error: unused variable ‘option’ 
 [-Werror=unused-variable]

Here's an updated patch, including this fix and others.  (Most notable
the fact that I got rid of conninfo_store_uri_encoded_value, instead
expanding conninfo_storeval a bit).  I also fixed the test script to run
in VPATH.  I intend to commit this shortly, barring objection from Peter
who is listed as committer in the CF app.

I think the only thing I'm not really sure about is the usage of the
synopsis tag to mark example URIs in the docs.  It looks to me that
they should mostly be literal instead, but I'm not really sure about
that either -- I'm suspecting the PDF output would look rather horrible.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


libpq-uri-v11.patch.gz
Description: GNU Zip compressed data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-27 Thread Heikki Linnakangas

On 22.03.2012 23:42, Alex wrote:

Okay, at last here's v9, rebased against current master branch.


Some quick comments on this patch:

I see a compiler warning:
fe-connect.c: In function ‘conninfo_parse’:
fe-connect.c:4113: warning: unused variable ‘option’

Docs are missing.

I wonder if you should get an error if you try specify the same option 
multiple times. At least the behavior needs to be documented.


Should %00 be forbidden?

The error message is a bit confusing for 
postgres://localhost?dbname=%XXfoo:

WARNING: ignoring unrecognized URI query parameter: dbname
There is in fact nothing wrong with dbname, it's the %XX in the value 
that's bogus.


Looking at conninfo_uri_decode(), I think it's missing a bounds check, 
and peeks at two bytes beyond the end of string if the input ends in a '%'.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-27 Thread Alex Shulgin

Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes:

 On 22.03.2012 23:42, Alex wrote:
 Okay, at last here's v9, rebased against current master branch.

 Some quick comments on this patch:

Heikki, thank you for taking a look at this!

 I see a compiler warning:
 fe-connect.c: In function ‘conninfo_parse’:
 fe-connect.c:4113: warning: unused variable ‘option’

The warning is due to the earlier commit e9ce658b.  I believe the above
line supposed to go away.

 Docs are missing.

Yes, my plan is to focus on the documentation and code comments while
sorting out any remaining issues with the code.

 I wonder if you should get an error if you try specify the same option
 multiple times. At least the behavior needs to be documented.

Since conninfo strings may contain duplicated keywords and the latter
just takes precedence, I think we should just do the same with URIs
(which we already do.)  I don't see the behavior of conninfo strings
documented anywhere, however.

 Should %00 be forbidden?

Probably yes, good spot.

 The error message is a bit confusing for
 postgres://localhost?dbname=%XXfoo:
 WARNING: ignoring unrecognized URI query parameter: dbname
 There is in fact nothing wrong with dbname, it's the %XX in the
 value that's bogus.

Hm, yes, that's a bug.

Looks like conninfo_uri_parse_params needs to be adjusted to properly
pass the error message generated by conninfo_store_uri_encoded_value.  I
wonder if examining the errorMessage buffer to tell if it's a hard error
(it's going to be empty in the case of ignoreMissing=true) is a good
practice.

 Looking at conninfo_uri_decode(), I think it's missing a bounds check,
 and peeks at two bytes beyond the end of string if the input ends in a
 %'.

No, in that case what happens on L4919 is this: we dereference q which
is pointing at NUL terminator and pass the value to the first
get_hexdigit in the if condition, the pointer itself is then
incremented and does point beyond the end of string, but since
get_hexdigit returns FALSE we don't call the second get_hexdigit, so we
don't dereference the invalid pointer.

There is a comment right before that if which says just that.

--
Alex

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-27 Thread Peter Eisentraut
On tor, 2012-03-22 at 23:42 +0200, Alex wrote:
 Okay, at last here's v9, rebased against current master branch.

Attached is a patch on top of your v9 with two small fixes:

- Don't provide a check target in libpq/Makefile if it's not
implemented.

- Use the configured port number for running the tests (otherwise it
runs against my system installation, for example).

diff --git i/src/interfaces/libpq/Makefile w/src/interfaces/libpq/Makefile
index f19f272..266e3db 100644
--- i/src/interfaces/libpq/Makefile
+++ w/src/interfaces/libpq/Makefile
@@ -121,7 +121,7 @@ install: all installdirs install-lib
 	$(INSTALL_DATA) $(srcdir)/pqexpbuffer.h '$(DESTDIR)$(includedir_internal)'
 	$(INSTALL_DATA) $(srcdir)/pg_service.conf.sample '$(DESTDIR)$(datadir)/pg_service.conf.sample'
 
-check installcheck:
+installcheck:
 	$(MAKE) -C test $@
 
 installdirs: installdirs-lib
diff --git i/src/interfaces/libpq/test/Makefile w/src/interfaces/libpq/test/Makefile
index 964bb20..869a2f0 100644
--- i/src/interfaces/libpq/test/Makefile
+++ w/src/interfaces/libpq/test/Makefile
@@ -5,7 +5,7 @@ include $(top_builddir)/src/Makefile.global
 all: installcheck
 
 installcheck:
-	BINDIR='$(bindir)' SUBDIR='$(subdir)' $(SHELL) ./regress.sh
+	BINDIR='$(bindir)' PGPORT='$(DEF_PGPORT)' SUBDIR='$(subdir)' $(SHELL) ./regress.sh
 
 clean distclean maintainer-clean:
 	rm -f regress.out regress.diff

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-27 Thread Alex

Peter Eisentraut pete...@gmx.net writes:

 On tor, 2012-03-22 at 23:42 +0200, Alex wrote:
 Okay, at last here's v9, rebased against current master branch.

 Attached is a patch on top of your v9 with two small fixes:

 - Don't provide a check target in libpq/Makefile if it's not
 implemented.

 - Use the configured port number for running the tests (otherwise it
 runs against my system installation, for example).

Neat, thank you.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-27 Thread Alex
Alex a...@commandprompt.com writes:

 Peter Eisentraut pete...@gmx.net writes:

 Attached is a patch on top of your v9 with two small fixes:

 - Don't provide a check target in libpq/Makefile if it's not
 implemented.

 - Use the configured port number for running the tests (otherwise it
 runs against my system installation, for example).

 Neat, thank you.

Attached is a gzipped v10, complete with the above fixes and fixes to
bugs found by Heikki.  Documentation and code comments are also finally
updated.

Of all the command-line utilities which can accept conninfo string, only
psql mentions that, so only its documentation was updated.  Other
utilities, like pg_dump and pg_restore can also work with either
conninfo or URI, however this remains undocumented.

--
Alex


libpq-uri-v10.patch.gz
Description: libpq-uri-v10.patch.gz

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: Trivial libpq refactoring patch (was: Re: [HACKERS] Another review of URI for libpq, v7 submission)

2012-03-22 Thread Tom Lane
Alex Shulgin a...@commandprompt.com writes:
 While working on this fix I've figured out that I need my
 conninfo_uri_parse to support use_defaults parameter, like
 conninfo(_array)_parse functions do.

 The problem is that the block of code which does the defaults handling
 is duplicated in both of the above functions.  What I'd like to do is
 extract it into a separate function to call.  What I wouldn't like is
 bloating the original URI patch with this unrelated change.

Applied with minor adjustments --- notably, I thought
conninfo_add_defaults was a better name for the function.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-22 Thread Alex
Alex a...@commandprompt.com writes:

 Marko Kreen mark...@gmail.com writes:

 On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
 https://github.com/a1exsh/postgres/commits/uri

 The point of the patch is to have one string with all connection options,
 in standard format, yes?  So why does not this work:

   db = PQconnectdb(postgres://localhost);

 ?

 Good catch.

 I've figured out that we'll need a bit more intrusive change than simply
 overriding the expand_dbname check in conninfo_array_parse (like the
 current version does) to support URIs in all PQconnect* variants.

Okay, at last here's v9, rebased against current master branch.

What's new in this version is parse_connection_string function to be
called instead of conninfo_parse.  It will check for possible URI in the
connection string and dispatch to conninfo_uri_parse if URI was found,
otherwise it will fall back to conninfo_parse.

In two places in code we don't want to parse the string unless it is
deemed a real connection string and not plain dbname.  The new function
recognized_connection_string is added to check this.

Thanks Marko for spotting the problem and thanks Tom for committing the
refactoring patch!

--
Alex



binFI5wAVF1ys.bin
Description: libpq-uri-v9.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Trivial libpq refactoring patch (was: Re: [HACKERS] Another review of URI for libpq, v7 submission)

2012-03-21 Thread Alex Shulgin
Alex a...@commandprompt.com writes:

 Marko Kreen mark...@gmail.com writes:

 On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
 https://github.com/a1exsh/postgres/commits/uri

 The point of the patch is to have one string with all connection options,
 in standard format, yes?  So why does not this work:

   db = PQconnectdb(postgres://localhost);

 ?

 Good catch.

 I've figured out that we'll need a bit more intrusive change than simply
 overriding the expand_dbname check in conninfo_array_parse (like the
 current version does) to support URIs in all PQconnect* variants.

 I still need to figure out some details, but this is to give people a
 status update.

While working on this fix I've figured out that I need my
conninfo_uri_parse to support use_defaults parameter, like
conninfo(_array)_parse functions do.

The problem is that the block of code which does the defaults handling
is duplicated in both of the above functions.  What I'd like to do is
extract it into a separate function to call.  What I wouldn't like is
bloating the original URI patch with this unrelated change.

So here's a trivial patch to do the refactoring.  Also, it uses the
newly added conninfo_fill_defaults directly in PQconndefaults, instead
of doing the parse empty conninfo string trick.  If this could be
applied, I'd rebase my patch against the updated master branch and
submit the new version.

As it goes, the patch adds a little duplication when it comes to
creating a working copy of PQconninfoOptions array.  Attached is the
second patch on top of the first one to extract this bit of code into
conninfo_init.  Not sure if we should go all the way through this, so
it's not critical if this one is not applied.

--
Regards,
Alex
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***
*** 297,302  static PQconninfoOption *conninfo_parse(const char *conninfo,
--- 297,304 
  static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
 const char *const * values, 
PQExpBuffer errorMessage,
 bool use_defaults, int expand_dbname);
+ static bool conninfo_fill_defaults(PQconninfoOption *options,
+  PQExpBuffer errorMessage);
  static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
  static void defaultNoticeReceiver(void *arg, const PGresult *res);
***
*** 836,842  PQconndefaults(void)
initPQExpBuffer(errorBuf);
if (PQExpBufferDataBroken(errorBuf))
return NULL;/* out of memory already :-( */
!   connOptions = conninfo_parse(, errorBuf, true);
termPQExpBuffer(errorBuf);
return connOptions;
  }
--- 838,860 
initPQExpBuffer(errorBuf);
if (PQExpBufferDataBroken(errorBuf))
return NULL;/* out of memory already :-( */
! 
!   /* Make a working copy of PQconninfoOptions */
!   connOptions = malloc(sizeof(PQconninfoOptions));
!   if (connOptions == NULL)
!   {
!   printfPQExpBuffer(errorBuf,
! libpq_gettext(out of 
memory\n));
!   return NULL;
!   }
!   memcpy(connOptions, PQconninfoOptions, sizeof(PQconninfoOptions));
! 
!   if (!conninfo_fill_defaults(connOptions, errorBuf))
!   {
!   PQconninfoFree(connOptions);
!   connOptions = NULL;
!   }
! 
termPQExpBuffer(errorBuf);
return connOptions;
  }
***
*** 4002,4008  conninfo_parse(const char *conninfo, PQExpBuffer 
errorMessage,
char   *pname;
char   *pval;
char   *buf;
-   char   *tmp;
char   *cp;
char   *cp2;
PQconninfoOption *options;
--- 4020,4025 
***
*** 4170,4245  conninfo_parse(const char *conninfo, PQExpBuffer 
errorMessage,
free(buf);
  
/*
!* Stop here if caller doesn't want defaults filled in.
!*/
!   if (!use_defaults)
!   return options;
! 
!   /*
!* If there's a service spec, use it to obtain any not-explicitly-given
!* parameters.
 */
!   if (parseServiceInfo(options, errorMessage))
{
PQconninfoFree(options);
return NULL;
}
  
-   /*
-* Get the fallback resources for parameters not specified in the 
conninfo
-* string nor the service.
-*/
-   for (option = options; option-keyword != NULL; option++)
-   {
-   if (option-val != NULL)
-   continue;   /* Value was in 
conninfo or service */
- 
-   /*
-* Try to get the environment variable fallback
-*/
-   if (option-envvar != NULL)
-   {

Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-20 Thread Alex

Marko Kreen mark...@gmail.com writes:

 On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
 https://github.com/a1exsh/postgres/commits/uri

 The point of the patch is to have one string with all connection options,
 in standard format, yes?  So why does not this work:

   db = PQconnectdb(postgres://localhost);

 ?

Good catch.

I've figured out that we'll need a bit more intrusive change than simply
overriding the expand_dbname check in conninfo_array_parse (like the
current version does) to support URIs in all PQconnect* variants.

I still need to figure out some details, but this is to give people a
status update.

--
Alex

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-17 Thread Marko Kreen
On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
 https://github.com/a1exsh/postgres/commits/uri

The point of the patch is to have one string with all connection options,
in standard format, yes?  So why does not this work:

  db = PQconnectdb(postgres://localhost);

?

-- 
marko


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-15 Thread Alvaro Herrera

Excerpts from Daniel Farina's message of jue mar 15 05:49:50 -0300 2012:

 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?

Ugh, ouch.  Yeah, that was done by the archives.  It seems that when
attachments are text/plain Mhonarc applies anti-spamming to them :-(
The original message doesn't have that problem.  Sorry about that.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-15 Thread Alex Shulgin
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[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



binaKreQKSDSW.bin
Description: libpq-uri-v8.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-15 Thread Daniel Farina
On Thu, Mar 15, 2012 at 7:36 AM, Alex Shulgin a...@commandprompt.com wrote:
 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.

I suspect we're long past the point in Internet history where such
simple obfuscation could possibly matter.

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

I'll give it another look-over. Do you have these in git somewhere? It
will help me save time on some of the incremental changes.

-- 
fdr

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Another review of URI for libpq, v7 submission

2012-03-15 Thread Alex

Daniel Farina dan...@heroku.com writes:

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

 I'll give it another look-over. Do you have these in git somewhere? It
 will help me save time on some of the incremental changes.

Yes, I've just pushed my dev branch to this fork of mine:
https://github.com/a1exsh/postgres/commits/uri

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers