Re: PATCH: Debugger Redesign

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

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.

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?

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>"

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.

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