Re: psql tab completion for updatable foreign tables

Recently i got annoyed that psql doesn't tab complete to updatable foreign
tables.

Attached is a patch to address this. I'm using the new
pg_relation_is_updatable() function to accomplish this. The function could
also be used for the trigger based stuff in the query, but i haven't
touched this part of the query. The patch ist against HEAD, but maybe it's
worth to apply this to REL9_3_STABLE, too, but not sure what our policy is
at this state...

--
Thanks

Bernd

On 8 July 2013 12:46, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
> Recently i got annoyed that psql doesn't tab complete to updatable foreign
> tables.
>
> Attached is a patch to address this. I'm using the new
> pg_relation_is_updatable() function to accomplish this. The function could
> also be used for the trigger based stuff in the query, but i haven't touched
> this part of the query. The patch ist against HEAD, but maybe it's worth to
> apply this to REL9_3_STABLE, too, but not sure what our policy is at this
> state...
>

+1

A couple of points though:

* pg_relation_is_updatable is only available in 9.3, whereas psql may
connect to older servers, so it needs to guard against that.

* If we're doing this, I think we should do it for auto-updatable
views at the same time.

There was concern that pg_relation_is_updatable() would end up opening
every relation in the database, hammering performance. I now realise
that these tab-complete queries have a limit (1000 by default) so I
don't think this is such an issue. I tested it by creating 10,000
randomly named auto-updatable views on top of a table, and didn't see
any performance problems.

Here's an updated patch based on the above points.

Regards,
Dean

--On 8. Juli 2013 16:04:31 +0000 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
wrote:

> * pg_relation_is_updatable is only available in 9.3, whereas psql may
> connect to older servers, so it needs to guard against that.
>

Oh of course, i forgot about this. Thanks for pointing out.

> * If we're doing this, I think we should do it for auto-updatable
> views at the same time.
>
> There was concern that pg_relation_is_updatable() would end up opening
> every relation in the database, hammering performance. I now realise
> that these tab-complete queries have a limit (1000 by default) so I
> don't think this is such an issue. I tested it by creating 10,000
> randomly named auto-updatable views on top of a table, and didn't see
> any performance problems.
>
> Here's an updated patch based on the above points.

Okay, are you adding this to the september commitfest?

--
Thanks

Bernd

On 11 July 2013 00:03, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>
>
> --On 8. Juli 2013 16:04:31 +0000 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
> wrote:
>
>> * pg_relation_is_updatable is only available in 9.3, whereas psql may
>> connect to older servers, so it needs to guard against that.
>>
>
> Oh of course, i forgot about this. Thanks for pointing out.
>
>
>> * If we're doing this, I think we should do it for auto-updatable
>> views at the same time.
>>
>> There was concern that pg_relation_is_updatable() would end up opening
>> every relation in the database, hammering performance. I now realise
>> that these tab-complete queries have a limit (1000 by default) so I
>> don't think this is such an issue. I tested it by creating 10,000
>> randomly named auto-updatable views on top of a table, and didn't see
>> any performance problems.
>>
>> Here's an updated patch based on the above points.
>
>
> Okay, are you adding this to the september commitfest?
>

OK, I've done that. I think that it's too late for 9.3.

Regards,
Dean

>

> > Okay, are you adding this to the september commitfest?
> >
>
> OK, I've done that. I think that it's too late for 9.3.
>
>

+1 for idea.

I have tested patch and got surprising results with Cent-OS
Patch is working fine for Cent-OS 6.2 and RHEL 6.3
But is is giving problem on Cent-OS 6.3 (tab complete for local tables but
not for foreign tables)

I have used following script:

Two postgres servers are running by using ports 5432 and 5433.
Server with port 5432 has postgres_fdw installed and will interact with the
remote server running under port 5433.

psql -p 5433 -c "CREATE TABLE aa_remote (a int, b int)" postgres
postgres=# CREATE EXTENSION postgres_fdw;
postgres=# CREATE SERVER postgres_server FOREIGN DATA WRAPPER postgres_fdw
OPTIONS (host 'localhost', port '5433', dbname 'postgres');
postgres=# CREATE USER MAPPING FOR PUBLIC SERVER postgres_server OPTIONS
(password '');
postgres=# CREATE FOREIGN TABLE aa_foreign (a int, b int) SERVER
postgres_server OPTIONS (table_name 'aa_remote');
postgres=# INSERT into aa_foreign values (1,2);

