Обсуждение: PATCH: Ticket#99: Support for Default Privileges

Hi Ashesh,

Le 05/04/2010 12:11, Ashesh Vashi a écrit :
> [...]
> PFA: patch for the support for Default Privileges.
> http://developer.postgresql.org/pgdocs/postgres/sql-alterdefaultprivileges.html
>

Just a quick note. Your patch applies cleanly, but doesn't compile for
me. I get all those errors (see attached file). I don't have time to
work on this right now, but will do so this afternoon.


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com

Le 05/04/2010 13:22, Ashesh Vashi a écrit :
> Hi Guillaume,
>
> I am able to compile the patch well on my side.
> I faced this issue, when I tried to apply the patch, when
> include/ctl/ctlDefaultPrivileges.h and ctl/ctlDefaultPrivileges.cpp. I had
> to remove these two files before applying the patch.
>
> Would you please verify on your side?
>

I don't have these two files, so I didn't need to remove them. Should
the patch create them? because they aren't part of it.


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com

Le 05/04/2010 13:52, Ashesh Vashi a écrit :
> hmm.. That's strange...
> It should..  :-(
>
> The log, sent by you, suggests that, they were created on your system..
> i.e.
> */include/wx-2.8 -D_FILE_OFFSET_BITS=64 -D_LARGE_FILES -D__WXDEBUG__
> -D__WXGTK__ -g -O0 -I/usr/include/libxml2 -I/usr/include/libxml2
> -DDATA_DIR=\"/opt/pgadmin-trunk/share/pgadmin3/\" -Wall
> -Wno-non-virtual-dtor -fno-strict-aliasing -I../pgadmin/include   -MT
> ctlDefaultSecurityPanel.o -MD -MP -MF .deps/ctlDefaultSecurityPanel.Tpo -c
> -o ctlDefaultSecurityPanel.o `test -f './ctl/ctlDefaultSecurityPanel.cpp' ||
> echo './'`./ctl/ctlDefaultSecurityPanel.cpp*
> *./ctl/ctlDefaultSecurityPanel.cpp:812: error: ISO C++ does not permit
> ‘wxPanel::sm_eventTable’ to be defined as
> ‘ctlDefaultSecurityPanel::sm_eventTable’*
> */opt/wxgtk-2.8/include/wx-2.8/wx/generic/panelg.h:91: error: ‘const
> wxEventTableEntry wxPanel::sm_eventTableEntries []’ is private*
> *./ctl/ctlDefaultSecurityPanel.cpp:812: error: within this context*
>

You were talking about ctlDefaultPrivileges.h and
ctlDefaultPrivileges.cpp, not ctlDefaultSecurityPanel.cpp, weren't you?


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com

On Thu, Apr 22, 2010 at 12:07 PM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> Please find the updated patch.
> Sorry for inconvenience..

I'm seeing this in the link phase:

Undefined symbols for architecture ppc:
  "dlgSecurityProperty::SetPrivilegesLayout()", referenced from:
      dlgPackage::OnChangeSize(wxSizeEvent&)        in dlgPackage.o
      dlgSchema::OnChangeSize(wxSizeEvent&)       in dlgSchema.o
      dlgSequence::OnChangeSize(wxSizeEvent&)        in dlgSequence.o
      dlgTablespace::OnChangeSize(wxSizeEvent&)        in dlgTablespace.o
ld: symbol(s) not found for architecture ppc
collect2: ld returned 1 exit status
Undefined symbols for architecture i386:
  "dlgSecurityProperty::SetPrivilegesLayout()", referenced from:
      dlgPackage::OnChangeSize(wxSizeEvent&)        in dlgPackage.o
      dlgSchema::OnChangeSize(wxSizeEvent&)       in dlgSchema.o
      dlgSequence::OnChangeSize(wxSizeEvent&)        in dlgSequence.o
      dlgTablespace::OnChangeSize(wxSizeEvent&)        in dlgTablespace.o
ld: symbol(s) not found for architecture i386
collect2: ld returned 1 exit status

Do we need to buy you a mac? :-)

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

