public inbox for [email protected]  

help / color / mirror / Atom feed

Re: Fix search_path for all maintenance commands
21+ messages / 7 participants
[nested] [flat]

* Re: Fix search_path for all maintenance commands
@ 2023-07-17 17:58  Nathan Bossart <[email protected]>
  0 siblings, 1 reply; 21+ messages in thread

From: Nathan Bossart @ 2023-07-17 17:58 UTC (permalink / raw)
  To: Noah Misch <[email protected]>; +Cc: David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; Jeff Davis <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Sat, Jul 15, 2023 at 02:13:33PM -0700, Noah Misch wrote:
> The 2018 security fixes instigated many function repairs that $SUBJECT would
> otherwise instigate.  That wasn't too painful.  The net new pain of $SUBJECT
> will be less, since the 2018 security fixes prepared the path.  Hence, I
> remain +1 for the latest Davis proposal.

I concur.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com






^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2023-07-17 19:16  Jeff Davis <[email protected]>
  parent: Nathan Bossart <[email protected]>
  0 siblings, 1 reply; 21+ messages in thread

From: Jeff Davis @ 2023-07-17 19:16 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; Noah Misch <[email protected]>; +Cc: David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Mon, 2023-07-17 at 10:58 -0700, Nathan Bossart wrote:
> On Sat, Jul 15, 2023 at 02:13:33PM -0700, Noah Misch wrote:
> > The 2018 security fixes instigated many function repairs that
> > $SUBJECT would
> > otherwise instigate.  That wasn't too painful.  The net new pain of
> > $SUBJECT
> > will be less, since the 2018 security fixes prepared the path. 
> > Hence, I
> > remain +1 for the latest Davis proposal.
> 
> I concur.

Based on feedback, I plan to commit soon.

Tom's objection seemed specific to v16, and Robert's concern seemed to
be about having the MAINTAIN privilege without this patch. If I missed
any objections to this patch, please let me know.

If we hear about breakage that suggests we need an escape hatch GUC, we
have time to add one later.

I remain open to considering more complete fixes for the search_path
problems.

Regards,
	Jeff Davis







^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2023-07-21 22:32  Jeff Davis <[email protected]>
  parent: Jeff Davis <[email protected]>
  0 siblings, 3 replies; 21+ messages in thread

From: Jeff Davis @ 2023-07-21 22:32 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; Noah Misch <[email protected]>; +Cc: David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Mon, 2023-07-17 at 12:16 -0700, Jeff Davis wrote:
> Based on feedback, I plan to commit soon.

Attached is a new version.

Changes:

* Also switch the search_path during CREATE MATERIALIZED VIEW, so that
it's consistent with REFRESH. As a part of this change, I slightly
reordered things in ExecCreateTableAs() so that the skipData path
returns early without entering the SECURITY_RESTRICTED_OPERATION. I
don't think that's a problem because (a) that is one place where
SECURITY_RESTRICTED_OPERATION is not used for security, but rather for
consistency; and (b) that path doesn't go through rewriter, planner, or
executor anyway so I don't see why it would matter.

* Use GUC_ACTION_SAVE rather than GUC_ACTION_SET. That was a problem
with the previous patch for index functions executed in parallel
workers, which can happen calling SQL functions from pg_amcheck.

* I used a wrapper function RestrictSearchPath() rather than calling
set_config_option() directly. That provides a nice place in case we
need to add a compatibility GUC to disable it.

Question:

Why do we switch to the table owner and use
SECURITY_RESTRICTED_OPERATION in DefineIndex(), when we will switch in
index_build (etc.) anyway? Similarly, why do we switch in vacuum_rel(),
when it doesn't matter for lazy vacuum and we will switch in
cluster_rel() and do_analyze_rel() anyway?

For now, I left the extra calls to RestrictSearchPath() in for
consistency with the switches to the table owner.

Regards,
	Jeff Davis



Attachments:

  [text/x-patch] v1-0001-Restrict-search_path-when-performing-maintenance.patch (31.5K, ../../[email protected]/2-v1-0001-Restrict-search_path-when-performing-maintenance.patch)
  download | inline diff:
From a2a2468dc9577498d75aaff5ea83726af0fd4d5d Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 6 Jul 2023 13:06:22 -0700
Subject: [PATCH v1] Restrict search_path when performing maintenance.

When executing maintenance operations (ANALYZE, CLUSTER,
CREATE/REFRESH MATERIALIZED VIEW, REINDEX, or VACUUM), set search_path
to 'pg_catalog, pg_temp'.

This change avoids problems when maintenance commands are executed
with a different search_path, which could lead to errors or
inconsistent results. It's also a step towards offering the MAINTAIN
privilege safely.

Functions that are used for functional indexes, partial indexes, or in
materialized views that depend on a different search path should be
declared with CREATE FUNCTION ... SET search_path='...'.

A previous version of this change was committed as 05e1737351, but was
not acceptable for 16 and reverted.

Discussion: https://postgr.es/m/CA%2BTgmoZVCHERUkXhAMT2Er-sKBc5C6_iX%2BTpxxivBevDHzq2TQ%40mail.gmail.com
Discussion: https://postgr.es/m/e44327179e5c9015c8dda67351c04da552066017.camel%40j-davis.com
---
 contrib/amcheck/verify_nbtree.c               |   2 +
 .../sgml/ref/create_materialized_view.sgml    |   6 +-
 .../sgml/ref/refresh_materialized_view.sgml   |  19 ++-
 src/backend/access/brin/brin.c                |   2 +
 src/backend/catalog/index.c                   |   5 +
 src/backend/commands/analyze.c                |   2 +
 src/backend/commands/cluster.c                |   2 +
 src/backend/commands/createas.c               | 136 ++++++++++--------
 src/backend/commands/indexcmds.c              |   7 +
 src/backend/commands/matview.c                |   2 +
 src/backend/commands/vacuum.c                 |   2 +
 src/backend/utils/init/usercontext.c          |  14 ++
 src/bin/scripts/t/100_vacuumdb.pl             |   4 -
 src/include/utils/usercontext.h               |   1 +
 .../expected/test_oat_hooks.out               |   4 +
 src/test/regress/expected/matview.out         |  40 +++++-
 src/test/regress/expected/privileges.out      |  12 +-
 src/test/regress/expected/vacuum.out          |   2 +-
 src/test/regress/sql/matview.sql              |  23 +++
 src/test/regress/sql/privileges.sql           |   8 +-
 src/test/regress/sql/vacuum.sql               |   2 +-
 21 files changed, 212 insertions(+), 83 deletions(-)

diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 94a9759322..bc43808d35 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -40,6 +40,7 @@
 #include "utils/guc.h"
 #include "utils/memutils.h"
 #include "utils/snapmgr.h"
+#include "utils/usercontext.h"
 
 
 PG_MODULE_MAGIC;
@@ -281,6 +282,7 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
 		SetUserIdAndSecContext(heaprel->rd_rel->relowner,
 							   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 		save_nestlevel = NewGUCNestLevel();
+		RestrictSearchPath();
 	}
 	else
 	{
diff --git a/doc/src/sgml/ref/create_materialized_view.sgml b/doc/src/sgml/ref/create_materialized_view.sgml
index 0d2fea2b97..06e3d80f70 100644
--- a/doc/src/sgml/ref/create_materialized_view.sgml
+++ b/doc/src/sgml/ref/create_materialized_view.sgml
@@ -37,8 +37,10 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] <replaceable>table_name</replaceable>
   <para>
    <command>CREATE MATERIALIZED VIEW</command> defines a materialized view of
    a query.  The query is executed and used to populate the view at the time
-   the command is issued (unless <command>WITH NO DATA</command> is used) and may be
-   refreshed later using <command>REFRESH MATERIALIZED VIEW</command>.
+   the command is issued (unless <command>WITH NO DATA</command> is used) and
+   may be refreshed later using <command>REFRESH MATERIALIZED VIEW</command>.
+   While the query is executing, the <xref linkend="guc-search-path"/> is set
+   to <literal>pg_catalog, pg_temp</literal>.
   </para>
 
   <para>
