From: Tom Lane <tgl@sss.pgh.pa.us>
To: depesz@depesz.com
Cc: Stephen Frost <sfrost@snowman.net>
Cc: Laurenz Albe <laurenz.albe@cybertec.at>
Cc: pgsql-general <pgsql-general@lists.postgresql.org>
Subject: Re: Can we get rid of repeated queries from pg_dump?
Date: Sat, 28 Aug 2021 18:26:21 -0400
Message-ID: <1082810.1630189581@sss.pgh.pa.us> (raw)
In-Reply-To: <1004390.1630160930@sss.pgh.pa.us>
References: <20210826084430.GA26282@depesz.com>
<648517.1629986527@sss.pgh.pa.us>
<20210826140827.GA15868@depesz.com>
<649446.1629987629@sss.pgh.pa.us>
<20210826160644.GA3120@depesz.com>
<e6a7503afd115d111fbb96c5787906d73cfe2605.camel@cybertec.at>
<896732.1630099403@sss.pgh.pa.us>
<20210828064650.GB11579@depesz.com>
<1004390.1630160930@sss.pgh.pa.us>
Here is a second patch, quite independent of the first one, that
gets rid of some other repetitive queries. On the regression database,
the number of queries needed to do "pg_dump -s regression" drops from
3260 to 2589, and on my machine it takes 1.8 sec instead of 2.1 sec.
What's attacked here is a fairly silly decision in getPolicies()
to query pg_policy once per table, when we could do so just once.
It might have been okay if we skipped the per-table query for
tables that lack policies, but it's not clear to me that we can
know that without looking into pg_policy. In any case I doubt
this is ever going to be less efficient than the original coding.
regards, tom lane
Attachments:query-pg_policy-just-once.patch (text/x-diff, 5.4K) downloaddiff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.cindex 6adbd20778..befe68de1a 100644--- a/src/bin/pg_dump/pg_dump.c+++ b/src/bin/pg_dump/pg_dump.c@@ -3656,7 +3656,7 @@ dumpBlobs(Archive *fout, const void *arg)
/*
* getPolicies
- * get information about policies on a dumpable table.+ * get information about all RLS policies on dumpable tables.
*/
void
getPolicies(Archive *fout, TableInfo tblinfo[], int numTables)
@@ -3666,6 +3666,7 @@ getPolicies(Archive *fout, TableInfo tblinfo[], int numTables)
PolicyInfo *polinfo;
int i_oid;
int i_tableoid;
+ int i_polrelid;
int i_polname;
int i_polcmd;
int i_polpermissive;
@@ -3681,6 +3682,10 @@ getPolicies(Archive *fout, TableInfo tblinfo[], int numTables)
query = createPQExpBuffer();
+ /*+ * First, check which tables have RLS enabled. We represent RLS being+ * enabled on a table by creating a PolicyInfo object with null polname.+ */
for (i = 0; i < numTables; i++)
{
TableInfo *tbinfo = &tblinfo[i];
@@ -3689,15 +3694,6 @@ getPolicies(Archive *fout, TableInfo tblinfo[], int numTables)
if (!(tbinfo->dobj.dump & DUMP_COMPONENT_POLICY))
continue;
- pg_log_info("reading row security enabled for table \"%s.%s\"",- tbinfo->dobj.namespace->dobj.name,- tbinfo->dobj.name);-- /*- * Get row security enabled information for the table. We represent- * RLS being enabled on a table by creating a PolicyInfo object with- * null polname.- */
if (tbinfo->rowsec)
{
/*
@@ -3719,51 +3715,35 @@ getPolicies(Archive *fout, TableInfo tblinfo[], int numTables)
polinfo->polqual = NULL;
polinfo->polwithcheck = NULL;
}
+ }- pg_log_info("reading policies for table \"%s.%s\"",- tbinfo->dobj.namespace->dobj.name,- tbinfo->dobj.name);-- resetPQExpBuffer(query);-- /* Get the policies for the table. */- if (fout->remoteVersion >= 100000)- appendPQExpBuffer(query,- "SELECT oid, tableoid, pol.polname, pol.polcmd, pol.polpermissive, "- "CASE WHEN pol.polroles = '{0}' THEN NULL ELSE "- " pg_catalog.array_to_string(ARRAY(SELECT pg_catalog.quote_ident(rolname) from pg_catalog.pg_roles WHERE oid = ANY(pol.polroles)), ', ') END AS polroles, "- "pg_catalog.pg_get_expr(pol.polqual, pol.polrelid) AS polqual, "- "pg_catalog.pg_get_expr(pol.polwithcheck, pol.polrelid) AS polwithcheck "- "FROM pg_catalog.pg_policy pol "- "WHERE polrelid = '%u'",- tbinfo->dobj.catId.oid);- else- appendPQExpBuffer(query,- "SELECT oid, tableoid, pol.polname, pol.polcmd, 't' as polpermissive, "- "CASE WHEN pol.polroles = '{0}' THEN NULL ELSE "- " pg_catalog.array_to_string(ARRAY(SELECT pg_catalog.quote_ident(rolname) from pg_catalog.pg_roles WHERE oid = ANY(pol.polroles)), ', ') END AS polroles, "- "pg_catalog.pg_get_expr(pol.polqual, pol.polrelid) AS polqual, "- "pg_catalog.pg_get_expr(pol.polwithcheck, pol.polrelid) AS polwithcheck "- "FROM pg_catalog.pg_policy pol "- "WHERE polrelid = '%u'",- tbinfo->dobj.catId.oid);- res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);+ /*+ * Now, read all RLS policies, and create PolicyInfo objects for all those+ * that are of interest.+ */+ pg_log_info("reading row-level security policies");- ntups = PQntuples(res);+ printfPQExpBuffer(query,+ "SELECT oid, tableoid, pol.polrelid, pol.polname, pol.polcmd, ");+ if (fout->remoteVersion >= 100000)+ appendPQExpBuffer(query, "pol.polpermissive, ");+ else+ appendPQExpBuffer(query, "'t' as polpermissive, ");+ appendPQExpBuffer(query,+ "CASE WHEN pol.polroles = '{0}' THEN NULL ELSE "+ " pg_catalog.array_to_string(ARRAY(SELECT pg_catalog.quote_ident(rolname) from pg_catalog.pg_roles WHERE oid = ANY(pol.polroles)), ', ') END AS polroles, "+ "pg_catalog.pg_get_expr(pol.polqual, pol.polrelid) AS polqual, "+ "pg_catalog.pg_get_expr(pol.polwithcheck, pol.polrelid) AS polwithcheck "+ "FROM pg_catalog.pg_policy pol");- if (ntups == 0)- {- /*- * No explicit policies to handle (only the default-deny policy,- * which is handled as part of the table definition). Clean up- * and return.- */- PQclear(res);- continue;- }+ res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);+ ntups = PQntuples(res);+ if (ntups > 0)+ {
i_oid = PQfnumber(res, "oid");
i_tableoid = PQfnumber(res, "tableoid");
+ i_polrelid = PQfnumber(res, "polrelid");
i_polname = PQfnumber(res, "polname");
i_polcmd = PQfnumber(res, "polcmd");
i_polpermissive = PQfnumber(res, "polpermissive");
@@ -3775,6 +3755,16 @@ getPolicies(Archive *fout, TableInfo tblinfo[], int numTables)
for (j = 0; j < ntups; j++)
{
+ Oid polrelid = atooid(PQgetvalue(res, j, i_polrelid));+ TableInfo *tbinfo = findTableByOid(polrelid);++ /*+ * Ignore row security on tables not to be dumped. (This will+ * result in some harmless wasted slots in polinfo[].)+ */+ if (!(tbinfo->dobj.dump & DUMP_COMPONENT_POLICY))+ continue;+
polinfo[j].dobj.objType = DO_POLICY;
polinfo[j].dobj.catId.tableoid =
atooid(PQgetvalue(res, j, i_tableoid));
@@ -3804,8 +3794,10 @@ getPolicies(Archive *fout, TableInfo tblinfo[], int numTables)
polinfo[j].polwithcheck
= pg_strdup(PQgetvalue(res, j, i_polwithcheck));
}
- PQclear(res);
}
++ PQclear(res);+
destroyPQExpBuffer(query);
}
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-general@postgresql.org
Cc: tgl@sss.pgh.pa.us, depesz@depesz.com, sfrost@snowman.net, laurenz.albe@cybertec.at, pgsql-general@lists.postgresql.org
Subject: Re: Can we get rid of repeated queries from pg_dump?
In-Reply-To: <1082810.1630189581@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox