Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: Improvements in psql hooks for variables
From: "Daniel Verite" <daniel@manitou-mail.org>
To: "Rahila Syed" <rahilasyed90@gmail.com>
Cc: "Ashutosh Bapat" <ashutosh.bapat@enterprisedb.com>,"pgsql-hackers"
 <pgsql-hackers@postgresql.org>
In-Reply-To: 
 <CAH2L28ucJ_d6zNBBaO_6Yc+HQSKY746UUDKh1wx0uTavednGyw@mail.gmail.com>
Date: Thu, 10 Nov 2016 22:27:19 +0100
Message-Id: <fc879967-da93-43b6-aa5a-92f2d825e786@mm>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

	Rahila Syed wrote:

> I have applied this patch on latest HEAD and have done basic testing which
> works fine.

Thanks for reviewing this patch!

> >            if (current->assign_hook)
> >-               (*current->assign_hook) (current->value);
> >-           return true;
> >+           {
> >+               confirmed =3D (*current->assign_hook) (value);
> >+           }
> >+           if (confirmed)
>=20
> Spurious brackets

OK.

> >static bool
> >+generic_boolean_hook(const char *newval, const char* varname, bool *fla=
g)
> >+{
>=20
> Contrary to what name suggests this function does not seem to have other
> implementations as in a hook.
> Also this takes care of rejecting a syntactically wrong value only for
> boolean variable hooks like autocommit_hook,
> on_error_stop_hook. However, there are other variable hooks which call
> ParseVariableBool.
> For instance, echo_hidden_hook which is handled separately in the patch.
> Thus there is some duplication of code between generic_boolean_hook and
> echo_hidden_hook.
> Similarly for on_error_rollback_hook.

The purpose of generic_boolean_hook() is to handle the case of a
boolean variable that only accepts ON or OFF, and has its pset.varname
declared as bool. I thought of this case as "generic" because that's
the base case and several variables need no more than that.

ECHO_HIDDEN differs from the generic boolean case because it also
accepts "noexec" and pset.echo_hidden is an enum, not a boolean. When
considering refactoring echo_hidden_hook() to call
generic_boolean_hook() instead of ParseVariableBool() after=20
having established that the value is not "noexec", I don't see
any advantage in clarity or code size, so I'm not in favor of that change.

The same applies to on_error_rollback_hook(), which has to deal
with a specific enum as well.

> >-static void
> >+static bool
> > fetch_count_hook(const char *newval)
> > {
> >    pset.fetch_count =3D ParseVariableNum(newval, -1, -1, false);
> >+   return true;
> > }
>=20
> Shouldn't invalid numeric string assignment for numeric variables be
> handled too?

Agreed. Assignments like "\set FETCH_COUNT bogus" don't provoke any
user feedback currently, which is not ideal. I'll add this in a
v3 of the patch tomorrow.

> Instead of generic_boolean_hook cant we have something like follows which
> like generic_boolean_hook can be called from specific variable assignment
> hooks,
>=20
> static bool
> ParseVariable(newval, VariableName, &pset.var)
> {
>     if (VariableName =3D=3D =E2=80=98AUTOCOMMIT=E2=80=99 || ECHO_HIDDEN |=
| other variable with
> hooks which call ParseVariableBool )
>         <logic here same as generic_boolean_hook in patch
>         <additional lines as there in the patch for ECHO_HIDDEN,
> ON_ERROR_ROLLBACK>
>     else if (VariableName =3D=3D =E2=80=98FETCH_COUNT=E2=80=99)
>         ParseVariableNum();
> }

It's not possible because pset.var corresponds to different fields from
struct _psqlSettings that have different types: bool, int and some
enum types.
Besides, I don't think it would go well with hooks. If we wanted one
big function that knows all about parsing all built-in variables, we
could just as well dispense with hooks, since their current purpose in
psql is to achieve this parsing, but in a decentralized way.
Or if we keep them, our various built-in variables would be
essentially tied to the same one-big-hook-that-does-all, but isn't
that an antipattern for hooks?


> >@@ -260,7 +276,7 @@ SetVariableAssignHook(VariableSpace space, const char
> *name, VariableAssignHook
> >    current->assign_hook =3D hook;
> >    current->next =3D NULL;
> >    previous->next =3D current;
> >-   (*hook) (NULL);
> >+   (void)(*hook) (NULL);       /* ignore return value */
>=20
> Sorry for my lack of understanding, can you explain why is above change
> needed?

"hook" is changed by the patch from [pointer to function returning
void], to [pointer to function returning bool]. The cast to void is
not essential, it just indicates that we deliberately want to
ignore the return value here. I expect some compilers might
complain under a high level of warnings without this cast, although
TBH if you ask me, I wouldn't know which compiler with which flags
exactly.


Best regards,
--=20
Daniel V=C3=A9rit=C3=A9
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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