Обсуждение: PATCH: Schema/Catalog Node [pgAdmin4]

Hi Dave,

As discussed, I have worked on this patch to improve some code level changes.

(because - Murtuza was not available due to some other engagement.)

Can you please review it, and share your feedback?

Hi

On Sun, Feb 28, 2016 at 6:49 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Dave,

As discussed, I have worked on this patch to improve some code level changes.

(because - Murtuza was not available due to some other engagement.)

Can you please review it, and share your feedback?

I think it's  mostly there. I've attached an updated patch where I've fixed a few minor issues as I read through the code. The following (likely simple) issues need to be fixed:

- Dropping a schema fails with an error message of "schema/pg/9.2_plus/sql/get_name.sql". 

- Creating a schema appears to fail with "'data' is undefined", however the schema is created, it's just that the dialogue doesn't close and the new schema isn't added to the tree.

- There is some discrepancy between default privileges as displayed on the properties summary, the edit dialogue, and the RE-SQL. As you can see in the screenshot, the SQL just GRANTS ALL, and the properties panel doesn't show anything.

Thanks.

-- 

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Mar 8, 2016 at 7:36 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Dave,

On Tue, Mar 8, 2016 at 12:20 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Dave,

On Thu, Mar 3, 2016 at 8:27 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Sun, Feb 28, 2016 at 6:49 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Dave,

As discussed, I have worked on this patch to improve some code level changes.

(because - Murtuza was not available due to some other engagement.)

Can you please review it, and share your feedback?

I think it's  mostly there. I've attached an updated patch where I've fixed a few minor issues as I read through the code. The following (likely simple) issues need to be fixed:

- Dropping a schema fails with an error message of "schema/pg/9.2_plus/sql/get_name.sql". 

Done. 

- Creating a schema appears to fail with "'data' is undefined", however the schema is created, it's just that the dialogue doesn't close and the new schema isn't added to the tree.

Done. 

- There is some discrepancy between default privileges as displayed on the properties summary, the edit dialogue, and the RE-SQL. As you can see in the screenshot, the SQL just GRANTS ALL, and the properties panel doesn't show anything.

Yes - there were some typos in the schema/catalog node implementation, which I have resolved now.

Please find the updated patch.

One more updated patch:

Some of the catalogs will not have all the schema child objects.

Hence - they will need to check certain thing likes they're not being loading in the catalog with such property (i.e. pg_catalog).

As per my conversation with Murtuza, who has already implemented catalog_obejcts for this kind of catalogs, these objects are only supported for catalogs like information_schema (and, PPAS specific dbo, sys).

To ease the work, I have introduced a class name SchemaChildModule, which does that job for us.

Please find the patch as per his input.

Hi Dave,

On Thu, Mar 3, 2016 at 8:27 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Sun, Feb 28, 2016 at 6:49 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Dave,

As discussed, I have worked on this patch to improve some code level changes.

(because - Murtuza was not available due to some other engagement.)

Can you please review it, and share your feedback?

I think it's  mostly there. I've attached an updated patch where I've fixed a few minor issues as I read through the code. The following (likely simple) issues need to be fixed:

- Dropping a schema fails with an error message of "schema/pg/9.2_plus/sql/get_name.sql". 

Done. 

- Creating a schema appears to fail with "'data' is undefined", however the schema is created, it's just that the dialogue doesn't close and the new schema isn't added to the tree.

Done. 

- There is some discrepancy between default privileges as displayed on the properties summary, the edit dialogue, and the RE-SQL. As you can see in the screenshot, the SQL just GRANTS ALL, and the properties panel doesn't show anything.

Yes - there were some typos in the schema/catalog node implementation, which I have resolved now.

Please find the updated patch.

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

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

--

Thanks & Regards,

Ashesh Vashi

Thanks.

-- 

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Dave,

On Tue, Mar 8, 2016 at 12:20 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Dave,

