Re: [pgadmin-hackers] Declarative partitioning in pgAdmin4

On Tue, Jul 4, 2017 at 3:59 PM, Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
wrote:

> On Mon, Jul 3, 2017 at 3:14 PM, Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Ashesh
>>
>> On Fri, Jun 30, 2017 at 12:46 PM, Ashesh Vashi <
>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> On Thu, Jun 29, 2017 at 5:02 PM, Ashesh Vashi <
>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Harshal,
>>>>
>>>> These are initial review comments.
>>>> 1.
>>>> Please share a separate patch for generic code changes from this patch
>>>> for the following files:
>>>> - web/pgadmin/tools/user_management/templates/user_management/
>>>> js/user_management.js
>>>> - web/pgadmin/static/js/backform.pgadmin.js
>>>> - web/pgadmin/static/js/backgrid.pgadmin.js
>>>>
>>>> This should be committed as separate functionality, and should not be
>>>> part of this commit.
>>>>
>>>
>> Committed.
>>
>>>
>>>> 2.
>>>> Please put a space after a colon (:) in javascript object definition.
>>>> i.e.
>>>> {cell: Backgrid.Extension.StringDepCell, cellHeaderClasses:
>>>> 'width_percent_30'}
>>>>
>>>> 3.
>>>> Conversion of ptid (partition table OID) to tid (table OID) for its
>>>> children must not be in 'web/pgadmin/browser/utils.py' file.
>>>> Please create a inherited class of PGChildNodeView, and extend the
>>>> functionality in it, and use it as the base class for all children of table.
>>>>
>>>> I will keep you posted for further review comments.
>>>>
>>> 4.
>>> URL definition of the javascript for tables & partition tables utility
>>> must be exposed from the table/partition table module, and not from the
>>> database module.
>>> i.e.
>>> No changes should be done in the database module for this feature. Hence
>>> - I don't expect any change in the file:
>>> web/pgadmin/browser/server_groups/servers/databases/__init__.py
>>>
>>
>> Fixed.
>>
>> Apart from above this patch contains test cases as well.
>>
> One bug:
> If I rename the partitioned table from the properties dialog, and attach a
> partition at the same time, then - it generates wrong modified queries.
>

Fixed. Attached is the latest patch

>
> -- Thanks, Ashesh
>
>>
>>> -- Thanks, Ashesh
>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Thanks & Regards,
>>>>
>>>> Ashesh Vashi
>>>> EnterpriseDB INDIA: Enterprise PostgreSQL Company
>>>> <http://www.enterprisedb.com>
>>>>
>>>>
>>>> *http://www.linkedin.com/in/asheshvashi*
>>>> <http://www.linkedin.com/in/asheshvashi>
>>>>
>>>> On Fri, Jun 23, 2017 at 6:55 PM, Harshal Dhumal <
>>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> Please find attached patch for partition support.
>>>>>
>>>>> This patch includes all the work done by Akashy and support for child
>>>>> nodes (constraints, rules, index, triggers and partition itself ) under
>>>>> partition node.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> --
>>>>> *Harshal Dhumal*
>>>>> *Sr. Software Engineer*
>>>>>
>>>>> EnterpriseDB India: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>> On Tue, Jun 20, 2017 at 12:16 AM, Shirley Wang <swang(at)pivotal(dot)io>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Mon, Jun 19, 2017 at 1:59 AM Akshay Joshi <
>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> On Fri, Jun 16, 2017 at 11:16 PM, Shirley Wang <swang(at)pivotal(dot)io>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Looks good. I noticed people clicking back and forth to the columns
>>>>>>>> tab to remember which columns they've created while filling out the
>>>>>>>> Expressions column. It might be better to have a list of the columns and
>>>>>>>> the datatype above the 'Partition Keys' subnode and have columns as a type
>>>>>>>> field rather than a drop down.
>>>>>>>>
>>>>>>>
>>>>>>> I think we should not duplicate that data as we already have all
>>>>>>> the information on "Columns" tab and by providing drop down user can select
>>>>>>> columns from there only.
>>>>>>>
>>>>>>>>
>>>>>>>> Also, I think the fields someone sees after selecting the Key type
>>>>>>>> needs to depend on what they select. Seeing both Column and Expressions
>>>>>>>> type field might lead someone to think they need to fill out both fields.
>>>>>>>>
>>>>>>>
>>>>>>> We can't, because user can select one column and provide an
>>>>>>> expression as partition key in this case we will have to show both the
>>>>>>> columns in subnode control. Anyways when user select columns I have
>>>>>>> disabled the expression cell and if user selects expression column cell is
>>>>>>> disabled.
>>>>>>>
>>>>>>
>>>>>> Ah I see what you mean. What I meant was that the column would change
>>>>>> depending on if someone selects Column or Expressions from the dropdown
>>>>>> [image: expression.png]
>>>>>> Can a user select more than one key type? The use case I can see
>>>>>> where hiding 'Columns' or 'Expressions' would fail is if someone can create
>>>>>> an expression key type and a column key type.
>>>>>>
>>>>>> Disabling a feature is one way to guide user behavior, but it doesn't
>>>>>> provide enough context for someone to understand why it's disabled. It's
>>>>>> better to only display what is absolutely necessary and hide fields that
>>>>>> are unrelated to the workflow.
>>>>>>
>>>>>> Typically, disabling a UI element is useful when that disabled UI
>>>>>> element also provides some context or value while disabled. In this case,
>>>>>> I'm not sure it is.
>>>>>>
>>>>>> If hiding options isn't possible, providing some text in the fields
>>>>>> (like N/A or --) would be helpful.
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>> [image: coluns_partitioning.png]
>>>>>>>> When is the 'In' column in the Partitions subnode enabled?
>>>>>>>>
>>>>>>>
>>>>>>> In case of 'List' Partition.
>>>>>>>
>>>>>>
>>>>>> It would improve the experience if the 'In' column was removed when a
>>>>>> user selects 'Range' partitions then. And then if a user is creating a
>>>>>> 'List' partition, 'From/To' should be hidden. In this case, 'From/To' and
>>>>>> 'In' are dependent on that first drop down step, so seeing 'In' while on
>>>>>> 'Range partitions' (and 'From/To' on 'List partitions') is not providing
>>>>>> any value.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>> For the NoteControl on the bottom, what do 'Mode Control' or
>>>>>>>> 'Attach Mode' refer to? And how can I tell the difference between 'Create
>>>>>>>> Mode' and 'Edit Mode'?
>>>>>>>>
>>>>>>>
>>>>>>> 'Mode control' is a switch control in subnode control that should
>>>>>>> be "Mode switch control". 'Create Mode' is when user creates the new table
>>>>>>> by clicking create-> table and 'Edit Mode' is when user open the properties
>>>>>>> dialog for the existing table. In case of 'Edit Mode' there are two ways
>>>>>>> user can create/attach partitions. In Attach mode we will identify and list
>>>>>>> down the suitable tables to be attached.
>>>>>>>
>>>>>>
>>>>>> I see. Describing these various states is great in case a user needs
>>>>>> it. What are your thoughts on having it live in the documentation of
>>>>>> pgAdmin4 rather than in the dialog? This seems to be the established
>>>>>> pattern for all other explanations.
>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>>
>> --
>> *Akshay Joshi*
>> *Principal Software Engineer *
>>
>>
>>
>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>
>
>

--
*Akshay Joshi*
*Principal Software Engineer *

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