Le 23/04/2010 23:27, Ashesh Vashi a écrit :
> Hi Dave,
>
> On Sat, Apr 24, 2010 at 1:47 AM, Dave Page <dave.page@enterprisedb.com>wrote:
>
>> On Thu, Apr 22, 2010 at 12:07 PM, Ashesh Vashi
>> <ashesh.vashi@enterprisedb.com> wrote:
>>> Please find the updated patch.
>>> Sorry for inconvenience..
>>
>> I'm seeing this in the link phase:
>>
>> Undefined symbols for architecture ppc:
>>  "dlgSecurityProperty::SetPrivilegesLayout()", referenced from:
>>      dlgPackage::OnChangeSize(wxSizeEvent&)        in dlgPackage.o
>>      dlgSchema::OnChangeSize(wxSizeEvent&)       in dlgSchema.o
>>      dlgSequence::OnChangeSize(wxSizeEvent&)        in dlgSequence.o
>>      dlgTablespace::OnChangeSize(wxSizeEvent&)        in dlgTablespace.o
>> ld: symbol(s) not found for architecture ppc
>> collect2: ld returned 1 exit status
>> Undefined symbols for architecture i386:
>>  "dlgSecurityProperty::SetPrivilegesLayout()", referenced from:
>>      dlgPackage::OnChangeSize(wxSizeEvent&)        in dlgPackage.o
>>      dlgSchema::OnChangeSize(wxSizeEvent&)       in dlgSchema.o
>>      dlgSequence::OnChangeSize(wxSizeEvent&)        in dlgSequence.o
>>      dlgTablespace::OnChangeSize(wxSizeEvent&)        in dlgTablespace.o
>> ld: symbol(s) not found for architecture i386
>> collect2: ld returned 1 exit status
>>
> Please find the updated patch.
>
>
>>
>> Do we need to buy you a mac? :-)
>>
> I think - that's a good idea.. :P
>

+1 for Ashesh to get a Mac :)


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com

Hi Ashesh

On Fri, Apr 23, 2010 at 10:27 PM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> Please find the updated patch.
>

OK, this builds now. I still see a few issues that need resolving though:

- The layout needs some work - see the attached screenshot. On the
Tables tab, the listview doesn't have any space, yet there's a blank
area at the bottom (I guess it needs to be made the expandable row in
the layout grid).

- The only place we should be using all caps is for the permissions
names. The 'PRIVILEGES ON', 'TABLES', 'SEQUENCES', 'FUNCTIONS' should
all be init-capped. The default permission for 'PUBLIC' should be
'public', per the existing permissions tabs.

- The label by the combo box should say 'Role' not 'Group'.

- The column widths in the listview need more appropriately sizing.

- In some cases, if I click the Add button without selecting a Role, I
get a row added to the listview with an icon but no text. In others, I
get a crash.

- Default ACLs aren't reverse engineered and shown in the SQL pane for
databases or schemas.

- Default ACLs aren't shown in the Properties dialog for Databases
(seems to work for Schemas, though I couldn't check the Tables tab).

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

On Mon, Apr 26, 2010 at 2:01 PM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
>> - Default ACLs aren't shown in the Properties dialog for Databases
>> (seems to work for Schemas, though I couldn't check the Tables tab).
>
> It should be available for the existing objects, not for new object creation
> for the database.

I was working on an existing 'postgres' database containing a 'public'
schema. I added default privileges to both, and re-opened the
properties dialogues, but only the schema one showed the changes I
made.



--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

On Tue, Apr 27, 2010 at 10:02 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> Hi Dave,
> Please find the updated patch.
> It has all the changes - as you suggested.

Close - but not quite there :-)

- The listview that lists the privileges is still a fixed height, and
on Mac, it pushes the TRIGGER privilege below the bottom of the
control area. Please see the two attached screenshots. If you make the
listview the variable height component (as it is on the privileges
tab, it'll make sizing work properly.

- The listview columns are still too small by default, as can be seen
in the screenshot. Compare them to the sizes of the equivalent columns
on the 'Privileges' tab.

- TABLES, SEQUENCES and FUNCTIONS are still in upper case, and not init-capped.

- The labels in the properties list view shouldn't have each work
init-capped (and we can shorten them a little) - eg. "Default
Privileges On Tables" should be "Default table ACL" (we use ACL
already in the list).

- There is no blank line between the table definition and the first
ALTER DEFAULT... statement in the SQL pane, eg.:

=====
CREATE DATABASE postgres
  WITH OWNER = postgres
       ENCODING = 'UTF8'
       TABLESPACE = pg_default
       LC_COLLATE = 'C'
       LC_CTYPE = 'C'
       CONNECTION LIMIT = -1;
ALTER DEFAULT PRIVILEGES
    GRANT INSERT, SELECT, UPDATE, DELETE, TRUNCATE, REFERENCES,
TRIGGER ON TABLES
    TO postgres;

ALTER DEFAULT PRIVILEGES
    GRANT INSERT, SELECT, UPDATE, DELETE, TRUNCATE, REFERENCES,
TRIGGER ON TABLES
    TO public;

ALTER DEFAULT PRIVILEGES
    GRANT SELECT, UPDATE, USAGE ON SEQUENCES
    TO postgres;
=====

Otherwise, it's looks like it's now working as expected :-)

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

