Subject: Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion
To: Shinya11.Kato@nttdata.com, sawada.mshk@gmail.com
Cc: pgsql-hackers@postgresql.org
References: 
 <b0e4c5c53ef84c5395524f5056fc71f0@MP-MSGSS-MBX001.msg.nttdata.co.jp>
 <CAD21AoBRw9FY6oNj8ubY7XEwJ9Bprfehu88_ePbCfH93JLxWsw@mail.gmail.com>
 <7559e8ee8a0644c8aad79837092143c7@MP-MSGSS-MBX001.msg.nttdata.co.jp>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <0eab839e-5827-e517-4abf-00b3e581cd45@oss.nttdata.com>
Date: Tue, 5 Jan 2021 18:08:21 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0)
 Gecko/20100101 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: 
 <7559e8ee8a0644c8aad79837092143c7@MP-MSGSS-MBX001.msg.nttdata.co.jp>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2021/01/05 15:02, Shinya11.Kato@nttdata.com wrote:
> Thank you for your review!
> I fixed some codes and attach a new patch.

Thanks for updating the patch!

+#define Query_for_list_of_cursors \
+" SELECT name FROM pg_cursors"\

This query should be the following?

" SELECT pg_catalog.quote_ident(name) "\
"   FROM pg_catalog.pg_cursors "\
"  WHERE substring(pg_catalog.quote_ident(name),1,%d)='%s'"

+/* CLOSE */
+	else if (Matches("CLOSE"))
+		COMPLETE_WITH_QUERY(Query_for_list_of_cursors
+							" UNION ALL SELECT 'ALL'");

"UNION ALL" should be "UNION"?

+		COMPLETE_WITH_QUERY(Query_for_list_of_cursors
+							" UNION SELECT 'ABSOLUTE'"
+							" UNION SELECT 'BACKWARD'"
+							" UNION SELECT 'FORWARD'"
+							" UNION SELECT 'RELATIVE'"
+							" UNION SELECT 'ALL'"
+							" UNION SELECT 'NEXT'"
+							" UNION SELECT 'PRIOR'"
+							" UNION SELECT 'FIRST'"
+							" UNION SELECT 'LAST'"
+							" UNION SELECT 'FROM'"
+							" UNION SELECT 'IN'");

This change makes psql unexpectedly output "FROM" and "IN" just after "FETCH".

> 
>> When I applied the patch, I got the following whitespace warnings:
>>
>> $ git apply ~/patches/fix_tab_complete_close_fetch_move.patch
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:40:
>> indent with spaces.
>>         COMPLETE_WITH_QUERY(Query_for_list_of_cursors
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:41:
>> indent with spaces.
>>                             " UNION SELECT 'ABSOLUTE'"
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:42:
>> indent with spaces.
>>                             " UNION SELECT 'BACKWARD'"
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:43:
>> indent with spaces.
>>                             " UNION SELECT 'FORWARD'"
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:44:
>> indent with spaces.
>>                             " UNION SELECT 'RELATIVE'"
>> warning: squelched 19 whitespace errors
>> warning: 24 lines add whitespace errors.
>>
>> I recommend you checking whitespaces or running pgindent.
> 
> Thank you for your advice and I have corrected it.
> 
>> Here are some comments:
>>
>>     /*
>> -    * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD,
>> RELATIVE, ALL,
>> -    * NEXT, PRIOR, FIRST, LAST
>> +    * Complete FETCH with a list of cursors and one of ABSOLUTE,
>> BACKWARD, FORWARD, RELATIVE, ALL,
>> +    * NEXT, PRIOR, FIRST, LAST, FROM, IN
>>      */
>>
>> Maybe I think the commend should say:
>>
>> +    * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD,
>> RELATIVE, ALL,
>> +    * NEXT, PRIOR, FIRST, LAST, FROM, IN, and a list of cursors
>>
>> Other updates of the comment seem to have the same issue.
>>
>> Or I think we can omit the details from the comment since it seems redundant
>> information. We can read it from the arguments of the following
>> COMPLETE_WITH_QUERY().
> 
> It certainly seems redundant, so I deleted them.

I think that it's better to update and keep those comments rather than removing them.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION