MAINTAIN privilege -- what do we need to un-revert it?
Started by Jeff Davisalmost 2 years ago30 messages
The MAINTAIN privilege was reverted during the 16 cycle because of the
potential for someone to play tricks with search_path.
For instance, if user foo does:
CREATE FUNCTION mod7(INT) RETURNS INT IMMUTABLE
LANGUAGE plpgsql AS $$ BEGIN RETURN mod($1, 7); END; $$;
CREATE TABLE x(i INT);
CREATE UNIQUE INDEX x_mod7_idx ON x (mod7(i));
GRANT MAINTAIN ON x TO bar;
Then user bar can create their own function named "bar.mod(int, int)",
and "SET search_path = bar, pg_catalog", and then issue a "REINDEX x"
and cause problems.
There are several factors required for that to be a problem:
1. foo hasn't used a "SET search_path" clause on their function
2. bar must have the privileges to create a function somewhere
3. bar must have privileges on table x
There's an argument that we should blame factor #1. Robert stated[1]/messages/by-id/CA+TgmoYEP40iBW-A9nPfDp8AhGoekPp3aPDFzTgBUrqmfCwZzQ@mail.gmail.com
that users should use SET search_path clauses on their functions, even
SECURITY INVOKER functions. And I've added a search_path cache which
improves the performance enough to make that more reasonable to do
generally.
There's also an argument that #2 is to blame. Given the realities of
our system, best practice is that users shouldn't have the privileges
to create objects, even in their own schema, unless required. (Joe made
this suggestion in an offline discussion.)
There's also an arugment that #3 is not specific to the MAINTAIN
privilege. Clearly similar risks exist for other privileges, like
TRIGGER. And even the INSERT privilege, in the above example, would
allow bar to violate the unique constraint and corrupt the index[2]/messages/by-id/fff566293c9165c69bb4c555da1ac02c63660664.camel@j-davis.com.
If those arguments are still unconvincing, then the next idea is to fix
the search_path for all maintenance commands[3]/messages/by-id/e44327179e5c9015c8dda67351c04da552066017.camel@j-davis.com. I tried this during
the 16 cycle, but due to timing issues it was also reverted. I can
proceed with this approach again, but I'd like a clear endorsement, in
case there were other reasons to doubt the approach.
Regards,
Jeff Davis
[1]: /messages/by-id/CA+TgmoYEP40iBW-A9nPfDp8AhGoekPp3aPDFzTgBUrqmfCwZzQ@mail.gmail.com
/messages/by-id/CA+TgmoYEP40iBW-A9nPfDp8AhGoekPp3aPDFzTgBUrqmfCwZzQ@mail.gmail.com
[2]: /messages/by-id/fff566293c9165c69bb4c555da1ac02c63660664.camel@j-davis.com
/messages/by-id/fff566293c9165c69bb4c555da1ac02c63660664.camel@j-davis.com
[3]: /messages/by-id/e44327179e5c9015c8dda67351c04da552066017.camel@j-davis.com
/messages/by-id/e44327179e5c9015c8dda67351c04da552066017.camel@j-davis.com
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Wed, Feb 14, 2024 at 10:20:28AM -0800, Jeff Davis wrote:
If those arguments are still unconvincing, then the next idea is to fix
the search_path for all maintenance commands[3]. I tried this during
the 16 cycle, but due to timing issues it was also reverted. I can
proceed with this approach again, but I'd like a clear endorsement, in
case there were other reasons to doubt the approach.
This seemed like the approach folks were most in favor of at the developer
meeting a couple weeks ago [0]https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2024_Developer_Meeting#The_Path_to_un-reverting_the_MAINTAIN_privilege. At least, that was my interpretation of
the discussion.
BTW I have been testing reverting commit 151c22d (i.e., un-reverting
MAINTAIN) every month or two, and last I checked, it still applies pretty
cleanly. The only changes I've needed to make are to the catversion and to
a hard-coded version in a test (16 -> 17).
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Wed, Feb 14, 2024 at 01:02:26PM -0600, Nathan Bossart wrote:
BTW I have been testing reverting commit 151c22d (i.e., un-reverting
MAINTAIN) every month or two, and last I checked, it still applies pretty
cleanly. The only changes I've needed to make are to the catversion and to
a hard-coded version in a test (16 -> 17).
Posting to get some cfbot coverage.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Revert-Revert-MAINTAIN-privilege-and-pg_maintain-.patchtext/x-diff; charset=us-asciiDownload
From ce4f0c8cc87c2534d46060dc0725783ad6275f21 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 15 Feb 2024 10:02:41 -0600
Subject: [PATCH v1 1/1] Revert "Revert MAINTAIN privilege and pg_maintain
predefined role."
XXX: NEEDS CATVERSION BUMP
This reverts commit 151c22deee66a3390ca9a1c3675e29de54ae73fc.
---
doc/src/sgml/ddl.sgml | 35 ++++--
doc/src/sgml/func.sgml | 2 +-
.../sgml/ref/alter_default_privileges.sgml | 4 +-
doc/src/sgml/ref/analyze.sgml | 6 +-
doc/src/sgml/ref/cluster.sgml | 10 +-
doc/src/sgml/ref/grant.sgml | 3 +-
doc/src/sgml/ref/lock.sgml | 4 +-
.../sgml/ref/refresh_materialized_view.sgml | 5 +-
doc/src/sgml/ref/reindex.sgml | 23 ++--
doc/src/sgml/ref/revoke.sgml | 2 +-
doc/src/sgml/ref/vacuum.sgml | 6 +-
doc/src/sgml/user-manag.sgml | 12 ++
src/backend/catalog/aclchk.c | 15 +++
src/backend/commands/analyze.c | 13 +-
src/backend/commands/cluster.c | 43 +++++--
src/backend/commands/indexcmds.c | 34 ++---
src/backend/commands/lockcmds.c | 2 +-
src/backend/commands/matview.c | 3 +-
src/backend/commands/tablecmds.c | 16 +--
src/backend/commands/vacuum.c | 65 +++++-----
src/backend/utils/adt/acl.c | 8 ++
src/bin/pg_dump/dumputils.c | 1 +
src/bin/pg_dump/t/002_pg_dump.pl | 2 +-
src/bin/psql/tab-complete.c | 6 +-
src/include/catalog/pg_authid.dat | 5 +
src/include/commands/tablecmds.h | 5 +-
src/include/commands/vacuum.h | 5 +-
src/include/nodes/parsenodes.h | 3 +-
src/include/utils/acl.h | 5 +-
.../expected/cluster-conflict-partition.out | 8 +-
.../specs/cluster-conflict-partition.spec | 2 +-
.../perl/PostgreSQL/Test/AdjustUpgrade.pm | 11 ++
src/test/regress/expected/cluster.out | 7 ++
src/test/regress/expected/create_index.out | 4 +-
src/test/regress/expected/dependency.out | 22 ++--
src/test/regress/expected/privileges.out | 116 ++++++++++++++----
src/test/regress/expected/rowsecurity.out | 34 ++---
src/test/regress/sql/cluster.sql | 5 +
src/test/regress/sql/dependency.sql | 2 +-
src/test/regress/sql/privileges.sql | 68 ++++++++++
40 files changed, 444 insertions(+), 178 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 9d7e2c756b..8616a8e9cc 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -1868,8 +1868,8 @@ ALTER TABLE products RENAME TO items;
<literal>INSERT</literal>, <literal>UPDATE</literal>, <literal>DELETE</literal>,
<literal>TRUNCATE</literal>, <literal>REFERENCES</literal>, <literal>TRIGGER</literal>,
<literal>CREATE</literal>, <literal>CONNECT</literal>, <literal>TEMPORARY</literal>,
- <literal>EXECUTE</literal>, <literal>USAGE</literal>, <literal>SET</literal>
- and <literal>ALTER SYSTEM</literal>.
+ <literal>EXECUTE</literal>, <literal>USAGE</literal>, <literal>SET</literal>,
+ <literal>ALTER SYSTEM</literal>, and <literal>MAINTAIN</literal>.
The privileges applicable to a particular
object vary depending on the object's type (table, function, etc.).
More detail about the meanings of these privileges appears below.
@@ -2160,7 +2160,19 @@ REVOKE ALL ON accounts FROM PUBLIC;
</para>
</listitem>
</varlistentry>
- </variablelist>
+
+ <varlistentry id="ddl-priv-maintain">
+ <term><literal>MAINTAIN</literal></term>
+ <listitem>
+ <para>
+ Allows <command>VACUUM</command>, <command>ANALYZE</command>,
+ <command>CLUSTER</command>, <command>REFRESH MATERIALIZED VIEW</command>,
+ <command>REINDEX</command>, and <command>LOCK TABLE</command> on a
+ relation.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
The privileges required by other commands are listed on the
reference page of the respective command.
@@ -2309,6 +2321,11 @@ REVOKE ALL ON accounts FROM PUBLIC;
<entry><literal>A</literal></entry>
<entry><literal>PARAMETER</literal></entry>
</row>
+ <row>
+ <entry><literal>MAINTAIN</literal></entry>
+ <entry><literal>m</literal></entry>
+ <entry><literal>TABLE</literal></entry>
+ </row>
</tbody>
</tgroup>
</table>
@@ -2399,7 +2416,7 @@ REVOKE ALL ON accounts FROM PUBLIC;
</row>
<row>
<entry><literal>TABLE</literal> (and table-like objects)</entry>
- <entry><literal>arwdDxt</literal></entry>
+ <entry><literal>arwdDxtm</literal></entry>
<entry>none</entry>
<entry><literal>\dp</literal></entry>
</row>
@@ -2465,11 +2482,11 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO miriam_rw;
<programlisting>
=> \dp mytable
Access privileges
- Schema | Name | Type | Access privileges | Column privileges | Policies
---------+---------+-------+-----------------------+-----------------------+----------
- public | mytable | table | miriam=arwdDxt/miriam+| col1: +|
- | | | =r/miriam +| miriam_rw=rw/miriam |
- | | | admin=arw/miriam | |
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------+---------+-------+------------------------+-----------------------+----------
+ public | mytable | table | miriam=arwdDxtm/miriam+| col1: +|
+ | | | =r/miriam +| miriam_rw=rw/miriam |
+ | | | admin=arw/miriam | |
(1 row)
</programlisting>
</para>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index cf3de80394..f1a64a113e 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24233,7 +24233,7 @@ SELECT has_function_privilege('joeuser', 'myfunc(int, text)', 'execute');
are <literal>SELECT</literal>, <literal>INSERT</literal>,
<literal>UPDATE</literal>, <literal>DELETE</literal>,
<literal>TRUNCATE</literal>, <literal>REFERENCES</literal>,
- and <literal>TRIGGER</literal>.
+ <literal>TRIGGER</literal>, and <literal>MAINTAIN</literal>.
</para></entry>
</row>
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 78744470c8..1de4c5c1b4 100644
--- a/doc/src/sgml/ref/alter_default_privileges.sgml
+++ b/doc/src/sgml/ref/alter_default_privileges.sgml
@@ -28,7 +28,7 @@ ALTER DEFAULT PRIVILEGES
<phrase>where <replaceable class="parameter">abbreviated_grant_or_revoke</replaceable> is one of:</phrase>
-GRANT { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
+GRANT { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | MAINTAIN }
[, ...] | ALL [ PRIVILEGES ] }
ON TABLES
TO { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
@@ -51,7 +51,7 @@ GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
TO { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
REVOKE [ GRANT OPTION FOR ]
- { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
+ { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | MAINTAIN }
[, ...] | ALL [ PRIVILEGES ] }
ON TABLES
FROM { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...]
diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index 6ed8e7241b..0eaf59cb49 100644
--- a/doc/src/sgml/ref/analyze.sgml
+++ b/doc/src/sgml/ref/analyze.sgml
@@ -174,11 +174,9 @@ ANALYZE [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] [ <r
<title>Notes</title>
<para>
- To analyze a table, one must ordinarily be the table's owner or a
- superuser. However, database owners are allowed to
+ To analyze a table, one must ordinarily have the <literal>MAINTAIN</literal>
+ privilege on the table. However, database owners are allowed to
analyze all tables in their databases, except shared catalogs.
- (The restriction for shared catalogs means that a true database-wide
- <command>ANALYZE</command> can only be performed by a superuser.)
<command>ANALYZE</command> will skip over any tables that the calling user
does not have permission to analyze.
</para>
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 557a94cea7..b9aa7c8428 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -68,9 +68,8 @@ CLUSTER [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] [ <r
<command>CLUSTER</command> without a
<replaceable class="parameter">table_name</replaceable> reclusters all the
previously-clustered tables in the current database that the calling user
- owns, or all such tables if called by a superuser. This
- form of <command>CLUSTER</command> cannot be executed inside a transaction
- block.
+ has privileges for. This form of <command>CLUSTER</command> cannot be
+ executed inside a transaction block.
</para>
<para>
@@ -131,6 +130,11 @@ CLUSTER [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] [ <r
<refsect1>
<title>Notes</title>
+ <para>
+ To cluster a table, one must have the <literal>MAINTAIN</literal> privilege
+ on the table.
+ </para>
+
<para>
In cases where you are accessing single rows randomly
within a table, the actual order of the data in the
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index 9d27b7fcde..65b1fe7711 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-GRANT { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
+GRANT { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | MAINTAIN }
[, ...] | ALL [ PRIVILEGES ] }
ON { [ TABLE ] <replaceable class="parameter">table_name</replaceable> [, ...]
| ALL TABLES IN SCHEMA <replaceable class="parameter">schema_name</replaceable> [, ...] }
@@ -193,6 +193,7 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
<term><literal>USAGE</literal></term>
<term><literal>SET</literal></term>
<term><literal>ALTER SYSTEM</literal></term>
+ <term><literal>MAINTAIN</literal></term>
<listitem>
<para>
Specific types of privileges, as defined in <xref linkend="ddl-priv"/>.
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index 6ce2518de7..070855da18 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -166,8 +166,8 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ]
<para>
To lock a table, the user must have the right privilege for the specified
- <replaceable class="parameter">lockmode</replaceable>, or be the table's
- owner or a superuser. If the user has
+ <replaceable class="parameter">lockmode</replaceable>.
+ If the user has <literal>MAINTAIN</literal>,
<literal>UPDATE</literal>, <literal>DELETE</literal>, or
<literal>TRUNCATE</literal> privileges on the table, any <replaceable
class="parameter">lockmode</replaceable> is permitted. If the user has
diff --git a/doc/src/sgml/ref/refresh_materialized_view.sgml b/doc/src/sgml/ref/refresh_materialized_view.sgml
index 7a019162c3..4214055692 100644
--- a/doc/src/sgml/ref/refresh_materialized_view.sgml
+++ b/doc/src/sgml/ref/refresh_materialized_view.sgml
@@ -31,8 +31,9 @@ REFRESH MATERIALIZED VIEW [ CONCURRENTLY ] <replaceable class="parameter">name</
<para>
<command>REFRESH MATERIALIZED VIEW</command> completely replaces the
- contents of a materialized view. To execute this command you must be the
- owner of the materialized view. The old contents are discarded. If
+ contents of a materialized view. To execute this command you must have the
+ <literal>MAINTAIN</literal>
+ privilege on the materialized view. The old contents are discarded. If
<literal>WITH DATA</literal> is specified (or defaults) the backing query
is executed to provide the new data, and the materialized view is left in a
scannable state. If <literal>WITH NO DATA</literal> is specified no new
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 21e2e91d89..bef3486843 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -292,16 +292,21 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DA
</para>
<para>
- Reindexing a single index or table requires being the owner of that
- index or table. Reindexing a schema or database requires being the
- owner of that schema or database. Note specifically that it's thus
+ Reindexing a single index or table requires
+ having the <literal>MAINTAIN</literal> privilege on the
+ table. Note that while <command>REINDEX</command> on a partitioned index or
+ table requires having the <literal>MAINTAIN</literal> privilege on the
+ partitioned table, such commands skip the privilege checks when processing
+ the individual partitions. Reindexing a schema or database requires being the
+ owner of that schema or database or having privileges of the
+ <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link>
+ role. Note specifically that it's thus
possible for non-superusers to rebuild indexes of tables owned by
- other users. However, as a special exception, when
- <command>REINDEX DATABASE</command>, <command>REINDEX SCHEMA</command>
- or <command>REINDEX SYSTEM</command> is issued by a non-superuser,
- indexes on shared catalogs will be skipped unless the user owns the
- catalog (which typically won't be the case). Of course, superusers
- can always reindex anything.
+ other users. However, as a special exception,
+ <command>REINDEX DATABASE</command>, <command>REINDEX SCHEMA</command>,
+ and <command>REINDEX SYSTEM</command> will skip indexes on shared catalogs
+ unless the user has the <literal>MAINTAIN</literal> privilege on the
+ catalog.
</para>
<para>
diff --git a/doc/src/sgml/ref/revoke.sgml b/doc/src/sgml/ref/revoke.sgml
index 2db66bbf37..8df492281a 100644
--- a/doc/src/sgml/ref/revoke.sgml
+++ b/doc/src/sgml/ref/revoke.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
REVOKE [ GRANT OPTION FOR ]
- { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
+ { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | MAINTAIN }
[, ...] | ALL [ PRIVILEGES ] }
ON { [ TABLE ] <replaceable class="parameter">table_name</replaceable> [, ...]
| ALL TABLES IN SCHEMA <replaceable>schema_name</replaceable> [, ...] }
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index a87c2e320e..88c191a262 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -434,11 +434,9 @@ VACUUM [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] [ <re
<title>Notes</title>
<para>
- To vacuum a table, one must ordinarily be the table's owner or a
- superuser. However, database owners are allowed to
+ To vacuum a table, one must ordinarily have the <literal>MAINTAIN</literal>
+ privilege on the table. However, database owners are allowed to
vacuum all tables in their databases, except shared catalogs.
- (The restriction for shared catalogs means that a true database-wide
- <command>VACUUM</command> can only be performed by a superuser.)
<command>VACUUM</command> will skip over any tables that the calling user
does not have permission to vacuum.
</para>
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index e026a4e271..07a16247d7 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -682,6 +682,18 @@ DROP ROLE doomed_role;
the <link linkend="sql-checkpoint"><command>CHECKPOINT</command></link>
command.</entry>
</row>
+ <row>
+ <entry>pg_maintain</entry>
+ <entry>Allow executing
+ <link linkend="sql-vacuum"><command>VACUUM</command></link>,
+ <link linkend="sql-analyze"><command>ANALYZE</command></link>,
+ <link linkend="sql-cluster"><command>CLUSTER</command></link>,
+ <link linkend="sql-refreshmaterializedview"><command>REFRESH MATERIALIZED VIEW</command></link>,
+ <link linkend="sql-reindex"><command>REINDEX</command></link>,
+ and <link linkend="sql-lock"><command>LOCK TABLE</command></link> on all
+ relations, as if having <literal>MAINTAIN</literal> rights on those
+ objects, even without having it explicitly.</entry>
+ </row>
<row>
<entry>pg_use_reserved_connections</entry>
<entry>Allow use of connection slots reserved via
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 1e44a71f61..8820a88912 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -2644,6 +2644,8 @@ string_to_privilege(const char *privname)
return ACL_SET;
if (strcmp(privname, "alter system") == 0)
return ACL_ALTER_SYSTEM;
+ if (strcmp(privname, "maintain") == 0)
+ return ACL_MAINTAIN;
if (strcmp(privname, "rule") == 0)
return 0; /* ignore old RULE privileges */
ereport(ERROR,
@@ -2685,6 +2687,8 @@ privilege_to_string(AclMode privilege)
return "SET";
case ACL_ALTER_SYSTEM:
return "ALTER SYSTEM";
+ case ACL_MAINTAIN:
+ return "MAINTAIN";
default:
elog(ERROR, "unrecognized privilege: %d", (int) privilege);
}
@@ -3443,6 +3447,17 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
has_privs_of_role(roleid, ROLE_PG_WRITE_ALL_DATA))
result |= (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE));
+ /*
+ * Check if ACL_MAINTAIN is being checked and, if so, and not already set
+ * as part of the result, then check if the user is a member of the
+ * pg_maintain role, which allows VACUUM, ANALYZE, CLUSTER, REFRESH
+ * MATERIALIZED VIEW, and REINDEX on all relations.
+ */
+ if (mask & ACL_MAINTAIN &&
+ !(result & ACL_MAINTAIN) &&
+ has_privs_of_role(roleid, ROLE_PG_MAINTAIN))
+ result |= ACL_MAINTAIN;
+
return result;
}
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index a03495d6c9..acc774e41a 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -159,16 +159,15 @@ analyze_rel(Oid relid, RangeVar *relation,
return;
/*
- * Check if relation needs to be skipped based on ownership. This check
+ * Check if relation needs to be skipped based on privileges. This check
* happens also when building the relation list to analyze for a manual
* operation, and needs to be done additionally here as ANALYZE could
- * happen across multiple transactions where relation ownership could have
- * changed in-between. Make sure to generate only logs for ANALYZE in
- * this case.
+ * happen across multiple transactions where privileges could have changed
+ * in-between. Make sure to generate only logs for ANALYZE in this case.
*/
- if (!vacuum_is_relation_owner(RelationGetRelid(onerel),
- onerel->rd_rel,
- params->options & VACOPT_ANALYZE))
+ if (!vacuum_is_permitted_for_relation(RelationGetRelid(onerel),
+ onerel->rd_rel,
+ params->options & ~VACOPT_VACUUM))
{
relation_close(onerel, ShareUpdateExclusiveLock);
return;
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index e2c48ec560..44b4dbdb6b 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -80,6 +80,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
+static bool cluster_is_permitted_for_relation(Oid relid, Oid userid);
/*---------------------------------------------------------------------------
@@ -147,7 +148,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
tableOid = RangeVarGetRelidExtended(stmt->relation,
AccessExclusiveLock,
0,
- RangeVarCallbackOwnsTable, NULL);
+ RangeVarCallbackMaintainsTable,
+ NULL);
rel = table_open(tableOid, NoLock);
/*
@@ -364,8 +366,8 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
*/
if (recheck)
{
- /* Check that the user still owns the relation */
- if (!object_ownercheck(RelationRelationId, tableOid, save_userid))
+ /* Check that the user still has privileges for the relation */
+ if (!cluster_is_permitted_for_relation(tableOid, save_userid))
{
relation_close(OldHeap, AccessExclusiveLock);
goto out;
@@ -1621,7 +1623,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
/*
- * Get a list of tables that the current user owns and
+ * Get a list of tables that the current user has privileges on and
* have indisclustered set. Return the list in a List * of RelToCluster
* (stored in the specified memory context), each one giving the tableOid
* and the indexOid on which the table is already clustered.
@@ -1638,8 +1640,8 @@ get_tables_to_cluster(MemoryContext cluster_context)
List *rtcs = NIL;
/*
- * Get all indexes that have indisclustered set and are owned by
- * appropriate user.
+ * Get all indexes that have indisclustered set and that the current user
+ * has the appropriate privileges for.
*/
indRelation = table_open(IndexRelationId, AccessShareLock);
ScanKeyInit(&entry,
@@ -1653,7 +1655,7 @@ get_tables_to_cluster(MemoryContext cluster_context)
index = (Form_pg_index) GETSTRUCT(indexTuple);
- if (!object_ownercheck(RelationRelationId, index->indrelid, GetUserId()))
+ if (!cluster_is_permitted_for_relation(index->indrelid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1701,10 +1703,13 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
if (get_rel_relkind(indexrelid) != RELKIND_INDEX)
continue;
- /* Silently skip partitions which the user has no access to. */
- if (!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
- (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) ||
- IsSharedRelation(relid)))
+ /*
+ * It's possible that the user does not have privileges to CLUSTER the
+ * leaf partition despite having such privileges on the partitioned
+ * table. We skip any partitions which the user is not permitted to
+ * CLUSTER.
+ */
+ if (!cluster_is_permitted_for_relation(relid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1720,3 +1725,19 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
return rtcs;
}
+
+/*
+ * Return whether userid has privileges to CLUSTER relid. If not, this
+ * function emits a WARNING.
+ */
+static bool
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+ if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK)
+ return true;
+
+ ereport(WARNING,
+ (errmsg("permission denied to cluster \"%s\", skipping it",
+ get_rel_name(relid))));
+ return false;
+}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 7a87626f5f..d1052881d9 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -27,6 +27,7 @@
#include "catalog/index.h"
#include "catalog/indexing.h"
#include "catalog/pg_am.h"
+#include "catalog/pg_authid.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_database.h"
#include "catalog/pg_inherits.h"
@@ -2944,6 +2945,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
char relkind;
struct ReindexIndexCallbackState *state = arg;
LOCKMODE table_lockmode;
+ Oid table_oid;
/*
* Lock level here should match table lock in reindex_index() for
@@ -2983,14 +2985,19 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
errmsg("\"%s\" is not an index", relation->relname)));
/* Check permissions */
- if (!object_ownercheck(RelationRelationId, relId, GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, relation->relname);
+ table_oid = IndexGetRelation(relId, true);
+ if (OidIsValid(table_oid))
+ {
+ AclResult aclresult;
+
+ aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
+ }
/* Lock heap before index to avoid deadlock. */
if (relId != oldRelId)
{
- Oid table_oid = IndexGetRelation(relId, true);
-
/*
* If the OID isn't valid, it means the index was concurrently
* dropped, which is not a problem for us; just return normally.
@@ -3026,7 +3033,7 @@ ReindexTable(const ReindexStmt *stmt, const ReindexParams *params, bool isTopLev
(params->options & REINDEXOPT_CONCURRENTLY) != 0 ?
ShareUpdateExclusiveLock : ShareLock,
0,
- RangeVarCallbackOwnsTable, NULL);
+ RangeVarCallbackMaintainsTable, NULL);
if (get_rel_relkind(heapOid) == RELKIND_PARTITIONED_TABLE)
ReindexPartitions(stmt, heapOid, params, isTopLevel);
@@ -3110,7 +3117,8 @@ ReindexMultipleTables(const ReindexStmt *stmt, const ReindexParams *params)
{
objectOid = get_namespace_oid(objectName, false);
- if (!object_ownercheck(NamespaceRelationId, objectOid, GetUserId()))
+ if (!object_ownercheck(NamespaceRelationId, objectOid, GetUserId()) &&
+ !has_privs_of_role(GetUserId(), ROLE_PG_MAINTAIN))
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SCHEMA,
objectName);
}
@@ -3122,7 +3130,8 @@ ReindexMultipleTables(const ReindexStmt *stmt, const ReindexParams *params)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("can only reindex the currently open database")));
- if (!object_ownercheck(DatabaseRelationId, objectOid, GetUserId()))
+ if (!object_ownercheck(DatabaseRelationId, objectOid, GetUserId()) &&
+ !has_privs_of_role(GetUserId(), ROLE_PG_MAINTAIN))
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_DATABASE,
get_database_name(objectOid));
}
@@ -3194,15 +3203,12 @@ ReindexMultipleTables(const ReindexStmt *stmt, const ReindexParams *params)
continue;
/*
- * The table can be reindexed if the user is superuser, the table
- * owner, or the database/schema owner (but in the latter case, only
- * if it's not a shared relation). object_ownercheck includes the
- * superuser case, and depending on objectKind we already know that
- * the user has permission to run REINDEX on this database or schema
- * per the permission checks at the beginning of this routine.
+ * We already checked privileges on the database or schema, but we
+ * further restrict reindexing shared catalogs to roles with the
+ * MAINTAIN privilege on the relation.
*/
if (classtuple->relisshared &&
- !object_ownercheck(RelationRelationId, relid, GetUserId()))
+ pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
continue;
/*
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 29e9953bf4..3170b3e3a1 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -284,7 +284,7 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid)
AclMode aclmask;
/* any of these privileges permit any lock mode */
- aclmask = ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE;
+ aclmask = ACL_MAINTAIN | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE;
/* SELECT privileges also permit ACCESS SHARE and below */
if (lockmode <= AccessShareLock)
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 59920ced83..848931b483 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -165,7 +165,8 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
*/
matviewOid = RangeVarGetRelidExtended(stmt->relation,
lockmode, 0,
- RangeVarCallbackOwnsTable, NULL);
+ RangeVarCallbackMaintainsTable,
+ NULL);
matviewRel = table_open(matviewOid, NoLock);
relowner = matviewRel->rd_rel->relowner;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 86ea3a228b..0b76ddba8f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -18087,15 +18087,16 @@ AtEOSubXact_on_commit_actions(bool isCommit, SubTransactionId mySubid,
* This is intended as a callback for RangeVarGetRelidExtended(). It allows
* the relation to be locked only if (1) it's a plain or partitioned table,
* materialized view, or TOAST table and (2) the current user is the owner (or
- * the superuser). This meets the permission-checking needs of CLUSTER,
- * REINDEX TABLE, and REFRESH MATERIALIZED VIEW; we expose it here so that it
- * can be used by all.
+ * the superuser) or has been granted MAINTAIN. This meets the
+ * permission-checking needs of CLUSTER, REINDEX TABLE, and REFRESH
+ * MATERIALIZED VIEW; we expose it here so that it can be used by all.
*/
void
-RangeVarCallbackOwnsTable(const RangeVar *relation,
- Oid relId, Oid oldRelId, void *arg)
+RangeVarCallbackMaintainsTable(const RangeVar *relation,
+ Oid relId, Oid oldRelId, void *arg)
{
char relkind;
+ AclResult aclresult;
/* Nothing to do if the relation was not found. */
if (!OidIsValid(relId))
@@ -18116,8 +18117,9 @@ RangeVarCallbackOwnsTable(const RangeVar *relation,
errmsg("\"%s\" is not a table or materialized view", relation->relname)));
/* Check permissions */
- if (!object_ownercheck(RelationRelationId, relId, GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relId)), relation->relname);
+ aclresult = pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_TABLE, relation->relname);
}
/*
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 64da848627..13c012af50 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -697,32 +697,35 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
}
/*
- * Check if a given relation can be safely vacuumed or analyzed. If the
- * user is not the relation owner, issue a WARNING log message and return
- * false to let the caller decide what to do with this relation. This
- * routine is used to decide if a relation can be processed for VACUUM or
- * ANALYZE.
+ * Check if the current user has privileges to vacuum or analyze the relation.
+ * If not, issue a WARNING log message and return false to let the caller
+ * decide what to do with this relation. This routine is used to decide if a
+ * relation can be processed for VACUUM or ANALYZE.
*/
bool
-vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, bits32 options)
+vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
+ bits32 options)
{
char *relname;
Assert((options & (VACOPT_VACUUM | VACOPT_ANALYZE)) != 0);
/*
- * Check permissions.
- *
- * We allow the user to vacuum or analyze a table if he is superuser, the
- * table owner, or the database owner (but in the latter case, only if
- * it's not a shared relation). object_ownercheck includes the superuser
- * case.
- *
- * Note we choose to treat permissions failure as a WARNING and keep
- * trying to vacuum or analyze the rest of the DB --- is this appropriate?
+ * Privilege checks are bypassed in some cases (e.g., when recursing to a
+ * relation's TOAST table).
*/
- if (object_ownercheck(RelationRelationId, relid, GetUserId()) ||
- (object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared))
+ if (options & VACOPT_SKIP_PRIVS)
+ return true;
+
+ /*----------
+ * A role has privileges to vacuum or analyze the relation if any of the
+ * following are true:
+ * - the role owns the current database and the relation is not shared
+ * - the role has the MAINTAIN privilege on the relation
+ *----------
+ */
+ if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) ||
+ pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK)
return true;
relname = NameStr(reltuple->relname);
@@ -938,10 +941,10 @@ expand_vacuum_rel(VacuumRelation *vrel, MemoryContext vac_context,
classForm = (Form_pg_class) GETSTRUCT(tuple);
/*
- * Make a returnable VacuumRelation for this rel if user is a proper
- * owner.
+ * Make a returnable VacuumRelation for this rel if the user has the
+ * required privileges.
*/
- if (vacuum_is_relation_owner(relid, classForm, options))
+ if (vacuum_is_permitted_for_relation(relid, classForm, options))
{
oldcontext = MemoryContextSwitchTo(vac_context);
vacrels = lappend(vacrels, makeVacuumRelation(vrel->relation,
@@ -1038,7 +1041,7 @@ get_all_vacuum_rels(MemoryContext vac_context, int options)
continue;
/* check permissions of relation */
- if (!vacuum_is_relation_owner(relid, classForm, options))
+ if (!vacuum_is_permitted_for_relation(relid, classForm, options))
continue;
/*
@@ -2030,16 +2033,15 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
}
/*
- * Check if relation needs to be skipped based on ownership. This check
+ * Check if relation needs to be skipped based on privileges. This check
* happens also when building the relation list to vacuum for a manual
* operation, and needs to be done additionally here as VACUUM could
- * happen across multiple transactions where relation ownership could have
- * changed in-between. Make sure to only generate logs for VACUUM in this
- * case.
+ * happen across multiple transactions where privileges could have changed
+ * in-between. Make sure to only generate logs for VACUUM in this case.
*/
- if (!vacuum_is_relation_owner(RelationGetRelid(rel),
- rel->rd_rel,
- params->options & VACOPT_VACUUM))
+ if (!vacuum_is_permitted_for_relation(RelationGetRelid(rel),
+ rel->rd_rel,
+ params->options & ~VACOPT_ANALYZE))
{
relation_close(rel, lmode);
PopActiveSnapshot();
@@ -2225,9 +2227,14 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
{
VacuumParams toast_vacuum_params;
- /* force VACOPT_PROCESS_MAIN so vacuum_rel() processes it */
+ /*
+ * Force VACOPT_PROCESS_MAIN so vacuum_rel() processes it. Likewise,
+ * set VACOPT_SKIP_PRIVS since privileges on the main relation are
+ * sufficient to process it.
+ */
memcpy(&toast_vacuum_params, params, sizeof(VacuumParams));
toast_vacuum_params.options |= VACOPT_PROCESS_MAIN;
+ toast_vacuum_params.options |= VACOPT_SKIP_PRIVS;
vacuum_rel(toast_relid, NULL, &toast_vacuum_params, bstrategy);
}
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 85555d1c5a..2c9a8e8707 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -332,6 +332,9 @@ aclparse(const char *s, AclItem *aip, Node *escontext)
case ACL_ALTER_SYSTEM_CHR:
read = ACL_ALTER_SYSTEM;
break;
+ case ACL_MAINTAIN_CHR:
+ read = ACL_MAINTAIN;
+ break;
case 'R': /* ignore old RULE privileges */
read = 0;
break;
@@ -1623,6 +1626,7 @@ makeaclitem(PG_FUNCTION_ARGS)
{"CONNECT", ACL_CONNECT},
{"SET", ACL_SET},
{"ALTER SYSTEM", ACL_ALTER_SYSTEM},
+ {"MAINTAIN", ACL_MAINTAIN},
{"RULE", 0}, /* ignore old RULE privileges */
{NULL, 0}
};
@@ -1731,6 +1735,8 @@ convert_aclright_to_string(int aclright)
return "SET";
case ACL_ALTER_SYSTEM:
return "ALTER SYSTEM";
+ case ACL_MAINTAIN:
+ return "MAINTAIN";
default:
elog(ERROR, "unrecognized aclright: %d", aclright);
return NULL;
@@ -2043,6 +2049,8 @@ convert_table_priv_string(text *priv_type_text)
{"REFERENCES WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_REFERENCES)},
{"TRIGGER", ACL_TRIGGER},
{"TRIGGER WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_TRIGGER)},
+ {"MAINTAIN", ACL_MAINTAIN},
+ {"MAINTAIN WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_MAINTAIN)},
{"RULE", 0}, /* ignore old RULE privileges */
{"RULE WITH GRANT OPTION", 0},
{NULL, 0}
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 78193c3c2b..5649859aa1 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -463,6 +463,7 @@ do { \
CONVERT_PRIV('d', "DELETE");
CONVERT_PRIV('t', "TRIGGER");
CONVERT_PRIV('D', "TRUNCATE");
+ CONVERT_PRIV('m', "MAINTAIN");
}
}
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 00b5092713..c8b489d94e 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -794,7 +794,7 @@ my %tests = (
\QREVOKE ALL ON TABLES FROM regress_dump_test_role;\E\n
\QALTER DEFAULT PRIVILEGES \E
\QFOR ROLE regress_dump_test_role \E
- \QGRANT INSERT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLES TO regress_dump_test_role;\E
+ \QGRANT INSERT,REFERENCES,DELETE,TRIGGER,TRUNCATE,MAINTAIN,UPDATE ON TABLES TO regress_dump_test_role;\E
/xm,
like => { %full_runs, section_post_data => 1, },
unlike => { no_privs => 1, },
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 151a5211ee..834357ced3 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1151,7 +1151,7 @@ Keywords_for_list_of_owner_roles, "PUBLIC"
#define Privilege_options_of_grant_and_revoke \
"SELECT", "INSERT", "UPDATE", "DELETE", "TRUNCATE", "REFERENCES", "TRIGGER", \
"CREATE", "CONNECT", "TEMPORARY", "EXECUTE", "USAGE", "SET", "ALTER SYSTEM", \
-"ALL"
+"MAINTAIN", "ALL"
/* ALTER PROCEDURE options */
#define Alter_procedure_options \
@@ -3942,7 +3942,7 @@ psql_completion(const char *text, int start, int end)
if (HeadMatches("ALTER", "DEFAULT", "PRIVILEGES"))
COMPLETE_WITH("SELECT", "INSERT", "UPDATE",
"DELETE", "TRUNCATE", "REFERENCES", "TRIGGER",
- "CREATE", "EXECUTE", "USAGE", "ALL");
+ "CREATE", "EXECUTE", "USAGE", "MAINTAIN", "ALL");
else if (TailMatches("GRANT"))
COMPLETE_WITH_QUERY_PLUS(Query_for_list_of_roles,
Privilege_options_of_grant_and_revoke);
@@ -3994,7 +3994,7 @@ psql_completion(const char *text, int start, int end)
else if (TailMatches("GRANT|REVOKE", MatchAny) ||
TailMatches("REVOKE", "GRANT", "OPTION", "FOR", MatchAny))
{
- if (TailMatches("SELECT|INSERT|UPDATE|DELETE|TRUNCATE|REFERENCES|TRIGGER|CREATE|CONNECT|TEMPORARY|TEMP|EXECUTE|USAGE|ALL"))
+ if (TailMatches("SELECT|INSERT|UPDATE|DELETE|TRUNCATE|REFERENCES|TRIGGER|CREATE|CONNECT|TEMPORARY|TEMP|EXECUTE|USAGE|MAINTAIN|ALL"))
COMPLETE_WITH("ON");
else if (TailMatches("GRANT", MatchAny))
COMPLETE_WITH("TO");
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 82a2ec2862..55cabdda6f 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -84,6 +84,11 @@
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '9256', oid_symbol => 'ROLE_PG_MAINTAIN',
+ rolname => 'pg_maintain', rolsuper => 'f', rolinherit => 't',
+ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
+ rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
+ rolpassword => '_null_', rolvaliduntil => '_null_' },
{ oid => '4550', oid_symbol => 'ROLE_PG_USE_RESERVED_CONNECTIONS',
rolname => 'pg_use_reserved_connections', rolsuper => 'f', rolinherit => 't',
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 60d9b8f5b5..85cbad3d0c 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -98,8 +98,9 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit,
SubTransactionId mySubid,
SubTransactionId parentSubid);
-extern void RangeVarCallbackOwnsTable(const RangeVar *relation,
- Oid relId, Oid oldRelId, void *arg);
+extern void RangeVarCallbackMaintainsTable(const RangeVar *relation,
+ Oid relId, Oid oldRelId,
+ void *arg);
extern void RangeVarCallbackOwnsRelation(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 7f623b37fd..f935f39133 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -187,6 +187,7 @@ typedef struct VacAttrStats
#define VACOPT_DISABLE_PAGE_SKIPPING 0x100 /* don't skip any pages */
#define VACOPT_SKIP_DATABASE_STATS 0x200 /* skip vac_update_datfrozenxid() */
#define VACOPT_ONLY_DATABASE_STATS 0x400 /* only vac_update_datfrozenxid() */
+#define VACOPT_SKIP_PRIVS 0x800 /* skip privilege checks */
/*
* Values used by index_cleanup and truncate params.
@@ -343,8 +344,8 @@ extern bool vacuum_get_cutoffs(Relation rel, const VacuumParams *params,
extern bool vacuum_xid_failsafe_check(const struct VacuumCutoffs *cutoffs);
extern void vac_update_datfrozenxid(void);
extern void vacuum_delay_point(void);
-extern bool vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple,
- bits32 options);
+extern bool vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
+ bits32 options);
extern Relation vacuum_open_relation(Oid relid, RangeVar *relation,
bits32 options, bool verbose,
LOCKMODE lmode);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 476d55dd24..bbe13f0379 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -87,7 +87,8 @@ typedef uint64 AclMode; /* a bitmask of privilege bits */
#define ACL_CONNECT (1<<11) /* for databases */
#define ACL_SET (1<<12) /* for configuration parameters */
#define ACL_ALTER_SYSTEM (1<<13) /* for configuration parameters */
-#define N_ACL_RIGHTS 14 /* 1 plus the last 1<<x */
+#define ACL_MAINTAIN (1<<14) /* for relations */
+#define N_ACL_RIGHTS 15 /* 1 plus the last 1<<x */
#define ACL_NO_RIGHTS 0
/* Currently, SELECT ... FOR [KEY] UPDATE/SHARE requires UPDATE privileges */
#define ACL_SELECT_FOR_UPDATE ACL_UPDATE
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index a803eb1291..3a0baf3039 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -148,15 +148,16 @@ typedef struct ArrayType Acl;
#define ACL_CONNECT_CHR 'c'
#define ACL_SET_CHR 's'
#define ACL_ALTER_SYSTEM_CHR 'A'
+#define ACL_MAINTAIN_CHR 'm'
/* string holding all privilege code chars, in order by bitmask position */
-#define ACL_ALL_RIGHTS_STR "arwdDxtXUCTcsA"
+#define ACL_ALL_RIGHTS_STR "arwdDxtXUCTcsAm"
/*
* Bitmasks defining "all rights" for each supported object type
*/
#define ACL_ALL_RIGHTS_COLUMN (ACL_INSERT|ACL_SELECT|ACL_UPDATE|ACL_REFERENCES)
-#define ACL_ALL_RIGHTS_RELATION (ACL_INSERT|ACL_SELECT|ACL_UPDATE|ACL_DELETE|ACL_TRUNCATE|ACL_REFERENCES|ACL_TRIGGER)
+#define ACL_ALL_RIGHTS_RELATION (ACL_INSERT|ACL_SELECT|ACL_UPDATE|ACL_DELETE|ACL_TRUNCATE|ACL_REFERENCES|ACL_TRIGGER|ACL_MAINTAIN)
#define ACL_ALL_RIGHTS_SEQUENCE (ACL_USAGE|ACL_SELECT|ACL_UPDATE)
#define ACL_ALL_RIGHTS_DATABASE (ACL_CREATE|ACL_CREATE_TEMP|ACL_CONNECT)
#define ACL_ALL_RIGHTS_FDW (ACL_USAGE)
diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out
index 7acb675c97..7be9e56ef1 100644
--- a/src/test/isolation/expected/cluster-conflict-partition.out
+++ b/src/test/isolation/expected/cluster-conflict-partition.out
@@ -3,7 +3,7 @@ Parsed test spec with 2 sessions
starting permutation: s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset
step s1_begin: BEGIN;
step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE;
-step s2_auth: SET ROLE regress_cluster_part;
+step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR;
step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...>
step s1_commit: COMMIT;
step s2_cluster: <... completed>
@@ -11,7 +11,7 @@ step s2_reset: RESET ROLE;
starting permutation: s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset
step s1_begin: BEGIN;
-step s2_auth: SET ROLE regress_cluster_part;
+step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR;
step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE;
step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...>
step s1_commit: COMMIT;
@@ -21,14 +21,14 @@ step s2_reset: RESET ROLE;
starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset
step s1_begin: BEGIN;
step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE;
-step s2_auth: SET ROLE regress_cluster_part;
+step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR;
step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind;
step s1_commit: COMMIT;
step s2_reset: RESET ROLE;
starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset
step s1_begin: BEGIN;
-step s2_auth: SET ROLE regress_cluster_part;
+step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR;
step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE;
step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind;
step s1_commit: COMMIT;
diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec
index 5091f684a9..4d38a7f49a 100644
--- a/src/test/isolation/specs/cluster-conflict-partition.spec
+++ b/src/test/isolation/specs/cluster-conflict-partition.spec
@@ -23,7 +23,7 @@ step s1_lock_child { LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE;
step s1_commit { COMMIT; }
session s2
-step s2_auth { SET ROLE regress_cluster_part; }
+step s2_auth { SET ROLE regress_cluster_part; SET client_min_messages = ERROR; }
step s2_cluster { CLUSTER cluster_part_tab USING cluster_part_ind; }
step s2_reset { RESET ROLE; }
diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
index a6222ae14c..6f853e619c 100644
--- a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -287,6 +287,17 @@ sub adjust_old_dumpfile
$dump = _mash_view_qualifiers($dump);
}
+ if ($old_version >= 14 && $old_version < 17)
+ {
+ # Fix up some privilege-set discrepancies.
+ $dump =~
+ s {^REVOKE SELECT,INSERT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLE}
+ {REVOKE ALL ON TABLE}mg;
+ $dump =~
+ s {^(GRANT SELECT,INSERT,REFERENCES,TRIGGER,TRUNCATE),UPDATE ON TABLE}
+ {$1,MAINTAIN,UPDATE ON TABLE}mg;
+ }
+
if ($old_version < 14)
{
# Remove mentions of extended hash functions.
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index a666d89ef5..4d40a6809a 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -353,7 +353,9 @@ INSERT INTO clstr_3 VALUES (1);
-- this user can only cluster clstr_1 and clstr_3, but the latter
-- has not been clustered
SET SESSION AUTHORIZATION regress_clstr_user;
+SET client_min_messages = ERROR; -- order of "skipping" warnings may vary
CLUSTER;
+RESET client_min_messages;
SELECT * FROM clstr_1 UNION ALL
SELECT * FROM clstr_2 UNION ALL
SELECT * FROM clstr_3;
@@ -501,12 +503,17 @@ CREATE TABLE ptnowner1 PARTITION OF ptnowner FOR VALUES IN (1);
CREATE ROLE regress_ptnowner;
CREATE TABLE ptnowner2 PARTITION OF ptnowner FOR VALUES IN (2);
ALTER TABLE ptnowner1 OWNER TO regress_ptnowner;
+SET SESSION AUTHORIZATION regress_ptnowner;
+CLUSTER ptnowner USING ptnowner_i_idx;
+ERROR: permission denied for table ptnowner
+RESET SESSION AUTHORIZATION;
ALTER TABLE ptnowner OWNER TO regress_ptnowner;
CREATE TEMP TABLE ptnowner_oldnodes AS
SELECT oid, relname, relfilenode FROM pg_partition_tree('ptnowner') AS tree
JOIN pg_class AS c ON c.oid=tree.relid;
SET SESSION AUTHORIZATION regress_ptnowner;
CLUSTER ptnowner USING ptnowner_i_idx;
+WARNING: permission denied to cluster "ptnowner2", skipping it
RESET SESSION AUTHORIZATION;
SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a
JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C";
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 79fa117cb5..70ab47a92f 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2832,9 +2832,9 @@ RESET ROLE;
GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser;
SET SESSION ROLE regress_reindexuser;
REINDEX TABLE pg_toast.pg_toast_1260;
-ERROR: must be owner of table pg_toast_1260
+ERROR: permission denied for table pg_toast_1260
REINDEX INDEX pg_toast.pg_toast_1260_index;
-ERROR: must be owner of index pg_toast_1260_index
+ERROR: permission denied for index pg_toast_1260_index
-- Clean up
RESET ROLE;
REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser;
diff --git a/src/test/regress/expected/dependency.out b/src/test/regress/expected/dependency.out
index 5a9ee5d2cd..74d9ff2998 100644
--- a/src/test/regress/expected/dependency.out
+++ b/src/test/regress/expected/dependency.out
@@ -19,7 +19,7 @@ DETAIL: privileges for table deptest
REVOKE SELECT ON deptest FROM GROUP regress_dep_group;
DROP GROUP regress_dep_group;
-- can't drop the user if we revoke the privileges partially
-REVOKE SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES ON deptest FROM regress_dep_user;
+REVOKE SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, MAINTAIN ON deptest FROM regress_dep_user;
DROP USER regress_dep_user;
ERROR: role "regress_dep_user" cannot be dropped because some objects depend on it
DETAIL: privileges for table deptest
@@ -67,21 +67,21 @@ CREATE TABLE deptest (a serial primary key, b text);
GRANT ALL ON deptest1 TO regress_dep_user2;
RESET SESSION AUTHORIZATION;
\z deptest1
- Access privileges
- Schema | Name | Type | Access privileges | Column privileges | Policies
---------+----------+-------+----------------------------------------------------+-------------------+----------
- public | deptest1 | table | regress_dep_user0=arwdDxt/regress_dep_user0 +| |
- | | | regress_dep_user1=a*r*w*d*D*x*t*/regress_dep_user0+| |
- | | | regress_dep_user2=arwdDxt/regress_dep_user1 | |
+ Access privileges
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------+----------+-------+------------------------------------------------------+-------------------+----------
+ public | deptest1 | table | regress_dep_user0=arwdDxtm/regress_dep_user0 +| |
+ | | | regress_dep_user1=a*r*w*d*D*x*t*m*/regress_dep_user0+| |
+ | | | regress_dep_user2=arwdDxtm/regress_dep_user1 | |
(1 row)
DROP OWNED BY regress_dep_user1;
-- all grants revoked
\z deptest1
- Access privileges
- Schema | Name | Type | Access privileges | Column privileges | Policies
---------+----------+-------+---------------------------------------------+-------------------+----------
- public | deptest1 | table | regress_dep_user0=arwdDxt/regress_dep_user0 | |
+ Access privileges
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------+----------+-------+----------------------------------------------+-------------------+----------
+ public | deptest1 | table | regress_dep_user0=arwdDxtm/regress_dep_user0 | |
(1 row)
-- table was dropped
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index fbb0489a4f..d702a9b6e0 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2296,9 +2296,9 @@ SELECT pg_input_is_valid('regress_priv_user1=rY', 'aclitem');
(1 row)
SELECT * FROM pg_input_error_info('regress_priv_user1=rY', 'aclitem');
- message | detail | hint | sql_error_code
----------------------------------------------------------+--------+------+----------------
- invalid mode character: must be one of "arwdDxtXUCTcsA" | | | 22P02
+ message | detail | hint | sql_error_code
+----------------------------------------------------------+--------+------+----------------
+ invalid mode character: must be one of "arwdDxtXUCTcsAm" | | | 22P02
(1 row)
--
@@ -2639,38 +2639,38 @@ set session role regress_priv_user4;
grant select on dep_priv_test to regress_priv_user5;
\dp dep_priv_test
Access privileges
- Schema | Name | Type | Access privileges | Column privileges | Policies
---------+---------------+-------+-----------------------------------------------+-------------------+----------
- public | dep_priv_test | table | regress_priv_user1=arwdDxt/regress_priv_user1+| |
- | | | regress_priv_user2=r*/regress_priv_user1 +| |
- | | | regress_priv_user3=r*/regress_priv_user1 +| |
- | | | regress_priv_user4=r*/regress_priv_user2 +| |
- | | | regress_priv_user4=r*/regress_priv_user3 +| |
- | | | regress_priv_user5=r/regress_priv_user4 | |
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------+---------------+-------+------------------------------------------------+-------------------+----------
+ public | dep_priv_test | table | regress_priv_user1=arwdDxtm/regress_priv_user1+| |
+ | | | regress_priv_user2=r*/regress_priv_user1 +| |
+ | | | regress_priv_user3=r*/regress_priv_user1 +| |
+ | | | regress_priv_user4=r*/regress_priv_user2 +| |
+ | | | regress_priv_user4=r*/regress_priv_user3 +| |
+ | | | regress_priv_user5=r/regress_priv_user4 | |
(1 row)
set session role regress_priv_user2;
revoke select on dep_priv_test from regress_priv_user4 cascade;
\dp dep_priv_test
Access privileges
- Schema | Name | Type | Access privileges | Column privileges | Policies
---------+---------------+-------+-----------------------------------------------+-------------------+----------
- public | dep_priv_test | table | regress_priv_user1=arwdDxt/regress_priv_user1+| |
- | | | regress_priv_user2=r*/regress_priv_user1 +| |
- | | | regress_priv_user3=r*/regress_priv_user1 +| |
- | | | regress_priv_user4=r*/regress_priv_user3 +| |
- | | | regress_priv_user5=r/regress_priv_user4 | |
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------+---------------+-------+------------------------------------------------+-------------------+----------
+ public | dep_priv_test | table | regress_priv_user1=arwdDxtm/regress_priv_user1+| |
+ | | | regress_priv_user2=r*/regress_priv_user1 +| |
+ | | | regress_priv_user3=r*/regress_priv_user1 +| |
+ | | | regress_priv_user4=r*/regress_priv_user3 +| |
+ | | | regress_priv_user5=r/regress_priv_user4 | |
(1 row)
set session role regress_priv_user3;
revoke select on dep_priv_test from regress_priv_user4 cascade;
\dp dep_priv_test
Access privileges
- Schema | Name | Type | Access privileges | Column privileges | Policies
---------+---------------+-------+-----------------------------------------------+-------------------+----------
- public | dep_priv_test | table | regress_priv_user1=arwdDxt/regress_priv_user1+| |
- | | | regress_priv_user2=r*/regress_priv_user1 +| |
- | | | regress_priv_user3=r*/regress_priv_user1 | |
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------+---------------+-------+------------------------------------------------+-------------------+----------
+ public | dep_priv_test | table | regress_priv_user1=arwdDxtm/regress_priv_user1+| |
+ | | | regress_priv_user2=r*/regress_priv_user1 +| |
+ | | | regress_priv_user3=r*/regress_priv_user1 | |
(1 row)
set session role regress_priv_user1;
@@ -2800,6 +2800,20 @@ LOCK TABLE lock_table IN ACCESS EXCLUSIVE MODE; -- should pass
COMMIT;
\c
REVOKE TRUNCATE ON lock_table FROM regress_locktable_user;
+-- LOCK TABLE and MAINTAIN permission
+GRANT MAINTAIN ON lock_table TO regress_locktable_user;
+SET SESSION AUTHORIZATION regress_locktable_user;
+BEGIN;
+LOCK TABLE lock_table IN ACCESS SHARE MODE; -- should pass
+ROLLBACK;
+BEGIN;
+LOCK TABLE lock_table IN ROW EXCLUSIVE MODE; -- should pass
+COMMIT;
+BEGIN;
+LOCK TABLE lock_table IN ACCESS EXCLUSIVE MODE; -- should pass
+COMMIT;
+\c
+REVOKE MAINTAIN ON lock_table FROM regress_locktable_user;
-- clean up
DROP TABLE lock_table;
DROP USER regress_locktable_user;
@@ -2913,3 +2927,59 @@ DROP SCHEMA regress_roleoption;
DROP ROLE regress_roleoption_protagonist;
DROP ROLE regress_roleoption_donor;
DROP ROLE regress_roleoption_recipient;
+-- MAINTAIN
+CREATE ROLE regress_no_maintain;
+CREATE ROLE regress_maintain;
+CREATE ROLE regress_maintain_all IN ROLE pg_maintain;
+CREATE TABLE maintain_test (a INT);
+CREATE INDEX ON maintain_test (a);
+GRANT MAINTAIN ON maintain_test TO regress_maintain;
+CREATE MATERIALIZED VIEW refresh_test AS SELECT 1;
+GRANT MAINTAIN ON refresh_test TO regress_maintain;
+CREATE SCHEMA reindex_test;
+-- negative tests; should fail
+SET ROLE regress_no_maintain;
+VACUUM maintain_test;
+WARNING: permission denied to vacuum "maintain_test", skipping it
+ANALYZE maintain_test;
+WARNING: permission denied to analyze "maintain_test", skipping it
+VACUUM (ANALYZE) maintain_test;
+WARNING: permission denied to vacuum "maintain_test", skipping it
+CLUSTER maintain_test USING maintain_test_a_idx;
+ERROR: permission denied for table maintain_test
+REFRESH MATERIALIZED VIEW refresh_test;
+ERROR: permission denied for table refresh_test
+REINDEX TABLE maintain_test;
+ERROR: permission denied for table maintain_test
+REINDEX INDEX maintain_test_a_idx;
+ERROR: permission denied for index maintain_test_a_idx
+REINDEX SCHEMA reindex_test;
+ERROR: must be owner of schema reindex_test
+RESET ROLE;
+SET ROLE regress_maintain;
+VACUUM maintain_test;
+ANALYZE maintain_test;
+VACUUM (ANALYZE) maintain_test;
+CLUSTER maintain_test USING maintain_test_a_idx;
+REFRESH MATERIALIZED VIEW refresh_test;
+REINDEX TABLE maintain_test;
+REINDEX INDEX maintain_test_a_idx;
+REINDEX SCHEMA reindex_test;
+ERROR: must be owner of schema reindex_test
+RESET ROLE;
+SET ROLE regress_maintain_all;
+VACUUM maintain_test;
+ANALYZE maintain_test;
+VACUUM (ANALYZE) maintain_test;
+CLUSTER maintain_test USING maintain_test_a_idx;
+REFRESH MATERIALIZED VIEW refresh_test;
+REINDEX TABLE maintain_test;
+REINDEX INDEX maintain_test_a_idx;
+REINDEX SCHEMA reindex_test;
+RESET ROLE;
+DROP TABLE maintain_test;
+DROP MATERIALIZED VIEW refresh_test;
+DROP SCHEMA reindex_test;
+DROP ROLE regress_no_maintain;
+DROP ROLE regress_maintain;
+DROP ROLE regress_maintain_all;
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 6988128aa4..4538f0c37d 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -93,23 +93,23 @@ CREATE POLICY p2r ON document AS RESTRICTIVE TO regress_rls_dave
CREATE POLICY p1r ON document AS RESTRICTIVE TO regress_rls_dave
USING (cid <> 44);
\dp
- Access privileges
- Schema | Name | Type | Access privileges | Column privileges | Policies
---------------------+----------+-------+---------------------------------------------+-------------------+--------------------------------------------
- regress_rls_schema | category | table | regress_rls_alice=arwdDxt/regress_rls_alice+| |
- | | | =arwdDxt/regress_rls_alice | |
- regress_rls_schema | document | table | regress_rls_alice=arwdDxt/regress_rls_alice+| | p1: +
- | | | =arwdDxt/regress_rls_alice | | (u): (dlevel <= ( SELECT uaccount.seclv +
- | | | | | FROM uaccount +
- | | | | | WHERE (uaccount.pguser = CURRENT_USER)))+
- | | | | | p2r (RESTRICTIVE): +
- | | | | | (u): ((cid <> 44) AND (cid < 50)) +
- | | | | | to: regress_rls_dave +
- | | | | | p1r (RESTRICTIVE): +
- | | | | | (u): (cid <> 44) +
- | | | | | to: regress_rls_dave
- regress_rls_schema | uaccount | table | regress_rls_alice=arwdDxt/regress_rls_alice+| |
- | | | =r/regress_rls_alice | |
+ Access privileges
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------------------+----------+-------+----------------------------------------------+-------------------+--------------------------------------------
+ regress_rls_schema | category | table | regress_rls_alice=arwdDxtm/regress_rls_alice+| |
+ | | | =arwdDxtm/regress_rls_alice | |
+ regress_rls_schema | document | table | regress_rls_alice=arwdDxtm/regress_rls_alice+| | p1: +
+ | | | =arwdDxtm/regress_rls_alice | | (u): (dlevel <= ( SELECT uaccount.seclv +
+ | | | | | FROM uaccount +
+ | | | | | WHERE (uaccount.pguser = CURRENT_USER)))+
+ | | | | | p2r (RESTRICTIVE): +
+ | | | | | (u): ((cid <> 44) AND (cid < 50)) +
+ | | | | | to: regress_rls_dave +
+ | | | | | p1r (RESTRICTIVE): +
+ | | | | | (u): (cid <> 44) +
+ | | | | | to: regress_rls_dave
+ regress_rls_schema | uaccount | table | regress_rls_alice=arwdDxtm/regress_rls_alice+| |
+ | | | =r/regress_rls_alice | |
(3 rows)
\d document
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 6cb9c926c0..b7115f8610 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -145,7 +145,9 @@ INSERT INTO clstr_3 VALUES (1);
-- this user can only cluster clstr_1 and clstr_3, but the latter
-- has not been clustered
SET SESSION AUTHORIZATION regress_clstr_user;
+SET client_min_messages = ERROR; -- order of "skipping" warnings may vary
CLUSTER;
+RESET client_min_messages;
SELECT * FROM clstr_1 UNION ALL
SELECT * FROM clstr_2 UNION ALL
SELECT * FROM clstr_3;
@@ -236,6 +238,9 @@ CREATE TABLE ptnowner1 PARTITION OF ptnowner FOR VALUES IN (1);
CREATE ROLE regress_ptnowner;
CREATE TABLE ptnowner2 PARTITION OF ptnowner FOR VALUES IN (2);
ALTER TABLE ptnowner1 OWNER TO regress_ptnowner;
+SET SESSION AUTHORIZATION regress_ptnowner;
+CLUSTER ptnowner USING ptnowner_i_idx;
+RESET SESSION AUTHORIZATION;
ALTER TABLE ptnowner OWNER TO regress_ptnowner;
CREATE TEMP TABLE ptnowner_oldnodes AS
SELECT oid, relname, relfilenode FROM pg_partition_tree('ptnowner') AS tree
diff --git a/src/test/regress/sql/dependency.sql b/src/test/regress/sql/dependency.sql
index 2559c62d0b..8d74ed7122 100644
--- a/src/test/regress/sql/dependency.sql
+++ b/src/test/regress/sql/dependency.sql
@@ -21,7 +21,7 @@ REVOKE SELECT ON deptest FROM GROUP regress_dep_group;
DROP GROUP regress_dep_group;
-- can't drop the user if we revoke the privileges partially
-REVOKE SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES ON deptest FROM regress_dep_user;
+REVOKE SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, MAINTAIN ON deptest FROM regress_dep_user;
DROP USER regress_dep_user;
-- now we are OK to drop him
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 3f68cafcd1..323c3f25ce 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1793,6 +1793,21 @@ COMMIT;
\c
REVOKE TRUNCATE ON lock_table FROM regress_locktable_user;
+-- LOCK TABLE and MAINTAIN permission
+GRANT MAINTAIN ON lock_table TO regress_locktable_user;
+SET SESSION AUTHORIZATION regress_locktable_user;
+BEGIN;
+LOCK TABLE lock_table IN ACCESS SHARE MODE; -- should pass
+ROLLBACK;
+BEGIN;
+LOCK TABLE lock_table IN ROW EXCLUSIVE MODE; -- should pass
+COMMIT;
+BEGIN;
+LOCK TABLE lock_table IN ACCESS EXCLUSIVE MODE; -- should pass
+COMMIT;
+\c
+REVOKE MAINTAIN ON lock_table FROM regress_locktable_user;
+
-- clean up
DROP TABLE lock_table;
DROP USER regress_locktable_user;
@@ -1876,3 +1891,56 @@ DROP SCHEMA regress_roleoption;
DROP ROLE regress_roleoption_protagonist;
DROP ROLE regress_roleoption_donor;
DROP ROLE regress_roleoption_recipient;
+
+-- MAINTAIN
+CREATE ROLE regress_no_maintain;
+CREATE ROLE regress_maintain;
+CREATE ROLE regress_maintain_all IN ROLE pg_maintain;
+
+CREATE TABLE maintain_test (a INT);
+CREATE INDEX ON maintain_test (a);
+GRANT MAINTAIN ON maintain_test TO regress_maintain;
+CREATE MATERIALIZED VIEW refresh_test AS SELECT 1;
+GRANT MAINTAIN ON refresh_test TO regress_maintain;
+CREATE SCHEMA reindex_test;
+
+-- negative tests; should fail
+SET ROLE regress_no_maintain;
+VACUUM maintain_test;
+ANALYZE maintain_test;
+VACUUM (ANALYZE) maintain_test;
+CLUSTER maintain_test USING maintain_test_a_idx;
+REFRESH MATERIALIZED VIEW refresh_test;
+REINDEX TABLE maintain_test;
+REINDEX INDEX maintain_test_a_idx;
+REINDEX SCHEMA reindex_test;
+RESET ROLE;
+
+SET ROLE regress_maintain;
+VACUUM maintain_test;
+ANALYZE maintain_test;
+VACUUM (ANALYZE) maintain_test;
+CLUSTER maintain_test USING maintain_test_a_idx;
+REFRESH MATERIALIZED VIEW refresh_test;
+REINDEX TABLE maintain_test;
+REINDEX INDEX maintain_test_a_idx;
+REINDEX SCHEMA reindex_test;
+RESET ROLE;
+
+SET ROLE regress_maintain_all;
+VACUUM maintain_test;
+ANALYZE maintain_test;
+VACUUM (ANALYZE) maintain_test;
+CLUSTER maintain_test USING maintain_test_a_idx;
+REFRESH MATERIALIZED VIEW refresh_test;
+REINDEX TABLE maintain_test;
+REINDEX INDEX maintain_test_a_idx;
+REINDEX SCHEMA reindex_test;
+RESET ROLE;
+
+DROP TABLE maintain_test;
+DROP MATERIALIZED VIEW refresh_test;
+DROP SCHEMA reindex_test;
+DROP ROLE regress_no_maintain;
+DROP ROLE regress_maintain;
+DROP ROLE regress_maintain_all;
--
2.25.1
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Wed, 2024-02-14 at 13:02 -0600, Nathan Bossart wrote:
This seemed like the approach folks were most in favor of at the
developer
meeting a couple weeks ago [0]. At least, that was my interpretation
of
the discussion.
Attached rebased version.
Note the changes in amcheck. It's creating functions and calling those
functions from the comparators, and so the comparators need to set the
search_path. I don't think that's terribly common, but does represent a
behavior change and could break something.
Regards,
Jef Davis
Attachments:
v4-0001-Fix-search_path-to-a-safe-value-during-maintenanc.patchtext/x-patch; charset=UTF-8; name=v4-0001-Fix-search_path-to-a-safe-value-during-maintenanc.patchDownload
From 6f399aa46fb97e73ad88eac90fe53d7e19c88f2b Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Fri, 16 Feb 2024 14:04:23 -0800
Subject: [PATCH v4] Fix search_path to a safe value during maintenance
operations.
While executing maintenance operations (ANALYZE, CLUSTER, REFRESH
MATERIALIZED VIEW, REINDEX, or VACUUM), set search_path to
'pg_catalog, pg_temp' to prevent inconsistent behavior.
Functions that are used for functional indexes, in index expressions,
or in materialized views and depend on a different search path must be
declared with CREATE FUNCTION ... SET search_path='...'.
This change was previously committed as 05e1737351, then reverted in
commit 2fcc7ee7af because it was too late in the cycle.
Preparation for the MAINTAIN privilege, which was previously reverted
due to search_path manipulation hazards.
Discussion: https://postgr.es/m/d4ccaf3658cb3c281ec88c851a09733cd9482f22.camel@j-davis.com
Discussion: https://postgr.es/m/E1q7j7Y-000z1H-Hr%40gemulon.postgresql.org
Discussion: https://postgr.es/m/e44327179e5c9015c8dda67351c04da552066017.camel%40j-davis.com
Reviewed-by: Greg Stark
Reviewed-by: Nathan Bossart
---
contrib/amcheck/t/004_verify_nbtree_unique.pl | 33 +++++++++++--------
contrib/amcheck/verify_nbtree.c | 2 ++
src/backend/access/brin/brin.c | 2 ++
src/backend/catalog/index.c | 8 +++++
src/backend/catalog/namespace.c | 10 +++---
src/backend/commands/analyze.c | 2 ++
src/backend/commands/cluster.c | 2 ++
src/backend/commands/indexcmds.c | 6 ++++
src/backend/commands/matview.c | 2 ++
src/backend/commands/vacuum.c | 2 ++
src/bin/scripts/t/100_vacuumdb.pl | 4 ---
src/include/utils/guc.h | 6 ++++
.../test_oat_hooks/expected/alter_table.out | 2 ++
.../expected/test_oat_hooks.out | 4 +++
src/test/regress/expected/matview.out | 4 ++-
src/test/regress/expected/privileges.out | 12 +++----
src/test/regress/expected/vacuum.out | 2 +-
src/test/regress/sql/matview.sql | 4 ++-
src/test/regress/sql/privileges.sql | 8 ++---
src/test/regress/sql/vacuum.sql | 2 +-
20 files changed, 81 insertions(+), 36 deletions(-)
diff --git a/contrib/amcheck/t/004_verify_nbtree_unique.pl b/contrib/amcheck/t/004_verify_nbtree_unique.pl
index 3f474a158a..4b704e6815 100644
--- a/contrib/amcheck/t/004_verify_nbtree_unique.pl
+++ b/contrib/amcheck/t/004_verify_nbtree_unique.pl
@@ -20,8 +20,11 @@ $node->safe_psql(
'postgres', q(
CREATE EXTENSION amcheck;
+ CREATE SCHEMA test_amcheck;
+ SET search_path = test_amcheck;
+
CREATE FUNCTION ok_cmp (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT
CASE WHEN $1 < $2 THEN -1
@@ -34,7 +37,7 @@ $node->safe_psql(
--- Check 1: uniqueness violation.
---
CREATE FUNCTION ok_cmp1 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT ok_cmp($1, $2);
$$;
@@ -43,7 +46,7 @@ $node->safe_psql(
--- Make values 768 and 769 look equal.
---
CREATE FUNCTION bad_cmp1 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT
CASE WHEN ($1 = 768 AND $2 = 769) OR
@@ -56,13 +59,13 @@ $node->safe_psql(
--- Check 2: uniqueness violation without deduplication.
---
CREATE FUNCTION ok_cmp2 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT ok_cmp($1, $2);
$$;
CREATE FUNCTION bad_cmp2 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT
CASE WHEN $1 = $2 AND $1 = 400 THEN -1
@@ -74,13 +77,13 @@ $node->safe_psql(
--- Check 3: uniqueness violation with deduplication.
---
CREATE FUNCTION ok_cmp3 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT ok_cmp($1, $2);
$$;
CREATE FUNCTION bad_cmp3 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT bad_cmp2($1, $2);
$$;
@@ -142,7 +145,7 @@ my ($result, $stdout, $stderr);
# We have not yet broken the index, so we should get no corruption
$result = $node->safe_psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx1', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx1', true, true);
));
is($result, '', 'run amcheck on non-broken bttest_unique_idx1');
@@ -150,6 +153,7 @@ is($result, '', 'run amcheck on non-broken bttest_unique_idx1');
# values to be equal.
$node->safe_psql(
'postgres', q(
+ SET search_path = test_amcheck;
UPDATE pg_catalog.pg_amproc SET
amproc = 'bad_cmp1'::regproc
WHERE amproc = 'ok_cmp1'::regproc;
@@ -157,7 +161,7 @@ $node->safe_psql(
($result, $stdout, $stderr) = $node->psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx1', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx1', true, true);
));
ok( $stderr =~ /index uniqueness is violated for index "bttest_unique_idx1"/,
'detected uniqueness violation for index "bttest_unique_idx1"');
@@ -175,13 +179,14 @@ ok( $stderr =~ /index uniqueness is violated for index "bttest_unique_idx1"/,
# but no uniqueness violation.
($result, $stdout, $stderr) = $node->psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx2', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx2', true, true);
));
ok( $stderr =~ /item order invariant violated for index "bttest_unique_idx2"/,
'detected item order invariant violation for index "bttest_unique_idx2"');
$node->safe_psql(
'postgres', q(
+ SET search_path = test_amcheck;
UPDATE pg_catalog.pg_amproc SET
amproc = 'ok_cmp2'::regproc
WHERE amproc = 'bad_cmp2'::regproc;
@@ -189,7 +194,7 @@ $node->safe_psql(
($result, $stdout, $stderr) = $node->psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx2', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx2', true, true);
));
ok( $stderr =~ /index uniqueness is violated for index "bttest_unique_idx2"/,
'detected uniqueness violation for index "bttest_unique_idx2"');
@@ -206,7 +211,7 @@ ok( $stderr =~ /index uniqueness is violated for index "bttest_unique_idx2"/,
# but no uniqueness violation.
($result, $stdout, $stderr) = $node->psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx3', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx3', true, true);
));
ok( $stderr =~ /item order invariant violated for index "bttest_unique_idx3"/,
'detected item order invariant violation for index "bttest_unique_idx3"');
@@ -215,6 +220,7 @@ ok( $stderr =~ /item order invariant violated for index "bttest_unique_idx3"/,
# with different visibility.
$node->safe_psql(
'postgres', q(
+ SET search_path = test_amcheck;
DELETE FROM bttest_unique3 WHERE 380 <= i AND i <= 420;
INSERT INTO bttest_unique3 (SELECT * FROM generate_series(380, 420));
INSERT INTO bttest_unique3 VALUES (400);
@@ -228,6 +234,7 @@ $node->safe_psql(
$node->safe_psql(
'postgres', q(
+ SET search_path = test_amcheck;
UPDATE pg_catalog.pg_amproc SET
amproc = 'ok_cmp3'::regproc
WHERE amproc = 'bad_cmp3'::regproc;
@@ -235,7 +242,7 @@ $node->safe_psql(
($result, $stdout, $stderr) = $node->psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx3', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx3', true, true);
));
ok( $stderr =~ /index uniqueness is violated for index "bttest_unique_idx3"/,
'detected uniqueness violation for index "bttest_unique_idx3"');
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 91caa53dd8..bff8c61262 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -313,6 +313,8 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
SetUserIdAndSecContext(heaprel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
}
else
{
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 1087a9011e..d1854c2a1c 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1412,6 +1412,8 @@ brin_summarize_range(PG_FUNCTION_ARGS)
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
}
else
{
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 4b88a9cb87..2203ce7152 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1469,6 +1469,8 @@ index_concurrently_build(Oid heapRelationId,
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
indexRelation = index_open(indexRelationId, RowExclusiveLock);
@@ -3021,6 +3023,8 @@ index_build(Relation heapRelation,
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/* Set up initial progress report status */
{
@@ -3356,6 +3360,8 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
indexRelation = index_open(indexId, RowExclusiveLock);
@@ -3617,6 +3623,8 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
if (progress)
{
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 8df30b2440..da0380a3c0 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -4717,6 +4717,11 @@ assign_search_path(const char *newval, void *extra)
void
InitializeSearchPath(void)
{
+ /* Make the context we'll keep search path cache hashtable in */
+ SearchPathCacheContext = AllocSetContextCreate(TopMemoryContext,
+ "search_path processing cache",
+ ALLOCSET_DEFAULT_SIZES);
+
if (IsBootstrapProcessingMode())
{
/*
@@ -4739,11 +4744,6 @@ InitializeSearchPath(void)
}
else
{
- /* Make the context we'll keep search path cache hashtable in */
- SearchPathCacheContext = AllocSetContextCreate(TopMemoryContext,
- "search_path processing cache",
- ALLOCSET_DEFAULT_SIZES);
-
/*
* In normal mode, arrange for a callback on any syscache invalidation
* of pg_namespace or pg_authid rows. (Changing a role name may affect
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index a03495d6c9..70ce834c08 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -349,6 +349,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
SetUserIdAndSecContext(onerel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/* measure elapsed time iff autovacuum logging requires it */
if (IsAutoVacuumWorkerProcess() && params->log_min_duration >= 0)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index e2c48ec560..c2666bc386 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -353,6 +353,8 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
SetUserIdAndSecContext(OldHeap->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/*
* Since we may open a new transaction for each relation, we have to check
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 7a87626f5f..562da8653a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -584,6 +584,8 @@ DefineIndex(Oid tableId,
int root_save_nestlevel;
root_save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/*
* Some callers need us to run with an empty default_tablespace; this is a
@@ -1340,6 +1342,8 @@ DefineIndex(Oid tableId,
SetUserIdAndSecContext(childrel->rd_rel->relowner,
child_save_sec_context | SECURITY_RESTRICTED_OPERATION);
child_save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/*
* Don't try to create indexes on foreign tables, though. Skip
@@ -3881,6 +3885,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/* determine safety of this index for set_indexsafe_procflags */
idx->safe = (indexRel->rd_indexprs == NIL &&
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 59920ced83..c9046ac5a9 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -178,6 +178,8 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
SetUserIdAndSecContext(relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/* Make sure it is a materialized view. */
if (matviewRel->rd_rel->relkind != RELKIND_MATVIEW)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 64da848627..32a1a80954 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2168,6 +2168,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
SetUserIdAndSecContext(rel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/*
* If PROCESS_MAIN is set (the default), it's time to vacuum the main
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index 4ac021534b..0601fde205 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -109,7 +109,6 @@ $node->safe_psql(
CREATE FUNCTION f1(int) RETURNS int LANGUAGE SQL AS 'SELECT f0($1)';
CREATE TABLE funcidx (x int);
INSERT INTO funcidx VALUES (0),(1),(2),(3);
- CREATE INDEX i0 ON funcidx ((f1(x)));
CREATE SCHEMA "Foo";
CREATE TABLE "Foo".bar(id int);
CREATE SCHEMA "Bar";
@@ -117,9 +116,6 @@ $node->safe_psql(
|);
$node->command_ok([qw|vacuumdb -Z --table="need""q(uot"(")x") postgres|],
'column list');
-$node->command_fails(
- [qw|vacuumdb -Zt funcidx postgres|],
- 'unqualified name via functional index');
$node->command_fails(
[ 'vacuumdb', '--analyze', '--table', 'vactable(c)', 'postgres' ],
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 471d53da8f..391d8d0212 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -203,6 +203,12 @@ typedef enum
#define GUC_QUALIFIER_SEPARATOR '.'
+/*
+ * Safe search path when executing code as the table owner, such as during
+ * maintenance operations.
+ */
+#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"
+
/*
* Bit values in "flags" of a GUC variable. Note that these don't appear
* on disk, so we can reassign their values freely.
diff --git a/src/test/modules/test_oat_hooks/expected/alter_table.out b/src/test/modules/test_oat_hooks/expected/alter_table.out
index 19adb28ffb..8cbacca2c9 100644
--- a/src/test/modules/test_oat_hooks/expected/alter_table.out
+++ b/src/test/modules/test_oat_hooks/expected/alter_table.out
@@ -62,6 +62,8 @@ BEGIN
END IF;
END; $$;
NOTICE: in process utility: superuser attempting CREATE FUNCTION
+NOTICE: in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed]
+NOTICE: in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed]
NOTICE: in object access: superuser attempting create (subId=0x0) [explicit]
NOTICE: in object access: superuser finished create (subId=0x0) [explicit]
NOTICE: in process utility: superuser finished CREATE FUNCTION
diff --git a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
index f80373aecc..effdc49145 100644
--- a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
+++ b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
@@ -89,11 +89,15 @@ NOTICE: in object access: superuser finished create (subId=0x0) [internal]
NOTICE: in process utility: superuser finished CREATE TABLE
CREATE INDEX regress_test_table_t_idx ON regress_test_table (t);
NOTICE: in process utility: superuser attempting CREATE INDEX
+NOTICE: in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed]
+NOTICE: in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed]
NOTICE: in object access: superuser attempting create (subId=0x0) [explicit]
NOTICE: in object access: superuser finished create (subId=0x0) [explicit]
NOTICE: in process utility: superuser finished CREATE INDEX
GRANT SELECT ON Table regress_test_table TO public;
NOTICE: in process utility: superuser attempting GRANT
+NOTICE: in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed]
+NOTICE: in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed]
NOTICE: in process utility: superuser finished GRANT
CREATE FUNCTION regress_test_func (t text) RETURNS text AS $$
SELECT $1;
diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expected/matview.out
index 67a50bde3d..038ab73517 100644
--- a/src/test/regress/expected/matview.out
+++ b/src/test/regress/expected/matview.out
@@ -574,10 +574,11 @@ DROP OWNED BY regress_user_mvtest CASCADE;
DROP ROLE regress_user_mvtest;
-- Concurrent refresh requires a unique index on the materialized
-- view. Test what happens if it's dropped during the refresh.
+SET search_path = mvtest_mvschema, public;
CREATE OR REPLACE FUNCTION mvtest_drop_the_index()
RETURNS bool AS $$
BEGIN
- EXECUTE 'DROP INDEX IF EXISTS mvtest_drop_idx';
+ EXECUTE 'DROP INDEX IF EXISTS mvtest_mvschema.mvtest_drop_idx';
RETURN true;
END;
$$ LANGUAGE plpgsql;
@@ -588,6 +589,7 @@ CREATE UNIQUE INDEX mvtest_drop_idx ON drop_idx_matview (i);
REFRESH MATERIALIZED VIEW CONCURRENTLY drop_idx_matview;
ERROR: could not find suitable unique index on materialized view
DROP MATERIALIZED VIEW drop_idx_matview; -- clean up
+RESET search_path;
-- make sure that create WITH NO DATA works via SPI
BEGIN;
CREATE FUNCTION mvtest_func()
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index fbb0489a4f..5ae5757bde 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1769,7 +1769,7 @@ SET SESSION AUTHORIZATION regress_sro_user;
CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS
'GRANT regress_priv_group2 TO regress_sro_user';
CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
- 'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true';
+ 'DECLARE c CURSOR WITH HOLD FOR SELECT public.unwanted_grant(); SELECT true';
-- REFRESH of this MV will queue a GRANT at end of transaction
CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA;
REFRESH MATERIALIZED VIEW sro_mv;
@@ -1783,12 +1783,12 @@ SET SESSION AUTHORIZATION regress_sro_user;
-- INSERT to this table will queue a GRANT at end of transaction
CREATE TABLE sro_trojan_table ();
CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS
- 'BEGIN PERFORM unwanted_grant(); RETURN NULL; END';
+ 'BEGIN PERFORM public.unwanted_grant(); RETURN NULL; END';
CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table
INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan();
-- Now, REFRESH will issue such an INSERT, queueing the GRANT
CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
- 'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true';
+ 'INSERT INTO public.sro_trojan_table DEFAULT VALUES; SELECT true';
REFRESH MATERIALIZED VIEW sro_mv;
ERROR: cannot fire deferred trigger within security-restricted operation
CONTEXT: SQL function "mv_action" statement 1
@@ -1800,15 +1800,15 @@ BEGIN; SET CONSTRAINTS ALL IMMEDIATE; REFRESH MATERIALIZED VIEW sro_mv; COMMIT;
ERROR: permission denied to grant role "regress_priv_group2"
DETAIL: Only roles with the ADMIN option on role "regress_priv_group2" may grant this role.
CONTEXT: SQL function "unwanted_grant" statement 1
-SQL statement "SELECT unwanted_grant()"
-PL/pgSQL function sro_trojan() line 1 at PERFORM
+SQL statement "SELECT public.unwanted_grant()"
+PL/pgSQL function public.sro_trojan() line 1 at PERFORM
SQL function "mv_action" statement 1
-- REFRESH MATERIALIZED VIEW CONCURRENTLY use of eval_const_expressions()
SET SESSION AUTHORIZATION regress_sro_user;
CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int
IMMUTABLE LANGUAGE plpgsql AS $$
BEGIN
- PERFORM unwanted_grant();
+ PERFORM public.unwanted_grant();
RAISE WARNING 'owned';
RETURN 1;
EXCEPTION WHEN OTHERS THEN
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 4def90b805..330fcd884c 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -64,7 +64,7 @@ CLUSTER vaccluster;
CREATE FUNCTION do_analyze() RETURNS VOID VOLATILE LANGUAGE SQL
AS 'ANALYZE pg_am';
CREATE FUNCTION wrap_do_analyze(c INT) RETURNS INT IMMUTABLE LANGUAGE SQL
- AS 'SELECT $1 FROM do_analyze()';
+ AS 'SELECT $1 FROM public.do_analyze()';
CREATE INDEX ON vaccluster(wrap_do_analyze(i));
INSERT INTO vaccluster VALUES (1), (2);
ANALYZE vaccluster;
diff --git a/src/test/regress/sql/matview.sql b/src/test/regress/sql/matview.sql
index 235123de1e..b74ee305e0 100644
--- a/src/test/regress/sql/matview.sql
+++ b/src/test/regress/sql/matview.sql
@@ -233,10 +233,11 @@ DROP ROLE regress_user_mvtest;
-- Concurrent refresh requires a unique index on the materialized
-- view. Test what happens if it's dropped during the refresh.
+SET search_path = mvtest_mvschema, public;
CREATE OR REPLACE FUNCTION mvtest_drop_the_index()
RETURNS bool AS $$
BEGIN
- EXECUTE 'DROP INDEX IF EXISTS mvtest_drop_idx';
+ EXECUTE 'DROP INDEX IF EXISTS mvtest_mvschema.mvtest_drop_idx';
RETURN true;
END;
$$ LANGUAGE plpgsql;
@@ -247,6 +248,7 @@ CREATE MATERIALIZED VIEW drop_idx_matview AS
CREATE UNIQUE INDEX mvtest_drop_idx ON drop_idx_matview (i);
REFRESH MATERIALIZED VIEW CONCURRENTLY drop_idx_matview;
DROP MATERIALIZED VIEW drop_idx_matview; -- clean up
+RESET search_path;
-- make sure that create WITH NO DATA works via SPI
BEGIN;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 3f68cafcd1..2ef15a9d8c 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1177,7 +1177,7 @@ SET SESSION AUTHORIZATION regress_sro_user;
CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS
'GRANT regress_priv_group2 TO regress_sro_user';
CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
- 'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true';
+ 'DECLARE c CURSOR WITH HOLD FOR SELECT public.unwanted_grant(); SELECT true';
-- REFRESH of this MV will queue a GRANT at end of transaction
CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA;
REFRESH MATERIALIZED VIEW sro_mv;
@@ -1188,12 +1188,12 @@ SET SESSION AUTHORIZATION regress_sro_user;
-- INSERT to this table will queue a GRANT at end of transaction
CREATE TABLE sro_trojan_table ();
CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS
- 'BEGIN PERFORM unwanted_grant(); RETURN NULL; END';
+ 'BEGIN PERFORM public.unwanted_grant(); RETURN NULL; END';
CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table
INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan();
-- Now, REFRESH will issue such an INSERT, queueing the GRANT
CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
- 'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true';
+ 'INSERT INTO public.sro_trojan_table DEFAULT VALUES; SELECT true';
REFRESH MATERIALIZED VIEW sro_mv;
\c -
REFRESH MATERIALIZED VIEW sro_mv;
@@ -1204,7 +1204,7 @@ SET SESSION AUTHORIZATION regress_sro_user;
CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int
IMMUTABLE LANGUAGE plpgsql AS $$
BEGIN
- PERFORM unwanted_grant();
+ PERFORM public.unwanted_grant();
RAISE WARNING 'owned';
RETURN 1;
EXCEPTION WHEN OTHERS THEN
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 51d7b1fecc..0b63ef8dc6 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -49,7 +49,7 @@ CLUSTER vaccluster;
CREATE FUNCTION do_analyze() RETURNS VOID VOLATILE LANGUAGE SQL
AS 'ANALYZE pg_am';
CREATE FUNCTION wrap_do_analyze(c INT) RETURNS INT IMMUTABLE LANGUAGE SQL
- AS 'SELECT $1 FROM do_analyze()';
+ AS 'SELECT $1 FROM public.do_analyze()';
CREATE INDEX ON vaccluster(wrap_do_analyze(i));
INSERT INTO vaccluster VALUES (1), (2);
ANALYZE vaccluster;
--
2.34.1
Re: MAINTAIN privilege -- what do we need to un-revert it?
(Apologies in advance for anything I'm bringing up that we've already
covered somewhere else.)
On Fri, Feb 16, 2024 at 04:03:55PM -0800, Jeff Davis wrote:
Note the changes in amcheck. It's creating functions and calling those
functions from the comparators, and so the comparators need to set the
search_path. I don't think that's terribly common, but does represent a
behavior change and could break something.
Why is this change needed? Is the idea to make amcheck follow the same
rules as maintenance commands to encourage folks to set up index functions
correctly? Or is amcheck similarly affected by search_path tricks?
void InitializeSearchPath(void) { + /* Make the context we'll keep search path cache hashtable in */ + SearchPathCacheContext = AllocSetContextCreate(TopMemoryContext, + "search_path processing cache", + ALLOCSET_DEFAULT_SIZES); + if (IsBootstrapProcessingMode()) { /* @@ -4739,11 +4744,6 @@ InitializeSearchPath(void) } else { - /* Make the context we'll keep search path cache hashtable in */ - SearchPathCacheContext = AllocSetContextCreate(TopMemoryContext, - "search_path processing cache", - ALLOCSET_DEFAULT_SIZES); -
What is the purpose of this change?
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION);
I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new value
for these.
+/* + * Safe search path when executing code as the table owner, such as during + * maintenance operations. + */ +#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"
Is including pg_temp actually safe? I worry that a user could use their
temporary schema to inject objects that would take the place of
non-schema-qualified stuff in functions.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: MAINTAIN privilege -- what do we need to un-revert it?
New version attached.
Do we need a documentation update here? If so, where would be a good
place?
On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote:
Why is this change needed? Is the idea to make amcheck follow the
same
rules as maintenance commands to encourage folks to set up index
functions
correctly?
amcheck is calling functions it defined, so in order to find those
functions it needs to set the right search path.
What is the purpose of this [bootstrap-related] change?
DefineIndex() is called during bootstrap, and it's also a maintenance
command. I tried to handle the bootstrapping case, but I think it's
best to just guard it with a conditional. Done.
I also added Assert(!IsBootstrapProcessingMode()) in
assign_search_path().
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION);I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new
value
for these.
Did you have a particular concern about PGC_S_SESSION?
If it's less than PGC_S_SESSION, it won't work, because the caller's
SET command will override it, and the same manipulation is possible.
And I don't think we want it higher than PGC_S_SESSION, otherwise the
function can't set its own search_path, if needed.
+#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"
Is including pg_temp actually safe? I worry that a user could use
their
temporary schema to inject objects that would take the place of
non-schema-qualified stuff in functions.
pg_temp cannot (currently) be excluded. If it is omitted from the
string, it will be placed *first* in the search_path, which is more
dangerous.
pg_temp does not take part in function or operator resolution, which
makes it safer than it first appears. There are potentially some risks
around tables, but it's not typical to access a table in a function
called as part of an index expression.
If we determine that pg_temp is actually unsafe to include, we need to
do something like what I proposed here:
/messages/by-id/a6865db287596c9c6ea12bdd9de87216cb5e7902.camel@j-davis.com
before this change.
Regards,
Jeff Davis
Attachments:
v5-0001-Fix-search_path-to-a-safe-value-during-maintenanc.patchtext/x-patch; charset=UTF-8; name=v5-0001-Fix-search_path-to-a-safe-value-during-maintenanc.patchDownload
From d628e1b4e2632a7f853e2c1f016758569eaee14e Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Fri, 16 Feb 2024 14:04:23 -0800
Subject: [PATCH v5] Fix search_path to a safe value during maintenance
operations.
While executing maintenance operations (ANALYZE, CLUSTER, REFRESH
MATERIALIZED VIEW, REINDEX, or VACUUM), set search_path to
'pg_catalog, pg_temp' to prevent inconsistent behavior.
Functions that are used for functional indexes, in index expressions,
or in materialized views and depend on a different search path must be
declared with CREATE FUNCTION ... SET search_path='...'.
This change was previously committed as 05e1737351, then reverted in
commit 2fcc7ee7af because it was too late in the cycle.
Preparation for the MAINTAIN privilege, which was previously reverted
due to search_path manipulation hazards.
Discussion: https://postgr.es/m/d4ccaf3658cb3c281ec88c851a09733cd9482f22.camel@j-davis.com
Discussion: https://postgr.es/m/E1q7j7Y-000z1H-Hr%40gemulon.postgresql.org
Discussion: https://postgr.es/m/e44327179e5c9015c8dda67351c04da552066017.camel%40j-davis.com
Reviewed-by: Greg Stark
Reviewed-by: Nathan Bossart
---
contrib/amcheck/t/004_verify_nbtree_unique.pl | 33 +++++++++++--------
contrib/amcheck/verify_nbtree.c | 2 ++
src/backend/access/brin/brin.c | 2 ++
src/backend/catalog/index.c | 9 +++++
src/backend/catalog/namespace.c | 3 ++
src/backend/commands/analyze.c | 2 ++
src/backend/commands/cluster.c | 2 ++
src/backend/commands/indexcmds.c | 8 +++++
src/backend/commands/matview.c | 2 ++
src/backend/commands/vacuum.c | 2 ++
src/bin/scripts/t/100_vacuumdb.pl | 4 ---
src/include/utils/guc.h | 6 ++++
.../test_oat_hooks/expected/alter_table.out | 2 ++
.../expected/test_oat_hooks.out | 4 +++
src/test/regress/expected/matview.out | 4 ++-
src/test/regress/expected/privileges.out | 12 +++----
src/test/regress/expected/vacuum.out | 2 +-
src/test/regress/sql/matview.sql | 4 ++-
src/test/regress/sql/privileges.sql | 8 ++---
src/test/regress/sql/vacuum.sql | 2 +-
20 files changed, 82 insertions(+), 31 deletions(-)
diff --git a/contrib/amcheck/t/004_verify_nbtree_unique.pl b/contrib/amcheck/t/004_verify_nbtree_unique.pl
index 3f474a158a..4b704e6815 100644
--- a/contrib/amcheck/t/004_verify_nbtree_unique.pl
+++ b/contrib/amcheck/t/004_verify_nbtree_unique.pl
@@ -20,8 +20,11 @@ $node->safe_psql(
'postgres', q(
CREATE EXTENSION amcheck;
+ CREATE SCHEMA test_amcheck;
+ SET search_path = test_amcheck;
+
CREATE FUNCTION ok_cmp (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT
CASE WHEN $1 < $2 THEN -1
@@ -34,7 +37,7 @@ $node->safe_psql(
--- Check 1: uniqueness violation.
---
CREATE FUNCTION ok_cmp1 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT ok_cmp($1, $2);
$$;
@@ -43,7 +46,7 @@ $node->safe_psql(
--- Make values 768 and 769 look equal.
---
CREATE FUNCTION bad_cmp1 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT
CASE WHEN ($1 = 768 AND $2 = 769) OR
@@ -56,13 +59,13 @@ $node->safe_psql(
--- Check 2: uniqueness violation without deduplication.
---
CREATE FUNCTION ok_cmp2 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT ok_cmp($1, $2);
$$;
CREATE FUNCTION bad_cmp2 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT
CASE WHEN $1 = $2 AND $1 = 400 THEN -1
@@ -74,13 +77,13 @@ $node->safe_psql(
--- Check 3: uniqueness violation with deduplication.
---
CREATE FUNCTION ok_cmp3 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT ok_cmp($1, $2);
$$;
CREATE FUNCTION bad_cmp3 (int4, int4)
- RETURNS int LANGUAGE sql AS
+ RETURNS int LANGUAGE sql SET search_path = test_amcheck AS
$$
SELECT bad_cmp2($1, $2);
$$;
@@ -142,7 +145,7 @@ my ($result, $stdout, $stderr);
# We have not yet broken the index, so we should get no corruption
$result = $node->safe_psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx1', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx1', true, true);
));
is($result, '', 'run amcheck on non-broken bttest_unique_idx1');
@@ -150,6 +153,7 @@ is($result, '', 'run amcheck on non-broken bttest_unique_idx1');
# values to be equal.
$node->safe_psql(
'postgres', q(
+ SET search_path = test_amcheck;
UPDATE pg_catalog.pg_amproc SET
amproc = 'bad_cmp1'::regproc
WHERE amproc = 'ok_cmp1'::regproc;
@@ -157,7 +161,7 @@ $node->safe_psql(
($result, $stdout, $stderr) = $node->psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx1', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx1', true, true);
));
ok( $stderr =~ /index uniqueness is violated for index "bttest_unique_idx1"/,
'detected uniqueness violation for index "bttest_unique_idx1"');
@@ -175,13 +179,14 @@ ok( $stderr =~ /index uniqueness is violated for index "bttest_unique_idx1"/,
# but no uniqueness violation.
($result, $stdout, $stderr) = $node->psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx2', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx2', true, true);
));
ok( $stderr =~ /item order invariant violated for index "bttest_unique_idx2"/,
'detected item order invariant violation for index "bttest_unique_idx2"');
$node->safe_psql(
'postgres', q(
+ SET search_path = test_amcheck;
UPDATE pg_catalog.pg_amproc SET
amproc = 'ok_cmp2'::regproc
WHERE amproc = 'bad_cmp2'::regproc;
@@ -189,7 +194,7 @@ $node->safe_psql(
($result, $stdout, $stderr) = $node->psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx2', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx2', true, true);
));
ok( $stderr =~ /index uniqueness is violated for index "bttest_unique_idx2"/,
'detected uniqueness violation for index "bttest_unique_idx2"');
@@ -206,7 +211,7 @@ ok( $stderr =~ /index uniqueness is violated for index "bttest_unique_idx2"/,
# but no uniqueness violation.
($result, $stdout, $stderr) = $node->psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx3', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx3', true, true);
));
ok( $stderr =~ /item order invariant violated for index "bttest_unique_idx3"/,
'detected item order invariant violation for index "bttest_unique_idx3"');
@@ -215,6 +220,7 @@ ok( $stderr =~ /item order invariant violated for index "bttest_unique_idx3"/,
# with different visibility.
$node->safe_psql(
'postgres', q(
+ SET search_path = test_amcheck;
DELETE FROM bttest_unique3 WHERE 380 <= i AND i <= 420;
INSERT INTO bttest_unique3 (SELECT * FROM generate_series(380, 420));
INSERT INTO bttest_unique3 VALUES (400);
@@ -228,6 +234,7 @@ $node->safe_psql(
$node->safe_psql(
'postgres', q(
+ SET search_path = test_amcheck;
UPDATE pg_catalog.pg_amproc SET
amproc = 'ok_cmp3'::regproc
WHERE amproc = 'bad_cmp3'::regproc;
@@ -235,7 +242,7 @@ $node->safe_psql(
($result, $stdout, $stderr) = $node->psql(
'postgres', q(
- SELECT bt_index_check('bttest_unique_idx3', true, true);
+ SELECT bt_index_check('test_amcheck.bttest_unique_idx3', true, true);
));
ok( $stderr =~ /index uniqueness is violated for index "bttest_unique_idx3"/,
'detected uniqueness violation for index "bttest_unique_idx3"');
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 91caa53dd8..bff8c61262 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -313,6 +313,8 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
SetUserIdAndSecContext(heaprel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
}
else
{
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 1087a9011e..d1854c2a1c 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1412,6 +1412,8 @@ brin_summarize_range(PG_FUNCTION_ARGS)
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
}
else
{
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 4b88a9cb87..53278ec89b 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1469,6 +1469,8 @@ index_concurrently_build(Oid heapRelationId,
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
indexRelation = index_open(indexRelationId, RowExclusiveLock);
@@ -3021,6 +3023,9 @@ index_build(Relation heapRelation,
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ if (!IsBootstrapProcessingMode())
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/* Set up initial progress report status */
{
@@ -3356,6 +3361,8 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
indexRelation = index_open(indexId, RowExclusiveLock);
@@ -3617,6 +3624,8 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
if (progress)
{
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 8df30b2440..a68fdd844b 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -4697,6 +4697,9 @@ check_search_path(char **newval, void **extra, GucSource source)
void
assign_search_path(const char *newval, void *extra)
{
+ /* don't access search_path during bootstrap */
+ Assert(!IsBootstrapProcessingMode());
+
/*
* We mark the path as needing recomputation, but don't do anything until
* it's needed. This avoids trying to do database access during GUC
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index a03495d6c9..70ce834c08 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -349,6 +349,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
SetUserIdAndSecContext(onerel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/* measure elapsed time iff autovacuum logging requires it */
if (IsAutoVacuumWorkerProcess() && params->log_min_duration >= 0)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index e2c48ec560..c2666bc386 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -353,6 +353,8 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
SetUserIdAndSecContext(OldHeap->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/*
* Since we may open a new transaction for each relation, we have to check
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 7a87626f5f..96eb7d346a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -585,6 +585,10 @@ DefineIndex(Oid tableId,
root_save_nestlevel = NewGUCNestLevel();
+ if (!IsBootstrapProcessingMode())
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
+
/*
* Some callers need us to run with an empty default_tablespace; this is a
* necessary hack to be able to reproduce catalog state accurately when
@@ -1340,6 +1344,8 @@ DefineIndex(Oid tableId,
SetUserIdAndSecContext(childrel->rd_rel->relowner,
child_save_sec_context | SECURITY_RESTRICTED_OPERATION);
child_save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/*
* Don't try to create indexes on foreign tables, though. Skip
@@ -3881,6 +3887,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/* determine safety of this index for set_indexsafe_procflags */
idx->safe = (indexRel->rd_indexprs == NIL &&
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 59920ced83..c9046ac5a9 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -178,6 +178,8 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
SetUserIdAndSecContext(relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/* Make sure it is a materialized view. */
if (matviewRel->rd_rel->relkind != RELKIND_MATVIEW)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 64da848627..32a1a80954 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2168,6 +2168,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
SetUserIdAndSecContext(rel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+ SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+ PGC_S_SESSION);
/*
* If PROCESS_MAIN is set (the default), it's time to vacuum the main
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index 4ac021534b..0601fde205 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -109,7 +109,6 @@ $node->safe_psql(
CREATE FUNCTION f1(int) RETURNS int LANGUAGE SQL AS 'SELECT f0($1)';
CREATE TABLE funcidx (x int);
INSERT INTO funcidx VALUES (0),(1),(2),(3);
- CREATE INDEX i0 ON funcidx ((f1(x)));
CREATE SCHEMA "Foo";
CREATE TABLE "Foo".bar(id int);
CREATE SCHEMA "Bar";
@@ -117,9 +116,6 @@ $node->safe_psql(
|);
$node->command_ok([qw|vacuumdb -Z --table="need""q(uot"(")x") postgres|],
'column list');
-$node->command_fails(
- [qw|vacuumdb -Zt funcidx postgres|],
- 'unqualified name via functional index');
$node->command_fails(
[ 'vacuumdb', '--analyze', '--table', 'vactable(c)', 'postgres' ],
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 471d53da8f..391d8d0212 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -203,6 +203,12 @@ typedef enum
#define GUC_QUALIFIER_SEPARATOR '.'
+/*
+ * Safe search path when executing code as the table owner, such as during
+ * maintenance operations.
+ */
+#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"
+
/*
* Bit values in "flags" of a GUC variable. Note that these don't appear
* on disk, so we can reassign their values freely.
diff --git a/src/test/modules/test_oat_hooks/expected/alter_table.out b/src/test/modules/test_oat_hooks/expected/alter_table.out
index 19adb28ffb..8cbacca2c9 100644
--- a/src/test/modules/test_oat_hooks/expected/alter_table.out
+++ b/src/test/modules/test_oat_hooks/expected/alter_table.out
@@ -62,6 +62,8 @@ BEGIN
END IF;
END; $$;
NOTICE: in process utility: superuser attempting CREATE FUNCTION
+NOTICE: in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed]
+NOTICE: in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed]
NOTICE: in object access: superuser attempting create (subId=0x0) [explicit]
NOTICE: in object access: superuser finished create (subId=0x0) [explicit]
NOTICE: in process utility: superuser finished CREATE FUNCTION
diff --git a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
index f80373aecc..effdc49145 100644
--- a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
+++ b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
@@ -89,11 +89,15 @@ NOTICE: in object access: superuser finished create (subId=0x0) [internal]
NOTICE: in process utility: superuser finished CREATE TABLE
CREATE INDEX regress_test_table_t_idx ON regress_test_table (t);
NOTICE: in process utility: superuser attempting CREATE INDEX
+NOTICE: in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed]
+NOTICE: in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed]
NOTICE: in object access: superuser attempting create (subId=0x0) [explicit]
NOTICE: in object access: superuser finished create (subId=0x0) [explicit]
NOTICE: in process utility: superuser finished CREATE INDEX
GRANT SELECT ON Table regress_test_table TO public;
NOTICE: in process utility: superuser attempting GRANT
+NOTICE: in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed]
+NOTICE: in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed]
NOTICE: in process utility: superuser finished GRANT
CREATE FUNCTION regress_test_func (t text) RETURNS text AS $$
SELECT $1;
diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expected/matview.out
index 67a50bde3d..038ab73517 100644
--- a/src/test/regress/expected/matview.out
+++ b/src/test/regress/expected/matview.out
@@ -574,10 +574,11 @@ DROP OWNED BY regress_user_mvtest CASCADE;
DROP ROLE regress_user_mvtest;
-- Concurrent refresh requires a unique index on the materialized
-- view. Test what happens if it's dropped during the refresh.
+SET search_path = mvtest_mvschema, public;
CREATE OR REPLACE FUNCTION mvtest_drop_the_index()
RETURNS bool AS $$
BEGIN
- EXECUTE 'DROP INDEX IF EXISTS mvtest_drop_idx';
+ EXECUTE 'DROP INDEX IF EXISTS mvtest_mvschema.mvtest_drop_idx';
RETURN true;
END;
$$ LANGUAGE plpgsql;
@@ -588,6 +589,7 @@ CREATE UNIQUE INDEX mvtest_drop_idx ON drop_idx_matview (i);
REFRESH MATERIALIZED VIEW CONCURRENTLY drop_idx_matview;
ERROR: could not find suitable unique index on materialized view
DROP MATERIALIZED VIEW drop_idx_matview; -- clean up
+RESET search_path;
-- make sure that create WITH NO DATA works via SPI
BEGIN;
CREATE FUNCTION mvtest_func()
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index fbb0489a4f..5ae5757bde 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1769,7 +1769,7 @@ SET SESSION AUTHORIZATION regress_sro_user;
CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS
'GRANT regress_priv_group2 TO regress_sro_user';
CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
- 'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true';
+ 'DECLARE c CURSOR WITH HOLD FOR SELECT public.unwanted_grant(); SELECT true';
-- REFRESH of this MV will queue a GRANT at end of transaction
CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA;
REFRESH MATERIALIZED VIEW sro_mv;
@@ -1783,12 +1783,12 @@ SET SESSION AUTHORIZATION regress_sro_user;
-- INSERT to this table will queue a GRANT at end of transaction
CREATE TABLE sro_trojan_table ();
CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS
- 'BEGIN PERFORM unwanted_grant(); RETURN NULL; END';
+ 'BEGIN PERFORM public.unwanted_grant(); RETURN NULL; END';
CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table
INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan();
-- Now, REFRESH will issue such an INSERT, queueing the GRANT
CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
- 'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true';
+ 'INSERT INTO public.sro_trojan_table DEFAULT VALUES; SELECT true';
REFRESH MATERIALIZED VIEW sro_mv;
ERROR: cannot fire deferred trigger within security-restricted operation
CONTEXT: SQL function "mv_action" statement 1
@@ -1800,15 +1800,15 @@ BEGIN; SET CONSTRAINTS ALL IMMEDIATE; REFRESH MATERIALIZED VIEW sro_mv; COMMIT;
ERROR: permission denied to grant role "regress_priv_group2"
DETAIL: Only roles with the ADMIN option on role "regress_priv_group2" may grant this role.
CONTEXT: SQL function "unwanted_grant" statement 1
-SQL statement "SELECT unwanted_grant()"
-PL/pgSQL function sro_trojan() line 1 at PERFORM
+SQL statement "SELECT public.unwanted_grant()"
+PL/pgSQL function public.sro_trojan() line 1 at PERFORM
SQL function "mv_action" statement 1
-- REFRESH MATERIALIZED VIEW CONCURRENTLY use of eval_const_expressions()
SET SESSION AUTHORIZATION regress_sro_user;
CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int
IMMUTABLE LANGUAGE plpgsql AS $$
BEGIN
- PERFORM unwanted_grant();
+ PERFORM public.unwanted_grant();
RAISE WARNING 'owned';
RETURN 1;
EXCEPTION WHEN OTHERS THEN
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 4def90b805..330fcd884c 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -64,7 +64,7 @@ CLUSTER vaccluster;
CREATE FUNCTION do_analyze() RETURNS VOID VOLATILE LANGUAGE SQL
AS 'ANALYZE pg_am';
CREATE FUNCTION wrap_do_analyze(c INT) RETURNS INT IMMUTABLE LANGUAGE SQL
- AS 'SELECT $1 FROM do_analyze()';
+ AS 'SELECT $1 FROM public.do_analyze()';
CREATE INDEX ON vaccluster(wrap_do_analyze(i));
INSERT INTO vaccluster VALUES (1), (2);
ANALYZE vaccluster;
diff --git a/src/test/regress/sql/matview.sql b/src/test/regress/sql/matview.sql
index 235123de1e..b74ee305e0 100644
--- a/src/test/regress/sql/matview.sql
+++ b/src/test/regress/sql/matview.sql
@@ -233,10 +233,11 @@ DROP ROLE regress_user_mvtest;
-- Concurrent refresh requires a unique index on the materialized
-- view. Test what happens if it's dropped during the refresh.
+SET search_path = mvtest_mvschema, public;
CREATE OR REPLACE FUNCTION mvtest_drop_the_index()
RETURNS bool AS $$
BEGIN
- EXECUTE 'DROP INDEX IF EXISTS mvtest_drop_idx';
+ EXECUTE 'DROP INDEX IF EXISTS mvtest_mvschema.mvtest_drop_idx';
RETURN true;
END;
$$ LANGUAGE plpgsql;
@@ -247,6 +248,7 @@ CREATE MATERIALIZED VIEW drop_idx_matview AS
CREATE UNIQUE INDEX mvtest_drop_idx ON drop_idx_matview (i);
REFRESH MATERIALIZED VIEW CONCURRENTLY drop_idx_matview;
DROP MATERIALIZED VIEW drop_idx_matview; -- clean up
+RESET search_path;
-- make sure that create WITH NO DATA works via SPI
BEGIN;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 3f68cafcd1..2ef15a9d8c 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1177,7 +1177,7 @@ SET SESSION AUTHORIZATION regress_sro_user;
CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS
'GRANT regress_priv_group2 TO regress_sro_user';
CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
- 'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true';
+ 'DECLARE c CURSOR WITH HOLD FOR SELECT public.unwanted_grant(); SELECT true';
-- REFRESH of this MV will queue a GRANT at end of transaction
CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA;
REFRESH MATERIALIZED VIEW sro_mv;
@@ -1188,12 +1188,12 @@ SET SESSION AUTHORIZATION regress_sro_user;
-- INSERT to this table will queue a GRANT at end of transaction
CREATE TABLE sro_trojan_table ();
CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS
- 'BEGIN PERFORM unwanted_grant(); RETURN NULL; END';
+ 'BEGIN PERFORM public.unwanted_grant(); RETURN NULL; END';
CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table
INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan();
-- Now, REFRESH will issue such an INSERT, queueing the GRANT
CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
- 'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true';
+ 'INSERT INTO public.sro_trojan_table DEFAULT VALUES; SELECT true';
REFRESH MATERIALIZED VIEW sro_mv;
\c -
REFRESH MATERIALIZED VIEW sro_mv;
@@ -1204,7 +1204,7 @@ SET SESSION AUTHORIZATION regress_sro_user;
CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int
IMMUTABLE LANGUAGE plpgsql AS $$
BEGIN
- PERFORM unwanted_grant();
+ PERFORM public.unwanted_grant();
RAISE WARNING 'owned';
RETURN 1;
EXCEPTION WHEN OTHERS THEN
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 51d7b1fecc..0b63ef8dc6 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -49,7 +49,7 @@ CLUSTER vaccluster;
CREATE FUNCTION do_analyze() RETURNS VOID VOLATILE LANGUAGE SQL
AS 'ANALYZE pg_am';
CREATE FUNCTION wrap_do_analyze(c INT) RETURNS INT IMMUTABLE LANGUAGE SQL
- AS 'SELECT $1 FROM do_analyze()';
+ AS 'SELECT $1 FROM public.do_analyze()';
CREATE INDEX ON vaccluster(wrap_do_analyze(i));
INSERT INTO vaccluster VALUES (1), (2);
ANALYZE vaccluster;
--
2.34.1
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Tue, Feb 27, 2024 at 04:22:34PM -0800, Jeff Davis wrote:
Do we need a documentation update here? If so, where would be a good
place?
I'm afraid I don't have a better idea than adding a short note in each
affected commands's page.
On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote:
I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new
value
for these.Did you have a particular concern about PGC_S_SESSION?
My only concern is that it could obscure the source of the search_path
change, which in turn might cause confusion when things fail.
If it's less than PGC_S_SESSION, it won't work, because the caller's
SET command will override it, and the same manipulation is possible.And I don't think we want it higher than PGC_S_SESSION, otherwise the
function can't set its own search_path, if needed.
Yeah, we would have to make it equivalent in priority to PGC_S_SESSION,
which would likely require a bunch of special logic. I don't know if this
is worth it, and this seems like something that could pretty easily be
added in the future if it became necessary.
+#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"
Is including pg_temp actually safe?� I worry that a user could use
their
temporary schema to inject objects that would take the place of
non-schema-qualified stuff in functions.pg_temp cannot (currently) be excluded. If it is omitted from the
string, it will be placed *first* in the search_path, which is more
dangerous.pg_temp does not take part in function or operator resolution, which
makes it safer than it first appears. There are potentially some risks
around tables, but it's not typical to access a table in a function
called as part of an index expression.If we determine that pg_temp is actually unsafe to include, we need to
do something like what I proposed here:/messages/by-id/a6865db287596c9c6ea12bdd9de87216cb5e7902.camel@j-davis.com
I don't doubt anything you've said, but I can't help but think that we
might as well handle the pg_temp risk, too.
Furthermore, I see that we use "" as a safe search_path for autovacuum and
fe_utils/connect.h. Is there any way to unite these? IIUC it might be
possible to combine the autovacuum and maintenance command cases (i.e.,
"!pg_temp"), but we might need to keep pg_temp for the frontend case. I
think it's worth trying to add comments about why this setting is safe for
some cases but not others, too.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Wed, 2024-02-28 at 10:55 -0600, Nathan Bossart wrote:
I'm afraid I don't have a better idea than adding a short note in
each
affected commands's page.
OK, that works for now.
Later we should also document that the functions are run as the table
owner.
On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote:
I wonder if it's worth using PGC_S_INTERACTIVE or introducing a
new
value
for these.Did you have a particular concern about PGC_S_SESSION?
My only concern is that it could obscure the source of the
search_path
change, which in turn might cause confusion when things fail.
That's a good point. AutoVacWorkerMain uses PGC_S_OVERRIDE, but it
doesn't have to worry about SET, because there's no real session.
The function SET clause uses PGC_S_SESSION. It's arguable whether
that's really the same source as a SET command, but it's definitely
closer.
Yeah, we would have to make it equivalent in priority to
PGC_S_SESSION,
which would likely require a bunch of special logic.
I'm not clear on what problem that would solve.
I don't doubt anything you've said, but I can't help but think that
we
might as well handle the pg_temp risk, too.
That sounds good to me, but I didn't get many replies in that last
thread. And although it solves the problem, it is a bit awkward.
Can we get some closure on whether that !pg_temp patch is the right
approach? That was just my first idea, and it would be good to hear
what others think.
Furthermore, I see that we use "" as a safe search_path for
autovacuum and
fe_utils/connect.h. Is there any way to unite these?
We could have a single function like RestrictSearchPath(), which I
believe I had in some previous iteration. That would use the safest
search path (either excluding pg_temp or putting it at the end) and
PGC_S_SESSION, and then use it everywhere.
Regards,
Jeff Davis
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Wed, 2024-02-28 at 09:29 -0800, Jeff Davis wrote:
On Wed, 2024-02-28 at 10:55 -0600, Nathan Bossart wrote:
I'm afraid I don't have a better idea than adding a short note in
each
affected commands's page.OK, that works for now.
Committed.
The only changes are documentation and test updates.
This is a behavior change, so it still carries some risk, though we've
had a lot of discussion and generally it seems to be worth it. If it
turns out worse than expected during beta, of course we can re-revert
it.
I will restate the risks here, which come basically from two places:
(1) Functions called from index expressions which rely on search_path
(and don't have a SET clause).
Such a function would have already been fairly broken before my commit,
because anyone accessing the table without the right search_path would
have seen an error or wrong results. And there is no means to set the
"right" search_path for autoanalyze or logical replication, so those
would not have worked with such a broken function before my commit, no
matter what.
That being said, surely some users did have such broken functions, and
with this commit, they will have to remedy them with a SET clause.
Fortunately, the performance impact of doing so has been greatly
reduced.
(2) Matierialized views which call functions that rely on search_path
(and don't have a SET clause).
This is arguably a worse kind of breakage because materialized views
are often refreshed only by the table owner, and it's easier to control
search_path when running REFRESH. Additionally, functions called from
materialized views are more likely to be "interesting" than functions
called from an index expression. However, the remedy is
straightforward: use a SET clause.
Regards,
Jeff Davis
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Mon, Mar 04, 2024 at 07:52:05PM -0800, Jeff Davis wrote:
Committed.
Commit 2af07e2 wrote:
--- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -1412,6 +1412,8 @@ brin_summarize_range(PG_FUNCTION_ARGS) SetUserIdAndSecContext(heapRel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION);
I've audited NewGUCNestLevel() calls that didn't get this addition. Among
those, these need the addition:
- Each in ComputeIndexAttrs() -- they arise when the caller is DefineIndex()
- In DefineIndex(), after comment "changed a behavior-affecting GUC"
While "not necessary for security", ExecCreateTableAs() should do it for the
same reason it calls NewGUCNestLevel().
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Sun, Jun 30, 2024 at 03:23:44PM -0700, Noah Misch wrote:
I've audited NewGUCNestLevel() calls that didn't get this addition. Among
those, these need the addition:- Each in ComputeIndexAttrs() -- they arise when the caller is DefineIndex()
- In DefineIndex(), after comment "changed a behavior-affecting GUC"
Hmm. Is RestrictSearchPath() something that we should advertise more
strongly, thinking here about extensions that call NewGUCNestLevel()?
That would be really easy to miss, and it could have bad consequences.
I know that this is not something that's published in the release
notes, but it looks like something sensible to have, though.
While "not necessary for security", ExecCreateTableAs() should do it for the
same reason it calls NewGUCNestLevel().
+1.
--
MichaelRe: MAINTAIN privilege -- what do we need to un-revert it?
On Tue, 2024-07-09 at 15:20 +0900, Michael Paquier wrote:
On Sun, Jun 30, 2024 at 03:23:44PM -0700, Noah Misch wrote:
I've audited NewGUCNestLevel() calls that didn't get this
addition. Among
those, these need the addition:- Each in ComputeIndexAttrs() -- they arise when the caller is
DefineIndex()
- In DefineIndex(), after comment "changed a behavior-affecting
GUC"
Thank you for the report. Patch attached to address these missing call
sites.
Hmm. Is RestrictSearchPath() something that we should advertise more
strongly, thinking here about extensions that call NewGUCNestLevel()?
That would be really easy to miss, and it could have bad
consequences.
I know that this is not something that's published in the release
notes, but it looks like something sensible to have, though.
The pattern also involves SetUserIdAndSecContext(). Perhaps we could
come up with a wrapper function to better encapsulate the general
pattern?
While "not necessary for security", ExecCreateTableAs() should do
it for the
same reason it calls NewGUCNestLevel().+1.
Do you have a suggestion about how that should be done?
It's not trivial, because the both creates the table and populates it
in ExecutorRun. For table creation, we need to use the original
search_path, but we need to use the restricted search_path when
populating it.
I could try to refactor it into two statements and execute them
separately, or I could try to rewrite the statement to use a fully-
qualified destination table before execution. Thoughts?
Regards,
Jeff Davis
Attachments:
v1-0001-Add-missing-RestrictSearchPath-calls.patchtext/x-patch; charset=UTF-8; name=v1-0001-Add-missing-RestrictSearchPath-calls.patchDownload
From 58331d25defa861a087e11755da987bce932ed1f Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Tue, 9 Jul 2024 11:12:46 -0700
Subject: [PATCH v1] Add missing RestrictSearchPath() calls.
Reported-by: Noah Misch
Backpatch-through: 17
Discussion: https://postgr.es/m/20240630222344.db.nmisch@google.com
---
src/backend/commands/indexcmds.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 2caab88aa58..c5a56c75f69 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1230,6 +1230,7 @@ DefineIndex(Oid tableId,
*/
AtEOXact_GUC(false, root_save_nestlevel);
root_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
/* Add any requested comment */
if (stmt->idxcomment != NULL)
@@ -2027,6 +2028,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
}
@@ -2074,6 +2076,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
/*
@@ -2104,6 +2107,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
/*
--
2.34.1
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Tue, Jul 09, 2024 at 05:47:36PM -0700, Jeff Davis wrote:
On Tue, 2024-07-09 at 15:20 +0900, Michael Paquier wrote:
On Sun, Jun 30, 2024 at 03:23:44PM -0700, Noah Misch wrote:
Hmm.� Is RestrictSearchPath() something that we should advertise more
strongly, thinking here about extensions that call NewGUCNestLevel()?
That would be really easy to miss, and it could have bad
consequences.
I know that this is not something that's published in the release
notes, but it looks like something sensible to have, though.The pattern also involves SetUserIdAndSecContext(). Perhaps we could
come up with a wrapper function to better encapsulate the general
pattern?
Worth a look. usercontext.c has an existing wrapper for a superuser process
switching to an untrusted user. It could become the home for another wrapper
targeting MAINTAIN-relevant callers.
While "not necessary for security", ExecCreateTableAs() should do
it for the
same reason it calls NewGUCNestLevel().+1.
Do you have a suggestion about how that should be done?
It's not trivial, because the both creates the table and populates it
in ExecutorRun. For table creation, we need to use the original
search_path, but we need to use the restricted search_path when
populating it.I could try to refactor it into two statements and execute them
separately, or I could try to rewrite the statement to use a fully-
qualified destination table before execution. Thoughts?
Those sound fine. Also fine: just adding a comment on why creation namespace
considerations led to not doing it there.
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Thu, 2024-07-11 at 05:52 -0700, Noah Misch wrote:
I could try to refactor it into two statements and execute them
separately, or I could try to rewrite the statement to use a fully-
qualified destination table before execution. Thoughts?Those sound fine. Also fine: just adding a comment on why creation
namespace
considerations led to not doing it there.
Attached. 0002 separates the CREATE MATERIALIZED VIEW ... WITH DATA
into (effectively):
CREATE MATERIALIZED VIEW ... WITH NO DATA;
REFRESH MATERIALIZED VIEW ...;
Using refresh also achieves the stated goal more directly: to (mostly)
ensure that a subsequent REFRESH will succeed.
Note: the creation itself no longer executes in a security-restricted
context, but I don't think that's a problem. The only reason it's using
the security restricted context is so the following REFRESH will
succeed, right?
Regards,
Jeff Davis
Attachments:
v2-0001-Add-missing-RestrictSearchPath-calls.patchtext/x-patch; charset=UTF-8; name=v2-0001-Add-missing-RestrictSearchPath-calls.patchDownload
From cf1722bf0716897f42ce89282f361af4be56b60b Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Tue, 9 Jul 2024 11:12:46 -0700
Subject: [PATCH v2 1/2] Add missing RestrictSearchPath() calls.
Reported-by: Noah Misch
Backpatch-through: 17
Discussion: https://postgr.es/m/20240630222344.db.nmisch@google.com
---
src/backend/commands/indexcmds.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 2caab88aa58..c5a56c75f69 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1230,6 +1230,7 @@ DefineIndex(Oid tableId,
*/
AtEOXact_GUC(false, root_save_nestlevel);
root_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
/* Add any requested comment */
if (stmt->idxcomment != NULL)
@@ -2027,6 +2028,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
}
@@ -2074,6 +2076,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
/*
@@ -2104,6 +2107,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
/*
--
2.34.1
v2-0002-For-materialized-views-use-REFRESH-to-load-data-d.patchtext/x-patch; charset=UTF-8; name=v2-0002-For-materialized-views-use-REFRESH-to-load-data-d.patchDownload
From 00323c08d07f1fb53e29e89e44b9393d442c15b1 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Fri, 12 Jul 2024 14:23:17 -0700
Subject: [PATCH v2 2/2] For materialized views, use REFRESH to load data
during creation.
Previously, CREATE MATERIALIZED VIEW ... WITH DATA populated the MV
during creation. Instead, use REFRESH, which locks down
security-restricted operations and restricts the
search_path. Otherwise, it's possible to create MVs that cannot be
refreshed.
Reported-by: Noah Misch
Backpatch-through: 17
Discussion: https://postgr.es/m/20240630222344.db.nmisch@google.com
---
src/backend/commands/createas.c | 35 +++++++++++++------------
src/test/regress/expected/namespace.out | 4 +--
2 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 62050f4dc59..166ade5faa7 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -225,10 +225,8 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
Query *query = castNode(Query, stmt->query);
IntoClause *into = stmt->into;
bool is_matview = (into->viewQuery != NULL);
+ bool do_refresh = false;
DestReceiver *dest;
- Oid save_userid = InvalidOid;
- int save_sec_context = 0;
- int save_nestlevel = 0;
ObjectAddress address;
List *rewritten;
PlannedStmt *plan;
@@ -263,18 +261,13 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
Assert(query->commandType == CMD_SELECT);
/*
- * For materialized views, lock down security-restricted operations and
- * arrange to make GUC variable changes local to this command. This is
- * not necessary for security, but this keeps the behavior similar to
- * REFRESH MATERIALIZED VIEW. Otherwise, one could create a materialized
- * view not possible to refresh.
+ * For materialized views, always skip data during table creation, and use
+ * REFRESH instead.
*/
if (is_matview)
{
- GetUserIdAndSecContext(&save_userid, &save_sec_context);
- SetUserIdAndSecContext(save_userid,
- save_sec_context | SECURITY_RESTRICTED_OPERATION);
- save_nestlevel = NewGUCNestLevel();
+ do_refresh = !into->skipData;
+ into->skipData = true;
}
if (into->skipData)
@@ -346,13 +339,21 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
PopActiveSnapshot();
}
- if (is_matview)
+ /*
+ * For materialized views, use REFRESH, which locks down
+ * security-restricted operations and restricts the search_path.
+ * Otherwise, one could create a materialized view not possible to
+ * refresh.
+ */
+ if (do_refresh)
{
- /* Roll back any GUC changes */
- AtEOXact_GUC(false, save_nestlevel);
+ RefreshMatViewStmt *refresh = makeNode(RefreshMatViewStmt);
- /* Restore userid and security context */
- SetUserIdAndSecContext(save_userid, save_sec_context);
+ refresh->relation = into->rel;
+ ExecRefreshMatView(refresh, pstate->p_sourcetext, NULL, qc);
+
+ if (qc)
+ qc->commandTag = CMDTAG_SELECT;
}
return address;
diff --git a/src/test/regress/expected/namespace.out b/src/test/regress/expected/namespace.out
index 7d36e9cc0c4..dbbda72d395 100644
--- a/src/test/regress/expected/namespace.out
+++ b/src/test/regress/expected/namespace.out
@@ -129,8 +129,8 @@ $$;
CREATE TABLE test_maint(i INT);
INSERT INTO test_maint VALUES (1), (2);
CREATE MATERIALIZED VIEW test_maint_mv AS SELECT fn(i) FROM test_maint;
-NOTICE: current search_path: test_maint_search_path
-NOTICE: current search_path: test_maint_search_path
+NOTICE: current search_path: pg_catalog, pg_temp
+NOTICE: current search_path: pg_catalog, pg_temp
-- the following commands should see search_path as pg_catalog, pg_temp
CREATE INDEX test_maint_idx ON test_maint_search_path.test_maint (fn(i));
NOTICE: current search_path: pg_catalog, pg_temp
--
2.34.1
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Fri, Jul 12, 2024 at 02:50:52PM -0700, Jeff Davis wrote:
On Thu, 2024-07-11 at 05:52 -0700, Noah Misch wrote:
I could try to refactor it into two statements and execute them
separately, or I could try to rewrite the statement to use a fully-
qualified destination table before execution. Thoughts?Those sound fine.� Also fine: just adding a comment on why creation
namespace
considerations led to not doing it there.Attached. 0002 separates the CREATE MATERIALIZED VIEW ... WITH DATA
into (effectively):CREATE MATERIALIZED VIEW ... WITH NO DATA;
REFRESH MATERIALIZED VIEW ...;Using refresh also achieves the stated goal more directly: to (mostly)
ensure that a subsequent REFRESH will succeed.Note: the creation itself no longer executes in a security-restricted
context, but I don't think that's a problem. The only reason it's using
the security restricted context is so the following REFRESH will
succeed, right?
Right, that's the only reason.
@@ -346,13 +339,21 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
PopActiveSnapshot();
}- if (is_matview) + /* + * For materialized views, use REFRESH, which locks down + * security-restricted operations and restricts the search_path. + * Otherwise, one could create a materialized view not possible to + * refresh. + */ + if (do_refresh) { - /* Roll back any GUC changes */ - AtEOXact_GUC(false, save_nestlevel); + RefreshMatViewStmt *refresh = makeNode(RefreshMatViewStmt);- /* Restore userid and security context */ - SetUserIdAndSecContext(save_userid, save_sec_context); + refresh->relation = into->rel; + ExecRefreshMatView(refresh, pstate->p_sourcetext, NULL, qc); + + if (qc) + qc->commandTag = CMDTAG_SELECT; }
Since refresh->relation is a RangeVar, this departs from the standard against
repeated name lookups, from CVE-2014-0062 (commit 5f17304).
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Fri, 2024-07-12 at 16:11 -0700, Noah Misch wrote:
Since refresh->relation is a RangeVar, this departs from the standard
against
repeated name lookups, from CVE-2014-0062 (commit 5f17304).
Interesting, thank you.
I did a rough refactor and attached v3. Aside from cleanup issues, is
this what you had in mind?
Regards,
Jeff Davis
Attachments:
v3-0001-Add-missing-RestrictSearchPath-calls.patchtext/x-patch; charset=UTF-8; name=v3-0001-Add-missing-RestrictSearchPath-calls.patchDownload
From cf1722bf0716897f42ce89282f361af4be56b60b Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Tue, 9 Jul 2024 11:12:46 -0700
Subject: [PATCH v3 1/2] Add missing RestrictSearchPath() calls.
Reported-by: Noah Misch
Backpatch-through: 17
Discussion: https://postgr.es/m/20240630222344.db.nmisch@google.com
---
src/backend/commands/indexcmds.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 2caab88aa58..c5a56c75f69 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1230,6 +1230,7 @@ DefineIndex(Oid tableId,
*/
AtEOXact_GUC(false, root_save_nestlevel);
root_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
/* Add any requested comment */
if (stmt->idxcomment != NULL)
@@ -2027,6 +2028,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
}
@@ -2074,6 +2076,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
/*
@@ -2104,6 +2107,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
SetUserIdAndSecContext(save_userid, save_sec_context);
*ddl_save_nestlevel = NewGUCNestLevel();
+ RestrictSearchPath();
}
/*
--
2.34.1
v3-0002-For-materialized-views-use-REFRESH-to-load-data-d.patchtext/x-patch; charset=UTF-8; name=v3-0002-For-materialized-views-use-REFRESH-to-load-data-d.patchDownload
From e1f89d13d46cdf040572fc1ce760d5ebc1cacc76 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Fri, 12 Jul 2024 14:23:17 -0700
Subject: [PATCH v3 2/2] For materialized views, use REFRESH to load data
during creation.
Previously, CREATE MATERIALIZED VIEW ... WITH DATA populated the MV
during creation. Instead, use REFRESH, which locks down
security-restricted operations and restricts the
search_path. Otherwise, it's possible to create MVs that cannot be
refreshed.
Reported-by: Noah Misch
Backpatch-through: 17
Discussion: https://postgr.es/m/20240630222344.db.nmisch@google.com
---
src/backend/commands/createas.c | 33 +++++++++----------
src/backend/commands/matview.c | 43 +++++++++++++++----------
src/include/commands/matview.h | 3 ++
src/test/regress/expected/namespace.out | 4 +--
4 files changed, 47 insertions(+), 36 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 62050f4dc59..54a554f4b67 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -225,10 +225,8 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
Query *query = castNode(Query, stmt->query);
IntoClause *into = stmt->into;
bool is_matview = (into->viewQuery != NULL);
+ bool do_refresh = false;
DestReceiver *dest;
- Oid save_userid = InvalidOid;
- int save_sec_context = 0;
- int save_nestlevel = 0;
ObjectAddress address;
List *rewritten;
PlannedStmt *plan;
@@ -263,18 +261,13 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
Assert(query->commandType == CMD_SELECT);
/*
- * For materialized views, lock down security-restricted operations and
- * arrange to make GUC variable changes local to this command. This is
- * not necessary for security, but this keeps the behavior similar to
- * REFRESH MATERIALIZED VIEW. Otherwise, one could create a materialized
- * view not possible to refresh.
+ * For materialized views, always skip data during table creation, and use
+ * REFRESH instead.
*/
if (is_matview)
{
- GetUserIdAndSecContext(&save_userid, &save_sec_context);
- SetUserIdAndSecContext(save_userid,
- save_sec_context | SECURITY_RESTRICTED_OPERATION);
- save_nestlevel = NewGUCNestLevel();
+ do_refresh = !into->skipData;
+ into->skipData = true;
}
if (into->skipData)
@@ -346,13 +339,19 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
PopActiveSnapshot();
}
- if (is_matview)
+ /*
+ * For materialized views, use REFRESH, which locks down
+ * security-restricted operations and restricts the search_path.
+ * Otherwise, one could create a materialized view not possible to
+ * refresh.
+ */
+ if (do_refresh)
{
- /* Roll back any GUC changes */
- AtEOXact_GUC(false, save_nestlevel);
+ RefreshMatViewByOid(address.objectId, false, false,
+ pstate->p_sourcetext, NULL, qc);
- /* Restore userid and security context */
- SetUserIdAndSecContext(save_userid, save_sec_context);
+ if (qc)
+ qc->commandTag = CMDTAG_SELECT;
}
return address;
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index ea05d4b224f..4b03e80404d 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -134,6 +134,28 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
ParamListInfo params, QueryCompletion *qc)
{
Oid matviewOid;
+ LOCKMODE lockmode;
+
+ /* Determine strength of lock needed. */
+ lockmode = stmt->concurrent ? ExclusiveLock : AccessExclusiveLock;
+
+ /*
+ * Get a lock until end of transaction.
+ */
+ matviewOid = RangeVarGetRelidExtended(stmt->relation,
+ lockmode, 0,
+ RangeVarCallbackMaintainsTable,
+ NULL);
+
+ return RefreshMatViewByOid(matviewOid, stmt->skipData, stmt->concurrent,
+ queryString, params, qc);
+}
+
+ObjectAddress
+RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
+ const char *queryString, ParamListInfo params,
+ QueryCompletion *qc)
+{
Relation matviewRel;
RewriteRule *rule;
List *actions;
@@ -143,25 +165,12 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
Oid OIDNewHeap;
DestReceiver *dest;
uint64 processed = 0;
- bool concurrent;
- LOCKMODE lockmode;
char relpersistence;
Oid save_userid;
int save_sec_context;
int save_nestlevel;
ObjectAddress address;
- /* Determine strength of lock needed. */
- concurrent = stmt->concurrent;
- lockmode = concurrent ? ExclusiveLock : AccessExclusiveLock;
-
- /*
- * Get a lock until end of transaction.
- */
- matviewOid = RangeVarGetRelidExtended(stmt->relation,
- lockmode, 0,
- RangeVarCallbackMaintainsTable,
- NULL);
matviewRel = table_open(matviewOid, NoLock);
relowner = matviewRel->rd_rel->relowner;
@@ -190,7 +199,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
errmsg("CONCURRENTLY cannot be used when the materialized view is not populated")));
/* Check that conflicting options have not been specified. */
- if (concurrent && stmt->skipData)
+ if (concurrent && skipData)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s and %s options cannot be used together",
@@ -275,7 +284,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
* Tentatively mark the matview as populated or not (this will roll back
* if we fail later).
*/
- SetMatViewPopulatedState(matviewRel, !stmt->skipData);
+ SetMatViewPopulatedState(matviewRel, !skipData);
/* Concurrent refresh builds new data in temp tablespace, and does diff. */
if (concurrent)
@@ -301,7 +310,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
dest = CreateTransientRelDestReceiver(OIDNewHeap);
/* Generate the data, if wanted. */
- if (!stmt->skipData)
+ if (!skipData)
processed = refresh_matview_datafill(dest, dataQuery, queryString);
/* Make the matview match the newly generated data. */
@@ -333,7 +342,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
* inserts and deletes it issues get counted by lower-level code.)
*/
pgstat_count_truncate(matviewRel);
- if (!stmt->skipData)
+ if (!skipData)
pgstat_count_heap_insert(matviewRel, processed);
}
diff --git a/src/include/commands/matview.h b/src/include/commands/matview.h
index 817b2ba0b6f..a226b2e68fb 100644
--- a/src/include/commands/matview.h
+++ b/src/include/commands/matview.h
@@ -25,6 +25,9 @@ extern void SetMatViewPopulatedState(Relation relation, bool newstate);
extern ObjectAddress ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
ParamListInfo params, QueryCompletion *qc);
+extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
+ const char *queryString, ParamListInfo params,
+ QueryCompletion *qc);
extern DestReceiver *CreateTransientRelDestReceiver(Oid transientoid);
diff --git a/src/test/regress/expected/namespace.out b/src/test/regress/expected/namespace.out
index 7d36e9cc0c4..dbbda72d395 100644
--- a/src/test/regress/expected/namespace.out
+++ b/src/test/regress/expected/namespace.out
@@ -129,8 +129,8 @@ $$;
CREATE TABLE test_maint(i INT);
INSERT INTO test_maint VALUES (1), (2);
CREATE MATERIALIZED VIEW test_maint_mv AS SELECT fn(i) FROM test_maint;
-NOTICE: current search_path: test_maint_search_path
-NOTICE: current search_path: test_maint_search_path
+NOTICE: current search_path: pg_catalog, pg_temp
+NOTICE: current search_path: pg_catalog, pg_temp
-- the following commands should see search_path as pg_catalog, pg_temp
CREATE INDEX test_maint_idx ON test_maint_search_path.test_maint (fn(i));
NOTICE: current search_path: pg_catalog, pg_temp
--
2.34.1
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Fri, Jul 12, 2024 at 04:50:17PM -0700, Jeff Davis wrote:
On Fri, 2024-07-12 at 16:11 -0700, Noah Misch wrote:
Since refresh->relation is a RangeVar, this departs from the standard
against
repeated name lookups, from CVE-2014-0062 (commit 5f17304).Interesting, thank you.
I did a rough refactor and attached v3. Aside from cleanup issues, is
this what you had in mind?
+extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent, + const char *queryString, ParamListInfo params, + QueryCompletion *qc);
Yes, that's an API design that avoids repeated name lookups.
Re: MAINTAIN privilege -- what do we need to un-revert it?
Hi,
On Sat, 13 Jul 2024 14:47:48 -0700
Noah Misch <noah@leadboat.com> wrote:
On Fri, Jul 12, 2024 at 04:50:17PM -0700, Jeff Davis wrote:
On Fri, 2024-07-12 at 16:11 -0700, Noah Misch wrote:
Since refresh->relation is a RangeVar, this departs from the standard
against
repeated name lookups, from CVE-2014-0062 (commit 5f17304).Interesting, thank you.
I did a rough refactor and attached v3. Aside from cleanup issues, is
this what you had in mind?+extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent, + const char *queryString, ParamListInfo params, + QueryCompletion *qc);Yes, that's an API design that avoids repeated name lookups.
Since this commit, matviews are no longer handled in ExecCreateTableAs, so the
following error message has not to consider materialized view cases, and can be made simple.
/* SELECT should never rewrite to more or less than one SELECT query */
if (list_length(rewritten) != 1)
elog(ERROR, "unexpected rewrite result for %s",
is_matview ? "CREATE MATERIALIZED VIEW" :
"CREATE TABLE AS SELECT");
RefreshMatViewByOid has REFRESH specific error messages in spite of its use
in CREATE MATERIALIZED VIEW, but these errors seem not to occur in CREATE MATERIALIZED
VIEW case, so I don't think it would be a problem.
Another my question is why RefreshMatViewByOid has a ParamListInfo parameter.
I don't understand why ExecRefreshMatView has one, either, because currently
materialized views may not be defined using bound parameters, which is checked
in transformCreateTableAsStmt, and the param argument is not used at all. It might
be unsafe to change the interface of ExecRefreshMatView since this is public for a
long time, but I don't think the new interface RefreshMatViewByOid has to have this
unused argument.
I attaehd patches for fixing them respectedly.
What do you think about it?
Regards,
Yugo Nagata
--
Yugo Nagata <nagata@sraoss.co.jp>
Attachments:
0002-Remove-ParamListInfo-argument-from-RefreshMatViewByO.patchtext/x-diff; name=0002-Remove-ParamListInfo-argument-from-RefreshMatViewByO.patchDownload
From 854d01bfbdece781bad46ab686b08763b8d16a95 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Fri, 26 Jul 2024 12:02:56 +0900
Subject: [PATCH 2/2] Remove ParamListInfo argument from RefreshMatViewByOid
---
src/backend/commands/createas.c | 2 +-
src/backend/commands/matview.c | 5 ++---
src/include/commands/matview.h | 3 +--
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index a15a1e187e..880425da66 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -345,7 +345,7 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
if (do_refresh)
{
RefreshMatViewByOid(address.objectId, false, false,
- pstate->p_sourcetext, NULL, qc);
+ pstate->p_sourcetext, qc);
if (qc)
qc->commandTag = CMDTAG_SELECT;
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 6bb64be274..aba30caf25 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -136,7 +136,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
NULL);
return RefreshMatViewByOid(matviewOid, stmt->skipData, stmt->concurrent,
- queryString, params, qc);
+ queryString, qc);
}
/*
@@ -160,8 +160,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
*/
ObjectAddress
RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
- const char *queryString, ParamListInfo params,
- QueryCompletion *qc)
+ const char *queryString, QueryCompletion *qc)
{
Relation matviewRel;
RewriteRule *rule;
diff --git a/src/include/commands/matview.h b/src/include/commands/matview.h
index a226b2e68f..5ff80788fa 100644
--- a/src/include/commands/matview.h
+++ b/src/include/commands/matview.h
@@ -26,8 +26,7 @@ extern void SetMatViewPopulatedState(Relation relation, bool newstate);
extern ObjectAddress ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
ParamListInfo params, QueryCompletion *qc);
extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
- const char *queryString, ParamListInfo params,
- QueryCompletion *qc);
+ const char *queryString, QueryCompletion *qc);
extern DestReceiver *CreateTransientRelDestReceiver(Oid transientoid);
--
2.34.1
0001-Fix-an-error-message-in-ExecCreateTableAs.patchtext/x-diff; name=0001-Fix-an-error-message-in-ExecCreateTableAs.patchDownload
From 4d2ae5bdb9eaf227b5e3f1278ce965a7732f514a Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Fri, 26 Jul 2024 11:58:22 +0900
Subject: [PATCH 1/2] Fix an error message in ExecCreateTableAs
---
src/backend/commands/createas.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 2c8a93b6e5..a15a1e187e 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -292,9 +292,7 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
/* SELECT should never rewrite to more or less than one SELECT query */
if (list_length(rewritten) != 1)
- elog(ERROR, "unexpected rewrite result for %s",
- is_matview ? "CREATE MATERIALIZED VIEW" :
- "CREATE TABLE AS SELECT");
+ elog(ERROR, "unexpected rewrite result for CREATE TABLE AS SELECT");
query = linitial_node(Query, rewritten);
Assert(query->commandType == CMD_SELECT);
--
2.34.1
Import Notes
Reply to msg id not found: 20240713214748.cb@rfd.leadboat.com.sranhm
Re: MAINTAIN privilege -- what do we need to un-revert it?
Hello,
Thank you for looking.
On Fri, 2024-07-26 at 12:26 +0900, Yugo Nagata wrote:
Since this commit, matviews are no longer handled in
ExecCreateTableAs, so the
following error message has not to consider materialized view cases,
and can be made simple./* SELECT should never rewrite to more or less than one
SELECT query */
if (list_length(rewritten) != 1)
elog(ERROR, "unexpected rewrite result for %s",
is_matview ? "CREATE MATERIALIZED VIEW" :
"CREATE TABLE AS SELECT");
There's a similar error in refresh_matview_datafill(), and I suppose
that should account for the CREATE MATERIALIZED VIEW case. We could
pass an additional flag to RefreshMatViewByOid to indicate whether it's
a CREATE or REFRESH, but it's an internal error, so perhaps it's not
important.
Another my question is why RefreshMatViewByOid has a ParamListInfo
parameter.
I just passed the params through, but you're right, they aren't
referenced at all.
I looked at the history, and it appears to go all the way back to the
function's introduction in commit 3bf3ab8c56.
I don't understand why ExecRefreshMatView has one, either, because
currently
materialized views may not be defined using bound parameters, which
is checked
in transformCreateTableAsStmt, and the param argument is not used at
all. It might
be unsafe to change the interface of ExecRefreshMatView since this is
public for a
long time, but I don't think the new interface RefreshMatViewByOid
has to have this
unused argument.
Extensions should be prepared for reasonable changes in these kinds of
functions between releases. Even if the signatures remain the same, the
parse structures may change, which creates similar incompatibilities.
So let's just get rid of the 'params' argument from both functions.
Regards,
Jeff Davis
Re: MAINTAIN privilege -- what do we need to un-revert it?
Hi,
On Fri, 26 Jul 2024 16:47:23 -0700
Jeff Davis <pgsql@j-davis.com> wrote:
Hello,
Thank you for looking.
On Fri, 2024-07-26 at 12:26 +0900, Yugo Nagata wrote:
Since this commit, matviews are no longer handled in
ExecCreateTableAs, so the
following error message has not to consider materialized view cases,
and can be made simple./* SELECT should never rewrite to more or less than one
SELECT query */
if (list_length(rewritten) != 1)
elog(ERROR, "unexpected rewrite result for %s",
is_matview ? "CREATE MATERIALIZED VIEW" :
"CREATE TABLE AS SELECT");There's a similar error in refresh_matview_datafill(), and I suppose
that should account for the CREATE MATERIALIZED VIEW case. We could
pass an additional flag to RefreshMatViewByOid to indicate whether it's
a CREATE or REFRESH, but it's an internal error, so perhaps it's not
important.
Thank you for looking into the pach.
I agree that it might not be important, but I think adding the flag would be
also helpful for improving code-readability because it clarify the function
is used in the two cases. I attached patch for this fix (patch 0003).
Another my question is why RefreshMatViewByOid has a ParamListInfo
parameter.I just passed the params through, but you're right, they aren't
referenced at all.I looked at the history, and it appears to go all the way back to the
function's introduction in commit 3bf3ab8c56.I don't understand why ExecRefreshMatView has one, either, because
currently
materialized views may not be defined using bound parameters, which
is checked
in transformCreateTableAsStmt, and the param argument is not used at
all. It might
be unsafe to change the interface of ExecRefreshMatView since this is
public for a
long time, but I don't think the new interface RefreshMatViewByOid
has to have this
unused argument.Extensions should be prepared for reasonable changes in these kinds of
functions between releases. Even if the signatures remain the same, the
parse structures may change, which creates similar incompatibilities.
So let's just get rid of the 'params' argument from both functions.
Sure. I fixed the patch to remove 'param' from both functions. (patch 0002)
I also add the small refactoring around ExecCreateTableAs(). (patch 0001)
- Remove matview-related codes from intorel_startup.
Materialized views are no longer handled in this function.
- RefreshMatViewByOid is moved to just after create_ctas_nodata
call to improve code readability.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
v2-0003-Add-a-flag-to-RefreshMatviewByOid-to-indicate-whe.patchtext/x-diff; name=v2-0003-Add-a-flag-to-RefreshMatviewByOid-to-indicate-whe.patchDownload
From e6027d1e417b446d09411a98fca11a831c6b7b03 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Wed, 31 Jul 2024 17:22:41 +0900
Subject: [PATCH v2 3/3] Add a flag to RefreshMatviewByOid to indicate whether
it is CREATE or not
RefreshMatviewByOid is used for both REFRESH and CREATE MATERIALIZED VIEW.
This flag is currently just used for handling internal error messages,
but also aimed to improve code-readability.
---
src/backend/commands/matview.c | 38 +++++++++++++++++++++++++---------
src/include/commands/matview.h | 3 ++-
2 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 8644ad695c..29a80fe565 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -60,7 +60,7 @@ static bool transientrel_receive(TupleTableSlot *slot, DestReceiver *self);
static void transientrel_shutdown(DestReceiver *self);
static void transientrel_destroy(DestReceiver *self);
static uint64 refresh_matview_datafill(DestReceiver *dest, Query *query,
- const char *queryString);
+ const char *queryString, bool is_create);
static char *make_temptable_name_n(char *tempname, int n);
static void refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
int save_sec_context);
@@ -136,7 +136,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
NULL);
return RefreshMatViewByOid(matviewOid, stmt->skipData, stmt->concurrent,
- queryString, qc);
+ queryString, qc, false);
}
/*
@@ -157,10 +157,14 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
*
* The matview's "populated" state is changed based on whether the contents
* reflect the result set of the materialized view's query.
+ *
+ * This is also used to populate the materialized view created by CREATE
+ * MATERIALIZED VIEW command.
*/
ObjectAddress
RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
- const char *queryString, QueryCompletion *qc)
+ const char *queryString, QueryCompletion *qc,
+ bool is_create)
{
Relation matviewRel;
RewriteRule *rule;
@@ -169,7 +173,6 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
Oid tableSpace;
Oid relowner;
Oid OIDNewHeap;
- DestReceiver *dest;
uint64 processed = 0;
char relpersistence;
Oid save_userid;
@@ -248,6 +251,8 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
ListCell *indexoidscan;
bool hasUniqueIndex = false;
+ Assert(!is_create);
+
foreach(indexoidscan, indexoidlist)
{
Oid indexoid = lfirst_oid(indexoidscan);
@@ -284,7 +289,9 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
* NB: We count on this to protect us against problems with refreshing the
* data using TABLE_INSERT_FROZEN.
*/
- CheckTableNotInUse(matviewRel, "REFRESH MATERIALIZED VIEW");
+ CheckTableNotInUse(matviewRel,
+ is_create ? "CREATE MATERIALIZED VIEW" :
+ "REFRESH MATERIALIZED VIEW");
/*
* Tentatively mark the matview as populated or not (this will roll back
@@ -313,11 +320,16 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
matviewRel->rd_rel->relam,
relpersistence, ExclusiveLock);
LockRelationOid(OIDNewHeap, AccessExclusiveLock);
- dest = CreateTransientRelDestReceiver(OIDNewHeap);
/* Generate the data, if wanted. */
if (!skipData)
- processed = refresh_matview_datafill(dest, dataQuery, queryString);
+ {
+ DestReceiver *dest;
+
+ dest = CreateTransientRelDestReceiver(OIDNewHeap);
+ processed = refresh_matview_datafill(dest, dataQuery, queryString,
+ is_create);
+ }
/* Make the matview match the newly generated data. */
if (concurrent)
@@ -369,9 +381,14 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
* i.e., the display_rowcount flag of CMDTAG_REFRESH_MATERIALIZED_VIEW
* command tag is left false in cmdtaglist.h. Otherwise, the change of
* completion tag output might break applications using it.
+ *
+ * When called from CREATE MATERIALIZED VIEW comand, the rowcount is
+ * displayed with the command tag CMDTAG_SELECT.
*/
if (qc)
- SetQueryCompletion(qc, CMDTAG_REFRESH_MATERIALIZED_VIEW, processed);
+ SetQueryCompletion(qc,
+ is_create ? CMDTAG_SELECT : CMDTAG_REFRESH_MATERIALIZED_VIEW,
+ processed);
return address;
}
@@ -386,7 +403,7 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
*/
static uint64
refresh_matview_datafill(DestReceiver *dest, Query *query,
- const char *queryString)
+ const char *queryString, bool is_create)
{
List *rewritten;
PlannedStmt *plan;
@@ -401,7 +418,8 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
/* SELECT should never rewrite to more or less than one SELECT query */
if (list_length(rewritten) != 1)
- elog(ERROR, "unexpected rewrite result for REFRESH MATERIALIZED VIEW");
+ elog(ERROR, "unexpected rewrite result for %s",
+ is_create ? "CREATE MATERIALIZED VIEW ": "REFRESH MATERIALIZED VIEW");
query = (Query *) linitial(rewritten);
/* Check for user-requested abort. */
diff --git a/src/include/commands/matview.h b/src/include/commands/matview.h
index 7916df3039..f95e468d88 100644
--- a/src/include/commands/matview.h
+++ b/src/include/commands/matview.h
@@ -26,7 +26,8 @@ extern void SetMatViewPopulatedState(Relation relation, bool newstate);
extern ObjectAddress ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
QueryCompletion *qc);
extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
- const char *queryString, QueryCompletion *qc);
+ const char *queryString, QueryCompletion *qc,
+ bool is_create);
extern DestReceiver *CreateTransientRelDestReceiver(Oid transientoid);
--
2.34.1
v2-0002-Remove-ParamListInfo-argument-from-RefreshMatView.patchtext/x-diff; name=v2-0002-Remove-ParamListInfo-argument-from-RefreshMatView.patchDownload
From f314d87bedfd905aafbacdcade9dabd2f6ee384b Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Fri, 26 Jul 2024 12:02:56 +0900
Subject: [PATCH v2 2/3] Remove ParamListInfo argument from RefreshMatViewByOid
and ExecRefreshMatView
This argument is not used at all, so we can remove it.
---
src/backend/commands/matview.c | 7 +++----
src/backend/tcop/utility.c | 2 +-
src/include/commands/matview.h | 5 ++---
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 6bb64be274..8644ad695c 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -119,7 +119,7 @@ SetMatViewPopulatedState(Relation relation, bool newstate)
*/
ObjectAddress
ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
- ParamListInfo params, QueryCompletion *qc)
+ QueryCompletion *qc)
{
Oid matviewOid;
LOCKMODE lockmode;
@@ -136,7 +136,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
NULL);
return RefreshMatViewByOid(matviewOid, stmt->skipData, stmt->concurrent,
- queryString, params, qc);
+ queryString, qc);
}
/*
@@ -160,8 +160,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
*/
ObjectAddress
RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
- const char *queryString, ParamListInfo params,
- QueryCompletion *qc)
+ const char *queryString, QueryCompletion *qc)
{
Relation matviewRel;
RewriteRule *rule;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index fa66b8017e..702a6c3a0b 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1688,7 +1688,7 @@ ProcessUtilitySlow(ParseState *pstate,
PG_TRY(2);
{
address = ExecRefreshMatView((RefreshMatViewStmt *) parsetree,
- queryString, params, qc);
+ queryString, qc);
}
PG_FINALLY(2);
{
diff --git a/src/include/commands/matview.h b/src/include/commands/matview.h
index a226b2e68f..7916df3039 100644
--- a/src/include/commands/matview.h
+++ b/src/include/commands/matview.h
@@ -24,10 +24,9 @@
extern void SetMatViewPopulatedState(Relation relation, bool newstate);
extern ObjectAddress ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
- ParamListInfo params, QueryCompletion *qc);
+ QueryCompletion *qc);
extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
- const char *queryString, ParamListInfo params,
- QueryCompletion *qc);
+ const char *queryString, QueryCompletion *qc);
extern DestReceiver *CreateTransientRelDestReceiver(Oid transientoid);
--
2.34.1
v2-0001-Small-refactoring-around-ExecCreateTableAs.patchtext/x-diff; name=v2-0001-Small-refactoring-around-ExecCreateTableAs.patchDownload
From 616f674c81ae5aa07fad40f976050e56ffd1c339 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Wed, 31 Jul 2024 17:34:15 +0900
Subject: [PATCH v2 1/3] Small refactoring around ExecCreateTableAs
Since commit b4da732f, the refresh logic is used to populate
materialized views, so we can simplify the error message
in ExecCreateTableAs.
In addition, we can remove matview-related codes from
intorel_startup because materialized views are no longer handled
in this function.
Also, RefreshMatViewByOid is moved to just after create_ctas_nodata
call to improve code readability.
---
src/backend/commands/createas.c | 47 +++++++++++----------------------
1 file changed, 16 insertions(+), 31 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 2c8a93b6e5..68d5047924 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -228,9 +228,6 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
bool do_refresh = false;
DestReceiver *dest;
ObjectAddress address;
- List *rewritten;
- PlannedStmt *plan;
- QueryDesc *queryDesc;
/* Check if the relation exists or not */
if (CreateTableAsRelExists(stmt))
@@ -279,9 +276,24 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
* from running the planner before all dependencies are set up.
*/
address = create_ctas_nodata(query->targetList, into);
+
+ /*
+ * For materialized views, reuse the REFRESH logic, which locks down
+ * security-restricted operations and restricts the search_path. This
+ * reduces the chance that a subsequent refresh will fail.
+ */
+ if (do_refresh)
+ RefreshMatViewByOid(address.objectId, false, false,
+ pstate->p_sourcetext, NULL qc);
}
else
{
+ List *rewritten;
+ PlannedStmt *plan;
+ QueryDesc *queryDesc;
+
+ Assert(!is_matview);
+
/*
* Parse analysis was done already, but we still have to run the rule
* rewriter. We do not do AcquireRewriteLocks: we assume the query
@@ -292,9 +304,7 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
/* SELECT should never rewrite to more or less than one SELECT query */
if (list_length(rewritten) != 1)
- elog(ERROR, "unexpected rewrite result for %s",
- is_matview ? "CREATE MATERIALIZED VIEW" :
- "CREATE TABLE AS SELECT");
+ elog(ERROR, "unexpected rewrite result for CREATE TABLE AS SELECT");
query = linitial_node(Query, rewritten);
Assert(query->commandType == CMD_SELECT);
@@ -339,20 +349,6 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
PopActiveSnapshot();
}
- /*
- * For materialized views, reuse the REFRESH logic, which locks down
- * security-restricted operations and restricts the search_path. This
- * reduces the chance that a subsequent refresh will fail.
- */
- if (do_refresh)
- {
- RefreshMatViewByOid(address.objectId, false, false,
- pstate->p_sourcetext, NULL, qc);
-
- if (qc)
- qc->commandTag = CMDTAG_SELECT;
- }
-
return address;
}
@@ -453,7 +449,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
{
DR_intorel *myState = (DR_intorel *) self;
IntoClause *into = myState->into;
- bool is_matview;
List *attrList;
ObjectAddress intoRelationAddr;
Relation intoRelationDesc;
@@ -462,9 +457,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
Assert(into != NULL); /* else somebody forgot to set it */
- /* This code supports both CREATE TABLE AS and CREATE MATERIALIZED VIEW */
- is_matview = (into->viewQuery != NULL);
-
/*
* Build column definitions using "pre-cooked" type and collation info. If
* a column name list was specified in CREATE TABLE AS, override the
@@ -538,13 +530,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("policies not yet implemented for this command")));
- /*
- * Tentatively mark the target as populated, if it's a matview and we're
- * going to fill it; otherwise, no change needed.
- */
- if (is_matview && !into->skipData)
- SetMatViewPopulatedState(intoRelationDesc, true);
-
/*
* Fill private fields of myState for use by later routines
*/
--
2.34.1
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Wed, 2024-07-31 at 18:20 +0900, Yugo NAGATA wrote:
I agree that it might not be important, but I think adding the flag
would be
also helpful for improving code-readability because it clarify the
function
is used in the two cases. I attached patch for this fix (patch 0003).
Committed with one minor modification: I moved the boolean flag to be
near the other booleans rather than at the end. Thank you.
Sure. I fixed the patch to remove 'param' from both functions. (patch
0002)
Committed, thank you.
I also add the small refactoring around ExecCreateTableAs(). (patch
0001)- Remove matview-related codes from intorel_startup.
Materialized views are no longer handled in this function.- RefreshMatViewByOid is moved to just after create_ctas_nodata
call to improve code readability.
I'm not sure the changes in intorel_startup() are correct. I tried
adding an Assert(into->viewQuery == NULL), and it fails because there's
another path I did not consider: "EXPLAIN ANALYZE CREATE MATERIALIZED
VIEW ...", which does not go through ExecCreateTableAs() but does go
through CreateIntoRelDestReceiver().
See:
/messages/by-id/20444c382e6cb5e21e93c94d679d0198b0dba4dd.camel@j-davis.com
Should we refactor a bit and try to make EXPLAIN use the same code
paths?
Regards,
Jeff Davis
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Thu, 01 Aug 2024 11:31:53 -0700
Jeff Davis <pgsql@j-davis.com> wrote:
On Wed, 2024-07-31 at 18:20 +0900, Yugo NAGATA wrote:
I agree that it might not be important, but I think adding the flag
would be
also helpful for improving code-readability because it clarify the
function
is used in the two cases. I attached patch for this fix (patch 0003).Committed with one minor modification: I moved the boolean flag to be
near the other booleans rather than at the end. Thank you.Sure. I fixed the patch to remove 'param' from both functions. (patch
0002)Committed, thank you.
Thank you for committing them.
Should not they be backported to REL_17_STABLE?
I also add the small refactoring around ExecCreateTableAs(). (patch
0001)- Remove matview-related codes from intorel_startup.
Materialized views are no longer handled in this function.- RefreshMatViewByOid is moved to just after create_ctas_nodata
call to improve code readability.I'm not sure the changes in intorel_startup() are correct. I tried
adding an Assert(into->viewQuery == NULL), and it fails because there's
another path I did not consider: "EXPLAIN ANALYZE CREATE MATERIALIZED
VIEW ...", which does not go through ExecCreateTableAs() but does go
through CreateIntoRelDestReceiver().See:
/messages/by-id/20444c382e6cb5e21e93c94d679d0198b0dba4dd.camel@j-davis.com
Should we refactor a bit and try to make EXPLAIN use the same code
paths?
I overlooked that CreateIntoRelDestReceiver() is used from EXPLAIN. I saw the
thread above and I agree that we should refactor it to make EXPLAIN consistent
CREATE MATERIALIZED VIEW, but I suppose this should be discussed the other thread.
I attached a updated patch removed the intorel_startup() part from.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
v3-0001-Small-refactoring-around-ExecCreateTableAs.patchtext/x-diff; name=v3-0001-Small-refactoring-around-ExecCreateTableAs.patchDownload
From 3609e85c288726e16dc851996a3b99e6179f43db Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Fri, 2 Aug 2024 16:02:07 +0900
Subject: [PATCH v3] Small refactoring around ExecCreateTableAs
Since commit b4da732f, the refresh logic is used to populate
materialized views, so we can simplify the error message
in ExecCreateTableAs.
Also, RefreshMatViewByOid is moved to just after create_ctas_nodata
call to improve code readability.
---
src/backend/commands/createas.c | 34 ++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 36e192b79b..c71ff80188 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -228,9 +228,6 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
bool do_refresh = false;
DestReceiver *dest;
ObjectAddress address;
- List *rewritten;
- PlannedStmt *plan;
- QueryDesc *queryDesc;
/* Check if the relation exists or not */
if (CreateTableAsRelExists(stmt))
@@ -279,9 +276,25 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
* from running the planner before all dependencies are set up.
*/
address = create_ctas_nodata(query->targetList, into);
+
+ /*
+ * For materialized views, reuse the REFRESH logic, which locks down
+ * security-restricted operations and restricts the search_path. This
+ * reduces the chance that a subsequent refresh will fail.
+ */
+ if (do_refresh)
+ RefreshMatViewByOid(address.objectId, true, false, false,
+ pstate->p_sourcetext, qc);
+
}
else
{
+ List *rewritten;
+ PlannedStmt *plan;
+ QueryDesc *queryDesc;
+
+ Assert(!is_matview);
+
/*
* Parse analysis was done already, but we still have to run the rule
* rewriter. We do not do AcquireRewriteLocks: we assume the query
@@ -292,9 +305,7 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
/* SELECT should never rewrite to more or less than one SELECT query */
if (list_length(rewritten) != 1)
- elog(ERROR, "unexpected rewrite result for %s",
- is_matview ? "CREATE MATERIALIZED VIEW" :
- "CREATE TABLE AS SELECT");
+ elog(ERROR, "unexpected rewrite result for CREATE TABLE AS SELECT");
query = linitial_node(Query, rewritten);
Assert(query->commandType == CMD_SELECT);
@@ -339,17 +350,6 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
PopActiveSnapshot();
}
- /*
- * For materialized views, reuse the REFRESH logic, which locks down
- * security-restricted operations and restricts the search_path. This
- * reduces the chance that a subsequent refresh will fail.
- */
- if (do_refresh)
- {
- RefreshMatViewByOid(address.objectId, true, false, false,
- pstate->p_sourcetext, qc);
- }
-
return address;
}
--
2.34.1
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Fri, 2 Aug 2024 16:13:01 +0900
Yugo NAGATA <nagata@sraoss.co.jp> wrote:
On Thu, 01 Aug 2024 11:31:53 -0700
Jeff Davis <pgsql@j-davis.com> wrote:On Wed, 2024-07-31 at 18:20 +0900, Yugo NAGATA wrote:
I agree that it might not be important, but I think adding the flag
would be
also helpful for improving code-readability because it clarify the
function
is used in the two cases. I attached patch for this fix (patch 0003).Committed with one minor modification: I moved the boolean flag to be
near the other booleans rather than at the end. Thank you.Sure. I fixed the patch to remove 'param' from both functions. (patch
0002)Committed, thank you.
Thank you for committing them.
Should not they be backported to REL_17_STABLE?I also add the small refactoring around ExecCreateTableAs(). (patch
0001)- Remove matview-related codes from intorel_startup.
Materialized views are no longer handled in this function.- RefreshMatViewByOid is moved to just after create_ctas_nodata
call to improve code readability.I'm not sure the changes in intorel_startup() are correct. I tried
adding an Assert(into->viewQuery == NULL), and it fails because there's
another path I did not consider: "EXPLAIN ANALYZE CREATE MATERIALIZED
VIEW ...", which does not go through ExecCreateTableAs() but does go
through CreateIntoRelDestReceiver().See:
/messages/by-id/20444c382e6cb5e21e93c94d679d0198b0dba4dd.camel@j-davis.com
Should we refactor a bit and try to make EXPLAIN use the same code
paths?I overlooked that CreateIntoRelDestReceiver() is used from EXPLAIN. I saw the
thread above and I agree that we should refactor it to make EXPLAIN consistent
CREATE MATERIALIZED VIEW, but I suppose this should be discussed the other thread.I attached a updated patch removed the intorel_startup() part from.
I confirmed that this has been committed to the master branch.
Thank you!
I also noticed that the documentation of CREATE MATERIALIZED VIEW doesn't mention
search_path while it also changes search_path since it uses the REFRESH logic.
I attached a trivial patch to fix this.
Regards,
Yugo Nagata
--
Yugo Nagata <nagata@sraoss.co.jp>
Attachments:
doc_create_matview_search_path.patchtext/x-diff; name=doc_create_matview_search_path.patchDownload
diff --git a/doc/src/sgml/ref/create_materialized_view.sgml b/doc/src/sgml/ref/create_materialized_view.sgml
index 0d2fea2b97..1eb27166d9 100644
--- a/doc/src/sgml/ref/create_materialized_view.sgml
+++ b/doc/src/sgml/ref/create_materialized_view.sgml
@@ -163,6 +163,16 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] <replaceable>table_name</replaceable>
</variablelist>
</refsect1>
+ <refsect1>
+ <title>Notes</title>
+
+ <para>
+ While <command>CREATE MATERIALIZED VIEW</command> is running, the <xref
+ linkend="guc-search-path"/> is temporarily changed to <literal>pg_catalog,
+ pg_temp</literal>.
+ </para>
+ </refsect1>
+
<refsect1>
<title>Compatibility</title>
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Mon, Aug 05, 2024 at 04:05:02PM +0900, Yugo Nagata wrote:
+ <para> + While <command>CREATE MATERIALIZED VIEW</command> is running, the <xref + linkend="guc-search-path"/> is temporarily changed to <literal>pg_catalog, + pg_temp</literal>. + </para>
I think we should mention that this is not true when WITH NO DATA is used.
Maybe something like:
Unless WITH NO DATA is used, the search_path is temporarily changed to
pg_catalog, pg_temp while CREATE MATERIALIZED VIEW is running.
--
nathan
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Thu, 26 Sep 2024 16:33:06 -0500
Nathan Bossart <nathandbossart@gmail.com> wrote:
On Mon, Aug 05, 2024 at 04:05:02PM +0900, Yugo Nagata wrote:
+ <para> + While <command>CREATE MATERIALIZED VIEW</command> is running, the <xref + linkend="guc-search-path"/> is temporarily changed to <literal>pg_catalog, + pg_temp</literal>. + </para>I think we should mention that this is not true when WITH NO DATA is used.
Maybe something like:Unless WITH NO DATA is used, the search_path is temporarily changed to
pg_catalog, pg_temp while CREATE MATERIALIZED VIEW is running.
I agree with you. I overlooked WITH NO DATA.
I attached a updated patch.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
v2_doc_create_matview_search_path.patchtext/x-diff; name=v2_doc_create_matview_search_path.patchDownload
diff --git a/doc/src/sgml/ref/create_materialized_view.sgml b/doc/src/sgml/ref/create_materialized_view.sgml
index 0d2fea2b97..2fdc210c27 100644
--- a/doc/src/sgml/ref/create_materialized_view.sgml
+++ b/doc/src/sgml/ref/create_materialized_view.sgml
@@ -163,6 +163,17 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] <replaceable>table_name</replaceable>
</variablelist>
</refsect1>
+ <refsect1>
+ <title>Notes</title>
+
+ <para>
+ Unless <command>WITH NO DATA</command> is used, the <xref
+ linkend="guc-search-path"/> is temporarily changed to
+ <literal>pg_catalog, pg_temp</literal> while
+ <command>CREATE MATERIALIZED VIEW</command> is running.
+ </para>
+ </refsect1>
+
<refsect1>
<title>Compatibility</title>
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Fri, Sep 27, 2024 at 12:42:34PM +0900, Yugo NAGATA wrote:
I agree with you. I overlooked WITH NO DATA.
I attached a updated patch.
Thanks. Unless someone objects, I plan to commit this shortly.
--
nathan
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Fri, 2024-09-27 at 10:34 -0500, Nathan Bossart wrote:
On Fri, Sep 27, 2024 at 12:42:34PM +0900, Yugo NAGATA wrote:
I agree with you. I overlooked WITH NO DATA.
I attached a updated patch.Thanks. Unless someone objects, I plan to commit this shortly.
The command is run effectively in two parts: the CREATE part and the
REFRESH part. The former just uses the session search path, while the
latter uses the safe search path.
I suggest that we add the wording to the
<replaceable>query</replaceable> portion of the doc, near "security-
restricted operation".
Regards,
Jeff Davis
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Fri, Sep 27, 2024 at 09:22:48AM -0700, Jeff Davis wrote:
I suggest that we add the wording to the
<replaceable>query</replaceable> portion of the doc, near "security-
restricted operation".
How does this look?
diff --git a/doc/src/sgml/ref/create_materialized_view.sgml b/doc/src/sgml/ref/create_materialized_view.sgml
index 0d2fea2b97..62d897931c 100644
--- a/doc/src/sgml/ref/create_materialized_view.sgml
+++ b/doc/src/sgml/ref/create_materialized_view.sgml
@@ -143,7 +143,9 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] <replaceable>table_name</replaceable>
A <link linkend="sql-select"><command>SELECT</command></link>, <link linkend="sql-table"><command>TABLE</command></link>,
or <link linkend="sql-values"><command>VALUES</command></link> command. This query will run within a
security-restricted operation; in particular, calls to functions that
- themselves create temporary tables will fail.
+ themselves create temporary tables will fail. Also, while the query is
+ running, the <xref linkend="guc-search-path"/> is temporarily changed to
+ <literal>pg_catalog, pg_temp</literal>.
</para>
</listitem>
</varlistentry>--
nathan
Re: MAINTAIN privilege -- what do we need to un-revert it?
On Fri, 2024-09-27 at 15:04 -0500, Nathan Bossart wrote:
On Fri, Sep 27, 2024 at 09:22:48AM -0700, Jeff Davis wrote:
I suggest that we add the wording to the
<replaceable>query</replaceable> portion of the doc, near
"security-
restricted operation".How does this look?
Looks good to me.
Regards,
Jeff Davis