Re: [pgAdmin][RM4329] Initialization error when parameterised functions debugged in parallel in two separate tabs
Hi, This is not working on Python 2.7 :( Attached patch will add an __init__ file in the utils directory under debugger to make it work with python 2.7 On Fri, Jun 14, 2019 at 5:17 PM Dave Page wrote: > Thanks, applied. > > On Fri, Jun 14, 2019 at 10:38 AM Aditya Toshniwal < > aditya.toshni...@enterprisedb.com> wrote: > >> Hi Hackers, >> >> I have missed a line while implementing this. Attached is the patch to >> fix that. >> Although it has not caused any trouble, but still it should be changed. >> Kindly review. >> >> On Mon, Jun 10, 2019 at 7:28 PM Dave Page wrote: >> >>> Thanks, patch applied. >>> >>> On Mon, Jun 10, 2019 at 1:58 PM Aditya Toshniwal < >>> aditya.toshni...@enterprisedb.com> wrote: >>> Hi Hackers, Attached is the updated patch with fixes. On Mon, Jun 10, 2019 at 12:58 PM Akshay Joshi < akshay.jo...@enterprisedb.com> wrote: > Hi Aditya > > Following are the review comments: > >- "Set breakpoint" option not working, when click it throws an >error. > > Fixed. > >- Create an empty function and try to debug that. It should show >proper error message. > > This seems to be a bug in the debugger itself. I'll raise a bug with simulation steps if it is. But, not sure where to raise. > >- Got the following backend error when closing the connection, >please fix this: > - File > "E:\Projects\pgadmin4\web\pgadmin\tools\debugger\__init__.py", line > 2053, > in close_debugger_session > conn_id=dbg_obj['exe_conn_id']) > > Fixed. > > On Fri, Jun 7, 2019 at 12:21 PM Aditya Toshniwal < > aditya.toshni...@enterprisedb.com> wrote: > >> Hi Hackers, >> >> Attached is the patch for debugger improvements. The changes include: >> 1) Change the way debug info is stored in session. Removed redundant >> session related code in debugger code. All the session related handling >> done at one place. >> 2) Fixed a bug where debugger was not opening for EPAS package >> function. >> 3) If a package is defined without body and we try to debug a >> proc/func, the debugger opened a blank window. Changes made so that it >> will >> throw error as "XYZ is not defined in package body." >> >> -- >> Thanks and Regards, >> Aditya Toshniwal >> Software Engineer | EnterpriseDB India | Pune >> "Don't Complain about Heat, Plant a TREE" >> > > > -- > *Thanks & Regards* > *Akshay Joshi* > > *Sr. Software Architect* > *EnterpriseDB Software India Private Limited* > *Mobile: +91 976-788-8246* > -- Thanks and Regards, Aditya Toshniwal Software Engineer | EnterpriseDB India | Pune "Don't Complain about Heat, Plant a TREE" >>> >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> >> -- >> Thanks and Regards, >> Aditya Toshniwal >> Software Engineer | EnterpriseDB India | Pune >> "Don't Complain about Heat, Plant a TREE" >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Thanks and Regards, Aditya Toshniwal Software Engineer | EnterpriseDB India | Pune "Don't Complain about Heat, Plant a TREE" debugger.py27.patch Description: Binary data
pgAdmin 4 commit: Added missing __init__.py file in debugger utils whic
Added missing __init__.py file in debugger utils which is required for Python 2.7 Branch -- master Details --- https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=ce7679c4bd493c8972ea72d3575e8f47c0b10c8c Author: Aditya Toshniwal Modified Files -- web/pgadmin/tools/debugger/utils/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-)
Re: [pgAdmin][RM4329] Initialization error when parameterised functions debugged in parallel in two separate tabs
Thanks patch applied. On Wed, Jun 19, 2019 at 12:35 PM Aditya Toshniwal < aditya.toshni...@enterprisedb.com> wrote: > Hi, > > This is not working on Python 2.7 :( > Attached patch will add an __init__ file in the utils directory under > debugger to make it work with python 2.7 > > > On Fri, Jun 14, 2019 at 5:17 PM Dave Page wrote: > >> Thanks, applied. >> >> On Fri, Jun 14, 2019 at 10:38 AM Aditya Toshniwal < >> aditya.toshni...@enterprisedb.com> wrote: >> >>> Hi Hackers, >>> >>> I have missed a line while implementing this. Attached is the patch to >>> fix that. >>> Although it has not caused any trouble, but still it should be changed. >>> Kindly review. >>> >>> On Mon, Jun 10, 2019 at 7:28 PM Dave Page wrote: >>> Thanks, patch applied. On Mon, Jun 10, 2019 at 1:58 PM Aditya Toshniwal < aditya.toshni...@enterprisedb.com> wrote: > Hi Hackers, > > Attached is the updated patch with fixes. > > On Mon, Jun 10, 2019 at 12:58 PM Akshay Joshi < > akshay.jo...@enterprisedb.com> wrote: > >> Hi Aditya >> >> Following are the review comments: >> >>- "Set breakpoint" option not working, when click it throws an >>error. >> >> Fixed. > >> >>- Create an empty function and try to debug that. It should show >>proper error message. >> >> This seems to be a bug in the debugger itself. I'll raise a bug with > simulation steps if it is. But, not sure where to raise. > >> >>- Got the following backend error when closing the connection, >>please fix this: >> - File >> "E:\Projects\pgadmin4\web\pgadmin\tools\debugger\__init__.py", >> line 2053, >> in close_debugger_session >> conn_id=dbg_obj['exe_conn_id']) >> >> Fixed. > >> >> On Fri, Jun 7, 2019 at 12:21 PM Aditya Toshniwal < >> aditya.toshni...@enterprisedb.com> wrote: >> >>> Hi Hackers, >>> >>> Attached is the patch for debugger improvements. The changes include: >>> 1) Change the way debug info is stored in session. Removed redundant >>> session related code in debugger code. All the session related handling >>> done at one place. >>> 2) Fixed a bug where debugger was not opening for EPAS package >>> function. >>> 3) If a package is defined without body and we try to debug a >>> proc/func, the debugger opened a blank window. Changes made so that it >>> will >>> throw error as "XYZ is not defined in package body." >>> >>> -- >>> Thanks and Regards, >>> Aditya Toshniwal >>> Software Engineer | EnterpriseDB India | Pune >>> "Don't Complain about Heat, Plant a TREE" >>> >> >> >> -- >> *Thanks & Regards* >> *Akshay Joshi* >> >> *Sr. Software Architect* >> *EnterpriseDB Software India Private Limited* >> *Mobile: +91 976-788-8246* >> > > > -- > Thanks and Regards, > Aditya Toshniwal > Software Engineer | EnterpriseDB India | Pune > "Don't Complain about Heat, Plant a TREE" > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company >>> >>> >>> -- >>> Thanks and Regards, >>> Aditya Toshniwal >>> Software Engineer | EnterpriseDB India | Pune >>> "Don't Complain about Heat, Plant a TREE" >>> >> >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > > -- > Thanks and Regards, > Aditya Toshniwal > Software Engineer | EnterpriseDB India | Pune > "Don't Complain about Heat, Plant a TREE" > -- *Thanks & Regards* *Akshay Joshi* *Sr. Software Architect* *EnterpriseDB Software India Private Limited* *Mobile: +91 976-788-8246*
[pgAdmin4][Patch]: RM #4362 trigger function create script
Hi Hackers, Attached is the patch to fix RM #4362 trigger function create script. Please review it. -- *Thanks & Regards* *Akshay Joshi* *Sr. Software Architect* *EnterpriseDB Software India Private Limited* *Mobile: +91 976-788-8246* RM_4362.patch Description: Binary data
pgAdmin 4 commit: Capitalize the word 'export' used in Import/Export mo
Capitalize the word 'export' used in Import/Export module. Fixes #4345 Branch -- master Details --- https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdiff;h=5c0ea0c012839d69f9b40f3efcc632a5c4d52cff Modified Files -- docs/en_US/release_notes_4_9.rst | 1 + web/pgadmin/tools/import_export/static/js/import_export.js | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-)
Re: [GSoC][Patch] Automatic Mode Detection V1
Hi On Wed, Jun 19, 2019 at 6:18 AM Yosry Muhammad wrote: > > Waiting for the icon, will set it up once it is ready. > It's underway :-) > I ran pep8 checks and JS tests on this patch, however I could not run > python tests due to a problem with chromedriver (working on it), please let > me know if any tests fail. > Take a look in the Makefile (or web/regression/README) and you'll see how you can run tests selectively - e.g. to avoid the feature tests when running the Python suite, you can do "python regression/runtests.py --exclude feature_tests" As for chromedriver, there's a utility (tools/get_chromedriver.py) you can use to download and install the correct version. You should save it to somewhere in your path; I'd suggest the bin/ directory in your virtual environment. > > - When revising patches, please send an updated one for the whole thing, >> rather than incremental ones. Incrementals are more work to apply and don't >> give us any benefit in return. >> >> > The attached patch is a single patch including all old and new increments. > :-) Aditya; can you do a quick code review please? Bear in mind it's a work in progress and there are no docs or tests etc. yet. > > - We need to add a "do you want to continue" warning before actions like >> Execute or EXPLAIN are run, if there are unsaved changes in the grid. >> >> - I think we should make the text in any cells that has been edited bold >> until saved, so the user can see where changes have been made (as they can >> with deleted rows). >> > > Both done, new rows are highlighted too. > Nice! I realise it's most likely not your code, but if you can fix the wrapping so it doesn't break mid-word, that would be good. See the attached screenshot to see what I mean. > >> - If I make two data edits and then delete a row, I get 3 entries in the >> History panel, all showing the same delete. I would actually argue that >> data edit queries that pgAdmin generates should not go into the History at >> all, but maybe they should be added albeit with a flag to say they're >> internal queries and an option to hide them. Thoughts? >> > > That was a bug with the existing 'save changes' action of 'View Data', on > which mine is based upon. I fixed the bug, now the queries are shown > correctly. However, the queries are shown in the form in which they are > sent from the backend to the database driver (without parameters - also an > already existing bug in View Data Mode), for example: > > INSERT INTO public.kweek ( >> media_url, username, text, created_at) VALUES ( >> %(media_url)s::character varying, %(username)s::character varying, >> %(text)s::text, %(created_at)s::timestamp without time zone) >> returning id; >> > > I propose two solutions: > 1- Hide pgadmin's generated sql from history (in both modes). > 2- Show the actual sql query that was executed after the parameters are > plugged in (more understandable and potentially helpful). > I like the idea of doing 2 - but I think we should have a checkbox on the history panel to show/hide generated queries (and we should include transaction control - BEGIN, COMMIT etc - in the generated query class). > > >> - We need to think about how data editing fits in with transaction >> control. Right now, it seems to happen entirely outside of it - for >> example, I tend to work with auto commit turned off, so my connection sits >> idle-in-transaction following an initial select, and remains un-affected by >> edits. Please think about this and suggest options for us to discuss. >> > > I integrated the data editing in the transaction control as you noted. Now > the behavior is as follows: > 1- In View Data mode, same existing behavior. > 2- In Query Tool mode: > - If auto-commit is on: the modifications are made and commited once save > is pressed. > - If auto-commit is off: the modifications are made as part of the ongoing > transaction (or a new one if no transaction is ongoing), they are not > commited unless the user executes a commit command (or rollback). > That seems to work. I think we need to make it more obvious that there's a transaction in progress - especially as that can be the case after the user hits the Save button and thinks their data is safe (a side-thought is that perhaps we shouldn't require the Save button to be pressed when auto-commit is turned off, as that's just odd). We should highlight the transaction state more clearly to the user, and make sure we prompt for confirmation if they try to close the tab or the whole window. > I think it makes more sense for filters to be disabled. I mean since the >>> user is already writing SQL it would be more convenient to just edit it >>> directly. >>> >> >> Well we're not going to just disable them - we'll either remove them, or >> try to make them work. I'm leaning strongly towards just removing that code >> entirely. >> >> > I meant disabling them in the query tool while keeping them in the View > Data mode as the user cannot edit the sql
Re: [GSoC][Patch] Automatic Mode Detection V1
Hi, On Wed, Jun 19, 2019, 1:54 PM Dave Page wrote: . - We need to add a "do you want to continue" warning before actions like Execute or EXPLAIN are run, if there are unsaved changes in the grid. - I think we should make the text in any cells that has been edited bold until saved, so the user can see where changes have been made (as they can with deleted rows). Both done, new rows are highlighted too. Nice! I realise it's most likely not your code, but if you can fix the wrapping so it doesn't break mid-word, that would be good. See the attached screenshot to see what I mean. Will do. - If I make two data edits and then delete a row, I get 3 entries in the History panel, all showing the same delete. I would actually argue that data edit queries that pgAdmin generates should not go into the History at all, but maybe they should be added albeit with a flag to say they're internal queries and an option to hide them. Thoughts? That was a bug with the existing 'save changes' action of 'View Data', on which mine is based upon. I fixed the bug, now the queries are shown correctly. However, the queries are shown in the form in which they are sent from the backend to the database driver (without parameters - also an already existing bug in View Data Mode), for example: INSERT INTO public.kweek ( media_url, username, text, created_at) VALUES ( %(media_url)s::character varying, %(username)s::character varying, %(text)s::text, %(created_at)s::timestamp without time zone) returning id; I propose two solutions: 1- Hide pgadmin's generated sql from history (in both modes). 2- Show the actual sql query that was executed after the parameters are plugged in (more understandable and potentially helpful). I like the idea of doing 2 - but I think we should have a checkbox on the history panel to show/hide generated queries (and we should include transaction control - BEGIN, COMMIT etc - in the generated query class). I can work on option 2 now and then work on the checkbox if/when there is time. - We need to think about how data editing fits in with transaction control. Right now, it seems to happen entirely outside of it - for example, I tend to work with auto commit turned off, so my connection sits idle-in-transaction following an initial select, and remains un-affected by edits. Please think about this and suggest options for us to discuss. I integrated the data editing in the transaction control as you noted. Now the behavior is as follows: 1- In View Data mode, same existing behavior. 2- In Query Tool mode: - If auto-commit is on: the modifications are made and commited once save is pressed. - If auto-commit is off: the modifications are made as part of the ongoing transaction (or a new one if no transaction is ongoing), they are not commited unless the user executes a commit command (or rollback). That seems to work. I think we need to make it more obvious that there's a transaction in progress - especially as that can be the case after the user hits the Save button and thinks their data is safe (a side-thought is that perhaps we shouldn't require the Save button to be pressed when auto-commit is turned off, as that's just odd). We should highlight the transaction state more clearly to the user, and make sure we prompt for confirmation if they try to close the tab or the whole window. The transaction status can be made more obvious and point out when a transaction is in progress that changes aren't commited. However, removing the save button when auto commit is off will cause us to a send a request and execute a query every time any cell is changed (which can be by accident or some kind of draft). I also think it will make more sense when there is a dedicated button, which can be named such that it is clear that it only executes some queries. Also, the pop up that shows after edits are succeesful can also state thar these changes are not yet commited. I think it makes more sense for filters to be disabled. I mean since the user is already writing SQL it would be more convenient to just edit it directly. Well we're not going to just disable them - we'll either remove them, or try to make them work. I'm leaning strongly towards just removing that code entirely. I meant disabling them in the query tool while keeping them in the View Data mode as the user cannot edit the sql in the View Data mode. Do you want to remove the feature from both modes completely? I think you misunderstand - I want to remove the View Data mode entirely. Your work should replace it. As a user of pgAdmin I think this might not be the best option. Not all users of pgAdmin are developers or know SQL. I worked on several projects before where other people on the team (or frontend developers) would just want to take a look at some data or do simple edits using the GUI. Also, other management studios for other DBMSs also allow for this. In addition, the user can do sorting of data without knowing SQL. What I th
Re: [GSoC][Patch] Automatic Mode Detection V1
Hi On Wed, Jun 19, 2019 at 2:47 PM Yosry Muhammad wrote: > Hi, > > > On Wed, Jun 19, 2019, 1:54 PM Dave Page wrote: > . > > - We need to add a "do you want to continue" warning before actions like > Execute or EXPLAIN are run, if there are unsaved changes in the grid. > > - I think we should make the text in any cells that has been edited bold > until saved, so the user can see where changes have been made (as they can > with deleted rows). > > > Both done, new rows are highlighted too. > > > Nice! I realise it's most likely not your code, but if you can fix the > wrapping so it doesn't break mid-word, that would be good. See the attached > screenshot to see what I mean. > > > > Will do. > > > - If I make two data edits and then delete a row, I get 3 entries in the > History panel, all showing the same delete. I would actually argue that > data edit queries that pgAdmin generates should not go into the History at > all, but maybe they should be added albeit with a flag to say they're > internal queries and an option to hide them. Thoughts? > > > That was a bug with the existing 'save changes' action of 'View Data', on > which mine is based upon. I fixed the bug, now the queries are shown > correctly. However, the queries are shown in the form in which they are > sent from the backend to the database driver (without parameters - also an > already existing bug in View Data Mode), for example: > > INSERT INTO public.kweek ( > media_url, username, text, created_at) VALUES ( > %(media_url)s::character varying, %(username)s::character varying, > %(text)s::text, %(created_at)s::timestamp without time zone) > returning id; > > > I propose two solutions: > 1- Hide pgadmin's generated sql from history (in both modes). > 2- Show the actual sql query that was executed after the parameters are > plugged in (more understandable and potentially helpful). > > > I like the idea of doing 2 - but I think we should have a checkbox on the > history panel to show/hide generated queries (and we should include > transaction control - BEGIN, COMMIT etc - in the generated query class). > > > > I can work on option 2 now and then work on > the checkbox if/when there is time. > I'm pretty sure there will be time :-) > > > > - We need to think about how data editing fits in with transaction > control. Right now, it seems to happen entirely outside of it - for > example, I tend to work with auto commit turned off, so my connection sits > idle-in-transaction following an initial select, and remains un-affected by > edits. Please think about this and suggest options for us to discuss. > > > I integrated the data editing in the transaction control as you noted. Now > the behavior is as follows: > 1- In View Data mode, same existing behavior. > 2- In Query Tool mode: > - If auto-commit is on: the modifications are made and commited once save > is pressed. > - If auto-commit is off: the modifications are made as part of the ongoing > transaction (or a new one if no transaction is ongoing), they are not > commited unless the user executes a commit command (or rollback). > > > That seems to work. I think we need to make it more obvious that there's a > transaction in progress - especially as that can be the case after the user > hits the Save button and thinks their data is safe (a side-thought is that > perhaps we shouldn't require the Save button to be pressed when auto-commit > is turned off, as that's just odd). We should highlight the transaction > state more clearly to the user, and make sure we prompt for confirmation if > they try to close the tab or the whole window. > > > The transaction status can be made more obvious and point out when a > transaction is in progress that changes aren't commited. However, removing > the save button when auto commit is off will cause us to a send a request > and execute a query every time any cell is changed (which can be by > accident or some kind of draft). I also think it will make more sense when > there is a dedicated button, which can be named such that it is clear that > it only executes some queries. Also, the pop up that shows after edits are > succeesful can also state thar these changes are not yet commited. > Yeah, I agree removing the button for some modes only is weird. Maybe adding info to the notification, and having a more prominent "Your transaction is still in progress" notification will be enough. Another thought - we also need to figure out what happens if the user edits data in the grid and when saving, an error occurs (e.g. trying to insert null into a not-null field). How does that tie into transaction control? For example, auto-rollback may then revert other changes made via SQL (which should have been atomic, with the data changes) - or having auto-rollback turned off may then require the user to explicitly start a new transaction before attempting to save the data again. Perhaps we need to precede data changes with a savepoint, and then roll back to that if there
Re: [pgAdmin4][Patch]: RM #4362 trigger function create script
Hi On Wed, Jun 19, 2019 at 9:55 AM Akshay Joshi wrote: > Hi Hackers, > > Attached is the patch to fix RM #4362 trigger function create script. > Please review it. > While I can see how this works, I question why we have the string "SETOF" in prorettypname anyway? That's a separate property, so it should be in a separate variable. What if I have a type called "SETOFDavesStuff" (unlikely, but it illustrates the point)? -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [pgAdmin4][Patch]: RM #4362 trigger function create script
Hi Dave On Wed, 19 Jun, 2019, 19:57 Dave Page, wrote: > Hi > > On Wed, Jun 19, 2019 at 9:55 AM Akshay Joshi < > akshay.jo...@enterprisedb.com> wrote: > >> Hi Hackers, >> >> Attached is the patch to fix RM #4362 trigger function create script. >> Please review it. >> > > While I can see how this works, I question why we have the string "SETOF" > in prorettypname anyway? That's a separate property, so it should be in a > separate variable. What if I have a type called "SETOFDavesStuff" > (unlikely, but it illustrates the point)? > We have used pg_get_function_result(func_oid) to get the returns clause for function and assign its value to prorettypename variable. I have followed the same logic for trigger function and fixed it. Similar logic has already been used for functions. > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
Re: [GSoC][Patch] Automatic Mode Detection V1
Hi there ! On Wed, Jun 19, 2019 at 4:11 PM Dave Page wrote: > >> - We need to think about how data editing fits in with transaction >> control. Right now, it seems to happen entirely outside of it - for >> example, I tend to work with auto commit turned off, so my connection sits >> idle-in-transaction following an initial select, and remains un-affected by >> edits. Please think about this and suggest options for us to discuss. >> >> >> I integrated the data editing in the transaction control as you noted. >> Now the behavior is as follows: >> 1- In View Data mode, same existing behavior. >> 2- In Query Tool mode: >> - If auto-commit is on: the modifications are made and commited once save >> is pressed. >> - If auto-commit is off: the modifications are made as part of the >> ongoing transaction (or a new one if no transaction is ongoing), they are >> not commited unless the user executes a commit command (or rollback). >> >> >> That seems to work. I think we need to make it more obvious that there's >> a transaction in progress - especially as that can be the case after the >> user hits the Save button and thinks their data is safe (a side-thought is >> that perhaps we shouldn't require the Save button to be pressed when >> auto-commit is turned off, as that's just odd). We should highlight the >> transaction state more clearly to the user, and make sure we prompt for >> confirmation if they try to close the tab or the whole window. >> >> >> The transaction status can be made more obvious and point out when a >> transaction is in progress that changes aren't commited. However, removing >> the save button when auto commit is off will cause us to a send a request >> and execute a query every time any cell is changed (which can be by >> accident or some kind of draft). I also think it will make more sense when >> there is a dedicated button, which can be named such that it is clear that >> it only executes some queries. Also, the pop up that shows after edits are >> succeesful can also state thar these changes are not yet commited. >> > > Yeah, I agree removing the button for some modes only is weird. Maybe > adding info to the notification, and having a more prominent "Your > transaction is still in progress" notification will be enough. > Will work on adding a notification and making the transaction status more prominent. > > Another thought - we also need to figure out what happens if the user > edits data in the grid and when saving, an error occurs (e.g. trying to > insert null into a not-null field). How does that tie into transaction > control? For example, auto-rollback may then revert other changes made via > SQL (which should have been atomic, with the data changes) - or having > auto-rollback turned off may then require the user to explicitly start a > new transaction before attempting to save the data again. Perhaps we need > to precede data changes with a savepoint, and then roll back to that if > there's an error? > I think the savepoint is an adequate solution.If any problems happen then rollback to the savepoint then release it, else just release the savepoint. > I think it makes more sense for filters to be disabled. I mean since the >> user is already writing SQL it would be more convenient to just edit it >> directly. >> >> >> Well we're not going to just disable them - we'll either remove them, or >> try to make them work. I'm leaning strongly towards just removing that code >> entirely. >> >> >> I meant disabling them in the query tool while keeping them in the View >> Data mode as the user cannot edit the sql in the View Data mode. Do you >> want to remove the feature from both modes completely? >> >> >> I think you misunderstand - I want to remove the View Data mode entirely. >> Your work should replace it. >> >> >> As a user of pgAdmin I think this might not be the best option. Not all >> users of pgAdmin are developers or know SQL. I worked on several projects >> before where other people on the team (or frontend developers) would just >> want to take a look at some data or do simple edits using the GUI. Also, >> other management studios for other DBMSs also allow for this. In addition, >> the user can do sorting of data without knowing SQL. What I think can be >> done (potentially - maybe in the future) is limit the dependance on SQL >> knowledge when doing filters in View Data mode, while disabling filters and >> so in the Query Tool. >> > > Hmm, the point of this project (which has been a goal for maybe 20 years!) > was to remove that mode entirely. There is an argument that users can use > the "SELECT Script" option instead if they don't know SQL, but that would > still require the Sort/Filter options. > > What do other folks think? > Waiting for other people's opinion on that matter. Oh, and icon attached! > Will work on adding the new icon and switching the save functionality to it. I propose using this icon to save data in both View Data and Query Tool mode, and t
Re: [GSoC][Patch] Automatic Mode Detection V1
On Thu, Jun 20, 2019 at 2:20 AM Yosry Muhammad wrote: > Hi there ! > > On Wed, Jun 19, 2019 at 4:11 PM Dave Page wrote: > >> >>> - We need to think about how data editing fits in with transaction >>> control. Right now, it seems to happen entirely outside of it - for >>> example, I tend to work with auto commit turned off, so my connection sits >>> idle-in-transaction following an initial select, and remains un-affected by >>> edits. Please think about this and suggest options for us to discuss. >>> >>> >>> I integrated the data editing in the transaction control as you noted. >>> Now the behavior is as follows: >>> 1- In View Data mode, same existing behavior. >>> 2- In Query Tool mode: >>> - If auto-commit is on: the modifications are made and commited once >>> save is pressed. >>> - If auto-commit is off: the modifications are made as part of the >>> ongoing transaction (or a new one if no transaction is ongoing), they are >>> not commited unless the user executes a commit command (or rollback). >>> >>> >>> That seems to work. I think we need to make it more obvious that there's >>> a transaction in progress - especially as that can be the case after the >>> user hits the Save button and thinks their data is safe (a side-thought is >>> that perhaps we shouldn't require the Save button to be pressed when >>> auto-commit is turned off, as that's just odd). We should highlight the >>> transaction state more clearly to the user, and make sure we prompt for >>> confirmation if they try to close the tab or the whole window. >>> >>> >>> The transaction status can be made more obvious and point out when a >>> transaction is in progress that changes aren't commited. However, removing >>> the save button when auto commit is off will cause us to a send a request >>> and execute a query every time any cell is changed (which can be by >>> accident or some kind of draft). I also think it will make more sense when >>> there is a dedicated button, which can be named such that it is clear that >>> it only executes some queries. Also, the pop up that shows after edits are >>> succeesful can also state thar these changes are not yet commited. >>> >> >> Yeah, I agree removing the button for some modes only is weird. Maybe >> adding info to the notification, and having a more prominent "Your >> transaction is still in progress" notification will be enough. >> > > Will work on adding a notification and making the transaction status more > prominent. > > >> >> Another thought - we also need to figure out what happens if the user >> edits data in the grid and when saving, an error occurs (e.g. trying to >> insert null into a not-null field). How does that tie into transaction >> control? For example, auto-rollback may then revert other changes made via >> SQL (which should have been atomic, with the data changes) - or having >> auto-rollback turned off may then require the user to explicitly start a >> new transaction before attempting to save the data again. Perhaps we need >> to precede data changes with a savepoint, and then roll back to that if >> there's an error? >> > > I think the savepoint is an adequate solution.If any problems happen then > rollback to the savepoint then release it, else just release the savepoint. > > >> I think it makes more sense for filters to be disabled. I mean since the >>> user is already writing SQL it would be more convenient to just edit it >>> directly. >>> >>> >>> Well we're not going to just disable them - we'll either remove them, or >>> try to make them work. I'm leaning strongly towards just removing that code >>> entirely. >>> >>> >>> I meant disabling them in the query tool while keeping them in the View >>> Data mode as the user cannot edit the sql in the View Data mode. Do you >>> want to remove the feature from both modes completely? >>> >>> >>> I think you misunderstand - I want to remove the View Data mode >>> entirely. Your work should replace it. >>> >>> >>> As a user of pgAdmin I think this might not be the best option. Not all >>> users of pgAdmin are developers or know SQL. I worked on several projects >>> before where other people on the team (or frontend developers) would just >>> want to take a look at some data or do simple edits using the GUI. Also, >>> other management studios for other DBMSs also allow for this. In addition, >>> the user can do sorting of data without knowing SQL. What I think can be >>> done (potentially - maybe in the future) is limit the dependance on SQL >>> knowledge when doing filters in View Data mode, while disabling filters and >>> so in the Query Tool. >>> >> +1 View data mode is quick & easy way for any novice user who want to interact with table data. >> Hmm, the point of this project (which has been a goal for maybe 20 >> years!) was to remove that mode entirely. There is an argument that users >> can use the "SELECT Script" option instead if they don't know SQL, but that >> would still require the Sort/Filter options. >> >> Wha
Re: [GSoC][Patch] Automatic Mode Detection V1
[forked the mail chain for code review] Hi Yosry, On Wed, Jun 19, 2019 at 5:24 PM Dave Page wrote: > Hi > > On Wed, Jun 19, 2019 at 6:18 AM Yosry Muhammad wrote: > >> >> Waiting for the icon, will set it up once it is ready. >> > > It's underway :-) > > >> I ran pep8 checks and JS tests on this patch, however I could not run >> python tests due to a problem with chromedriver (working on it), please let >> me know if any tests fail. >> > > Take a look in the Makefile (or web/regression/README) and you'll see how > you can run tests selectively - e.g. to avoid the feature tests when > running the Python suite, you can do "python regression/runtests.py > --exclude feature_tests" > > As for chromedriver, there's a utility (tools/get_chromedriver.py) you can > use to download and install the correct version. You should save it to > somewhere in your path; I'd suggest the bin/ directory in your virtual > environment. > > >> >> - When revising patches, please send an updated one for the whole thing, >>> rather than incremental ones. Incrementals are more work to apply and don't >>> give us any benefit in return. >>> >>> >> The attached patch is a single patch including all old and new increments. >> > > :-) > > Aditya; can you do a quick code review please? Bear in mind it's a work in > progress and there are no docs or tests etc. yet. > Nice work there. :) I just had look on the code changes, and have few suggestions: 1) I found the code around primary key in the function check_for_updatable_resultset_and_primary_keys repeating. Try if you cpuld modify/reuse the get_primary_keys function. 2) The function name check_for_updatable_resultset_and_primary_keys could be shorter, something like check_updatabale_rset_pkeys. Same for __set_updatable_resultset_attributes to __set_updatable_rset_attr 3) You've used background: #f4f4f4; for .highlighted_grid_cells class. This should go to sqleditor.scss with appropriate color from web/pgadmin/static/scss/resources/_default.variables.scss. Hard coded color codes are highly discouraged. Otherwise, looks good (didn't run and check) > > >> >> - We need to add a "do you want to continue" warning before actions like >>> Execute or EXPLAIN are run, if there are unsaved changes in the grid. >>> >>> - I think we should make the text in any cells that has been edited bold >>> until saved, so the user can see where changes have been made (as they can >>> with deleted rows). >>> >> >> Both done, new rows are highlighted too. >> > > Nice! I realise it's most likely not your code, but if you can fix the > wrapping so it doesn't break mid-word, that would be good. See the attached > screenshot to see what I mean. > > >> >>> - If I make two data edits and then delete a row, I get 3 entries in the >>> History panel, all showing the same delete. I would actually argue that >>> data edit queries that pgAdmin generates should not go into the History at >>> all, but maybe they should be added albeit with a flag to say they're >>> internal queries and an option to hide them. Thoughts? >>> >> >> That was a bug with the existing 'save changes' action of 'View Data', on >> which mine is based upon. I fixed the bug, now the queries are shown >> correctly. However, the queries are shown in the form in which they are >> sent from the backend to the database driver (without parameters - also an >> already existing bug in View Data Mode), for example: >> >> INSERT INTO public.kweek ( >>> media_url, username, text, created_at) VALUES ( >>> %(media_url)s::character varying, %(username)s::character varying, >>> %(text)s::text, %(created_at)s::timestamp without time zone) >>> returning id; >>> >> >> I propose two solutions: >> 1- Hide pgadmin's generated sql from history (in both modes). >> 2- Show the actual sql query that was executed after the parameters are >> plugged in (more understandable and potentially helpful). >> > > I like the idea of doing 2 - but I think we should have a checkbox on the > history panel to show/hide generated queries (and we should include > transaction control - BEGIN, COMMIT etc - in the generated query class). > > >> >> >>> - We need to think about how data editing fits in with transaction >>> control. Right now, it seems to happen entirely outside of it - for >>> example, I tend to work with auto commit turned off, so my connection sits >>> idle-in-transaction following an initial select, and remains un-affected by >>> edits. Please think about this and suggest options for us to discuss. >>> >> >> I integrated the data editing in the transaction control as you noted. >> Now the behavior is as follows: >> 1- In View Data mode, same existing behavior. >> 2- In Query Tool mode: >> - If auto-commit is on: the modifications are made and commited once save >> is pressed. >> - If auto-commit is off: the modifications are made as part of the >> ongoing transaction (or a new one if no transaction is ongoing), they are >> not commited unless the user executes a commit command
Re: [pgAdmin4][Patch]: RM #4362 trigger function create script
Hi Dave On Wed, Jun 19, 2019 at 8:47 PM Akshay Joshi wrote: > Hi Dave > > On Wed, 19 Jun, 2019, 19:57 Dave Page, wrote: > >> Hi >> >> On Wed, Jun 19, 2019 at 9:55 AM Akshay Joshi < >> akshay.jo...@enterprisedb.com> wrote: >> >>> Hi Hackers, >>> >>> Attached is the patch to fix RM #4362 trigger function create script. >>> Please review it. >>> >> >> While I can see how this works, I question why we have the string "SETOF" >> in prorettypname anyway? That's a separate property, so it should be in a >> separate variable. What if I have a type called "SETOFDavesStuff" >> (unlikely, but it illustrates the point)? >> > > We have used pg_get_function_result(func_oid) to get the returns > clause for function and assign its value to prorettypename variable. I have > followed the same logic for trigger function and fixed it. Similar logic > has already been used for functions. > I have tested it with two different type "SETOF TypeTest" and "SETOFType" for function, as trigger_function only have return type either trigger or event_trigger. I haven't found any issue with the reverse engineered sql and create script as well. > >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > -- *Thanks & Regards* *Akshay Joshi* *Sr. Software Architect* *EnterpriseDB Software India Private Limited* *Mobile: +91 976-788-8246*
