Re: PATCH: Debugger Redesign

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>