Re: Tab completion for view triggers in psql

Folks,

Please find attached patch for $subject :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Oct 26, 2010 at 5:01 AM, David Fetter <david(at)fetter(dot)org> wrote:
> Please find attached patch for $subject :)

Thank you for maintaining psql tab completion,
but I'm not sure whether tgtype is the best column for the purpose.
How about has_table_privilege() to filter candidate relations
in Query_for_list_of_insertables/deleteables/updateables?

--
Itagaki Takahiro

On Tue, Oct 26, 2010 at 10:30:49AM +0900, Itagaki Takahiro wrote:
> On Tue, Oct 26, 2010 at 5:01 AM, David Fetter <david(at)fetter(dot)org> wrote:
> > Please find attached patch for $subject :)
>
> Thank you for maintaining psql tab completion, but I'm not sure
> whether tgtype is the best column for the purpose. How about
> has_table_privilege() to filter candidate relations in
> Query_for_list_of_insertables/deleteables/updateables?

That's orthogonal to tgtype, as far as I can tell. The tgtype test is
to tell whether it's possible for anyone to do the operation on the
view, where has_table_privilege, as I understand it, tells whether
some particular role that privilege. Shall I send a new patch with
that added?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Oct 26, 2010 at 10:53 AM, David Fetter <david(at)fetter(dot)org> wrote:
>> How about has_table_privilege() to filter candidate relations
>
> That's orthogonal to tgtype (snip)
> Shall I send a new patch with that added?

Do we need to 'add' it? I intended to replace the JOIN with pg_trigger
to has_table_privilege() (and relkind IN ('r', 'v')) for INSERT, UPDATE,
and DELETE cases. Query_for_list_of_writeables might still require your
patch, though.

--
Itagaki Takahiro

On Tue, Oct 26, 2010 at 11:10:53AM +0900, Itagaki Takahiro wrote:
> On Tue, Oct 26, 2010 at 10:53 AM, David Fetter <david(at)fetter(dot)org> wrote:
> >> How about has_table_privilege() to filter candidate relations
> >
> > That's orthogonal to tgtype (snip) Shall I send a new patch with
> > that added?
>
> Do we need to 'add' it?

Possibly. My understanding is that it couldn't really replace it.

> I intended to replace the JOIN with pg_trigger to
> has_table_privilege() (and relkind IN ('r', 'v')) for INSERT,
> UPDATE, and DELETE cases. Query_for_list_of_writeables might still
> require your patch, though.

My understanding is that there are two parts to this:

1. Does the view have the operation (INSERT, UPDATE, or DELETE)
defined on it at all?

2. Can the current role actually perform the operation defined?

If a view has at least one trigger, that view will have corresponding
entries in pg_trigger, and the tgtype entry determines which
operations have been defined, hence that EXISTS() query. This
establishes part 1.

The call to has_table_privilege() establishes part 2.

If I've misunderstood, please let me know :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Oct 26, 2010 at 11:34 AM, David Fetter <david(at)fetter(dot)org> wrote:
>> Do we need to 'add' it?
> Possibly.  My understanding is that it couldn't really replace it.

Ah, I see. I was wrong. We can have modification privileges for views
even if they have no INSTEAD OF triggers.

So, I think your original patch is the best solution. We could use
has_table_privilege() additionally, but we need to consider any
other places if we use it. For example, DROP privileges, etc.

--
Itagaki Takahiro

On 25 October 2010 21:01, David Fetter <david(at)fetter(dot)org> wrote:
> Folks,
>
> Please find attached patch for $subject :)
>

Thanks for looking at this. I forgot about tab completion.

I think that the change to ALTER TRIGGER is not necessary. AFAICT it
works OK unmodified. In fact, the modified code here:

*** 971,977 **** psql_completion(char *text, int start, int end)
else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
pg_strcasecmp(prev3_wd, "TRIGGER") == 0 &&
pg_strcasecmp(prev_wd, "ON") == 0)
! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, NULL);

/* ALTER TRIGGER <name> ON <name> */
else if (pg_strcasecmp(prev4_wd, "TRIGGER") == 0 &&
--- 1055,1061 ----
else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
pg_strcasecmp(prev3_wd, "TRIGGER") == 0 &&
pg_strcasecmp(prev_wd, "ON") == 0)
! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_writeables, NULL);

