Thread: Fix for OWNER TO breaking ACLs

Attached is a patch that fixes the owner change command on objects that
have privileges.

It probably needs a once over review since it involves a decent amount
of pointer arithmetic.

Note that languages don't have owners, and hence don't need fixing.

The owner change acl support is as follows:

1. If the acl is currently null, then don't do anything with the acl

2. If it is non null, then call aclnewowner() to get a new acl

3. aclnewowner() first looks for a grantee that is the new owner already
and remembers this item

4. A new acl is generated as a copy of the old acl, exlcuding the item
above if it exists.  During the copy, any grantors or grantees in the
new acl that refer to the old owner are changed to refer to the new owner.

5. The excluded acl item's rights (if it existed) are merged with those
of the old owner and become the new owner's rights.

Chris

Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
> Attached is a patch that fixes the owner change command on objects that
> have privileges.

Applied with revisions.  Just FYI ---

* The aclnewowner code wasn't really right at all.  It was changing
ai_grantee without checking whether that value was a UID, GID, or WORLD;
and it was not adequately handling the problem of merging duplicate
entries afterwards.  (You have to think about entries with the new owner
as grantor as well as such entries with grantee; and even considering
only the grantee case, it's wrong to suppose there can be only one.)
I replaced the logic with a general-purpose search for duplicate
entries, which might be overkill but it will reliably get the right
answer.

* You had consistently changed the simple_heap_update calls to do the
wrong thing.  (I'm surprised it didn't blow up on you in your testing.)
In a sequence like

        newtuple = heap_modifytuple(tup, rel, repl_val, repl_null, repl_repl);

        simple_heap_update(rel, &newtuple->t_self, newtuple);
        CatalogUpdateIndexes(rel, newtuple);

the second parameter to simple_heap_update *must* be newtuple->t_self
not tup->t_self.  The reason is that simple_heap_update stores the new
physical location of the updated tuple back into that parameter, and
then the CatalogUpdateIndexes call relies on newtuple->t_self to
generate new index entries.  The way you had it coded, it was generating
new index entries pointing at the old version of the tuple ...

            regards, tom lane

> * You had consistently changed the simple_heap_update calls to do the
> wrong thing.  (I'm surprised it didn't blow up on you in your testing.)
> In a sequence like
>
>         newtuple = heap_modifytuple(tup, rel, repl_val, repl_null, repl_repl);
>
>         simple_heap_update(rel, &newtuple->t_self, newtuple);
>         CatalogUpdateIndexes(rel, newtuple);
>
> the second parameter to simple_heap_update *must* be newtuple->t_self
> not tup->t_self.  The reason is that simple_heap_update stores the new
> physical location of the updated tuple back into that parameter, and
> then the CatalogUpdateIndexes call relies on newtuple->t_self to
> generate new index entries.  The way you had it coded, it was generating
> new index entries pointing at the old version of the tuple ...

Strange.  I guess I must have been testing with a database that had
short enough system catalogs that the indexes were never used?

Chris

Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
>> The way you had it coded, it was generating
>> new index entries pointing at the old version of the tuple ...

> Strange.  I guess I must have been testing with a database that had
> short enough system catalogs that the indexes were never used?

Could be, especially if you were using client-side queries to check the
result.  I think most of the system's internal catalog fetches are
hard-wired to use indexes except under the most dire circumstances
(mainly, a standalone backend with -P).  But a client-issued query
would do whatever the planner thought best.

            regards, tom lane