Re: [PATCHES] Fix for OWNER TO breaking ACLs

2004-08-01 Thread Tom Lane
Christopher Kings-Lynne [EMAIL PROTECTED] 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

---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]


Re: [PATCHES] Fix for OWNER TO breaking ACLs

2004-08-01 Thread Christopher Kings-Lynne
* 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
---(end of broadcast)---
TIP 8: explain analyze is your friend