Re: DROP OWNED BY fails to clean out pg_init_privs grants

Hi Daniel,

pg_upgrade is just one important user of pg_dump which is the one that
generates REVOKE for a non-existent role.

We should definitely also fix pg_dump, likely just checking that the
role exists when generating REVOKE commands (may be a good practice
for other cases too so instead of casting to ::regrole do the actual
join)

## here is the fix for pg_dump

While flying to Vancouver I looked around in pg_dump code, and it
looks like the easiest way to mitigate the dangling pg_init_priv
entries is to replace the query in pg_dump with one that filters out
invalid entries

The current query is at line 9336:

/* Fetch initial-privileges data */
if (fout->remoteVersion >= 90600)
{
printfPQExpBuffer(query,
"SELECT objoid, classoid, objsubid, privtype, initprivs "
"FROM pg_init_privs");

res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);

And we need the same but filtering out invalid aclitems from initprivs
something like this

WITH q AS (
SELECT objoid, classoid, objsubid, privtype, unnest(initprivs) AS
initpriv FROM saved_init_privs
)
SELECT objoid, classoid, objsubid, privtype, array_agg(initpriv) as initprivs
FROM q
WHERE is_valid_value_for_type(initpriv::text, 'aclitem')
GROUP BY 1,2,3,4;

### The proposed re[placement query:

Unfortunately we do not have an existing
is_this_a_valid_value_for_type(value text, type text, OUT res boolean)
function, so for a read-only workaround the following seems to work:

Here I first collect the initprivs array elements which fail the
conversion to text and back into an array and store it in GUC
pg_dump.bad_aclitems

Then I use this stored list to filter out the bad ones in the actual query.

DO $$
DECLARE
aclitem_text text;
bad_aclitems text[] = '{}';
BEGIN
FOR aclitem_text IN
SELECT DISTINCT unnest(initprivs)::text FROM pg_init_privs
LOOP
BEGIN /* try to convert back to aclitem */
PERFORM aclitem_text::aclitem;
EXCEPTION WHEN OTHERS THEN /* collect bad aclitems */
bad_aclitems := bad_aclitems || ARRAY[aclitem_text];
END;
END LOOP;
IF bad_aclitems != '{}' THEN
RAISE WARNING 'Ignoring bad aclitems "%" in pg_init_privs', bad_aclitems;
END IF;
PERFORM set_config('pg_dump.bad_aclitems', bad_aclitems::text,
false); -- true for trx-local
END;
$$;
WITH q AS (
SELECT objoid, classoid, objsubid, privtype, unnest(initprivs) AS
initpriv FROM pg_init_privs
)
SELECT objoid, classoid, objsubid, privtype, array_agg(initpriv) AS initprivs
FROM q
WHERE NOT initpriv::text = ANY
(current_setting('pg_dump.bad_aclitems')::text[])
GROUP BY 1,2,3,4;

--
Hannu

On Sun, May 26, 2024 at 11:27 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > On 26 May 2024, at 23:25, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Hannu Krosing <hannuk(at)google(dot)com> writes:
> >> Attached is a minimal patch to allow missing roles in REVOKE command
> >
> > FTR, I think this is a very bad idea.
>
> Agreed, this is papering over a bug. If we are worried about pg_upgrade it
> would be better to add a check to pg_upgrade which detects this case and
> advices the user how to deal with it.
>
> --
> Daniel Gustafsson
>