diff --git a/doc/src/sgml/ref/refresh_materialized_view.sgml b/doc/src/sgml/ref/refresh_materialized_view.sgml
index 675d6090f3..a2232a6438 100644
--- a/doc/src/sgml/ref/refresh_materialized_view.sgml
+++ b/doc/src/sgml/ref/refresh_materialized_view.sgml
@@ -32,12 +32,19 @@ 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
-   <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
-   data is generated and the materialized view is left in an unscannable
-   state.
+   owner of the materialized view.  The old contents are discarded.
+  </para>
+  <para>
+   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.  The query is executed as the owner of the
+   materialized view; and while the query is executing the <xref
+   linkend="guc-search-path"/> is set to <literal>pg_catalog,
+   pg_temp</literal>.
+  </para>
+  <para>
+   If <literal>WITH NO DATA</literal> is specified no new data is generated
+   and the materialized view is left in an unscannable state.
   </para>
   <para>
    <literal>CONCURRENTLY</literal> and <literal>WITH NO DATA</literal> may not
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 3c6a956eaa..ec7e135663 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -40,6 +40,7 @@
 #include "utils/index_selfuncs.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
+#include "utils/usercontext.h"
 
 
 /*
@@ -1066,6 +1067,7 @@ brin_summarize_range(PG_FUNCTION_ARGS)
 		SetUserIdAndSecContext(heapRel->rd_rel->relowner,
 							   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 		save_nestlevel = NewGUCNestLevel();
+		RestrictSearchPath();
 	}
 	else
 	{
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index eb2b8d84c3..c2f3398ac9 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -85,6 +85,7 @@
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
 #include "utils/tuplesort.h"
+#include "utils/usercontext.h"
 
 /* Potentially set by pg_upgrade_support functions */
 Oid			binary_upgrade_next_index_pg_class_oid = InvalidOid;
