Subject: RM #1250 Collection node counts

From: Akshay Joshi <[email-masked]>
Date: Mon, 8 Aug 2016 16:09:17 +0530

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.

Attached is the patch file. Please review it and let me know the review
comments.

-- 
*Akshay Joshi*
*Principal Software Engineer *

*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*

-- 
Sent via pgadmin-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

---

From: Dave Page <[email-masked]>
Date: Mon, 8 Aug 2016 13:03:50 +0100

Hi

On Mon, Aug 8, 2016 at 11:39 AM, Akshay Joshi <[email-masked]
> 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.

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?

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

---

From: Akshay Joshi <[email-masked]>
Date: Mon, 8 Aug 2016 17:48:28 +0530

On Mon, Aug 8, 2016 at 5:33 PM, Dave Page <[email-masked]> wrote:

> Hi
>
> On Mon, Aug 8, 2016 at 11:39 AM, Akshay Joshi <
> [email-masked]> wrote:
>
>> Hi All
>>

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

---

From: Dave Page <[email-masked]>
Date: Mon, 8 Aug 2016 13:20:56 +0100

> will show "Databases"  and when it gets expanded then we will update it to

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.

---

From: Akshay Joshi <[email-masked]>
Date: Tue, 9 Aug 2016 16:30:05 +0530

Hi Dave

I have implemented the logic as per your suggestion. When user will expand
the collection node label will get updated with collection count. Attached
is the new patch file, please review it and let me know the review
comments.

On Mon, Aug 8, 2016 at 5:50 PM, Dave Page <[email-masked]> wrote:

>
>
> On Mon, Aug 8, 2016 at 1:18 PM, Akshay Joshi <
>>>> I have fixed the RM#1250 "Collection node counts". To fix this RM I
>>>> need to do following changes
>>>>
>>>>    - Reuse SQL queries which is used to fetch nodes. Make that query
>>>>    as inner query like "SELECT count(*) FROM( <node's> query  ) AS
>>> 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?
>>>
>>
> 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

---

From: Dave Page <[email-masked]>
Date: Tue, 9 Aug 2016 12:12:34 +0100

Awesome - just what I was expecting, and it works nicely :-).

Thanks, committed.

On Tue, Aug 9, 2016 at 12:00 PM, Akshay Joshi <[email-masked]

>>>> 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
>>>>> Attached is the patch file. Please review it and let me know the
>>>>> review comments.
>>>>>
>>>>> --
>>>>> *Akshay Joshi*