Apologies - I forgot to attach the screenshots.

On Tue, Apr 27, 2010 at 1:58 PM, Dave Page <dpage@pgadmin.org> wrote:
> On Tue, Apr 27, 2010 at 10:02 AM, Ashesh Vashi
> <ashesh.vashi@enterprisedb.com> wrote:
>> Hi Dave,
>> Please find the updated patch.
>> It has all the changes - as you suggested.
>
> Close - but not quite there :-)
>
> - The listview that lists the privileges is still a fixed height, and
> on Mac, it pushes the TRIGGER privilege below the bottom of the
> control area. Please see the two attached screenshots. If you make the
> listview the variable height component (as it is on the privileges
> tab, it'll make sizing work properly.
>
> - The listview columns are still too small by default, as can be seen
> in the screenshot. Compare them to the sizes of the equivalent columns
> on the 'Privileges' tab.
>
> - TABLES, SEQUENCES and FUNCTIONS are still in upper case, and not init-capped.
>
> - The labels in the properties list view shouldn't have each work
> init-capped (and we can shorten them a little) - eg. "Default
> Privileges On Tables" should be "Default table ACL" (we use ACL
> already in the list).
>
> - There is no blank line between the table definition and the first
> ALTER DEFAULT... statement in the SQL pane, eg.:
>
> =====
> CREATE DATABASE postgres
>  WITH OWNER = postgres
>       ENCODING = 'UTF8'
>       TABLESPACE = pg_default
>       LC_COLLATE = 'C'
>       LC_CTYPE = 'C'
>       CONNECTION LIMIT = -1;
> ALTER DEFAULT PRIVILEGES
>    GRANT INSERT, SELECT, UPDATE, DELETE, TRUNCATE, REFERENCES,
> TRIGGER ON TABLES
>    TO postgres;
>
> ALTER DEFAULT PRIVILEGES
>    GRANT INSERT, SELECT, UPDATE, DELETE, TRUNCATE, REFERENCES,
> TRIGGER ON TABLES
>    TO public;
>
> ALTER DEFAULT PRIVILEGES
>    GRANT SELECT, UPDATE, USAGE ON SEQUENCES
>    TO postgres;
> =====
>
> Otherwise, it's looks like it's now working as expected :-)
>
> --
> Dave Page
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise Postgres Company
>



--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

On Tue, Apr 27, 2010 at 5:52 PM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> Hi Dave,
> PFA the updated patch.

Thanks Ashesh - applied with a few minor tweaks.

As I've got fed up adding [Ashesh Vashi] to most lines I add to the
changelog now, I also added your name to the list of developers at the
top so your patches will be listed with the developer AV from now on
:-)

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

Le 27/04/2010 21:25, Ashesh Vashi a écrit :
> On Wed, Apr 28, 2010 at 12:50 AM, Dave Page <dpage@pgadmin.org> wrote:
>
>> On Tue, Apr 27, 2010 at 5:52 PM, Ashesh Vashi
>> <ashesh.vashi@enterprisedb.com> wrote:
>>> Hi Dave,
>>> PFA the updated patch.
>>
>> Thanks Ashesh - applied with a few minor tweaks.
>>
>> As I've got fed up adding [Ashesh Vashi] to most lines I add to the
>> changelog now, I also added your name to the list of developers at the
>> top so your patches will be listed with the developer AV from now on
>> :-)
>>
> Wow..
> I feel like I am honoured... :-)
> Thanks..
>

Congrats, Ashesh :)


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com