@@ -1476,6 +1477,7 @@ index_concurrently_build(Oid heapRelationId,
 	SetUserIdAndSecContext(heapRel->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	RestrictSearchPath();
 
 	indexRelation = index_open(indexRelationId, RowExclusiveLock);
 
@@ -3007,6 +3009,7 @@ index_build(Relation heapRelation,
 	SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	RestrictSearchPath();
 
 	/* Set up initial progress report status */
 	{
@@ -3343,6 +3346,7 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
 	SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	RestrictSearchPath();
 
 	indexRelation = index_open(indexId, RowExclusiveLock);
 
@@ -3603,6 +3607,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
 	SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	RestrictSearchPath();
 
 	if (progress)
 	{
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index bfd981aa3f..fe730970cf 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -67,6 +67,7 @@
 #include "utils/spccache.h"
 #include "utils/syscache.h"
 #include "utils/timestamp.h"
+#include "utils/usercontext.h"
 
 
 /* Per-index data for ANALYZE */
@@ -349,6 +350,7 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 	SetUserIdAndSecContext(onerel->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	RestrictSearchPath();
 
 	/* 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 a3bef6ac34..1730a3cd59 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -59,6 +59,7 @@
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
 #include "utils/tuplesort.h"
+#include "utils/usercontext.h"
 
 /*
  * This struct is used to pass around the information on tables to be
@@ -353,6 +354,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
 	SetUserIdAndSecContext(OldHeap->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	RestrictSearchPath();
 
 	/*
 	 * Since we may open a new transaction for each relation, we have to check
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index e91920ca14..8e8e55641f 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -50,6 +50,7 @@
 #include "utils/rel.h"
 #include "utils/rls.h"
 #include "utils/snapmgr.h"
+#include "utils/usercontext.h"
 
 typedef struct
 {
@@ -61,6 +62,8 @@ typedef struct
 	CommandId	output_cid;		/* cmin to insert in output tuples */
 	int			ti_options;		/* table_tuple_insert performance options */
 	BulkInsertState bistate;	/* bulk insert state */
+	int		   *save_nestlevel;	/* initialized in ExecCreateTableAs if new GUC
+								 * nest level is created */
 } DR_intorel;
 
 /* utility functions for CTAS definition creation */
@@ -267,6 +270,17 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
 	}
 	Assert(query->commandType == CMD_SELECT);
 
+	if (into->skipData)
+	{
+		/*
+		 * If WITH NO DATA was specified, do not go through the rewriter,
+		 * planner and executor.  Just define the relation using a code path
+		 * similar to CREATE VIEW.  This avoids dump/restore problems stemming
+		 * from running the planner before all dependencies are set up.
+		 */
+		return create_ctas_nodata(query->targetList, into);
+	}
+
 	/*
 	 * For materialized views, lock down security-restricted operations and
 	 * arrange to make GUC variable changes local to this command.  This is
@@ -280,76 +294,65 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
 		SetUserIdAndSecContext(save_userid,
 							   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 		save_nestlevel = NewGUCNestLevel();
+		RestrictSearchPath();
+		((DR_intorel *) dest)->save_nestlevel = &save_nestlevel;
 	}
 
-	if (into->skipData)
-	{
-		/*
-		 * If WITH NO DATA was specified, do not go through the rewriter,
-		 * planner and executor.  Just define the relation using a code path
-		 * similar to CREATE VIEW.  This avoids dump/restore problems stemming
-		 * from running the planner before all dependencies are set up.
-		 */
-		address = create_ctas_nodata(query->targetList, into);
-	}
-	else
-	{
-		/*
-		 * Parse analysis was done already, but we still have to run the rule
-		 * rewriter.  We do not do AcquireRewriteLocks: we assume the query
-		 * either came straight from the parser, or suitable locks were
-		 * acquired by plancache.c.
-		 */
-		rewritten = QueryRewrite(query);
-
-		/* 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");
-		query = linitial_node(Query, rewritten);
-		Assert(query->commandType == CMD_SELECT);
+	/*
+	 * Parse analysis was done already, but we still have to run the rule
+	 * rewriter.  We do not do AcquireRewriteLocks: we assume the query either
+	 * came straight from the parser, or suitable locks were acquired by
+	 * plancache.c.
+	 */
+	rewritten = QueryRewrite(query);
+
+	/* 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");
+	query = linitial_node(Query, rewritten);
+	Assert(query->commandType == CMD_SELECT);
 
-		/* plan the query */
-		plan = pg_plan_query(query, pstate->p_sourcetext,
-							 CURSOR_OPT_PARALLEL_OK, params);
+	/* plan the query */
+	plan = pg_plan_query(query, pstate->p_sourcetext,
+						 CURSOR_OPT_PARALLEL_OK, params);
 
-		/*
-		 * Use a snapshot with an updated command ID to ensure this query sees
-		 * results of any previously executed queries.  (This could only
-		 * matter if the planner executed an allegedly-stable function that
-		 * changed the database contents, but let's do it anyway to be
-		 * parallel to the EXPLAIN code path.)
-		 */
-		PushCopiedSnapshot(GetActiveSnapshot());
-		UpdateActiveSnapshotCommandId();
+	/*
+	 * Use a snapshot with an updated command ID to ensure this query sees
+	 * results of any previously executed queries.  (This could only matter if
+	 * the planner executed an allegedly-stable function that changed the
+	 * database contents, but let's do it anyway to be parallel to the EXPLAIN
+	 * code path.)
+	 */
+	PushCopiedSnapshot(GetActiveSnapshot());
+	UpdateActiveSnapshotCommandId();
 
-		/* Create a QueryDesc, redirecting output to our tuple receiver */
-		queryDesc = CreateQueryDesc(plan, pstate->p_sourcetext,
-									GetActiveSnapshot(), InvalidSnapshot,
-									dest, params, queryEnv, 0);
+	/* Create a QueryDesc, redirecting output to our tuple receiver */
+	queryDesc = CreateQueryDesc(plan, pstate->p_sourcetext,
+								GetActiveSnapshot(), InvalidSnapshot,
+								dest, params, queryEnv, 0);
 
-		/* call ExecutorStart to prepare the plan for execution */
-		ExecutorStart(queryDesc, GetIntoRelEFlags(into));
+	/* call ExecutorStart to prepare the plan for execution */
+	ExecutorStart(queryDesc, GetIntoRelEFlags(into));
 
-		/* run the plan to completion */
-		ExecutorRun(queryDesc, ForwardScanDirection, 0, true);
+	/* run the plan to completion */
+	ExecutorRun(queryDesc, ForwardScanDirection, 0, true);
 
-		/* save the rowcount if we're given a qc to fill */
-		if (qc)
-			SetQueryCompletion(qc, CMDTAG_SELECT, queryDesc->estate->es_processed);
+	/* save the rowcount if we're given a qc to fill */
+	if (qc)
+		SetQueryCompletion(qc, CMDTAG_SELECT, queryDesc->estate->es_processed);
 
-		/* get object address that intorel_startup saved for us */
-		address = ((DR_intorel *) dest)->reladdr;
+	/* get object address that intorel_startup saved for us */
+	address = ((DR_intorel *) dest)->reladdr;
 
-		/* and clean up */
-		ExecutorFinish(queryDesc);
-		ExecutorEnd(queryDesc);
+	/* and clean up */
+	ExecutorFinish(queryDesc);
+	ExecutorEnd(queryDesc);
 
-		FreeQueryDesc(queryDesc);
+	FreeQueryDesc(queryDesc);
 
-		PopActiveSnapshot();
-	}
+	PopActiveSnapshot();
 
 	if (is_matview)
 	{
@@ -522,11 +525,28 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
 				(errcode(ERRCODE_SYNTAX_ERROR),
 				 errmsg("too many column names were specified")));
 
+	/*
+	 * Restore search_path to original value so that the object is created in
+	 * the correct namespace.
+	 */
+	if (myState->save_nestlevel)
+		AtEOXact_GUC(false, *myState->save_nestlevel);
+
 	/*
 	 * Actually create the target table
 	 */
 	intoRelationAddr = create_ctas_internal(attrList, into);
 
+	/*
+	 * Restrict the search_path again for execution of the materialized view
+	 * query.
+	 */
+	if (myState->save_nestlevel)
+	{
+		*myState->save_nestlevel = NewGUCNestLevel();
+		RestrictSearchPath();
+	}
+
 	/*
 	 * Finally we can open the target table
 	 */
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index baf3e6e57a..e855a50df8 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -68,6 +68,7 @@
 #include "utils/regproc.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
+#include "utils/usercontext.h"
 
 
 /* non-export function prototypes */
@@ -574,6 +575,7 @@ DefineIndex(Oid relationId,
 	int			root_save_nestlevel;
 
 	root_save_nestlevel = NewGUCNestLevel();
+	RestrictSearchPath();
 
 	/*
 	 * Some callers need us to run with an empty default_tablespace; this is a
@@ -1316,6 +1318,7 @@ DefineIndex(Oid relationId,
 				SetUserIdAndSecContext(childrel->rd_rel->relowner,
 									   child_save_sec_context | SECURITY_RESTRICTED_OPERATION);
 				child_save_nestlevel = NewGUCNestLevel();
+				RestrictSearchPath();
 
 				/*
 				 * Don't try to create indexes on foreign tables, though. Skip
@@ -2038,6 +2041,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
 			{
 				SetUserIdAndSecContext(save_userid, save_sec_context);
 				*ddl_save_nestlevel = NewGUCNestLevel();
+				RestrictSearchPath();
 			}
 		}
 
@@ -2085,6 +2089,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
 		{
 			SetUserIdAndSecContext(save_userid, save_sec_context);
 			*ddl_save_nestlevel = NewGUCNestLevel();
+			RestrictSearchPath();
 		}
 
 		/*
@@ -2115,6 +2120,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
 			{
 				SetUserIdAndSecContext(save_userid, save_sec_context);
 				*ddl_save_nestlevel = NewGUCNestLevel();
+				RestrictSearchPath();
 			}
 
 			/*
@@ -3778,6 +3784,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 		SetUserIdAndSecContext(heapRel->rd_rel->relowner,
 							   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 		save_nestlevel = NewGUCNestLevel();
+		RestrictSearchPath();
 
 		/* 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 ac2e74fa3f..c2f4c28c51 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -45,6 +45,7 @@
 #include "utils/rel.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
+#include "utils/usercontext.h"
 
 
 typedef struct
@@ -178,6 +179,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 	SetUserIdAndSecContext(relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	RestrictSearchPath();
 
 	/* 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 69ac276687..a791bd9d42 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -61,6 +61,7 @@
 #include "utils/pg_rusage.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
+#include "utils/usercontext.h"
 
 
 /*
@@ -2187,6 +2188,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	SetUserIdAndSecContext(rel->rd_rel->relowner,
 						   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 	save_nestlevel = NewGUCNestLevel();
+	RestrictSearchPath();
 
 	/*
 	 * If PROCESS_MAIN is set (the default), it's time to vacuum the main
diff --git a/src/backend/utils/init/usercontext.c b/src/backend/utils/init/usercontext.c
index dd9a0dd6a8..cd037c0c6a 100644
--- a/src/backend/utils/init/usercontext.c
+++ b/src/backend/utils/init/usercontext.c
@@ -90,3 +90,17 @@ RestoreUserContext(UserContext *context)
 		AtEOXact_GUC(false, context->save_nestlevel);
 	SetUserIdAndSecContext(context->save_userid, context->save_sec_context);
 }
+
+/*
+ * Set search_path to a safe and consistent value. The caller should have
+ * already called NewGUCNestLevel().
+ *
+ * This is important when executing code as another user; for example, when
+ * running a maintenance command on a table with a functional index.
+ */
+void
+RestrictSearchPath()
+{
+	set_config_option("search_path", "pg_catalog, pg_temp", PGC_USERSET,
+					  PGC_S_SESSION, GUC_ACTION_SAVE, true, 0, false);
+}
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index eccfcc54a1..f91b5127a8 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -109,15 +109,11 @@ $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);
 |);
 $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/usercontext.h b/src/include/utils/usercontext.h
index a8195c194d..548980ced8 100644
--- a/src/include/utils/usercontext.h
+++ b/src/include/utils/usercontext.h
@@ -22,5 +22,6 @@ typedef struct UserContext
 /* Function prototypes. */
 extern void SwitchToUntrustedUser(Oid userid, UserContext *context);
 extern void RestoreUserContext(UserContext *context);
+extern void RestrictSearchPath(void);
 
 #endif							/* USERCONTEXT_H */
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 7cb05827ca..f03aebf10a 100644
--- a/src/test/regress/expected/matview.out
+++ b/src/test/regress/expected/matview.out
@@ -631,12 +631,50 @@ REFRESH MATERIALIZED VIEW matview_schema.mv_nodata2;
 RESET SESSION AUTHORIZATION;
 ALTER DEFAULT PRIVILEGES FOR ROLE regress_matview_user
   GRANT INSERT ON TABLES TO regress_matview_user;
+-- Test that MV is populated using restricted search_path
+SET SESSION AUTHORIZATION regress_matview_user;
+SET search_path = matview_schema;
+CREATE FUNCTION matview_schema.mod(a INT, b INT) RETURNS INT
+  IMMUTABLE LANGUAGE plpgsql AS
+  $$ BEGIN RETURN 42; END; $$;
+CREATE FUNCTION matview_schema.mod10(i INT) RETURNS INT
+  IMMUTABLE LANGUAGE plpgsql AS
+  $$ BEGIN RETURN mod(i, 10); END; $$;
+CREATE MATERIALIZED VIEW matview_search_path AS
+  SELECT current_user, matview_schema.mod10(33);
+SELECT * FROM matview_search_path;
+     current_user     | mod10 
+----------------------+-------
+ regress_matview_user |     3
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+REFRESH MATERIALIZED VIEW matview_search_path;
+SELECT * FROM matview_search_path;
+     current_user     | mod10 
+----------------------+-------
+ regress_matview_user |     3
+(1 row)
+
+ALTER FUNCTION matview_schema.mod10(INT)
+  SET search_path = matview_schema, pg_catalog;
+REFRESH MATERIALIZED VIEW matview_search_path;
+SELECT * FROM matview_search_path;
+     current_user     | mod10 
+----------------------+-------
+ regress_matview_user |    42
+(1 row)
+
+RESET search_path;
 DROP SCHEMA matview_schema CASCADE;
-NOTICE:  drop cascades to 4 other objects
+NOTICE:  drop cascades to 7 other objects
 DETAIL:  drop cascades to materialized view matview_schema.mv_withdata1
 drop cascades to materialized view matview_schema.mv_withdata2
 drop cascades to materialized view matview_schema.mv_nodata1
 drop cascades to materialized view matview_schema.mv_nodata2
+drop cascades to function matview_schema.mod(integer,integer)
+drop cascades to function matview_schema.mod10(integer)
+drop cascades to materialized view matview_schema.matview_search_path
 DROP USER regress_matview_user;
 -- CREATE MATERIALIZED VIEW ... IF NOT EXISTS
 CREATE MATERIALIZED VIEW matview_ine_tab AS SELECT 1;
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index c1e610e62f..1c36d7f894 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 acc4519d01..9c026be325 100644
--- a/src/test/regress/sql/matview.sql
+++ b/src/test/regress/sql/matview.sql
@@ -270,6 +270,29 @@ RESET SESSION AUTHORIZATION;
 ALTER DEFAULT PRIVILEGES FOR ROLE regress_matview_user
   GRANT INSERT ON TABLES TO regress_matview_user;
 
+-- Test that MV is populated using restricted search_path
+SET SESSION AUTHORIZATION regress_matview_user;
+SET search_path = matview_schema;
+CREATE FUNCTION matview_schema.mod(a INT, b INT) RETURNS INT
+  IMMUTABLE LANGUAGE plpgsql AS
+  $$ BEGIN RETURN 42; END; $$;
+CREATE FUNCTION matview_schema.mod10(i INT) RETURNS INT
+  IMMUTABLE LANGUAGE plpgsql AS
+  $$ BEGIN RETURN mod(i, 10); END; $$;
+CREATE MATERIALIZED VIEW matview_search_path AS
+  SELECT current_user, matview_schema.mod10(33);
+SELECT * FROM matview_search_path;
+RESET SESSION AUTHORIZATION;
+REFRESH MATERIALIZED VIEW matview_search_path;
+SELECT * FROM matview_search_path;
+
+ALTER FUNCTION matview_schema.mod10(INT)
+  SET search_path = matview_schema, pg_catalog;
+REFRESH MATERIALIZED VIEW matview_search_path;
+SELECT * FROM matview_search_path;
+
+RESET search_path;
+
 DROP SCHEMA matview_schema CASCADE;
 DROP USER regress_matview_user;
 
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index bf0035d96d..8eed1f6735 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



^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2023-07-22 14:04  Noah Misch <[email protected]>
  parent: Jeff Davis <[email protected]>
  2 siblings, 1 reply; 21+ messages in thread

From: Noah Misch @ 2023-07-22 14:04 UTC (permalink / raw)
  To: Jeff Davis <[email protected]>; +Cc: Nathan Bossart <[email protected]>; David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Fri, Jul 21, 2023 at 03:32:43PM -0700, Jeff Davis wrote:
> Why do we switch to the table owner and use
> SECURITY_RESTRICTED_OPERATION in DefineIndex(), when we will switch in
> index_build (etc.) anyway?

Commit a117ceb added that, and it added some test cases that behaved
differently without that.

> Similarly, why do we switch in vacuum_rel(),
> when it doesn't matter for lazy vacuum and we will switch in
> cluster_rel() and do_analyze_rel() anyway?

It conforms to the "as soon as possible after locking the relation" coding
rule that commit a117ceb wrote into miscinit.c.  That provides future
proofing.






^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2023-07-22 18:52  Jeff Davis <[email protected]>
  parent: Noah Misch <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Jeff Davis @ 2023-07-22 18:52 UTC (permalink / raw)
  To: Noah Misch <[email protected]>; +Cc: Nathan Bossart <[email protected]>; David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Sat, 2023-07-22 at 07:04 -0700, Noah Misch wrote:
> Commit a117ceb added that, and it added some test cases that behaved
> differently without that.

Thank you. The reasoning there seems to apply to search_path
restriction as well, so I will leave it as-is.

I'll wait a few more days for comment since I made some changes (also
it's the weekend), but I plan to commit something like the latest
version soon.

I might adjust the CREATE MATERIALIZED VIEW changes to be more minimal,
but that path is not important for security (see pre-existing comment)
so it's probably fine either way.

Regards,
	Jeff Davis







^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH 1/1] bump MESSAGE_WIDTH to 62
@ 2023-08-23 16:31  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Nathan Bossart @ 2023-08-23 16:31 UTC (permalink / raw)

---
 src/bin/pg_upgrade/pg_upgrade.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 3eea0139c7..7afa96716e 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -22,7 +22,7 @@
 #define MAX_STRING			1024
 #define QUERY_ALLOC			8192
 
-#define MESSAGE_WIDTH		60
+#define MESSAGE_WIDTH		62
 
 #define GET_MAJOR_VERSION(v)	((v) / 100)
 
-- 
2.25.1


--PEIAKu/WMn1b1Hv9--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2023-10-27 23:04  Jeff Davis <[email protected]>
  parent: Jeff Davis <[email protected]>
  2 siblings, 2 replies; 21+ messages in thread

From: Jeff Davis @ 2023-10-27 23:04 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; Noah Misch <[email protected]>; +Cc: David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Fri, 2023-07-21 at 15:32 -0700, Jeff Davis wrote:
> Attached is a new version.

Do we still want to do this?

Right now, the MAINTAIN privilege is blocking on some way to prevent
malicious users from abusing the MAINTAIN privilege and search_path to
acquire the table owner's privileges.

The approach of locking down search_path during maintenance commands
would solve the problem, but it means that we are enforcing search_path
in some contexts and not others. That's not great, but it's similar to
what we are doing when we ignore SECURITY INVOKER and run the function
as the table owner during a maintenance command, or (by default) for
subscriptions.

My attempts to more generally try to lock down search_path for
functions attached to tables didn't seem to get much consensus, so if
we do make an exception to lock down search_path for maintenance
commands only, it would stay an exception for the foreseeable future.

Thoughts?

Regards,
	Jeff Davis







^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2023-10-31 16:58  Nathan Bossart <[email protected]>
  parent: Jeff Davis <[email protected]>
  1 sibling, 0 replies; 21+ messages in thread

From: Nathan Bossart @ 2023-10-31 16:58 UTC (permalink / raw)
  To: Jeff Davis <[email protected]>; +Cc: Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Fri, Oct 27, 2023 at 04:04:26PM -0700, Jeff Davis wrote:
> Do we still want to do this?
> 
> Right now, the MAINTAIN privilege is blocking on some way to prevent
> malicious users from abusing the MAINTAIN privilege and search_path to
> acquire the table owner's privileges.

I vote +1 for proceeding with this.  You've been threatening to commit this
since July, and from a quick skim, I don't sense any sustained objections.
Given one of the main objections for v16 was the timing, I would rather
commit this relatively early in the v17 cycle so we have ample time to deal
with any breakage it reveals or to further discuss any nuances.

Of course, I am a bit biased because I would love to un-revert MAINTAIN,
but I believe others would like to see that un-reverted, too.

> The approach of locking down search_path during maintenance commands
> would solve the problem, but it means that we are enforcing search_path
> in some contexts and not others. That's not great, but it's similar to
> what we are doing when we ignore SECURITY INVOKER and run the function
> as the table owner during a maintenance command, or (by default) for
> subscriptions.

Given the experience gained from the 2018 security fixes [0], I think this
is okay.

[0] https://postgr.es/m/20230715211333.GB3675150%40rfd.leadboat.com

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com






^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2023-10-31 17:16  Isaac Morland <[email protected]>
  parent: Jeff Davis <[email protected]>
  1 sibling, 2 replies; 21+ messages in thread

From: Isaac Morland @ 2023-10-31 17:16 UTC (permalink / raw)
  To: Jeff Davis <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Fri, 27 Oct 2023 at 19:04, Jeff Davis <[email protected]> wrote:

The approach of locking down search_path during maintenance commands
> would solve the problem, but it means that we are enforcing search_path
> in some contexts and not others. That's not great, but it's similar to
> what we are doing when we ignore SECURITY INVOKER and run the function
> as the table owner during a maintenance command, or (by default) for
> subscriptions.
>

I don't agree that this is ignoring SECURITY INVOKER. Instead, I see it as
running the maintenance command as the owner of the table, which is
therefore the invoker of the function. As far as I can tell we need to do
this for security anyway - otherwise as soon as superuser runs a
maintenance command (which it can already do), the owner of any function
called in the course of the maintenance operation has an opportunity to get
superuser.

For that matter, what would it even mean to ignore SECURITY INVOKER? Run
the function as its owner if it were SECURITY DEFINER?

I understand what ignoring SECURITY DEFINER would mean: obviously, don't
adjust the current user on entry and exit.

The privilege boundary should be at the point where the maintenance command
starts: the role with MAINTAIN privilege gets to kick off maintenance, but
doesn't get to specify anything after that, including the search_path (of
course, function execution search paths should not normally depend on the
caller's search path anyway, but that's a bigger discussion with an
unfortunate backward compatibility problem).