On Thu, Mar 3, 2016 at 8:27 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Sun, Feb 28, 2016 at 6:49 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Dave,

As discussed, I have worked on this patch to improve some code level changes.

(because - Murtuza was not available due to some other engagement.)

Can you please review it, and share your feedback?

I think it's  mostly there. I've attached an updated patch where I've fixed a few minor issues as I read through the code. The following (likely simple) issues need to be fixed:

- Dropping a schema fails with an error message of "schema/pg/9.2_plus/sql/get_name.sql". 

Done. 

- Creating a schema appears to fail with "'data' is undefined", however the schema is created, it's just that the dialogue doesn't close and the new schema isn't added to the tree.

Done. 

- There is some discrepancy between default privileges as displayed on the properties summary, the edit dialogue, and the RE-SQL. As you can see in the screenshot, the SQL just GRANTS ALL, and the properties panel doesn't show anything.

Yes - there were some typos in the schema/catalog node implementation, which I have resolved now.

Please find the updated patch.

One more updated patch:

Some of the catalogs will not have all the schema child objects.

Hence - they will need to check certain thing likes they're not being loading in the catalog with such property (i.e. pg_catalog).

To ease the work, I have introduced a class name SchemaChildModule, which does that job for us.

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

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

--

Thanks & Regards,

Ashesh Vashi

Thanks.

-- 

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Mar 8, 2016 at 2:18 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Tue, Mar 8, 2016 at 7:36 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Dave,

On Tue, Mar 8, 2016 at 12:20 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Dave,

On Thu, Mar 3, 2016 at 8:27 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Sun, Feb 28, 2016 at 6:49 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Dave,

As discussed, I have worked on this patch to improve some code level changes.

(because - Murtuza was not available due to some other engagement.)

Can you please review it, and share your feedback?

I think it's  mostly there. I've attached an updated patch where I've fixed a few minor issues as I read through the code. The following (likely simple) issues need to be fixed:

- Dropping a schema fails with an error message of "schema/pg/9.2_plus/sql/get_name.sql". 

Done. 

- Creating a schema appears to fail with "'data' is undefined", however the schema is created, it's just that the dialogue doesn't close and the new schema isn't added to the tree.

Done. 

- There is some discrepancy between default privileges as displayed on the properties summary, the edit dialogue, and the RE-SQL. As you can see in the screenshot, the SQL just GRANTS ALL, and the properties panel doesn't show anything.

Yes - there were some typos in the schema/catalog node implementation, which I have resolved now.

Please find the updated patch.

One more updated patch:

Some of the catalogs will not have all the schema child objects.

Hence - they will need to check certain thing likes they're not being loading in the catalog with such property (i.e. pg_catalog).

As per my conversation with Murtuza, who has already implemented catalog_obejcts for this kind of catalogs, these objects are only supported for catalogs like information_schema (and, PPAS specific dbo, sys).

To ease the work, I have introduced a class name SchemaChildModule, which does that job for us.

Please find the patch as per his input.

Can you split out the new changes please? I just spent 30 minutes tweaking the last patch.

 

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Mar 8, 2016 at 2:46 PM, Ashesh Vashi

