Date: Mon, 21 Nov 2016 08:41:35 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Daniel Verite <daniel@manitou-mail.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, 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: <20161121134135.GF13284@tamriel.snowman.net>
References: <7464.1479599073@sss.pgh.pa.us>
 <4d4b8d34-6233-4fb4-93dc-7e929808b982@mm>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="MRNWp8VlfMufsMVa"
Content-Disposition: inline
In-Reply-To: <4d4b8d34-6233-4fb4-93dc-7e929808b982@mm>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--MRNWp8VlfMufsMVa
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Daniel,

* Daniel Verite (daniel@manitou-mail.org) wrote:
> 	Tom Lane wrote:
>=20
> > Stephen Frost <sfrost@snowman.net> writes:
> > > 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.
> >=20
> > I suppose that's meant to be backwards-compatible with the current
> > behavior:
> >=20
> > regression=3D# \timing foo
> > unrecognized value "foo" for "\timing"; assuming "on"
> > Timing is on.
>=20
> Exactly. The scope of the patch is limited to the effect
> of \set assignments to built-in variables.
>=20
> > but I agree that if we're changing things in this area, that would
> > be high on my list of things to change.  I think what we want going
> > forward is to disallow setting "special" variables to invalid values,
> > and that should hold both for regular variables that have special
> > behaviors, and for the special-syntax cases like \timing.
>=20
> +1

Not sure I follow your reply here.  There seems to be broad agreement to
improve how we handle both \set and "special" variables and the code
paths are related and this patch is touching them, so it seems like the
correct next step here is to adjust the patch to update the code based
on that agreement.

Are you working to make those changes?  I'd rather we make the changes
to this code once rather than push what you have now only to turn around
and change it significantly again.

Thanks!

Stephen

--MRNWp8VlfMufsMVa
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

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

iQIcBAEBCgAGBQJYMvmPAAoJEO1sijiDR2RVBwUP/1liOq8T05c5whC+68Um7qQt
jaLZ4MdRZuT8eBkMVqCBdvDIG4+zfI3kQV5l3PcQiwWgMA7HPve/7wgM8IbWMoUW
0e3JUsNx0bA8HIxAgPhDCNHW512F0IfLOK6bdB6TW4UoktE7thFTZwitGd/nTaVX
h3ALoOIrvhoCETdTcKj5PU/qQ3ftwrWkF7Eq21fk0O89TKTZ9VwW8qLX0GbQwntd
UMFMjQrJ8SRpJR5s31oTwVRyvmhcZ7+cio/uvrxuTA1TfS0vjkXHkBWcwWHEgndn
VdJLe4hFG66v+OROfwSuRQiTKBG+2dqT5CzrWblYW1OBRc4mOx82Ue9OB9w+djsr
hTz9Fb31HolYRf7O3N7ipqrTrhNbfQ1Dx4SLqKOL+TMQp/Wi2mPtGomCYjlJugyC
nRSaXv1L8N4179af6LQsrrOQOIYQKF5q6NgW3+xY8/jSu5cTTr1vImhVzP6bleiv
RyoecJJnZ0GRoZgdzdX6s/nWXt9LQMbc12lKHNZXe+udoeShm8UIHrD87U5d5/6A
//8/UVUFsLq+CxUnzsvI6hz5+7QpwGmcKRVvIZQT5h2fTgl7NG95raMgqz42uNQd
NTGsiKwTmdYfjj1Ssk/8eC+/YKb2Q5+4XkAWkhcc1VXBSUYUAZuJEZDD75UsirXc
/3ILBI46jPkczkBHRN7W
=2LLE
-----END PGP SIGNATURE-----

--MRNWp8VlfMufsMVa--