Perhaps the search_path for running a maintenance command should be the
search_path set for the table owner (ALTER ROLE … SET search_path …)? If
none set, the default "$user", public. After that, change search_path on
function invocation as usual rather than having special rules for what
happens when a function is invoked during a maintenance command.

My attempts to more generally try to lock down search_path for
> functions attached to tables didn't seem to get much consensus, so if
> we do make an exception to lock down search_path for maintenance
> commands only, it would stay an exception for the foreseeable future.
>


^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2023-11-02 18:21  Jeff Davis <[email protected]>
  parent: Isaac Morland <[email protected]>
  1 sibling, 1 reply; 21+ messages in thread

From: Jeff Davis @ 2023-11-02 18:21 UTC (permalink / raw)
  To: Isaac Morland <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Tue, 2023-10-31 at 13:16 -0400, Isaac Morland wrote:

> Perhaps the search_path for running a maintenance command should be
> the search_path set for the table owner (ALTER ROLE … SET search_path
> …)?

That's an interesting idea; I hadn't considered that, or at least not
very deeply. I feel like it came up before but I can't remember what
(if anything) was wrong with it.

If we expanded this idea a bit, I could imagine it applying to SECURITY
DEFINER functions as well, and that would make writing SECURITY DEFINER
functions a lot safer.

