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@yesql.se> wrote:
>
> > On 26 May 2024, at 23:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Hannu Krosing <hannuk@google.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
>