From 307254746851ce34ff891f0b18d7a1dafeaf037c Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Thu, 16 Mar 2023 11:46:08 -0700 Subject: [PATCH v3 2/2] pg_dump: skip lock for extension tables without policies [backpatch to 11] If a user without SELECT permissions on an internal extension table tries to dump the extension, the dump will fail while trying to lock the table with ACCESS SHARE, even though the user doesn't want or need to dump the table in question. (The lock is taken to allow later pg_get_expr() calls on pg_policy to remain consistent in the face of concurrent schema changes.) It'd be ideal not to require SELECT permissions on a table to be able to dump its policies, but I don't have a great idea for how to implement that without races. As a workaround, skip the policy queries entirely if we can determine that no policies exist for a table at the time of getTables(). Fixes the previous commit's failing test. --- src/bin/pg_dump/common.c | 92 ++++++++++++++++++++++++++++++++++++++- src/bin/pg_dump/pg_dump.c | 66 ++++++++++++++++++++++++++++ src/bin/pg_dump/pg_dump.h | 4 ++ 3 files changed, 161 insertions(+), 1 deletion(-) diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c index 8ceba04bfe..ef94eef87c 100644 --- a/src/bin/pg_dump/common.c +++ b/src/bin/pg_dump/common.c @@ -64,9 +64,11 @@ static int numNamespaces; static int numExtensions; static int numPublications; -/* This is an array of object identities, not actual DumpableObjects */ +/* These are arrays of object identities, not actual DumpableObjects */ static ExtensionMemberId *extmembers; +static CatalogId *policytables; static int numextmembers; +static int numpolicytables; static void flagInhTables(Archive *fout, TableInfo *tbinfo, int numTables, InhInfo *inhinfo, int numInherits); @@ -74,6 +76,7 @@ static void flagInhIndexes(Archive *fout, TableInfo *tblinfo, int numTables); static void flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables); static DumpableObject **buildIndexArray(void *objArray, int numObjs, Size objSize); +static int CatalogIdCompare(const void *p1, const void *p2); static int DOCatalogIdCompare(const void *p1, const void *p2); static int ExtensionMemberIdCompare(const void *p1, const void *p2); static void findParentsByOid(TableInfo *self, @@ -132,6 +135,14 @@ getSchemaData(Archive *fout, int *numTablesPtr) write_msg(NULL, "identifying extension members\n"); getExtensionMembership(fout, extinfo, numExtensions); + /* + * Similarly, the existence of RLS policies influences whether some tables + * need to be locked. + */ + if (g_verbose) + write_msg(NULL, "checking for row-level security policies\n"); + getTablesWithPolicies(fout); + if (g_verbose) write_msg(NULL, "reading schemas\n"); nspinfo = getNamespaces(fout, &numNamespaces); @@ -777,6 +788,26 @@ buildIndexArray(void *objArray, int numObjs, Size objSize) return ptrs; } +/* + * qsort comparator for pointers to CatalogIds + */ +static int +CatalogIdCompare(const void *p1, const void *p2) +{ + const CatalogId *id1 = (const CatalogId *) p1; + const CatalogId *id2 = (const CatalogId *) p2; + int cmpval; + + /* + * Compare OID first since it's usually unique, whereas there will only be + * a few distinct values of tableoid. + */ + cmpval = oidcmp(id1->oid, id2->oid); + if (cmpval == 0) + cmpval = oidcmp(id1->tableoid, id2->tableoid); + return cmpval; +} + /* * qsort comparator for pointers to DumpableObjects */ @@ -1043,6 +1074,65 @@ ExtensionMemberIdCompare(const void *p1, const void *p2) } +/* + * setPolicyExistence + * Store a list of tables which have RLS policies + */ +void +setPolicyExistence(CatalogId *tables, int ntables) +{ + /* + * Same as setExtensionMembership: sort array in preparation for binary + * searches + */ + if (ntables > 1) + qsort((void *) tables, ntables, sizeof(CatalogId), + CatalogIdCompare); + /* And save */ + policytables = tables; + numpolicytables = ntables; +} + +/* + * hasPolicies + * return whether the specified catalog ID has RLS policies + */ +bool +hasPolicies(CatalogId catalogId) +{ + CatalogId *low; + CatalogId *high; + + /* + * We could use bsearch() here, but the notational cruft of calling + * bsearch is nearly as bad as doing it ourselves; and the generalized + * bsearch function is noticeably slower as well. + */ + if (numpolicytables <= 0) + return NULL; + low = policytables; + high = policytables + (numpolicytables - 1); + while (low <= high) + { + CatalogId *middle; + int difference; + + middle = low + (high - low) / 2; + /* comparison must match CatalogIdCompare, above */ + difference = oidcmp(middle->oid, catalogId.oid); + if (difference == 0) + difference = oidcmp(middle->tableoid, catalogId.tableoid); + if (difference == 0) + return true; + else if (difference < 0) + low = middle + 1; + else + high = middle - 1; + } + return false; +} + + /* * findParentsByOid * find a table's parents in tblinfo[] diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 5bc1858f07..2f60b0c94e 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -3498,6 +3498,60 @@ dumpBlobs(Archive *fout, void *arg) return 1; } +/* + * getTablesWithPolicies + * retrieve the IDs of all tables with pg_policy entries + */ +void +getTablesWithPolicies(Archive *fout) +{ + PQExpBuffer query; + PGresult *res; + int i_classid; + int i_polrelid; + int i, + ntups; + CatalogId *policytables; + + /* No policies before 9.5 */ + if (fout->remoteVersion < 90500) + return; + + query = createPQExpBuffer(); + + /* Figure out which tables have RLS policies. */ + printfPQExpBuffer(query, + "SELECT DISTINCT 'pg_class'::regclass::oid AS classid, " + " polrelid " + "FROM pg_catalog.pg_policy"); + + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); + + ntups = PQntuples(res); + + i_classid = PQfnumber(res, "classid"); + i_polrelid = PQfnumber(res, "polrelid"); + + policytables = pg_malloc(ntups * sizeof(CatalogId)); + + for (i = 0; i < ntups; i++) + { + CatalogId objId; + + objId.tableoid = atooid(PQgetvalue(res, i, i_classid)); + objId.oid = atooid(PQgetvalue(res, i, i_polrelid)); + + policytables[i] = objId; + } + + PQclear(res); + + /* Remember the data for use later */ + setPolicyExistence(policytables, ntups); + + destroyPQExpBuffer(query); +} + /* * getPolicies * get information about all RLS policies on dumpable tables. @@ -6665,6 +6719,18 @@ getTables(Archive *fout, int *numTables) strcmp(PQgetvalue(res, i, i_changed_acl), "f") == 0) tblinfo[i].dobj.dump &= ~DUMP_COMPONENT_ACL; + /* + * If the table has no policies, we don't need to worry about those + * either. + * + * For tables internal to an extension, this may mean we don't need to + * take an ACCESS SHARE lock, which in turn allows less privileged users + * to successfully perform a dump if they don't have SELECT access to + * those tables (which they weren't trying to dump in the first place). + */ + if (!hasPolicies(tblinfo[i].dobj.catId)) + tblinfo[i].dobj.dump &= ~DUMP_COMPONENT_POLICY; + tblinfo[i].interesting = tblinfo[i].dobj.dump ? true : false; tblinfo[i].dummy_view = false; /* might get set during sort */ tblinfo[i].postponed_def = false; /* might get set during sort */ diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index 8522b519cd..ecf7662819 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -678,6 +678,9 @@ extern PublicationInfo *findPublicationByOid(Oid oid); extern void setExtensionMembership(ExtensionMemberId *extmems, int nextmems); extern ExtensionInfo *findOwningExtension(CatalogId catalogId); +extern void setPolicyExistence(CatalogId *tables, int ntables); +extern bool hasPolicies(CatalogId catId); + extern void parseOidArray(const char *str, Oid *array, int arraysize); extern void sortDumpableObjects(DumpableObject **objs, int numObjs, @@ -727,6 +730,7 @@ extern void getExtensionMembership(Archive *fout, ExtensionInfo extinfo[], extern void processExtensionTables(Archive *fout, ExtensionInfo extinfo[], int numExtensions); extern EventTriggerInfo *getEventTriggers(Archive *fout, int *numEventTriggers); +extern void getTablesWithPolicies(Archive *fout); extern void getPolicies(Archive *fout, TableInfo tblinfo[], int numTables); extern PublicationInfo *getPublications(Archive *fout, int *numPublications); -- 2.25.1