Re: [pgAdmin][RM-2341]: Add menu option for starting PSQL

Hi Nikhil

Please rebase and send the patch again.

On Tue, May 25, 2021 at 2:52 PM Nikhil Mohite <
nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:

> Hi Akshay/ Team
>
> On Mon, May 24, 2021 at 9:19 PM Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Nikhil
>>
>> Following are the review comments:
>>
>> - Set "ENABLE_PSQL = False", PSQL button from browser tree and
>> context menu option should not be visible.
>> - Documentation screenshot should be in standard theme for
>> consistency, and check the size it's very large. Take the screenshot with
>> the new PSQL button on the browser tree.
>> - Update 'menu_bar.rst' and 'toolbar.rst' with new changes.
>> - Remove commented code (if any)
>> - Check SonarQube (I haven't run)
>>
>> Please find the updated patch, resolve all the review comments, and
> update the code to resolve the SonarQube issues.
>
>>
>> On Thu, May 20, 2021 at 2:52 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Thanks Nikhil. Can someone else review this version please?
>>>
>>> On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <
>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Dave/ Team,
>>>>
>>>> On Wed, May 19, 2021 at 1:43 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <
>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi Dave/ Team,
>>>>>>
>>>>>> On Tue, May 18, 2021 at 8:41 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <
>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi Dave/Team,
>>>>>>>
>>>>>>>
>>>>>>> On Mon, May 17, 2021 at 6:47 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <
>>>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>>>
>>>>>>>>> Hi Akshay/ Team,
>>>>>>>>>
>>>>>>>>> Please find the attached updated patch for the psql tool.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hmm, this version is also broken. There's a typo in
>>>>>>>> editor_template.html on line 138 - it splits a string across two lines
>>>>>>>> which throws an error. Having fixed that...
>>>>>>>>
>>>>>>>> I also note there's a lot of Javascript in that HTML file. That
>>>>>>>> should be pushed into the webpacked bundle I think, and not included inline
>>>>>>>> in HTML.
>>>>>>>>
>>>>>>> I have moved most of the code in the js file, few things are still
>>>>>>> in HTML.
>>>>>>>
>>>>>>
>>>>>> Hmm, yes - in particular, colours for the different themes. Please
>>>>>> move them into the css for the themes. You have a mix of style, layout and
>>>>>> code in this file which needs to be cleaned up.
>>>>>> xterm V3 onwards they have provided the API to set the theme and
>>>>>> other settings, earlier I tried with CSS to override the theme but couldn’t
>>>>>> able to apply the theme properly as some style get applied as in-line style
>>>>>> for the HTML, so used the API to set the theme.
>>>>>>
>>>>>
>>>>> OK, but either way we can't hard-code styles from themes in HTML
>>>>> templates for individual features; that way leads to madness.
>>>>>
>>>>> Perhaps Aditya or one of the other team members can give some
>>>>> assistance?
>>>>>
>>>> I have moved the color settings to the respective theme files. Aditya
>>>> helped in this.
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> Speaking of themes, the background colour for selected text doesn't
>>>>>>> seem right (it's barely visible) in the dark theme. Can you fix that to
>>>>>>> match the colouring in the SQL text boxes please?
>>>>>>>
>>>>>> I tried the default selection color from SQL for the dark and
>>>> standard themes but still, it was not readable so just updated the color
>>>> code with another color as follows.
>>>> 1. Dark Theme:
>>>> [image: Screenshot 2021-05-19 at 6.29.43 PM.png]
>>>> 2. High Contrast: (using default SQL selection color)
>>>> [image: Screenshot 2021-05-19 at 6.59.52 PM.png]
>>>> 3. Standard:
>>>> [image: image.png]
>>>> can we go with the colors or should we update it?
>>>>
>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> A couple of other things I noticed:
>>>>>>>>>
>>>>>>>>> - The button is enabled if the treeview has a Server selected. It
>>>>>>>>> could be argued that the query tool should do the same (defaulting to the
>>>>>>>>> maintenance database), however, that would be a separate change, and psql
>>>>>>>>> should be consistent with the query tool.
>>>>>>>>>
>>>>>>>> It is now consistent with the query tool.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> - If I do a "select * from pg_class;" I still get:
>>>>>>>>>
>>>>>>>>> postgres=# select * from pg_class;
>>>>>>>>> WARNING: terminal is not fully functional
>>>>>>>>>
>>>>>>>> I am not able to reproduce the warning for the terminal (I am
>>>>>>>> working on Catalina 10.15.7), I checked on browsers (chrome, firefox,
>>>>>>>> Safari) and also checked on local nwjs runtime but still not able to
>>>>>>>> reproduce the warning. but found one limitation:
>>>>>>>>
>>>>>>>
>>>>>>> It looks like that can be fixed by adding:
>>>>>>>
>>>>>>> env={'TERM': 'xterm'}
>>>>>>>
>>>>>> Added this in the environment when opening the psql panel.
>>>>
>>>>>
>>>>>>> to the subprocess.Popen() call.
>>>>>>>
>>>>>>> I noticed while I was playing with that, that you are passing the
>>>>>>> password as part of the connection string. As I've mentioned in the past,
>>>>>>> that is absolutely not acceptable; it will expose the password to all
>>>>>>> manner of tools (such as ps -ef). You *must* pass the password to psql
>>>>>>> using the PGPASSWORD environment variable.
>>>>>>>
>>>>>>>
>>>>>>>> if we try to load data from the table containing millions of
>>>>>>>> records, UI gets very slow.
>>>>>>>>
>>>>>>> Removed the password from the connection string and added
>>>> 'PGPASSWORD' in the environment.
>>>>
>>>>>
>>>>>>> Is xtermjs discarding the older buffer contents when it fills up?
>>>>>>> Can you tell where the memory usage is?
>>>>>>>
>>>>>> I checked the psql memory consumption in terminal and pgAdmin psql
>>>>>> tool memory consumption is the similar. Also tested the performance and
>>>>>> query execution timing is also similar.
>>>>>>
>>>>>
>>>>> OK, so there's probably not much we can do here.
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> - I'm sure using \q in the previous version displayed a message
>>>>>>>>> saying the session exited (the one on line 138 of editor_template.html). It
>>>>>>>>> no longer seems to do so.
>>>>>>>>>
>>>>>>>>
>>>>>>> In addition to the issue above, it looks like the \! blocking may
>>>>>>> have lost it's ability to ignore quoted strings:
>>>>>>>
>>>>>>> pgweb=# select '\!';
>>>>>>> ERROR: Shell commands are disabled in psql for security
>>>>>>>
>>>>>> Fixed the issue now it is consistent with the psql terminal.
>>>>
>>>>>
>>>>>>>
>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, May 11, 2021 at 3:40 PM Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi
>>>>>>>>>>>
>>>>>>>>>>> On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <
>>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi Nikhil
>>>>>>>>>>>>
>>>>>>>>>>>> Following are the review comments:
>>>>>>>>>>>>
>>>>>>>>>>>> *GUI specific*:
>>>>>>>>>>>>
>>>>>>>>>>>> - We need a panel icon for PSQL like query tool, we can
>>>>>>>>>>>> also add that on the browser tree toolbar.
>>>>>>>>>>>> - PSQL Tool menu should be visible for all the child nodes
>>>>>>>>>>>> of the database node. Follow the same as Query Tool.
>>>>>>>>>>>> - PSQL tab title should be only database server name as the
>>>>>>>>>>>> user can change the database/user from PSQL command, so it's been difficult
>>>>>>>>>>>> to update the tab title.
>>>>>>>>>>>> - PSQL connection is still open even if we disconnect the
>>>>>>>>>>>> database server from the browser tree.
>>>>>>>>>>>>
>>>>>>>>>>>> *Code specific:*
>>>>>>>>>>>>
>>>>>>>>>>>> - Remove an extra space from requirements.txt and
>>>>>>>>>>>> package.json
>>>>>>>>>>>> - Documentation needs to be updated to let the user know
>>>>>>>>>>>> from where the PSQL tool will open and on which node it is applicable.
>>>>>>>>>>>> - psql/__init__.py check there are so many unused imports
>>>>>>>>>>>> please remove them.
>>>>>>>>>>>> - We are not using cheroot so it should be removed from
>>>>>>>>>>>> requirements.txt and also remove the import statement from pgAdmin4.py
>>>>>>>>>>>> - Test cases are showing successful but actually, there are
>>>>>>>>>>>> some routing errors please check.
>>>>>>>>>>>>
>>>>>>>>>>>> A few other things I noticed:
>>>>>>>>>>>
>>>>>>>>>>> - I was prompted to enter a password. This should be passed in
>>>>>>>>>>> the environment to psql as it is for pg_dump etc.
>>>>>>>>>>> - There seems to be an issue with terminal compatibility (which
>>>>>>>>>>> I didn't have on my prototype):
>>>>>>>>>>>
>>>>>>>>>>> ml=# select * from pg_class;
>>>>>>>>>>> WARNING: terminal is not fully functional
>>>>>>>>>>> -[ RECORD 1
>>>>>>>>>>> ]-------+----------------------------------------------
>>>>>>>>>>> oid | 79354
>>>>>>>>>>> relname | housing
>>>>>>>>>>> ...
>>>>>>>>>>>
>>>>>>>>>>> - The panel should honour the styleguide. I'm running in dark
>>>>>>>>>>> mode, and see a jet black background. I would expect to see the same
>>>>>>>>>>> background/foreground colours as the treeview.
>>>>>>>>>>> - I spotted at least one print() statement that shouldn't be
>>>>>>>>>>> there (debug output should go through the logger) - psql/__init__.py:235
>>>>>>>>>>> - This seems suspect - why would there be a password in a
>>>>>>>>>>> connection string we've built? And why would it be xxx?
>>>>>>>>>>>
>>>>>>>>>>> if 'password=xxx' in conn_attr:
>>>>>>>>>>> conn_attr = conn_attr.replace('password=xxx', '')
>>>>>>>>>>>
>>>>>>>>>>> - There's a thick white line at the bottom of the panel, where a
>>>>>>>>>>> horizontal scrollbar might be if there was one.
>>>>>>>>>>> - The trailing semi-colon should be removed from: "ERROR: Shell
>>>>>>>>>>> commands are disabled in psql for security;"
>>>>>>>>>>>
>>>>>>>>>>> Once we're happy with the patch in general, I'll do a string
>>>>>>>>>>> review before committing. In particular, I want to be sure the text in
>>>>>>>>>>> config.py is appropriately worded.
>>>>>>>>>>>
>>>>>>>>>>> This is shaping up nicely! Good work.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, May 10, 2021 at 7:32 PM Nikhil Mohite <
>>>>>>>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Dave/ Team,
>>>>>>>>>>>>>
>>>>>>>>>>>>> PFA updated patch, sorry for the inconvenience, while cleanup
>>>>>>>>>>>>> I removed the unwanted libraries but forgot to remove the code related to
>>>>>>>>>>>>> them.
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Mon, May 10, 2021 at 7:10 PM Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <
>>>>>>>>>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Hackers,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Please find the attached patch for RM-2341
>>>>>>>>>>>>>>> <https://redmine.postgresql.org/issues/2341>: Add Menu
>>>>>>>>>>>>>>> option for starting PSQL.
>>>>>>>>>>>>>>> 1. Added new Option PSQL Tool in Tools menu.
>>>>>>>>>>>>>>> 2. Added the same option for Server and Database nodes from
>>>>>>>>>>>>>>> the tree view.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Unfortunately there's a trailing comma in package.json that
>>>>>>>>>>>>>> makes it invalid. If I fix that, then I get the error below, so I'm
>>>>>>>>>>>>>> guessing the intention was to actually include another package there?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ERROR in ./pgadmin/tools/psql/static/js/psql_module.js
>>>>>>>>>>>>>> 23:50-82
>>>>>>>>>>>>>> Module not found: Error: Can't resolve
>>>>>>>>>>>>>> 'local-echo-controller' in
>>>>>>>>>>>>>> '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js'
>>>>>>>>>>>>>> resolve 'local-echo-controller' in
>>>>>>>>>>>>>> '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js'
>>>>>>>>>>>>>> Parsed request is a module
>>>>>>>>>>>>>> using description file:
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>>>>>>>>>>>>>> ./pgadmin/tools/psql/static/js)
>>>>>>>>>>>>>> aliased with mapping 'local-echo-controller':
>>>>>>>>>>>>>> '/Users/dpage/git/pgadmin4/web/node_modules/local-echo' to
>>>>>>>>>>>>>> '/Users/dpage/git/pgadmin4/web/node_modules/local-echo'
>>>>>>>>>>>>>> using description file:
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>>>>>>>>>>>>>> ./pgadmin/tools/psql/static/js)
>>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>> root path /Users/dpage/git/pgadmin4/web
>>>>>>>>>>>>>> using description file:
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>>>>>>>>>>>>>> ./Users/dpage/git/pgadmin4/web/node_modules/local-echo)
>>>>>>>>>>>>>> no extension
>>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo
>>>>>>>>>>>>>> doesn't exist
>>>>>>>>>>>>>> .js
>>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.js
>>>>>>>>>>>>>> doesn't exist
>>>>>>>>>>>>>> .jsx
>>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx
>>>>>>>>>>>>>> doesn't exist
>>>>>>>>>>>>>> as directory
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo
>>>>>>>>>>>>>> doesn't exist
>>>>>>>>>>>>>> using description file:
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>>>>>>>>>>>>>> ./node_modules/local-echo)
>>>>>>>>>>>>>> no extension
>>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn't exist
>>>>>>>>>>>>>> .js
>>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo.js doesn't exist
>>>>>>>>>>>>>> .jsx
>>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx doesn't exist
>>>>>>>>>>>>>> as directory
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn't exist
>>>>>>>>>>>>>> @ ./pgadmin/tools/psql/static/js/index.js 17:19-43
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2021-05-10 14:38:37: webpack 5.21.2 compiled with 1 error in
>>>>>>>>>>>>>> 60041 ms
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> Dave Page
>>>>>>>>>>>>>> Blog: https://pgsnake.blogspot.com
>>>>>>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> EDB: https://www.enterprisedb.com
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>> Nikhil Mohite
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> *Thanks & Regards*
>>>>>>>>>>>> *Akshay Joshi*
>>>>>>>>>>>> *pgAdmin Hacker | Principal Software Architect*
>>>>>>>>>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>>>>>>>>>
>>>>>>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Dave Page
>>>>>>>>>>> Blog: https://pgsnake.blogspot.com
>>>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>>>
>>>>>>>>>>> EDB: https://www.enterprisedb.com
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>> Nikhil Mohite
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Dave Page
>>>>>>>>> Blog: https://pgsnake.blogspot.com
>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>
>>>>>>>>> EDB: https://www.enterprisedb.com
>>>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Nikhil Mohite
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Dave Page
>>>>>>> Blog: https://pgsnake.blogspot.com
>>>>>>> Twitter: @pgsnake
>>>>>>>
>>>>>>> EDB: https://www.enterprisedb.com
>>>>>>>
>>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Nikhil Mohite
>>>>>>
>>>>>>> <https://www.enterprisedb.com>
>>>>>>>
>>>>>>> --
>>>>>> *Thanks & Regards,*
>>>>>> *Nikhil Mohite*
>>>>>> *Software Engineer.*
>>>>>> *EDB Postgres* <https://www.enterprisedb.com/>
>>>>>> *Mob.No: +91-7798364578.*
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Dave Page
>>>>> Blog: https://pgsnake.blogspot.com
>>>>> Twitter: @pgsnake
>>>>>
>>>>> EDB: https://www.enterprisedb.com
>>>>>
>>>>> Regards,
>>>> Nikhil Mohite
>>>>
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: https://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EDB: https://www.enterprisedb.com
>>>
>>>
>>
>> --
>> *Thanks & Regards*
>> *Akshay Joshi*
>> *pgAdmin Hacker | Principal Software Architect*
>> *EDB Postgres <http://edbpostgres.com>*
>>
>> *Mobile: +91 976-788-8246*
>>
>

--
*Thanks & Regards*
*Akshay Joshi*
*pgAdmin Hacker | Principal Software Architect*
*EDB Postgres <http://edbpostgres.com>*

*Mobile: +91 976-788-8246*