Re: PATCH: Debugger Redesign

On Mon, Apr 29, 2013 at 10:30 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Thanks Ashesh - committed (with a few minor tweaks, mostly to messages)!
> Good work :-)
>
Thanks

>
> I think we have the following issues to discuss and possibly resolve
> during beta:
>
> 1) I find the progress dialogue annoying, and as discussed previously
> think we should replace it with a progress indicator on the status bar.
>
Yeah - I am aware of that.
I will work on it once I get some time.

>
> 2) I don't see RAISE DEBUG messages in the server message pane. Perhaps we
> should explicitly set client_min_messages so the user can see their debug
> messages?
>
Yup

>
> 3) The stack pane shows functions as "foo(integer)(param_name=1)@<line>".
> I think perhaps we should change it to: "foo(integer param_name=1)@<line>"
>
+1

>
> 4) When in-process debugging, if the calling process is terminated we get
> a connection lost error (which I special-cased in the code, as it displays
> an unsightly message from the plugin by default). Instead, I think we
> should just automatically start listening for another calling process.
>
The problem is: this error message is coming from backend (debugger plugin).
And hence, it may require us to make changes in the plugin.

And - once again thanks for giving me this opportunity.

>
> Thoughts?
>
> Thanks!
>
>
> On Fri, Apr 26, 2013 at 9:27 PM, Ashesh Vashi <
> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> On Thu, Apr 25, 2013 at 7:25 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Thanks Dinesh. I believe Ashesh is working on the remaining issues, so
>>> I'll await a cumulative update from him before I test further.
>>>
>> Yeah - I was working on the same.
>> It took a lot more time than I anticipated. (I found some other bugs
>> too. i.e. fetching frame information on click one of the stacks.)
>>
>> Here is the updated patch.
>>
>>> On Wed, Apr 24, 2013 at 2:39 PM, Dinesh Kumar <
>>> dinesh(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Ashesh,
>>>>
>>>> Thank you very much for your guidance on resolving the above issues. As
>>>> of now, i have debugged the debugger and fixed some of the issues, as per
>>>> the Dave's previous conversation. Please find the below status on this.
>>>>
>>>>
>>>> *- The layout of the parameters dialogue needs work - the grid needs to
>>>> size to the dialogue.*
>>>> Fixed.
>>>>
>>>>
>>>> *- The checkboxes in the grid are jumbo sized. We have them in the Edit
>>>> Grid already - can the code be reused?
>>>> *
>>>> Fixed.
>>>>
>>>> *- The popup activity dialog is kinda annoying. Perhaps we could put a
>>>> progress indicator in the status bar? Low priority.
>>>> *
>>>> Not Fixed, since it's a low priority i have scheduled it as my last
>>>> task.
>>>>
>>>> *- When injecting new values into variables, if the value can't be
>>>> accepted (wrong datatype etc), then the grid should be reset to the
>>>> original value when the error message box is accepted.*
>>>> Fixed.
>>>>
>>>> *- If a function is re-executed in the same session, the return value
>>>> isn't cleared from the Results grid.*
>>>> Fixed.
>>>>
>>>>
>>>> *- If a function is re-executed in the same session, the breakpoints
>>>> are cleared. This doesn't seem to happen with in-process debugging,
>>>> only direct.
>>>> *
>>>> I have been tried a lot to resolve this issue with my trivial
>>>> knowledge, but i am not able to figure our out how to fix this. Apologizes
>>>> for that. As per my understanding, the re-start process of debugger,
>>>> closing the previous session handler and creating new session, may be this
>>>> is the reason, dbgController::ResultStack()'s fetch break point operation
>>>> not able to get the breakpoints of the previous session handler.
>>>> *
>>>> *
>>>> *- Aborting a direct debug session mid-function caused an indefinite(?)
>>>> hang with a busy cursor.*
>>>> As per our discussion, you have fixed this for the windows, i haven't
>>>> tested the fix for the mac. Sorry :)
>>>> *
>>>> - The status messages in the status bar are a little confusing. I
>>>> think you need to add "Done." to the end of the string when an action
>>>> completes, otherwise it looks like things never complete until you
>>>> step or run.
>>>> *
>>>> Fixed.
>>>>
>>>> Adding the patch for the above fixes.
>>>>
>>>> ** This is the extension to your patch. Requesting you to use vim -d
>>>> between your patch and this patch to get my patch. :)
>>>>
>>>> Kindly let me know, if any thing i missed here.
>>>>
>>>> Thanks in advance.
>>>>
>>>> Dinesh
>>>>
>>>> --
>>>> *Dinesh Kumar*
>>>> Software Engineer
>>>>
>>>> Ph: +918087463317
>>>> Skype ID: dinesh.kumar432
>>>> www.enterprisedb.co <http://www.enterprisedb.com/>m<http://www.enterprisedb.com/>
>>>> *
>>>> Follow us on Twitter*
>>>> @EnterpriseDB
>>>>
>>>> Visit EnterpriseDB for tutorials, webinars, whitepapers<http://www.enterprisedb.com/resources-community> and
>>>> more <http://www.enterprisedb.com/resources-community>
>>>>
>>>>
>>>> On Thu, Apr 11, 2013 at 10:40 PM, Ashesh Vashi <
>>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Sure.
>>>>> I'll work on it...
>>>>> On 11 Apr 2013 21:00, "Dave Page" <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> First impressions - it already seems more stable than the old code,
>>>>>> and I only played with it for an hour or so on Mac so far. Nice work
>>>>>> :-)
>>>>>>
>>>>>> I did find a few things in my initial testing that need some
>>>>>> attention. I didn't get the impression anything was particularly
>>>>>> serious or would be troublesome to fix though:
>>>>>>
>>>>>> - The layout of the parameters dialogue needs work - the grid needs to
>>>>>> size to the dialogue.
>>>>>>
>>>>>> - The checkboxes in the grid are jumbo sized. We have them in the Edit
>>>>>> Grid already - can the code be reused?
>>>>>>
>>>>>> - The popup activity dialog is kinda annoying. Perhaps we could put a
>>>>>> progress indicator in the status bar? Low priority.
>>>>>>
>>>>>> - When injecting new values into variables, if the value can't be
>>>>>> accepted (wrong datatype etc), then the grid should be reset to the
>>>>>> original value when the error message box is accepted.
>>>>>>
>>>>>> - If a function is re-executed in the same session, the return value
>>>>>> isn't cleared from the Results grid.
>>>>>>
>>>>>> - If a function is re-executed in the same session, the breakpoints
>>>>>> are cleared. This doesn't seem to happen with in-process debugging,
>>>>>> only direct.
>>>>>>
>>>>>> - Aborting a direct debug session mid-function caused an indefinite(?)
>>>>>> hang with a busy cursor.
>>>>>>
>>>>>> - The status messages in the status bar are a little confusing. I
>>>>>> think you need to add "Done." to the end of the string when an action
>>>>>> completes, otherwise it looks like things never complete until you
>>>>>> step or run.
>>>>>>
>>>>>> If you can fix those issues (with the possible exception of the
>>>>>> progress indicator), I'll dive in a bit deeper. I don't want to go too
>>>>>> deep just yet in case you have to change more than I expect.
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>> On Wed, Apr 10, 2013 at 12:10 PM, Ashesh Vashi
>>>>>> <ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>> > Hi All,
>>>>>> >
>>>>>> > We always wanted to redesign the plpgsql debugger in pgAdmin
>>>>>> III from
>>>>>> > very long time due to very long list of bug
>>>>>> > Reports of the debugger. We were not able crack down variety of
>>>>>> cases across
>>>>>> > the platforms. For many cases - resolving
>>>>>> > On one platform, we ended up introducing other issues on another
>>>>>> platforms.
>>>>>> > This inability of handling variety of cases
>>>>>> > Across platforms made us to redesign the debugger from the scratch.
>>>>>> >
>>>>>> > Let me list down few the limitations/flaws with the current
>>>>>> implementation
>>>>>> > of the debugger:
>>>>>> > - Uses its own mechanism for making connection, querying
>>>>>> (async/sync) &
>>>>>> > handing results instead of using the common
>>>>>> > available classes, which are used by all other components in
>>>>>> pgAdmin III.
>>>>>> > i.e.
>>>>>> > dbgConn, dbgResult, dbgPgThread, etc.
>>>>>> >
>>>>>> > Because of this, the improvements in the existing infrastructure
>>>>>> were
>>>>>> > never reflected in the debugger.
>>>>>> > i.e.
>>>>>> > The connection with debugger were never be created with the SSL
>>>>>> support.
>>>>>> > Because - dbgConn never supported the SSL.
>>>>>> > This is also results for the many bugs.
>>>>>> > i.e.
>>>>>> > The dbgPgThread has its own mechanism to run queries
>>>>>> asynchronously and
>>>>>> > resulted into many bugs. It generates few
>>>>>> > events, which shouldn't be done after the completion of the
>>>>>> debugger
>>>>>> > operations. And, that leads to crash. Using
>>>>>> > TestDestroy() function for JOINABLE threads were unnecessary and
>>>>>> never
>>>>>> > required. But - it is using it.
>>>>>> >
>>>>>> > - A hidden windows is taking care of the events generated during the
>>>>>> > debugging the actual window. It could have been
>>>>>> > Good, if all the tasks having intialited and handled from by this
>>>>>> central
>>>>>> > mechanism, instead we do have different
>>>>>> > places for handling both the direct/indirect debugging. And, that
>>>>>> > introduced a lot of issues. And, that made the
>>>>>> > things very confusing even to understand the existing mechanism.
>>>>>> >
>>>>>> > And, many more.
>>>>>> >
>>>>>> > We had to made changes in the existing infrastructure/classes to
>>>>>> use it with
>>>>>> > the debugger.
>>>>>> > * pgConn:
>>>>>> > + Report error quietly (if told) while error found in execution,
>>>>>> this
>>>>>> > allows these functions to be used from any
>>>>>> > Thread. Otherwise, it used to throw an error.
>>>>>> > + Set/Reset the PGcancel object before and after the query
>>>>>> execution,
>>>>>> > which will be useful for us to use the new
>>>>>> > Mechanism of libpq (of course, not very new) to cancel any query
>>>>>> > execution from in between.
>>>>>> > + Allow new application name, while duplicating the connection
>>>>>> > + Save the version and the features list, while duplicating the
>>>>>> > connection, this will avoid/reduce the duplicate calls
>>>>>> > To the database server.
>>>>>> > * pgQueryThread:
>>>>>> > + Allow to specify the custom notice processor and handler
>>>>>> > + Allow to run multiple queries by creating queue
>>>>>> > + Allow to use the EnterpriseDB's callable statement from the
>>>>>> > pgQueryThread (if available)
>>>>>> > + Proper cancellation mechanism implementation
>>>>>> > + Send a custom event to the caller (instead of standard event)
>>>>>> for
>>>>>> > successful execution or on different errors
>>>>>> > (but - not if it is explicitly cancelled by the user.) This
>>>>>> allows us to
>>>>>> > send more information to the developer.
>>>>>> > + Allows the developer to add a batch queries (which should run
>>>>>> > asynchronously) even after starting of the thread.
>>>>>> > + Allow to use the PGparam now, to avoid generating the queries
>>>>>> > dynamically
>>>>>> >
>>>>>> > The new design is based on the MVC (Model-View-Controller)
>>>>>> architecture.
>>>>>> > * Controller (dbgController)
>>>>>> > - A central mechanism to handle all the operations.
>>>>>> > * View (frmDebugger - mostly reused)
>>>>>> > - Responsible for presentation and interaction with the end-user
>>>>>> > i.e.
>>>>>> > code viewer, different variable windows, stack window, etc.
>>>>>> > - It also controls other part of view.
>>>>>> > * Model (dbgModel)
>>>>>> > - Handles/Contains all the data
>>>>>> > - It contains the information about the target function/procedure.
>>>>>> >
>>>>>> > For any operation, View and Model will ask the Controller to handle
>>>>>> it.
>>>>>> >
>>>>>> > Summary of the changes:
>>>>>> >
>>>>>> > INSERTED| DELETED| FILENAME |
>>>>>> SUMMARY
>>>>>> >
>>>>>> ---------+----------+---------------------------------------------|----------------------------
>>>>>> > 3| 1| pgadmin/ctl/ctlSQLResult.cpp |
>>>>>> - Call
>>>>>> > cancel thread execution on exit, if already
>>>>>> > |
>>>>>> running
>>>>>> > for nice exit handling
>>>>>> > 196| 18| pgadmin/db/pgConn.cpp |
>>>>>> -
>>>>>> > Set/Reset the PGcancel object
>>>>>> > |
>>>>>> - Saving
>>>>>> > settings and new application name, while
>>>>>> > |
>>>>>> cloning
>>>>>> > 31| 24| pgadmin/include/db/pgConn.h |
>>>>>> > 245| 33| pgadmin/include/db/pgQueryThread.h |
>>>>>> > 647| 111| pgadmin/db/pgQueryThread.cpp |
>>>>>> -
>>>>>> > Multiple queries execution support (one-by-one)
>>>>>> > 8| 0| pgadmin/include/db/pgSet.h |
>>>>>> -
>>>>>> > Introduce a new function GetCommandStatus for the
>>>>>> > |
>>>>>> result
>>>>>> > 0| 1| pgadmin/db/pgSet.cpp |
>>>>>> -
>>>>>> > Removed unnecessary header inclusion
>>>>>> > 0| 1563| pgadmin/debugger/ctlCodeWindow.cpp |
>>>>>> -
>>>>>> > Omitted/Removed it completely
>>>>>> > 0| 250| pgadmin/include/debugger/ctlCodeWindow.h |
>>>>>> > 9| 9| pgadmin/debugger/ctlMessageWindow.cpp |
>>>>>> -
>>>>>> > Standardize the function naming conversion with the
>>>>>> > |
>>>>>> other
>>>>>> > components
>>>>>> > 5| 12| pgadmin/include/debugger/ctlMessageWindow.h |
>>>>>> > 28| 27| pgadmin/debugger/ctlResultGrid.cpp |
>>>>>> - Using
>>>>>> > the standard querying mechanism (pgSet)
>>>>>> > |
>>>>>> instead
>>>>>> > of libpq's PGresult directly
>>>>>> > 4| 3| pgadmin/include/debugger/ctlResultGrid.h |
>>>>>> > 4| 7| pgadmin/debugger/ctlStackWindow.cpp |
>>>>>> -
>>>>>> > Standardize the function naming conversion with the
>>>>>> > |
>>>>>> other
>>>>>> > components
>>>>>> > 2| 2| pgadmin/include/debugger/ctlStackWindow.h |
>>>>>> > 50| 56| pgadmin/debugger/ctlTabWindow.cpp |
>>>>>> -
>>>>>> > Standardize the function naming conversion with the
>>>>>> > |
>>>>>> other
>>>>>> > components
>>>>>> > 20| 17| pgadmin/include/debugger/ctlTabWindow.h |
>>>>>> > 60| 55| pgadmin/debugger/ctlVarWindow.cpp |
>>>>>> -
>>>>>> > Standardize the function naming conversion with the
>>>>>> > |
>>>>>> other
>>>>>> > components
>>>>>> > 17| 15| pgadmin/include/debugger/ctlVarWindow.h |
>>>>>> > 819| 0| pgadmin/debugger/dbgController.cpp |
>>>>>> -
>>>>>> > Central of the debugger (it takes care of all the
>>>>>> > |
>>>>>> > operations, the frmDebugger (view) and dbgModel
>>>>>> > |
>>>>>> (model)
>>>>>> > interact with it.
>>>>>> > 173| 0| pgadmin/include/debugger/dbgController.h |
>>>>>> > 0| 21| pgadmin/debugger/dbgDbResult.cpp |
>>>>>> -
>>>>>> > Omitted/Removed it completely
>>>>>> > 703| 0| pgadmin/debugger/dbgEvents.cpp |
>>>>>> - Added
>>>>>> > new file, which contains all the event
>>>>>> > |
>>>>>> handling
>>>>>> > functions of the dbgController to reduce
>>>>>> > |
>>>>>> the size
>>>>>> > of dbgController and separating them from
>>>>>> > |
>>>>>> the
>>>>>> > direct operations
>>>>>> > 63| 0| pgadmin/debugger/dbgModel.cpp |
>>>>>> -
>>>>>> > Contains all the information regarding the
>>>>>> > |
>>>>>> target.
>>>>>> > 143| 0| pgadmin/include/debugger/dbgModel.h |
>>>>>> > 0| 544| pgadmin/debugger/dbgPgConn.cpp |
>>>>>> -
>>>>>> > Omitted/Removed it completely
>>>>>> > 0| 363| pgadmin/debugger/dbgPgThread.cpp |
>>>>>> -
>>>>>> > Omitted/Removed it completely
>>>>>> > 0| 157| pgadmin/debugger/dbgResultset.cpp |
>>>>>> -
>>>>>> > Omitted/Removed it completely
>>>>>> > 536| 194| pgadmin/debugger/dbgTargetInfo.cpp |
>>>>>> - It
>>>>>> > fetches and saves the target information from
>>>>>> > |
>>>>>> the
>>>>>> > database server (it is no more using the
>>>>>> > |
>>>>>> > deprecated pldbg_target_info function).
>>>>>> > 146| 75| pgadmin/include/debugger/dbgTargetInfo.h |
>>>>>> > 44| 101| pgadmin/debugger/debugger.cpp |
>>>>>> -
>>>>>> > Debugger menu factory, modified accordingly new
>>>>>> > |
>>>>>> design
>>>>>> > 770| 682| pgadmin/debugger/dlgDirectDbg.cpp |
>>>>>> -
>>>>>> > Handles now only taking input from the end-user
>>>>>> > |
>>>>>> - It
>>>>>> > also takes care of converting an expression
>>>>>> > |
>>>>>> input to
>>>>>> > a proper value
>>>>>> > |
>>>>>> - It
>>>>>> > also allows the end-user to use the default
>>>>>> > |
>>>>>> value of
>>>>>> > an argument through UI.
>>>>>> > |
>>>>>> - It
>>>>>> > does not control the debugger execution any
>>>>>> > |
>>>>>> more
>>>>>> > 74| 48| pgadmin/include/debugger/dlgDirectDbg.h |
>>>>>> > 734| 165| pgadmin/debugger/frmDebugger.cpp |
>>>>>> - Mostly
>>>>>> > reused
>>>>>> > |
>>>>>> - Works
>>>>>> > as presentation layer and takes input from
>>>>>> > |
>>>>>> from the
>>>>>> > end-user
>>>>>> > |
>>>>>> i.e.
>>>>>> > set/reset break-points, changing parameter
>>>>>> > |
>>>>>> > values, showing the target code, showing
>>>>>> > |
>>>>>> > current parameter values, etc.
>>>>>> > 114| 92| pgadmin/include/debugger/frmDebugger.h |
>>>>>> > 3| 5| pgadmin/debugger/module.mk |
>>>>>> > 5| 1| pgadmin/dlg/dlgClasses.cpp |
>>>>>> - Call
>>>>>> > cancel thread execution on exit, if already
>>>>>> > |
>>>>>> running
>>>>>> > for nice exit handling (ExecutionDialog)
>>>>>> > 4| 1| pgadmin/frm/frmEditGrid.cpp |
>>>>>> - Call
>>>>>> > cancel thread execution on abort
>>>>>> > 2| 2| pgadmin/frm/frmQuery.cpp |
>>>>>> -
>>>>>> > Modified to use new custom event on query
>>>>>> > |
>>>>>> > execution completion
>>>>>> > 2| 1| pgadmin/include/frm/frmQuery.h |
>>>>>> > 1| 1| pgadmin/include/db/module.mk |
>>>>>> > 79| 0| pgadmin/include/db/pgQueryResultEvent.h |
>>>>>> -
>>>>>> > Introduced a new custom event for handling the
>>>>>> > |
>>>>>> query
>>>>>> > execution error/success (pgQueryThread)
>>>>>> > 14| 21| pgadmin/include/debugger/dbgBreakPoint.h |
>>>>>> -
>>>>>> > Reformatted the break-point according to new design
>>>>>> > 0| 38| pgadmin/include/debugger/dbgConnProp.h |
>>>>>> -
>>>>>> > Omitted/Removed it completely
>>>>>> > 57| 10| pgadmin/include/debugger/dbgConst.h |
>>>>>> > 0| 53| pgadmin/include/debugger/dbgDbResult.h |
>>>>>> -
>>>>>> > Omitted/Removed it completely
>>>>>> > 0| 99| pgadmin/include/debugger/dbgPgConn.h |
>>>>>> -
>>>>>> > Omitted/Removed it completely
>>>>>> > 0| 95| pgadmin/include/debugger/dbgPgThread.h |
>>>>>> -
>>>>>> > Omitted/Removed it completely
>>>>>> > 0| 52| pgadmin/include/debugger/dbgResultset.h |
>>>>>> -
>>>>>> > Omitted/Removed it completely
>>>>>> > 2| 6| pgadmin/include/debugger/module.mk |
>>>>>> > 3| 6| pgadmin/include/precomp.h |
>>>>>> > 7| 12| pgadmin/pgAdmin3.vcxproj |
>>>>>> - Window
>>>>>> > build script changes
>>>>>> > 10| 28| pgadmin/pgAdmin3.vcxproj.filters |
>>>>>> > 3| 3| pgadmin/ii/dlgDirectDbg.xrc |
>>>>>> - Input
>>>>>> > Arguments dialog change (changed the
>>>>>> > |
>>>>>> > dimensions.)
>>>>>> > 44| 44| pgadmin/ii/xrcDialogs.cpp |
>>>>>> >
>>>>>> >
>>>>>> > --
>>>>>> >
>>>>>> > Thanks & Regards,
>>>>>> >
>>>>>> > Ashesh Vashi
>>>>>> > EnterpriseDB INDIA: Enterprise PostgreSQL Company
>>>>>> >
>>>>>> >
>>>>>> > http://www.linkedin.com/in/asheshvashi
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>> > --
>>>>>> > Sent via pgadmin-hackers mailing list (
>>>>>> pgadmin-hackers(at)postgresql(dot)org)
>>>>>> > To make changes to your subscription:
>>>>>> > http://www.postgresql.org/mailpref/pgadmin-hackers
>>>>>> >
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Dave Page
>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>> Twitter: @pgsnake
>>>>>>
>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>> The Enterprise PostgreSQL Company
>>>>>>
>>>>>
>>>>
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
>>
>> --
>> --
>>
>> Thanks & Regards,
>>
>> Ashesh Vashi
>> EnterpriseDB INDIA: Enterprise PostgreSQL Company<http://www.enterprisedb.com>
>>
>>
>>
>> *http://www.linkedin.com/in/asheshvashi*<http://www.linkedin.com/in/asheshvashi>
>>
>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company<http://www.enterprisedb.com>

*http://www.linkedin.com/in/asheshvashi*<http://www.linkedin.com/in/asheshvashi>