Date: Sat, 19 Nov 2016 18:36:03 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Daniel Verite <daniel@manitou-mail.org>
Cc: Rahila Syed <rahilasyed90@gmail.com>,
	Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Improvements in psql hooks for variables
Message-ID: <20161119233603.GC13284@tamriel.snowman.net>
References: 
 <CAH2L28tsJWuYtcbqgWYmdeuXshiPFta6S1OxOx5_O62Xss8r6g@mail.gmail.com>
 <b0c61324-8ad3-413e-bb88-5c71d6412bba@mm>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="1E8AXipGA1cDb+SH"
Content-Disposition: inline
In-Reply-To: <b0c61324-8ad3-413e-bb88-5c71d6412bba@mm>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--1E8AXipGA1cDb+SH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Daniel,

* Daniel Verite (daniel@manitou-mail.org) wrote:
> I'm attaching v3 of the patch with error reporting for
> FETCH_COUNT as mentioned upthread, and rebased
> on the most recent master.

Just fyi, there seems to be some issues with this patch because setting
my PROMPT1 and PROMPT2 variables result in rather odd behavior.

Here's what I use:

-------------
\set PROMPT1 '\033[33;1m%M(from '`hostname`').%/.%n.%> [%`date`]\033[0m\n=%x%# '
\set PROMPT2 '-%x%# '
-------------

In reviewing this patch, I also noticed that it's set up to assume a
'true' result when a variable can't be parsed by ParseVariableBool.

-----------
postgres=# \timing off
Timing is off.
postgres=# \timing asdsa
unrecognized value "asdsa" for "\timing": boolean expected
Timing is on.
-----------

That certainly doesn't feel right.  I'm thinking that if we're going to
throw an error back to the user about a value being invalid then we
shouldn't change the current value.

My initial thought was that perhaps we should pass the current value to
ParseVariableBool() and let it return whatever the "right" answer is,
however, your use of ParseVariableBool() for enums that happen to accept
on/off means that won't really work.

Perhaps the right answer is to flip this around a bit and treat booleans
as a special case of enums and have a generic solution for enums.
Consider something like:

ParseVariableEnum(valid_enums, str_value, name, curr_value);

'valid_enums' would be a simple list of what the valid values are for a
given variable and their corresponding value, str_value the string the
user typed, name the name of the variable, and curr_value the current
value of the variable.

ParseVariableEnum() could then detect if the string passed in is valid
or not and report to the user if it's incorrect and leave the existing
value alone.  This could also generically handle the question of if the
string passed in is a unique prefix of a correct value by comparing it
to all of the valid values and seeing if there's a unique match or not.

Thoughts?

Thanks!

Stephen

--1E8AXipGA1cDb+SH
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJYMOHiAAoJEO1sijiDR2RViJAP/j5QGq2w48g0a2cGt8ldt7Bg
o69lPGUGcOAibGENcZZLK+JJCBx/m9iUbIgUbIFAFo/NnhBqZa/S+5lyHDmWUsIk
A/D/0898UfJJ3ZGfVtQ51pvKj1DBV6cgmnjpD/2KYYrFlNRDmLkjF1KXO/E1vQvJ
0zXROKTQTqsjdlxvZJa3SiBfZFq61Djd88IJOzcN47rbpWD4FE+xVgRhidZeEcm9
8YFIk9X2OgxuLX9skj5aZzQMMy5vbx9kB4s7G3PAg4w9JRy4Zl7u9gUERwB2pxJm
soD86havb8I9qlFaoywLTvfxfH9+8KH2Rw9/ZHCl6Zvf6e9zD7xZ8M+cH+IaM7mD
j7MlcvOrP/faTbgeROjtHNdRJri3wpvlvGUa4ry8QawRzkxk/P4f4MwDgnxylpFA
VIwTvKWwGVw0pFn8UiysuxVpDRturhVgnF5REyeULvqbecFabBffg9ZlUH55/+q7
0XN2lGLQ4oP4Qz5ulHYG1a5+l5G1h/K9xT+Fezre1FMWzhHgsuorJiPyctrRQIGh
h/Nrc/g6Qnb/uFbe3wfVoAWLf+toJp+mpLg+IkP1ig2CSlJCozcQMnuNul9XKZyj
OENoGLQXHtdXry1teSDoAootie101/oBP2tMr+em1eQA9fkP68XubGeoWMZBPPzx
ldlY2KmDCEoKWfKUrbA7
=soBd
-----END PGP SIGNATURE-----

--1E8AXipGA1cDb+SH--