Re: [HACKERS] specifying repeatable read in PGOPTIONS
On Fri, Feb 7, 2014 at 5:06 AM, Andres Freund and...@2ndquadrant.com wrote: 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. Like Tom, I'm -1 on this. This is fixing the problem from the wrong end. But introducing an escaping convention seems more than prudent. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] specifying repeatable read in PGOPTIONS
On 2014-02-09 12:00:02 -0500, Robert Haas wrote: On Fri, Feb 7, 2014 at 5:06 AM, Andres Freund and...@2ndquadrant.com wrote: So I still think we should add read_committed, repeatable_read as aliases. Like Tom, I'm -1 on this. This is fixing the problem from the wrong end. Why? We do have other options with aliases for option values and all other enum option has taken care not to need spaces. But introducing an escaping convention seems more than prudent. I've attached a patch implementing \ escapes one mail up... But I don't really see that as prohibiting also adding sensible aliases. It's just annoying that it's currently not possible to run to pgbenches testing both without either restarting the user or ALTER ROLE/DATABASE. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] specifying repeatable read in PGOPTIONS
On Sun, Feb 9, 2014 at 12:10 PM, Andres Freund and...@2ndquadrant.com wrote: On 2014-02-09 12:00:02 -0500, Robert Haas wrote: On Fri, Feb 7, 2014 at 5:06 AM, Andres Freund and...@2ndquadrant.com wrote: So I still think we should add read_committed, repeatable_read as aliases. Like Tom, I'm -1 on this. This is fixing the problem from the wrong end. Why? We do have other options with aliases for option values and all other enum option has taken care not to need spaces. I think that's probably mostly a happy coincidence; I'm not committed to a policy of ensuring that all GUCs can be set to whatever value you want without using the space character. Besides, what's so special about enum GUCs? There can certainly be spaces in string-valued GUCs, and you're not going to be able to get around the problem there with one-off kludges. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] specifying repeatable read in PGOPTIONS
Robert Haas robertmh...@gmail.com writes: On Sun, Feb 9, 2014 at 12:10 PM, Andres Freund and...@2ndquadrant.com wrote: Why? We do have other options with aliases for option values and all other enum option has taken care not to need spaces. I think that's probably mostly a happy coincidence; I'm not committed to a policy of ensuring that all GUCs can be set to whatever value you want without using the space character. Besides, what's so special about enum GUCs? There can certainly be spaces in string-valued GUCs, and you're not going to be able to get around the problem there with one-off kludges. Pathname GUCs can have spaces in them (that's even pretty common, on certain platforms). Other GUCs contain SQL identifiers, which can legally have spaces in them too. So really this is a mechanism deficiency, not something we should work around by instituting a policy against spaces in GUC values. 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] specifying repeatable read in PGOPTIONS
On 2014-02-09 12:38:18 -0500, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Sun, Feb 9, 2014 at 12:10 PM, Andres Freund and...@2ndquadrant.com wrote: Why? We do have other options with aliases for option values and all other enum option has taken care not to need spaces. I think that's probably mostly a happy coincidence; I'm not committed to a policy of ensuring that all GUCs can be set to whatever value you want without using the space character. Besides, what's so special about enum GUCs? There can certainly be spaces in string-valued GUCs, and you're not going to be able to get around the problem there with one-off kludges. Pathname GUCs can have spaces in them (that's even pretty common, on certain platforms). Other GUCs contain SQL identifiers, which can legally have spaces in them too. But pretty much all of those GUCs are either PGC_SIGHUP or PGC_POSTMASTER and thus cannot be set via PGOPTIONS anyway. The two exceptions are application_name (which can in many cases not set because libpq overrides it with a SET) and search_path. Anybody setting the latter to schemas containing spaces deserves having to escape values. So really this is a mechanism deficiency, not something we should work around by instituting a policy against spaces in GUC values. Please note, I am absolutely *not* against such a mechanism, that's why I submitted a patch implementing one. But unneccearily requiring escaping still annoys me. We imo should add the escaping mechanism to master and backpatch the aliases (read_committed, repeatable_read). There's already not enough benchmarking during beta/rc, we shouldn't make it unneccesarily hard. And there's sufficient reason to benchmark the difference between isolation modes, with mvcc catalog snapshots and such. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] specifying repeatable read in PGOPTIONS
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 +
Re: [HACKERS] specifying repeatable read in PGOPTIONS
Andres Freund and...@2ndquadrant.com writes: PGOPTIONS='-c default_transaction_isolation=serializable' \ psql ... -c SHOW default_transaction_isolation works well enough, but PGOPTIONS='-c default_transaction_isolation=repeatable read' \ psql ... -c SHOW default_transaction_isolation doesn't, because of the whitespace. I couldn't come up with any adequate quoting. I'd like to propose adding aliases with dashes instead of spaces to the isolation_level_options array? I'd even like to backport it, because it makes benchmarking across versions unneccessarily hard. -1. This is not a general solution to the problem. There are other GUCs for which people might want spaces in the value. Additionally we might want to think about a bit better quoting support for such options? 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. 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] specifying repeatable read in PGOPTIONS
On 2014-02-04 11:36:22 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: PGOPTIONS='-c default_transaction_isolation=serializable' \ psql ... -c SHOW default_transaction_isolation works well enough, but PGOPTIONS='-c default_transaction_isolation=repeatable read' \ psql ... -c SHOW default_transaction_isolation doesn't, because of the whitespace. I couldn't come up with any adequate quoting. I'd like to propose adding aliases with dashes instead of spaces to the isolation_level_options array? I'd even like to backport it, because it makes benchmarking across versions unneccessarily hard. -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. Sure, you can generate a search_path that does so, but that's just asking for problems. Most other GUCs that can contain spaces are PGC_SIGHUP/POSTMASTER. And having to use quoting just makes it awkward to use from shell. Since all the other option values try to take not to force using spaces, I see little reason not to do so here as well. Additionally we might want to think about a bit better quoting support for such options? 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 \? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] specifying repeatable read in PGOPTIONS
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.) 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. 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
