Re: Improvements in psql hooks for variables - Mailing list pgsql-hackers
| From | Rahila Syed |
|---|---|
| Subject | Re: Improvements in psql hooks for variables |
| Date | |
| Msg-id | CAH2L28ucJ_d6zNBBaO_6Yc+HQSKY746UUDKh1wx0uTavednGyw@mail.gmail.com Whole thread Raw |
| In response to | Re: Improvements in psql hooks for variables ("Daniel Verite" <daniel@manitou-mail.org>) |
| Responses |
Re: Improvements in psql hooks for variables
|
| List | pgsql-hackers |
Hello,
I have applied this patch on latest HEAD and have done basic testing which works fine.Some comments,
> if (current->assign_hook)
>- (*current->assign_hook) (current->value);
>- return true;
>+ {
>+ confirmed = (*current->assign_hook) (value);
>+ }
>+ if (confirmed)
Spurious brackets
>static bool
>+generic_boolean_hook(const char *newval, const char* varname, bool *flag)
>+{
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.
>+static bool
> fetch_count_hook(const char *newval)
> {
> pset.fetch_count = ParseVariableNum(newval, -1, -1, false);
>+ return true;
> }
Instead of generic_boolean_hook cant we have something like follows which
like generic_boolean_hook can be called from specific variable assignment hooks,
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
{
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();
}
ParseVariableNum();
}
This will help merge the logic which is to check for valid syntax before
assigning and returning false on error, in one place.
assigning and returning false on error, in one place.
>@@ -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 */
> 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?
Thank you,
Rahila Syed
On Tue, Sep 20, 2016 at 11:16 PM, Daniel Verite <daniel@manitou-mail.org> wrote:
Ashutosh Bapat wrote:
> You may want to add this to the November commitfest
> https://commitfest.postgresql.org/11/.
Done. It's at https://commitfest.postgresql.org/11/799/
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
pgsql-hackers by date: