Thread: Tab completion for view triggers in psql
Folks, Please find attached patch for $subject :) Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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
Attachment
On Tue, Oct 26, 2010 at 5:01 AM, David Fetter <david@fetter.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@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@fetter.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@fetter.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@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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@fetter.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@fetter.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@fetter.org> http://fetter.org/
> Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
> Skype: davidfetter XMPP: david.fetter@gmail.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@postgresql.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@fetter.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@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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
Attachment
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@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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
Attachment
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@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@fetter.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@fetter.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@fetter.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@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@fetter.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@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@fetter.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@fetter.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@fetter.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@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@commandprompt.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@fetter.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@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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
Attachment
On Tue, Nov 23, 2010 at 10:21 PM, David Fetter <david@fetter.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@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@fetter.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@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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
Attachment
On Tue, Nov 30, 2010 at 18:18, David Fetter <david@fetter.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
Attachment
On Tue, Nov 30, 2010 at 08:13:37PM +0900, Itagaki Takahiro wrote: > On Tue, Nov 30, 2010 at 18:18, David Fetter <david@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@fetter.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
Attachment
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@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@2ndQuadrant.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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