Обсуждение: BUG #14456: pg_dump doesn't restore permissions on tables belonging to an extension

VGhlIGZvbGxvd2luZyBidWcgaGFzIGJlZW4gbG9nZ2VkIG9uIHRoZSB3ZWJz
aXRlOgoKQnVnIHJlZmVyZW5jZTogICAgICAxNDQ1NgpMb2dnZWQgYnk6ICAg
ICAgICAgIERhbmllbGUgVmFycmF6em8KRW1haWwgYWRkcmVzczogICAgICBk
YW5pZWxlLnZhcnJhenpvQGdtYWlsLmNvbQpQb3N0Z3JlU1FMIHZlcnNpb246
IDkuNS40Ck9wZXJhdGluZyBzeXN0ZW06ICAgTGludXgKRGVzY3JpcHRpb246
ICAgICAgICAKCkFzIHBlciB0aXRsZS4gVG8gdGVzdDoNCg0KMSkgY3JlYXRl
IGFuIGV4dGVuc2lvbiBjb250YWluaW5nIGEgdGFibGUsIGUuZy46DQoNCiQg
Y2F0IC91c3Ivc2hhcmUvcG9zdGdyZXNxbC85LjUvZXh0ZW5zaW9uL3Rlc3Rl
eHQuY29udHJvbCANCmRlZmF1bHRfdmVyc2lvbiA9ICcxLjAnDQpjb21tZW50
ID0gJ3Rlc3Qgb2YgYSBwZyBidWcnDQpzdXBlcnVzZXIgPSBmYWxzZQ0KDQok
IGNhdCAvdXNyL3NoYXJlL3Bvc3RncmVzcWwvOS41L2V4dGVuc2lvbi90ZXN0
ZXh0LS0xLjAuc3FsIA0KY3JlYXRlIHRhYmxlIHRlc3R0YmwgKGlkIHNlcmlh
bCBwcmltYXJ5IGtleSwgZGF0YSB0ZXh0KTsNCnNlbGVjdCBwZ19jYXRhbG9n
LnBnX2V4dGVuc2lvbl9jb25maWdfZHVtcCgndGVzdHRibCcsICcnKTsNCg0K
DQoyKSBMb2FkIHRoZSBleHRlbnNpb24gaW4gdGhlIGRhdGFiYXNlDQoNCj0j
IGNyZWF0ZSBkYXRhYmFzZSB0ZXN0Ow0KQ1JFQVRFIERBVEFCQVNFDQoNCj0j
IFxjIHRlc3Q7DQp0ZXN0PSMgY3JlYXRlIHNjaGVtYSBleHRzY2hlbWE7DQpD
UkVBVEUgU0NIRU1BDQoNCnRlc3Q9IyBjcmVhdGUgZXh0ZW5zaW9uIHRlc3Rl
eHQgd2l0aCBzY2hlbWEgZXh0c2NoZW1hOw0KQ1JFQVRFIEVYVEVOU0lPTg0K
DQoNCjMpIEN1c3RvbWl6ZSBleHRlbnNpb24gdGFibGUgcGVybWlzc2lvbnMN
Cg0KdGVzdD0jIGNyZWF0ZSB1c2VyIHUyOw0KQ1JFQVRFIFJPTEUNCg0KdGVz
dD0jIGdyYW50IHNlbGVjdCBvbiBleHRzY2hlbWEudGVzdHRibCB0byB1MjsN
CkdSQU5UDQoNCnRlc3Q9IyBcZHBwIGV4dHNjaGVtYS50ZXN0dGJsDQogICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgQWNjZXNzIHByaXZpbGVnZXMN
CiAgU2NoZW1hICAgfCAgTmFtZSAgIHwgVHlwZSAgfCBBY2Nlc3MgcHJpdmls
ZWdlcyB8IENvbHVtbiBwcml2aWxlZ2VzIHwKUG9saWNpZXMgDQotLS0tLS0t
LS0tLSstLS0tLS0tLS0rLS0tLS0tLSstLS0tLS0tLS0tLS0tLS0tLS0tKy0t
LS0tLS0tLS0tLS0tLS0tLS0rLS0tLS0tLS0tLQ0KIGV4dHNjaGVtYSB8IHRl
c3R0YmwgfCB0YWJsZSB8IHBpcm89YXJ3ZER4dC9waXJvK3wgICAgICAgICAg
ICAgICAgICAgfCANCiAgICAgICAgICAgfCAgICAgICAgIHwgICAgICAgfCB1
Mj1yL3Bpcm8gICAgICAgICB8ICAgICAgICAgICAgICAgICAgIHwgDQooMSBy
b3cpDQoNCnRlc3Q9IyBpbnNlcnQgaW50byBleHRzY2hlbWEudGVzdHRibCAo
ZGF0YSkgdmFsdWVzICgnYXNkZicpOw0KSU5TRVJUIDAgMQ0KDQoNCjQpIENy
ZWF0ZSBhIG5ldyBkYXRhYmFzZSBhbmQgZHVtcC9yZXN0b3JlIGRhdGEgdGhl
cmUNCg0KdGVzdD0jIGNyZWF0ZSBkYXRhYmFzZSB0ZXN0MjsNCkNSRUFURSBE
QVRBQkFTRQ0KDQokIHBnX2R1bXAgdGVzdCB8IHBzcWwgLTEgdGVzdDINCg0K
DQo1KSBwZXJtaXNzaW9ucyBhcmUgbm90IHJlc3RvcmVkDQoNCnRlc3QyPSMg
XGRwcCBleHRzY2hlbWEudGVzdHRibA0KICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgIEFjY2VzcyBwcml2aWxlZ2VzDQogIFNjaGVtYSAgIHwgIE5h
bWUgICB8IFR5cGUgIHwgQWNjZXNzIHByaXZpbGVnZXMgfCBDb2x1bW4gcHJp
dmlsZWdlcyB8ClBvbGljaWVzIA0KLS0tLS0tLS0tLS0rLS0tLS0tLS0tKy0t
LS0tLS0rLS0tLS0tLS0tLS0tLS0tLS0tLSstLS0tLS0tLS0tLS0tLS0tLS0t
Ky0tLS0tLS0tLS0NCiBleHRzY2hlbWEgfCB0ZXN0dGJsIHwgdGFibGUgfCAg
ICAgICAgICAgICAgICAgICB8ICAgICAgICAgICAgICAgICAgIHwgDQooMSBy
b3cpDQoNCg0KTm90ZSB0aGF0IHVzaW5nIEFMVEVSIERFRkFVTFQgUFJJVklM
RUdFUyBkb2Vzbid0IHdvcmsgZWl0aGVyOiB0aGUgZGVmYXVsdApwcml2cyBv
biB0aGUgc2NoZW1hcyBhcmUgcmVzdG9yZWQgYWZ0ZXIgdGhlIGV4dGVuc2lv
biBpcyBjcmVhdGVkIHNvIHRoZQp0YWJsZXMgY3JlYXRlZCBieSB0aGUgZXh0
ZW5zaW9ucyBkb24ndCBiZW5lZml0IG9mIGl0LgoK