/* ALTER TRIGGER <name> ON <name> */
else if (pg_strcasecmp(prev4_wd, "TRIGGER") == 0 &&

appears to be unreachable, because it is preceded by

else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
pg_strcasecmp(prev3_wd, "TRIGGER") == 0)
{
completion_info_charp = prev2_wd;
COMPLETE_WITH_QUERY(Query_for_list_of_tables_for_trigger);
}

which works for tables and views, and makes the next "elseif"
impossible to satisfy. So I think that block could just be deleted,
right?

Regards,
Dean

> Cheers,
> David.
> --
> David Fetter <david(at)fetter(dot)org> http://fetter.org/
> Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
> Skype: davidfetter      XMPP: david(dot)fetter(at)gmail(dot)com
> iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
>
> Remember to vote!
> Consider donating to Postgres: http://www.postgresql.org/about/donate
>
>
> --
> 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 Tue, Oct 26, 2010 at 12:35:13PM +0100, Dean Rasheed wrote:
> On 25 October 2010 21:01, David Fetter <david(at)fetter(dot)org> wrote:
> > Folks,
> >
> > Please find attached patch for $subject :)
> >
>
> Thanks for looking at this. I forgot about tab completion.
>
> I think that the change to ALTER TRIGGER is not necessary. AFAICT it
> works OK unmodified. In fact, the modified code here:
>
> *** 971,977 **** psql_completion(char *text, int start, int end)
> else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
> pg_strcasecmp(prev3_wd, "TRIGGER") == 0 &&
> pg_strcasecmp(prev_wd, "ON") == 0)
> ! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, NULL);
>
> /* ALTER TRIGGER <name> ON <name> */
> else if (pg_strcasecmp(prev4_wd, "TRIGGER") == 0 &&
> --- 1055,1061 ----
> else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
> pg_strcasecmp(prev3_wd, "TRIGGER") == 0 &&
> pg_strcasecmp(prev_wd, "ON") == 0)
> ! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_writeables, NULL);
>
> /* ALTER TRIGGER <name> ON <name> */
> else if (pg_strcasecmp(prev4_wd, "TRIGGER") == 0 &&
>
> appears to be unreachable, because it is preceded by
>
> else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
> pg_strcasecmp(prev3_wd, "TRIGGER") == 0)
> {
> completion_info_charp = prev2_wd;
> COMPLETE_WITH_QUERY(Query_for_list_of_tables_for_trigger);
> }

It is indeed unreachable.

> which works for tables and views, and makes the next "elseif"
> impossible to satisfy. So I think that block could just be deleted,
> right?

Yes. Good catch. New patch attached :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Oct 26, 2010 at 11:55:07AM +0900, Itagaki Takahiro wrote:
> On Tue, Oct 26, 2010 at 11:34 AM, David Fetter <david(at)fetter(dot)org> wrote:
> >> Do we need to 'add' it?
> > Possibly.  My understanding is that it couldn't really replace it.
>
> Ah, I see. I was wrong. We can have modification privileges for
> views even if they have no INSTEAD OF triggers.

Right.

> So, I think your original patch is the best solution. We could use
> has_table_privilege() additionally, but we need to consider any
> other places if we use it. For example, DROP privileges, etc.

That seems like a matter for a separate patch. Looking this over, I
found I'd created a query that can never get used, so please find
enclosed the next version of the patch :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Fri, Oct 29, 2010 at 08:33:00AM -0700, David Fetter wrote:
> On Tue, Oct 26, 2010 at 11:55:07AM +0900, Itagaki Takahiro wrote:
> > On Tue, Oct 26, 2010 at 11:34 AM, David Fetter <david(at)fetter(dot)org> wrote:
> > >> Do we need to 'add' it?
> > > Possibly.  My understanding is that it couldn't really replace it.
> >
> > Ah, I see. I was wrong. We can have modification privileges for
> > views even if they have no INSTEAD OF triggers.
>
> Right.
>
> > So, I think your original patch is the best solution. We could use
> > has_table_privilege() additionally, but we need to consider any
> > other places if we use it. For example, DROP privileges, etc.
>
> That seems like a matter for a separate patch. Looking this over, I
> found I'd created a query that can never get used, so please find
> enclosed the next version of the patch :)

