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 ?
