[PATCH] Tab complete EXECUTE FUNCTION for CREATE (EVENT) TRIGGER
Hi hackers,
The last-minute change for CREATE (EVENT) TRIGGER to accept EXECUTE
FUNCTION as well as EXECUTE PROCEDURE did not update the tab completion
in psql to match. Please find attached two patches that do this for
CREATE TRIGGER and CREATE EVENT TRIGGER, respectively.
To keep the duplication minimal, I've changed it from completing EXECUTE
PROCEDURE as a single thing to just EXECUTE, and then completing
FUNCTION or FUNCTION and PROCEDURE after that depending on the server
version.
- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
Hi hackers,
The last-minute change for CREATE (EVENT) TRIGGER to accept EXECUTE
FUNCTION as well as EXECUTE PROCEDURE did not update the tab completion
in psql to match. Please find attached two patches that do this for
CREATE TRIGGER and CREATE EVENT TRIGGER, respectively.
Added to the current commitfest: https://commitfest.postgresql.org/20/1837/
- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law
On Tue, Oct 23, 2018 at 12:26:35PM +0100, Dagfinn Ilmari Mannsåker wrote:
The last-minute change for CREATE (EVENT) TRIGGER to accept EXECUTE
FUNCTION as well as EXECUTE PROCEDURE did not update the tab completion
in psql to match. Please find attached two patches that do this for
CREATE TRIGGER and CREATE EVENT TRIGGER, respectively.
Yes, it would be nice to fix that.
To keep the duplication minimal, I've changed it from completing EXECUTE
PROCEDURE as a single thing to just EXECUTE, and then completing
FUNCTION or FUNCTION and PROCEDURE after that depending on the server
version.
+ else if (HeadMatches("CREATE", "EVENT", "TRIGGER") && TailMatches("EXECUTE"))
+ if (pset.sversion >= 110000)
+ COMPLETE_WITH("FUNCTION", "PROCEDURE");
+ else
+ COMPLETE_WITH("PROCEDURE");PROCEDURE is documented as deprecated as of v11 for CREATE TRIGGER and
CREATE EVENT TRIGGER. Wouldn't it be better to just complete with
FUNCTION for a v11 or newer server? I think that we want to encourage
users to use EXECUTE FUNCTION if possible.
--
Michael
On Wed, Oct 24, 2018 at 08:43:05AM +0900, Michael Paquier wrote:
On Tue, Oct 23, 2018 at 12:26:35PM +0100, Dagfinn Ilmari Manns�ker wrote:
The last-minute change for CREATE (EVENT) TRIGGER to accept EXECUTE
FUNCTION as well as EXECUTE PROCEDURE did not update the tab completion
in psql to match. Please find attached two patches that do this for
CREATE TRIGGER and CREATE EVENT TRIGGER, respectively.Yes, it would be nice to fix that.
To keep the duplication minimal, I've changed it from completing EXECUTE
PROCEDURE as a single thing to just EXECUTE, and then completing
FUNCTION or FUNCTION and PROCEDURE after that depending on the server
version.+ else if (HeadMatches("CREATE", "EVENT", "TRIGGER") && TailMatches("EXECUTE")) + if (pset.sversion >= 110000) + COMPLETE_WITH("FUNCTION", "PROCEDURE"); + else + COMPLETE_WITH("PROCEDURE");PROCEDURE is documented as deprecated as of v11 for CREATE TRIGGER
and CREATE EVENT TRIGGER. Wouldn't it be better to just complete
with FUNCTION for a v11 or newer server? I think that we want to
encourage users to use EXECUTE FUNCTION if possible.
+1 for not completing with syntax we've just deprecated.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
David Fetter <david@fetter.org> writes:
On Wed, Oct 24, 2018 at 08:43:05AM +0900, Michael Paquier wrote:
On Tue, Oct 23, 2018 at 12:26:35PM +0100, Dagfinn Ilmari Mannsåker wrote:
The last-minute change for CREATE (EVENT) TRIGGER to accept EXECUTE
FUNCTION as well as EXECUTE PROCEDURE did not update the tab completion
in psql to match. Please find attached two patches that do this for
CREATE TRIGGER and CREATE EVENT TRIGGER, respectively.Yes, it would be nice to fix that.
To keep the duplication minimal, I've changed it from completing EXECUTE
PROCEDURE as a single thing to just EXECUTE, and then completing
FUNCTION or FUNCTION and PROCEDURE after that depending on the server
version.+ else if (HeadMatches("CREATE", "EVENT", "TRIGGER") && TailMatches("EXECUTE")) + if (pset.sversion >= 110000) + COMPLETE_WITH("FUNCTION", "PROCEDURE"); + else + COMPLETE_WITH("PROCEDURE");PROCEDURE is documented as deprecated as of v11 for CREATE TRIGGER
and CREATE EVENT TRIGGER. Wouldn't it be better to just complete
with FUNCTION for a v11 or newer server? I think that we want to
encourage users to use EXECUTE FUNCTION if possible.+1 for not completing with syntax we've just deprecated.
Fair point. I was unsure about whether to complete every supported
variant or just the new one. Updated patches attached.
- ilmari
--
"I use RMS as a guide in the same way that a boat captain would use
a lighthouse. It's good to know where it is, but you generally
don't want to find yourself in the same spot." - Tollef Fog Heen
On Wed, Oct 24, 2018 at 10:36:41AM +0100, Dagfinn Ilmari Mannsåker wrote:
Fair point. I was unsure about whether to complete every supported
variant or just the new one. Updated patches attached.
One problem with this approach is that a user needs to use twice tab.
The first time is to show "EXECUTE", and the second time is to show
automatically "PROCEDURE" or "FUNCTION" automatically. The patch as it
stands is less complicated, but I can imagine as well that what's
propose could confuse people into thinking that they need to type
something after EXECUTE has showed up on the prompt. I vote for
simplicity and there are other code paths where completion for one
element only is done. Any objections?
+ else if (HeadMatches("CREATE", "TRIGGER") && TailMatches("WHEN", "(*)"))
+ COMPLETE_WITH("EXECUTE");It seems to me that this should be removed, it would fail at parsing if
completed.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Wed, Oct 24, 2018 at 10:36:41AM +0100, Dagfinn Ilmari Mannsåker wrote:
Fair point. I was unsure about whether to complete every supported
variant or just the new one. Updated patches attached.
One problem with this approach is that a user needs to use twice tab.
The first time is to show "EXECUTE", and the second time is to show
automatically "PROCEDURE" or "FUNCTION" automatically.
Yeah. Why don't we keep the existing behavior of completing both
words at once, but make it server-version-dependent which completion
you get?
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes:
Michael Paquier <michael@paquier.xyz> writes:
On Wed, Oct 24, 2018 at 10:36:41AM +0100, Dagfinn Ilmari Mannsåker wrote:
Fair point. I was unsure about whether to complete every supported
variant or just the new one. Updated patches attached.One problem with this approach is that a user needs to use twice tab.
The first time is to show "EXECUTE", and the second time is to show
automatically "PROCEDURE" or "FUNCTION" automatically.Yeah. Why don't we keep the existing behavior of completing both
words at once, but make it server-version-dependent which completion
you get?
I did that initially, but because COMPLETE_WITH() requres constant
arguments, I had to repeat the whole list with just changing PROCEDURE
to FUNCTION, which I thought was undesirably repetitive. If there's a
more concise alternative to the below, or the consensus is that saving
one TAB press is worth it, I'll change it.
else if (HeadMatches("CREATE", "TRIGGER") && TailMatches("ON", MatchAny))
if (pset.sversion >= 110000)
COMPLETE_WITH("NOT DEFERRABLE", "DEFERRABLE", "INITIALLY",
"REFERENCING", "FOR", "WHEN (", "EXECUTE FUNCTION");
else
COMPLETE_WITH("NOT DEFERRABLE", "DEFERRABLE", "INITIALLY",
"REFERENCING", "FOR", "WHEN (", "EXECUTE PROCEDURE");
- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law
ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
Tom Lane <tgl@sss.pgh.pa.us> writes:
Yeah. Why don't we keep the existing behavior of completing both
words at once, but make it server-version-dependent which completion
you get?
I did that initially, but because COMPLETE_WITHc() requres constant
arguments, I had to repeat the whole list with just changing PROCEDURE
to FUNCTION, which I thought was undesirably repetitive. If there's a
more concise alternative to the below, or the consensus is that saving
one TAB press is worth it, I'll change it.
else if (HeadMatches("CREATE", "TRIGGER") && TailMatches("ON", MatchAny))
if (pset.sversion >= 110000)
COMPLETE_WITH("NOT DEFERRABLE", "DEFERRABLE", "INITIALLY",
"REFERENCING", "FOR", "WHEN (", "EXECUTE FUNCTION");
else
COMPLETE_WITH("NOT DEFERRABLE", "DEFERRABLE", "INITIALLY",
"REFERENCING", "FOR", "WHEN (", "EXECUTE PROCEDURE");
Well, that's not beautiful, but there aren't so many alternatives
that it's really unmaintainable. I think it's probably better than
requiring an additional TAB-press.
regards, tom lane
Michael Paquier <michael@paquier.xyz> writes:
+ else if (HeadMatches("CREATE", "TRIGGER") && TailMatches("WHEN", "(*)")) + COMPLETE_WITH("EXECUTE");It seems to me that this should be removed, it would fail at parsing if
completed.
The * is a wildcard, so it completes EXECUTE after CREATE TRIGGER … WHEN
(<condition>), which is perfectly valid. Using * in the middle of an
alternative is a fairly new feature, added by Tom a month ago in commit
121213d9d8527f880f153e4a032ee1a4cd43833f.
- ilmari
--
"I use RMS as a guide in the same way that a boat captain would use
a lighthouse. It's good to know where it is, but you generally
don't want to find yourself in the same spot." - Tollef Fog Heen
On Thu, Oct 25, 2018 at 12:25:33PM +0100, Dagfinn Ilmari Mannsåker wrote:
I did that initially, but because COMPLETE_WITH() requres constant
arguments, I had to repeat the whole list with just changing PROCEDURE
to FUNCTION, which I thought was undesirably repetitive. If there's a
more concise alternative to the below, or the consensus is that saving
one TAB press is worth it, I'll change it.else if (HeadMatches("CREATE", "TRIGGER") && TailMatches("ON", MatchAny))
if (pset.sversion >= 110000)
COMPLETE_WITH("NOT DEFERRABLE", "DEFERRABLE", "INITIALLY",
"REFERENCING", "FOR", "WHEN (", "EXECUTE FUNCTION");
else
COMPLETE_WITH("NOT DEFERRABLE", "DEFERRABLE", "INITIALLY",
"REFERENCING", "FOR", "WHEN (", "EXECUTE PROCEDURE");
[thinking]
To keep the code simple, you could do something like that, by checking
the head keywords for a match with CREATE TRIGGER, and then move all the
existing conditions within it:
else if (HeadMatches("CREATE", "TRIGGER", MatchAny))
{
char *execute_keyword;
if (pset.sversion >= 110000)
execute_keyword = "EXECUTE FUNCTION";
else
execute_keyword = "EXECUTE PROCEDURE";
if (TailMatches("CREATE", "TRIGGER", MatchAny))
COMPLETE_WITH("BEFORE", "AFTER", "INSTEAD OF");
[...]
else if (the other existing conditions)
blah and use execute_keyword in the lists;
}
If we do the automatic completion of both words at the same time, let's
put only in a single place the version-based switch. This method costs
an extra match check on the header keywords when CREATE TRIGGER matches,
but it allows all the other checks to skip steps, which is actually a
win for the rest.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
To keep the code simple, you could do something like that, by checking
the head keywords for a match with CREATE TRIGGER, and then move all the
existing conditions within it:
...
char *execute_keyword;
...
blah and use execute_keyword in the lists;
Don't think that works unless we remove some of the "const" annotation
in COMPLETE_WITH; which I'd rather not.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes:
ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
Tom Lane <tgl@sss.pgh.pa.us> writes:
Yeah. Why don't we keep the existing behavior of completing both
words at once, but make it server-version-dependent which completion
you get?I did that initially, but because COMPLETE_WITHc() requres constant
arguments, I had to repeat the whole list with just changing PROCEDURE
to FUNCTION, which I thought was undesirably repetitive. If there's a
more concise alternative to the below, or the consensus is that saving
one TAB press is worth it, I'll change it.else if (HeadMatches("CREATE", "TRIGGER") && TailMatches("ON", MatchAny))
if (pset.sversion >= 110000)
COMPLETE_WITH("NOT DEFERRABLE", "DEFERRABLE", "INITIALLY",
"REFERENCING", "FOR", "WHEN (", "EXECUTE FUNCTION");
else
COMPLETE_WITH("NOT DEFERRABLE", "DEFERRABLE", "INITIALLY",
"REFERENCING", "FOR", "WHEN (", "EXECUTE PROCEDURE");Well, that's not beautiful, but there aren't so many alternatives
that it's really unmaintainable. I think it's probably better than
requiring an additional TAB-press.
Okay, revised patches attached. I also tweaked the CREATE EVENT TRIGGER
completion to accept multple <filter_varaible> IN (<filter_value>)
conditions seprated by AND in the WHEN clause (but not to suggest that,
since we only actually support one <filter_variable>, TAG).
- ilmari
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen
On Thu, Oct 25, 2018 at 12:52:01PM +0100, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
To keep the code simple, you could do something like that, by checking
the head keywords for a match with CREATE TRIGGER, and then move all the
existing conditions within it:
...
char *execute_keyword;
...
blah and use execute_keyword in the lists;Don't think that works unless we remove some of the "const" annotation
in COMPLETE_WITH; which I'd rather not.
Indeed, gcc just complained about the thing not being a constant after
testing, so please let me discard this suggestion.
--
Michael
On Thu, Oct 25, 2018 at 05:02:32PM +0100, Dagfinn Ilmari Mannsåker wrote:
Okay, revised patches attached. I also tweaked the CREATE EVENT TRIGGER
completion to accept multple <filter_varaible> IN (<filter_value>)
conditions seprated by AND in the WHEN clause (but not to suggest that,
since we only actually support one <filter_variable>, TAG).
Committed 0001 now which adds tab completion for CREATE TRIGGER.
Something you missed is that we should still be able to complete with
PROCEDURE or FUNCTION (depending on the backend version) if CREATE
TRIGGER .. EXECUTE is present on screen. The priority is given to
complete with both EXECUTE PROCEDURE and EXECUTE FUNCTION at the same
time, but you also should have the intermediate state. I also added
some brackets for clarity, and a comment about the grammar selection.
--
Michael
On Fri, Oct 26, 2018 at 09:31:48AM +0900, Michael Paquier wrote:
Committed 0001 now which adds tab completion for CREATE TRIGGER.
Something you missed is that we should still be able to complete with
PROCEDURE or FUNCTION (depending on the backend version) if CREATE
TRIGGER .. EXECUTE is present on screen. The priority is given to
complete with both EXECUTE PROCEDURE and EXECUTE FUNCTION at the same
time, but you also should have the intermediate state. I also added
some brackets for clarity, and a comment about the grammar selection.
And 0002 is committed as well. Thanks for the patches!
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Fri, Oct 26, 2018 at 09:31:48AM +0900, Michael Paquier wrote:
Committed 0001 now which adds tab completion for CREATE TRIGGER.
And 0002 is committed as well. Thanks for the patches!
The committed patches look sane to me, but should we back-patch into 11?
This isn't quite a bug fix maybe, but it's inconsistent if v11 server &
docs recommend this syntax while v11 psql doesn't produce it.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes:
Michael Paquier <michael@paquier.xyz> writes:
On Fri, Oct 26, 2018 at 09:31:48AM +0900, Michael Paquier wrote:
Committed 0001 now which adds tab completion for CREATE TRIGGER.
And 0002 is committed as well. Thanks for the patches!
Thanks for reviewing and committing!
The committed patches look sane to me, but should we back-patch into 11?
This isn't quite a bug fix maybe, but it's inconsistent if v11 server &
docs recommend this syntax while v11 psql doesn't produce it.
I was going to suggest backpatching it, as I consider it a bug in the
original implementation, if not critical. Making it harder for people
to use the recommended syntax than the deprecated one is not nice.
regards, tom lane
- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law
On Fri, Oct 26, 2018 at 11:15:19AM +0100, Dagfinn Ilmari Mannsåker wrote:
Tom Lane <tgl@sss.pgh.pa.us> writes:
The committed patches look sane to me, but should we back-patch into 11?
This isn't quite a bug fix maybe, but it's inconsistent if v11 server &
docs recommend this syntax while v11 psql doesn't produce it.I was going to suggest backpatching it, as I consider it a bug in the
original implementation, if not critical. Making it harder for people
to use the recommended syntax than the deprecated one is not nice.
I didn't think that this was much of a big deal as the deprecated
grammar is still supported on v11, but as both of you think in this
sense I am fine to patch REL_11_STABLE as well. Please just wait a
bit...
--
Michael
Tom Lane <tgl@sss.pgh.pa.us> writes:
Michael Paquier <michael@paquier.xyz> writes:
On Fri, Oct 26, 2018 at 09:31:48AM +0900, Michael Paquier wrote:
Committed 0001 now which adds tab completion for CREATE TRIGGER.
And 0002 is committed as well. Thanks for the patches!
The committed patches look sane to me, but should we back-patch into 11?
This isn't quite a bug fix maybe, but it's inconsistent if v11 server &
docs recommend this syntax while v11 psql doesn't produce it.
A related issue is whether we should change pg_get_triggerdef() to emit
the new syntax as well (in HEAD only, since it would break things that
parse the output).
- ilmari
--
"I use RMS as a guide in the same way that a boat captain would use
a lighthouse. It's good to know where it is, but you generally
don't want to find yourself in the same spot." - Tollef Fog Heen