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

Hi

we support SET ROLE name and SET ROLE TO name. Second form isn't supported
by tabcomplete. Attached trivial patch fixes it.

Regards

Pavel

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

2015-05-29 9:28 GMT+02:00 Jeevan Chalke <jeevan(dot)chalke(at)gmail(dot)com>:

> 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.
>

Surely, we cannot to support tab-complete everywhere. But if we can do it
simply, we should to do it. Why:

1. It is good help for beginners

2. I am PostgreSQL lecture and evangelist in Czech Republic and Slovak
Republic. The tabcomplete possibility is surprisingly good factor for
accepting PostgreSQL concept, architecture, psql

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.

>
> 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.

ok

>
> The new status of this patch is: Ready for Committer
>

Thank you very much

Regards

Pavel

>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On 05/29/2015 10:41 AM, Pavel Stehule wrote:
> 2015-05-29 9:28 GMT+02:00 Jeevan Chalke <jeevan(dot)chalke(at)gmail(dot)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

2015-07-02 11:03 GMT+02:00 Heikki Linnakangas <hlinnaka(at)iki(dot)fi>:

> On 05/29/2015 10:41 AM, Pavel Stehule wrote:
>
>> 2015-05-29 9:28 GMT+02:00 Jeevan Chalke <jeevan(dot)chalke(at)gmail(dot)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.
>
>
ok

Pavel

> 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
>
>

2015-07-02 11:03 GMT+02:00 Heikki Linnakangas <hlinnaka(at)iki(dot)fi>:

> On 05/29/2015 10:41 AM, Pavel Stehule wrote:
>
>> 2015-05-29 9:28 GMT+02:00 Jeevan Chalke <jeevan(dot)chalke(at)gmail(dot)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.
>

I wrote a patch for fallback tabcomplete for bool and enum GUC variables

Regards

Pavel

>
> - 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

2015-09-02 15:23 GMT+02:00 Andres Freund <andres(at)anarazel(dot)de>:

> 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?
>

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'"

>
> > +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
> :(
>

fixed

>
> > + 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.
>

fixed

>
> Greetings,
>
> Andres Freund
>

I am sending new version

Regards

Pavel

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

2015-09-07 21:44 GMT+02:00 Andres Freund <andres(at)anarazel(dot)de>:

> 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?
>

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?

>
> Andres
>

On 2015-09-08 07:06:04 +0200, Pavel Stehule wrote:
> 2015-09-07 21:44 GMT+02:00 Andres Freund <andres(at)anarazel(dot)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(at)anarazel(dot)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