Greetings Daniele,

* daniele.varrazzo@gmail.com (daniele.varrazzo@gmail.com) wrote:
> PostgreSQL version: 9.5.4
[...]
> As per title. To test:

Prior to 9.6, dump/reload of an extension with custom ACLs wasn't
really supported.  This should work correctly in 9.6.  Unfortunately,
the changes required are too much to be able to back-patch to prior
versions of PG.

Thanks!

Stephen

Greetings Daniele,

* daniele.varrazzo@gmail.com (daniele.varrazzo@gmail.com) wrote:
> PostgreSQL version: 9.5.4
[...]
> As per title. To test:

Prior to 9.6, dump/reload of an extension with custom ACLs wasn't
really supported.  This should work correctly in 9.6.  Unfortunately,
the changes required are too much to be able to back-patch to prior
versions of PG.

Thanks!

Stephen

Greetings,

* Moshe Jacobson (moshe@neadwerx.com) wrote:
> My extension dynamically creates extension-owned tables and puts ACLs on
> them.

Ok, in 9.6, we should realize that your extension changed the ACLs for
those tables.

> When the database is dumped, it includes grants/revokes for those tables,
> which will not exist when the extension is re-installed.

When the database is dumped, it should include a CREATE EXTENSION
command.  It also shouldn't include GRANTs/REVOKEs unless the user
changed the permissions on the extension's tables from what they were
set to when the extension was installed.

> As a result, when the database is restored, I keep getting warnings because
> it's trying to apply ACLs to tables that don't exist.
>
> Is there a way around this issue?

A self-contained test case against 9.6 which shows the issue you're
having would really be the best way to help us.

If the issue is that you're working on a pre-9.6 version of PG, then I'm
afraid you'll need to upgrade or live with the warnings.

Thanks!

Stephen

Moshe Jacobson <moshe@neadwerx.com> writes:
> Scenario:

>    1. Extension is installed into its own schema. Installation is now
>    complete.
>    2. Extension creates a new table in its schema
>    3. Extension changes ACLs on the table.

Extensions are not actors, so claiming that "the extension" did something
is at best pretty fuzzy thinking.

>    4. After changing ACLs, the table is added to the extension (ALTER
>    EXTENSION)
>    5. A pg_dump of this database will now include ACL commands for the
>    table.

Hmm.  There's an argument to be made that ALTER EXTENSION ADD should
absorb whatever the object's current ACLs are into the pg_init_privs
entries for the extension.  (I don't think it does that now, though
I might be wrong.)  However ...

>    6. A pg_restore of this file will give warnings because the ACLs refer
>    to a table that is not created as part of the installation process.

I think this scenario is simply pilot error, or at least gross abuse of
the extension system.  If you dump and reload a DB containing an extension,
the extension definition that's fetched by CREATE EXTENSION is expected
to define (at least) all the objects that belonged to the extension in the
old DB.  You can't just randomly ALTER EXTENSION and not update the
extension definition script to match.

            regards, tom lane


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Moshe Jacobson <moshe@neadwerx.com> writes:
> > Scenario:
>
> >    1. Extension is installed into its own schema. Installation is now
> >    complete.
> >    2. Extension creates a new table in its schema
> >    3. Extension changes ACLs on the table.
>
> Extensions are not actors, so claiming that "the extension" did something
> is at best pretty fuzzy thinking.

Agreed.

> >    4. After changing ACLs, the table is added to the extension (ALTER
> >    EXTENSION)
> >    5. A pg_dump of this database will now include ACL commands for the
> >    table.
>
> Hmm.  There's an argument to be made that ALTER EXTENSION ADD should
> absorb whatever the object's current ACLs are into the pg_init_privs
> entries for the extension.  (I don't think it does that now, though
> I might be wrong.)  However ...

I've not gone and looked yet, but I doubt that it does.  I think I can
agree with the argument that it really should add those ACLs to
pg_init_privs.  Of course, any furhter manipulation of the ACLs from
that point will cause those ACLs to be included in the pg_dump.

I'll take a look at ALTER EXTENSION ADD and pg_init_privs.

> >    6. A pg_restore of this file will give warnings because the ACLs refer
> >    to a table that is not created as part of the installation process.
>
> I think this scenario is simply pilot error, or at least gross abuse of
> the extension system.  If you dump and reload a DB containing an extension,
> the extension definition that's fetched by CREATE EXTENSION is expected
> to define (at least) all the objects that belonged to the extension in the
> old DB.  You can't just randomly ALTER EXTENSION and not update the
> extension definition script to match.

Agreed.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Hmm.  There's an argument to be made that ALTER EXTENSION ADD should
>> absorb whatever the object's current ACLs are into the pg_init_privs
>> entries for the extension.  (I don't think it does that now, though
>> I might be wrong.)  However ...

> I've not gone and looked yet, but I doubt that it does.  I think I can
> agree with the argument that it really should add those ACLs to
> pg_init_privs.  Of course, any furhter manipulation of the ACLs from
> that point will cause those ACLs to be included in the pg_dump.

> I'll take a look at ALTER EXTENSION ADD and pg_init_privs.

By the same token, does ALTER EXTENSION DROP remove those entries?

            regards, tom lane


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> Hmm.  There's an argument to be made that ALTER EXTENSION ADD should
> >> absorb whatever the object's current ACLs are into the pg_init_privs
> >> entries for the extension.  (I don't think it does that now, though
> >> I might be wrong.)  However ...
>
> > I've not gone and looked yet, but I doubt that it does.  I think I can
> > agree with the argument that it really should add those ACLs to
> > pg_init_privs.  Of course, any furhter manipulation of the ACLs from
> > that point will cause those ACLs to be included in the pg_dump.
>
> > I'll take a look at ALTER EXTENSION ADD and pg_init_privs.
>
> By the same token, does ALTER EXTENSION DROP remove those entries?

I'll make sure it does.  My guess at the moment is that it doesn't.

Thanks!

Stephen

Moshe Jacobson <moshe@neadwerx.com> writes:
> On Thu, Jan 12, 2017 at 2:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think this scenario is simply pilot error, or at least gross abuse of
>> the extension system.  If you dump and reload a DB containing an extension,
>> the extension definition that's fetched by CREATE EXTENSION is expected
>> to define (at least) all the objects that belonged to the extension in the
>> old DB.  You can't just randomly ALTER EXTENSION and not update the
>> extension definition script to match.

> The reason I add the dynamically-created tables to the extension is so that
> they are never included in the pg_dump output. If this is a gross abuse of
> the extension system, is there another way you can suggest to mark these
> tables as not-to-be-dumped?

The extension mechanism definitely isn't meant to do that ;-).  Maybe
you could put these not-to-dump tables in their own schema and exclude
that schema from pg_dump with -N?

            regards, tom lane


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Tom Lane wrote:
> Moshe Jacobson <moshe@neadwerx.com> writes:
> > On Thu, Jan 12, 2017 at 2:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I think this scenario is simply pilot error, or at least gross abuse of
> >> the extension system.  If you dump and reload a DB containing an extension,
> >> the extension definition that's fetched by CREATE EXTENSION is expected
> >> to define (at least) all the objects that belonged to the extension in the
> >> old DB.  You can't just randomly ALTER EXTENSION and not update the
> >> extension definition script to match.
> 
> > The reason I add the dynamically-created tables to the extension is so that
> > they are never included in the pg_dump output. If this is a gross abuse of
> > the extension system, is there another way you can suggest to mark these
> > tables as not-to-be-dumped?
> 
> The extension mechanism definitely isn't meant to do that ;-).  Maybe
> you could put these not-to-dump tables in their own schema and exclude
> that schema from pg_dump with -N?

Is this related to this commit?
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=f2fcad27d59c8e5c48f8fa0a96c8355e40f24273

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Tom, Moshe,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> Hmm.  There's an argument to be made that ALTER EXTENSION ADD should
> >> absorb whatever the object's current ACLs are into the pg_init_privs
> >> entries for the extension.  (I don't think it does that now, though
> >> I might be wrong.)  However ...
>
> > I've not gone and looked yet, but I doubt that it does.  I think I can
> > agree with the argument that it really should add those ACLs to
> > pg_init_privs.  Of course, any furhter manipulation of the ACLs from
> > that point will cause those ACLs to be included in the pg_dump.
>
> > I'll take a look at ALTER EXTENSION ADD and pg_init_privs.
>
> By the same token, does ALTER EXTENSION DROP remove those entries?

Please find attached a WIP patch to have ALTER EXTENSION ADD/DROP update
pg_init_privs accordingly.

It's a bit big as it needs to have independent code for every different
kind of object that ALTER EXTENSION ADD/DROP supports.  Most of that
code is pretty boiler-plate, of course.

I'm planning to review it further, add more regression tests, and then
back-patch it to 9.6.

Moshe, if you're feeling adventurous and want to give it a spin and make
sure it behaves as you're expecting, that'd be great.

Tom, your thoughts and comments are always welcome, if you'd like to
peruse the patch, of course.

Otherwise, I expect to have time to wrap this all up over the weekend.

Thanks!

Stephen

* Moshe Jacobson (moshe@neadwerx.com) wrote:
> On Fri, Jan 20, 2017 at 5:10 PM Stephen Frost <sfrost@snowman.net> wrote:
> > Please find attached a WIP patch to have ALTER EXTENSION ADD/DROP update
> > pg_init_privs accordingly.
>
> Thanks so much Stephen.
>
> We're not used to compiling from source around here, so it may be difficult
> for me to find the time to test, but if I am able, I will let you know.

No problem.  I'm planning to push it later today and it'll be in 9.6.2.

Thanks!

Stephen

All,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> Hmm.  There's an argument to be made that ALTER EXTENSION ADD should
> >> absorb whatever the object's current ACLs are into the pg_init_privs
> >> entries for the extension.  (I don't think it does that now, though
> >> I might be wrong.)  However ...
>
> > I've not gone and looked yet, but I doubt that it does.  I think I can
> > agree with the argument that it really should add those ACLs to
> > pg_init_privs.  Of course, any furhter manipulation of the ACLs from
> > that point will cause those ACLs to be included in the pg_dump.
>
> > I'll take a look at ALTER EXTENSION ADD and pg_init_privs.
>
> By the same token, does ALTER EXTENSION DROP remove those entries?

I've pushed a fix for this and back-patched it to 9.6.

Thanks!

Stephen