But while doing any operation on aa_foreign tab do not complete on Cent-OS
6.3 (tab complete for local tables but not for foreign tables)
Is that a problem ?

Regards,
Samrat Revagade

On 20 September 2013 11:29, Samrat Revagade <revagade(dot)samrat(at)gmail(dot)com> wrote:
>>
>>
>> > Okay, are you adding this to the september commitfest?
>> >
>>
>> OK, I've done that. I think that it's too late for 9.3.
>>
>
>
> +1 for idea.
>
> I have tested patch and got surprising results with Cent-OS
> Patch is working fine for Cent-OS 6.2 and RHEL 6.3
> But is is giving problem on Cent-OS 6.3 (tab complete for local tables but
> not for foreign tables)
>
> I have used following script:
>
> Two postgres servers are running by using ports 5432 and 5433.
> Server with port 5432 has postgres_fdw installed and will interact with the
> remote server running under port 5433.
>
> psql -p 5433 -c "CREATE TABLE aa_remote (a int, b int)" postgres
> postgres=# CREATE EXTENSION postgres_fdw;
> postgres=# CREATE SERVER postgres_server FOREIGN DATA WRAPPER postgres_fdw
> OPTIONS (host 'localhost', port '5433', dbname 'postgres');
> postgres=# CREATE USER MAPPING FOR PUBLIC SERVER postgres_server OPTIONS
> (password '');
> postgres=# CREATE FOREIGN TABLE aa_foreign (a int, b int) SERVER
> postgres_server OPTIONS (table_name 'aa_remote');
> postgres=# INSERT into aa_foreign values (1,2);
>
> But while doing any operation on aa_foreign tab do not complete on Cent-OS
> 6.3 (tab complete for local tables but not for foreign tables)
> Is that a problem ?
>

Hmm. It works for me. What does pg_relation_is_updatable() return for
your foreign table?

Regards,
Dean

On Fri, Sep 20, 2013 at 7:54 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>wrote:

> On 20 September 2013 11:29, Samrat Revagade <revagade(dot)samrat(at)gmail(dot)com>
> wrote:
> >>
> >>
> >> > Okay, are you adding this to the september commitfest?
> >> >
> >>
> >> OK, I've done that. I think that it's too late for 9.3.
> >>
> >
> >
> > +1 for idea.
> >
> > I have tested patch and got surprising results with Cent-OS
> > Patch is working fine for Cent-OS 6.2 and RHEL 6.3
> > But is is giving problem on Cent-OS 6.3 (tab complete for local tables
> but
> > not for foreign tables)
> >
> > I have used following script:
> >
> > Two postgres servers are running by using ports 5432 and 5433.
> > Server with port 5432 has postgres_fdw installed and will interact with
> the
> > remote server running under port 5433.
> >
> > psql -p 5433 -c "CREATE TABLE aa_remote (a int, b int)" postgres
> > postgres=# CREATE EXTENSION postgres_fdw;
> > postgres=# CREATE SERVER postgres_server FOREIGN DATA WRAPPER
> postgres_fdw
> > OPTIONS (host 'localhost', port '5433', dbname 'postgres');
> > postgres=# CREATE USER MAPPING FOR PUBLIC SERVER postgres_server OPTIONS
> > (password '');
> > postgres=# CREATE FOREIGN TABLE aa_foreign (a int, b int) SERVER
> > postgres_server OPTIONS (table_name 'aa_remote');
> > postgres=# INSERT into aa_foreign values (1,2);
> >
> > But while doing any operation on aa_foreign tab do not complete on
> Cent-OS
> > 6.3 (tab complete for local tables but not for foreign tables)
> > Is that a problem ?
> >
>
> Hmm. It works for me. What does pg_relation_is_updatable() return for
> your foreign table?
>
>

Sorry .my environment has some problem. when I compiled it with fresh
installation of Cent-OS 6.3 it worked.
Patch :
1. Applies cleanly to git master
2. Has necessary source code comments
3. Follows coding standards
4. Solves use case.

On Mon, Oct 14, 2013 at 7:20 PM, Samrat Revagade
<revagade(dot)samrat(at)gmail(dot)com>wrote:

>
> Sorry .my environment has some problem.
>

May be you were using old version of psql ? IIRC tab-completion relies
heavily on the psql side,

Thanks,
Pavan
--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee

