Thread: psql tabcomplete - minor bugfix - tabcomplete for SET ROLE TO xxx

On 5/22/15 6:35 AM, Pavel Stehule wrote:
> we support SET ROLE name and SET ROLE TO name. Second form isn't
> supported by tabcomplete. Attached trivial patch fixes it.

We don't tab-complete everything we possibly could.  The documentation
only lists the first form, so I don't think we necessarily need to
complete the second form.

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

I agree with Peter that "We don't tab-complete everything we possibly could", but using tabs after "SET ROLE TO "
provides"DEFAULT" as an option which seems wrong.
 
This patch adds list of roles over there, which I guess good to have than giving something unusual.

I reviewed this straight forward patch and looks good to me.

Since we might not want this, review is done and thus passing it to committer to decide.

The new status of this patch is: Ready for Committer

On 05/29/2015 10:41 AM, Pavel Stehule wrote:
> 2015-05-29 9:28 GMT+02:00 Jeevan Chalke <jeevan.chalke@gmail.com>:
>
>> I agree with Peter that "We don't tab-complete everything we possibly
>> could", but using tabs after "SET ROLE TO " provides "DEFAULT" as an option
>> which seems wrong.
>> This patch adds list of roles over there, which I guess good to have than
>> giving something unusual.
>>
>...
>
> But back to this topic. I am thinking so it is little bit different due
> fact so we support two very syntax for one feature. And looks little bit
> strange, so one way is supported by autocomplete and second not.

Yeah, it's a bit strange. We have a specific autocomplete rule for "SET 
ROLE", but "SET ROLE TO" is treated as a generic GUC. With your patch, 
we'd also lose the auto-completion to "SET ROLE TO DEFAULT".

I think we want to encourage people to use the SQL-standard syntax "SET 
ROLE ..." rather than the PostgreSQL-specific "SET ROLE TO ...". On the 
whole, this just doesn't seem like much of an improvement. I'll mark 
this as 'rejected' in the commitfest app.

PS. I note that the auto-completion for "SET XXX TO ... is pretty lousy 
in general. We have rules for DateStyle, IntervalStyle, GEQO and 
search_path, but that's it. That could be expanded a lot. All enum-type 
GUCs could be handled with a single rule that queries 
pg_settings.enumvals, for example, and booleans would be easy too. But 
that's a different story.

- Heikki

Hi,

On 2015-07-08 14:50:37 +0200, Pavel Stehule wrote:
> -            static const char *const my_list[] =
> -            {"DEFAULT", NULL};
> +            /* fallback for GUC settings */
>  
> -            COMPLETE_WITH_LIST(my_list);
> +            char *vartype = get_vartype(prev2_wd);
> +
> +            if (strcmp(vartype, "enum") == 0)
> +            {
> +                char querybuf[1024];
> +
> +                snprintf(querybuf, 1024, Query_for_enum, prev2_wd);
> +                COMPLETE_WITH_QUERY(querybuf);
> +            }

Won't that mean that enum variables don't complete to default anymore?

> +static char *
> +get_vartype(const char *varname)
> +{
> +    PQExpBufferData query_buffer;
> +    char    *e_varname;
> +    PGresult *result;
> +    int    string_length;
> +    static char resbuf[10];
> +
> +    initPQExpBuffer(&query_buffer);
> +
> +    string_length = strlen(varname);
> +    e_varname = pg_malloc(string_length * 2 + 1);
> +    PQescapeString(e_varname, varname, string_length);

Independent of this patch, we really shouldn't do this in several places
:(

> +    appendPQExpBuffer(&query_buffer,
> +        "SELECT vartype FROM pg_settings WHERE pg_catalog.lower(name) = pg_catalog.lower('%s')",
> +             e_varname);

Missing pg_catalog for pg_settings.

Greetings,

Andres Freund

Hi,

On 2015-09-02 22:58:21 +0200, Pavel Stehule wrote:
> > Won't that mean that enum variables don't complete to default anymore?

> no, it does
>
> #define Query_for_enum \
> " SELECT name FROM ( "\
> "   SELECT unnest(enumvals) AS name "\
> "    FROM pg_catalog.pg_settings "\
> "   WHERE pg_catalog.lower(name)=pg_catalog.lower('%s') "\
> "   UNION SELECT 'DEFAULT' ) ss "\
> ****************
> "  WHERE pg_catalog.substring(name,1,%%d)='%%s'"

Ah.

I've added a quote_ident() around unnest() - otherwise the
auto-completions for default_transaction_isolation will mostly be wrong
due to spaces.

I also renamed get_vartype into get_guctype, changed the comment as I
found the reference to the pg type system confusing, and more
importantly made it not return a static buffer.

The spellings for boolean values were a relatively small subset of what
the backend accepts - it's now on,off,true,false,yes,no,1,0. I'm not
sure whether that's a good idea. Comments?

Andres

On 2015-09-08 07:06:04 +0200, Pavel Stehule wrote:
> 2015-09-07 21:44 GMT+02:00 Andres Freund <andres@anarazel.de>:
> > The spellings for boolean values were a relatively small subset of what
> > the backend accepts - it's now on,off,true,false,yes,no,1,0. I'm not
> > sure whether that's a good idea. Comments?
> >
> 
> if somebody prefer true, false, and we will support only on, off, then the
> tabcomplete will not be too user friendly :(
> 
> "1, 0" can be out - but other?

After sleeping on it I think we should keep all of them - they'll show
for lots of "boolean like" GUCs (e.g. constraint_exclusion, sync_commit,
huge_pages) so not showing them for booleans just seems
inconsisten. Unless somebody protests pdq I'll push it that way.

Andres

Andres Freund wrote:
> On 2015-09-08 07:06:04 +0200, Pavel Stehule wrote:
> > 2015-09-07 21:44 GMT+02:00 Andres Freund <andres@anarazel.de>:
> > > The spellings for boolean values were a relatively small subset of what
> > > the backend accepts - it's now on,off,true,false,yes,no,1,0. I'm not
> > > sure whether that's a good idea. Comments?
> > 
> > if somebody prefer true, false, and we will support only on, off, then the
> > tabcomplete will not be too user friendly :(
> > 
> > "1, 0" can be out - but other?
> 
> After sleeping on it I think we should keep all of them - they'll show
> for lots of "boolean like" GUCs (e.g. constraint_exclusion, sync_commit,
> huge_pages) so not showing them for booleans just seems
> inconsisten. Unless somebody protests pdq I'll push it that way.

Yeah, seems fine to list the whole lot.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services