I was earlier pushing for search_path to be tied to the function (in my
"CREATE FUNCTION ... SEARCH" proposal) on the grounds that the author
(usually) doesn't want the behavior to depend on the caller's
search_path. That proposal didn't go very well because it required
extra DDL.

But if we tie the search_path to the user-switching behavior rather
than the function, that still protects the function author from many
sorts of search_path attacks, because it's either running as the
function author with the function author's search_path; or running as
the invoking user with their search_path. And there aren't a lot of
cases where a function author would want it to run with their
privileges and the caller's search_path.

[ It would still leave open the problem of a SECURITY INVOKER function
in an index expression returning inconsistent results due to a changing
search_path, which would compromise the index structure and any
constraints using that index. But that problem is more bounded, at
least. ]

>  After that, change search_path on function invocation as usual
> rather than having special rules for what happens when a function is
> invoked during a maintenance command.

I don't follow what you mean here.

Regards,
	Jeff Davis







^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2023-11-06 20:31  Isaac Morland <[email protected]>
  parent: Jeff Davis <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Isaac Morland @ 2023-11-06 20:31 UTC (permalink / raw)
  To: Jeff Davis <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Thu, 2 Nov 2023 at 14:22, Jeff Davis <[email protected]> wrote:

> On Tue, 2023-10-31 at 13:16 -0400, Isaac Morland wrote:
>
> > Perhaps the search_path for running a maintenance command should be
> > the search_path set for the table owner (ALTER ROLE … SET search_path
> > …)?
>
> That's an interesting idea; I hadn't considered that, or at least not
> very deeply. I feel like it came up before but I can't remember what
> (if anything) was wrong with it.
>
> If we expanded this idea a bit, I could imagine it applying to SECURITY
> DEFINER functions as well, and that would make writing SECURITY DEFINER
> functions a lot safer.
>

I still think the right default is that CREATE FUNCTION stores the
search_path in effect when it runs with the function, and that is the
search_path used to run the function (and don't "BEGIN ATOMIC" functions
partially work this way already?). But I suggest the owner search_path as
an option which is clearly better than using the caller's search_path in
most cases.

I think the problems are essentially the same for security invoker vs.
security definer. The difference is that the problems are security problems
only for security definers.

>  After that, change search_path on function invocation as usual
> > rather than having special rules for what happens when a function is
> > invoked during a maintenance command.
>
> I don't follow what you mean here.
>

I’m referring to the idea that the search_path during function execution
should be determined at function creation time (or, at least, not at
function execution time). While this is a security requirement for security
definer functions, I think it’s what is wanted about 99.9% of the time
for security invoker functions as well. So when the maintenance command
ends up running a function, the search_path in effect during the function
execution will be the one established at function definition time; or if we
go with this "search_path associated with function owner" idea, then again
the search_path is determined by the usual rule (function owner), rather
than by any special rules associated with maintenance commands.


^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2023-11-07 21:47  Jeff Davis <[email protected]>
  parent: Isaac Morland <[email protected]>
  1 sibling, 0 replies; 21+ messages in thread

From: Jeff Davis @ 2023-11-07 21:47 UTC (permalink / raw)
  To: Isaac Morland <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Tue, 2023-10-31 at 13:16 -0400, Isaac Morland wrote:
> Perhaps the search_path for running a maintenance command should be
> the search_path set for the table owner (ALTER ROLE … SET search_path
> …)?

After some thought, I don't think that's the right approach. It adds
another way search path can be changed, which adds to the complexity.

Also, by default it's "$user", public; and given that "public" was
world-writable until recently, that doesn't seem like a good idea for a
change intended to prevent search_path manipulation.

Regards,
	Jeff Davis







^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2024-01-18 03:54  Shubham Khanna <[email protected]>
  parent: Jeff Davis <[email protected]>
  2 siblings, 1 reply; 21+ messages in thread

From: Shubham Khanna @ 2024-01-18 03:54 UTC (permalink / raw)
  To: Jeff Davis <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Thu, Jan 18, 2024 at 9:19 AM Jeff Davis <[email protected]> wrote:
>
> On Mon, 2023-07-17 at 12:16 -0700, Jeff Davis wrote:
> > Based on feedback, I plan to commit soon.
>
> Attached is a new version.
>
> Changes:
>
> * Also switch the search_path during CREATE MATERIALIZED VIEW, so that
> it's consistent with REFRESH. As a part of this change, I slightly
> reordered things in ExecCreateTableAs() so that the skipData path
> returns early without entering the SECURITY_RESTRICTED_OPERATION. I
> don't think that's a problem because (a) that is one place where
> SECURITY_RESTRICTED_OPERATION is not used for security, but rather for
> consistency; and (b) that path doesn't go through rewriter, planner, or
> executor anyway so I don't see why it would matter.
>
> * Use GUC_ACTION_SAVE rather than GUC_ACTION_SET. That was a problem
> with the previous patch for index functions executed in parallel
> workers, which can happen calling SQL functions from pg_amcheck.
>
> * I used a wrapper function RestrictSearchPath() rather than calling
> set_config_option() directly. That provides a nice place in case we
> need to add a compatibility GUC to disable it.
>
> Question:
>
> Why do we switch to the table owner and use
> SECURITY_RESTRICTED_OPERATION in DefineIndex(), when we will switch in
> index_build (etc.) anyway? Similarly, why do we switch in vacuum_rel(),
> when it doesn't matter for lazy vacuum and we will switch in
> cluster_rel() and do_analyze_rel() anyway?

