* Tom Lane (t...@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 and builds 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
signature.asc
Description: Digital signature