Thread: Re: [GENERAL] ON_ERROR_ROLLBACK
Adrian Klaver <adrian.klaver@aklaver.com> writes:
> So it seems you can turn ON_ERROR_ROLLBACK on with either 1 or 'on', but you can only turn it off with 'off'.
> With ON_ERROR_STOP 1/on and 0/off both seem to work.
> Is this expected?
on_error_stop_hook() uses ParseVariableBool, while
on_error_rollback_hook() uses some hard-coded logic that checks for
"interactive" and "off" and otherwise assumes "on". This is inconsistent
first in not accepting as many spellings as ParseVariableBool, and second
in not complaining about invalid input --- ParseVariableBool does, so
why not here?
echo_hook, echo_hidden_hook, histcontrol_hook, and verbosity_hook are
equally cavalierly written.
For on_error_rollback_hook and echo_hidden_hook, where "on" and "off"
are documented values, I think it would make sense to allow all spellings
accepted by ParseVariableBool, say like this:
if (newval == NULL)
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
else if (pg_strcasecmp(newval, "interactive") == 0)
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_INTERACTIVE;
- else if (pg_strcasecmp(newval, "off") == 0)
- pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
- else
- pset.on_error_rollback = PSQL_ERROR_ROLLBACK_ON;
+ else if (ParseVariableBool(newval))
+ pset.on_error_rollback = PSQL_ERROR_ROLLBACK_ON;
+ else
+ pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
The other 3 functions don't need to accept on/off I think, but they should
print a warning for unrecognized input like ParseVariableBool does.
And while we're at it, we should consistently allow case-insensitive
input; right now it looks like somebody rolled dice to decide whether
to use "strcmp" or "pg_strcasecmp" in these functions. Per line, not
even per function.
Given the lack of previous complaints, this probably isn't backpatching
material, but it sure seems like a bit of attention to consistency
would be warranted here.
regards, tom lane
On 12/29/2014 09:55 AM, Tom Lane wrote: > Adrian Klaver <adrian.klaver@aklaver.com> writes: >> So it seems you can turn ON_ERROR_ROLLBACK on with either 1 or 'on', but you can only turn it off with 'off'. >> With ON_ERROR_STOP 1/on and 0/off both seem to work. > >> Is this expected? > > > Given the lack of previous complaints, this probably isn't backpatching > material, but it sure seems like a bit of attention to consistency > would be warranted here. I would appreciate it, thanks. > > regards, tom lane > -- Adrian Klaver adrian.klaver@aklaver.com
--On 29. Dezember 2014 12:55:11 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Given the lack of previous complaints, this probably isn't backpatching
> material, but it sure seems like a bit of attention to consistency
> would be warranted here.
Now that i read it i remember a client complaining about this some time
ago. I forgot about it, but i think there's value in it to backpatch.
--
Thanks
Bernd
Bernd Helmle <mailings@oopsware.de> writes:
> --On 29. Dezember 2014 12:55:11 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Given the lack of previous complaints, this probably isn't backpatching
>> material, but it sure seems like a bit of attention to consistency
>> would be warranted here.
> Now that i read it i remember a client complaining about this some time
> ago. I forgot about it, but i think there's value in it to backpatch.
Hm. Last night I wrote the attached draft patch, which I was intending
to apply to HEAD only. The argument against back-patching is basically
that this might change the interpretation of scripts that had been
accepted silently before. For example
\set ECHO_HIDDEN NoExec
will now select "noexec" mode whereas before you silently got "on" mode.
In one light this is certainly a bug fix, but in another it's just
definitional instability.
If we'd gotten a field bug report we might well have chosen to back-patch,
though, and perhaps your client's complaint counts as that.
Opinions anyone?
regards, tom lane
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d29dfa2..bdfb67c 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*************** EOF
*** 173,180 ****
Echo the actual queries generated by <command>\d</command> and other backslash
commands. You can use this to study <application>psql</application>'s
internal operations. This is equivalent to
! setting the variable <varname>ECHO_HIDDEN</varname> from within
! <application>psql</application>.
</para>
</listitem>
</varlistentry>
--- 173,179 ----
Echo the actual queries generated by <command>\d</command> and other backslash
commands. You can use this to study <application>psql</application>'s
internal operations. This is equivalent to
! setting the variable <varname>ECHO_HIDDEN</varname> to <literal>on</>.
</para>
</listitem>
</varlistentry>
*************** EOF
*** 333,340 ****
quietly. By default, it prints welcome messages and various
informational output. If this option is used, none of this
happens. This is useful with the <option>-c</option> option.
! Within <application>psql</application> you can also set the
! <varname>QUIET</varname> variable to achieve the same effect.
</para>
</listitem>
</varlistentry>
--- 332,339 ----
quietly. By default, it prints welcome messages and various
informational output. If this option is used, none of this
happens. This is useful with the <option>-c</option> option.
! This is equivalent to setting the variable <varname>QUIET</varname>
! to <literal>on</>.
</para>
</listitem>
</varlistentry>
*************** bar
*** 2884,2891 ****
<term><varname>ECHO_HIDDEN</varname></term>
<listitem>
<para>
! When this variable is set and a backslash command queries the
! database, the query is first shown. This way you can study the
<productname>PostgreSQL</productname> internals and provide
similar functionality in your own programs. (To select this behavior
on program start-up, use the switch <option>-E</option>.) If you set
--- 2883,2891 ----
<term><varname>ECHO_HIDDEN</varname></term>
<listitem>
<para>
! When this variable is set to <literal>on</> and a backslash command
! queries the database, the query is first shown.
! This feature helps you to study
<productname>PostgreSQL</productname> internals and provide
similar functionality in your own programs. (To select this behavior
on program start-up, use the switch <option>-E</option>.) If you set
*************** bar
*** 3046,3061 ****
</term>
<listitem>
<para>
! When <literal>on</>, if a statement in a transaction block
generates an error, the error is ignored and the transaction
! continues. When <literal>interactive</>, such errors are only
ignored in interactive sessions, and not when reading script
! files. When <literal>off</> (the default), a statement in a
transaction block that generates an error aborts the entire
! transaction. The on_error_rollback-on mode works by issuing an
implicit <command>SAVEPOINT</> for you, just before each command
! that is in a transaction block, and rolls back to the savepoint
! on error.
</para>
</listitem>
</varlistentry>
--- 3046,3061 ----
</term>
<listitem>
<para>
! When set to <literal>on</>, if a statement in a transaction block
generates an error, the error is ignored and the transaction
! continues. When set to <literal>interactive</>, such errors are only
ignored in interactive sessions, and not when reading script
! files. When unset or set to <literal>off</>, a statement in a
transaction block that generates an error aborts the entire
! transaction. The error rollback mode works by issuing an
implicit <command>SAVEPOINT</> for you, just before each command
! that is in a transaction block, and then rolling back to the
! savepoint if the command fails.
</para>
</listitem>
</varlistentry>
*************** bar
*** 3065,3071 ****
<listitem>
<para>
By default, command processing continues after an error. When this
! variable is set, it will instead stop immediately. In interactive mode,
<application>psql</application> will return to the command prompt;
otherwise, <application>psql</application> will exit, returning
error code 3 to distinguish this case from fatal error
--- 3065,3072 ----
<listitem>
<para>
By default, command processing continues after an error. When this
! variable is set to <literal>on</>, processing will instead stop
! immediately. In interactive mode,
<application>psql</application> will return to the command prompt;
otherwise, <application>psql</application> will exit, returning
error code 3 to distinguish this case from fatal error
*************** bar
*** 3107,3114 ****
<term><varname>QUIET</varname></term>
<listitem>
<para>
! This variable is equivalent to the command line option
! <option>-q</option>. It is probably not too useful in
interactive mode.
</para>
</listitem>
--- 3108,3115 ----
<term><varname>QUIET</varname></term>
<listitem>
<para>
! Setting this variable to <literal>on</> is equivalent to the command
! line option <option>-q</option>. It is probably not too useful in
interactive mode.
</para>
</listitem>
*************** bar
*** 3118,3125 ****
<term><varname>SINGLELINE</varname></term>
<listitem>
<para>
! This variable is equivalent to the command line option
! <option>-S</option>.
</para>
</listitem>
</varlistentry>
--- 3119,3126 ----
<term><varname>SINGLELINE</varname></term>
<listitem>
<para>
! Setting this variable to <literal>on</> is equivalent to the command
! line option <option>-S</option>.
</para>
</listitem>
</varlistentry>
*************** bar
*** 3128,3135 ****
<term><varname>SINGLESTEP</varname></term>
<listitem>
<para>
! This variable is equivalent to the command line option
! <option>-s</option>.
</para>
</listitem>
</varlistentry>
--- 3129,3136 ----
<term><varname>SINGLESTEP</varname></term>
<listitem>
<para>
! Setting this variable to <literal>on</> is equivalent to the command
! line option <option>-s</option>.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index c7a17d7..504cc0d 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*************** exec_command(const char *cmd,
*** 1362,1368 ****
OT_NORMAL, NULL, false);
if (opt)
! pset.timing = ParseVariableBool(opt);
else
pset.timing = !pset.timing;
if (!pset.quiet)
--- 1362,1368 ----
OT_NORMAL, NULL, false);
if (opt)
! pset.timing = ParseVariableBool(opt, "\\timing");
else
pset.timing = !pset.timing;
if (!pset.quiet)
*************** do_pset(const char *param, const char *v
*** 2412,2423 ****
}
/* set expanded/vertical mode */
! else if (strcmp(param, "x") == 0 || strcmp(param, "expanded") == 0 || strcmp(param, "vertical") == 0)
{
if (value && pg_strcasecmp(value, "auto") == 0)
popt->topt.expanded = 2;
else if (value)
! popt->topt.expanded = ParseVariableBool(value);
else
popt->topt.expanded = !popt->topt.expanded;
}
--- 2412,2425 ----
}
/* set expanded/vertical mode */
! else if (strcmp(param, "x") == 0 ||
! strcmp(param, "expanded") == 0 ||
! strcmp(param, "vertical") == 0)
{
if (value && pg_strcasecmp(value, "auto") == 0)
popt->topt.expanded = 2;
else if (value)
! popt->topt.expanded = ParseVariableBool(value, param);
else
popt->topt.expanded = !popt->topt.expanded;
}
*************** do_pset(const char *param, const char *v
*** 2426,2432 ****
else if (strcmp(param, "numericlocale") == 0)
{
if (value)
! popt->topt.numericLocale = ParseVariableBool(value);
else
popt->topt.numericLocale = !popt->topt.numericLocale;
}
--- 2428,2434 ----
else if (strcmp(param, "numericlocale") == 0)
{
if (value)
! popt->topt.numericLocale = ParseVariableBool(value, param);
else
popt->topt.numericLocale = !popt->topt.numericLocale;
}
*************** do_pset(const char *param, const char *v
*** 2481,2487 ****
else if (strcmp(param, "t") == 0 || strcmp(param, "tuples_only") == 0)
{
if (value)
! popt->topt.tuples_only = ParseVariableBool(value);
else
popt->topt.tuples_only = !popt->topt.tuples_only;
}
--- 2483,2489 ----
else if (strcmp(param, "t") == 0 || strcmp(param, "tuples_only") == 0)
{
if (value)
! popt->topt.tuples_only = ParseVariableBool(value, param);
else
popt->topt.tuples_only = !popt->topt.tuples_only;
}
*************** do_pset(const char *param, const char *v
*** 2512,2521 ****
if (value && pg_strcasecmp(value, "always") == 0)
popt->topt.pager = 2;
else if (value)
! if (ParseVariableBool(value))
popt->topt.pager = 1;
else
popt->topt.pager = 0;
else if (popt->topt.pager == 1)
popt->topt.pager = 0;
else
--- 2514,2525 ----
if (value && pg_strcasecmp(value, "always") == 0)
popt->topt.pager = 2;
else if (value)
! {
! if (ParseVariableBool(value, param))
popt->topt.pager = 1;
else
popt->topt.pager = 0;
+ }
else if (popt->topt.pager == 1)
popt->topt.pager = 0;
else
*************** do_pset(const char *param, const char *v
*** 2526,2532 ****
else if (strcmp(param, "footer") == 0)
{
if (value)
! popt->topt.default_footer = ParseVariableBool(value);
else
popt->topt.default_footer = !popt->topt.default_footer;
}
--- 2530,2536 ----
else if (strcmp(param, "footer") == 0)
{
if (value)
! popt->topt.default_footer = ParseVariableBool(value, param);
else
popt->topt.default_footer = !popt->topt.default_footer;
}
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index ef24a4e..bad70e4 100644
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
***************
*** 27,32 ****
--- 27,37 ----
#define DEFAULT_PROMPT2 "%/%R%# "
#define DEFAULT_PROMPT3 ">> "
+ /*
+ * Note: these enums should generally be chosen so that zero corresponds
+ * to the default behavior.
+ */
+
typedef enum
{
PSQL_ECHO_NONE,
*************** typedef enum
*** 51,56 ****
--- 56,69 ----
typedef enum
{
+ PSQL_COMP_CASE_PRESERVE_UPPER,
+ PSQL_COMP_CASE_PRESERVE_LOWER,
+ PSQL_COMP_CASE_UPPER,
+ PSQL_COMP_CASE_LOWER
+ } PSQL_COMP_CASE;
+
+ typedef enum
+ {
hctl_none = 0,
hctl_ignorespace = 1,
hctl_ignoredups = 2,
*************** typedef struct _psqlSettings
*** 110,115 ****
--- 123,129 ----
PSQL_ECHO echo;
PSQL_ECHO_HIDDEN echo_hidden;
PSQL_ERROR_ROLLBACK on_error_rollback;
+ PSQL_COMP_CASE comp_case;
HistControl histcontrol;
const char *prompt1;
const char *prompt2;
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 11a159a..a23ebce 100644
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** showVersion(void)
*** 705,735 ****
static void
autocommit_hook(const char *newval)
{
! pset.autocommit = ParseVariableBool(newval);
}
static void
on_error_stop_hook(const char *newval)
{
! pset.on_error_stop = ParseVariableBool(newval);
}
static void
quiet_hook(const char *newval)
{
! pset.quiet = ParseVariableBool(newval);
}
static void
singleline_hook(const char *newval)
{
! pset.singleline = ParseVariableBool(newval);
}
static void
singlestep_hook(const char *newval)
{
! pset.singlestep = ParseVariableBool(newval);
}
static void
--- 705,735 ----
static void
autocommit_hook(const char *newval)
{
! pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");
}
static void
on_error_stop_hook(const char *newval)
{
! pset.on_error_stop = ParseVariableBool(newval, "ON_ERROR_STOP");
}
static void
quiet_hook(const char *newval)
{
! pset.quiet = ParseVariableBool(newval, "QUIET");
}
static void
singleline_hook(const char *newval)
{
! pset.singleline = ParseVariableBool(newval, "SINGLELINE");
}
static void
singlestep_hook(const char *newval)
{
! pset.singlestep = ParseVariableBool(newval, "SINGLESTEP");
}
static void
*************** echo_hook(const char *newval)
*** 743,756 ****
{
if (newval == NULL)
pset.echo = PSQL_ECHO_NONE;
! else if (strcmp(newval, "queries") == 0)
pset.echo = PSQL_ECHO_QUERIES;
! else if (strcmp(newval, "errors") == 0)
pset.echo = PSQL_ECHO_ERRORS;
! else if (strcmp(newval, "all") == 0)
pset.echo = PSQL_ECHO_ALL;
else
pset.echo = PSQL_ECHO_NONE;
}
static void
--- 743,762 ----
{
if (newval == NULL)
pset.echo = PSQL_ECHO_NONE;
! else if (pg_strcasecmp(newval, "queries") == 0)
pset.echo = PSQL_ECHO_QUERIES;
! else if (pg_strcasecmp(newval, "errors") == 0)
pset.echo = PSQL_ECHO_ERRORS;
! else if (pg_strcasecmp(newval, "all") == 0)
pset.echo = PSQL_ECHO_ALL;
+ else if (pg_strcasecmp(newval, "none") == 0)
+ pset.echo = PSQL_ECHO_NONE;
else
+ {
+ psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
+ newval, "ECHO", "none");
pset.echo = PSQL_ECHO_NONE;
+ }
}
static void
*************** echo_hidden_hook(const char *newval)
*** 758,769 ****
{
if (newval == NULL)
pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
! else if (strcmp(newval, "noexec") == 0)
pset.echo_hidden = PSQL_ECHO_HIDDEN_NOEXEC;
! else if (pg_strcasecmp(newval, "off") == 0)
! pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
! else
pset.echo_hidden = PSQL_ECHO_HIDDEN_ON;
}
static void
--- 764,775 ----
{
if (newval == NULL)
pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
! else if (pg_strcasecmp(newval, "noexec") == 0)
pset.echo_hidden = PSQL_ECHO_HIDDEN_NOEXEC;
! else if (ParseVariableBool(newval, "ECHO_HIDDEN"))
pset.echo_hidden = PSQL_ECHO_HIDDEN_ON;
+ else /* ParseVariableBool printed msg if needed */
+ pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
}
static void
*************** on_error_rollback_hook(const char *newva
*** 773,782 ****
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
else if (pg_strcasecmp(newval, "interactive") == 0)
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_INTERACTIVE;
! else if (pg_strcasecmp(newval, "off") == 0)
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
else
! pset.on_error_rollback = PSQL_ERROR_ROLLBACK_ON;
}
static void
--- 779,809 ----
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
else if (pg_strcasecmp(newval, "interactive") == 0)
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_INTERACTIVE;
! else if (ParseVariableBool(newval, "ON_ERROR_ROLLBACK"))
! pset.on_error_rollback = PSQL_ERROR_ROLLBACK_ON;
! else /* ParseVariableBool printed msg if needed */
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
+ }
+
+ static void
+ comp_keyword_case_hook(const char *newval)
+ {
+ if (newval == NULL)
+ pset.comp_case = PSQL_COMP_CASE_PRESERVE_UPPER;
+ else if (pg_strcasecmp(newval, "preserve-upper") == 0)
+ pset.comp_case = PSQL_COMP_CASE_PRESERVE_UPPER;
+ else if (pg_strcasecmp(newval, "preserve-lower") == 0)
+ pset.comp_case = PSQL_COMP_CASE_PRESERVE_LOWER;
+ else if (pg_strcasecmp(newval, "upper") == 0)
+ pset.comp_case = PSQL_COMP_CASE_UPPER;
+ else if (pg_strcasecmp(newval, "lower") == 0)
+ pset.comp_case = PSQL_COMP_CASE_LOWER;
else
! {
! psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
! newval, "COMP_KEYWORD_CASE", "preserve-upper");
! pset.comp_case = PSQL_COMP_CASE_PRESERVE_UPPER;
! }
}
static void
*************** histcontrol_hook(const char *newval)
*** 784,797 ****
{
if (newval == NULL)
pset.histcontrol = hctl_none;
! else if (strcmp(newval, "ignorespace") == 0)
pset.histcontrol = hctl_ignorespace;
! else if (strcmp(newval, "ignoredups") == 0)
pset.histcontrol = hctl_ignoredups;
! else if (strcmp(newval, "ignoreboth") == 0)
pset.histcontrol = hctl_ignoreboth;
else
pset.histcontrol = hctl_none;
}
static void
--- 811,830 ----
{
if (newval == NULL)
pset.histcontrol = hctl_none;
! else if (pg_strcasecmp(newval, "ignorespace") == 0)
pset.histcontrol = hctl_ignorespace;
! else if (pg_strcasecmp(newval, "ignoredups") == 0)
pset.histcontrol = hctl_ignoredups;
! else if (pg_strcasecmp(newval, "ignoreboth") == 0)
pset.histcontrol = hctl_ignoreboth;
+ else if (pg_strcasecmp(newval, "none") == 0)
+ pset.histcontrol = hctl_none;
else
+ {
+ psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
+ newval, "HISTCONTROL", "none");
pset.histcontrol = hctl_none;
+ }
}
static void
*************** verbosity_hook(const char *newval)
*** 817,830 ****
{
if (newval == NULL)
pset.verbosity = PQERRORS_DEFAULT;
! else if (strcmp(newval, "default") == 0)
pset.verbosity = PQERRORS_DEFAULT;
! else if (strcmp(newval, "terse") == 0)
pset.verbosity = PQERRORS_TERSE;
! else if (strcmp(newval, "verbose") == 0)
pset.verbosity = PQERRORS_VERBOSE;
else
pset.verbosity = PQERRORS_DEFAULT;
if (pset.db)
PQsetErrorVerbosity(pset.db, pset.verbosity);
--- 850,867 ----
{
if (newval == NULL)
pset.verbosity = PQERRORS_DEFAULT;
! else if (pg_strcasecmp(newval, "default") == 0)
pset.verbosity = PQERRORS_DEFAULT;
! else if (pg_strcasecmp(newval, "terse") == 0)
pset.verbosity = PQERRORS_TERSE;
! else if (pg_strcasecmp(newval, "verbose") == 0)
pset.verbosity = PQERRORS_VERBOSE;
else
+ {
+ psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
+ newval, "VERBOSITY", "default");
pset.verbosity = PQERRORS_DEFAULT;
+ }
if (pset.db)
PQsetErrorVerbosity(pset.db, pset.verbosity);
*************** EstablishVariableSpace(void)
*** 845,850 ****
--- 882,888 ----
SetVariableAssignHook(pset.vars, "ECHO", echo_hook);
SetVariableAssignHook(pset.vars, "ECHO_HIDDEN", echo_hidden_hook);
SetVariableAssignHook(pset.vars, "ON_ERROR_ROLLBACK", on_error_rollback_hook);
+ SetVariableAssignHook(pset.vars, "COMP_KEYWORD_CASE", comp_keyword_case_hook);
SetVariableAssignHook(pset.vars, "HISTCONTROL", histcontrol_hook);
SetVariableAssignHook(pset.vars, "PROMPT1", prompt1_hook);
SetVariableAssignHook(pset.vars, "PROMPT2", prompt2_hook);
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 82c926d..ba78f7e 100644
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*************** complete_from_files(const char *text, in
*** 4452,4458 ****
/*
* Make a pg_strdup copy of s and convert the case according to
! * COMP_KEYWORD_CASE variable, using ref as the text that was already entered.
*/
static char *
pg_strdup_keyword_case(const char *s, const char *ref)
--- 4452,4458 ----
/*
* Make a pg_strdup copy of s and convert the case according to
! * COMP_KEYWORD_CASE setting, using ref as the text that was already entered.
*/
static char *
pg_strdup_keyword_case(const char *s, const char *ref)
*************** pg_strdup_keyword_case(const char *s, co
*** 4460,4497 ****
char *ret,
*p;
unsigned char first = ref[0];
- int tocase;
- const char *varval;
-
- varval = GetVariable(pset.vars, "COMP_KEYWORD_CASE");
- if (!varval)
- tocase = 0;
- else if (strcmp(varval, "lower") == 0)
- tocase = -2;
- else if (strcmp(varval, "preserve-lower") == 0)
- tocase = -1;
- else if (strcmp(varval, "preserve-upper") == 0)
- tocase = +1;
- else if (strcmp(varval, "upper") == 0)
- tocase = +2;
- else
- tocase = 0;
-
- /* default */
- if (tocase == 0)
- tocase = +1;
ret = pg_strdup(s);
! if (tocase == -2
! || ((tocase == -1 || tocase == +1) && islower(first))
! || (tocase == -1 && !isalpha(first))
! )
for (p = ret; *p; p++)
*p = pg_tolower((unsigned char) *p);
else
for (p = ret; *p; p++)
*p = pg_toupper((unsigned char) *p);
return ret;
}
--- 4460,4481 ----
char *ret,
*p;
unsigned char first = ref[0];
ret = pg_strdup(s);
! if (pset.comp_case == PSQL_COMP_CASE_LOWER ||
! ((pset.comp_case == PSQL_COMP_CASE_PRESERVE_LOWER ||
! pset.comp_case == PSQL_COMP_CASE_PRESERVE_UPPER) && islower(first)) ||
! (pset.comp_case == PSQL_COMP_CASE_PRESERVE_LOWER && !isalpha(first)))
! {
for (p = ret; *p; p++)
*p = pg_tolower((unsigned char) *p);
+ }
else
+ {
for (p = ret; *p; p++)
*p = pg_toupper((unsigned char) *p);
+ }
return ret;
}
diff --git a/src/bin/psql/variables.c b/src/bin/psql/variables.c
index 1f871d7..29c50f9 100644
*** a/src/bin/psql/variables.c
--- b/src/bin/psql/variables.c
*************** GetVariable(VariableSpace space, const c
*** 79,89 ****
}
/*
! * Try to interpret value as boolean value. Valid values are: true,
! * false, yes, no, on, off, 1, 0; as well as unique prefixes thereof.
*/
bool
! ParseVariableBool(const char *value)
{
size_t len;
--- 79,94 ----
}
/*
! * Try to interpret "value" as boolean value.
! *
! * Valid values are: true, false, yes, no, on, off, 1, 0; as well as unique
! * prefixes thereof.
! *
! * "name" is the name of the variable we're assigning to, to use in error
! * report if any. Pass name == NULL to suppress the error report.
*/
bool
! ParseVariableBool(const char *value, const char *name)
{
size_t len;
*************** ParseVariableBool(const char *value)
*** 112,118 ****
else
{
/* NULL is treated as false, so a non-matching value is 'true' */
! psql_error("unrecognized Boolean value; assuming \"on\"\n");
return true;
}
}
--- 117,125 ----
else
{
/* NULL is treated as false, so a non-matching value is 'true' */
! if (name)
! psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
! value, name, "on");
return true;
}
}
diff --git a/src/bin/psql/variables.h b/src/bin/psql/variables.h
index 5a2b98e..f94f779 100644
*** a/src/bin/psql/variables.h
--- b/src/bin/psql/variables.h
*************** typedef struct _variable *VariableSpace;
*** 35,41 ****
VariableSpace CreateVariableSpace(void);
const char *GetVariable(VariableSpace space, const char *name);
! bool ParseVariableBool(const char *val);
int ParseVariableNum(const char *val,
int defaultval,
int faultval,
--- 35,41 ----
VariableSpace CreateVariableSpace(void);
const char *GetVariable(VariableSpace space, const char *name);
! bool ParseVariableBool(const char *value, const char *name);
int ParseVariableNum(const char *val,
int defaultval,
int faultval,
On 12/30/2014 07:43 AM, David G Johnston wrote:Tom Lane-2 wroteBernd Helmle <mailings@> writes:--On 29. Dezember 2014 12:55:11 -0500 Tom Lane <tgl@.pa> wrote:Given the lack of previous complaints, this probably isn't backpatching
material, but it sure seems like a bit of attention to consistency
would be warranted here.Now that i read it i remember a client complaining about this some time
ago. I forgot about it, but i think there's value in it to backpatch.
Hm. Last night I wrote the attached draft patch, which I was intending
to apply to HEAD only. The argument against back-patching is basically
that this might change the interpretation of scripts that had been
accepted silently before. For example
\set ECHO_HIDDEN NoExec
will now select "noexec" mode whereas before you silently got "on" mode.
In one light this is certainly a bug fix, but in another it's just
definitional instability.
If we'd gotten a field bug report we might well have chosen to back-patch,
though, and perhaps your client's complaint counts as that.
Opinions anyone?
-0.5 for back patching
The one thing supporting this is that we'd potentially be fixing scripts
that are broken but don't know it yet. But the downside of changing active
settings for working scripts - even if they are only accidentally working -
is enough to counter that for me. Being more liberal in our acceptance of
input is more feature than bug fix even if we document that we accept more
items.
It is more about being consistent then liberal. Personally I think a situation where for one variable 0 = off but for another 0 = on, is a bug
I can sorta buy the consistency angle but what will seal it for me is script portability - the ability to write a script and instructions using the most current release and have it run on previous versions without having to worry about this kind of incompatibility.
So, +1 for back patching from me.
David J.
On 12/30/2014 09:20 AM, Tom Lane wrote: > Bernd Helmle <mailings@oopsware.de> writes: >> --On 29. Dezember 2014 12:55:11 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Given the lack of previous complaints, this probably isn't backpatching >>> material, but it sure seems like a bit of attention to consistency >>> would be warranted here. >> Now that i read it i remember a client complaining about this some time >> ago. I forgot about it, but i think there's value in it to backpatch. > Hm. Last night I wrote the attached draft patch, which I was intending > to apply to HEAD only. The argument against back-patching is basically > that this might change the interpretation of scripts that had been > accepted silently before. For example > \set ECHO_HIDDEN NoExec > will now select "noexec" mode whereas before you silently got "on" mode. > In one light this is certainly a bug fix, but in another it's just > definitional instability. > > If we'd gotten a field bug report we might well have chosen to back-patch, > though, and perhaps your client's complaint counts as that. > > Opinions anyone? > > r I got caught by this with ON_ERROR_ROLLBACK on 9.3 just this afternoon before remembering this thread. So there's a field report :-) +0.75 for backpatching (It's hard to imagine someone relying on the bad behaviour, but you never know). cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 12/30/2014 09:20 AM, Tom Lane wrote:
>> In one light this is certainly a bug fix, but in another it's just
>> definitional instability.
>>
>> If we'd gotten a field bug report we might well have chosen to back-patch,
>> though, and perhaps your client's complaint counts as that.
> I got caught by this with ON_ERROR_ROLLBACK on 9.3 just this afternoon
> before remembering this thread. So there's a field report :-)
> +0.75 for backpatching (It's hard to imagine someone relying on the bad
> behaviour, but you never know).
It seems like there's a consensus in favor of back-patching this change,
so I'll go ahead and do that.
regards, tom lane