[Jacob Champion <jchampion@timescale.com>]

Hi all,

We have a situation where we need to revoke SELECT on a table that
belongs to our extension, and we also need to let less privileged users
dump the extension's external config tables. (The restricted table's
contents are exposed through a security_barrier view, and it's a cloud
environment where "admin" users don't necessarily have true superuser
access.)

Since the restricted table is internal, its contents aren't included in
dumps anyway, so we expected to be able to meet both use cases at once.
Unfortunately:

    $ pg_dump -U unprivileged_user -d postgres
    pg_dump: error: query failed: ERROR:  permission denied for relation
ext_table
    pg_dump: error: query was: LOCK TABLE public.ext_table IN ACCESS
SHARE MODE

...and there appears to be no way to work around this with
--exclude-table, since the table is part of the extension.

It looks like the only reason pg_dump locks this particular table is
because it's been marked with DUMP_COMPONENT_POLICY, which needs a lock
to ensure the consistency of later pg_get_expr() calls. That stings for
two reasons: 1) it doesn't seem like you need SELECT access on a table
to see its policies, and 2) we have no policies on the table anyway;
there are no pg_get_expr() calls to protect.

So I've attached the simplest backportable workaround I could think of:
unmark DUMP_COMPONENT_POLICY for a table that has no policies at the
time of the getTables() query. This is similar to the ACL optimization
that back branches do; it should ensure that there will be no
pg_get_expr() calls on pg_policy for that table later, due to
repeatable-read, and it omits the lock when there's no reason to grab
it. It won't fix the problem for tables that have do policies, but I
don't have any ideas for how to do that safely, unless there's some lock
mode that uses fewer privileges.

I also attached a speculative backport to 11 to illustrate what that
might look like, but first I have to convince you it's a bug. :)

WDYT?

Thanks,
--Jacob

Attachments: download all (2 files, .tar.gz)

0001-pg_dump-skip-lock-for-extension-tables-without-polic.patch (text/x-patch, 2.8K) download
From 86517f376339a65fcf432caa21dbc68d8c2a93a4 Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchampion@timescale.com>
Date: Thu, 16 Mar 2023 11:46:08 -0700
Subject: [PATCH] pg_dump: skip lock for extension tables without policies

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 determine that no policies exist for a table at the time of
getTables().
---
 src/bin/pg_dump/pg_dump.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 2e068c6620..49f01247c1 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6317,6 +6317,7 @@ getTables(Archive *fout, int *numTables)
 	int			i_relacl;
 	int			i_acldefault;
 	int			i_ispartition;