<ashesh.vashi@enterprisedb.com> wrote:
> On Tue, Mar 8, 2016 at 8:08 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>>
>>
>> On Tue, Mar 8, 2016 at 2:18 PM, Ashesh Vashi
>> <ashesh.vashi@enterprisedb.com> wrote:
>>>
>>> On Tue, Mar 8, 2016 at 7:36 PM, Ashesh Vashi
>>> <ashesh.vashi@enterprisedb.com> wrote:
>>>>
>>>> Hi Dave,
>>>>
>>>> On Tue, Mar 8, 2016 at 12:20 AM, Ashesh Vashi
>>>> <ashesh.vashi@enterprisedb.com> wrote:
>>>>>
>>>>> Hi Dave,
>>>>>
>>>>>
>>>>> On Thu, Mar 3, 2016 at 8:27 PM, Dave Page <dpage@pgadmin.org> wrote:
>>>>>>
>>>>>> Hi
>>>>>>
>>>>>> On Sun, Feb 28, 2016 at 6:49 AM, Ashesh Vashi
>>>>>> <ashesh.vashi@enterprisedb.com> wrote:
>>>>>>>
>>>>>>> Hi Dave,
>>>>>>>
>>>>>>> As discussed, I have worked on this patch to improve some code level
>>>>>>> changes.
>>>>>>> (because - Murtuza was not available due to some other engagement.)
>>>>>>>
>>>>>>> Can you please review it, and share your feedback?
>>>>>>
>>>>>>
>>>>>> I think it's  mostly there. I've attached an updated patch where I've
>>>>>> fixed a few minor issues as I read through the code. The following (likely
>>>>>> simple) issues need to be fixed:
>>>>>>
>>>>>> - Dropping a schema fails with an error message of
>>>>>> "schema/pg/9.2_plus/sql/get_name.sql".
>>>>>
>>>>> Done.
>>>>>>
>>>>>>
>>>>>> - Creating a schema appears to fail with "'data' is undefined",
>>>>>> however the schema is created, it's just that the dialogue doesn't close and
>>>>>> the new schema isn't added to the tree.
>>>>>
>>>>> Done.
>>>>>>
>>>>>>
>>>>>> - There is some discrepancy between default privileges as displayed on
>>>>>> the properties summary, the edit dialogue, and the RE-SQL. As you can see in
>>>>>> the screenshot, the SQL just GRANTS ALL, and the properties panel doesn't
>>>>>> show anything.
>>>>>
>>>>> Yes - there were some typos in the schema/catalog node implementation,
>>>>> which I have resolved now.
>>>>>
>>>>> Please find the updated patch.
>>>>
>>>> One more updated patch:
>>>> Some of the catalogs will not have all the schema child objects.
>>>> Hence - they will need to check certain thing likes they're not being
>>>> loading in the catalog with such property (i.e. pg_catalog).
>>>
>>> As per my conversation with Murtuza, who has already implemented
>>> catalog_obejcts for this kind of catalogs, these objects are only supported
>>> for catalogs like information_schema (and, PPAS specific dbo, sys).
>>>>
>>>> To ease the work, I have introduced a class name SchemaChildModule,
>>>> which does that job for us.
>>>
>>> Please find the patch as per his input.
>>>
>>
>> Can you split out the new changes please? I just spent 30 minutes tweaking
>> the last patch.
>
> Sure.
> Here is the patch based on the v10 patch.

Thanks - patch committed. I made the following changes:

- Removed explicit support for 9.0 and below.
- Hid the default ACLs from the properties list for catalogs.
- Tidied up some of the SQL formatting

Open questions:

- We don't allow default ACLs to be specified when creating a schema
(neither does pgAdmin). Why not? Shouldn't we?

- We create new objects in 2 SQL statements, one that runs create.sql
and one that runs alter.sql to apply ACL, label options and more. I
strongly believe we need to push this into a single statement for all
object types, to ensure creation is completely atomic. Right now, you
can easily get an error by trying to set an unregistered security
label, which keeps the create dialogue open, however the object has
been successfully created.

Thoughts?

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Mar 8, 2016 at 9:23 PM, Dave Page <dpage@pgadmin.org> wrote:

On Tue, Mar 8, 2016 at 2:46 PM, Ashesh Vashi

