Re: Quick coding question with acl fixes

Hi,

Usually i'd ply away at this one until I figured it out, but we're
running out of time :)

If I have the nspForm variable (which is a Form_pg_namespace) struct
pointer, then how do I check if the nspacl field is default (ie.
"NULL"). This is so I can avoid running DatumGetAclP on it, which seems
to cause alloc error reports.

gdb seems to think it's "$3 = {126}", and that doesn't seem to be NULL
or Datum(0) or anything. Anyone know the answer?

Also, there is an implementation question?

When I run through the acl changing all references from the old owner to
the new owner, should I combine the resulting acls if possible? Because
if the new owner already has some acls on that item, they will end up
with two acls.

Chris

Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
> If I have the nspForm variable (which is a Form_pg_namespace) struct
> pointer, then how do I check if the nspacl field is default (ie.
> "NULL"). This is so I can avoid running DatumGetAclP on it, which seems
> to cause alloc error reports.

Best practice is to use SysCacheGetAttr if you are dealing with a row
from cache or heap_getattr if it's from a table scan. For instance
in aclchk.c there is

aclDatum = SysCacheGetAttr(NAMESPACENAME, tuple,
Anum_pg_namespace_nspacl,
&isNull);
if (isNull)
old_acl = acldefault(ACL_OBJECT_NAMESPACE, ownerId);
else
/* get a detoasted copy of the ACL */
old_acl = DatumGetAclPCopy(aclDatum);

> When I run through the acl changing all references from the old owner to
> the new owner, should I combine the resulting acls if possible? Because
> if the new owner already has some acls on that item, they will end up
> with two acls.

If possible ... how painful would it be to do?

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
>> When I run through the acl changing all references from the old owner to
>> the new owner, should I combine the resulting acls if possible? Because
>> if the new owner already has some acls on that item, they will end up
>> with two acls.

> If possible ... how painful would it be to do?

Actually it looks like you'd better, because for example aclupdate
assumes there's only one entry for a given grantor/grantee pair.

BTW, are you sure Fabien did not already solve this problem in his
pending patch?

regards, tom lane

>>If possible ... how painful would it be to do?

I'm yet to do that part, so I guess I'll find out.

> Actually it looks like you'd better, because for example aclupdate
> assumes there's only one entry for a given grantor/grantee pair.

OK, many thanks for the prompt reply :)

> BTW, are you sure Fabien did not already solve this problem in his
> pending patch?

You mean schema ownership? I thought that was just upon the first
connection to a database or something? I'm using schemas as my first
case for fixing OWNER TO commands and acls...

Chris

Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
>> BTW, are you sure Fabien did not already solve this problem in his
>> pending patch?

> You mean schema ownership? I thought that was just upon the first
> connection to a database or something?

Yeah, but the point was that he was doing an ALTER OWNER and needed to
fix the ACL to match. I thought he claimed to have written the needed
subroutine. I have not yet looked at his patch though.

regards, tom lane

> Yeah, but the point was that he was doing an ALTER OWNER and needed to
> fix the ACL to match. I thought he claimed to have written the needed
> subroutine. I have not yet looked at his patch though.

I think Fabien's owner changing routine will end up being a strict
subset of my routine. I think his just happens to only work on the
newly created public and info_schema in a new db. It's not complex
enough to work on arbitrary acls. Also, his needs to work as a public
SQL function:

+ /* acl acl_switch_grantor(acl, oldgrantor, newgrantor);
+ * switch grantor id in aclitem array.
+ * used internally for fixing owner rights in new databases.
+ * must be STRICT.
+ */
+ Datum acl_switch_grantor(PG_FUNCTION_ARGS)
+ {
+ Acl * acls = PG_GETARG_ACL_P_COPY(0);
+ int i,
+ old_grantor = PG_GETARG_INT32(1),
+ new_grantor = PG_GETARG_INT32(2);
+ AclItem * item;
+
+ for (i=0, item=ACL_DAT(acls); i<ACL_NUM(acls); i++, item++)
+ if (item->ai_grantor == old_grantor)
+ item->ai_grantor = new_grantor;
+
+ PG_RETURN_ACL_P(acls);
+ }

Chris

>>When I run through the acl changing all references from the old owner to
>>the new owner, should I combine the resulting acls if possible? Because
>>if the new owner already has some acls on that item, they will end up
>>with two acls.
>
> If possible ... how painful would it be to do?

I'm pretty close to finishing this. Just applying it to all the
acl'able objects atm. It's a pain when you can only find 15 mins every
other day to work on it :(

Chris