Hi Hüseyin,
I reviewed and tested v6. The filtering logic is correct and applied in the
right place: doing it in the source queries (getAdditionalACLs and the
column-level ACL query) means the dangling entries never reach the
binary-upgrade "SET SESSION AUTHORIZATION <oid>" path, which is where the
upgrade actually broke. Using pg_roles instead of pg_authid for the
existence check is also right for non-superuser pg_dump. With the test
running, all of its assertions pass.
Two things on the test:
1. The TAP test doesn't run for me at all -- it dies in setup with
"role \"rui\" does not exist". The cause is that the aclitem literals
are built by concatenating current_user unquoted, e.g.
ARRAY[('ghost_grantee=X/' || current_user)::aclitem]
My bootstrap superuser is "rui.zhao", so this becomes
'ghost_grantee=X/rui.zhao', and aclitemin parses the grantor only up to
the dot:
=# SELECT ('g=X/' || 'a.b')::aclitem;
ERROR: role "a" does not exist
So the test fails before any assertion runs on any cluster whose
superuser name needs quoting (a dot, uppercase, etc.). Wrapping it as
quote_ident(current_user) in the four aclitem literals fixes it (the
test then passes 12/12 here). A bit ironic given the patch is about
handling odd role names.
2. The PUBLIC case (grantee = 0) isn't covered. The "ace.grantee <> 0"
branch is what keeps PUBLIC grants from being filtered, but there's no
test for either direction: a valid PUBLIC grant ("=r/validgrantor")
being kept, or a PUBLIC grant whose grantor is dangling ("=r/ghost")
being dropped. Worth a case or two.
Thanks,
Rui
