Re: PATCH: Debugger Redesign

Thanks Dinesh. I believe Ashesh is working on the remaining issues, so I'll
await a cumulative update from him before I test further.

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