Re: RM #1250 Collection node counts

On Mon, Aug 8, 2016 at 1:18 PM, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
wrote:

>
>
> On Mon, Aug 8, 2016 at 5:33 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> Hi
>>
>> On Mon, Aug 8, 2016 at 11:39 AM, Akshay Joshi <
>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi All
>>>
>>> I have fixed the RM#1250 "Collection node counts". To fix this RM I need
>>> to do following changes
>>>
>>> - Move "check_precondition" function from module's view class to
>>> global level within that python file itself, so that module class will use
>>> it.
>>> - Modified "get_nodes" function of each module's class, run the sql
>>> query to count the number of objects and pass the count to
>>> "generate_browser_collection_node" function to display the
>>> collection count.
>>> - Reuse SQL queries which is used to fetch nodes. Make that query as
>>> inner query like "SELECT count(*) FROM( <node's> query ) AS
>>> collection_count". For that I'll have to remove semicolon's from some of
>>> the SQL queries.
>>>
>>> One case is not handled with this patch and that is on "Refresh" of
>>> collection node, count is not updated. If user refresh the parent node then
>>> it will be updated. I'll create a separate RM for that.
>>>
>>
>> Sorry Akshay, but I really don't like the way you've done this. It seems
>> like an unnecessarily large patch, and if I'm reading the patch correctly,
>> it doubles the amount of SQL queries run against the database when
>> navigating the tree, and introduces race conditions where the count
>> displayed could be different from the actual number of nodes.
>>
>> I was expecting to see this implemented by watching for tree events (e.g.
>> 'added' and 'removed') and using those events to update the label on the
>> parent node, if that node is a collection. That should just be a few lines,
>> and should be correct at all times right?
>>
>
> With current implementation children's of any collection node will be
> fetched/added when user will expand that collection node, in that case we
> will update the label once the node gets expanded. For example initially we
> will show "Databases" and when it gets expanded then we will update it to
> "Databases (5)".
>

Right, but you also need to allow for removal and addition of children when
the node is already expanded, and refreshes. Plus my other comments are
still valid I believe - race condition, double the SQL and a very large
change late in the beta cycle which isn't ideal.

>
>>
>>>
>>> Attached is the patch file. Please review it and let me know the review
>>> comments.
>>>
>>> --
>>> *Akshay Joshi*
>>> *Principal Software Engineer *
>>>
>>>
>>>
>>> *Phone: +91 20-3058-9517 <%2B91%2020-3058-9517>Mobile: +91 976-788-8246*
>>>
>>>
>>> --
>>> 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
>>
>
>
>
> --
> *Akshay Joshi*
> *Principal Software Engineer *
>
>
>
> *Phone: +91 20-3058-9517 <%2B91%2020-3058-9517>Mobile: +91 976-788-8246*
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company