From: | "Daniel Verite" <daniel(at)manitou-mail(dot)org> |
---|---|
To: | "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | "Rahila Syed" <rahilasyed90(at)gmail(dot)com>,"Stephen Frost" <sfrost(at)snowman(dot)net>,"Ashutosh Bapat" <ashutosh(dot)bapat(at)enterprisedb(dot)com>,"pgsql-hackers" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Improvements in psql hooks for variables |
Date: | 2017-01-16 16:27:19 |
Message-ID: | bfad679d-d4af-4363-a4f3-289cd691556b@manitou-mail.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Tom Lane wrote:
> "Daniel Verite" <daniel(at)manitou-mail(dot)org> writes:
> > [ psql-var-hooks-v6.patch ]
>
> I took a quick look through this. It seems to be going in generally
> the right direction, but here's a couple of thoughts:
Thanks for looking into this!
> I'm inclined to think that the best choice is for the function result
> to be the ok/not ok flag, and pass the variable-to-be-modified as an
> output parameter. That fits better with the notion that the variable
> is not to be modified on failure.
Agreed, if never clobbering the variable, having the valid/invalid state
returned by ParseVariableBool() allows for simpler code. I'm changing it
that way.
> Also, why aren't you using ParseVariableBool's existing ability to report
> errors?
Its' because there are two cases:
- either only a boolean can be given to the command or variable,
in which we let ParseVariableBool() tell:
unrecognized value "bogus" for "command": boolean expected
- or other parameters besides boolean are acceptable, in which case we
can't say "boolean expected", because in fact a boolean is no more or
less expected than the other valid values.
We could shave code by reducing ParseVariableBool()'s error message to:
unrecognized value "bogus" for "name"
clearing the distinction between [only booleans are expected]
and [booleans or enum are expected].
Then almost all callers that have their own message could rely
on ParseVariableBool() instead, as they did previously.
Do we care about the "boolean expected" part of the error message?
> else if (value)
> ! {
> ! bool valid;
> ! bool newval = ParseVariableBool(value, NULL, &valid);
> ! if (valid)
> ! popt->topt.expanded = newval;
> ! else
> ! {
> ! psql_error("unrecognized value \"%s\" for \"%s\"\n",
> ! value, param);
> ! return false;
> ! }
> ! }
> is really the hard way and you could have just done
>
> - popt->topt.expanded = ParseVariableBool(value, param);
> + success = ParseVariableBool(value, param,
> &popt->topt.expanded);
I get the idea, except that in this example, the compiler is
unhappy because popt->topt.expanded is not a bool, and an
explicit cast here would be kludgy.
For the error printing part, it would go away with the above
suggestion
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2017-01-16 16:42:08 | Re: Improvements in psql hooks for variables |
Previous Message | Karl O. Pinc | 2017-01-16 16:09:26 | Re: Patch to implement pg_current_logfile() function |