<ashesh.vashi@enterprisedb.com> wrote:
> On Tue, Mar 8, 2016 at 8:08 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>>
>>
>> On Tue, Mar 8, 2016 at 2:18 PM, Ashesh Vashi
>> <ashesh.vashi@enterprisedb.com> wrote:
>>>
>>> On Tue, Mar 8, 2016 at 7:36 PM, Ashesh Vashi
>>> <ashesh.vashi@enterprisedb.com> wrote:
>>>>
>>>> Hi Dave,
>>>>
>>>> On Tue, Mar 8, 2016 at 12:20 AM, Ashesh Vashi
>>>> <ashesh.vashi@enterprisedb.com> wrote:
>>>>>
>>>>> Hi Dave,
>>>>>
>>>>>
>>>>> On Thu, Mar 3, 2016 at 8:27 PM, Dave Page <dpage@pgadmin.org> wrote:
>>>>>>
>>>>>> Hi
>>>>>>
>>>>>> On Sun, Feb 28, 2016 at 6:49 AM, Ashesh Vashi
>>>>>> <ashesh.vashi@enterprisedb.com> wrote:
>>>>>>>
>>>>>>> Hi Dave,
>>>>>>>
>>>>>>> As discussed, I have worked on this patch to improve some code level
>>>>>>> changes.
>>>>>>> (because - Murtuza was not available due to some other engagement.)
>>>>>>>
>>>>>>> Can you please review it, and share your feedback?
>>>>>>
>>>>>>
>>>>>> I think it's  mostly there. I've attached an updated patch where I've
>>>>>> fixed a few minor issues as I read through the code. The following (likely
>>>>>> simple) issues need to be fixed:
>>>>>>
>>>>>> - Dropping a schema fails with an error message of
>>>>>> "schema/pg/9.2_plus/sql/get_name.sql".
>>>>>
>>>>> Done.
>>>>>>
>>>>>>
>>>>>> - Creating a schema appears to fail with "'data' is undefined",
>>>>>> however the schema is created, it's just that the dialogue doesn't close and
>>>>>> the new schema isn't added to the tree.
>>>>>
>>>>> Done.
>>>>>>
>>>>>>
>>>>>> - There is some discrepancy between default privileges as displayed on
>>>>>> the properties summary, the edit dialogue, and the RE-SQL. As you can see in
>>>>>> the screenshot, the SQL just GRANTS ALL, and the properties panel doesn't
>>>>>> show anything.
>>>>>
>>>>> Yes - there were some typos in the schema/catalog node implementation,
>>>>> which I have resolved now.
>>>>>
>>>>> Please find the updated patch.
>>>>
>>>> One more updated patch:
>>>> Some of the catalogs will not have all the schema child objects.
>>>> Hence - they will need to check certain thing likes they're not being
>>>> loading in the catalog with such property (i.e. pg_catalog).
>>>
>>> As per my conversation with Murtuza, who has already implemented
>>> catalog_obejcts for this kind of catalogs, these objects are only supported
>>> for catalogs like information_schema (and, PPAS specific dbo, sys).
>>>>
>>>> To ease the work, I have introduced a class name SchemaChildModule,
>>>> which does that job for us.
>>>
>>> Please find the patch as per his input.
>>>
>>
>> Can you split out the new changes please? I just spent 30 minutes tweaking
>> the last patch.
>
> Sure.
> Here is the patch based on the v10 patch.

Thanks - patch committed. I made the following changes:

- Removed explicit support for 9.0 and below.
- Hid the default ACLs from the properties list for catalogs.
- Tidied up some of the SQL formatting

Open questions:

- We don't allow default ACLs to be specified when creating a schema
(neither does pgAdmin). Why not? Shouldn't we?

Hmm.. I don't see any reason, why we should not do it.

We have adopted that from pgAdmin III.

- We create new objects in 2 SQL statements, one that runs create.sql
and one that runs alter.sql to apply ACL, label options and more. I
strongly believe we need to push this into a single statement for all
object types, to ensure creation is completely atomic. Right now, you
can easily get an error by trying to set an unregistered security
label, which keeps the create dialogue open, however the object has
been successfully created.

Agree.

Even if they needed to be created from two, or more separate templates, they should ran together unless there are some statements, which require to run in separate transaction.