I tried to apply the patch but it is failing at the Head. It is giving
the following error:
Hunk #7 succeeded at 3772 (offset -12 lines).
patching file src/backend/commands/matview.c
patching file src/backend/commands/vacuum.c
Hunk #2 succeeded at 2169 (offset -19 lines).
patching file src/backend/utils/init/usercontext.c
patching file src/bin/scripts/t/100_vacuumdb.pl
Hunk #1 FAILED at 109.
1 out of 1 hunk FAILED -- saving rejects to file
src/bin/scripts/t/100_vacuumdb.pl.rej
patching file src/include/utils/usercontext.h
patching file src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
patching file src/test/regress/expected/matview.out
patching file src/test/regress/expected/privileges.out
patching file src/test/regress/expected/vacuum.out
patching file src/test/regress/sql/matview.sql
patching file src/test/regress/sql/privileges.sql
patching file src/test/regress/sql/vacuum.sql

Please send the Re-base version of the patch.

Thanks and Regards,
Shubham Khanna.





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Fix search_path for all maintenance commands
@ 2024-01-18 05:11  Jeff Davis <[email protected]>
  parent: Shubham Khanna <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Jeff Davis @ 2024-01-18 05:11 UTC (permalink / raw)
  To: Shubham Khanna <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>

On Thu, 2024-01-18 at 09:24 +0530, Shubham Khanna wrote:
> I tried to apply the patch but it is failing at the Head. It is
> giving
> the following error:

I am withdrawing this patch from the CF until it's more clear that this
is what we really want to do.

Thank you for looking into it.

Regards,
	Jeff Davis






^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH] rework locking code in GetMultiXactIdMembers
@ 2024-03-03 14:20  Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw)

Per Coverity
---
 src/backend/access/transam/multixact.c | 52 ++++++++++----------------
 1 file changed, 20 insertions(+), 32 deletions(-)

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 9b81506145..ec446949a9 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1250,14 +1250,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
 	MultiXactOffset offset;
 	int			length;
 	int			truelength;
-	int			i;
 	MultiXactId oldestMXact;
 	MultiXactId nextMXact;
 	MultiXactId tmpMXact;
 	MultiXactOffset nextOffset;
 	MultiXactMember *ptr;
 	LWLock	   *lock;
-	LWLock	   *prevlock = NULL;
 
 	debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
 
@@ -1364,18 +1362,9 @@ retry:
 	pageno = MultiXactIdToOffsetPage(multi);
 	entryno = MultiXactIdToOffsetEntry(multi);
 
-	/*
-	 * If this page falls under a different bank, release the old bank's lock
-	 * and acquire the lock of the new bank.
-	 */
+	/* Acquire the bank lock for the page we need. */
 	lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-	if (lock != prevlock)
-	{
-		if (prevlock != NULL)
-			LWLockRelease(prevlock);
-		LWLockAcquire(lock, LW_EXCLUSIVE);
-		prevlock = lock;
-	}
+	LWLockAcquire(lock, LW_EXCLUSIVE);
 
 	slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
 	offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
@@ -1410,17 +1399,19 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-			if (prevlock != lock)
+			newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
+			if (newlock != lock)
 			{
-				LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 			slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact);
 		}
@@ -1432,8 +1423,8 @@ retry:
 		if (nextMXOffset == 0)
 		{
 			/* Corner case 2: next multixact is still being filled in */
-			LWLockRelease(prevlock);
-			prevlock = NULL;
+			LWLockRelease(lock);
+			lock = NULL;
 			CHECK_FOR_INTERRUPTS();
 			pg_usleep(1000L);
 			goto retry;
@@ -1442,14 +1433,11 @@ retry:
 		length = nextMXOffset - offset;
 	}
 
-	LWLockRelease(prevlock);
-	prevlock = NULL;
-
 	ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
 
 	truelength = 0;
 	prev_pageno = -1;
-	for (i = 0; i < length; i++, offset++)
+	for (int i = 0; i < length; i++, offset++)
 	{
 		TransactionId *xactptr;
 		uint32	   *flagsptr;
@@ -1462,18 +1450,19 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
-			if (lock != prevlock)
+			newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
+			if (newlock != lock)
 			{
-				if (prevlock)
-					LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 
 			slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
@@ -1499,8 +1488,7 @@ retry:
 		truelength++;
 	}
 
-	if (prevlock)
-		LWLockRelease(prevlock);
+	LWLockRelease(lock);
 
 	/* A multixid with zero members should not happen */
 	Assert(truelength > 0);
-- 
2.39.2


--rb5vb6debo3bx6lt--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers
@ 2024-03-03 14:20  Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw)

Per Coverity
---
 src/backend/access/transam/multixact.c | 53 +++++++++++---------------
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index cd476b94fa..83b578dced 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1247,14 +1247,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
 	MultiXactOffset offset;
 	int			length;
 	int			truelength;
-	int			i;
 	MultiXactId oldestMXact;
 	MultiXactId nextMXact;
 	MultiXactId tmpMXact;
 	MultiXactOffset nextOffset;
 	MultiXactMember *ptr;
 	LWLock	   *lock;
-	LWLock	   *prevlock = NULL;
 
 	debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
 
@@ -1361,18 +1359,9 @@ retry:
 	pageno = MultiXactIdToOffsetPage(multi);
 	entryno = MultiXactIdToOffsetEntry(multi);
 
-	/*
-	 * If this page falls under a different bank, release the old bank's lock
-	 * and acquire the lock of the new bank.
-	 */
+	/* Acquire the bank lock for the page we need. */
 	lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-	if (lock != prevlock)
-	{
-		if (prevlock != NULL)
-			LWLockRelease(prevlock);
-		LWLockAcquire(lock, LW_EXCLUSIVE);
-		prevlock = lock;
-	}
+	LWLockAcquire(lock, LW_EXCLUSIVE);
 
 	slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
 	offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
@@ -1407,17 +1396,19 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-			if (prevlock != lock)
+			newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
+			if (newlock != lock)
 			{
-				LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 			slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact);
 		}
@@ -1429,8 +1420,7 @@ retry:
 		if (nextMXOffset == 0)
 		{
 			/* Corner case 2: next multixact is still being filled in */
-			LWLockRelease(prevlock);
-			prevlock = NULL;
+			LWLockRelease(lock);
 			CHECK_FOR_INTERRUPTS();
 			pg_usleep(1000L);
 			goto retry;
@@ -1439,14 +1429,14 @@ retry:
 		length = nextMXOffset - offset;
 	}
 
-	LWLockRelease(prevlock);
-	prevlock = NULL;
+	LWLockRelease(lock);
+	lock = NULL;
 
 	ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
 
 	truelength = 0;
 	prev_pageno = -1;
-	for (i = 0; i < length; i++, offset++)
+	for (int i = 0; i < length; i++, offset++)
 	{
 		TransactionId *xactptr;
 		uint32	   *flagsptr;
@@ -1459,18 +1449,20 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
-			if (lock != prevlock)
+			newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
+			if (newlock != lock)
 			{
-				if (prevlock)
-					LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				if (lock)
+					LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 
 			slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
@@ -1496,8 +1488,7 @@ retry:
 		truelength++;
 	}
 
-	if (prevlock)
-		LWLockRelease(prevlock);
+	LWLockRelease(lock);
 
 	/* A multixid with zero members should not happen */
 	Assert(truelength > 0);
-- 
2.39.2


--gflnge3qfnp343ni
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v2-0002-Rework-redundant-loop-in-subtrans.c.patch"



^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH] rework locking code in GetMultiXactIdMembers
@ 2024-03-03 14:20  Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw)

Per Coverity
---
 src/backend/access/transam/multixact.c | 52 ++++++++++----------------
 1 file changed, 20 insertions(+), 32 deletions(-)

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 9b81506145..ec446949a9 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1250,14 +1250,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
 	MultiXactOffset offset;
 	int			length;
 	int			truelength;
-	int			i;
 	MultiXactId oldestMXact;
 	MultiXactId nextMXact;
 	MultiXactId tmpMXact;
 	MultiXactOffset nextOffset;
 	MultiXactMember *ptr;
 	LWLock	   *lock;
-	LWLock	   *prevlock = NULL;
 
 	debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
 
