Re: pg_dump broken for non-super user

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Why is it that we need to lock a table at all if we're just going to dump
> its ACL?  I understand the failure modes that motivate locking when we're
> going to dump data or schema, but the ACL is not really subject to that
> kind of problem: we are going to see a unitary, unchanging view of
> pg_class.relacl in our snapshot, and we aren't relying on any server-side
> logic to interpret that AFAIR.

I think I'm coming around to agree with that, but it seems like it'd be
better to look at each component and say "we know X is safe, so we won't
lock the table if we're only doing X" rather than saying "we only need to
lock the table for case X".

Also, we should document why we need to lock the table in some cases and
not others.  To that end, I'd like to make sure that it's clear what
cases we are considering here.

In particular, dumping the schema definition of a table may involve
calling server-side functions which will see changes to the table which
have happened after our transaction started, primairly due to SysCache
usage in those functions.  Any of the components which only look at the
tables in pg_catalog should be fine and not require us to lock the
table.

When considering the components:

- DEFINITION
 pg_get_viewdef(), pg_get_indexdef(), pg_get_constraintdef(), pg_get_triggerdef(), etc...

- DATA
 Depends on the table structure itself not changing.

- COMMENT
 Shouldn't require a lock, only uses a relatively simple query against pg_description.

- SECLABEL
 Similar to COMMENT, shouldn't require a lock.

- ACL
 ACL info is collected from pg_class relacl without any server-side functions being used which would impact the
result.

- POLICY
 Uses pg_get_expr(), which at least gets the relation name from SysCache, so we'll want to lock the table.

- USERMAP
 Uses pg_options_to_table(), but I don't think that actually uses SysCache at all, it's just taking the array provided
andbuilds a table out of it, so I think this case is ok.
 

If the above looks reasonable to others, I can write up a patch which
will skip locking the table if we are only looking to include components
that don't require a lock.  Since we only ever look at pg_catalog for
ACLs (except if a user explicitly asks to dump one of the tables in
pg_catalog), we shouldn't ever attempt to lock those tables.

Of course, the pg_dump would still end up including the ACLs for
pg_authid and whatever other tables the user has changed the ACLs on and
errors will be thrown during restore if the restore is done with a
non-superuser.

Thanks!

Stephen