Re: pg_dump broken for non-super user - Mailing list pgsql-hackers

From Rushabh Lathia
Subject Re: pg_dump broken for non-super user
Date
Msg-id CAGPqQf0sAQBnMEJ9_UP7ya0Om1YuBOr_XK6LCnw8ME+vf-yP-Q@mail.gmail.com
Whole thread Raw
In response to Re: pg_dump broken for non-super user  (Stephen Frost <sfrost@snowman.net>)
Responses Re: pg_dump broken for non-super user  (Stephen Frost <sfrost@snowman.net>)
List pgsql-hackers


On Tue, May 3, 2016 at 8:34 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Rushabh Lathia (rushabh.lathia@gmail.com) wrote:
> With commit a9f0e8e5a2e779a888988cb64479a6723f668c84, now pg_dump, use a
> bitmap
> to represent what to include. With this commit if non-super user is unable
> to perform the dump.
[...]
> pg_dump: [archiver (db)] query was: LOCK TABLE pg_catalog.pg_authid IN
> ACCESS SHARE MODE

This does get addressed, in a way, in one of the patches that I've
currently got pending for pg_dump.  That particular change is actually
one for performance and therefore it's only going to help in cases where
none of the catalog tables have had their ACLs changed.

If the ACL has changed for any catalog table then pg_dump will still try
to LOCK the table, because we're going to dump out information about
that table (the ACLs on it).  I'm not sure if that's really an issue or
not..  Generally, if you're using pg_dump as a non-superuser, I'd expect
you to be limiting the tables you're dumping to ones you have access to
normally (using -n).

If its limitation that if someone is using pg_dump as a non-superuser, it
should be limiting the tables using -n, then better we document that. But
I don't think this is valid limitation. Comments ?
 

> getTables() take read-lock target tables to make sure they aren't DROPPED
> or altered in schema before we get around to dumping them. Here it having
> below condition to take  a lock:
>
>         if (tblinfo[i].dobj.dump && tblinfo[i].relkind == RELKIND_RELATION)
>
> which need to replace with:
>
>         if ((tblinfo[i].dobj.dump & DUMP_COMPONENT_DEFINITION) &&
>             tblinfo[i].relkind == RELKIND_RELATION)
>
> PFA patch to fix the issue.

I don't think we want to limit the cases where we take a lock to just
when we're dumping out the table definition..  Consider what happens if
someone drops the table before we get to dumping out the data in the
table, or gathering the ACLs on it (which happens much, much later).

Right, condition should check for (DUMP_COMPONENT_DEFINITION ||
DUMP_COMPONENT_DATA).

I am confused, in getTables() we executing LOCK TABLE, which holds the
share lock on that table till we get around to dumping them ( and that include
dumping data or gathering the ACLs).  isn't that right ?

 

Thanks!

Stephen



--
Rushabh Lathia

pgsql-hackers by date:

Previous
From: Vitaly Burovoy
Date:
Subject: Re: Make PG's "NOT NULL"s and attnotnull ("is_nullable") conform to SQL-2011
Next
From: Dean Rasheed
Date:
Subject: Re: psql :: support for \ev viewname and \sv viewname