Re: [pgAdmin4][Patch]: Fixed RM #2315 : Sorting by size is broken

Thanks - patch applied.

On Mon, May 8, 2017 at 8:08 AM, Khushboo Vashi <
khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:

> Hi,
>
> Please find the attached updated patch.
>
> Thanks,
> Khushboo
>
> On Fri, May 5, 2017 at 9:00 PM, Joao Pedro De Almeida Pereira <
> jdealmeidapereira(at)pivotal(dot)io> wrote:
>
>> Hi Khushboo,
>>
>> We looked at your updated patch:
>> - the tests look good!
>> - there's a small comment header change needed in size_prettify_spec
>>
> Fixed
>
>> - it looks like the previous and new functions have different behaviors
>> (where the new behavior changes units on 10000 of the lower unit, a 9999GB)
>> - We should clarify that in size_prettify, we were mostly talking about
>> name readability in your first patch, and that the original structure was
>> better (especially the sizes array)
>> At first glance, the new sizePrettify appears to behave like a for loop,
>> so that might be the simplest refactor.
>>
>> Added loop.
>
>
>> Thanks,
>> Joao and George
>>
>>
>>
>> On Thu, May 4, 2017 at 5:02 AM, Khushboo Vashi <
>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi,
>>>
>>> Please find the attached updated patch.
>>>
>>> Thanks,
>>> Khushboo
>>>
>>> On Fri, Apr 28, 2017 at 7:58 PM, Matthew Kleiman <mkleiman(at)pivotal(dot)io>
>>> wrote:
>>>
>>>> Hi Khushboo,
>>>>
>>>> That sounds good. Sorry if we weren't clear at first.
>>>>
>>>> Have a good holiday weekend!
>>>>
>>>> Sarah & Matt
>>>>
>>>>
>>>> On Fri, Apr 28, 2017 at 4:35 AM, Khushboo Vashi <
>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Sarah,
>>>>>
>>>>> On Thu, Apr 27, 2017 at 7:38 PM, Sarah McAlear <smcalear(at)pivotal(dot)io>
>>>>> wrote:
>>>>>
>>>>>> Hi Kushboo!
>>>>>>
>>>>>> We understand your point, but we believe that relying on 2
>>>>>> independent functions to deliver the same formatting can become a problem
>>>>>> if the PG function changes. Our suggestion is to use a single function in
>>>>>> our javascript code to do this formatting.
>>>>>>
>>>>>> It seems reasonable to me and I am going to use a single javascript
>>>>> function which will support PB also (as per Dave we should add support till
>>>>> PB) .
>>>>>
>>>>>> If the community believes we can live with this risk, let's move
>>>>>> forward.
>>>>>>
>>>>>> Thanks!
>>>>>> Sarah & Joao
>>>>>>
>>>>>> On Thu, Apr 27, 2017 at 1:50 AM, Khushboo Vashi <
>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi Joao & Sarah,
>>>>>>>
>>>>>>> On Wed, Apr 26, 2017 at 8:46 PM, Joao Pedro De Almeida Pereira <
>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>
>>>>>>>> Hi Khushboo!
>>>>>>>>
>>>>>>>> Thanks for your reply!
>>>>>>>>
>>>>>>>>
>>>>>>>>> *SQL Files:*
>>>>>>>>>>
>>>>>>>>>> - Is there a way to avoid conditionals here?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> - Maybe we can use the same javascript function to prettify
>>>>>>>>>> all the sizes
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> In case of collection node (ex: Databases), I have implemented
>>>>>>>>>> this functionality via putting a formatter for a back-grid column. So, it
>>>>>>>>>> is applicable for the entire column not for the individual cell. To apply
>>>>>>>>>> the javascript function on a single cell for the column (string column),
>>>>>>>>>> first we need to identify that cell and then apply the function, I think
>>>>>>>>>> this is overhead. Just to avoid conditional sql template I would not prefer
>>>>>>>>>> this approach.
>>>>>>>>>
>>>>>>>>>
>>>>>>>> We are not totally sure we understood what you meant on the
>>>>>>>> previous statement. Are you saying that the conditionals in SQL are used to
>>>>>>>> ensure that we can apply a javascript function at column level instead of
>>>>>>>> cell level?
>>>>>>>>
>>>>>>>> Correct.
>>>>>>>
>>>>>>>> Our concern is that the templates are being made more complex and
>>>>>>>> inconsistencies are introduced in the code and the UI.
>>>>>>>>
>>>>>>>
>>>>>>> Inconsistencies in the UI can be avoided through making the
>>>>>>> size_formatter same as pg_size_pretty function which I will implement.
>>>>>>> I have checked the pg_size_pretty function code and it supports till
>>>>>>> TB format, so I think we should keep till TB only.
>>>>>>>
>>>>>>> In this particular example, we are allowing the backend to respond
>>>>>>>> sometimes with prettified data and sometimes without it, so at UI level we
>>>>>>>> will have inconsistencies between screens or more complex Javascript code
>>>>>>>> to support sometimes prettifying and sometimes not prettify the same
>>>>>>>> fields.
>>>>>>>>
>>>>>>>> We have separate logic for collection and single node in
>>>>>>> statistics.js and we are using javascript code for prettifying only for
>>>>>>> collection node.
>>>>>>>
>>>>>>>
>>>>>>>> What we were thinking was to maybe not specify on the SQL level and
>>>>>>>> have the same format for "Size" everywhere in the UI.
>>>>>>>>
>>>>>>>>
>>>>>>> Here our main concern is inconsistency in "Size" format in the UI
>>>>>>> that can be avoided as I said earlier.
>>>>>>> We are using pg_size_pretty function for different fields like Size,
>>>>>>> Index Size, Table space size, Tuple length, Size of Temporary files in
>>>>>>> different modules and some of them are cell level and we don't require to
>>>>>>> put overhead on cell level fields as sorting is not required for individual
>>>>>>> node statistics.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Joao & Sarah
>>>>>>>>
>>>>>>>> On Tue, Apr 25, 2017 at 11:48 PM, Khushboo Vashi <
>>>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>>>
>>>>>>>>> Hi Joao & Sarah,
>>>>>>>>>
>>>>>>>>> Thanks for reviewing the patch.
>>>>>>>>>
>>>>>>>>> On Tue, Apr 25, 2017 at 8:34 PM, Joao Pedro De Almeida Pereira <
>>>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>>>
>>>>>>>>>> Hello Khushboo,
>>>>>>>>>>
>>>>>>>>>> We reviewed the this patch and have some suggestions:
>>>>>>>>>>
>>>>>>>>>> *Python:*
>>>>>>>>>> ​
>>>>>>>>>> The functionality for adding the "can_prettify" is repeated in
>>>>>>>>>> multiple places. Maybe this could be extracted into a function.
>>>>>>>>>>
>>>>>>>>>> When I have implemented this, my first thought is exactly same as
>>>>>>>>> you suggested but while looking at the code I felt its not a good idea to
>>>>>>>>> have a function. In case of a function, we need to pass the whole
>>>>>>>>> result-set as well as the list of fields which we need to be prettified.
>>>>>>>>> So, only for 2 lines, I didn't find any reason to make a function.
>>>>>>>>>
>>>>>>>>>> *Javascript:*
>>>>>>>>>> ​
>>>>>>>>>>
>>>>>>>>>> - The class Backgrid.SizeFormatter doesn't seem to have any
>>>>>>>>>> tests.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Sure, will do.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> - The function pg_size_pretty displays bytes and Kilobytes
>>>>>>>>>> differently.
>>>>>>>>>> - Is it possible to add PB as well?
>>>>>>>>>>
>>>>>>>>>> Will check and add PB.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -
>>>>>>>>>> - The function is a little bit hard to read, is it possible
>>>>>>>>>> to refactor using private functions like:
>>>>>>>>>>
>>>>>>>>>> Will make it more readable.
>>>>>>>>>
>>>>>>>>>> fromRaw: function (rawData, model) {
>>>>>>>>>> var unitIdx = findDataUnitIndex(rawData);
>>>>>>>>>> if (unitIdx == 0) {
>>>>>>>>>> return rawData + ' ' + this.dataUnits[i];
>>>>>>>>>> }
>>>>>>>>>> return formatOutput(rawData, unitIdx);
>>>>>>>>>> },
>>>>>>>>>>
>>>>>>>>>> ​
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> - In statistics.js:326 we believe it would make the code more
>>>>>>>>>> readable if we change the variable "c" to "rawColumn" and "col" to "column".
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I will change the variable name from "c" to "rawColumn" but I
>>>>>>>>> think "col" is appropriate as we already have columns variable and anyone
>>>>>>>>> can confuse while reading the code (for columns and column).
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> *SQL Files:*
>>>>>>>>>> ​
>>>>>>>>>>
>>>>>>>>>> - Is there a way to avoid conditionals here?
>>>>>>>>>> - Maybe we can use the same javascript function to prettify
>>>>>>>>>> all the sizes
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> In case of collection node (ex: Databases), I have implemented
>>>>>>>>> this functionality via putting a formatter for a back-grid column. So, it
>>>>>>>>> is applicable for the entire column not for the individual cell. To apply
>>>>>>>>> the javascript function on a single cell for the column (string column),
>>>>>>>>> first we need to identify that cell and then apply the function, I think
>>>>>>>>> this is overhead. Just to avoid conditional sql template I would not prefer
>>>>>>>>> this approach.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Visually we saw a difference between "Databases" statistics and a
>>>>>>>>>> specific database statistics. In "Databases" statistics the "Size" is "7.4
>>>>>>>>>> MB" but when you are in the specific database the "Size" is "7420 kB".
>>>>>>>>>> Is this the intended behavior?
>>>>>>>>>>
>>>>>>>>>> Only for the Databases (collection node), the client side
>>>>>>>>> functionality is implemented not for individual node , so this behaviour is
>>>>>>>>> different. For the individual node still, we are using pg_size_pretty
>>>>>>>>> function
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Joao & Sarah
>>>>>>>>>>
>>>>>>>>>> On Tue, Apr 25, 2017 at 7:58 AM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Ashesh, can you review/commit this please?
>>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Apr 25, 2017 at 10:18 AM, Khushboo Vashi <
>>>>>>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Fixed RM #2315 : Sorting by size is broken.
>>>>>>>>>>>>
>>>>>>>>>>>> Removed the pg_size_pretty function from query for the
>>>>>>>>>>>> collection and introduced the client side function to convert size into
>>>>>>>>>>>> human readable format. So, the sorting issue is fixed as the algorithm will
>>>>>>>>>>>> get the actual value of size instead of formatted value.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Khushboo
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Khushboo
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> Thanks,
>>>>>>> Khushboo
>>>>>>>
>>>>>>
>>>>>>
>>>>> Thanks,
>>>>> Khushboo
>>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> 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