Re: PATCH: Schema/Catalog Node [pgAdmin4]

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.

--

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>

>
> Thoughts?
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>