Could someone please commit this? :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Sun, Nov 21, 2010 at 1:07 PM, David Fetter <david(at)fetter(dot)org> wrote:
> On Fri, Oct 29, 2010 at 08:33:00AM -0700, David Fetter wrote:
>> On Tue, Oct 26, 2010 at 11:55:07AM +0900, Itagaki Takahiro wrote:
>> > On Tue, Oct 26, 2010 at 11:34 AM, David Fetter <david(at)fetter(dot)org> wrote:
>> > >> Do we need to 'add' it?
>> > > Possibly.  My understanding is that it couldn't really replace it.
>> >
>> > Ah, I see.  I was wrong.  We can have modification privileges for
>> > views even if they have no INSTEAD OF triggers.
>>
>> Right.
>>
>> > So, I think your original patch is the best solution.  We could use
>> > has_table_privilege() additionally, but we need to consider any
>> > other places if we use it.  For example, DROP privileges, etc.
>>
>> That seems like a matter for a separate patch.  Looking this over, I
>> found I'd created a query that can never get used, so please find
>> enclosed the next version of the patch :)
>
> Could someone please commit this? :)

Eh... was there some reason you didn't add it to the CommitFest app?
Because that's what I work from.

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

On Sun, Nov 21, 2010 at 03:36:58PM -0500, Robert Haas wrote:
> On Sun, Nov 21, 2010 at 1:07 PM, David Fetter <david(at)fetter(dot)org> wrote:
> > On Fri, Oct 29, 2010 at 08:33:00AM -0700, David Fetter wrote:
> >> On Tue, Oct 26, 2010 at 11:55:07AM +0900, Itagaki Takahiro wrote:
> >> > On Tue, Oct 26, 2010 at 11:34 AM, David Fetter <david(at)fetter(dot)org> wrote:
> >> > >> Do we need to 'add' it?
> >> > > Possibly.  My understanding is that it couldn't really replace it.
> >> >
> >> > Ah, I see.  I was wrong.  We can have modification privileges for
> >> > views even if they have no INSTEAD OF triggers.
> >>
> >> Right.
> >>
> >> > So, I think your original patch is the best solution.  We could use
> >> > has_table_privilege() additionally, but we need to consider any
> >> > other places if we use it.  For example, DROP privileges, etc.
> >>
> >> That seems like a matter for a separate patch.  Looking this over, I
> >> found I'd created a query that can never get used, so please find
> >> enclosed the next version of the patch :)
> >
> > Could someone please commit this? :)
>
> Eh... was there some reason you didn't add it to the CommitFest app?

I forgot.

> Because that's what I work from.

It's pretty trivial, but I don't feel comfortable adding it
after the close. :/

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Sun, Nov 21, 2010 at 4:05 PM, David Fetter <david(at)fetter(dot)org> wrote:
>> > Could someone please commit this? :)
>>
>> Eh... was there some reason you didn't add it to the CommitFest app?
>
> I forgot.

A fair excuse. :-)

>> Because that's what I work from.
>
> It's pretty trivial, but I don't feel comfortable adding it
> after the close. :/

So add it to the next one, and we'll get it then if nobody picks it up sooner...

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

On Sun, Nov 21, 2010 at 07:09:08PM -0500, Robert Haas wrote:
> On Sun, Nov 21, 2010 at 4:05 PM, David Fetter <david(at)fetter(dot)org> wrote:
> >> > Could someone please commit this? :)
> >>
> >> Eh... was there some reason you didn't add it to the CommitFest app?
> >
> > I forgot.
>
> A fair excuse. :-)
>
> >> Because that's what I work from.
> >
> > It's pretty trivial, but I don't feel comfortable adding it
> > after the close. :/
>
> So add it to the next one, and we'll get it then if nobody picks it up sooner...

Given its small and isolated nature, I was hoping we could get this in
sooner rather than later. As I understand it, CFs are there to review
patches that take significant effort for even a committer to
understand, so this doesn't really fit that model.

Cheers,
David (refraining from mentioning anything about the time taken today
to discuss this vs. the time it would have taken to push the thing)
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Sun, Nov 21, 2010 at 7:17 PM, David Fetter <david(at)fetter(dot)org> wrote:
> On Sun, Nov 21, 2010 at 07:09:08PM -0500, Robert Haas wrote:
>> On Sun, Nov 21, 2010 at 4:05 PM, David Fetter <david(at)fetter(dot)org> wrote:
>> >> > Could someone please commit this? :)
>> >>
>> >> Eh... was there some reason you didn't add it to the CommitFest app?
>> >
>> > I forgot.
>>
>> A fair excuse.  :-)
>>
>> >> Because that's what I work from.
>> >
>> > It's pretty trivial, but I don't feel comfortable adding it
>> > after the close. :/
>>
>> So add it to the next one, and we'll get it then if nobody picks it up sooner...
>
> Given its small and isolated nature, I was hoping we could get this in
> sooner rather than later.  As I understand it, CFs are there to review
> patches that take significant effort for even a committer to
> understand, so this doesn't really fit that model.

