public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH] Asymmetric partitionwise join.
26+ messages / 10 participants
[nested] [flat]

* [PATCH] Asymmetric partitionwise join.
@ 2021-04-02 06:02 Andrey Lepikhov <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Andrey Lepikhov @ 2021-04-02 06:02 UTC (permalink / raw)

Teach optimizer to consider partitionwise join of non-partitioned
table with each partition of partitioned table.

Disallow asymmetric machinery for joining of two partitioned (or appended)
relations because it could cause huge consumption of CPU and memory
during reparameterization of NestLoop path.

Change logic of the multilevel child relids adjustment, because this
feature allows the optimizer to plan in new way.
---
 src/backend/optimizer/path/joinpath.c        |   9 +
 src/backend/optimizer/path/joinrels.c        | 187 ++++++++
 src/backend/optimizer/plan/setrefs.c         |  17 +-
 src/backend/optimizer/util/appendinfo.c      |  44 +-
 src/backend/optimizer/util/pathnode.c        |   9 +-
 src/backend/optimizer/util/relnode.c         |  19 +-
 src/include/optimizer/paths.h                |   7 +-
 src/test/regress/expected/partition_join.out | 425 +++++++++++++++++++
 src/test/regress/sql/partition_join.sql      | 180 ++++++++
 9 files changed, 867 insertions(+), 30 deletions(-)

diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 6407ede12a..32618ebbd5 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -335,6 +335,15 @@ add_paths_to_joinrel(PlannerInfo *root,
 	if (set_join_pathlist_hook)
 		set_join_pathlist_hook(root, joinrel, outerrel, innerrel,
 							   jointype, &extra);
+
+	/*
+	 * 7. If outer relation is delivered from partition-tables, consider
+	 * distributing inner relation to every partition-leaf prior to
+	 * append these leafs.
+	 */
+	try_asymmetric_partitionwise_join(root, joinrel,
+									  outerrel, innerrel,
+									  jointype, &extra);
 }
 
 /*
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
index 8b69870cf4..9453258f83 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -16,6 +16,7 @@
 
 #include "miscadmin.h"
 #include "optimizer/appendinfo.h"
+#include "optimizer/cost.h"
 #include "optimizer/joininfo.h"
 #include "optimizer/pathnode.h"
 #include "optimizer/paths.h"
@@ -1552,6 +1553,192 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
 	}
 }
 
+/*
+ * Build RelOptInfo on JOIN of each partition of the outer relation and the inner
+ * relation. Return List of such RelOptInfo's. Return NIL, if at least one of
+ * these JOINs is impossible to build.
+ */
+static List *
+extract_asymmetric_partitionwise_subjoin(PlannerInfo *root,
+										 RelOptInfo *joinrel,
+										 AppendPath *append_path,
+										 RelOptInfo *inner_rel,
+										 JoinType jointype,
+										 JoinPathExtraData *extra)
+{
+	List		*result = NIL;
+	ListCell	*lc;
+
+	foreach (lc, append_path->subpaths)
+	{
+		Path			*child_path = lfirst(lc);
+		RelOptInfo		*child_rel = child_path->parent;
+		Relids			child_joinrelids;
+		Relids			parent_relids;
+		RelOptInfo		*child_joinrel;
+		SpecialJoinInfo	*child_sjinfo;
+		List			*child_restrictlist;
+
+		child_joinrelids = bms_union(child_rel->relids, inner_rel->relids);
+		parent_relids = bms_union(append_path->path.parent->relids,
+								  inner_rel->relids);
+
+		child_sjinfo = build_child_join_sjinfo(root, extra->sjinfo,
+											   child_rel->relids,
+											   inner_rel->relids);
+		child_restrictlist = (List *)
+			adjust_appendrel_attrs_multilevel(root, (Node *)extra->restrictlist,
+											  child_joinrelids, parent_relids);
+
+		child_joinrel = find_join_rel(root, child_joinrelids);
+		if (!child_joinrel)
+			child_joinrel = build_child_join_rel(root,
+												 child_rel,
+												 inner_rel,
+												 joinrel,
+												 child_restrictlist,
+												 child_sjinfo,
+												 jointype);
+		else
+		{
+			/*
+			 * The join relation already exists. For example, it could happen if
+			 * we join two plane tables with partitioned table(s).
+			 * Populating this join with additional paths could push out some
+			 * previously added paths which could be pointed in a subplans list
+			 * of an higher level append.
+			 * Of course, we could save such paths before generating new. But it
+			 * can increase too much the number of paths in complex queries. It
+			 * can be a task for future work.
+			 */
+			return NIL;
+		}
+
+		populate_joinrel_with_paths(root,
+									child_rel,
+									inner_rel,
+									child_joinrel,
+									child_sjinfo,
+									child_restrictlist);
+
+		/* Give up if asymmetric partition-wise join is not available */
+		if (child_joinrel->pathlist == NIL)
+			return NIL;
+
+		set_cheapest(child_joinrel);
+		result = lappend(result, child_joinrel);
+	}
+	return result;
+}
+
+static bool
+is_asymmetric_join_feasible(PlannerInfo *root,
+						   RelOptInfo *outer_rel,
+						   RelOptInfo *inner_rel,
+						   JoinType jointype)
+{
+	ListCell *lc;
+
+	if (jointype != JOIN_INNER && jointype != JOIN_LEFT)
+		return false;
+
+	if (IS_OTHER_REL(outer_rel) || IS_OTHER_REL(inner_rel))
+		return false;
+
+	/* Disallow recursive usage of asymmertic join machinery */
+	if (root->join_rel_level == NULL)
+		return false;
+
+	/*
+	 * Don't allow asymmetric JOIN of two append subplans.
+	 * In the case of a parameterized NL join, a reparameterization procedure
+	 * will lead to large memory allocations and a CPU consumption:
+	 * each reparameterization will induce subpath duplication, creating new
+	 * ParamPathInfo instance and increasing of ppilist up to number of
+	 * partitions in the inner. Also, if we have many partitions, each bitmapset
+	 * variable will be large and many leaks of such variable (caused by relid
+	 * replacement) will highly increase memory consumption.
+	 * So, we deny such paths for now.
+	 */
+	foreach(lc, inner_rel->pathlist)
+	{
+		if (IsA(lfirst(lc), AppendPath))
+			return false;
+	}
+
+	return true;
+}
+
+void
+try_asymmetric_partitionwise_join(PlannerInfo *root,
+								  RelOptInfo *joinrel,
+								  RelOptInfo *outer_rel,
+								  RelOptInfo *inner_rel,
+								  JoinType jointype,
+								  JoinPathExtraData *extra)
+{
+	ListCell *lc;
+
+	/*
+	 * Try this kind of paths if we allow complex partitionwise joins and we know
+	 * we can build this join safely.
+	 */
+	if (!enable_partitionwise_join ||
+		!is_asymmetric_join_feasible(root, outer_rel, inner_rel, jointype))
+		return;
+
+	foreach (lc, outer_rel->pathlist)
+	{
+		AppendPath *append_path = lfirst(lc);
+
+		/*
+		 * We assume this pathlist keeps at least one AppendPath that
+		 * represents partitioned table-scan, symmetric or asymmetric
+		 * partition-wise join. Asymmetric join isn't needed if the append node
+		 * has only one child.
+		 */
+		if (IsA(append_path, AppendPath) &&
+			list_length(append_path->subpaths) > 1)
+		{
+			List **join_rel_level_saved;
+			List *live_childrels = NIL;
+
+			join_rel_level_saved = root->join_rel_level;
+			PG_TRY();
+			{
+				/* temporary disables "dynamic programming" algorithm */
+				root->join_rel_level = NULL;
+
+				live_childrels =
+					extract_asymmetric_partitionwise_subjoin(root,
+															 joinrel,
+															 append_path,
+															 inner_rel,
+															 jointype,
+															 extra);
+			}
+			PG_FINALLY();
+			{
+				root->join_rel_level = join_rel_level_saved;
+			}
+			PG_END_TRY();
+
+			if (live_childrels != NIL)
+			{
+				/*
+				 * Add new append relation. We must choose cheapest paths after
+				 * this operation because the pathlist possibly contains
+				 * joinrels and appendrels that can be suboptimal.
+				 */
+				add_paths_to_append_rel(root, joinrel, live_childrels);
+				set_cheapest(joinrel);
+			}
+
+			break;
+		}
+	}
+}
+
 /*
  * Construct the SpecialJoinInfo for a child-join by translating
  * SpecialJoinInfo for the join between parents. left_relids and right_relids
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index e50624c465..fccc0685d7 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -281,24 +281,29 @@ set_plan_references(PlannerInfo *root, Plan *plan)
 
 	/*
 	 * Adjust RT indexes of AppendRelInfos and add to final appendrels list.
-	 * We assume the AppendRelInfos were built during planning and don't need
-	 * to be copied.
+	 * The AppendRelInfos are copied, because as a part of a subplan they could
+	 * be visited many times in the case of asymmetric join.
 	 */
 	foreach(lc, root->append_rel_list)
 	{
 		AppendRelInfo *appinfo = lfirst_node(AppendRelInfo, lc);
+		AppendRelInfo *newappinfo;
+
+		/* flat copy is enough since all valuable fields are scalars */
+		newappinfo = (AppendRelInfo *) palloc(sizeof(AppendRelInfo));
+		memcpy(newappinfo, appinfo, sizeof(AppendRelInfo));
 
 		/* adjust RT indexes */
-		appinfo->parent_relid += rtoffset;
-		appinfo->child_relid += rtoffset;
+		newappinfo->parent_relid += rtoffset;
+		newappinfo->child_relid += rtoffset;
 
 		/*
 		 * Rather than adjust the translated_vars entries, just drop 'em.
 		 * Neither the executor nor EXPLAIN currently need that data.
 		 */
-		appinfo->translated_vars = NIL;
+		newappinfo->translated_vars = NIL;
 
-		glob->appendRelations = lappend(glob->appendRelations, appinfo);
+		glob->appendRelations = lappend(glob->appendRelations, newappinfo);
 	}
 
 	/* Now fix the Plan tree */
