Re: [pgadmin-hackers] Declarative partitioning in pgAdmin4

Hi Ashesh

On Fri, Jul 7, 2017 at 9:01 AM, Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
wrote:

> Hi Akshay,
>
> Please find the updated patch with the refresh logic.
>
> Please create redmine cases for the following todos:
> - PostgreSQL allows to create partioned table using 'OF TYPE'.
> - We should add collation, and opclass for 'partition by statement.
> - Allow to attach/create/detach partitions on the partition table
> directly, open the subcontrol in dialog for the same. (We support detaching
> an individual partitions, not the bulk operation.)
>

We are supporting detach as bulk operation from parent table. I'll
create RM's for the above todo's.

>
> Please review it, and commit it (if possible).
>

Code looks good to me. I have committed the code.
Thanks Harshal and Ashesh for your support to finish this feature.

>
> --
>
> 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 Tue, Jul 4, 2017 at 5:42 PM, Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>>
>>
>> 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*
>>
>
>

--
*Akshay Joshi*
*Principal Software Engineer *

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