Well, then add it to this one if you think that's more appropriate.
My point is simple: I review patches because they are in the CF queue.
Your point seems to be: put mine ahead of the others, and review it
immediately. Someone else may very well be willing to do that; I'm
not.

> David (refraining from mentioning anything about the time taken today
> to discuss this vs. the time it would have taken to push the thing)

Mention anything you want. My guess is it would take me an hour.
You're certainly right that this discussion is a waste of time, but
possibly not for the reasons you are supposing.

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

On Sun, Nov 21, 2010 at 08:27:34PM -0500, Robert Haas wrote:
> On Sun, Nov 21, 2010 at 7:17 PM, David Fetter <david(at)fetter(dot)org> wrote:
> > On Sun, Nov 21, 2010 at 07:09:08PM -0500, Robert Haas wrote:
> >> On Sun, Nov 21, 2010 at 4:05 PM, David Fetter <david(at)fetter(dot)org> wrote:
> >> >> > Could someone please commit this? :)
> >> >>
> >> >> Eh... was there some reason you didn't add it to the
> >> >> CommitFest app?
> >> >
> >> > I forgot.
> >>
> >> A fair excuse.  :-)
> >>
> >> >> Because that's what I work from.
> >> >
> >> > It's pretty trivial, but I don't feel comfortable adding it
> >> > after the close. :/
> >>
> >> So add it to the next one, and we'll get it then if nobody picks
> >> it up sooner...
> >
> > Given its small and isolated nature, I was hoping we could get
> > this in sooner rather than later.  As I understand it, CFs are
> > there to review patches that take significant effort for even a
> > committer to understand, so this doesn't really fit that model.
>
> Well, then add it to this one if you think that's more appropriate.

Done. :)

> My point is simple: I review patches because they are in the CF
> queue. Your point seems to be: put mine ahead of the others, and
> review it immediately. Someone else may very well be willing to do
> that; I'm not.

Fair enough.

> > David (refraining from mentioning anything about the time taken
> > today to discuss this vs. the time it would have taken to push the
> > thing)
>
> Mention anything you want. My guess is it would take me an hour.
> You're certainly right that this discussion is a waste of time, but
> possibly not for the reasons you are supposing.

LOL!

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Excerpts from David Fetter's message of dom nov 21 21:17:12 -0300 2010:

> Given its small and isolated nature, I was hoping we could get this in
> sooner rather than later. As I understand it, CFs are there to review
> patches that take significant effort for even a committer to
> understand, so this doesn't really fit that model.

It's a 300 line patch -- far from small. It takes me 10 minutes to go
through a 3 line change in docs. Maybe that makes me slow, but then if
I'm too slow for your patch, I probably don't want to handle it anyhow.

Please let's try to not work around the process.

(Also, like Robert, I work from the CF app, not from the mailing list
anymore. It''s far more convenient -- at least when people follow the
rules.)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Fri, Oct 29, 2010 at 10:33 AM, David Fetter <david(at)fetter(dot)org> wrote:
> That seems like a matter for a separate patch.  Looking this over, I
> found I'd created a query that can never get used, so please find
> enclosed the next version of the patch :)

I like "deletables" better than "deleteables" for
Query_for_list_of_deleteables. Sources: dictionary.com and a git grep
through the rest of the PG source.

Josh

On Tue, Nov 23, 2010 at 09:37:57PM -0500, Josh Kupershmidt wrote:
> On Fri, Oct 29, 2010 at 10:33 AM, David Fetter <david(at)fetter(dot)org> wrote:
> > That seems like a matter for a separate patch.  Looking this over, I
> > found I'd created a query that can never get used, so please find
> > enclosed the next version of the patch :)
>
> I like "deletables" better than "deleteables" for
> Query_for_list_of_deleteables. Sources: dictionary.com and a git grep
> through the rest of the PG source.

Thanks for the review.

Please find attached a patch changing both this and "updateable" to
"updatable," also per the very handy git grep I just learned about :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Nov 23, 2010 at 10:21 PM, David Fetter <david(at)fetter(dot)org> wrote:
> Please find attached a patch changing both this and "updateable" to
> "updatable," also per the very handy git grep I just learned about :)

