Hi
On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <
nikhil.moh...@enterprisedb.com> wrote:
> Hi Dave/ Team,
>
> On Tue, May 18, 2021 at 8:41 PM Dave Page <dp...@pgadmin.org> wrote:
>
>> Hi
>>
>> On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <
> nikhil.moh...@enterprisedb.com> wrote:
>
>> Hi Dave/Team,
>>
>>
>> On Mon, May 17, 2021 at 6:47 PM Dave Page <dp...@pgadmin.org> wrote:
>>
>>> Hi
>>>
>>> On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <
>>> nikhil.moh...@enterprisedb.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?
>
>
>> 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?
>>
>
>>
>>>
>>>> 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'}
>>
>> 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.
>>>
>>
>> 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
>>
>
>>
>
>>>>
>>>>>
>>>>>
>>>>> On Tue, May 11, 2021 at 3:40 PM Dave Page <dp...@pgadmin.org> wrote:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <
>>>>>> akshay.jo...@enterprisedb.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.moh...@enterprisedb.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 <dp...@pgadmin.org>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <
>>>>>>>>> nikhil.moh...@enterprisedb.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
