Hi Tom,

On 2014-02-04 12:02:45 -0500, Tom Lane wrote:
> Andres Freund <and...@2ndquadrant.com> writes:
> > On 2014-02-04 11:36:22 -0500, Tom Lane wrote:
> >> -1.  This is not a general solution to the problem.  There are other
> >> GUCs for which people might want spaces in the value.
> 
> > Sure, I didn't say it was. But I don't see any oother values that are
> > likely being passed via PGOPTIONS that frequently contain spaces.
> 
> application_name --- weren't we just reading about people passing entire
> command lines there?  (They must be using some other way of setting it
> currently, but PGOPTIONS doesn't seem like an implausible source.)

You can't easily use PGOPTIONS to set application_name in many cases
anyway, libpq's support for it gets in the way since it takes effect
later. And I think libpq is much more likely way to set it. Also you can
simply circumvent the problem by using a different naming convention,
that's not problem with repeatable read.

So I still think we should add read_committed, repeatable_read as aliases.

> >> Yeah.  See pg_split_opts(), which explicitly acknowledges that it'll fall
> >> down for space-containing options.  Not sure what the most appropriate
> >> quoting convention would be there, but I'm sure we can think of something.
> 
> > No argument against introducing it. What about simply allowing escaping
> > of the next character using \?
> 
> The same thought had occurred to me.  Since it'll typically already be
> inside some levels of quoting, any quoted-string convention seems like
> it'd be a pain to use.  But a straight backslash-escapes-the-next-char
> thing wouldn't be too awful, I think.

Ok, here's a patch implementing that. There's a slight backward concern
in that a \ would earlier have been passed through unmodified, but now
would be taken as a escape. I think that's not too much of a problem
though.
I thought about simply outputting the escape unless it's been used as an
escape before a speace, but that seems like a bad idea, barring future
uses to me.

Greetings,

Andres Freund

-- 
 Andres Freund                     http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
>From 8baed020afeda57d2e292a7f37e3c9a97ceaf524 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Fri, 7 Feb 2014 10:59:23 +0100
Subject: [PATCH] Allow escaping of option values for options passed at
 connection start.

This is primarily useful because it allows to set a per-connection
default_transaction_isolation value of 'repeatable read' which
previously was not possible, but other usecases like seach_path do
also exist.
---
 src/backend/postmaster/postmaster.c |  3 +--
 src/backend/utils/init/postinit.c   | 51 +++++++++++++++++++++++++++----------
 2 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 5bc5213..7619fb5 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4065,8 +4065,7 @@ BackendRun(Port *port)
 
 	/*
 	 * Pass any backend switches specified with -o on the postmaster's own
-	 * command line.  We assume these are secure.  (It's OK to mangle
-	 * ExtraOptions now, since we're safely inside a subprocess.)
+	 * command line.  We assume these are secure.
 	 */
 	pg_split_opts(av, &ac, ExtraOptions);
 
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 2a57ed3..6f25777 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -407,31 +407,55 @@ InitCommunication(void)
 /*
  * pg_split_opts -- split a string of options and append it to an argv array
  *
- * NB: the input string is destructively modified!	Also, caller is responsible
- * for ensuring the argv array is large enough.  The maximum possible number
- * of arguments added by this routine is (strlen(optstr) + 1) / 2.
+ * The caller is responsible for ensuring the argv array is large enough.  The
+ * maximum possible number of arguments added by this routine is
+ * (strlen(optstr) + 1) / 2.
  *
- * Since no current POSTGRES arguments require any quoting characters,
- * we can use the simple-minded tactic of assuming each set of space-
- * delimited characters is a separate argv element.
- *
- * If you don't like that, well, we *used* to pass the whole option string
- * as ONE argument to execl(), which was even less intelligent...
+ * Because some option values can contain spaces we allow escaping using
+ * backslashes, with a \\ representing a literal backslash.
  */
 void
 pg_split_opts(char **argv, int *argcp, char *optstr)
 {
+	StringInfoData s;
+
+	initStringInfo(&s);
+
 	while (*optstr)
 	{
+		bool last_was_escape = false;
+
+		resetStringInfo(&s);
+
+		/* skip over leading space */
 		while (isspace((unsigned char) *optstr))
 			optstr++;
+
 		if (*optstr == '\0')
 			break;
-		argv[(*argcp)++] = optstr;
-		while (*optstr && !isspace((unsigned char) *optstr))
+
+		/*
+		 * Parse a single option + value, stopping at the first space, unless
+		 * it's escaped.
+		 */
+		while (*optstr)
+		{
+			if (isspace(*optstr) && !last_was_escape)
+				break;
+
+			if (!last_was_escape && *optstr == '\\')
+				last_was_escape = true;
+			else
+			{
+				last_was_escape = false;
+				appendStringInfoChar(&s, *optstr);
+			}
+
 			optstr++;
-		if (*optstr)
-			*optstr++ = '\0';
+		}
+
+		/* now store the option */
+		argv[(*argcp)++] = pstrdup(s.data);
 	}
 }
 
@@ -979,7 +1003,6 @@ process_startup_options(Port *port, bool am_superuser)
 
 		av[ac++] = "postgres";
 
-		/* Note this mangles port->cmdline_options */
 		pg_split_opts(av, &ac, port->cmdline_options);
 
 		av[ac] = NULL;
-- 
1.8.3.251.g1462b67

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

Reply via email to