Re: PATCH: Schema/Catalog Node [pgAdmin4]

On Tue, Mar 8, 2016 at 4:34 PM, Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
wrote:

> On Tue, Mar 8, 2016 at 9:23 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> On Tue, Mar 8, 2016 at 2:46 PM, Ashesh Vashi
>> <ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>> > On Tue, Mar 8, 2016 at 8:08 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> >>
>> >>
>> >>
>> >> On Tue, Mar 8, 2016 at 2:18 PM, Ashesh Vashi
>> >> <ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>> >>>
>> >>> On Tue, Mar 8, 2016 at 7:36 PM, Ashesh Vashi
>> >>> <ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>> >>>>
>> >>>> Hi Dave,
>> >>>>
>> >>>> On Tue, Mar 8, 2016 at 12:20 AM, Ashesh Vashi
>> >>>> <ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>> >>>>>
>> >>>>> Hi Dave,
>> >>>>>
>> >>>>>
>> >>>>> On Thu, Mar 3, 2016 at 8:27 PM, Dave Page <dpage(at)pgadmin(dot)org>
>> wrote:
>> >>>>>>
>> >>>>>> Hi
>> >>>>>>
>> >>>>>> On Sun, Feb 28, 2016 at 6:49 AM, Ashesh Vashi
>> >>>>>> <ashesh(dot)vashi(at)enterprisedb(dot)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.
>

OK, I added tasks to do both to our internal tracker.

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

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