+	int			i_has_policies;
 
 	/*
 	 * Find all the tables and table-like objects.
@@ -6358,6 +6359,13 @@ getTables(Archive *fout, int *numTables)
 						 "d.refobjsubid AS owning_col, "
 						 "tsp.spcname AS reltablespace, ");
 
+	if (fout->remoteVersion >= 90500)
+		appendPQExpBufferStr(query,
+							 "EXISTS (SELECT 1 FROM pg_policy WHERE polrelid = c.oid) AS has_policies, ");
+	else
+		appendPQExpBufferStr(query,
+							 "false AS has_policies, ");
+
 	if (fout->remoteVersion >= 120000)
 		appendPQExpBufferStr(query,
 							 "false AS relhasoids, ");
@@ -6531,6 +6539,7 @@ getTables(Archive *fout, int *numTables)
 	i_relacl = PQfnumber(res, "relacl");
 	i_acldefault = PQfnumber(res, "acldefault");
 	i_ispartition = PQfnumber(res, "ispartition");
+	i_has_policies = PQfnumber(res, "has_policies");
 
 	if (dopt->lockWaitTimeout)
 	{
@@ -6621,6 +6630,17 @@ getTables(Archive *fout, int *numTables)
 		else
 			selectDumpableTable(&tblinfo[i], fout);
 
+		/*
+		 * If the table has no policies, we don't need to worry about those.
+		 *
+		 * 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 (strcmp(PQgetvalue(res, i, i_has_policies), "f") == 0)
+			tblinfo[i].dobj.dump &= ~DUMP_COMPONENT_POLICY;
+
 		/*
 		 * Now, consider the table "interesting" if we need to dump its
 		 * definition or its data.  Later on, we'll skip a lot of data
-- 
2.25.1



0001-pg_dump-skip-lock-for-extension-tables-without-polic.REL_11_STABLE.patch (text/x-patch, 6.2K) download
From ffdb8bac10ecf40c40a3e8c441e299423ee07cef Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchampion@timescale.com>
Date: Thu, 16 Mar 2023 11:46:08 -0700
Subject: [PATCH] pg_dump: skip lock for extension tables without policies

[backport 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 determine that no policies exist for a table at the time of
getTables().
---
 src/bin/pg_dump/pg_dump.c | 41 ++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 56d5571366..125c6038c6 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -5982,6 +5982,7 @@ getTables(Archive *fout, int *numTables)
 	int			i_is_identity_sequence;
 	int			i_changed_acl;
 	int			i_ispartition;
+	int			i_has_policies;
 
 	/*
 	 * Find all the tables and table-like objects.
@@ -6081,7 +6082,8 @@ getTables(Archive *fout, int *numTables)
 						  "OR %s IS NOT NULL"
 						  "))"
 						  "AS changed_acl, "
-						  "%s AS ispartition "
+						  "%s AS ispartition, "
+						  "EXISTS (SELECT 1 FROM pg_policy WHERE polrelid = c.oid) AS has_policies "
 						  "FROM pg_class c "
 						  "LEFT JOIN pg_depend d ON "
 						  "(c.relkind = '%c' AND "
@@ -6152,7 +6154,8 @@ getTables(Archive *fout, int *numTables)
 						  "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS checkoption, "
 						  "tc.reloptions AS toast_reloptions, "
 						  "NULL AS changed_acl, "
-						  "false AS ispartition "
+						  "false AS ispartition, "
+						  "EXISTS (SELECT 1 FROM pg_policy WHERE polrelid = c.oid) AS has_policies "
 						  "FROM pg_class c "
 						  "LEFT JOIN pg_depend d ON "
 						  "(c.relkind = '%c' AND "
@@ -6199,7 +6202,8 @@ getTables(Archive *fout, int *numTables)
 						  "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS checkoption, "
 						  "tc.reloptions AS toast_reloptions, "
 						  "NULL AS changed_acl, "
-						  "false AS ispartition "
+						  "false AS ispartition, "
+						  "false AS has_policies "
 						  "FROM pg_class c "
 						  "LEFT JOIN pg_depend d ON "
 						  "(c.relkind = '%c' AND "
@@ -6246,7 +6250,8 @@ getTables(Archive *fout, int *numTables)
 						  "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS checkoption, "
 						  "tc.reloptions AS toast_reloptions, "
 						  "NULL AS changed_acl, "
-						  "false AS ispartition "
+						  "false AS ispartition, "
+						  "false AS has_policies "
 						  "FROM pg_class c "
 						  "LEFT JOIN pg_depend d ON "
 						  "(c.relkind = '%c' AND "
@@ -6291,7 +6296,8 @@ getTables(Archive *fout, int *numTables)
 						  "c.reloptions AS reloptions, "
 						  "tc.reloptions AS toast_reloptions, "
 						  "NULL AS changed_acl, "
-						  "false AS ispartition "
+						  "false AS ispartition, "
+						  "false AS has_policies "
 						  "FROM pg_class c "
 						  "LEFT JOIN pg_depend d ON "
 						  "(c.relkind = '%c' AND "
@@ -6336,7 +6342,8 @@ getTables(Archive *fout, int *numTables)
 						  "c.reloptions AS reloptions, "
 						  "tc.reloptions AS toast_reloptions, "
 						  "NULL AS changed_acl, "
-						  "false AS ispartition "
+						  "false AS ispartition, "
+						  "false AS has_policies "
 						  "FROM pg_class c "
 						  "LEFT JOIN pg_depend d ON "
 						  "(c.relkind = '%c' AND "
@@ -6380,7 +6387,8 @@ getTables(Archive *fout, int *numTables)
 						  "c.reloptions AS reloptions, "
 						  "tc.reloptions AS toast_reloptions, "
 						  "NULL AS changed_acl, "
-						  "false AS ispartition "
+						  "false AS ispartition, "
+						  "false AS has_policies "
 						  "FROM pg_class c "
 						  "LEFT JOIN pg_depend d ON "
 						  "(c.relkind = '%c' AND "
@@ -6424,7 +6432,8 @@ getTables(Archive *fout, int *numTables)
 						  "c.reloptions AS reloptions, "
 						  "NULL AS toast_reloptions, "
 						  "NULL AS changed_acl, "
-						  "false AS ispartition "
+						  "false AS ispartition, "
+						  "false AS has_policies "
 						  "FROM pg_class c "
 						  "LEFT JOIN pg_depend d ON "
 						  "(c.relkind = '%c' AND "
@@ -6467,7 +6476,8 @@ getTables(Archive *fout, int *numTables)
 						  "NULL AS reloptions, "
 						  "NULL AS toast_reloptions, "
 						  "NULL AS changed_acl, "
-						  "false AS ispartition "
+						  "false AS ispartition, "
+						  "false AS has_policies "
 						  "FROM pg_class c "
 						  "LEFT JOIN pg_depend d ON "
 						  "(c.relkind = '%c' AND "
@@ -6535,6 +6545,7 @@ getTables(Archive *fout, int *numTables)
 	i_is_identity_sequence = PQfnumber(res, "is_identity_sequence");
 	i_changed_acl = PQfnumber(res, "changed_acl");
 	i_ispartition = PQfnumber(res, "ispartition");
+	i_has_policies = PQfnumber(res, "has_policies");
 
 	if (dopt->lockWaitTimeout)
 	{
@@ -6627,6 +6638,18 @@ getTables(Archive *fout, int *numTables)
 			strcmp(PQgetvalue(res, i, i_changed_acl), "f") == 0)
 			tblinfo[i].dobj.dump &= ~DUMP_COMPONENT_ACL;
 
+		/*
+		 * Similarly, 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 (strcmp(PQgetvalue(res, i, i_has_policies), "f") == 0)
+			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 */
-- 
2.25.1