Re: [pgadmin-hackers] Declarative partitioning in pgAdmin4 - Mailing list pgadmin-hackers

From Ashesh Vashi
Subject Re: [pgadmin-hackers] Declarative partitioning in pgAdmin4
Date
Msg-id CAG7mmozGNYZ+PZMPgnfuTa4pwbNCZnzaxXxGqNOnChiC97rk8A@mail.gmail.com
Whole thread Raw
In response to Re: [pgadmin-hackers] Declarative partitioning in pgAdmin4  (Akshay Joshi <akshay.joshi@enterprisedb.com>)
Responses Re: [pgadmin-hackers] Declarative partitioning in pgAdmin4
List pgadmin-hackers
On Mon, Jul 3, 2017 at 3:14 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi Ashesh

On Fri, Jun 30, 2017 at 12:46 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
On Thu, Jun 29, 2017 at 5:02 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.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.

-- Thanks, Ashesh 

-- Thanks, Ashesh


--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company


http://www.linkedin.com/in/asheshvashi


On Fri, Jun 23, 2017 at 6:55 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.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@pivotal.io> wrote:


On Mon, Jun 19, 2017 at 1:59 AM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
On Fri, Jun 16, 2017 at 11:16 PM, Shirley Wang <swang@pivotal.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
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.

 

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-9517
Mobile: +91 976-788-8246

pgadmin-hackers by date:

Previous
From: Akshay Joshi
Date:
Subject: Re: [pgadmin-hackers] Declarative partitioning in pgAdmin4
Next
From: Dave Page
Date:
Subject: Re: [pgAdmin4][Patch]: Fixed the issue related to Domain Constraint module