Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bWjYG-0000LF-Kv for pgadmin-hackers@arkaria.postgresql.org; Mon, 08 Aug 2016 12:21:04 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1bWjYG-0002Nu-7h for pgadmin-hackers@arkaria.postgresql.org; Mon, 08 Aug 2016 12:21:04 +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 1bWjYF-0002No-On for pgadmin-hackers@postgresql.org; Mon, 08 Aug 2016 12:21:03 +0000 Received: from mail-io0-x234.google.com ([2607:f8b0:4001:c06::234]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1bWjYA-0000g1-PU for pgadmin-hackers@postgresql.org; Mon, 08 Aug 2016 12:21:02 +0000 Received: by mail-io0-x234.google.com with SMTP id b62so355589263iod.3 for ; Mon, 08 Aug 2016 05:20:58 -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=ZhKxCkR4h0Nlmpl2dNDHqfdDn9tqizg+rNZ8G8f4GuQ=; b=mzODLpdLvFNu2SRqKHy/84LQqrYNGlRIBo6rakAbLeUpWiZMpu34BHQ3Im/8iWR5p8 oRBtxlUkcnaLAQwQ1GRFVpaEx5ho+J3BvLs0eHYAxko5wBZtyaahxa9T5jhmEmFOaBsM sF6sVKjf6ZF88Wr/uPK234J1kXU2VQcW5tj11VgQw9lNeW0uyykeXP5/+hW02akUesfe PtHIsq8hygZjfOyIYxySzwoHooPMlddRjWAmzpK2dr64rVi0QRw9AUnNmkotSasRHNTb gk8zyFAETUbhVyuNUXjXcdxUZTnhLdkS2RQ6LYFZNYceFdNnclwHh8gUhZtd+CQpILlM fQtQ== 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=ZhKxCkR4h0Nlmpl2dNDHqfdDn9tqizg+rNZ8G8f4GuQ=; b=Am9Zx64XCjBnpxw6sHSOdP3gZiaswjKafyzcR4pL0NukPsRQOPUZnv6DaTrG4WDIvP 2T2tk01zVmWRrQOMYX8SdEyYxIVMPT7+ZLbPPalbxEtb8eQ5k9Gvbgr31KpJLf9QBsnx pVV6PJhrsXoltviSDz4Rq2kwbqjXYftev8aV/HQMrXeMBH1cj+y1yzu1n61VdGxsMz78 tpsdokF4tLb4zudNGfWzCse4vr0+5YE+mdsq0ZQlzJpr/lmlERROercwPtdcKEmet9ac Dqxm6hIBID5zxsck/sWcULqzlvcbJ1nZrnIbVLxyYJsQY15Iu9uTAfIFxGJKLwjr1UAN W6TQ== X-Gm-Message-State: AEkoousssvJ16wf/d/d00SuEGOPh1MopbHBQYCOlJS0/54LDiT5dmNU/9GU+IvQWT7FDUjJjApUfQt/L6/6GXA== X-Received: by 10.107.175.34 with SMTP id y34mr516042ioe.70.1470658856879; Mon, 08 Aug 2016 05:20:56 -0700 (PDT) MIME-Version: 1.0 Received: by 10.64.208.97 with HTTP; Mon, 8 Aug 2016 05:20:56 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Mon, 8 Aug 2016 13:20:56 +0100 Message-ID: Subject: Re: RM #1250 Collection node counts To: Akshay Joshi Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary=001a114452b8c06b6d05398e722b 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 --001a114452b8c06b6d05398e722b Content-Type: text/plain; charset=UTF-8 On Mon, Aug 8, 2016 at 1:18 PM, Akshay Joshi 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 --001a114452b8c06b6d05398e722b Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Mon, Aug 8, 2016 at 1:18 PM, Akshay Joshi <akshay.joshi= @enterprisedb.com> wrote:
<= div dir=3D"ltr">

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 ha= ve fixed the RM#1250 "Collection node counts". To fix this RM I n= eed to do following changes
  • Move "check_precondition= " function from module's view class to global level within that py= thon file itself, so that module class will use it.=C2=A0
  • Modified = "get_nodes" function of each module's class, run the sq= l query to count the number of objects and pass the count to "g= enerate_browser_collection_node" function to display the collecti= on count.
  • Reuse SQL queries which is used to fetch nod= es. Make that query as inner query like "SELECT count(*) FROM( <nod= e's> query =C2=A0) AS collection_count". For that I'll have= to remove semicolon's from some of the SQL queries.
    • <= /ul>One case is not handled with this patch and that is on "Refresh&qu= ot; of collection node, count is not updated. If user refresh the parent no= de then it will be updated. I'll create a separate RM for that.
=C2=A0
Sorry Akshay, but I really do= n't like the way you've done this. It seems like an unnecessarily l= arge patch, and if I'm reading the patch correctly, it doubles the amou= nt of SQL queries run against the database when navigating the tree, and in= troduces 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 = 9;removed') and using those events to update the label on the parent no= de, if that node is a collection. That should just be a few lines, and shou= ld be correct at all times right?
=
=C2=A0 =C2=A0With 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 e= xpanded. For example initially we will show "Databases" =C2=A0and= when it gets expanded then we will update it to "Databases (5)".= =C2=A0 =C2=A0 =C2=A0

Right, but you also need to allow for removal and addition of children w= hen 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 ch= ange late in the beta cycle which isn't ideal.
=C2=A0
= =C2=A0

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

--
Akshay Joshi
Principal Software Engin= eer=C2=A0

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



<= /div>--
<= font color=3D"#3333FF">Akshay Joshi
Principal Software Engineer=C2=A0=


Phone: +91 20-3058-9= 517
Mobile: +91 976-788-8246



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

EnterpriseDB UK: http://www.enterprisedb.com<= br>The Enterprise PostgreSQL Company
--001a114452b8c06b6d05398e722b--