Re: pg_dump broken for non-super user

On Tue, May 3, 2016 at 8:34 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> * Rushabh Lathia (rushabh(dot)lathia(at)gmail(dot)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