@@ -1364,18 +1362,9 @@ retry:
 	pageno = MultiXactIdToOffsetPage(multi);
 	entryno = MultiXactIdToOffsetEntry(multi);
 
-	/*
-	 * If this page falls under a different bank, release the old bank's lock
-	 * and acquire the lock of the new bank.
-	 */
+	/* Acquire the bank lock for the page we need. */
 	lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-	if (lock != prevlock)
-	{
-		if (prevlock != NULL)
-			LWLockRelease(prevlock);
-		LWLockAcquire(lock, LW_EXCLUSIVE);
-		prevlock = lock;
-	}
+	LWLockAcquire(lock, LW_EXCLUSIVE);
 
 	slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
 	offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
@@ -1410,17 +1399,19 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-			if (prevlock != lock)
+			newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
+			if (newlock != lock)
 			{
-				LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 			slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact);
 		}
@@ -1432,8 +1423,8 @@ retry:
 		if (nextMXOffset == 0)
 		{
 			/* Corner case 2: next multixact is still being filled in */
-			LWLockRelease(prevlock);
-			prevlock = NULL;
+			LWLockRelease(lock);
+			lock = NULL;
 			CHECK_FOR_INTERRUPTS();
 			pg_usleep(1000L);
 			goto retry;
@@ -1442,14 +1433,11 @@ retry:
 		length = nextMXOffset - offset;
 	}
 
-	LWLockRelease(prevlock);
-	prevlock = NULL;
-
 	ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
 
 	truelength = 0;
 	prev_pageno = -1;
-	for (i = 0; i < length; i++, offset++)
+	for (int i = 0; i < length; i++, offset++)
 	{
 		TransactionId *xactptr;
 		uint32	   *flagsptr;
@@ -1462,18 +1450,19 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
-			if (lock != prevlock)
+			newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
+			if (newlock != lock)
 			{
-				if (prevlock)
-					LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 
 			slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
@@ -1499,8 +1488,7 @@ retry:
 		truelength++;
 	}
 
-	if (prevlock)
-		LWLockRelease(prevlock);
+	LWLockRelease(lock);
 
 	/* A multixid with zero members should not happen */
 	Assert(truelength > 0);
-- 
2.39.2


--rb5vb6debo3bx6lt--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers
@ 2024-03-03 14:20  Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw)

Per Coverity
---
 src/backend/access/transam/multixact.c | 53 +++++++++++---------------
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index cd476b94fa..83b578dced 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1247,14 +1247,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
 	MultiXactOffset offset;
 	int			length;
 	int			truelength;
-	int			i;
 	MultiXactId oldestMXact;
 	MultiXactId nextMXact;
 	MultiXactId tmpMXact;
 	MultiXactOffset nextOffset;
 	MultiXactMember *ptr;
 	LWLock	   *lock;
-	LWLock	   *prevlock = NULL;
 
 	debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
 
@@ -1361,18 +1359,9 @@ retry:
 	pageno = MultiXactIdToOffsetPage(multi);
 	entryno = MultiXactIdToOffsetEntry(multi);
 
-	/*
-	 * If this page falls under a different bank, release the old bank's lock
-	 * and acquire the lock of the new bank.
-	 */
+	/* Acquire the bank lock for the page we need. */
 	lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-	if (lock != prevlock)
-	{
-		if (prevlock != NULL)
-			LWLockRelease(prevlock);
-		LWLockAcquire(lock, LW_EXCLUSIVE);
-		prevlock = lock;
-	}
+	LWLockAcquire(lock, LW_EXCLUSIVE);
 
 	slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
 	offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
@@ -1407,17 +1396,19 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-			if (prevlock != lock)
+			newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
+			if (newlock != lock)
 			{
-				LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 			slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact);
 		}
@@ -1429,8 +1420,7 @@ retry:
 		if (nextMXOffset == 0)
 		{
 			/* Corner case 2: next multixact is still being filled in */
-			LWLockRelease(prevlock);
-			prevlock = NULL;
+			LWLockRelease(lock);
 			CHECK_FOR_INTERRUPTS();
 			pg_usleep(1000L);
 			goto retry;
@@ -1439,14 +1429,14 @@ retry:
 		length = nextMXOffset - offset;
 	}
 
-	LWLockRelease(prevlock);
-	prevlock = NULL;
+	LWLockRelease(lock);
+	lock = NULL;
 
 	ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
 
 	truelength = 0;
 	prev_pageno = -1;
-	for (i = 0; i < length; i++, offset++)
+	for (int i = 0; i < length; i++, offset++)
 	{
 		TransactionId *xactptr;
 		uint32	   *flagsptr;
@@ -1459,18 +1449,20 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
-			if (lock != prevlock)
+			newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
+			if (newlock != lock)
 			{
-				if (prevlock)
-					LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				if (lock)
+					LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 
 			slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
@@ -1496,8 +1488,7 @@ retry:
 		truelength++;
 	}
 
-	if (prevlock)
-		LWLockRelease(prevlock);
+	LWLockRelease(lock);
 
 	/* A multixid with zero members should not happen */
 	Assert(truelength > 0);
-- 
2.39.2


--gflnge3qfnp343ni
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v2-0002-Rework-redundant-loop-in-subtrans.c.patch"



^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH] rework locking code in GetMultiXactIdMembers
@ 2024-03-03 14:20  Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw)

Per Coverity
---
 src/backend/access/transam/multixact.c | 52 ++++++++++----------------
 1 file changed, 20 insertions(+), 32 deletions(-)

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 9b81506145..ec446949a9 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1250,14 +1250,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
 	MultiXactOffset offset;
 	int			length;
 	int			truelength;
-	int			i;
 	MultiXactId oldestMXact;
 	MultiXactId nextMXact;
 	MultiXactId tmpMXact;
 	MultiXactOffset nextOffset;
 	MultiXactMember *ptr;
 	LWLock	   *lock;
-	LWLock	   *prevlock = NULL;
 
 	debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
 
@@ -1364,18 +1362,9 @@ retry:
 	pageno = MultiXactIdToOffsetPage(multi);
 	entryno = MultiXactIdToOffsetEntry(multi);
 
-	/*
-	 * If this page falls under a different bank, release the old bank's lock
-	 * and acquire the lock of the new bank.
-	 */
+	/* Acquire the bank lock for the page we need. */
 	lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-	if (lock != prevlock)
-	{
-		if (prevlock != NULL)
-			LWLockRelease(prevlock);
-		LWLockAcquire(lock, LW_EXCLUSIVE);
-		prevlock = lock;
-	}
+	LWLockAcquire(lock, LW_EXCLUSIVE);
 
 	slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
 	offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
@@ -1410,17 +1399,19 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-			if (prevlock != lock)
+			newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
+			if (newlock != lock)
 			{
-				LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 			slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact);
 		}
@@ -1432,8 +1423,8 @@ retry:
 		if (nextMXOffset == 0)
 		{
 			/* Corner case 2: next multixact is still being filled in */
-			LWLockRelease(prevlock);
-			prevlock = NULL;
+			LWLockRelease(lock);
+			lock = NULL;
 			CHECK_FOR_INTERRUPTS();
 			pg_usleep(1000L);
 			goto retry;
@@ -1442,14 +1433,11 @@ retry:
 		length = nextMXOffset - offset;
 	}
 
-	LWLockRelease(prevlock);
-	prevlock = NULL;
-
 	ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
 
 	truelength = 0;
 	prev_pageno = -1;
-	for (i = 0; i < length; i++, offset++)
+	for (int i = 0; i < length; i++, offset++)
 	{
 		TransactionId *xactptr;
 		uint32	   *flagsptr;
@@ -1462,18 +1450,19 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
-			if (lock != prevlock)
+			newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
+			if (newlock != lock)
 			{
-				if (prevlock)
-					LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 
 			slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
@@ -1499,8 +1488,7 @@ retry:
 		truelength++;
 	}
 
-	if (prevlock)
-		LWLockRelease(prevlock);
+	LWLockRelease(lock);
 
 	/* A multixid with zero members should not happen */
 	Assert(truelength > 0);
-- 
2.39.2


--rb5vb6debo3bx6lt--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH] rework locking code in GetMultiXactIdMembers
@ 2024-03-03 14:20  Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw)

