Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bX4xe-0005ro-VV for pgadmin-hackers@arkaria.postgresql.org; Tue, 09 Aug 2016 11:12:43 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1bX4xe-0001gS-Ie for pgadmin-hackers@arkaria.postgresql.org; Tue, 09 Aug 2016 11:12:42 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1bX4xe-0001gM-2I for pgadmin-hackers@postgresql.org; Tue, 09 Aug 2016 11:12:42 +0000 Received: from mail-it0-x233.google.com ([2607:f8b0:4001:c0b::233]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1bX4xa-0003Nb-0U for pgadmin-hackers@postgresql.org; Tue, 09 Aug 2016 11:12:41 +0000 Received: by mail-it0-x233.google.com with SMTP id x130so9731110ite.1 for ; Tue, 09 Aug 2016 04:12:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=5oZ+/6FS2VBSucPLF2RILUtMBH4nlx9vRo2i48604/4=; b=Dpg5mSfor0JAsFTiXBBKHev3I+pZJrdjoQOebkdo6PVlfxTmze+ytdtnb8JDLXFmw8 WSuqoKl68SJOF+Zd+u5mEABzqhjjCVIVP1bRDv/OBKErIKov63puqRC473geK7aFp/MP z7IC1euaqlceQF1gQcovsmygK/yIOWVxuqXrY6mHF+hyPiUSkN1NBQwgHjGvK02oNVZk OMjinEARUWXGVhK5p5TeQZ9+dBuFdm/TihHG0uotZrHRoqkt0RGn42zLTepK6HWuOsG4 /Ik2X39kXgKGidEdMfGL8ISN5HkjpydBHLDlaQusM7SRej4LLTDh/2QyyLn8s6vUafLK aLMg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=5oZ+/6FS2VBSucPLF2RILUtMBH4nlx9vRo2i48604/4=; b=TskFK8D7M+7DQdnGgn+x7zszaCKLr1wXU1+JOAjsDYxTczhBnPtXOET+nUGsNu8mRb JLRfUs3l2rQw/8xe6w7JFMFdksSknf6mqcjRW1izTI+N1HUpMPDDyVTCf3Fo2TplBiL2 9bwIJJIMNXyOzuYaq7lZLDxqttmSVoN1Z9DQfmJCJUIp9eo8c7aQHWM4IsXBaBFPFDzh nJbUTCM/mVLCKYWHQXQuoGnVLYJOyQrlTmbggmuoFv86TG7Z1ElISml5IkOg3hIKpctf Iro4N8lob8HfnObQg5Sc/NvuO1gf/XSlNUn/7maVxgI3n4gaKHlhYN13snhvgVExzy/W l/tA== X-Gm-Message-State: AEkoousZmw7xzArP60fDR8AESri1TRDDfTPPVYKTa7pgM556TCVMEOnN9Ocm4AwVXuF1BF88YsG2j7Llhu3HHg== X-Received: by 10.36.117.79 with SMTP id y76mr22700557itc.35.1470741155789; Tue, 09 Aug 2016 04:12:35 -0700 (PDT) MIME-Version: 1.0 Received: by 10.64.208.97 with HTTP; Tue, 9 Aug 2016 04:12:34 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Tue, 9 Aug 2016 12:12:34 +0100 Message-ID: Subject: Re: RM #1250 Collection node counts To: Akshay Joshi Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary=001a114a90ce261d110539a19cba X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --001a114a90ce261d110539a19cba Content-Type: text/plain; charset=UTF-8 Awesome - just what I was expecting, and it works nicely :-). Thanks, committed. On Tue, Aug 9, 2016 at 12:00 PM, Akshay Joshi wrote: > 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 wrote: > >> >> >> On Mon, Aug 8, 2016 at 1:18 PM, Akshay Joshi < >> akshay.joshi@enterprisedb.com> wrote: >> >>> >>> >>> On Mon, Aug 8, 2016 at 5:33 PM, Dave Page wrote: >>> >>>> Hi >>>> >>>> On Mon, Aug 8, 2016 at 11:39 AM, Akshay Joshi < >>>> akshay.joshi@enterprisedb.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( 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@postgresql.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 >> > > > > -- > *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 --001a114a90ce261d110539a19cba Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Awesome - just what I was expecting, and it works nicely := -).

Thanks, committed.

On Tue, Aug 9, 2016 at 12:00 PM, Akshay= Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi Dave=C2=A0

I have implemented the logic as per your suggestion. When user will = expand the collection node label will get updated with collection count. At= tached is the new patch file, please review it and let me know the review c= omments. =C2=A0

On Mon, Aug 8, 2016 at 5:= 50 PM, Dave Page <dpage@pgadmin.org> wrote:


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


On Mon, Aug 8, 2016 at 5:33 PM, Dave = Page <dpage@pgadmin.org> wrote:
Hi

On Mon, Aug 8, 2016 at 11:39 AM= , Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi All
I have fixed the RM#1250 "Collection node counts". T= o fix this RM I need to do following changes
  • Move "c= heck_precondition" function from module's view class to global lev= el within that python file itself, so that module class will use it.=C2=A0<= /li>
  • 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 dis= play 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 =C2=A0) AS collection_count". For t= hat 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 refr= esh the parent node then it will be updated. I'll create a separate RM = for that.
=C2=A0
Sorry Akshay= , but I really don't like the way you've done this. It seems like a= n 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 b= e different from the actual number of nodes.

I was= expecting to see this implemented by watching for tree events (e.g. 'a= dded' 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 fe= w lines, and should be correct at all times right?
<= /blockquote>

=C2=A0 =C2=A0With current implementa= tion 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 onc= e the node gets expanded. For example initially we will show "Database= s" =C2=A0and when it gets expanded then we will update it to "Dat= abases (5)". =C2=A0 =C2=A0 =C2=A0
=

Right, but you also need to allow for removal an= d addition of children when the node is already expanded, and refreshes. Pl= us 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.<= /div>
=C2=A0
=C2=A0

Attached is the patch file. Please revie= w it and let me know the review comments. =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0=C2=A0

--
Akshay Joshi


<= b>Phone: +91 20-3058-9517
Mobile: +91= 976-788-8246


--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-ha= ckers




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

EnterpriseDB= UK: http://www.e= nterprisedb.com
The Enterprise PostgreSQL Company



--
Akshay Joshi<= /span>
Principal Software Engineer=C2=A0


Phone: +91 20-3058-9517<= br>Mobile: +91 976-788-8246



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

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



--
<= b>Akshay Joshi
Principal Software Enginee= r=C2=A0

=

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



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

EnterpriseDB UK: http://www.enterprised= b.com
The Enterprise PostgreSQL Company
--001a114a90ce261d110539a19cba--