I looked a little more at this patch today. I didn't find any serious
problems, though it would have helped me (and maybe other reviewers)
to have a comprehensive list of the SQL statements which the patch
implements autocompletion for.

Not sure how hard this would be to add in, but the following gets
autocompleted with an INSERT:
CREATE TRIGGER mytrg BEFORE IN[TAB]
while the following doesn't find any autocompletions:
CREATE TRIGGER mytrg BEFORE INSERT OR UP[TAB]

Also, this existed before the patch, but this SQL:
CREATE TRIGGER mytrg INSTEAD OF INSERT ON [TAB]
autocompletes to:
CREATE TRIGGER mytrg INSTEAD OF INSERT ON SET

not sure how that "SET" is getting in there -- some incorrect tab
completion match?

Josh

On Wed, Nov 24, 2010 at 11:01:28PM -0500, Josh Kupershmidt wrote:
> On Tue, Nov 23, 2010 at 10:21 PM, David Fetter <david(at)fetter(dot)org> wrote:
> > Please find attached a patch changing both this and "updateable" to
> > "updatable," also per the very handy git grep I just learned about :)
>
> I looked a little more at this patch today. I didn't find any serious
> problems, though it would have helped me (and maybe other reviewers)
> to have a comprehensive list of the SQL statements which the patch
> implements autocompletion for.

OK, will add those to the description.

> Not sure how hard this would be to add in, but the following gets
> autocompleted with an INSERT:
> CREATE TRIGGER mytrg BEFORE IN[TAB]
> while the following doesn't find any autocompletions:
> CREATE TRIGGER mytrg BEFORE INSERT OR UP[TAB]

I might be able to hack something like this together :)

> Also, this existed before the patch, but this SQL:
> CREATE TRIGGER mytrg INSTEAD OF INSERT ON [TAB]
> autocompletes to:
> CREATE TRIGGER mytrg INSTEAD OF INSERT ON SET
>
> not sure how that "SET" is getting in there -- some incorrect tab
> completion match?

Very likely. At some point, we will have to redo this stuff entirely
with a not-yet-invented parser API which will require a live
connection to the database in order to work correctly.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, Nov 24, 2010 at 12:21, David Fetter <david(at)fetter(dot)org> wrote:
> Please find attached a patch changing both this and "updateable" to
> "updatable," also per the very handy git grep I just learned about :)

I think the patch has two issues to be fixed.

It expands all tables (and views) in tab-completion after INSERT,
UPDATE, and DELETE FROM. Is it an intended change? IMHO, I don't
want to expand any schema because my console is filled with system
tables and duplicated tables with or without schema names :-( .

(original)
=# INSERT INTO [tab]
information_schema. pg_temp_1. pg_toast_temp_1. tbl
pg_catalog. pg_toast. public.

(patched)
=# INSERT INTO [tab]
Display all 113 possibilities? (y or n)

Also, event names are not completed after INSTEAD OF:

=# CREATE TRIGGER trg BEFORE [tab]
DELETE INSERT TRUNCATE UPDATE
=# CREATE TRIGGER trg INSTEAD OF [tab]
(no candidates)

--
Itagaki Takahiro

On Tue, Nov 30, 2010 at 05:48:04PM +0900, Itagaki Takahiro wrote:
> On Wed, Nov 24, 2010 at 12:21, David Fetter <david(at)fetter(dot)org> wrote:
> > Please find attached a patch changing both this and "updateable" to
> > "updatable," also per the very handy git grep I just learned about :)
>
> I think the patch has two issues to be fixed.
>
> It expands all tables (and views) in tab-completion after INSERT,
> UPDATE, and DELETE FROM. Is it an intended change? IMHO, I don't
> want to expand any schema because my console is filled with system
> tables and duplicated tables with or without schema names :-( .
>
> (original)
> =# INSERT INTO [tab]
> information_schema. pg_temp_1. pg_toast_temp_1. tbl
> pg_catalog. pg_toast. public.
>
> (patched)
> =# INSERT INTO [tab]
> Display all 113 possibilities? (y or n)

Yes. I believe that the question of having INSERT INTO [tab] check
for permissions is a separate issue from this patch. This patch does
bring the question of whether it *should* check such permission into
more focus, though. Whether it should is probably a matter for a
separate thread, though. I could create arguments in both directions.

> Also, event names are not completed after INSTEAD OF:
>
> =# CREATE TRIGGER trg BEFORE [tab]
> DELETE INSERT TRUNCATE UPDATE
> =# CREATE TRIGGER trg INSTEAD OF [tab]
> (no candidates)

This one's fixed in the attached patch, although subsequent events, as
in

=# CREATE TRIGGER trg BEFORE INSERT OR [tab]

are not.

Thanks for your review :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Nov 30, 2010 at 18:18, David Fetter <david(at)fetter(dot)org> wrote:
>> It expands all tables (and views) in tab-completion after INSERT,
>> UPDATE, and DELETE FROM.  Is it an intended change?

I found it was a simple bug; we need ( ) around selcondition.

In addition, I modified your patch a bit:

* I added a separated CREATE TRIGGER INSTEAD OF if-branch
because it doesn't accept tables actually; it only accepts
views. OTOH, BEFORE or AFTER only accepts tables.

* I think "t.tgtype & (1 << N) <> 0" is more natural
bit operation than "t.tgtype | (1 << N) = t.tgtype".

Patch attached. If you think my changes are ok,
please change the patch status to "Ready for Committer".

--
Itagaki Takahiro

On Tue, Nov 30, 2010 at 08:13:37PM +0900, Itagaki Takahiro wrote:
> On Tue, Nov 30, 2010 at 18:18, David Fetter <david(at)fetter(dot)org> wrote:
> >> It expands all tables (and views) in tab-completion after INSERT,
> >> UPDATE, and DELETE FROM.  Is it an intended change?
>
> I found it was a simple bug; we need ( ) around selcondition.

Oh. Heh. Thanks for tracking this down.

> In addition, I modified your patch a bit:
>
> * I added a separated CREATE TRIGGER INSTEAD OF if-branch
> because it doesn't accept tables actually; it only accepts
> views. OTOH, BEFORE or AFTER only accepts tables.

OK

> * I think "t.tgtype & (1 << N) <> 0" is more natural
> bit operation than "t.tgtype | (1 << N) = t.tgtype".

OK

> Patch attached. If you think my changes are ok,
> please change the patch status to "Ready for Committer".

Done :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Nov 30, 2010 at 9:15 AM, David Fetter <david(at)fetter(dot)org> wrote:
>> Patch attached. If you think my changes are ok,
>> please change the patch status to "Ready for Committer".
>
> Done :)