Per Coverity
---
 src/backend/access/transam/multixact.c | 52 ++++++++++----------------
 1 file changed, 20 insertions(+), 32 deletions(-)

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 9b81506145..ec446949a9 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1250,14 +1250,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
 	MultiXactOffset offset;
 	int			length;
 	int			truelength;
-	int			i;
 	MultiXactId oldestMXact;
 	MultiXactId nextMXact;
 	MultiXactId tmpMXact;
 	MultiXactOffset nextOffset;
 	MultiXactMember *ptr;
 	LWLock	   *lock;
-	LWLock	   *prevlock = NULL;
 
 	debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
 
@@ -1364,18 +1362,9 @@ retry:
 	pageno = MultiXactIdToOffsetPage(multi);
 	entryno = MultiXactIdToOffsetEntry(multi);
 
-	/*
-	 * If this page falls under a different bank, release the old bank's lock
-	 * and acquire the lock of the new bank.
-	 */
+	/* Acquire the bank lock for the page we need. */
 	lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-	if (lock != prevlock)
-	{
-		if (prevlock != NULL)
-			LWLockRelease(prevlock);
-		LWLockAcquire(lock, LW_EXCLUSIVE);
-		prevlock = lock;
-	}
+	LWLockAcquire(lock, LW_EXCLUSIVE);
 
 	slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
 	offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
@@ -1410,17 +1399,19 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-			if (prevlock != lock)
+			newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
+			if (newlock != lock)
 			{
-				LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 			slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact);
 		}
@@ -1432,8 +1423,8 @@ retry:
 		if (nextMXOffset == 0)
 		{
 			/* Corner case 2: next multixact is still being filled in */
-			LWLockRelease(prevlock);
-			prevlock = NULL;
+			LWLockRelease(lock);
+			lock = NULL;
 			CHECK_FOR_INTERRUPTS();
 			pg_usleep(1000L);
 			goto retry;
@@ -1442,14 +1433,11 @@ retry:
 		length = nextMXOffset - offset;
 	}
 
-	LWLockRelease(prevlock);
-	prevlock = NULL;
-
 	ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
 
 	truelength = 0;
 	prev_pageno = -1;
-	for (i = 0; i < length; i++, offset++)
+	for (int i = 0; i < length; i++, offset++)
 	{
 		TransactionId *xactptr;
 		uint32	   *flagsptr;
@@ -1462,18 +1450,19 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
-			if (lock != prevlock)
+			newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
+			if (newlock != lock)
 			{
-				if (prevlock)
-					LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 
 			slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
@@ -1499,8 +1488,7 @@ retry:
 		truelength++;
 	}
 
-	if (prevlock)
-		LWLockRelease(prevlock);
+	LWLockRelease(lock);
 
 	/* A multixid with zero members should not happen */
 	Assert(truelength > 0);
-- 
2.39.2


--rb5vb6debo3bx6lt--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers
@ 2024-03-03 14:20  Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw)

Per Coverity
---
 src/backend/access/transam/multixact.c | 53 +++++++++++---------------
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index cd476b94fa..83b578dced 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1247,14 +1247,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
 	MultiXactOffset offset;
 	int			length;
 	int			truelength;
-	int			i;
 	MultiXactId oldestMXact;
 	MultiXactId nextMXact;
 	MultiXactId tmpMXact;
 	MultiXactOffset nextOffset;
 	MultiXactMember *ptr;
 	LWLock	   *lock;
-	LWLock	   *prevlock = NULL;
 
 	debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
 
@@ -1361,18 +1359,9 @@ retry:
 	pageno = MultiXactIdToOffsetPage(multi);
 	entryno = MultiXactIdToOffsetEntry(multi);
 
-	/*
-	 * If this page falls under a different bank, release the old bank's lock
-	 * and acquire the lock of the new bank.
-	 */
+	/* Acquire the bank lock for the page we need. */
 	lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-	if (lock != prevlock)
-	{
-		if (prevlock != NULL)
-			LWLockRelease(prevlock);
-		LWLockAcquire(lock, LW_EXCLUSIVE);
-		prevlock = lock;
-	}
+	LWLockAcquire(lock, LW_EXCLUSIVE);
 
 	slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
 	offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
@@ -1407,17 +1396,19 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
-			if (prevlock != lock)
+			newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
+			if (newlock != lock)
 			{
-				LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 			slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact);
 		}
@@ -1429,8 +1420,7 @@ retry:
 		if (nextMXOffset == 0)
 		{
 			/* Corner case 2: next multixact is still being filled in */
-			LWLockRelease(prevlock);
-			prevlock = NULL;
+			LWLockRelease(lock);
 			CHECK_FOR_INTERRUPTS();
 			pg_usleep(1000L);
 			goto retry;
@@ -1439,14 +1429,14 @@ retry:
 		length = nextMXOffset - offset;
 	}
 
-	LWLockRelease(prevlock);
-	prevlock = NULL;
+	LWLockRelease(lock);
+	lock = NULL;
 
 	ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
 
 	truelength = 0;
 	prev_pageno = -1;
-	for (i = 0; i < length; i++, offset++)
+	for (int i = 0; i < length; i++, offset++)
 	{
 		TransactionId *xactptr;
 		uint32	   *flagsptr;
@@ -1459,18 +1449,20 @@ retry:
 
 		if (pageno != prev_pageno)
 		{
+			LWLock	   *newlock;
+
 			/*
 			 * Since we're going to access a different SLRU page, if this page
 			 * falls under a different bank, release the old bank's lock and
 			 * acquire the lock of the new bank.
 			 */
-			lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
-			if (lock != prevlock)
+			newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
+			if (newlock != lock)
 			{
-				if (prevlock)
-					LWLockRelease(prevlock);
-				LWLockAcquire(lock, LW_EXCLUSIVE);
-				prevlock = lock;
+				if (lock)
+					LWLockRelease(lock);
+				LWLockAcquire(newlock, LW_EXCLUSIVE);
+				lock = newlock;
 			}
 
 			slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
@@ -1496,8 +1488,7 @@ retry:
 		truelength++;
 	}
 
-	if (prevlock)
-		LWLockRelease(prevlock);
+	LWLockRelease(lock);
 
 	/* A multixid with zero members should not happen */
 	Assert(truelength > 0);
-- 
2.39.2


--gflnge3qfnp343ni
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v2-0002-Rework-redundant-loop-in-subtrans.c.patch"



^ permalink  raw  reply  [nested|flat] 21+ messages in thread

end of thread, other threads:[~2024-03-03 14:20 UTC | newest]

Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-07-17 17:58 Re: Fix search_path for all maintenance commands Nathan Bossart <[email protected]>
2023-07-17 19:16 ` Jeff Davis <[email protected]>
2023-07-21 22:32   ` Jeff Davis <[email protected]>
2023-07-22 14:04     ` Noah Misch <[email protected]>
2023-07-22 18:52       ` Jeff Davis <[email protected]>
2023-10-27 23:04     ` Jeff Davis <[email protected]>
2023-10-31 16:58       ` Nathan Bossart <[email protected]>
2023-10-31 17:16       ` Isaac Morland <[email protected]>
2023-11-02 18:21         ` Jeff Davis <[email protected]>
2023-11-06 20:31           ` Isaac Morland <[email protected]>
2023-11-07 21:47         ` Jeff Davis <[email protected]>
2024-01-18 03:54     ` Shubham Khanna <[email protected]>
2024-01-18 05:11       ` Jeff Davis <[email protected]>
2023-08-23 16:31 [PATCH 1/1] bump MESSAGE_WIDTH to 62 Nathan Bossart <[email protected]>
2024-03-03 14:20 [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]>
2024-03-03 14:20 [PATCH] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]>
2024-03-03 14:20 [PATCH] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]>
2024-03-03 14:20 [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]>
2024-03-03 14:20 [PATCH] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]>
2024-03-03 14:20 [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]>
2024-03-03 14:20 [PATCH] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox