From: Daniel Verite <daniel@manitou-mail.org>
To: Rahila Syed <rahilasyed90@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Improvements in psql hooks for variables
Date: Thu, 10 Nov 2016 22:27:19 +0100
Message-ID: <fc879967-da93-43b6-aa5a-92f2d825e786@mm> (raw)
In-Reply-To: <CAH2L28ucJ_d6zNBBaO_6Yc+HQSKY746UUDKh1wx0uTavednGyw@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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 = (*current->assign_hook) (value);
> >+ }
> >+ if (confirmed)
>
> Spurious brackets
OK.
> >static bool
> >+generic_boolean_hook(const char *newval, const char* varname, bool *flag)
> >+{
>
> 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
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 = ParseVariableNum(newval, -1, -1, false);
> >+ return true;
> > }
>
> 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,
>
> static bool
> ParseVariable(newval, VariableName, &pset.var)
> {
> if (VariableName == ‘AUTOCOMMIT’ || 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 == ‘FETCH_COUNT’)
> 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 = hook;
> > current->next = NULL;
> > previous->next = current;
> >- (*hook) (NULL);
> >+ (void)(*hook) (NULL); /* ignore return value */
>
> 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,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: daniel@manitou-mail.org, rahilasyed90@gmail.com, ashutosh.bapat@enterprisedb.com
Subject: Re: Improvements in psql hooks for variables
In-Reply-To: <fc879967-da93-43b6-aa5a-92f2d825e786@mm>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox