Thread: Tab complete for CREATE OR REPLACE TRIGGER statement

Hi, Hackers.

 Yesterday, OR REPLACE clause was provided to the CREATE TRIGGER statement, so I wrote a patch for tab completion for
thepsql command. 
TRIGGER adds tab completion to the CREATE OR REPLACE statement, and the CREATE TRIGGER and CREATE OR REPLACE TRIGGER
statementsare completed in the same way. 
I referred to the tab completion code of the CREATE RULE statement.

Regards,
Noriyoshi Shinoda

On Mon, Nov 16, 2020 at 08:12:05AM +0000, Shinoda, Noriyoshi (PN Japan FSI) wrote:
>  Yesterday, OR REPLACE clause was provided to the CREATE TRIGGER
> statement, so I wrote a patch for tab completion for the psql
> command.

Thanks, the logic looks fine.  I'll apply if there are no objections.
Please note that git diff --check and that the indentation does not
seem quite right, but that's easy enough to fix.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Nov 16, 2020 at 08:12:05AM +0000, Shinoda, Noriyoshi (PN Japan FSI) wrote:
>> Yesterday, OR REPLACE clause was provided to the CREATE TRIGGER
>> statement, so I wrote a patch for tab completion for the psql
>> command.

> Thanks, the logic looks fine.  I'll apply if there are no objections.
> Please note that git diff --check and that the indentation does not
> seem quite right, but that's easy enough to fix.

It's kind of depressing how repetitive the patch is.  There's
probably not much to be done about that in the short run, but
it seems to point up the need to start thinking about how to
refactor tab-complete.c more thoroughly.

            regards, tom lane

On Mon, Nov 16, 2020 at 09:14:51PM -0500, Tom Lane wrote:
> It's kind of depressing how repetitive the patch is.  There's
> probably not much to be done about that in the short run, but
> it seems to point up the need to start thinking about how to
> refactor tab-complete.c more thoroughly.

Agreed.  One thing that I'd think could help is a new wild card to
make some of the words conditional in the list of items.  But that may
be tricky once you consider the case of a group of words.

I don't think that this is a requirement for this thread, though.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> I don't think that this is a requirement for this thread, though.

Agreed, I'm not trying to block this patch.  Just wishing
there were a better way.

            regards, tom lane

On Mon, Nov 16, 2020 at 10:14:10PM -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> I don't think that this is a requirement for this thread, though.
>
> Agreed, I'm not trying to block this patch.  Just wishing
> there were a better way.

Okay.  I have tweaked a couple of things in the patch and applied it.
I am wondering if libreadline gives the possibility to implement an
optional grouping of words to complete, but diving into its
documentation I have not found something that we could use.
--
Michael

> Okay.  I have tweaked a couple of things in the patch and applied it.

Thank you so much.
The next time I post a patch, be careful with git --diff check and indentation.

Regards,
Noriyoshi Shinoda

-----Original Message-----
From: Michael Paquier [mailto:michael@paquier.xyz]
Sent: Wednesday, November 18, 2020 2:07 PM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Shinoda, Noriyoshi (PN Japan FSI) <noriyoshi.shinoda@hpe.com>; pgsql-hackers@lists.postgresql.org
Subject: Re: Tab complete for CREATE OR REPLACE TRIGGER statement

On Mon, Nov 16, 2020 at 10:14:10PM -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> I don't think that this is a requirement for this thread, though.
>
> Agreed, I'm not trying to block this patch.  Just wishing there were a
> better way.

Okay.  I have tweaked a couple of things in the patch and applied it.
I am wondering if libreadline gives the possibility to implement an optional grouping of words to complete, but diving
intoits documentation I have not found something that we could use. 
--
Michael

On 2020-11-18 06:06, Michael Paquier wrote:
> On Mon, Nov 16, 2020 at 10:14:10PM -0500, Tom Lane wrote:
>> Michael Paquier <michael@paquier.xyz> writes:
>>> I don't think that this is a requirement for this thread, though.
>> 
>> Agreed, I'm not trying to block this patch.  Just wishing
>> there were a better way.
> 
> Okay.  I have tweaked a couple of things in the patch and applied it.
> I am wondering if libreadline gives the possibility to implement an
> optional grouping of words to complete, but diving into its
> documentation I have not found something that we could use.

To me the code looks like a prime candidate for "data-driven" 
refactoring.

It should be redone as generic code that reads a table of rules with
params and then checks and applies each. Thus the repetitive code would
be replaced by a bit more generic code and a lot of code-free data.

-- 
Best regards,

Tels

Tels <nospam-pg-abuse@bloodgate.com> writes:
> On 2020-11-18 06:06, Michael Paquier wrote:
>> On Mon, Nov 16, 2020 at 10:14:10PM -0500, Tom Lane wrote:
>>> Agreed, I'm not trying to block this patch.  Just wishing
>>> there were a better way.

> To me the code looks like a prime candidate for "data-driven" 
> refactoring.
> It should be redone as generic code that reads a table of rules with
> params and then checks and applies each. Thus the repetitive code would
> be replaced by a bit more generic code and a lot of code-free data.

In the past I've looked into whether the rules could be autogenerated
from the backend's grammar.  It did not look very promising though.
The grammar isn't really factorized appropriately -- for instance,
tab-complete has a lot of knowledge about which kinds of objects can
be named in particular places, while the Bison productions only know
that's a "name".  Still, the precedent of ECPG suggests it might be
possible to process the grammar rules somehow to get to a useful
result.

            regards, tom lane

Hello Tom,

On 2020-11-18 16:49, Tom Lane wrote:
> Tels <nospam-pg-abuse@bloodgate.com> writes:
>> On 2020-11-18 06:06, Michael Paquier wrote:
>>> On Mon, Nov 16, 2020 at 10:14:10PM -0500, Tom Lane wrote:
>>>> Agreed, I'm not trying to block this patch.  Just wishing
>>>> there were a better way.
> 
>> To me the code looks like a prime candidate for "data-driven"
>> refactoring.
>> It should be redone as generic code that reads a table of rules with
>> params and then checks and applies each. Thus the repetitive code 
>> would
>> be replaced by a bit more generic code and a lot of code-free data.
> 
> In the past I've looked into whether the rules could be autogenerated
> from the backend's grammar.  It did not look very promising though.
> The grammar isn't really factorized appropriately -- for instance,
> tab-complete has a lot of knowledge about which kinds of objects can
> be named in particular places, while the Bison productions only know
> that's a "name".  Still, the precedent of ECPG suggests it might be
> possible to process the grammar rules somehow to get to a useful
> result.

Hm, that would be even better, for now I was just thinking that
code like this:

     IF RULE_A_MATCHES THEN
     DO STUFF A
     ELSE IF RULE_B_MATCHES THEN
     DO_STUFF_B
     ELSE IF RULE_C_MATCHES THEN
     DO_STUFF_C
     ...

should be replaced by

     RULE_A MATCH STUFF
     RULE_B MATCH STUFF
     RULE_C MATCH STUFF
     ...

     FOREACH RULE DO
       IF RULE.MATCH THEN
         DO RULE.STUFF
     END FOREACH

Even if the rules would be manually created (converted from the current
code), that would be more compact and probably less error-prone.

Creating the rule automatically turns this into a completely different
story.

-- 
Best regards,

Tels