Re: pg_dump broken for non-super user

Rushabh,

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

There is no such limitation on using pg_dump as a non-superuser. It's
always been the case that you need to be able to LOCK the table that
you're dumping. If you don't have rights to LOCK a certain table then
pg_dump is going to throw an error just like the one you saw.

Existing versions of pg_dump would complain just the same if you ran
it with "pg_dump -t pg_authid".

> > > 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).

No. The point of locking the table is to allow us to gather information
about the table later on, which includes all of the components, not just
the definition or the 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 ?

The patch you've proposed is changing it so that we wouldn't always
execute LOCK TABLE.

Thanks!

Stephen