On Tue, May 3, 2016 at 8:34 PM, Stephen Frost <sfr...@snowman.net> wrote:
> * Rushabh Lathia (rushabh.lat...@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