On Mon, 2013-07-08 at 16:04 +0000, Dean Rasheed wrote:
> There was concern that pg_relation_is_updatable() would end up opening
> every relation in the database, hammering performance. I now realise
> that these tab-complete queries have a limit (1000 by default) so I
> don't think this is such an issue. I tested it by creating 10,000
> randomly named auto-updatable views on top of a table, and didn't see
> any performance problems.

Even if performance isn't a problem, do we really want tab completion
interfering with table locking? That might be a step too far.

Personally, I think this is too fancy anyway. I'd just complete all
views and foreign tables and be done with it. We don't inspect
permissions either, for example. This might be too confusing for users.

On 17 October 2013 03:29, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On Mon, 2013-07-08 at 16:04 +0000, Dean Rasheed wrote:
>> There was concern that pg_relation_is_updatable() would end up opening
>> every relation in the database, hammering performance. I now realise
>> that these tab-complete queries have a limit (1000 by default) so I
>> don't think this is such an issue. I tested it by creating 10,000
>> randomly named auto-updatable views on top of a table, and didn't see
>> any performance problems.
>
> Even if performance isn't a problem, do we really want tab completion
> interfering with table locking? That might be a step too far.
>

That's a good point. Currently it's calling pg_table_is_visible(),
which is doing similar work opening each relation, but it isn't
locking any of them.

> Personally, I think this is too fancy anyway. I'd just complete all
> views and foreign tables and be done with it. We don't inspect
> permissions either, for example. This might be too confusing for users.
>

Yeah, I think you're probably right.

Regards,
Dean

On Fri, Oct 18, 2013 at 1:34 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> Personally, I think this is too fancy anyway. I'd just complete all
>> views and foreign tables and be done with it. We don't inspect
>> permissions either, for example. This might be too confusing for users.
>
> Yeah, I think you're probably right.

I tend to agree. When the rules were simple (i.e. pretty much nothing
was updateable) it might have made sense to make tab completion hew to
them, but they're complex enough now that I think it no longer does.
There are now three different ways that a view can be updateable
(auto, trigger, rule) and the rules are complex.

Based on that it sounds like we need a new version of this patch. If
that's not going to happen RSN, we should mark this returned with
feedback and it can be resubmitted if and when someone finds the time
to update it.

Thanks,

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 18 October 2013 16:41, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Oct 18, 2013 at 1:34 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>> Personally, I think this is too fancy anyway. I'd just complete all
>>> views and foreign tables and be done with it. We don't inspect
>>> permissions either, for example. This might be too confusing for users.
>>
>> Yeah, I think you're probably right.
>
> I tend to agree. When the rules were simple (i.e. pretty much nothing
> was updateable) it might have made sense to make tab completion hew to
> them, but they're complex enough now that I think it no longer does.
> There are now three different ways that a view can be updateable
> (auto, trigger, rule) and the rules are complex.
>
> Based on that it sounds like we need a new version of this patch. If
> that's not going to happen RSN, we should mark this returned with
> feedback and it can be resubmitted if and when someone finds the time
> to update it.
>

OK, here's a new version that just completes with all tables, views
and foreign tables.

Doing this makes the insert, update and delete queries all the same,
which means there's not much point in keeping all three, so I've just
kept Query_for_list_of_updatables for use with INSERT, UPDATE and
DELETE.

Regards,
Dean

On Sat, Oct 19, 2013 at 5:44 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 18 October 2013 16:41, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Oct 18, 2013 at 1:34 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>>> Personally, I think this is too fancy anyway. I'd just complete all
>>>> views and foreign tables and be done with it. We don't inspect
>>>> permissions either, for example. This might be too confusing for users.
>>>
>>> Yeah, I think you're probably right.
>>
>> I tend to agree. When the rules were simple (i.e. pretty much nothing
>> was updateable) it might have made sense to make tab completion hew to
>> them, but they're complex enough now that I think it no longer does.
>> There are now three different ways that a view can be updateable
>> (auto, trigger, rule) and the rules are complex.
>>
>> Based on that it sounds like we need a new version of this patch. If
>> that's not going to happen RSN, we should mark this returned with
>> feedback and it can be resubmitted if and when someone finds the time
>> to update it.
>>
>
> OK, here's a new version that just completes with all tables, views
> and foreign tables.
>
> Doing this makes the insert, update and delete queries all the same,
> which means there's not much point in keeping all three, so I've just
> kept Query_for_list_of_updatables for use with INSERT, UPDATE and
> DELETE.

Committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company