diff --git a/src/backend/optimizer/util/appendinfo.c b/src/backend/optimizer/util/appendinfo.c
index af46f581ac..f4d12f76e1 100644
--- a/src/backend/optimizer/util/appendinfo.c
+++ b/src/backend/optimizer/util/appendinfo.c
@@ -202,7 +202,9 @@ adjust_appendrel_attrs(PlannerInfo *root, Node *node, int nappinfos,
 	context.appinfos = appinfos;
 
 	/* If there's nothing to adjust, don't call this function. */
-	Assert(nappinfos >= 1 && appinfos != NULL);
+	/* If there's nothing to adjust, just return a duplication */
+	if (nappinfos == 0)
+		return copyObject(node);
 
 	/* Should never be translating a Query tree. */
 	Assert(node == NULL || !IsA(node, Query));
@@ -490,12 +492,10 @@ adjust_appendrel_attrs_multilevel(PlannerInfo *root, Node *node,
 								  Relids top_parent_relids)
 {
 	AppendRelInfo **appinfos;
-	Bitmapset  *parent_relids = NULL;
+	Relids		parent_relids = NULL;
 	int			nappinfos;
 	int			cnt;
 
-	Assert(bms_num_members(child_relids) == bms_num_members(top_parent_relids));
-
 	appinfos = find_appinfos_by_relids(root, child_relids, &nappinfos);
 
 	/* Construct relids set for the immediate parent of given child. */
@@ -506,8 +506,13 @@ adjust_appendrel_attrs_multilevel(PlannerInfo *root, Node *node,
 		parent_relids = bms_add_member(parent_relids, appinfo->parent_relid);
 	}
 
-	/* Recurse if immediate parent is not the top parent. */
-	if (!bms_equal(parent_relids, top_parent_relids))
+	/*
+	 * Recurse if immediate parent is not the top parent. Keep in mind that in a
+	 * case of asymmetric JOIN top_parent_relids can contain relids which aren't
+	 * part of an append node.
+	 */
+	if (!bms_equal(parent_relids, top_parent_relids) &&
+		!bms_is_subset(parent_relids, top_parent_relids))
 		node = adjust_appendrel_attrs_multilevel(root, node, parent_relids,
 												 top_parent_relids);
 
@@ -515,12 +520,13 @@ adjust_appendrel_attrs_multilevel(PlannerInfo *root, Node *node,
 	node = adjust_appendrel_attrs(root, node, nappinfos, appinfos);
 
 	pfree(appinfos);
+	pfree(parent_relids);
 
 	return node;
 }
 
 /*
- * Substitute child relids for parent relids in a Relid set.  The array of
+ * Substitute child relids for parent relids in a Relid set. The array of
  * appinfos specifies the substitutions to be performed.
  */
 Relids
@@ -565,6 +571,7 @@ adjust_child_relids_multilevel(PlannerInfo *root, Relids relids,
 	AppendRelInfo **appinfos;
 	int			nappinfos;
 	Relids		parent_relids = NULL;
+	Relids		normal_relids = NULL;
 	Relids		result;
 	Relids		tmp_result = NULL;
 	int			cnt;
@@ -579,13 +586,24 @@ adjust_child_relids_multilevel(PlannerInfo *root, Relids relids,
 	appinfos = find_appinfos_by_relids(root, child_relids, &nappinfos);
 
 	/* Construct relids set for the immediate parent of the given child. */
+	normal_relids = bms_copy(child_relids);
 	for (cnt = 0; cnt < nappinfos; cnt++)
 	{
 		AppendRelInfo *appinfo = appinfos[cnt];
 
 		parent_relids = bms_add_member(parent_relids, appinfo->parent_relid);
+		normal_relids = bms_del_member(normal_relids, appinfo->child_relid);
 	}
 
+	if (bms_is_subset(relids, normal_relids))
+	{
+		/* Nothing to do. Parameters set points to plain relations only. */
+		result = relids;
+		goto cleanup;
+	}
+
+	parent_relids = bms_union(parent_relids, normal_relids);
+
 	/* Recurse if immediate parent is not the top parent. */
 	if (!bms_equal(parent_relids, top_parent_relids))
 	{
@@ -597,10 +615,11 @@ adjust_child_relids_multilevel(PlannerInfo *root, Relids relids,
 
 	result = adjust_child_relids(relids, nappinfos, appinfos);
 
+cleanup:
 	/* Free memory consumed by any intermediate result. */
-	if (tmp_result)
-		bms_free(tmp_result);
+	bms_free(tmp_result);
 	bms_free(parent_relids);
+	bms_free(normal_relids);
 	pfree(appinfos);
 
 	return result;
@@ -715,11 +734,11 @@ AppendRelInfo **
 find_appinfos_by_relids(PlannerInfo *root, Relids relids, int *nappinfos)
 {
 	AppendRelInfo **appinfos;
+	int			nrooms = bms_num_members(relids);
 	int			cnt = 0;
 	int			i;
 
-	*nappinfos = bms_num_members(relids);
-	appinfos = (AppendRelInfo **) palloc(sizeof(AppendRelInfo *) * *nappinfos);
+	appinfos = (AppendRelInfo **) palloc(sizeof(AppendRelInfo *) * nrooms);
 
 	i = -1;
 	while ((i = bms_next_member(relids, i)) >= 0)
@@ -727,10 +746,11 @@ find_appinfos_by_relids(PlannerInfo *root, Relids relids, int *nappinfos)
 		AppendRelInfo *appinfo = root->append_rel_array[i];
 
 		if (!appinfo)
-			elog(ERROR, "child rel %d not found in append_rel_array", i);
+			continue;
 
 		appinfos[cnt++] = appinfo;
 	}
+	*nappinfos = cnt;
 	return appinfos;
 }
 
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index cedb3848dd..17e215b72d 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -4225,7 +4225,14 @@ do { \
 
 		MemoryContextSwitchTo(oldcontext);
 	}
-	bms_free(required_outer);
+
+	/*
+	 * If adjust_child_relids_multilevel don't do replacements it returns
+	 * the original set, not a copy. It is possible in the case of asymmetric
+	 * JOIN and child_rel->relids contains relids only of plane relations.
+	 */
+	if (required_outer != old_ppi->ppi_req_outer)
+		bms_free(required_outer);
 
 	new_path->param_info = new_ppi;
 
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index 47769cea45..ddf0f5a876 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -792,11 +792,8 @@ build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
 	AppendRelInfo **appinfos;
 	int			nappinfos;
 
-	/* Only joins between "other" relations land here. */
-	Assert(IS_OTHER_REL(outer_rel) && IS_OTHER_REL(inner_rel));
-
-	/* The parent joinrel should have consider_partitionwise_join set. */
-	Assert(parent_joinrel->consider_partitionwise_join);
+	/* Either of relations must be "other" relation at least. */
+	Assert(IS_OTHER_REL(outer_rel) || IS_OTHER_REL(inner_rel));
 
 	joinrel->reloptkind = RELOPT_OTHER_JOINREL;
 	joinrel->relids = bms_union(outer_rel->relids, inner_rel->relids);
@@ -854,8 +851,11 @@ build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
 	joinrel->partexprs = NULL;
 	joinrel->nullable_partexprs = NULL;
 
-	joinrel->top_parent_relids = bms_union(outer_rel->top_parent_relids,
-										   inner_rel->top_parent_relids);
+	joinrel->top_parent_relids =
+		bms_union(IS_OTHER_REL(outer_rel) ?
+				  outer_rel->top_parent_relids : outer_rel->relids,
+				  IS_OTHER_REL(inner_rel) ?
+				  inner_rel->top_parent_relids : inner_rel->relids);
 
 	/* Compute information relevant to foreign relations. */
 	set_foreign_rel_properties(joinrel, outer_rel, inner_rel);
@@ -2036,9 +2036,8 @@ build_child_join_reltarget(PlannerInfo *root,
 {
 	/* Build the targetlist */
 	childrel->reltarget->exprs = (List *)
-		adjust_appendrel_attrs(root,
-							   (Node *) parentrel->reltarget->exprs,
-							   nappinfos, appinfos);
+		adjust_appendrel_attrs_multilevel(root, (Node *)parentrel->reltarget->exprs,
+										childrel->relids, parentrel->relids);
 
 	/* Set the cost and width fields */
 	childrel->reltarget->cost.startup = parentrel->reltarget->cost.startup;
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index f1d111063c..a0106dc539 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -109,7 +109,12 @@ extern bool have_join_order_restriction(PlannerInfo *root,
 extern bool have_dangerous_phv(PlannerInfo *root,
 							   Relids outer_relids, Relids inner_params);
 extern void mark_dummy_rel(RelOptInfo *rel);
-
+extern void try_asymmetric_partitionwise_join(PlannerInfo *root,
+											  RelOptInfo *joinrel,
+											  RelOptInfo *outer_rel,
+											  RelOptInfo *inner_rel,
+											  JoinType jointype,
+											  JoinPathExtraData *extra);
 /*
  * equivclass.c
  *	  routines for managing EquivalenceClasses
diff --git a/src/test/regress/expected/partition_join.out b/src/test/regress/expected/partition_join.out
index 27f7525b3e..3eddd9bec3 100644
--- a/src/test/regress/expected/partition_join.out
+++ b/src/test/regress/expected/partition_join.out
@@ -2320,6 +2320,431 @@ SELECT t1.a, t1.c, t2.b, t2.c FROM prt1_adv t1 INNER JOIN prt2_adv t2 ON (t1.a =
  375 | 0375 | 375 | 0375
 (8 rows)
 
+--
+-- For asymmetric partition-wise join
+--
+CREATE TABLE prt5 (hkey int, a int, b int) PARTITION BY HASH(hkey);
+CREATE TABLE prt5_p0 PARTITION OF prt5
+                     FOR VALUES WITH (modulus 3, remainder 0);
+CREATE TABLE prt5_p1 PARTITION OF prt5
+                     FOR VALUES WITH (modulus 3, remainder 1);
+CREATE TABLE prt5_p2 PARTITION OF prt5
+                     FOR VALUES WITH (modulus 3, remainder 2);
+CREATE TABLE prt6 (aid int, alabel text) PARTITION BY HASH(aid);
+CREATE TABLE prt6_p0 PARTITION OF prt6
+                     FOR VALUES WITH (modulus 2, remainder 0);
+CREATE TABLE prt6_p1 PARTITION OF prt6
+                     FOR VALUES WITH (modulus 2, remainder 1);
+CREATE TABLE t5_1 (aid int, alabel text);
+CREATE TABLE t5_2 (bid int, blabel text);
+INSERT INTO prt5 (SELECT x, (x % 1000)::int,
+                            ((x+1) % 1000)::int
+                    FROM generate_series(1,1000000) x);
+INSERT INTO t5_1 (SELECT x, md5(x::text) FROM generate_series(-200, 1500) x);
+INSERT INTO t5_2 (SELECT x, md5(x::text) FROM generate_series(-200, 1500) x);
+INSERT INTO prt6 (SELECT * FROM t5_1);
+VACUUM ANALYZE prt5,prt6,t5_1,t5_2;
+SET max_parallel_workers_per_gather = 0;
+-- Trivial asymmetric JOIN of partitioned table with a relation
+EXPLAIN (COSTS OFF)
+SELECT *
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+                      QUERY PLAN                       
+-------------------------------------------------------
+ Append
+   ->  Hash Join
+         Hash Cond: (prt5_1.a = t5_1.aid)
+         ->  Seq Scan on prt5_p0 prt5_1
+         ->  Hash
+               ->  Seq Scan on t5_1
+                     Filter: (alabel ~~ '%abc%'::text)
+   ->  Hash Join
+         Hash Cond: (prt5_2.a = t5_1.aid)
+         ->  Seq Scan on prt5_p1 prt5_2
+         ->  Hash
+               ->  Seq Scan on t5_1
+                     Filter: (alabel ~~ '%abc%'::text)
+   ->  Hash Join
+         Hash Cond: (prt5_3.a = t5_1.aid)
+         ->  Seq Scan on prt5_p2 prt5_3
+         ->  Hash
+               ->  Seq Scan on t5_1
+                     Filter: (alabel ~~ '%abc%'::text)
+(19 rows)
+
+-- The same, but appended with UNION ALL
+EXPLAIN (COSTS OFF)
+SELECT * FROM (
+	(SELECT * FROM prt5_p0)
+	UNION ALL
+	(SELECT * FROM prt5_p1)
+	UNION ALL
+	(SELECT * FROM prt5_p2)
+	) AS sq1
+JOIN t5_1 ON a = aid AND alabel like '%abc%';
+                      QUERY PLAN                       
+-------------------------------------------------------
+ Append
+   ->  Hash Join
+         Hash Cond: (prt5_p0.a = t5_1.aid)
+         ->  Seq Scan on prt5_p0
+         ->  Hash
+               ->  Seq Scan on t5_1
+                     Filter: (alabel ~~ '%abc%'::text)
+   ->  Hash Join
+         Hash Cond: (prt5_p1.a = t5_1.aid)
+         ->  Seq Scan on prt5_p1
+         ->  Hash
+               ->  Seq Scan on t5_1
+                     Filter: (alabel ~~ '%abc%'::text)
+   ->  Hash Join
+         Hash Cond: (prt5_p2.a = t5_1.aid)
+         ->  Seq Scan on prt5_p2
+         ->  Hash
+               ->  Seq Scan on t5_1
+                     Filter: (alabel ~~ '%abc%'::text)
+(19 rows)
+
+-- Don't allow asymmetric JOIN of two partitioned tables.
+EXPLAIN (COSTS OFF)
+SELECT count(*) FROM prt5 JOIN prt6 ON a = aid AND alabel like '%abc%';
+                         QUERY PLAN                          
+-------------------------------------------------------------
+ Aggregate
+   ->  Hash Join
+         Hash Cond: (prt5.a = prt6.aid)
+         ->  Append
+               ->  Seq Scan on prt5_p0 prt5_1
+               ->  Seq Scan on prt5_p1 prt5_2
+               ->  Seq Scan on prt5_p2 prt5_3
+         ->  Hash
+               ->  Append
+                     ->  Seq Scan on prt6_p0 prt6_1
+                           Filter: (alabel ~~ '%abc%'::text)
+                     ->  Seq Scan on prt6_p1 prt6_2
+                           Filter: (alabel ~~ '%abc%'::text)
+(13 rows)
+
+SELECT count(*) FROM prt5 JOIN prt6 ON a = aid AND alabel like '%abc%';
+ count 
+-------
+  4000
+(1 row)
+
+-- Check asymmetric JOIN with Subquery
+EXPLAIN (COSTS OFF)
+SELECT count(*) FROM prt5 JOIN (
+	SELECT * FROM prt6 LIMIT 1000
+) AS sq1 ON a = aid AND alabel like '%abc%';
+                              QUERY PLAN                              
+----------------------------------------------------------------------
+ Aggregate
+   ->  Append
+         ->  Hash Join
+               Hash Cond: (prt5_1.a = sq1.aid)
+               ->  Seq Scan on prt5_p0 prt5_1
+               ->  Hash
+                     ->  Subquery Scan on sq1
+                           Filter: (sq1.alabel ~~ '%abc%'::text)
+                           ->  Limit
+                                 ->  Append
+                                       ->  Seq Scan on prt6_p0 prt6_1
+                                       ->  Seq Scan on prt6_p1 prt6_2
+         ->  Hash Join
+               Hash Cond: (prt5_2.a = sq1.aid)
+               ->  Seq Scan on prt5_p1 prt5_2
+               ->  Hash
+                     ->  Subquery Scan on sq1
+                           Filter: (sq1.alabel ~~ '%abc%'::text)
+                           ->  Limit
+                                 ->  Append
+                                       ->  Seq Scan on prt6_p0 prt6_4
+                                       ->  Seq Scan on prt6_p1 prt6_5
+         ->  Hash Join
+               Hash Cond: (prt5_3.a = sq1.aid)
+               ->  Seq Scan on prt5_p2 prt5_3
+               ->  Hash
+                     ->  Subquery Scan on sq1
+                           Filter: (sq1.alabel ~~ '%abc%'::text)
+                           ->  Limit
+                                 ->  Append
+                                       ->  Seq Scan on prt6_p0 prt6_7
+                                       ->  Seq Scan on prt6_p1 prt6_8
+(32 rows)
+
+SELECT count(*) FROM prt5 JOIN (SELECT * FROM prt6 LIMIT 1000) AS sq1
+	ON a = aid AND alabel like '%abc%';
+ count 
+-------
+  2000
+(1 row)
+
+-- Asymmetric JOIN of two plane tables and one partitioned
+EXPLAIN (COSTS OFF)
+SELECT count(*)
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+            JOIN t5_2 ON b = bid AND blabel like '%cd%';
+                            QUERY PLAN                            
+------------------------------------------------------------------
+ Aggregate
+   ->  Append
+         ->  Hash Join
+               Hash Cond: (prt5_1.b = t5_2.bid)
+               ->  Hash Join
+                     Hash Cond: (prt5_1.a = t5_1.aid)
+                     ->  Seq Scan on prt5_p0 prt5_1
+                     ->  Hash
+                           ->  Seq Scan on t5_1
+                                 Filter: (alabel ~~ '%ab%'::text)
+               ->  Hash
+                     ->  Seq Scan on t5_2
+                           Filter: (blabel ~~ '%cd%'::text)
+         ->  Hash Join
+               Hash Cond: (prt5_2.b = t5_2.bid)
+               ->  Hash Join
+                     Hash Cond: (prt5_2.a = t5_1.aid)
+                     ->  Seq Scan on prt5_p1 prt5_2
+                     ->  Hash
+                           ->  Seq Scan on t5_1
+                                 Filter: (alabel ~~ '%ab%'::text)
+               ->  Hash
+                     ->  Seq Scan on t5_2
+                           Filter: (blabel ~~ '%cd%'::text)
+         ->  Hash Join
+               Hash Cond: (prt5_3.b = t5_2.bid)
+               ->  Hash Join
+                     Hash Cond: (prt5_3.a = t5_1.aid)
+                     ->  Seq Scan on prt5_p2 prt5_3
+                     ->  Hash
+                           ->  Seq Scan on t5_1
+                                 Filter: (alabel ~~ '%ab%'::text)
+               ->  Hash
+                     ->  Seq Scan on t5_2
+                           Filter: (blabel ~~ '%cd%'::text)
+(35 rows)
+
+SELECT count(*)
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+            JOIN t5_2 ON b = bid AND blabel like '%cd%';
+ count 
+-------
+ 11000
+(1 row)
+
+-- unable to extract non-partitioned right relation
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt5 RIGHT JOIN t5_1 ON a = aid AND alabel like '%abc%';
+                  QUERY PLAN                   
+-----------------------------------------------
+ Hash Right Join
+   Hash Cond: (prt5.a = t5_1.aid)
+   Join Filter: (t5_1.alabel ~~ '%abc%'::text)
+   ->  Append
+         ->  Seq Scan on prt5_p0 prt5_1
+         ->  Seq Scan on prt5_p1 prt5_2
+         ->  Seq Scan on prt5_p2 prt5_3
+   ->  Hash
+         ->  Seq Scan on t5_1
+(9 rows)
+
+-- left side can be extracted, but no cost benefit
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt5 LEFT JOIN t5_1 ON a = aid AND alabel like '%abc%';
+                   QUERY PLAN                    
+-------------------------------------------------
+ Hash Left Join
+   Hash Cond: (prt5.a = t5_1.aid)
+   ->  Append
+         ->  Seq Scan on prt5_p0 prt5_1
+         ->  Seq Scan on prt5_p1 prt5_2
+         ->  Seq Scan on prt5_p2 prt5_3
+   ->  Hash
+         ->  Seq Scan on t5_1
+               Filter: (alabel ~~ '%abc%'::text)
+(9 rows)
+
+-- validation of the results with/without asymmetric partition-wise join
+SELECT * INTO pg_temp.result01a
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+SELECT * INTO pg_temp.result02a
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+            JOIN t5_2 ON b = bid AND blabel like '%cd%';
+SET enable_partitionwise_join = off;
+SELECT * INTO pg_temp.result01b
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+SELECT * INTO pg_temp.result02b
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+            JOIN t5_2 ON b = bid AND blabel like '%cd%';
+SELECT * FROM pg_temp.result01a EXCEPT SELECT * FROM pg_temp.result01b;
+ hkey | a | b | aid | alabel 
+------+---+---+-----+--------
+(0 rows)
+
+SELECT * FROM pg_temp.result02a EXCEPT SELECT * FROM pg_temp.result02b;
+ hkey | a | b | aid | alabel | bid | blabel 
+------+---+---+-----+--------+-----+--------
+(0 rows)
+
+SET enable_partitionwise_join = on;
+-- Check reparameterization code when an optimizer have to make two level relids
+-- adjustment.
+SET enable_hashjoin = 'off';
+SET enable_mergejoin = 'off';
+SET enable_material = 'off';
+CREATE TABLE big AS SELECT x AS x FROM generate_series(1,1E4) x;
+CREATE INDEX ON big(x);
+CREATE TABLE small AS SELECT x, -x AS y FROM generate_series(1,100) x;
+CREATE TABLE part_l0 (x int, y int, z int) PARTITION BY HASH (y);
+CREATE TABLE part0_l1 PARTITION OF part_l0 (y)
+	FOR VALUES WITH (modulus 2, remainder 0) PARTITION BY HASH (z);
+CREATE TABLE part0_l2 PARTITION OF part0_l1 (z)
+	FOR VALUES WITH (modulus 2, remainder 0);
+CREATE TABLE part1_l2 PARTITION OF part0_l1 (z)
+	FOR VALUES WITH (modulus 2, remainder 1);
+CREATE TABLE part1_l1 PARTITION OF part_l0 (y)
+	FOR VALUES WITH (modulus 2, remainder 1);
+INSERT INTO part_l0 (x, y, z) (SELECT x,x,x FROM generate_series(1,1E4) x);
+ANALYZE big,small,part_l0;
+-- Parameter have to be reparameterized by a plane relation.
+EXPLAIN (COSTS OFF)
+SELECT small.* FROM small
+	JOIN part_l0 ON small.y = part_l0.y AND small.y + part_l0.y < 10
+	LEFT JOIN big ON big.x = small.x;
+                                       QUERY PLAN                                        
+-----------------------------------------------------------------------------------------
+ Nested Loop Left Join
+   ->  Append
+         ->  Nested Loop
+               Join Filter: ((small.y = part_l0_1.y) AND ((small.y + part_l0_1.y) < 10))
+               ->  Seq Scan on small
+               ->  Seq Scan on part0_l2 part_l0_1
+         ->  Nested Loop
+               Join Filter: ((small.y = part_l0_2.y) AND ((small.y + part_l0_2.y) < 10))
+               ->  Seq Scan on part1_l2 part_l0_2
+               ->  Seq Scan on small
+         ->  Nested Loop
+               Join Filter: ((small.y = part_l0_3.y) AND ((small.y + part_l0_3.y) < 10))
+               ->  Seq Scan on small
+               ->  Seq Scan on part1_l1 part_l0_3
+   ->  Index Only Scan using big_x_idx on big
+         Index Cond: (x = (small.x)::numeric)
+(16 rows)
+
+SELECT small.* FROM small
+	JOIN part_l0 ON small.y = part_l0.y AND small.y + part_l0.y < 10
+	LEFT JOIN big ON big.x = small.x;
+ x | y 
+---+---
+(0 rows)
+
+-- Parameters have to be reparameterized by plane and partitioned relations.
+EXPLAIN (COSTS OFF)
+SELECT count(*) FROM small
+	JOIN part_l0 ON small.y = part_l0.y AND small.y + part_l0.y < 10
+	LEFT JOIN big ON big.x = small.x OR big.x = part_l0.x;
+                                          QUERY PLAN                                           
+-----------------------------------------------------------------------------------------------
+ Aggregate
+   ->  Nested Loop Left Join
+         ->  Append
+               ->  Nested Loop
+                     Join Filter: ((small.y = part_l0_1.y) AND ((small.y + part_l0_1.y) < 10))
+                     ->  Seq Scan on small
+                     ->  Seq Scan on part0_l2 part_l0_1
+               ->  Nested Loop
+                     Join Filter: ((small.y = part_l0_2.y) AND ((small.y + part_l0_2.y) < 10))
+                     ->  Seq Scan on part1_l2 part_l0_2
+                     ->  Seq Scan on small
+               ->  Nested Loop
+                     Join Filter: ((small.y = part_l0_3.y) AND ((small.y + part_l0_3.y) < 10))
+                     ->  Seq Scan on small
+                     ->  Seq Scan on part1_l1 part_l0_3
+         ->  Bitmap Heap Scan on big
+               Recheck Cond: ((x = (small.x)::numeric) OR (x = (part_l0.x)::numeric))
+               ->  BitmapOr
+                     ->  Bitmap Index Scan on big_x_idx
+                           Index Cond: (x = (small.x)::numeric)
+                     ->  Bitmap Index Scan on big_x_idx
+                           Index Cond: (x = (part_l0.x)::numeric)
+(22 rows)
+
+SELECT count(*) FROM small
+	JOIN part_l0 ON small.y = part_l0.y AND small.y + part_l0.y < 10
+	LEFT JOIN big ON big.x = small.x OR big.x = part_l0.x;
+ count 
+-------
+     0
+(1 row)
+
+DROP TABLE IF EXISTS big,small,part_l0 CASCADE;
+RESET enable_hashjoin;
+RESET enable_mergejoin;
+RESET enable_material;
+RESET max_parallel_workers_per_gather;
+-- Parameterized path examples.
+CREATE TABLE prta (id integer, payload text) PARTITION BY HASH (id);
+CREATE TABLE prta1 PARTITION OF prta FOR VALUES WITH (modulus 2, remainder 0);
+CREATE TABLE prta2 PARTITION OF prta FOR VALUES WITH (modulus 2, remainder 1);
+CREATE UNIQUE INDEX ON prta1 (id);
+CREATE UNIQUE INDEX ON prta2 (id);
+INSERT INTO prta (id, payload)
+	(SELECT *, ('abc' || id)::text AS payload
+		FROM generate_series(1,1) AS id);
+CREATE TABLE prtb (id integer, payload text) PARTITION BY HASH (id);
+CREATE TABLE prtb1 PARTITION OF prtb FOR VALUES WITH (modulus 2, remainder 0);
+CREATE TABLE prtb2 PARTITION OF prtb FOR VALUES WITH (modulus 2, remainder 1);
+CREATE UNIQUE INDEX ON prtb1 (id);
+CREATE UNIQUE INDEX ON prtb2 (id);
+INSERT INTO prtb (id, payload)
+    (SELECT *, ('def' || id)::text AS payload
+		FROM generate_series(1,1000) AS id);
+CREATE TABLE e (id integer, payload text) PARTITION BY HASH (id);
+CREATE TABLE e1 PARTITION OF e FOR VALUES WITH (modulus 2, remainder 0);
+CREATE TABLE e2 PARTITION OF e FOR VALUES WITH (modulus 2, remainder 1);
+INSERT INTO e (id, payload)
+	(SELECT *, ('ghi' || id)::text AS payload
+		FROM generate_series(1,1000) AS id);
+CREATE UNIQUE INDEX ON e1 (id);
+CREATE UNIQUE INDEX ON e2 (id);
+ANALYZE prta,prtb,e;
+EXPLAIN (COSTS OFF)
+SELECT * FROM prta,prtb WHERE prta.id=prtb.id;
+                        QUERY PLAN                         
+-----------------------------------------------------------
+ Append
+   ->  Nested Loop
+         ->  Seq Scan on prta1 prta_1
+         ->  Index Scan using prtb1_id_idx on prtb1 prtb_1
+               Index Cond: (id = prta_1.id)
+   ->  Nested Loop
+         ->  Seq Scan on prta2 prta_2
+         ->  Index Scan using prtb2_id_idx on prtb2 prtb_2
+               Index Cond: (id = prta_2.id)
+(9 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT * FROM prta,prtb,e WHERE prta.id=prtb.id AND prta.id=e.id;
+                           QUERY PLAN                            
+-----------------------------------------------------------------
+ Append
+   ->  Nested Loop
+         Join Filter: (prta_1.id = e_1.id)
+         ->  Nested Loop
+               ->  Seq Scan on prta1 prta_1
+               ->  Index Scan using prtb1_id_idx on prtb1 prtb_1
+                     Index Cond: (id = prta_1.id)
+         ->  Index Scan using e1_id_idx on e1 e_1
+               Index Cond: (id = prtb_1.id)
+   ->  Nested Loop
+         Join Filter: (prta_2.id = e_2.id)
+         ->  Nested Loop
+               ->  Seq Scan on prta2 prta_2
+               ->  Index Scan using prtb2_id_idx on prtb2 prtb_2
+                     Index Cond: (id = prta_2.id)
+         ->  Index Scan using e2_id_idx on e2 e_2
+               Index Cond: (id = prtb_2.id)
+(17 rows)
+
 -- semi join
 EXPLAIN (COSTS OFF)
 SELECT t1.* FROM prt1_adv t1 WHERE EXISTS (SELECT 1 FROM prt2_adv t2 WHERE t1.a = t2.b) AND t1.b = 0 ORDER BY t1.a;
diff --git a/src/test/regress/sql/partition_join.sql b/src/test/regress/sql/partition_join.sql
index d97b5b69ff..4d1dd5bec1 100644
--- a/src/test/regress/sql/partition_join.sql
+++ b/src/test/regress/sql/partition_join.sql
@@ -536,6 +536,186 @@ EXPLAIN (COSTS OFF)
 SELECT t1.a, t1.c, t2.b, t2.c FROM prt1_adv t1 INNER JOIN prt2_adv t2 ON (t1.a = t2.b) WHERE t1.b = 0 ORDER BY t1.a, t2.b;
 SELECT t1.a, t1.c, t2.b, t2.c FROM prt1_adv t1 INNER JOIN prt2_adv t2 ON (t1.a = t2.b) WHERE t1.b = 0 ORDER BY t1.a, t2.b;
 
+--
+-- For asymmetric partition-wise join
+--
+CREATE TABLE prt5 (hkey int, a int, b int) PARTITION BY HASH(hkey);
+CREATE TABLE prt5_p0 PARTITION OF prt5
+                     FOR VALUES WITH (modulus 3, remainder 0);
+CREATE TABLE prt5_p1 PARTITION OF prt5
+                     FOR VALUES WITH (modulus 3, remainder 1);
+CREATE TABLE prt5_p2 PARTITION OF prt5
+                     FOR VALUES WITH (modulus 3, remainder 2);
+CREATE TABLE prt6 (aid int, alabel text) PARTITION BY HASH(aid);
+CREATE TABLE prt6_p0 PARTITION OF prt6
+                     FOR VALUES WITH (modulus 2, remainder 0);
+CREATE TABLE prt6_p1 PARTITION OF prt6
+                     FOR VALUES WITH (modulus 2, remainder 1);
+CREATE TABLE t5_1 (aid int, alabel text);
+CREATE TABLE t5_2 (bid int, blabel text);
+
+INSERT INTO prt5 (SELECT x, (x % 1000)::int,
+                            ((x+1) % 1000)::int
+                    FROM generate_series(1,1000000) x);
+INSERT INTO t5_1 (SELECT x, md5(x::text) FROM generate_series(-200, 1500) x);
+INSERT INTO t5_2 (SELECT x, md5(x::text) FROM generate_series(-200, 1500) x);
+INSERT INTO prt6 (SELECT * FROM t5_1);
+
+VACUUM ANALYZE prt5,prt6,t5_1,t5_2;
+
+SET max_parallel_workers_per_gather = 0;
+
+-- Trivial asymmetric JOIN of partitioned table with a relation
+EXPLAIN (COSTS OFF)
+SELECT *
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+
+-- The same, but appended with UNION ALL
+EXPLAIN (COSTS OFF)
+SELECT * FROM (
+	(SELECT * FROM prt5_p0)
+	UNION ALL
+	(SELECT * FROM prt5_p1)
+	UNION ALL
+	(SELECT * FROM prt5_p2)
+	) AS sq1
+JOIN t5_1 ON a = aid AND alabel like '%abc%';
+
+-- Don't allow asymmetric JOIN of two partitioned tables.
+EXPLAIN (COSTS OFF)
+SELECT count(*) FROM prt5 JOIN prt6 ON a = aid AND alabel like '%abc%';
+SELECT count(*) FROM prt5 JOIN prt6 ON a = aid AND alabel like '%abc%';
+
+-- Check asymmetric JOIN with Subquery
+EXPLAIN (COSTS OFF)
+SELECT count(*) FROM prt5 JOIN (
+	SELECT * FROM prt6 LIMIT 1000
+) AS sq1 ON a = aid AND alabel like '%abc%';
+SELECT count(*) FROM prt5 JOIN (SELECT * FROM prt6 LIMIT 1000) AS sq1
+	ON a = aid AND alabel like '%abc%';
+
+-- Asymmetric JOIN of two plane tables and one partitioned
+EXPLAIN (COSTS OFF)
+SELECT count(*)
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+            JOIN t5_2 ON b = bid AND blabel like '%cd%';
+SELECT count(*)
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+            JOIN t5_2 ON b = bid AND blabel like '%cd%';
+
+-- unable to extract non-partitioned right relation
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt5 RIGHT JOIN t5_1 ON a = aid AND alabel like '%abc%';
+-- left side can be extracted, but no cost benefit
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt5 LEFT JOIN t5_1 ON a = aid AND alabel like '%abc%';
+
+-- validation of the results with/without asymmetric partition-wise join
+SELECT * INTO pg_temp.result01a
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+
+SELECT * INTO pg_temp.result02a
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+            JOIN t5_2 ON b = bid AND blabel like '%cd%';
+
+SET enable_partitionwise_join = off;
+
+SELECT * INTO pg_temp.result01b
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+
+SELECT * INTO pg_temp.result02b
+  FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+            JOIN t5_2 ON b = bid AND blabel like '%cd%';
+
+SELECT * FROM pg_temp.result01a EXCEPT SELECT * FROM pg_temp.result01b;
+SELECT * FROM pg_temp.result02a EXCEPT SELECT * FROM pg_temp.result02b;
+
+SET enable_partitionwise_join = on;
+
+-- Check reparameterization code when an optimizer have to make two level relids
+-- adjustment.
+
+SET enable_hashjoin = 'off';
+SET enable_mergejoin = 'off';
+SET enable_material = 'off';
+
+CREATE TABLE big AS SELECT x AS x FROM generate_series(1,1E4) x;
+CREATE INDEX ON big(x);
+CREATE TABLE small AS SELECT x, -x AS y FROM generate_series(1,100) x;
+
+CREATE TABLE part_l0 (x int, y int, z int) PARTITION BY HASH (y);
+CREATE TABLE part0_l1 PARTITION OF part_l0 (y)
+	FOR VALUES WITH (modulus 2, remainder 0) PARTITION BY HASH (z);
+CREATE TABLE part0_l2 PARTITION OF part0_l1 (z)
+	FOR VALUES WITH (modulus 2, remainder 0);
+CREATE TABLE part1_l2 PARTITION OF part0_l1 (z)
+	FOR VALUES WITH (modulus 2, remainder 1);
+CREATE TABLE part1_l1 PARTITION OF part_l0 (y)
+	FOR VALUES WITH (modulus 2, remainder 1);
+INSERT INTO part_l0 (x, y, z) (SELECT x,x,x FROM generate_series(1,1E4) x);
+
+ANALYZE big,small,part_l0;
+
+-- Parameter have to be reparameterized by a plane relation.
+EXPLAIN (COSTS OFF)
+SELECT small.* FROM small
+	JOIN part_l0 ON small.y = part_l0.y AND small.y + part_l0.y < 10
+	LEFT JOIN big ON big.x = small.x;
+SELECT small.* FROM small
+	JOIN part_l0 ON small.y = part_l0.y AND small.y + part_l0.y < 10
+	LEFT JOIN big ON big.x = small.x;
+
+-- Parameters have to be reparameterized by plane and partitioned relations.
+EXPLAIN (COSTS OFF)
+SELECT count(*) FROM small
+	JOIN part_l0 ON small.y = part_l0.y AND small.y + part_l0.y < 10
+	LEFT JOIN big ON big.x = small.x OR big.x = part_l0.x;
+SELECT count(*) FROM small
+	JOIN part_l0 ON small.y = part_l0.y AND small.y + part_l0.y < 10
+	LEFT JOIN big ON big.x = small.x OR big.x = part_l0.x;
+
+DROP TABLE IF EXISTS big,small,part_l0 CASCADE;
+RESET enable_hashjoin;
+RESET enable_mergejoin;
+RESET enable_material;
+RESET max_parallel_workers_per_gather;
+
+-- Parameterized path examples.
+CREATE TABLE prta (id integer, payload text) PARTITION BY HASH (id);
+CREATE TABLE prta1 PARTITION OF prta FOR VALUES WITH (modulus 2, remainder 0);
+CREATE TABLE prta2 PARTITION OF prta FOR VALUES WITH (modulus 2, remainder 1);
+CREATE UNIQUE INDEX ON prta1 (id);
+CREATE UNIQUE INDEX ON prta2 (id);
+INSERT INTO prta (id, payload)
+	(SELECT *, ('abc' || id)::text AS payload
+		FROM generate_series(1,1) AS id);
+
+CREATE TABLE prtb (id integer, payload text) PARTITION BY HASH (id);
+CREATE TABLE prtb1 PARTITION OF prtb FOR VALUES WITH (modulus 2, remainder 0);
+CREATE TABLE prtb2 PARTITION OF prtb FOR VALUES WITH (modulus 2, remainder 1);
+CREATE UNIQUE INDEX ON prtb1 (id);
+CREATE UNIQUE INDEX ON prtb2 (id);
+INSERT INTO prtb (id, payload)
+    (SELECT *, ('def' || id)::text AS payload
+		FROM generate_series(1,1000) AS id);
+
+CREATE TABLE e (id integer, payload text) PARTITION BY HASH (id);
+CREATE TABLE e1 PARTITION OF e FOR VALUES WITH (modulus 2, remainder 0);
+CREATE TABLE e2 PARTITION OF e FOR VALUES WITH (modulus 2, remainder 1);
+INSERT INTO e (id, payload)
+	(SELECT *, ('ghi' || id)::text AS payload
+		FROM generate_series(1,1000) AS id);
+CREATE UNIQUE INDEX ON e1 (id);
+CREATE UNIQUE INDEX ON e2 (id);
+
+ANALYZE prta,prtb,e;
+
+EXPLAIN (COSTS OFF)
+SELECT * FROM prta,prtb WHERE prta.id=prtb.id;
+
+EXPLAIN (COSTS OFF)
+SELECT * FROM prta,prtb,e WHERE prta.id=prtb.id AND prta.id=e.id;
+
 -- semi join
 EXPLAIN (COSTS OFF)
 SELECT t1.* FROM prt1_adv t1 WHERE EXISTS (SELECT 1 FROM prt2_adv t2 WHERE t1.a = t2.b) AND t1.b = 0 ORDER BY t1.a;
-- 
2.33.0


--------------9DCA5E9A07F91111D4169DAB--





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

* introduce dynamic shared memory registry
@ 2023-12-05 03:46 Nathan Bossart <[email protected]>
  2023-12-05 15:34 ` Re: introduce dynamic shared memory registry Joe Conway <[email protected]>
  2023-12-08 07:36 ` Re: introduce dynamic shared memory registry Michael Paquier <[email protected]>
  2023-12-18 06:39 ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03 ` Re: introduce dynamic shared memory registry Zhang Mingli <[email protected]>
  0 siblings, 5 replies; 26+ messages in thread

From: Nathan Bossart @ 2023-12-05 03:46 UTC (permalink / raw)
  To: pgsql-hackers

Every once in a while, I find myself wanting to use shared memory in a
loadable module without requiring it to be loaded at server start via
shared_preload_libraries.  The DSM API offers a nice way to create and
manage dynamic shared memory segments, so creating a segment after server
start is easy enough.  However, AFAICT there's no easy way to teach other
backends about the segment without storing the handles in shared memory,
which puts us right back at square one.

The attached 0001 introduces a "DSM registry" to solve this problem.  The
API provides an easy way to allocate/initialize a segment or to attach to
an existing one.  The registry itself is just a dshash table that stores
the handles keyed by a module-specified string.  0002 adds a test for the
registry that demonstrates basic usage.

I don't presently have any concrete plans to use this for anything, but I
thought it might be useful for extensions for caching, etc. and wanted to
see whether there was any interest in the feature.

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


Attachments:

  [text/x-diff] v1-0001-add-dsm-registry.patch (9.8K, ../../20231205034647.GA2705267@nathanxps13/2-v1-0001-add-dsm-registry.patch)
  download | inline diff:
From b63c28303384636699f2f514e71b62829346be4b Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 11 Oct 2023 22:07:26 -0500
Subject: [PATCH v1 1/2] add dsm registry

---
 src/backend/storage/ipc/Makefile         |   1 +
 src/backend/storage/ipc/dsm_registry.c   | 176 +++++++++++++++++++++++
 src/backend/storage/ipc/ipci.c           |   3 +
 src/backend/storage/ipc/meson.build      |   1 +
 src/backend/storage/lmgr/lwlock.c        |   4 +
 src/backend/storage/lmgr/lwlocknames.txt |   1 +
 src/include/storage/dsm_registry.h       |  22 +++
 src/include/storage/lwlock.h             |   4 +-
 src/tools/pgindent/typedefs.list         |   2 +
 9 files changed, 213 insertions(+), 1 deletion(-)
 create mode 100644 src/backend/storage/ipc/dsm_registry.c
 create mode 100644 src/include/storage/dsm_registry.h

diff --git a/src/backend/storage/ipc/Makefile b/src/backend/storage/ipc/Makefile
index 6d5b921038..d8a1653eb6 100644
--- a/src/backend/storage/ipc/Makefile
+++ b/src/backend/storage/ipc/Makefile
@@ -12,6 +12,7 @@ OBJS = \
 	barrier.o \
 	dsm.o \
 	dsm_impl.o \
+	dsm_registry.o \
 	ipc.o \
 	ipci.o \
 	latch.o \
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
new file mode 100644
index 0000000000..ea80f45716
--- /dev/null
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -0,0 +1,176 @@
+/*-------------------------------------------------------------------------
+ *
+ * dsm_registry.c
+ *
+ * Functions for interfacing with the dynamic shared memory registry.  This
+ * provides a way for libraries to use shared memory without needing to
+ * request it at startup time via a shmem_request_hook.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ *	  src/backend/storage/ipc/dsm_registry.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "lib/dshash.h"
+#include "storage/dsm_registry.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
+#include "utils/memutils.h"
+
+typedef struct DSMRegistryCtxStruct
+{
+	dsa_handle	dsah;
+	dshash_table_handle dshh;
+} DSMRegistryCtxStruct;
+
+static DSMRegistryCtxStruct *DSMRegistryCtx;
+
+typedef struct DSMRegistryEntry
+{
+	char		key[256];
+	dsm_handle	handle;
+} DSMRegistryEntry;
+
+static const dshash_parameters dsh_params = {
+	offsetof(DSMRegistryEntry, handle),
+	sizeof(DSMRegistryEntry),
+	dshash_memcmp,
+	dshash_memhash,
+	LWTRANCHE_DSM_REGISTRY_HASH
+};
+
+static dsa_area *dsm_registry_dsa;
+static dshash_table *dsm_registry_table;
+
+static void init_dsm_registry(void);
+
+Size
+DSMRegistryShmemSize(void)
+{
+	return MAXALIGN(sizeof(DSMRegistryCtxStruct));
+}
+
+void
+DSMRegistryShmemInit(void)
+{
+	bool		found;
+
+	DSMRegistryCtx = (DSMRegistryCtxStruct *)
+		ShmemInitStruct("DSM Registry Data",
+						DSMRegistryShmemSize(),
+						&found);
+
+	if (!found)
+	{
+		DSMRegistryCtx->dsah = DSA_HANDLE_INVALID;
+		DSMRegistryCtx->dshh = DSHASH_HANDLE_INVALID;
+	}
+}
+
+/*
+ * Initialize or attach to the dynamic shared hash table that stores the DSM
+ * registry entries, if not already done.  This must be called before accessing
+ * the table.
+ */
+static void
+init_dsm_registry(void)
+{
+	/* Quick exit if we already did this. */
+	if (dsm_registry_table)
+		return;
+
+	/* Otherwise, use a lock to ensure only one process creates the table. */
+	LWLockAcquire(DSMRegistryLock, LW_EXCLUSIVE);
+
+	if (DSMRegistryCtx->dshh == DSHASH_HANDLE_INVALID)
+	{
+		/* Initialize dynamic shared hash table for registry. */
+		dsm_registry_dsa = dsa_create(LWTRANCHE_DSM_REGISTRY_DSA);
+		dsa_pin(dsm_registry_dsa);
+		dsa_pin_mapping(dsm_registry_dsa);
+		dsm_registry_table = dshash_create(dsm_registry_dsa, &dsh_params, 0);
+
+		/* Store handles in shared memory for other backends to use. */
+		DSMRegistryCtx->dsah = dsa_get_handle(dsm_registry_dsa);
+		DSMRegistryCtx->dshh = dshash_get_hash_table_handle(dsm_registry_table);
+	}
+	else
+	{
+		/* Attach to existing dynamic shared hash table. */
+		dsm_registry_dsa = dsa_attach(DSMRegistryCtx->dsah);
+		dsa_pin_mapping(dsm_registry_dsa);
+		dsm_registry_table = dshash_attach(dsm_registry_dsa, &dsh_params,
+										   DSMRegistryCtx->dshh, 0);
+	}
+
+	LWLockRelease(DSMRegistryLock);
+}
+
+/*
+ * Initialize or attach a DSM entry.
+ *
+ * *ptr should initially be set to NULL.  If it is not NULL, this routine will
+ * assume that the segment has already been attached to the current session.
+ * Otherwise, this routine will set *ptr appropriately.
+ *
+ * init_callback is called to initialize the segment when it is first created.
+ */
+void
+dsm_registry_init_or_attach(const char *key, void **ptr, size_t size,
+							void (*init_callback) (void *ptr))
+{
+	DSMRegistryEntry *entry;
+	MemoryContext oldcontext;
+	bool		found;
+	char		key_padded[offsetof(DSMRegistryEntry, handle)] = {0};
+
+	Assert(key);
+	Assert(ptr);
+	Assert(size);
+
+	if (strlen(key) >= offsetof(DSMRegistryEntry, handle))
+		elog(ERROR, "DSM registry key too long");
+
+	/* Quick exit if the value is already set. */
+	if (*ptr)
+		return;
+
+	/* Be sure any local memory allocated by DSM/DSA routines is persistent. */
+	oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+	/* Connect to the registry. */
+	init_dsm_registry();
+
+	strcpy(key_padded, key);
+	entry = dshash_find_or_insert(dsm_registry_table, key_padded, &found);
+	if (!found)
+	{
+		/* Initialize DSM registry entry. */
+		dsm_segment *seg = dsm_create(size, 0);
+
+		dsm_pin_segment(seg);
+		dsm_pin_mapping(seg);
+		entry->handle = dsm_segment_handle(seg);
+		*ptr = dsm_segment_address(seg);
+
+		if (init_callback)
+			(*init_callback) (*ptr);
+	}
+	else
+	{
+		/* Attach to existing DSM registry entry. */
+		dsm_segment *seg = dsm_attach(entry->handle);
+
+		dsm_pin_mapping(seg);
+		*ptr = dsm_segment_address(seg);
+	}
+
+	dshash_release_lock(dsm_registry_table, entry);
+	MemoryContextSwitchTo(oldcontext);
+}
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 2225a4a6e6..034b656115 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -38,6 +38,7 @@
 #include "replication/walsender.h"
 #include "storage/bufmgr.h"
 #include "storage/dsm.h"
+#include "storage/dsm_registry.h"
 #include "storage/ipc.h"
 #include "storage/pg_shmem.h"
 #include "storage/pmsignal.h"
@@ -113,6 +114,7 @@ CalculateShmemSize(int *num_semaphores)
 	size = add_size(size, hash_estimate_size(SHMEM_INDEX_SIZE,
 											 sizeof(ShmemIndexEnt)));
 	size = add_size(size, dsm_estimate_size());
+	size = add_size(size, DSMRegistryShmemSize());
 	size = add_size(size, BufferShmemSize());
 	size = add_size(size, LockShmemSize());
 	size = add_size(size, PredicateLockShmemSize());
@@ -285,6 +287,7 @@ CreateOrAttachShmemStructs(void)
 	InitShmemIndex();
 
 	dsm_shmem_init();
+	DSMRegistryShmemInit();
 
 	/*
 	 * Set up xlog, clog, and buffers
diff --git a/src/backend/storage/ipc/meson.build b/src/backend/storage/ipc/meson.build
index 79a16d077f..88fef448be 100644
--- a/src/backend/storage/ipc/meson.build
+++ b/src/backend/storage/ipc/meson.build
@@ -4,6 +4,7 @@ backend_sources += files(
   'barrier.c',
   'dsm.c',
   'dsm_impl.c',
+  'dsm_registry.c',
   'ipc.c',
   'ipci.c',
   'latch.c',
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 315a78cda9..f3faa991d1 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -190,6 +190,10 @@ static const char *const BuiltinTrancheNames[] = {
 	"LogicalRepLauncherDSA",
 	/* LWTRANCHE_LAUNCHER_HASH: */
 	"LogicalRepLauncherHash",
+	/* LWTRANCHE_DSM_REGISTRY_DSA: */
+	"DSMRegistryDSA",
+	/* LWTRANCHE_DSM_REGISTRY_HASH: */
+	"DSMRegistryHash",
 };
 
 StaticAssertDecl(lengthof(BuiltinTrancheNames) ==
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index f72f2906ce..e8f679c8ae 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -54,3 +54,4 @@ XactTruncationLock					44
 WrapLimitsVacuumLock				46
 NotifyQueueTailLock					47
 WaitEventExtensionLock				48
+DSMRegistryLock						49
diff --git a/src/include/storage/dsm_registry.h b/src/include/storage/dsm_registry.h
new file mode 100644
index 0000000000..8c311e50ae
--- /dev/null
+++ b/src/include/storage/dsm_registry.h
@@ -0,0 +1,22 @@
+/*-------------------------------------------------------------------------
+ *
+ * dsm_registry.h
+ *
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/storage/dsm_registry.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef DSM_REGISTRY_H
+#define DSM_REGISTRY_H
+
+extern void dsm_registry_init_or_attach(const char *key, void **ptr, size_t size,
+										void (*init_callback) (void *ptr));
+
+extern Size DSMRegistryShmemSize(void);
+extern void DSMRegistryShmemInit(void);
+
+#endif							/* DSM_REGISTRY_H */
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index b038e599c0..665d471418 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -207,7 +207,9 @@ typedef enum BuiltinTrancheIds
 	LWTRANCHE_PGSTATS_DATA,
 	LWTRANCHE_LAUNCHER_DSA,
 	LWTRANCHE_LAUNCHER_HASH,
-	LWTRANCHE_FIRST_USER_DEFINED,
+	LWTRANCHE_DSM_REGISTRY_DSA,
+	LWTRANCHE_DSM_REGISTRY_HASH,
+	LWTRANCHE_FIRST_USER_DEFINED
 }			BuiltinTrancheIds;
 
 /*
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index d659adbfd6..c89a268d9e 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -606,6 +606,8 @@ DropSubscriptionStmt
 DropTableSpaceStmt
 DropUserMappingStmt
 DropdbStmt
+DSMRegistryCtxStruct
+DSMRegistryEntry
 DumpComponents
 DumpId
 DumpOptions
-- 
2.25.1



  [text/x-diff] v1-0002-test-dsm-registry.patch (7.5K, ../../20231205034647.GA2705267@nathanxps13/3-v1-0002-test-dsm-registry.patch)
  download | inline diff:
From 43c37b8dab039cb31efb8e65d67f1131b15c72b6 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Mon, 4 Dec 2023 16:40:30 -0600
Subject: [PATCH v1 2/2] test dsm registry

---
 src/test/modules/Makefile                     |  1 +
 src/test/modules/meson.build                  |  1 +
 src/test/modules/test_dsm_registry/.gitignore |  4 ++
 src/test/modules/test_dsm_registry/Makefile   | 23 ++++++++
 .../modules/test_dsm_registry/meson.build     | 33 ++++++++++++
 .../t/001_test_dsm_registry.pl                | 25 +++++++++
 .../test_dsm_registry--1.0.sql                | 10 ++++
 .../test_dsm_registry/test_dsm_registry.c     | 54 +++++++++++++++++++
 .../test_dsm_registry.control                 |  4 ++
 9 files changed, 155 insertions(+)
 create mode 100644 src/test/modules/test_dsm_registry/.gitignore
 create mode 100644 src/test/modules/test_dsm_registry/Makefile
 create mode 100644 src/test/modules/test_dsm_registry/meson.build
 create mode 100644 src/test/modules/test_dsm_registry/t/001_test_dsm_registry.pl
 create mode 100644 src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
 create mode 100644 src/test/modules/test_dsm_registry/test_dsm_registry.c
 create mode 100644 src/test/modules/test_dsm_registry/test_dsm_registry.control

diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 5d33fa6a9a..f656032589 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -18,6 +18,7 @@ SUBDIRS = \
 		  test_custom_rmgrs \
 		  test_ddl_deparse \
 		  test_dsa \
+		  test_dsm_registry \
 		  test_extensions \
 		  test_ginpostinglist \
 		  test_integerset \
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index b76f588559..bd53d52a3f 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -15,6 +15,7 @@ subdir('test_copy_callbacks')
 subdir('test_custom_rmgrs')
 subdir('test_ddl_deparse')
 subdir('test_dsa')
+subdir('test_dsm_registry')
 subdir('test_extensions')
 subdir('test_ginpostinglist')
 subdir('test_integerset')
diff --git a/src/test/modules/test_dsm_registry/.gitignore b/src/test/modules/test_dsm_registry/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_dsm_registry/Makefile b/src/test/modules/test_dsm_registry/Makefile
new file mode 100644
index 0000000000..6f2508bc37
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/Makefile
@@ -0,0 +1,23 @@
+# src/test/modules/test_dsm_registry/Makefile
+
+MODULE_big = test_dsm_registry
+OBJS = \
+	$(WIN32RES) \
+	test_dsm_registry.o
+PGFILEDESC = "test_dsm_registry - test code for the DSM registry"
+
+EXTENSION = test_dsm_registry
+DATA = test_dsm_registry--1.0.sql
+
+TAP_TESTS = 1
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_dsm_registry
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_dsm_registry/meson.build b/src/test/modules/test_dsm_registry/meson.build
new file mode 100644
index 0000000000..e5d8272d10
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/meson.build
@@ -0,0 +1,33 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+test_dsm_registry_sources = files(
+  'test_dsm_registry.c',
+)
+
+if host_system == 'windows'
+  test_dsm_registry_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'test_dsm_registry',
+    '--FILEDESC', 'test_dsm_registry - test code for the DSM registry',])
+endif
+
+test_dsm_registry = shared_module('test_dsm_registry',
+  test_dsm_registry_sources,
+  kwargs: pg_test_mod_args,
+)
+test_install_libs += test_dsm_registry
+
+test_install_data += files(
+  'test_dsm_registry.control',
+  'test_dsm_registry--1.0.sql',
+)
+
+tests += {
+  'name': 'test_dsm_registry',
+  'sd': meson.current_source_dir(),
+  'bd': meson.current_build_dir(),
+  'tap': {
+    'tests': [
+      't/001_test_dsm_registry.pl',
+    ],
+  },
+}
diff --git a/src/test/modules/test_dsm_registry/t/001_test_dsm_registry.pl b/src/test/modules/test_dsm_registry/t/001_test_dsm_registry.pl
new file mode 100644
index 0000000000..4ad6097d1c
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/t/001_test_dsm_registry.pl
@@ -0,0 +1,25 @@
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->start;
+
+$node->safe_psql('postgres', "CREATE DATABASE test;");
+$node->safe_psql('postgres', "CREATE EXTENSION test_dsm_registry;");
+$node->safe_psql('postgres', "SELECT set_val_in_shmem(1236);");
+
+$node->safe_psql('test', "CREATE EXTENSION test_dsm_registry;");
+my $result = $node->safe_psql('test', "SELECT get_val_in_shmem();");
+is($result, "1236", "check shmem val");
+
+$node->stop;
+
+done_testing();
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
new file mode 100644
index 0000000000..8c55b0919b
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
@@ -0,0 +1,10 @@
+/* src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_dsm_registry" to load this file. \quit
+
+CREATE FUNCTION set_val_in_shmem(val INT) RETURNS VOID
+	AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION get_val_in_shmem() RETURNS INT
+	AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry.c b/src/test/modules/test_dsm_registry/test_dsm_registry.c
new file mode 100644
index 0000000000..8f78012e52
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry.c
@@ -0,0 +1,54 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_dsm_registry.c
+ *		Test the DSM registry
+ *
+ * Copyright (c) 2023, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		src/test/modules/test_dsm_registry/test_dsm_registry.c
+ *
+ * -------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "port/atomics.h"
+#include "storage/dsm_registry.h"
+
+PG_MODULE_MAGIC;
+
+static pg_atomic_uint32 *val;
+
+static void
+init_val(void *ptr)
+{
+	pg_atomic_init_u32(ptr, 0);
+}
+
+static void
+dsm_registry_attach(void)
+{
+	dsm_registry_init_or_attach("test_dsm_registry", (void **) &val,
+								sizeof(pg_atomic_uint32), init_val);
+}
+
+PG_FUNCTION_INFO_V1(set_val_in_shmem);
+Datum
+set_val_in_shmem(PG_FUNCTION_ARGS)
+{
+	dsm_registry_attach();
+
+	(void) pg_atomic_exchange_u32(val, PG_GETARG_UINT32(0));
+
+	PG_RETURN_VOID();
+}
+
+PG_FUNCTION_INFO_V1(get_val_in_shmem);
+Datum
+get_val_in_shmem(PG_FUNCTION_ARGS)
+{
+	dsm_registry_attach();
+
+	PG_RETURN_UINT32(pg_atomic_fetch_add_u32(val, 0));
+}
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry.control b/src/test/modules/test_dsm_registry/test_dsm_registry.control
new file mode 100644
index 0000000000..813f099889
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry.control
@@ -0,0 +1,4 @@
+comment = 'Test code for the DSM registry'
+default_version = '1.0'
+module_pathname = '$libdir/test_dsm_registry'
+relocatable = true
-- 
2.25.1



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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
@ 2023-12-05 15:34 ` Joe Conway <[email protected]>
  2023-12-05 16:16   ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
  4 siblings, 1 reply; 26+ messages in thread

From: Joe Conway @ 2023-12-05 15:34 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; pgsql-hackers

On 12/4/23 22:46, Nathan Bossart wrote:
> Every once in a while, I find myself wanting to use shared memory in a
> loadable module without requiring it to be loaded at server start via
> shared_preload_libraries.  The DSM API offers a nice way to create and
> manage dynamic shared memory segments, so creating a segment after server
> start is easy enough.  However, AFAICT there's no easy way to teach other
> backends about the segment without storing the handles in shared memory,
> which puts us right back at square one.
> 
> The attached 0001 introduces a "DSM registry" to solve this problem.  The
> API provides an easy way to allocate/initialize a segment or to attach to
> an existing one.  The registry itself is just a dshash table that stores
> the handles keyed by a module-specified string.  0002 adds a test for the
> registry that demonstrates basic usage.
> 
> I don't presently have any concrete plans to use this for anything, but I
> thought it might be useful for extensions for caching, etc. and wanted to
> see whether there was any interest in the feature.

Notwithstanding any dragons there may be, and not having actually looked 
at the the patches, I love the concept! +<many>

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com







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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-05 15:34 ` Re: introduce dynamic shared memory registry Joe Conway <[email protected]>
@ 2023-12-05 16:16   ` Robert Haas <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Robert Haas @ 2023-12-05 16:16 UTC (permalink / raw)
  To: Joe Conway <[email protected]>; +Cc: Nathan Bossart <[email protected]>; pgsql-hackers

On Tue, Dec 5, 2023 at 10:35 AM Joe Conway <[email protected]> wrote:
> Notwithstanding any dragons there may be, and not having actually looked
> at the the patches, I love the concept! +<many>

Seems fine to me too. I haven't looked at the patches or searched for
dragons either, though.

-- 
Robert Haas
EDB: http://www.enterprisedb.com






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
@ 2023-12-08 07:36 ` Michael Paquier <[email protected]>
  4 siblings, 0 replies; 26+ messages in thread

From: Michael Paquier @ 2023-12-08 07:36 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers

On Mon, Dec 04, 2023 at 09:46:47PM -0600, Nathan Bossart wrote:
> The attached 0001 introduces a "DSM registry" to solve this problem.  The
> API provides an easy way to allocate/initialize a segment or to attach to
> an existing one.  The registry itself is just a dshash table that stores
> the handles keyed by a module-specified string.  0002 adds a test for the
> registry that demonstrates basic usage.
> 
> I don't presently have any concrete plans to use this for anything, but I
> thought it might be useful for extensions for caching, etc. and wanted to
> see whether there was any interest in the feature.

Yes, tracking that in a more central way can have many usages, so your
patch sounds like a good idea.  Note that we have one case in core
that be improved and make use of what you have here: autoprewarm.c.

The module supports the launch of dynamic workers but the library may
not be loaded with shared_preload_libraries, meaning that it can
allocate a chunk of shared memory worth a size of
AutoPrewarmSharedState without having requested it in a
shmem_request_hook.  AutoPrewarmSharedState could be moved to a DSM
and tracked with the shared hash table introduced by the patch instead
of acquiring AddinShmemInitLock while eating the plate of other
facilities that asked for a chunk of shmem, leaving any conflict
handling to dsm_registry_table.

+dsm_registry_init_or_attach(const char *key, void **ptr, size_t size,
+                           void (*init_callback) (void *ptr))

This is shaped around dshash_find_or_insert(), but it looks like you'd
want an equivalent for dshash_find(), as well.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
@ 2023-12-18 06:39 ` Andrei Lepikhov <[email protected]>
  2023-12-18 08:32   ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
  4 siblings, 1 reply; 26+ messages in thread

From: Andrei Lepikhov @ 2023-12-18 06:39 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; pgsql-hackers

On 5/12/2023 10:46, Nathan Bossart wrote:
> I don't presently have any concrete plans to use this for anything, but I
> thought it might be useful for extensions for caching, etc. and wanted to
> see whether there was any interest in the feature.

I am delighted that you commenced this thread.
Designing extensions, every time I feel pain introducing one shared 
value or some global stat, the extension must be required to be loadable 
on startup only. It reduces the flexibility of even very lightweight 
extensions, which look harmful to use in a cloud.

-- 
regards,
Andrei Lepikhov
Postgres Professional







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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-18 06:39 ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
@ 2023-12-18 08:32   ` Andrei Lepikhov <[email protected]>
  2023-12-18 09:05     ` Re: introduce dynamic shared memory registry Nikita Malakhov <[email protected]>
  2023-12-19 15:49     ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
  2023-12-19 16:01     ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  0 siblings, 3 replies; 26+ messages in thread

From: Andrei Lepikhov @ 2023-12-18 08:32 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; pgsql-hackers

On 18/12/2023 13:39, Andrei Lepikhov wrote:
> On 5/12/2023 10:46, Nathan Bossart wrote:
>> I don't presently have any concrete plans to use this for anything, but I
>> thought it might be useful for extensions for caching, etc. and wanted to
>> see whether there was any interest in the feature.
> 
> I am delighted that you commenced this thread.
> Designing extensions, every time I feel pain introducing one shared 
> value or some global stat, the extension must be required to be loadable 
> on startup only. It reduces the flexibility of even very lightweight 
> extensions, which look harmful to use in a cloud.

After looking into the code, I have some comments:
1. The code looks good; I didn't find possible mishaps. Some proposed 
changes are in the attachment.
2. I think a separate file for this feature looks too expensive. 
According to the gist of that code, it is a part of the DSA module.
3. The dsm_registry_init_or_attach routine allocates a DSM segment. Why 
not create dsa_area for a requestor and return it?

-- 
regards,
Andrei Lepikhov
Postgres Professional

diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
index ea80f45716..0343ce987f 100644
--- a/src/backend/storage/ipc/dsm_registry.c
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -45,8 +45,8 @@ static const dshash_parameters dsh_params = {
 	LWTRANCHE_DSM_REGISTRY_HASH
 };
 
-static dsa_area *dsm_registry_dsa;
-static dshash_table *dsm_registry_table;
+static dsa_area *dsm_registry_dsa = NULL;
+static dshash_table *dsm_registry_table = NULL;
 
 static void init_dsm_registry(void);
 
@@ -83,13 +83,20 @@ init_dsm_registry(void)
 {
 	/* Quick exit if we already did this. */
 	if (dsm_registry_table)
+	{
+		Assert(dsm_registry_dsa != NULL);
 		return;
+	}
+
+	Assert(dsm_registry_dsa == NULL);
 
 	/* Otherwise, use a lock to ensure only one process creates the table. */
 	LWLockAcquire(DSMRegistryLock, LW_EXCLUSIVE);
 
 	if (DSMRegistryCtx->dshh == DSHASH_HANDLE_INVALID)
 	{
+		Assert(DSMRegistryCtx->dsah == DSHASH_HANDLE_INVALID);
+
 		/* Initialize dynamic shared hash table for registry. */
 		dsm_registry_dsa = dsa_create(LWTRANCHE_DSM_REGISTRY_DSA);
 		dsa_pin(dsm_registry_dsa);
@@ -102,6 +109,8 @@ init_dsm_registry(void)
 	}
 	else
 	{
+		Assert(DSMRegistryCtx->dsah != DSHASH_HANDLE_INVALID);
+
 		/* Attach to existing dynamic shared hash table. */
 		dsm_registry_dsa = dsa_attach(DSMRegistryCtx->dsah);
 		dsa_pin_mapping(dsm_registry_dsa);
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 665d471418..e0e7b3b765 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -209,7 +209,7 @@ typedef enum BuiltinTrancheIds
 	LWTRANCHE_LAUNCHER_HASH,
 	LWTRANCHE_DSM_REGISTRY_DSA,
 	LWTRANCHE_DSM_REGISTRY_HASH,
-	LWTRANCHE_FIRST_USER_DEFINED
+	LWTRANCHE_FIRST_USER_DEFINED,
 }			BuiltinTrancheIds;
 
 /*


Attachments:

  [text/plain] additions.txt (1.8K, ../../[email protected]/2-additions.txt)
  download | inline diff:
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
index ea80f45716..0343ce987f 100644
--- a/src/backend/storage/ipc/dsm_registry.c
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -45,8 +45,8 @@ static const dshash_parameters dsh_params = {
 	LWTRANCHE_DSM_REGISTRY_HASH
 };
 
-static dsa_area *dsm_registry_dsa;
-static dshash_table *dsm_registry_table;
+static dsa_area *dsm_registry_dsa = NULL;
+static dshash_table *dsm_registry_table = NULL;
 
 static void init_dsm_registry(void);
 
@@ -83,13 +83,20 @@ init_dsm_registry(void)
 {
 	/* Quick exit if we already did this. */
 	if (dsm_registry_table)
+	{
+		Assert(dsm_registry_dsa != NULL);
 		return;
+	}
+
+	Assert(dsm_registry_dsa == NULL);
 
 	/* Otherwise, use a lock to ensure only one process creates the table. */
 	LWLockAcquire(DSMRegistryLock, LW_EXCLUSIVE);
 
 	if (DSMRegistryCtx->dshh == DSHASH_HANDLE_INVALID)
 	{
+		Assert(DSMRegistryCtx->dsah == DSHASH_HANDLE_INVALID);
+
 		/* Initialize dynamic shared hash table for registry. */
 		dsm_registry_dsa = dsa_create(LWTRANCHE_DSM_REGISTRY_DSA);
 		dsa_pin(dsm_registry_dsa);
@@ -102,6 +109,8 @@ init_dsm_registry(void)
 	}
 	else
 	{
+		Assert(DSMRegistryCtx->dsah != DSHASH_HANDLE_INVALID);
+
 		/* Attach to existing dynamic shared hash table. */
 		dsm_registry_dsa = dsa_attach(DSMRegistryCtx->dsah);
 		dsa_pin_mapping(dsm_registry_dsa);
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 665d471418..e0e7b3b765 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -209,7 +209,7 @@ typedef enum BuiltinTrancheIds
 	LWTRANCHE_LAUNCHER_HASH,
 	LWTRANCHE_DSM_REGISTRY_DSA,
 	LWTRANCHE_DSM_REGISTRY_HASH,
-	LWTRANCHE_FIRST_USER_DEFINED
+	LWTRANCHE_FIRST_USER_DEFINED,
 }			BuiltinTrancheIds;
 
 /*


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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-18 06:39 ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
  2023-12-18 08:32   ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
@ 2023-12-18 09:05     ` Nikita Malakhov <[email protected]>
  2023-12-19 16:09       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2 siblings, 1 reply; 26+ messages in thread

From: Nikita Malakhov @ 2023-12-18 09:05 UTC (permalink / raw)
  To: Andrei Lepikhov <[email protected]>; +Cc: Nathan Bossart <[email protected]>; pgsql-hackers

Hi!

This patch looks like a good solution for a pain in the ass, I'm too for
this patch to be committed.
Have looked through the code and agree with Andrei, the code looks good.
Just a suggestion - maybe it is worth adding a function for detaching the
segment,
for cases when we unload and/or re-load the extension?

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/


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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-18 06:39 ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
  2023-12-18 08:32   ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
  2023-12-18 09:05     ` Re: introduce dynamic shared memory registry Nikita Malakhov <[email protected]>
@ 2023-12-19 16:09       ` Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Nathan Bossart @ 2023-12-19 16:09 UTC (permalink / raw)
  To: Nikita Malakhov <[email protected]>; +Cc: Andrei Lepikhov <[email protected]>; pgsql-hackers

On Mon, Dec 18, 2023 at 12:05:28PM +0300, Nikita Malakhov wrote:
> Just a suggestion - maybe it is worth adding a function for detaching the
> segment,
> for cases when we unload and/or re-load the extension?

Hm.  We don't presently have a good way to unload a library, but you can
certainly DROP EXTENSION, in which case you might expect the segment to go
away or at least be reset.  But even today, once a preloaded library is
loaded, it stays loaded and its shared memory remains regardless of whether
you CREATE/DROP extension.  Can you think of problems with keeping the
segment attached?

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






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-18 06:39 ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
  2023-12-18 08:32   ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
@ 2023-12-19 15:49     ` Robert Haas <[email protected]>
  2 siblings, 0 replies; 26+ messages in thread

From: Robert Haas @ 2023-12-19 15:49 UTC (permalink / raw)
  To: Andrei Lepikhov <[email protected]>; +Cc: Nathan Bossart <[email protected]>; pgsql-hackers

On Mon, Dec 18, 2023 at 3:32 AM Andrei Lepikhov
<[email protected]> wrote:
> 2. I think a separate file for this feature looks too expensive.
> According to the gist of that code, it is a part of the DSA module.

-1. I think this is a totally different thing than DSA. More files
aren't nearly as expensive as the confusion that comes from smushing
unrelated things together.

-- 
Robert Haas
EDB: http://www.enterprisedb.com






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-18 06:39 ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
  2023-12-18 08:32   ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
@ 2023-12-19 16:01     ` Nathan Bossart <[email protected]>
  2 siblings, 0 replies; 26+ messages in thread

From: Nathan Bossart @ 2023-12-19 16:01 UTC (permalink / raw)
  To: Andrei Lepikhov <[email protected]>; +Cc: pgsql-hackers

On Mon, Dec 18, 2023 at 03:32:08PM +0700, Andrei Lepikhov wrote:
> 3. The dsm_registry_init_or_attach routine allocates a DSM segment. Why not
> create dsa_area for a requestor and return it?

My assumption is that most modules just need a fixed-size segment, and if
they really needed a DSA segment, the handle, tranche ID, etc. could just
be stored in the DSM segment.  Maybe that assumption is wrong, though...

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






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
@ 2023-12-20 09:58 ` Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  4 siblings, 1 reply; 26+ messages in thread

From: Bharath Rupireddy @ 2023-12-20 09:58 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers

On Tue, Dec 5, 2023 at 9:17 AM Nathan Bossart <[email protected]> wrote:
>
> Every once in a while, I find myself wanting to use shared memory in a
> loadable module without requiring it to be loaded at server start via
> shared_preload_libraries.  The DSM API offers a nice way to create and
> manage dynamic shared memory segments, so creating a segment after server
> start is easy enough.  However, AFAICT there's no easy way to teach other
> backends about the segment without storing the handles in shared memory,
> which puts us right back at square one.
>
> The attached 0001 introduces a "DSM registry" to solve this problem.  The
> API provides an easy way to allocate/initialize a segment or to attach to
> an existing one.  The registry itself is just a dshash table that stores
> the handles keyed by a module-specified string.  0002 adds a test for the
> registry that demonstrates basic usage.

+1 for something like this.

> I don't presently have any concrete plans to use this for anything, but I
> thought it might be useful for extensions for caching, etc. and wanted to
> see whether there was any interest in the feature.

Isn't the worker_spi best place to show the use of the DSM registry
instead of a separate test extension? Note the custom wait event
feature that added its usage code to worker_spi. Since worker_spi
demonstrates typical coding patterns, having just set_val_in_shmem()
and get_val_in_shmem() in there makes this patch simple and shaves
some code off.

Comments on the v1 patch set:

1. IIUC, this feature lets external modules create as many DSM
segments as possible with different keys right? If yes, is capping the
max number of DSMs a good idea?

2. Why does this feature have to deal with DSMs? Why not DSAs? With
DSA and an API that gives the DSA handle to the external modules, the
modules can dsa_allocate and dsa_free right? Do you see any problem
with it?

3.
+typedef struct DSMRegistryEntry
+{
+    char        key[256];

Key length 256 feels too much, can it be capped at NAMEDATALEN 64
bytes (similar to some of the key lengths for hash_create()) to start
with?

4. Do we need on_dsm_detach for each DSM created?
dsm_backend_shutdown

5.
+ *
+ * *ptr should initially be set to NULL.  If it is not NULL, this routine will
+ * assume that the segment has already been attached to the current session.
+ * Otherwise, this routine will set *ptr appropriately.

+    /* Quick exit if the value is already set. */
+    if (*ptr)
+        return;

Instead of the above assumption and quick exit condition, can it be
something like if (dsm_find_mapping(dsm_segment_handle(*ptr)) != NULL)
return;?

6.
+static pg_atomic_uint32 *val;

Any specific reason for it to be an atomic variable?

7.
+static pg_atomic_uint32 *val;

Instead of a run-of-the-mill example with just an integer val that
gets stored in shared memory, can it be something more realistic, a
struct with 2 or more variables or a struct to store linked list
(slist_head or dlist_head) in shared memory or such?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
@ 2023-12-20 16:03   ` Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Nathan Bossart @ 2023-12-20 16:03 UTC (permalink / raw)
  To: Bharath Rupireddy <[email protected]>; +Cc: pgsql-hackers

On Wed, Dec 20, 2023 at 03:28:38PM +0530, Bharath Rupireddy wrote:
> Isn't the worker_spi best place to show the use of the DSM registry
> instead of a separate test extension? Note the custom wait event
> feature that added its usage code to worker_spi. Since worker_spi
> demonstrates typical coding patterns, having just set_val_in_shmem()
> and get_val_in_shmem() in there makes this patch simple and shaves
> some code off.

I don't agree.  The test case really isn't that complicated, and I'd rather
have a dedicated test suite for this feature that we can build on instead
of trying to squeeze it into something unrelated.

> 1. IIUC, this feature lets external modules create as many DSM
> segments as possible with different keys right? If yes, is capping the
> max number of DSMs a good idea?

Why?  Even if it is a good idea, what limit could we choose that wouldn't
be arbitrary and eventually cause problems down the road?

> 2. Why does this feature have to deal with DSMs? Why not DSAs? With
> DSA and an API that gives the DSA handle to the external modules, the
> modules can dsa_allocate and dsa_free right? Do you see any problem
> with it?

Please see upthread discussion [0].

> +typedef struct DSMRegistryEntry
> +{
> +    char        key[256];
> 
> Key length 256 feels too much, can it be capped at NAMEDATALEN 64
> bytes (similar to some of the key lengths for hash_create()) to start
> with?

Why is it too much?

> 4. Do we need on_dsm_detach for each DSM created?

Presently, I've designed this such that the DSM remains attached for the
lifetime of a session (and stays present even if all attached sessions go
away) to mimic what you get when you allocate shared memory during startup.
Perhaps there's a use-case for having backends do some cleanup before
exiting, in which case a detach_cb might be useful.  IMHO we should wait
for a concrete use-case before adding too many bells and whistles, though.

> + * *ptr should initially be set to NULL.  If it is not NULL, this routine will
> + * assume that the segment has already been attached to the current session.
> + * Otherwise, this routine will set *ptr appropriately.
> 
> +    /* Quick exit if the value is already set. */
> +    if (*ptr)
> +        return;
> 
> Instead of the above assumption and quick exit condition, can it be
> something like if (dsm_find_mapping(dsm_segment_handle(*ptr)) != NULL)
> return;?

Yeah, I think something like that could be better.  One of the things I
dislike about the v1 API is that it depends a little too much on the caller
doing exactly the right things, and I think it's possible to make it a
little more robust.

> +static pg_atomic_uint32 *val;
> 
> Any specific reason for it to be an atomic variable?

A regular integer would probably be fine for testing, but I figured we
might as well ensure correctness for when this code is inevitably
copy/pasted somewhere.

> +static pg_atomic_uint32 *val;
> 
> Instead of a run-of-the-mill example with just an integer val that
> gets stored in shared memory, can it be something more realistic, a
> struct with 2 or more variables or a struct to store linked list
> (slist_head or dlist_head) in shared memory or such?

This is the second time this has come up [1].  The intent of this test is
to verify the DSM registry behavior, not how folks are going to use the
shared memory it manages, so I'm really not inclined to make this more
complicated without a good reason.  I don't mind changing this if I'm
outvoted on this one, though.

[0] https://postgr.es/m/20231219160117.GB831499%40nathanxps13
[1] https://postgr.es/m/20231220153342.GA833819%40nathanxps13

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






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
@ 2023-12-29 15:23     ` Bharath Rupireddy <[email protected]>
  2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Bharath Rupireddy @ 2023-12-29 15:23 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers

On Wed, Dec 20, 2023 at 9:33 PM Nathan Bossart <[email protected]> wrote:
>
> On Wed, Dec 20, 2023 at 03:28:38PM +0530, Bharath Rupireddy wrote:
> > Isn't the worker_spi best place to show the use of the DSM registry
> > instead of a separate test extension? Note the custom wait event
> > feature that added its usage code to worker_spi. Since worker_spi
> > demonstrates typical coding patterns, having just set_val_in_shmem()
> > and get_val_in_shmem() in there makes this patch simple and shaves
> > some code off.
>
> I don't agree.  The test case really isn't that complicated, and I'd rather
> have a dedicated test suite for this feature that we can build on instead
> of trying to squeeze it into something unrelated.

With the use of dsm registry for pg_prewarm, do we need this
test_dsm_registry module at all? Because 0002 patch pretty much
demonstrates how to use the DSM registry. With this comment and my
earlier comment on incorporating the use of dsm registry in
worker_spi, I'm trying to make a point to reduce the code for this
feature. However, if others have different opinions, I'm okay with
having a separate test module.

> > 1. IIUC, this feature lets external modules create as many DSM
> > segments as possible with different keys right? If yes, is capping the
> > max number of DSMs a good idea?
>
> Why?  Even if it is a good idea, what limit could we choose that wouldn't
> be arbitrary and eventually cause problems down the road?

I've tried with a shared memory structure size of 10GB on my
development machine and I have seen an intermittent crash (I haven't
got a chance to dive deep into it). I think the shared memory that a
named-DSM segment can get via this DSM registry feature depends on the
total shared memory area that a typical DSM client can allocate today.
I'm not sure it's worth it to limit the shared memory for this feature
given that we don't limit the shared memory via startup hook.

> > 2. Why does this feature have to deal with DSMs? Why not DSAs? With
> > DSA and an API that gives the DSA handle to the external modules, the
> > modules can dsa_allocate and dsa_free right? Do you see any problem
> > with it?
>
> Please see upthread discussion [0].

Per my understanding, this feature allows one to define and manage
named-DSM segments.

> > +typedef struct DSMRegistryEntry
> > +{
> > +    char        key[256];
> >
> > Key length 256 feels too much, can it be capped at NAMEDATALEN 64
> > bytes (similar to some of the key lengths for hash_create()) to start
> > with?
>
> Why is it too much?

Are we expecting, for instance, a 128-bit UUID being used as a key and
hence limiting it to a higher value 256 instead of just NAMEDATALEN?
My thoughts were around saving a few bytes of shared memory space that
can get higher when multiple modules using a DSM registry with
multiple DSM segments.

> > 4. Do we need on_dsm_detach for each DSM created?
>
> Presently, I've designed this such that the DSM remains attached for the
> lifetime of a session (and stays present even if all attached sessions go
> away) to mimic what you get when you allocate shared memory during startup.
> Perhaps there's a use-case for having backends do some cleanup before
> exiting, in which case a detach_cb might be useful.  IMHO we should wait
> for a concrete use-case before adding too many bells and whistles, though.

On Thu, Dec 28, 2023 at 9:05 PM Nathan Bossart <[email protected]> wrote:
>
> On Wed, Dec 27, 2023 at 01:53:27PM -0600, Nathan Bossart wrote:
> > Here is a new version of the patch.  In addition to various small changes,
> > I've rewritten the test suite to use an integer and a lock, added a
> > dsm_registry_find() function, and adjusted autoprewarm to use the registry.
>
> Here's a v3 that fixes a silly mistake in the test.

Some comments on the v3 patch set:

1. Typo: missing "an" before "already-attached".
+        /* Return address of already-attached DSM registry entry. */

2. Do you see any immediate uses of dsm_registry_find()? Especially
given that dsm_registry_init_or_attach() does the necessary work
(returns the DSM address if DSM already exists for a given key) for a
postgres process. If there is no immediate use (the argument to remove
this function goes similar to detach_cb above), I'm sure we can
introduce it when there's one.

3. I think we don't need miscadmin.h inclusion in autoprewarm.c, do we?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
@ 2024-01-02 16:20       ` Nathan Bossart <[email protected]>
  2024-01-02 16:31         ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Nathan Bossart @ 2024-01-02 16:20 UTC (permalink / raw)
  To: Bharath Rupireddy <[email protected]>; +Cc: pgsql-hackers

On Fri, Dec 29, 2023 at 08:53:54PM +0530, Bharath Rupireddy wrote:
> With the use of dsm registry for pg_prewarm, do we need this
> test_dsm_registry module at all? Because 0002 patch pretty much
> demonstrates how to use the DSM registry. With this comment and my
> earlier comment on incorporating the use of dsm registry in
> worker_spi, I'm trying to make a point to reduce the code for this
> feature. However, if others have different opinions, I'm okay with
> having a separate test module.

I don't have a strong opinion here, but I lean towards still having a
dedicated test suite, if for no other reason that to guarantee some
coverage even if the other in-tree uses disappear.

> I've tried with a shared memory structure size of 10GB on my
> development machine and I have seen an intermittent crash (I haven't
> got a chance to dive deep into it). I think the shared memory that a
> named-DSM segment can get via this DSM registry feature depends on the
> total shared memory area that a typical DSM client can allocate today.
> I'm not sure it's worth it to limit the shared memory for this feature
> given that we don't limit the shared memory via startup hook.

I would be interested to see more details about the crashes you are seeing.
Is this unique to the registry or an existing problem with DSM segments?

> Are we expecting, for instance, a 128-bit UUID being used as a key and
> hence limiting it to a higher value 256 instead of just NAMEDATALEN?
> My thoughts were around saving a few bytes of shared memory space that
> can get higher when multiple modules using a DSM registry with
> multiple DSM segments.

I'm not really expecting folks to use more than, say, 16 characters for the
key, but I intentionally set it much higher in case someone did have a
reason to use longer keys.  I'll lower it to 64 in the next revision unless
anyone else objects.

> 2. Do you see any immediate uses of dsm_registry_find()? Especially
> given that dsm_registry_init_or_attach() does the necessary work
> (returns the DSM address if DSM already exists for a given key) for a
> postgres process. If there is no immediate use (the argument to remove
> this function goes similar to detach_cb above), I'm sure we can
> introduce it when there's one.

See [0].  FWIW I tend to agree that we could leave this one out for now.

> 3. I think we don't need miscadmin.h inclusion in autoprewarm.c, do we?

I'll take a look at this one.

[0] https://postgr.es/m/ZYIu_JL-usgd3E1q%40paquier.xyz

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






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
@ 2024-01-02 16:31         ` Robert Haas <[email protected]>
  2024-01-02 22:49           ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Robert Haas @ 2024-01-02 16:31 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; pgsql-hackers

On Tue, Jan 2, 2024 at 11:21 AM Nathan Bossart <[email protected]> wrote:
> > Are we expecting, for instance, a 128-bit UUID being used as a key and
> > hence limiting it to a higher value 256 instead of just NAMEDATALEN?
> > My thoughts were around saving a few bytes of shared memory space that
> > can get higher when multiple modules using a DSM registry with
> > multiple DSM segments.
>
> I'm not really expecting folks to use more than, say, 16 characters for the
> key, but I intentionally set it much higher in case someone did have a
> reason to use longer keys.  I'll lower it to 64 in the next revision unless
> anyone else objects.

This surely doesn't matter either way. We're not expecting this hash
table to have more than a handful of entries; the difference between
256, 64, and NAMEDATALEN won't even add up to kilobytes in any
realistic scenario, let along MB or GB.

-- 
Robert Haas
EDB: http://www.enterprisedb.com






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-02 16:31         ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
@ 2024-01-02 22:49           ` Nathan Bossart <[email protected]>
  2024-01-06 14:04             ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Nathan Bossart @ 2024-01-02 22:49 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; pgsql-hackers

Here's a new version of the patch set with Bharath's feedback addressed.

On Tue, Jan 02, 2024 at 11:31:14AM -0500, Robert Haas wrote:
> On Tue, Jan 2, 2024 at 11:21 AM Nathan Bossart <[email protected]> wrote:
>> > Are we expecting, for instance, a 128-bit UUID being used as a key and
>> > hence limiting it to a higher value 256 instead of just NAMEDATALEN?
>> > My thoughts were around saving a few bytes of shared memory space that
>> > can get higher when multiple modules using a DSM registry with
>> > multiple DSM segments.
>>
>> I'm not really expecting folks to use more than, say, 16 characters for the
>> key, but I intentionally set it much higher in case someone did have a
>> reason to use longer keys.  I'll lower it to 64 in the next revision unless
>> anyone else objects.
> 
> This surely doesn't matter either way. We're not expecting this hash
> table to have more than a handful of entries; the difference between
> 256, 64, and NAMEDATALEN won't even add up to kilobytes in any
> realistic scenario, let along MB or GB.

Right.

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


Attachments:

  [text/x-diff] v4-0001-add-dsm-registry.patch (19.2K, ../../20240102224907.GA1246933@nathanxps13/2-v4-0001-add-dsm-registry.patch)
  download | inline diff:
From 01343422efdd4bb0d3ccc4c45aa7e964ca5d04d0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 11 Oct 2023 22:07:26 -0500
Subject: [PATCH v4 1/2] add dsm registry

---
 src/backend/storage/ipc/Makefile              |   1 +
 src/backend/storage/ipc/dsm_registry.c        | 176 ++++++++++++++++++
 src/backend/storage/ipc/ipci.c                |   3 +
 src/backend/storage/ipc/meson.build           |   1 +
 src/backend/storage/lmgr/lwlock.c             |   4 +
 src/backend/storage/lmgr/lwlocknames.txt      |   1 +
 .../utils/activity/wait_event_names.txt       |   3 +
 src/include/storage/dsm_registry.h            |  23 +++
 src/include/storage/lwlock.h                  |   2 +
 src/test/modules/Makefile                     |   1 +
 src/test/modules/meson.build                  |   1 +
 src/test/modules/test_dsm_registry/.gitignore |   4 +
 src/test/modules/test_dsm_registry/Makefile   |  23 +++
 .../expected/test_dsm_registry.out            |  14 ++
 .../modules/test_dsm_registry/meson.build     |  33 ++++
 .../sql/test_dsm_registry.sql                 |   4 +
 .../test_dsm_registry--1.0.sql                |  10 +
 .../test_dsm_registry/test_dsm_registry.c     |  75 ++++++++
 .../test_dsm_registry.control                 |   4 +
 src/tools/pgindent/typedefs.list              |   3 +
 20 files changed, 386 insertions(+)
 create mode 100644 src/backend/storage/ipc/dsm_registry.c
 create mode 100644 src/include/storage/dsm_registry.h
 create mode 100644 src/test/modules/test_dsm_registry/.gitignore
 create mode 100644 src/test/modules/test_dsm_registry/Makefile
 create mode 100644 src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
 create mode 100644 src/test/modules/test_dsm_registry/meson.build
 create mode 100644 src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
 create mode 100644 src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
 create mode 100644 src/test/modules/test_dsm_registry/test_dsm_registry.c
 create mode 100644 src/test/modules/test_dsm_registry/test_dsm_registry.control

diff --git a/src/backend/storage/ipc/Makefile b/src/backend/storage/ipc/Makefile
index 6d5b921038..d8a1653eb6 100644
--- a/src/backend/storage/ipc/Makefile
+++ b/src/backend/storage/ipc/Makefile
@@ -12,6 +12,7 @@ OBJS = \
 	barrier.o \
 	dsm.o \
 	dsm_impl.o \
+	dsm_registry.o \
 	ipc.o \
 	ipci.o \
 	latch.o \
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
new file mode 100644
index 0000000000..2b2be4bb99
--- /dev/null
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -0,0 +1,176 @@
+/*-------------------------------------------------------------------------
+ *
+ * dsm_registry.c
+ *
+ * Functions for interfacing with the dynamic shared memory registry.  This
+ * provides a way for libraries to use shared memory without needing to
+ * request it at startup time via a shmem_request_hook.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ *	  src/backend/storage/ipc/dsm_registry.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "lib/dshash.h"
+#include "storage/dsm_registry.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
+#include "utils/memutils.h"
+
+typedef struct DSMRegistryCtxStruct
+{
+	dsa_handle	dsah;
+	dshash_table_handle dshh;
+} DSMRegistryCtxStruct;
+
+static DSMRegistryCtxStruct *DSMRegistryCtx;
+
+typedef struct DSMRegistryEntry
+{
+	char		key[64];
+	dsm_handle	handle;
+} DSMRegistryEntry;
+
+static const dshash_parameters dsh_params = {
+	offsetof(DSMRegistryEntry, handle),
+	sizeof(DSMRegistryEntry),
+	dshash_memcmp,
+	dshash_memhash,
+	LWTRANCHE_DSM_REGISTRY_HASH
+};
+
+static dsa_area *dsm_registry_dsa;
+static dshash_table *dsm_registry_table;
+
+static void init_dsm_registry(void);
+
+Size
+DSMRegistryShmemSize(void)
+{
+	return MAXALIGN(sizeof(DSMRegistryCtxStruct));
+}
+
+void
+DSMRegistryShmemInit(void)
+{
+	bool		found;
+
+	DSMRegistryCtx = (DSMRegistryCtxStruct *)
+		ShmemInitStruct("DSM Registry Data",
+						DSMRegistryShmemSize(),
+						&found);
+
+	if (!found)
+	{
+		DSMRegistryCtx->dsah = DSA_HANDLE_INVALID;
+		DSMRegistryCtx->dshh = DSHASH_HANDLE_INVALID;
+	}
+}
+
+/*
+ * Initialize or attach to the dynamic shared hash table that stores the DSM
+ * registry entries, if not already done.  This must be called before accessing
+ * the table.
+ */
+static void
+init_dsm_registry(void)
+{
+	/* Quick exit if we already did this. */
+	if (dsm_registry_table)
+		return;
+
+	/* Otherwise, use a lock to ensure only one process creates the table. */
+	LWLockAcquire(DSMRegistryLock, LW_EXCLUSIVE);
+
+	if (DSMRegistryCtx->dshh == DSHASH_HANDLE_INVALID)
+	{
+		/* Initialize dynamic shared hash table for registry. */
+		dsm_registry_dsa = dsa_create(LWTRANCHE_DSM_REGISTRY_DSA);
+		dsa_pin(dsm_registry_dsa);
+		dsa_pin_mapping(dsm_registry_dsa);
+		dsm_registry_table = dshash_create(dsm_registry_dsa, &dsh_params, 0);
+
+		/* Store handles in shared memory for other backends to use. */
+		DSMRegistryCtx->dsah = dsa_get_handle(dsm_registry_dsa);
+		DSMRegistryCtx->dshh = dshash_get_hash_table_handle(dsm_registry_table);
+	}
+	else
+	{
+		/* Attach to existing dynamic shared hash table. */
+		dsm_registry_dsa = dsa_attach(DSMRegistryCtx->dsah);
+		dsa_pin_mapping(dsm_registry_dsa);
+		dsm_registry_table = dshash_attach(dsm_registry_dsa, &dsh_params,
+										   DSMRegistryCtx->dshh, 0);
+	}
+
+	LWLockRelease(DSMRegistryLock);
+}
+
+/*
+ * Initialize or attach a DSM entry.
+ *
+ * This routine returns the address of the segment.  init_callback is called to
+ * initialize the segment when it is first created.
+ */
+void *
+dsm_registry_init_or_attach(const char *key, size_t size,
+							void (*init_callback) (void *ptr), bool *found)
+{
+	DSMRegistryEntry *entry;
+	MemoryContext oldcontext;
+	char		key_padded[offsetof(DSMRegistryEntry, handle)] = {0};
+	void	   *ret;
+
+	Assert(key);
+	Assert(size);
+	Assert(found);
+
+	if (strlen(key) >= offsetof(DSMRegistryEntry, handle))
+		elog(ERROR, "DSM registry key too long");
+
+	/* Be sure any local memory allocated by DSM/DSA routines is persistent. */
+	oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+	/* Connect to the registry. */
+	init_dsm_registry();
+
+	strcpy(key_padded, key);
+	entry = dshash_find_or_insert(dsm_registry_table, key_padded, found);
+	if (!(*found))
+	{
+		/* Initialize DSM registry entry. */
+		dsm_segment *seg = dsm_create(size, 0);
+
+		dsm_pin_segment(seg);
+		dsm_pin_mapping(seg);
+		entry->handle = dsm_segment_handle(seg);
+		ret = dsm_segment_address(seg);
+
+		if (init_callback)
+			(*init_callback) (ret);
+	}
+	else if (!dsm_find_mapping(entry->handle))
+	{
+		/* Attach to existing DSM registry entry. */
+		dsm_segment *seg = dsm_attach(entry->handle);
+
+		dsm_pin_mapping(seg);
+		ret = dsm_segment_address(seg);
+	}
+	else
+	{
+		/* Return address of already-attached DSM registry entry. */
+		ret = dsm_segment_address(dsm_find_mapping(entry->handle));
+	}
+
+	dshash_release_lock(dsm_registry_table, entry);
+	MemoryContextSwitchTo(oldcontext);
+
+	return ret;
+}
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 706140eb9f..44d6a243e5 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -40,6 +40,7 @@
 #include "replication/walsender.h"
 #include "storage/bufmgr.h"
 #include "storage/dsm.h"
+#include "storage/dsm_registry.h"
 #include "storage/ipc.h"
 #include "storage/pg_shmem.h"
 #include "storage/pmsignal.h"
@@ -115,6 +116,7 @@ CalculateShmemSize(int *num_semaphores)
 	size = add_size(size, hash_estimate_size(SHMEM_INDEX_SIZE,
 											 sizeof(ShmemIndexEnt)));
 	size = add_size(size, dsm_estimate_size());
+	size = add_size(size, DSMRegistryShmemSize());
 	size = add_size(size, BufferShmemSize());
 	size = add_size(size, LockShmemSize());
 	size = add_size(size, PredicateLockShmemSize());
@@ -289,6 +291,7 @@ CreateOrAttachShmemStructs(void)
 	InitShmemIndex();
 
 	dsm_shmem_init();
+	DSMRegistryShmemInit();
 
 	/*
 	 * Set up xlog, clog, and buffers
diff --git a/src/backend/storage/ipc/meson.build b/src/backend/storage/ipc/meson.build
index 79a16d077f..88fef448be 100644
--- a/src/backend/storage/ipc/meson.build
+++ b/src/backend/storage/ipc/meson.build
@@ -4,6 +4,7 @@ backend_sources += files(
   'barrier.c',
   'dsm.c',
   'dsm_impl.c',
+  'dsm_registry.c',
   'ipc.c',
   'ipci.c',
   'latch.c',
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 315a78cda9..f3faa991d1 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -190,6 +190,10 @@ static const char *const BuiltinTrancheNames[] = {
 	"LogicalRepLauncherDSA",
 	/* LWTRANCHE_LAUNCHER_HASH: */
 	"LogicalRepLauncherHash",
+	/* LWTRANCHE_DSM_REGISTRY_DSA: */
+	"DSMRegistryDSA",
+	/* LWTRANCHE_DSM_REGISTRY_HASH: */
+	"DSMRegistryHash",
 };
 
 StaticAssertDecl(lengthof(BuiltinTrancheNames) ==
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index d621f5507f..ef8542de46 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -55,3 +55,4 @@ WrapLimitsVacuumLock				46
 NotifyQueueTailLock					47
 WaitEventExtensionLock				48
 WALSummarizerLock					49
+DSMRegistryLock						50
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index f61ec3e59d..f13077bd8c 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -325,6 +325,7 @@ WrapLimitsVacuum	"Waiting to update limits on transaction id and multixact consu
 NotifyQueueTail	"Waiting to update limit on <command>NOTIFY</command> message storage."
 WaitEventExtension	"Waiting to read or update custom wait events information for extensions."
 WALSummarizer	"Waiting to read or update WAL summarization state."
+DSMRegistry	"Waiting to read or update the dynamic shared memory registry."
 
 XactBuffer	"Waiting for I/O on a transaction status SLRU buffer."
 CommitTsBuffer	"Waiting for I/O on a commit timestamp SLRU buffer."
@@ -355,6 +356,8 @@ PgStatsHash	"Waiting for stats shared memory hash table access."
 PgStatsData	"Waiting for shared memory stats data access."
 LogicalRepLauncherDSA	"Waiting to access logical replication launcher's dynamic shared memory allocator."
 LogicalRepLauncherHash	"Waiting to access logical replication launcher's shared hash table."
+DSMRegistryDSA	"Waiting to access dynamic shared memory registry's dynamic shared memory allocator."
+DSMRegistryHash	"Waiting to access dynamic shared memory registry's shared hash table."
 
 #
 # Wait Events - Lock
diff --git a/src/include/storage/dsm_registry.h b/src/include/storage/dsm_registry.h
new file mode 100644
index 0000000000..247ac0acc1
--- /dev/null
+++ b/src/include/storage/dsm_registry.h
@@ -0,0 +1,23 @@
+/*-------------------------------------------------------------------------
+ *
+ * dsm_registry.h
+ *
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/storage/dsm_registry.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef DSM_REGISTRY_H
+#define DSM_REGISTRY_H
+
+extern void *dsm_registry_init_or_attach(const char *key, size_t size,
+										 void (*init_callback) (void *ptr),
+										 bool *found);
+
+extern Size DSMRegistryShmemSize(void);
+extern void DSMRegistryShmemInit(void);
+
+#endif							/* DSM_REGISTRY_H */
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index b038e599c0..e0e7b3b765 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -207,6 +207,8 @@ typedef enum BuiltinTrancheIds
 	LWTRANCHE_PGSTATS_DATA,
 	LWTRANCHE_LAUNCHER_DSA,
 	LWTRANCHE_LAUNCHER_HASH,
+	LWTRANCHE_DSM_REGISTRY_DSA,
+	LWTRANCHE_DSM_REGISTRY_HASH,
 	LWTRANCHE_FIRST_USER_DEFINED,
 }			BuiltinTrancheIds;
 
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 5d33fa6a9a..f656032589 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -18,6 +18,7 @@ SUBDIRS = \
 		  test_custom_rmgrs \
 		  test_ddl_deparse \
 		  test_dsa \
+		  test_dsm_registry \
 		  test_extensions \
 		  test_ginpostinglist \
 		  test_integerset \
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index b76f588559..bd53d52a3f 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -15,6 +15,7 @@ subdir('test_copy_callbacks')
 subdir('test_custom_rmgrs')
 subdir('test_ddl_deparse')
 subdir('test_dsa')
+subdir('test_dsm_registry')
 subdir('test_extensions')
 subdir('test_ginpostinglist')
 subdir('test_integerset')
diff --git a/src/test/modules/test_dsm_registry/.gitignore b/src/test/modules/test_dsm_registry/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_dsm_registry/Makefile b/src/test/modules/test_dsm_registry/Makefile
new file mode 100644
index 0000000000..b13e99a354
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/Makefile
@@ -0,0 +1,23 @@
+# src/test/modules/test_dsm_registry/Makefile
+
+MODULE_big = test_dsm_registry
+OBJS = \
+	$(WIN32RES) \
+	test_dsm_registry.o
+PGFILEDESC = "test_dsm_registry - test code for the DSM registry"
+
+EXTENSION = test_dsm_registry
+DATA = test_dsm_registry--1.0.sql
+
+REGRESS = test_dsm_registry
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_dsm_registry
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
new file mode 100644
index 0000000000..8ffbd343a0
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
@@ -0,0 +1,14 @@
+CREATE EXTENSION test_dsm_registry;
+SELECT set_val_in_shmem(1236);
+ set_val_in_shmem 
+------------------
+ 
+(1 row)
+
+\c
+SELECT get_val_in_shmem();
+ get_val_in_shmem 
+------------------
+             1236
+(1 row)
+
diff --git a/src/test/modules/test_dsm_registry/meson.build b/src/test/modules/test_dsm_registry/meson.build
new file mode 100644
index 0000000000..4a7992109b
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/meson.build
@@ -0,0 +1,33 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+test_dsm_registry_sources = files(
+  'test_dsm_registry.c',
+)
+
+if host_system == 'windows'
+  test_dsm_registry_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'test_dsm_registry',
+    '--FILEDESC', 'test_dsm_registry - test code for the DSM registry',])
+endif
+
+test_dsm_registry = shared_module('test_dsm_registry',
+  test_dsm_registry_sources,
+  kwargs: pg_test_mod_args,
+)
+test_install_libs += test_dsm_registry
+
+test_install_data += files(
+  'test_dsm_registry.control',
+  'test_dsm_registry--1.0.sql',
+)
+
+tests += {
+  'name': 'test_dsm_registry',
+  'sd': meson.current_source_dir(),
+  'bd': meson.current_build_dir(),
+  'regress': {
+    'sql': [
+      'test_dsm_registry',
+    ],
+  },
+}
diff --git a/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
new file mode 100644
index 0000000000..b3351be0a1
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
@@ -0,0 +1,4 @@
+CREATE EXTENSION test_dsm_registry;
+SELECT set_val_in_shmem(1236);
+\c
+SELECT get_val_in_shmem();
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
new file mode 100644
index 0000000000..8c55b0919b
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
@@ -0,0 +1,10 @@
+/* src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_dsm_registry" to load this file. \quit
+
+CREATE FUNCTION set_val_in_shmem(val INT) RETURNS VOID
+	AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION get_val_in_shmem() RETURNS INT
+	AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry.c b/src/test/modules/test_dsm_registry/test_dsm_registry.c
new file mode 100644
index 0000000000..068f47ed02
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry.c
@@ -0,0 +1,75 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_dsm_registry.c
+ *		Test the DSM registry
+ *
+ * Copyright (c) 2023, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		src/test/modules/test_dsm_registry/test_dsm_registry.c
+ *
+ * -------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "storage/dsm_registry.h"
+#include "storage/lwlock.h"
+
+PG_MODULE_MAGIC;
+
+typedef struct TestDSMRegistryStruct
+{
+	int			val;
+	LWLock		lck;
+} TestDSMRegistryStruct;
+
+static TestDSMRegistryStruct *tdr_state;
+
+static void
+init_state(void *ptr)
+{
+	TestDSMRegistryStruct *state = (TestDSMRegistryStruct *) ptr;
+
+	LWLockInitialize(&state->lck, LWLockNewTrancheId());
+	state->val = 0;
+}
+
+static void
+dsm_registry_attach(void)
+{
+	bool		found;
+
+	tdr_state = dsm_registry_init_or_attach("test_dsm_registry",
+											sizeof(TestDSMRegistryStruct),
+											init_state, &found);
+	LWLockRegisterTranche(tdr_state->lck.tranche, "test_dsm_registry");
+}
+
+PG_FUNCTION_INFO_V1(set_val_in_shmem);
+Datum
+set_val_in_shmem(PG_FUNCTION_ARGS)
+{
+	dsm_registry_attach();
+
+	LWLockAcquire(&tdr_state->lck, LW_EXCLUSIVE);
+	tdr_state->val = PG_GETARG_UINT32(0);
+	LWLockRelease(&tdr_state->lck);
+
+	PG_RETURN_VOID();
+}
+
+PG_FUNCTION_INFO_V1(get_val_in_shmem);
+Datum
+get_val_in_shmem(PG_FUNCTION_ARGS)
+{
+	int			ret;
+
+	dsm_registry_attach();
+
+	LWLockAcquire(&tdr_state->lck, LW_SHARED);
+	ret = tdr_state->val;
+	LWLockRelease(&tdr_state->lck);
+
+	PG_RETURN_UINT32(ret);
+}
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry.control b/src/test/modules/test_dsm_registry/test_dsm_registry.control
new file mode 100644
index 0000000000..813f099889
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry.control
@@ -0,0 +1,4 @@
+comment = 'Test code for the DSM registry'
+default_version = '1.0'
+module_pathname = '$libdir/test_dsm_registry'
+relocatable = true
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 5fd46b7bd1..469f7570f5 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -610,6 +610,8 @@ DropSubscriptionStmt
 DropTableSpaceStmt
 DropUserMappingStmt
 DropdbStmt
+DSMRegistryCtxStruct
+DSMRegistryEntry
 DumpComponents
 DumpId
 DumpOptions
@@ -2799,6 +2801,7 @@ Tcl_NotifierProcs
 Tcl_Obj
 Tcl_Time
 TempNamespaceStatus
+TestDSMRegistryStruct
 TestDecodingData
 TestDecodingTxnData
 TestSpec
-- 
2.25.1



  [text/x-diff] v4-0002-use-dsm-registry-for-pg_prewarm.patch (3.1K, ../../20240102224907.GA1246933@nathanxps13/3-v4-0002-use-dsm-registry-for-pg_prewarm.patch)
  download | inline diff:
From 30e083b2930ce88bf7b9bbb925bd24953074635b Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 26 Dec 2023 22:25:45 -0600
Subject: [PATCH v4 2/2] use dsm registry for pg_prewarm

---
 contrib/pg_prewarm/autoprewarm.c | 45 ++++++++++----------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 0993bd2453..56406ab6ba 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -32,12 +32,12 @@
 #include "access/xact.h"
 #include "catalog/pg_class.h"
 #include "catalog/pg_type.h"
-#include "miscadmin.h"
 #include "pgstat.h"
 #include "postmaster/bgworker.h"
 #include "postmaster/interrupt.h"
 #include "storage/buf_internals.h"
 #include "storage/dsm.h"
+#include "storage/dsm_registry.h"
 #include "storage/fd.h"
 #include "storage/ipc.h"
 #include "storage/latch.h"
@@ -95,8 +95,6 @@ static void apw_start_database_worker(void);
 static bool apw_init_shmem(void);
 static void apw_detach_shmem(int code, Datum arg);
 static int	apw_compare_blockinfo(const void *p, const void *q);
-static void autoprewarm_shmem_request(void);
-static shmem_request_hook_type prev_shmem_request_hook = NULL;
 
 /* Pointer to shared-memory state. */
 static AutoPrewarmSharedState *apw_state = NULL;
@@ -140,26 +138,11 @@ _PG_init(void)
 
 	MarkGUCPrefixReserved("pg_prewarm");
 
-	prev_shmem_request_hook = shmem_request_hook;
-	shmem_request_hook = autoprewarm_shmem_request;
-
 	/* Register autoprewarm worker, if enabled. */
 	if (autoprewarm)
 		apw_start_leader_worker();
 }
 
-/*
- * Requests any additional shared memory required for autoprewarm.
- */
-static void
-autoprewarm_shmem_request(void)
-{
-	if (prev_shmem_request_hook)
-		prev_shmem_request_hook();
-
-	RequestAddinShmemSpace(MAXALIGN(sizeof(AutoPrewarmSharedState)));
-}
-
 /*
  * Main entry point for the leader autoprewarm process.  Per-database workers
  * have a separate entry point.
@@ -767,6 +750,16 @@ autoprewarm_dump_now(PG_FUNCTION_ARGS)
 	PG_RETURN_INT64((int64) num_blocks);
 }
 
+static void
+init_state(void *ptr)
+{
+	AutoPrewarmSharedState *state = (AutoPrewarmSharedState *) ptr;
+
+	LWLockInitialize(&state->lock, LWLockNewTrancheId());
+	state->bgworker_pid = InvalidPid;
+	state->pid_using_dumpfile = InvalidPid;
+}
+
 /*
  * Allocate and initialize autoprewarm related shared memory, if not already
  * done, and set up backend-local pointer to that state.  Returns true if an
@@ -777,19 +770,9 @@ apw_init_shmem(void)
 {
 	bool		found;
 
-	LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
-	apw_state = ShmemInitStruct("autoprewarm",
-								sizeof(AutoPrewarmSharedState),
-								&found);
-	if (!found)
-	{
-		/* First time through ... */
-		LWLockInitialize(&apw_state->lock, LWLockNewTrancheId());
-		apw_state->bgworker_pid = InvalidPid;
-		apw_state->pid_using_dumpfile = InvalidPid;
-	}
-	LWLockRelease(AddinShmemInitLock);
-
+	apw_state = dsm_registry_init_or_attach("autoprewarm",
+											sizeof(AutoPrewarmSharedState),
+											init_state, &found);
 	LWLockRegisterTranche(apw_state->lock.tranche, "autoprewarm");
 
 	return found;
-- 
2.25.1



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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-02 16:31         ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
  2024-01-02 22:49           ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
@ 2024-01-06 14:04             ` Bharath Rupireddy <[email protected]>
  2024-01-06 16:35               ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Bharath Rupireddy @ 2024-01-06 14:04 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers

On Wed, Jan 3, 2024 at 4:19 AM Nathan Bossart <[email protected]> wrote:
>
> Here's a new version of the patch set with Bharath's feedback addressed.

Thanks. The v4 patches look good to me except for a few minor
comments. I've marked it as RfC in CF.

1. Update all the copyright to the new year. A run of
src/tools/copyright.pl on the source tree will take care of it at some
point, but still it's good if we can update while we are here.
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+# Copyright (c) 2023, PostgreSQL Global Development Group
+ * Copyright (c) 2023, PostgreSQL Global Development Group

2. Typo: missing "an" before "already-attached".
+        /* Return address of already-attached DSM registry entry. */

3. Use NAMEDATALEN instead of 64?
+    char        key[64];

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-02 16:31         ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
  2024-01-02 22:49           ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-06 14:04             ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
@ 2024-01-06 16:35               ` Nathan Bossart <[email protected]>
  2024-01-08 05:23                 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Nathan Bossart @ 2024-01-06 16:35 UTC (permalink / raw)
  To: Bharath Rupireddy <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers

On Sat, Jan 06, 2024 at 07:34:15PM +0530, Bharath Rupireddy wrote:
> 1. Update all the copyright to the new year. A run of
> src/tools/copyright.pl on the source tree will take care of it at some
> point, but still it's good if we can update while we are here.

Done.

> 2. Typo: missing "an" before "already-attached".
> +        /* Return address of already-attached DSM registry entry. */

Done.

> 3. Use NAMEDATALEN instead of 64?
> +    char        key[64];

I kept this the same, as I didn't see any need to tie the key size to
NAMEDATALEN.

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


Attachments:

  [text/x-diff] v5-0001-add-dsm-registry.patch (19.2K, ../../20240106163516.GB2435448@nathanxps13/2-v5-0001-add-dsm-registry.patch)
  download | inline diff:
From c09b49c5a75846c537aaaf1e39843123ac5a6d91 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 11 Oct 2023 22:07:26 -0500
Subject: [PATCH v5 1/2] add dsm registry

---
 src/backend/storage/ipc/Makefile              |   1 +
 src/backend/storage/ipc/dsm_registry.c        | 176 ++++++++++++++++++
 src/backend/storage/ipc/ipci.c                |   3 +
 src/backend/storage/ipc/meson.build           |   1 +
 src/backend/storage/lmgr/lwlock.c             |   4 +
 src/backend/storage/lmgr/lwlocknames.txt      |   1 +
 .../utils/activity/wait_event_names.txt       |   3 +
 src/include/storage/dsm_registry.h            |  23 +++
 src/include/storage/lwlock.h                  |   2 +
 src/test/modules/Makefile                     |   1 +
 src/test/modules/meson.build                  |   1 +
 src/test/modules/test_dsm_registry/.gitignore |   4 +
 src/test/modules/test_dsm_registry/Makefile   |  23 +++
 .../expected/test_dsm_registry.out            |  14 ++
 .../modules/test_dsm_registry/meson.build     |  33 ++++
 .../sql/test_dsm_registry.sql                 |   4 +
 .../test_dsm_registry--1.0.sql                |  10 +
 .../test_dsm_registry/test_dsm_registry.c     |  75 ++++++++
 .../test_dsm_registry.control                 |   4 +
 src/tools/pgindent/typedefs.list              |   3 +
 20 files changed, 386 insertions(+)
 create mode 100644 src/backend/storage/ipc/dsm_registry.c
 create mode 100644 src/include/storage/dsm_registry.h
 create mode 100644 src/test/modules/test_dsm_registry/.gitignore
 create mode 100644 src/test/modules/test_dsm_registry/Makefile
 create mode 100644 src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
 create mode 100644 src/test/modules/test_dsm_registry/meson.build
 create mode 100644 src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
 create mode 100644 src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
 create mode 100644 src/test/modules/test_dsm_registry/test_dsm_registry.c
 create mode 100644 src/test/modules/test_dsm_registry/test_dsm_registry.control

diff --git a/src/backend/storage/ipc/Makefile b/src/backend/storage/ipc/Makefile
index 6d5b921038..d8a1653eb6 100644
--- a/src/backend/storage/ipc/Makefile
+++ b/src/backend/storage/ipc/Makefile
@@ -12,6 +12,7 @@ OBJS = \
 	barrier.o \
 	dsm.o \
 	dsm_impl.o \
+	dsm_registry.o \
 	ipc.o \
 	ipci.o \
 	latch.o \
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
new file mode 100644
index 0000000000..3a32dcfe40
--- /dev/null
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -0,0 +1,176 @@
+/*-------------------------------------------------------------------------
+ *
+ * dsm_registry.c
+ *
+ * Functions for interfacing with the dynamic shared memory registry.  This
+ * provides a way for libraries to use shared memory without needing to
+ * request it at startup time via a shmem_request_hook.
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ *	  src/backend/storage/ipc/dsm_registry.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "lib/dshash.h"
+#include "storage/dsm_registry.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
+#include "utils/memutils.h"
+
+typedef struct DSMRegistryCtxStruct
+{
+	dsa_handle	dsah;
+	dshash_table_handle dshh;
+} DSMRegistryCtxStruct;
+
+static DSMRegistryCtxStruct *DSMRegistryCtx;
+
+typedef struct DSMRegistryEntry
+{
+	char		key[64];
+	dsm_handle	handle;
+} DSMRegistryEntry;
+
+static const dshash_parameters dsh_params = {
+	offsetof(DSMRegistryEntry, handle),
+	sizeof(DSMRegistryEntry),
+	dshash_memcmp,
+	dshash_memhash,
+	LWTRANCHE_DSM_REGISTRY_HASH
+};
+
+static dsa_area *dsm_registry_dsa;
+static dshash_table *dsm_registry_table;
+
+static void init_dsm_registry(void);
+
+Size
+DSMRegistryShmemSize(void)
+{
+	return MAXALIGN(sizeof(DSMRegistryCtxStruct));
+}
+
+void
+DSMRegistryShmemInit(void)
+{
+	bool		found;
+
+	DSMRegistryCtx = (DSMRegistryCtxStruct *)
+		ShmemInitStruct("DSM Registry Data",
+						DSMRegistryShmemSize(),
+						&found);
+
+	if (!found)
+	{
+		DSMRegistryCtx->dsah = DSA_HANDLE_INVALID;
+		DSMRegistryCtx->dshh = DSHASH_HANDLE_INVALID;
+	}
+}
+
+/*
+ * Initialize or attach to the dynamic shared hash table that stores the DSM
+ * registry entries, if not already done.  This must be called before accessing
+ * the table.
+ */
+static void
+init_dsm_registry(void)
+{
+	/* Quick exit if we already did this. */
+	if (dsm_registry_table)
+		return;
+
+	/* Otherwise, use a lock to ensure only one process creates the table. */
+	LWLockAcquire(DSMRegistryLock, LW_EXCLUSIVE);
+
+	if (DSMRegistryCtx->dshh == DSHASH_HANDLE_INVALID)
+	{
+		/* Initialize dynamic shared hash table for registry. */
+		dsm_registry_dsa = dsa_create(LWTRANCHE_DSM_REGISTRY_DSA);
+		dsa_pin(dsm_registry_dsa);
+		dsa_pin_mapping(dsm_registry_dsa);
+		dsm_registry_table = dshash_create(dsm_registry_dsa, &dsh_params, 0);
+
+		/* Store handles in shared memory for other backends to use. */
+		DSMRegistryCtx->dsah = dsa_get_handle(dsm_registry_dsa);
+		DSMRegistryCtx->dshh = dshash_get_hash_table_handle(dsm_registry_table);
+	}
+	else
+	{
+		/* Attach to existing dynamic shared hash table. */
+		dsm_registry_dsa = dsa_attach(DSMRegistryCtx->dsah);
+		dsa_pin_mapping(dsm_registry_dsa);
+		dsm_registry_table = dshash_attach(dsm_registry_dsa, &dsh_params,
+										   DSMRegistryCtx->dshh, 0);
+	}
+
+	LWLockRelease(DSMRegistryLock);
+}
+
+/*
+ * Initialize or attach a DSM entry.
+ *
+ * This routine returns the address of the segment.  init_callback is called to
+ * initialize the segment when it is first created.
+ */
+void *
+dsm_registry_init_or_attach(const char *key, size_t size,
+							void (*init_callback) (void *ptr), bool *found)
+{
+	DSMRegistryEntry *entry;
+	MemoryContext oldcontext;
+	char		key_padded[offsetof(DSMRegistryEntry, handle)] = {0};
+	void	   *ret;
+
+	Assert(key);
+	Assert(size);
+	Assert(found);
+
+	if (strlen(key) >= offsetof(DSMRegistryEntry, handle))
+		elog(ERROR, "DSM registry key too long");
+
+	/* Be sure any local memory allocated by DSM/DSA routines is persistent. */
+	oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+	/* Connect to the registry. */
+	init_dsm_registry();
+
+	strcpy(key_padded, key);
+	entry = dshash_find_or_insert(dsm_registry_table, key_padded, found);
+	if (!(*found))
+	{
+		/* Initialize DSM registry entry. */
+		dsm_segment *seg = dsm_create(size, 0);
+
+		dsm_pin_segment(seg);
+		dsm_pin_mapping(seg);
+		entry->handle = dsm_segment_handle(seg);
+		ret = dsm_segment_address(seg);
+
+		if (init_callback)
+			(*init_callback) (ret);
+	}
+	else if (!dsm_find_mapping(entry->handle))
+	{
+		/* Attach to existing DSM registry entry. */
+		dsm_segment *seg = dsm_attach(entry->handle);
+
+		dsm_pin_mapping(seg);
+		ret = dsm_segment_address(seg);
+	}
+	else
+	{
+		/* Return address of an already-attached DSM registry entry. */
+		ret = dsm_segment_address(dsm_find_mapping(entry->handle));
+	}
+
+	dshash_release_lock(dsm_registry_table, entry);
+	MemoryContextSwitchTo(oldcontext);
+
+	return ret;
+}
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index e5119ed55d..fbc62b1563 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -40,6 +40,7 @@
 #include "replication/walsender.h"
 #include "storage/bufmgr.h"
 #include "storage/dsm.h"
+#include "storage/dsm_registry.h"
 #include "storage/ipc.h"
 #include "storage/pg_shmem.h"
 #include "storage/pmsignal.h"
@@ -115,6 +116,7 @@ CalculateShmemSize(int *num_semaphores)
 	size = add_size(size, hash_estimate_size(SHMEM_INDEX_SIZE,
 											 sizeof(ShmemIndexEnt)));
 	size = add_size(size, dsm_estimate_size());
+	size = add_size(size, DSMRegistryShmemSize());
 	size = add_size(size, BufferShmemSize());
 	size = add_size(size, LockShmemSize());
 	size = add_size(size, PredicateLockShmemSize());
@@ -289,6 +291,7 @@ CreateOrAttachShmemStructs(void)
 	InitShmemIndex();
 
 	dsm_shmem_init();
+	DSMRegistryShmemInit();
 
 	/*
 	 * Set up xlog, clog, and buffers
diff --git a/src/backend/storage/ipc/meson.build b/src/backend/storage/ipc/meson.build
index 08bdc718b8..5a936171f7 100644
--- a/src/backend/storage/ipc/meson.build
+++ b/src/backend/storage/ipc/meson.build
@@ -4,6 +4,7 @@ backend_sources += files(
   'barrier.c',
   'dsm.c',
   'dsm_impl.c',
+  'dsm_registry.c',
   'ipc.c',
   'ipci.c',
   'latch.c',
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index b4b989ac56..2f2de5a562 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -190,6 +190,10 @@ static const char *const BuiltinTrancheNames[] = {
 	"LogicalRepLauncherDSA",
 	/* LWTRANCHE_LAUNCHER_HASH: */
 	"LogicalRepLauncherHash",
+	/* LWTRANCHE_DSM_REGISTRY_DSA: */
+	"DSMRegistryDSA",
+	/* LWTRANCHE_DSM_REGISTRY_HASH: */
+	"DSMRegistryHash",
 };
 
 StaticAssertDecl(lengthof(BuiltinTrancheNames) ==
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index d621f5507f..ef8542de46 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -55,3 +55,4 @@ WrapLimitsVacuumLock				46
 NotifyQueueTailLock					47
 WaitEventExtensionLock				48
 WALSummarizerLock					49
+DSMRegistryLock						50
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 088eb977d4..33c4c645b8 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -325,6 +325,7 @@ WrapLimitsVacuum	"Waiting to update limits on transaction id and multixact consu
 NotifyQueueTail	"Waiting to update limit on <command>NOTIFY</command> message storage."
 WaitEventExtension	"Waiting to read or update custom wait events information for extensions."
 WALSummarizer	"Waiting to read or update WAL summarization state."
+DSMRegistry	"Waiting to read or update the dynamic shared memory registry."
 
 XactBuffer	"Waiting for I/O on a transaction status SLRU buffer."
 CommitTsBuffer	"Waiting for I/O on a commit timestamp SLRU buffer."
@@ -355,6 +356,8 @@ PgStatsHash	"Waiting for stats shared memory hash table access."
 PgStatsData	"Waiting for shared memory stats data access."
 LogicalRepLauncherDSA	"Waiting to access logical replication launcher's dynamic shared memory allocator."
 LogicalRepLauncherHash	"Waiting to access logical replication launcher's shared hash table."
+DSMRegistryDSA	"Waiting to access dynamic shared memory registry's dynamic shared memory allocator."
+DSMRegistryHash	"Waiting to access dynamic shared memory registry's shared hash table."
 
 #
 # Wait Events - Lock
diff --git a/src/include/storage/dsm_registry.h b/src/include/storage/dsm_registry.h
new file mode 100644
index 0000000000..5d4cf9ca49
--- /dev/null
+++ b/src/include/storage/dsm_registry.h
@@ -0,0 +1,23 @@
+/*-------------------------------------------------------------------------
+ *
+ * dsm_registry.h
+ *
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/storage/dsm_registry.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef DSM_REGISTRY_H
+#define DSM_REGISTRY_H
+
+extern void *dsm_registry_init_or_attach(const char *key, size_t size,
+										 void (*init_callback) (void *ptr),
+										 bool *found);
+
+extern Size DSMRegistryShmemSize(void);
+extern void DSMRegistryShmemInit(void);
+
+#endif							/* DSM_REGISTRY_H */
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 167ae34208..50a65e046d 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -207,6 +207,8 @@ typedef enum BuiltinTrancheIds
 	LWTRANCHE_PGSTATS_DATA,
 	LWTRANCHE_LAUNCHER_DSA,
 	LWTRANCHE_LAUNCHER_HASH,
+	LWTRANCHE_DSM_REGISTRY_DSA,
+	LWTRANCHE_DSM_REGISTRY_HASH,
 	LWTRANCHE_FIRST_USER_DEFINED,
 }			BuiltinTrancheIds;
 
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 5d33fa6a9a..f656032589 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -18,6 +18,7 @@ SUBDIRS = \
 		  test_custom_rmgrs \
 		  test_ddl_deparse \
 		  test_dsa \
+		  test_dsm_registry \
 		  test_extensions \
 		  test_ginpostinglist \
 		  test_integerset \
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 00ff1d77d1..2c3b8d73bc 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -15,6 +15,7 @@ subdir('test_copy_callbacks')
 subdir('test_custom_rmgrs')
 subdir('test_ddl_deparse')
 subdir('test_dsa')
+subdir('test_dsm_registry')
 subdir('test_extensions')
 subdir('test_ginpostinglist')
 subdir('test_integerset')
diff --git a/src/test/modules/test_dsm_registry/.gitignore b/src/test/modules/test_dsm_registry/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_dsm_registry/Makefile b/src/test/modules/test_dsm_registry/Makefile
new file mode 100644
index 0000000000..b13e99a354
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/Makefile
@@ -0,0 +1,23 @@
+# src/test/modules/test_dsm_registry/Makefile
+
+MODULE_big = test_dsm_registry
+OBJS = \
+	$(WIN32RES) \
+	test_dsm_registry.o
+PGFILEDESC = "test_dsm_registry - test code for the DSM registry"
+
+EXTENSION = test_dsm_registry
+DATA = test_dsm_registry--1.0.sql
+
+REGRESS = test_dsm_registry
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_dsm_registry
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
new file mode 100644
index 0000000000..8ffbd343a0
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
@@ -0,0 +1,14 @@
+CREATE EXTENSION test_dsm_registry;
+SELECT set_val_in_shmem(1236);
+ set_val_in_shmem 
+------------------
+ 
+(1 row)
+
+\c
+SELECT get_val_in_shmem();
+ get_val_in_shmem 
+------------------
+             1236
+(1 row)
+
diff --git a/src/test/modules/test_dsm_registry/meson.build b/src/test/modules/test_dsm_registry/meson.build
new file mode 100644
index 0000000000..a4045fea37
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/meson.build
@@ -0,0 +1,33 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+test_dsm_registry_sources = files(
+  'test_dsm_registry.c',
+)
+
+if host_system == 'windows'
+  test_dsm_registry_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'test_dsm_registry',
+    '--FILEDESC', 'test_dsm_registry - test code for the DSM registry',])
+endif
+
+test_dsm_registry = shared_module('test_dsm_registry',
+  test_dsm_registry_sources,
+  kwargs: pg_test_mod_args,
+)
+test_install_libs += test_dsm_registry
+
+test_install_data += files(
+  'test_dsm_registry.control',
+  'test_dsm_registry--1.0.sql',
+)
+
+tests += {
+  'name': 'test_dsm_registry',
+  'sd': meson.current_source_dir(),
+  'bd': meson.current_build_dir(),
+  'regress': {
+    'sql': [
+      'test_dsm_registry',
+    ],
+  },
+}
diff --git a/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
new file mode 100644
index 0000000000..b3351be0a1
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
@@ -0,0 +1,4 @@
+CREATE EXTENSION test_dsm_registry;
+SELECT set_val_in_shmem(1236);
+\c
+SELECT get_val_in_shmem();
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
new file mode 100644
index 0000000000..8c55b0919b
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
@@ -0,0 +1,10 @@
+/* src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_dsm_registry" to load this file. \quit
+
+CREATE FUNCTION set_val_in_shmem(val INT) RETURNS VOID
+	AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION get_val_in_shmem() RETURNS INT
+	AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry.c b/src/test/modules/test_dsm_registry/test_dsm_registry.c
new file mode 100644
index 0000000000..b57f13cebb
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry.c
@@ -0,0 +1,75 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_dsm_registry.c
+ *		Test the DSM registry
+ *
+ * Copyright (c) 2024, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		src/test/modules/test_dsm_registry/test_dsm_registry.c
+ *
+ * -------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "storage/dsm_registry.h"
+#include "storage/lwlock.h"
+
+PG_MODULE_MAGIC;
+
+typedef struct TestDSMRegistryStruct
+{
+	int			val;
+	LWLock		lck;
+} TestDSMRegistryStruct;
+
+static TestDSMRegistryStruct *tdr_state;
+
+static void
+init_state(void *ptr)
+{
+	TestDSMRegistryStruct *state = (TestDSMRegistryStruct *) ptr;
+
+	LWLockInitialize(&state->lck, LWLockNewTrancheId());
+	state->val = 0;
+}
+
+static void
+dsm_registry_attach(void)
+{
+	bool		found;
+
+	tdr_state = dsm_registry_init_or_attach("test_dsm_registry",
+											sizeof(TestDSMRegistryStruct),
+											init_state, &found);
+	LWLockRegisterTranche(tdr_state->lck.tranche, "test_dsm_registry");
+}
+
+PG_FUNCTION_INFO_V1(set_val_in_shmem);
+Datum
+set_val_in_shmem(PG_FUNCTION_ARGS)
+{
+	dsm_registry_attach();
+
+	LWLockAcquire(&tdr_state->lck, LW_EXCLUSIVE);
+	tdr_state->val = PG_GETARG_UINT32(0);
+	LWLockRelease(&tdr_state->lck);
+
+	PG_RETURN_VOID();
+}
+
+PG_FUNCTION_INFO_V1(get_val_in_shmem);
+Datum
+get_val_in_shmem(PG_FUNCTION_ARGS)
+{
+	int			ret;
+
+	dsm_registry_attach();
+
+	LWLockAcquire(&tdr_state->lck, LW_SHARED);
+	ret = tdr_state->val;
+	LWLockRelease(&tdr_state->lck);
+
+	PG_RETURN_UINT32(ret);
+}
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry.control b/src/test/modules/test_dsm_registry/test_dsm_registry.control
new file mode 100644
index 0000000000..813f099889
--- /dev/null
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry.control
@@ -0,0 +1,4 @@
+comment = 'Test code for the DSM registry'
+default_version = '1.0'
+module_pathname = '$libdir/test_dsm_registry'
+relocatable = true
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 5fd46b7bd1..469f7570f5 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -610,6 +610,8 @@ DropSubscriptionStmt
 DropTableSpaceStmt
 DropUserMappingStmt
 DropdbStmt
+DSMRegistryCtxStruct
+DSMRegistryEntry
 DumpComponents
 DumpId
 DumpOptions
@@ -2799,6 +2801,7 @@ Tcl_NotifierProcs
 Tcl_Obj
 Tcl_Time
 TempNamespaceStatus
+TestDSMRegistryStruct
 TestDecodingData
 TestDecodingTxnData
 TestSpec
-- 
2.25.1



  [text/x-diff] v5-0002-use-dsm-registry-for-pg_prewarm.patch (3.1K, ../../20240106163516.GB2435448@nathanxps13/3-v5-0002-use-dsm-registry-for-pg_prewarm.patch)
  download | inline diff:
From b690a9be35085341d53cd3a937193b2f49abb158 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 26 Dec 2023 22:25:45 -0600
Subject: [PATCH v5 2/2] use dsm registry for pg_prewarm

---
 contrib/pg_prewarm/autoprewarm.c | 45 ++++++++++----------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 9085a409db..b9fd724788 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -32,12 +32,12 @@
 #include "access/xact.h"
 #include "catalog/pg_class.h"
 #include "catalog/pg_type.h"
-#include "miscadmin.h"
 #include "pgstat.h"
 #include "postmaster/bgworker.h"
 #include "postmaster/interrupt.h"
 #include "storage/buf_internals.h"
 #include "storage/dsm.h"
+#include "storage/dsm_registry.h"
 #include "storage/fd.h"
 #include "storage/ipc.h"
 #include "storage/latch.h"
@@ -95,8 +95,6 @@ static void apw_start_database_worker(void);
 static bool apw_init_shmem(void);
 static void apw_detach_shmem(int code, Datum arg);
 static int	apw_compare_blockinfo(const void *p, const void *q);
-static void autoprewarm_shmem_request(void);
-static shmem_request_hook_type prev_shmem_request_hook = NULL;
 
 /* Pointer to shared-memory state. */
 static AutoPrewarmSharedState *apw_state = NULL;
@@ -140,26 +138,11 @@ _PG_init(void)
 
 	MarkGUCPrefixReserved("pg_prewarm");
 
-	prev_shmem_request_hook = shmem_request_hook;
-	shmem_request_hook = autoprewarm_shmem_request;
-
 	/* Register autoprewarm worker, if enabled. */
 	if (autoprewarm)
 		apw_start_leader_worker();
 }
 
-/*
- * Requests any additional shared memory required for autoprewarm.
- */
-static void
-autoprewarm_shmem_request(void)
-{
-	if (prev_shmem_request_hook)
-		prev_shmem_request_hook();
-
-	RequestAddinShmemSpace(MAXALIGN(sizeof(AutoPrewarmSharedState)));
-}
-
 /*
  * Main entry point for the leader autoprewarm process.  Per-database workers
  * have a separate entry point.
@@ -767,6 +750,16 @@ autoprewarm_dump_now(PG_FUNCTION_ARGS)
 	PG_RETURN_INT64((int64) num_blocks);
 }
 
+static void
+init_state(void *ptr)
+{
+	AutoPrewarmSharedState *state = (AutoPrewarmSharedState *) ptr;
+
+	LWLockInitialize(&state->lock, LWLockNewTrancheId());
+	state->bgworker_pid = InvalidPid;
+	state->pid_using_dumpfile = InvalidPid;
+}
+
 /*
  * Allocate and initialize autoprewarm related shared memory, if not already
  * done, and set up backend-local pointer to that state.  Returns true if an
@@ -777,19 +770,9 @@ apw_init_shmem(void)
 {
 	bool		found;
 
-	LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
-	apw_state = ShmemInitStruct("autoprewarm",
-								sizeof(AutoPrewarmSharedState),
-								&found);
-	if (!found)
-	{
-		/* First time through ... */
-		LWLockInitialize(&apw_state->lock, LWLockNewTrancheId());
-		apw_state->bgworker_pid = InvalidPid;
-		apw_state->pid_using_dumpfile = InvalidPid;
-	}
-	LWLockRelease(AddinShmemInitLock);
-
+	apw_state = dsm_registry_init_or_attach("autoprewarm",
+											sizeof(AutoPrewarmSharedState),
+											init_state, &found);
 	LWLockRegisterTranche(apw_state->lock.tranche, "autoprewarm");
 
 	return found;
-- 
2.25.1



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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-02 16:31         ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
  2024-01-02 22:49           ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-06 14:04             ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-06 16:35               ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
@ 2024-01-08 05:23                 ` Bharath Rupireddy <[email protected]>
  2024-01-08 05:43                   ` Re: introduce dynamic shared memory registry Amul Sul <[email protected]>
  2024-01-08 17:16                   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  0 siblings, 2 replies; 26+ messages in thread

From: Bharath Rupireddy @ 2024-01-08 05:23 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers

On Sat, Jan 6, 2024 at 10:05 PM Nathan Bossart <[email protected]> wrote:
>
> I kept this the same, as I didn't see any need to tie the key size to
> NAMEDATALEN.

Thanks. A fresh look at the v5 patches left me with the following thoughts:

1. I think we need to add some notes about this new way of getting
shared memory for external modules in the <title>Shared Memory and
LWLocks</title> section in xfunc.sgml? This will at least tell there's
another way for external modules to get shared memory, not just with
the shmem_request_hook and shmem_startup_hook. What do you think?

2. FWIW, I'd like to call this whole feature "Support for named DSM
segments in Postgres". Do you see anything wrong with this?

3. IIUC, this feature eventually makes both shmem_request_hook and
shmem_startup_hook pointless, no? Or put another way, what's the
significance of shmem request and startup hooks in lieu of this new
feature? I think it's quite possible to get rid of the shmem request
and startup hooks (of course, not now but at some point in future to
not break the external modules), because all the external modules can
allocate and initialize the same shared memory via
dsm_registry_init_or_attach and its init_callback. All the external
modules will then need to call dsm_registry_init_or_attach in their
_PG_init callbacks and/or in their bg worker's main functions in case
the modules intend to start up bg workers. Am I right?

4. With the understanding in comment #3, can pg_stat_statements and
test_slru.c get rid of its shmem_request_hook and shmem_startup_hook
and use dsm_registry_init_or_attach? It's not that this patch need to
remove them now, but just asking if there's something in there that
makes this new feature unusable.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-02 16:31         ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
  2024-01-02 22:49           ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-06 14:04             ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-06 16:35               ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-08 05:23                 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
@ 2024-01-08 05:43                   ` Amul Sul <[email protected]>
  2024-01-08 17:18                     ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  1 sibling, 1 reply; 26+ messages in thread

From: Amul Sul @ 2024-01-08 05:43 UTC (permalink / raw)
  To: Bharath Rupireddy <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

On Mon, Jan 8, 2024 at 10:53 AM Bharath Rupireddy <
[email protected]> wrote:

> On Sat, Jan 6, 2024 at 10:05 PM Nathan Bossart <[email protected]>
> wrote:
> >
> > I kept this the same, as I didn't see any need to tie the key size to
> > NAMEDATALEN.
>
> Thanks. A fresh look at the v5 patches left me with the following thoughts:
>
> 1. I think we need to add some notes about this new way of getting
> shared memory for external modules in the <title>Shared Memory and
> LWLocks</title> section in xfunc.sgml? This will at least tell there's
> another way for external modules to get shared memory, not just with
> the shmem_request_hook and shmem_startup_hook. What do you think?
>
> 2. FWIW, I'd like to call this whole feature "Support for named DSM
> segments in Postgres". Do you see anything wrong with this?
>
> 3. IIUC, this feature eventually makes both shmem_request_hook and
> shmem_startup_hook pointless, no? Or put another way, what's the
> significance of shmem request and startup hooks in lieu of this new
> feature? I think it's quite possible to get rid of the shmem request
> and startup hooks (of course, not now but at some point in future to
> not break the external modules), because all the external modules can
> allocate and initialize the same shared memory via
> dsm_registry_init_or_attach and its init_callback. All the external
> modules will then need to call dsm_registry_init_or_attach in their
> _PG_init callbacks and/or in their bg worker's main functions in case
> the modules intend to start up bg workers. Am I right?
>
> 4. With the understanding in comment #3, can pg_stat_statements and
> test_slru.c get rid of its shmem_request_hook and shmem_startup_hook
> and use dsm_registry_init_or_attach? It's not that this patch need to
> remove them now, but just asking if there's something in there that
> makes this new feature unusable.
>

+1, since doing for pg_prewarm, better to do for these modules as well.

A minor comment for v5:

+void *
+dsm_registry_init_or_attach(const char *key, size_t size,

I think the name could be simple as dsm_registry_init() like we use
elsewhere e.g. ShmemInitHash() which doesn't say attach explicitly.

Similarly, I think dshash_find_or_insert() can be as simple as
dshash_search() and
accept HASHACTION like hash_search().

Regards,
Amul


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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-02 16:31         ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
  2024-01-02 22:49           ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-06 14:04             ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-06 16:35               ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-08 05:23                 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-08 05:43                   ` Re: introduce dynamic shared memory registry Amul Sul <[email protected]>
@ 2024-01-08 17:18                     ` Nathan Bossart <[email protected]>
  2024-01-09 03:59                       ` Re: introduce dynamic shared memory registry Amul Sul <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Nathan Bossart @ 2024-01-08 17:18 UTC (permalink / raw)
  To: Amul Sul <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

On Mon, Jan 08, 2024 at 11:13:42AM +0530, Amul Sul wrote:
> +void *
> +dsm_registry_init_or_attach(const char *key, size_t size,
> 
> I think the name could be simple as dsm_registry_init() like we use
> elsewhere e.g. ShmemInitHash() which doesn't say attach explicitly.

That seems reasonable to me.

> Similarly, I think dshash_find_or_insert() can be as simple as
> dshash_search() and
> accept HASHACTION like hash_search().

I'm not totally sure what you mean here.  If you mean changing the dshash
API, I'd argue that's a topic for another thread.

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






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-02 16:31         ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
  2024-01-02 22:49           ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-06 14:04             ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-06 16:35               ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-08 05:23                 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-08 05:43                   ` Re: introduce dynamic shared memory registry Amul Sul <[email protected]>
  2024-01-08 17:18                     ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
@ 2024-01-09 03:59                       ` Amul Sul <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Amul Sul @ 2024-01-09 03:59 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

On Mon, Jan 8, 2024 at 10:48 PM Nathan Bossart <[email protected]>
wrote:

> On Mon, Jan 08, 2024 at 11:13:42AM +0530, Amul Sul wrote:
> > +void *
> > +dsm_registry_init_or_attach(const char *key, size_t size,
> >
> > I think the name could be simple as dsm_registry_init() like we use
> > elsewhere e.g. ShmemInitHash() which doesn't say attach explicitly.
>
> That seems reasonable to me.
>
> > Similarly, I think dshash_find_or_insert() can be as simple as
> > dshash_search() and
> > accept HASHACTION like hash_search().
>
> I'm not totally sure what you mean here.  If you mean changing the dshash
> API, I'd argue that's a topic for another thread.
>

Yes, you are correct. I didn't realize that existing code -- now sure, why
wouldn't we implemented as the dynahash. Sorry for the noise.

Regards,
Amul


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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-02 16:31         ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
  2024-01-02 22:49           ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-06 14:04             ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-06 16:35               ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-08 05:23                 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
@ 2024-01-08 17:16                   ` Nathan Bossart <[email protected]>
  2024-01-11 05:12                     ` Re: introduce dynamic shared memory registry Michael Paquier <[email protected]>
  1 sibling, 1 reply; 26+ messages in thread

From: Nathan Bossart @ 2024-01-08 17:16 UTC (permalink / raw)
  To: Bharath Rupireddy <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers

On Mon, Jan 08, 2024 at 10:53:17AM +0530, Bharath Rupireddy wrote:
> 1. I think we need to add some notes about this new way of getting
> shared memory for external modules in the <title>Shared Memory and
> LWLocks</title> section in xfunc.sgml? This will at least tell there's
> another way for external modules to get shared memory, not just with
> the shmem_request_hook and shmem_startup_hook. What do you think?

Good call.  I definitely think this stuff ought to be documented.  After a
quick read, I also wonder if it'd be worth spending some time refining that
section.

> 2. FWIW, I'd like to call this whole feature "Support for named DSM
> segments in Postgres". Do you see anything wrong with this?

Why do you feel it should be renamed?  I don't see anything wrong with it,
but I also don't see any particular advantage with that name compared to
"dynamic shared memory registry."

> 3. IIUC, this feature eventually makes both shmem_request_hook and
> shmem_startup_hook pointless, no? Or put another way, what's the
> significance of shmem request and startup hooks in lieu of this new
> feature? I think it's quite possible to get rid of the shmem request
> and startup hooks (of course, not now but at some point in future to
> not break the external modules), because all the external modules can
> allocate and initialize the same shared memory via
> dsm_registry_init_or_attach and its init_callback. All the external
> modules will then need to call dsm_registry_init_or_attach in their
> _PG_init callbacks and/or in their bg worker's main functions in case
> the modules intend to start up bg workers. Am I right?

Well, modules might need to do a number of other things (e.g., adding
hooks) that can presently only be done when preloaded, in which case I
doubt there's much benefit from switching to the DSM registry.  I don't
really intend for it to replace the existing request/startup hooks, but
you're probably right that most, if not all, could use the registry
instead.  IMHO this is well beyond the scope of this thread, though.

> 4. With the understanding in comment #3, can pg_stat_statements and
> test_slru.c get rid of its shmem_request_hook and shmem_startup_hook
> and use dsm_registry_init_or_attach? It's not that this patch need to
> remove them now, but just asking if there's something in there that
> makes this new feature unusable.

It might be possible, but IIUC you'd still need a way to know whether the
library was preloaded, i.e., all the other necessary hooks were in place.
It's convenient to just be able to check whether the shared memory was set
up for this purpose.

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






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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-02 16:31         ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
  2024-01-02 22:49           ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-06 14:04             ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-06 16:35               ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
  2024-01-08 05:23                 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
  2024-01-08 17:16                   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
@ 2024-01-11 05:12                     ` Michael Paquier <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Michael Paquier @ 2024-01-11 05:12 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

On Mon, Jan 08, 2024 at 11:16:27AM -0600, Nathan Bossart wrote:
> On Mon, Jan 08, 2024 at 10:53:17AM +0530, Bharath Rupireddy wrote:
>> 1. I think we need to add some notes about this new way of getting
>> shared memory for external modules in the <title>Shared Memory and
>> LWLocks</title> section in xfunc.sgml? This will at least tell there's
>> another way for external modules to get shared memory, not just with
>> the shmem_request_hook and shmem_startup_hook. What do you think?
> 
> Good call.  I definitely think this stuff ought to be documented.  After a
> quick read, I also wonder if it'd be worth spending some time refining that
> section.

+1.  It would be a second thing to point at autoprewarm.c in the docs
as an extra pointer that can be fed to users reading the docs.

>> 3. IIUC, this feature eventually makes both shmem_request_hook and
>> shmem_startup_hook pointless, no? Or put another way, what's the
>> significance of shmem request and startup hooks in lieu of this new
>> feature? I think it's quite possible to get rid of the shmem request
>> and startup hooks (of course, not now but at some point in future to
>> not break the external modules), because all the external modules can
>> allocate and initialize the same shared memory via
>> dsm_registry_init_or_attach and its init_callback. All the external
>> modules will then need to call dsm_registry_init_or_attach in their
>> _PG_init callbacks and/or in their bg worker's main functions in case
>> the modules intend to start up bg workers. Am I right?
> 
> Well, modules might need to do a number of other things (e.g., adding
> hooks) that can presently only be done when preloaded, in which case I
> doubt there's much benefit from switching to the DSM registry.  I don't
> really intend for it to replace the existing request/startup hooks, but
> you're probably right that most, if not all, could use the registry
> instead.  IMHO this is well beyond the scope of this thread, though.

Even if that's not in the scope of this thread, just removing these
hooks would break a lot of out-of-core things, and they still have a
lot of value when extensions expect to always be loaded with shared.
They don't cost in maintenance at this stage.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

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

* Re: introduce dynamic shared memory registry
  2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
@ 2023-12-20 16:03 ` Zhang Mingli <[email protected]>
  4 siblings, 0 replies; 26+ messages in thread

From: Zhang Mingli @ 2023-12-20 16:03 UTC (permalink / raw)
  To: pgsql-hackers; Nathan Bossart <[email protected]>

Hi, all

I see most xxxShmemInit functions have the logic to handle IsUnderPostmaster env.
Do we need to consider it in DSMRegistryShmemInit() too? For example, add some assertions.
Others LGTM.


Zhang Mingli
www.hashdata.xyz
On Dec 5, 2023 at 11:47 +0800, Nathan Bossart <[email protected]>, wrote:
> Every once in a while, I find myself wanting to use shared memory in a
> loadable module without requiring it to be loaded at server start via
> shared_preload_libraries. The DSM API offers a nice way to create and
> manage dynamic shared memory segments, so creating a segment after server
> start is easy enough. However, AFAICT there's no easy way to teach other
> backends about the segment without storing the handles in shared memory,
> which puts us right back at square one.
>
> The attached 0001 introduces a "DSM registry" to solve this problem. The
> API provides an easy way to allocate/initialize a segment or to attach to
> an existing one. The registry itself is just a dshash table that stores
> the handles keyed by a module-specified string. 0002 adds a test for the
> registry that demonstrates basic usage.
>
> I don't presently have any concrete plans to use this for anything, but I
> thought it might be useful for extensions for caching, etc. and wanted to
> see whether there was any interest in the feature.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com


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


end of thread, other threads:[~2024-01-11 05:12 UTC | newest]

Thread overview: 26+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-04-02 06:02 [PATCH] Asymmetric partitionwise join. Andrey Lepikhov <[email protected]>
2023-12-05 03:46 introduce dynamic shared memory registry Nathan Bossart <[email protected]>
2023-12-05 15:34 ` Re: introduce dynamic shared memory registry Joe Conway <[email protected]>
2023-12-05 16:16   ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
2023-12-08 07:36 ` Re: introduce dynamic shared memory registry Michael Paquier <[email protected]>
2023-12-18 06:39 ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
2023-12-18 08:32   ` Re: introduce dynamic shared memory registry Andrei Lepikhov <[email protected]>
2023-12-18 09:05     ` Re: introduce dynamic shared memory registry Nikita Malakhov <[email protected]>
2023-12-19 16:09       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
2023-12-19 15:49     ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
2023-12-19 16:01     ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
2023-12-20 09:58 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
2023-12-20 16:03   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
2023-12-29 15:23     ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
2024-01-02 16:20       ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
2024-01-02 16:31         ` Re: introduce dynamic shared memory registry Robert Haas <[email protected]>
2024-01-02 22:49           ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
2024-01-06 14:04             ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
2024-01-06 16:35               ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
2024-01-08 05:23                 ` Re: introduce dynamic shared memory registry Bharath Rupireddy <[email protected]>
2024-01-08 05:43                   ` Re: introduce dynamic shared memory registry Amul Sul <[email protected]>
2024-01-08 17:18                     ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
2024-01-09 03:59                       ` Re: introduce dynamic shared memory registry Amul Sul <[email protected]>
2024-01-08 17:16                   ` Re: introduce dynamic shared memory registry Nathan Bossart <[email protected]>
2024-01-11 05:12                     ` Re: introduce dynamic shared memory registry Michael Paquier <[email protected]>
2023-12-20 16:03 ` Re: introduce dynamic shared memory registry Zhang Mingli <[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