I have committed part of this patch. The rest is attached. I don't
know that there's any problem with it, but I ran out of steam.

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

On Mon, Dec 13, 2010 at 10:48:54PM -0500, Robert Haas wrote:
> On Tue, Nov 30, 2010 at 9:15 AM, David Fetter <david(at)fetter(dot)org> wrote:
> >> Patch attached. If you think my changes are ok,
> >> please change the patch status to "Ready for Committer".
> >
> > Done :)
>
> I have committed part of this patch.

Great!

> The rest is attached. I don't know that there's any problem with
> it, but I ran out of steam.

The issue with not committing it is that having a two-word condition
(INSTEAD OF vs. BEFORE or AFTER) means that thing that know about
preceding BEFORE or AFTER now have to look one word further backward,
at least as tab completion works now.

That we're in the position of having prevN_wd for N = 1..5 as the
current code exists is a sign that we need to refactor the whole
thing, as you've suggested before.

I'll work up a design and prototype for this by this weekend.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter wrote:
> That we're in the position of having prevN_wd for N = 1..5 as the
> current code exists is a sign that we need to refactor the whole
> thing, as you've suggested before.
>
> I'll work up a design and prototype for this by this weekend.
>

Great. I don't think issues around tab completion are enough to block
the next alpha though, and it sounds like the next stage of this needs
to gel a bit more before it will be ready to commit anyway. I'm going
to mark the remaining bits here as returned for now, and trust that
you'll continue chugging away on this so we can get it into the next CF
early.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services and Support www.2ndQuadrant.us
"PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books

On Thu, Dec 16, 2010 at 07:40:31AM -0500, Greg Smith wrote:
> David Fetter wrote:
> >That we're in the position of having prevN_wd for N = 1..5 as the
> >current code exists is a sign that we need to refactor the whole
> >thing, as you've suggested before.
> >
> >I'll work up a design and prototype for this by this weekend.
>
> Great. I don't think issues around tab completion are enough to
> block the next alpha though, and it sounds like the next stage of
> this needs to gel a bit more before it will be ready to commit
> anyway. I'm going to mark the remaining bits here as returned for
> now, and trust that you'll continue chugging away on this so we can
> get it into the next CF early.

Will chug.

"By this weekend" may have been a touch optimistic. "This weekend"
seems a little more realistic :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate