Thread: REASSIGN OWNED lacks support for FDWs
As per http://archives.postgresql.org/pgsql-general/2012-02/msg00304.php there is no switch case in shdepReassignOwned for foreign data wrappers. The obvious short-term answer (and probably the only back-patchable one) is to add a case for that object type. But after all the refactoring that's been done in the general area of this type of command, I'm a bit surprised that shdepReassignOwned still looks like this. Can't we merge this knowledge into someplace where it doesn't have to be maintained separately? regards, tom lane
On Wed, Feb 15, 2012 at 12:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > As per http://archives.postgresql.org/pgsql-general/2012-02/msg00304.php > there is no switch case in shdepReassignOwned for foreign data wrappers. > > The obvious short-term answer (and probably the only back-patchable one) > is to add a case for that object type. But after all the refactoring > that's been done in the general area of this type of command, I'm a bit > surprised that shdepReassignOwned still looks like this. Can't we merge > this knowledge into someplace where it doesn't have to be maintained > separately? Hmm. I guess we could add function pointers to the ObjectProperty array in objectaddress.c. Then we could just search the array for the catalog ID and call the associated function through the function pointer, rather than having a switch in shdepReassignOwned(). Since anyone adding a new object type ought to be looking at objectaddress.c anyway, that would be one less place for people to forget to update. However, I'm not 100% sure that's an improvement. Switches by object type are probably not going to go away... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, Feb 15, 2012 at 12:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> As per http://archives.postgresql.org/pgsql-general/2012-02/msg00304.php >> there is no switch case in shdepReassignOwned for foreign data wrappers. >> >> The obvious short-term answer (and probably the only back-patchable one) >> is to add a case for that object type. �But after all the refactoring >> that's been done in the general area of this type of command, I'm a bit >> surprised that shdepReassignOwned still looks like this. �Can't we merge >> this knowledge into someplace where it doesn't have to be maintained >> separately? > Hmm. I guess we could add function pointers to the ObjectProperty > array in objectaddress.c. Then we could just search the array for the > catalog ID and call the associated function through the function > pointer, rather than having a switch in shdepReassignOwned(). Since > anyone adding a new object type ought to be looking at objectaddress.c > anyway, that would be one less place for people to forget to update. I was wondering more whether there isn't some single entry point that would allow access to ALTER OWNER functionality for any object type. If we still are in a situation where new shdepReassignOwned-specific code has to be written for every object type, it's not really much better. BTW, code freeze for the upcoming releases is Thursday ... is anyone going to actually fix this bug before then? I'm unlikely to find the time myself. regards, tom lane
Excerpts from Tom Lane's message of lun feb 20 12:37:45 -0300 2012: > Robert Haas <robertmhaas@gmail.com> writes: > > On Wed, Feb 15, 2012 at 12:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> As per http://archives.postgresql.org/pgsql-general/2012-02/msg00304.php > >> there is no switch case in shdepReassignOwned for foreign data wrappers. > >> > >> The obvious short-term answer (and probably the only back-patchable one) > >> is to add a case for that object type. But after all the refactoring > >> that's been done in the general area of this type of command, I'm a bit > >> surprised that shdepReassignOwned still looks like this. Can't we merge > >> this knowledge into someplace where it doesn't have to be maintained > >> separately? > > > Hmm. I guess we could add function pointers to the ObjectProperty > > array in objectaddress.c. Then we could just search the array for the > > catalog ID and call the associated function through the function > > pointer, rather than having a switch in shdepReassignOwned(). Since > > anyone adding a new object type ought to be looking at objectaddress.c > > anyway, that would be one less place for people to forget to update. > > I was wondering more whether there isn't some single entry point that > would allow access to ALTER OWNER functionality for any object type. > If we still are in a situation where new shdepReassignOwned-specific > code has to be written for every object type, it's not really much > better. > > BTW, code freeze for the upcoming releases is Thursday ... is anyone > going to actually fix this bug before then? I'm unlikely to find > the time myself. I'm gonna take a stab at fixing this now (the simple way). -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Excerpts from Alvaro Herrera's message of mar feb 21 15:54:03 -0300 2012: > Excerpts from Tom Lane's message of lun feb 20 12:37:45 -0300 2012: > > Robert Haas <robertmhaas@gmail.com> writes: > > > On Wed, Feb 15, 2012 at 12:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > >> As per http://archives.postgresql.org/pgsql-general/2012-02/msg00304.php > > >> there is no switch case in shdepReassignOwned for foreign data wrappers. > I'm gonna take a stab at fixing this now (the simple way). Here's the patch I have; last minute opinions. I intend to push a backpatch to 9.1 and 9.0. 8.4 has the same problem, but since Heikki punted in e356743f3ed45c36dcc4d0dbf6c1e8751b3d70b5, I'm not going to bother either. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
Alvaro Herrera <alvherre@commandprompt.com> writes: > Excerpts from Alvaro Herrera's message of mar feb 21 15:54:03 -0300 2012: >> Excerpts from Tom Lane's message of lun feb 20 12:37:45 -0300 2012: >>> As per http://archives.postgresql.org/pgsql-general/2012-02/msg00304.php >>> there is no switch case in shdepReassignOwned for foreign data wrappers. >> I'm gonna take a stab at fixing this now (the simple way). > Here's the patch I have; last minute opinions. I intend to push a > backpatch to 9.1 and 9.0. 8.4 has the same problem, but since Heikki > punted in e356743f3ed45c36dcc4d0dbf6c1e8751b3d70b5, I'm not going to > bother either. Looks roughly like what I expected, but I haven't tested it. regards, tom lane
(2012/02/22 9:30), Tom Lane wrote: > Alvaro Herrera<alvherre@commandprompt.com> writes: >> Excerpts from Alvaro Herrera's message of mar feb 21 15:54:03 -0300 2012: >>> Excerpts from Tom Lane's message of lun feb 20 12:37:45 -0300 2012: >>>> As per http://archives.postgresql.org/pgsql-general/2012-02/msg00304.php >>>> there is no switch case in shdepReassignOwned for foreign data wrappers. > >>> I'm gonna take a stab at fixing this now (the simple way). > >> Here's the patch I have; last minute opinions. I intend to push a >> backpatch to 9.1 and 9.0. 8.4 has the same problem, but since Heikki >> punted in e356743f3ed45c36dcc4d0dbf6c1e8751b3d70b5, I'm not going to >> bother either. > > Looks roughly like what I expected, but I haven't tested it. I did some tests. The results look good to me. Please find attached a logfile. My only concern on the patch is +static void +AlterForeignServerOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId) +{ + Form_pg_foreign_server form; - srvId = HeapTupleGetOid(tup); form = (Form_pg_foreign_server) GETSTRUCT(tup); if (form->srvowner != newOwnerId) @@ -366,10 +388,15 @@ AlterForeignServerOwner(const char *name, Oid newOwnerId) /* Superusers can always do it */ if (!superuser()) { I wonder if superusers can always do it. For example, is it OK for superusers to change the ownership of a foreign server owned by old_role to new_role that doesn't have USAGE privilege on its foreign data wrapper. Best regards, Etsuro Fujita
Attachment
Excerpts from Etsuro Fujita's message of mié feb 22 05:37:36 -0300 2012:
> I did some tests. The results look good to me. Please find attached a
> logfile.
Thanks.
> My only concern on the patch is
>
> +static void
> +AlterForeignServerOwner_internal(Relation rel, HeapTuple tup, Oid
> newOwnerId)
> +{
> + Form_pg_foreign_server form;
>
> - srvId = HeapTupleGetOid(tup);
> form = (Form_pg_foreign_server) GETSTRUCT(tup);
>
> if (form->srvowner != newOwnerId)
> @@ -366,10 +388,15 @@ AlterForeignServerOwner(const char *name, Oid
> newOwnerId)
> /* Superusers can always do it */
> if (!superuser())
> {
>
> I wonder if superusers can always do it. For example, is it OK for
> superusers to change the ownership of a foreign server owned by old_role
> to new_role that doesn't have USAGE privilege on its foreign data wrapper.
Well, permission checking are just what they were before the patch. I
did not change them here. I didn't participate in the discussions that
led to the current behavior, but as far as I know the guiding principle
here is that superusers always can do whatever they please. Maybe what
you point out is a bug in the behavior (both before and after my patch),
but if so, please raise it separately.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Excerpts from Tom Lane's message of mar feb 21 21:30:39 -0300 2012: > > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Excerpts from Alvaro Herrera's message of mar feb 21 15:54:03 -0300 2012: > >> Excerpts from Tom Lane's message of lun feb 20 12:37:45 -0300 2012: > >>> As per http://archives.postgresql.org/pgsql-general/2012-02/msg00304.php > >>> there is no switch case in shdepReassignOwned for foreign data wrappers. > > >> I'm gonna take a stab at fixing this now (the simple way). > > > Here's the patch I have; last minute opinions. I intend to push a > > backpatch to 9.1 and 9.0. 8.4 has the same problem, but since Heikki > > punted in e356743f3ed45c36dcc4d0dbf6c1e8751b3d70b5, I'm not going to > > bother either. > > Looks roughly like what I expected, but I haven't tested it. Thanks, pushed. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
(2012/02/23 5:32), Alvaro Herrera wrote:
>> My only concern on the patch is
>>
>> +static void
>> +AlterForeignServerOwner_internal(Relation rel, HeapTuple tup, Oid
>> newOwnerId)
>> +{
>> + Form_pg_foreign_server form;
>>
>> - srvId = HeapTupleGetOid(tup);
>> form = (Form_pg_foreign_server) GETSTRUCT(tup);
>>
>> if (form->srvowner != newOwnerId)
>> @@ -366,10 +388,15 @@ AlterForeignServerOwner(const char *name, Oid
>> newOwnerId)
>> /* Superusers can always do it */
>> if (!superuser())
>> {
>>
>> I wonder if superusers can always do it. For example, is it OK for
>> superusers to change the ownership of a foreign server owned by old_role
>> to new_role that doesn't have USAGE privilege on its foreign data wrapper.
>
> Well, permission checking are just what they were before the patch. I
> did not change them here. I didn't participate in the discussions that
> led to the current behavior, but as far as I know the guiding principle
> here is that superusers always can do whatever they please. Maybe what
> you point out is a bug in the behavior (both before and after my patch),
> but if so, please raise it separately.
OK. Thanks.
Best regards,
Etsuro Fujita