Date: Fri, 20 Sep 2019 21:09:31 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Joe Nelson <joe@begriffs.com>
Cc: Thomas Munro <thomas.munro@gmail.com>,
	David Steele <david@pgmasters.net>,
	Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
	Nikita Glukhov <n.gluhov@postgrespro.ru>,
	PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: Optimization of some jsonb functions
Message-ID: <20190921000931.GA7648@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="J/dobhs11T7y2rNN"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190919034715.GA6743@alvherre.pgsql>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--J/dobhs11T7y2rNN
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

I pushed the first few parts.  The attached is a rebased copy of the
last remaining piece.  However, I didn't quite understand what this was
doing, so I refrained from pushing.  I think there are two patches here:
one that adapts the API of findJsonbValueFromContainer and
getIthJsonbValueFromContainer to take the output result pointer as an
argument, allowing to save palloc cycles just like the newly added
getKeyJsonValueFromContainer(); and the other changes JsonbDeepContains
so that it uses a new function (which is a function with a weird API
that would be extracted from findJsonbValueFromContainer).

Also, the current patch just passes NULL into the routines from
jsonpath_exec.c but I think it would be useful to pass pointers into
stack-allocated result structs instead, at least in getJsonPathVariable.

Since the majority of this patchset got pushed, I'll leave this for
Nikita to handle for the next commitfest if he wants to, and mark this
CF entry as committed.

Thanks!

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--J/dobhs11T7y2rNN
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="findJsonbElementInArray.patch"

diff --git a/src/backend/utils/adt/jsonb_op.c b/src/backend/utils/adt/jsonb_op.c
index a64206eeb1..82c4b0b2cb 100644
--- a/src/backend/utils/adt/jsonb_op.c
+++ b/src/backend/utils/adt/jsonb_op.c
@@ -24,6 +24,7 @@ jsonb_exists(PG_FUNCTION_ARGS)
 	Jsonb	   *jb = PG_GETARG_JSONB_P(0);
 	text	   *key = PG_GETARG_TEXT_PP(1);
 	JsonbValue	kval;
+	JsonbValue	vval;
 	JsonbValue *v = NULL;
 
 	/*
@@ -38,7 +39,7 @@ jsonb_exists(PG_FUNCTION_ARGS)
 
 	v = findJsonbValueFromContainer(&jb->root,
 									JB_FOBJECT | JB_FARRAY,
-									&kval);
+									&kval, &vval);
 
 	PG_RETURN_BOOL(v != NULL);
 }
@@ -59,6 +60,7 @@ jsonb_exists_any(PG_FUNCTION_ARGS)
 	for (i = 0; i < elem_count; i++)
 	{
 		JsonbValue	strVal;
+		JsonbValue	valVal;
 
 		if (key_nulls[i])
 			continue;
@@ -69,7 +71,7 @@ jsonb_exists_any(PG_FUNCTION_ARGS)
 
 		if (findJsonbValueFromContainer(&jb->root,
 										JB_FOBJECT | JB_FARRAY,
-										&strVal) != NULL)
+										&strVal, &valVal) != NULL)
 			PG_RETURN_BOOL(true);
 	}
 
@@ -92,6 +94,7 @@ jsonb_exists_all(PG_FUNCTION_ARGS)
 	for (i = 0; i < elem_count; i++)
 	{
 		JsonbValue	strVal;
+		JsonbValue	valVal;
 
 		if (key_nulls[i])
 			continue;
@@ -102,7 +105,7 @@ jsonb_exists_all(PG_FUNCTION_ARGS)
 
 		if (findJsonbValueFromContainer(&jb->root,
 										JB_FOBJECT | JB_FARRAY,
-										&strVal) == NULL)
+										&strVal, &valVal) == NULL)
 			PG_RETURN_BOOL(false);
 	}
 
diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index 7e0d9de7f0..7ec33cebf2 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -33,6 +33,8 @@
 #define JSONB_MAX_ELEMS (Min(MaxAllocSize / sizeof(JsonbValue), JB_CMASK))
 #define JSONB_MAX_PAIRS (Min(MaxAllocSize / sizeof(JsonbPair), JB_CMASK))
 
+static JsonbValue *findJsonbElementInArray(JsonbContainer *container,
+										   JsonbValue *elem, JsonbValue *res);
 static void fillJsonbValue(JsonbContainer *container, int index,
 						   char *base_addr, uint32 offset,
 						   JsonbValue *result);
@@ -327,38 +329,19 @@ compareJsonbContainers(JsonbContainer *a, JsonbContainer *b)
  */
 JsonbValue *
 findJsonbValueFromContainer(JsonbContainer *container, uint32 flags,
-							JsonbValue *key)
+							JsonbValue *key, JsonbValue *res)
 {
-	JEntry	   *children = container->children;
 	int			count = JsonContainerSize(container);
 
 	Assert((flags & ~(JB_FARRAY | JB_FOBJECT)) == 0);
 
-	/* Quick out without a palloc cycle if object/array is empty */
+	/* Quick out if object/array is empty */
 	if (count <= 0)
 		return NULL;
 
 	if ((flags & JB_FARRAY) && JsonContainerIsArray(container))
 	{
-		JsonbValue *result = palloc(sizeof(JsonbValue));
-		char	   *base_addr = (char *) (children + count);
-		uint32		offset = 0;
-		int			i;
-
-		for (i = 0; i < count; i++)
-		{
-			fillJsonbValue(container, i, base_addr, offset, result);
-
-			if (key->type == result->type)
-			{
-				if (equalsJsonbScalarValue(key, result))
-					return result;
-			}
-
-			JBE_ADVANCE_OFFSET(offset, children[i]);
-		}
-
-		pfree(result);
+		return findJsonbElementInArray(container, key, res);
 	}
 	else if ((flags & JB_FOBJECT) && JsonContainerIsObject(container))
 	{
@@ -366,13 +349,48 @@ findJsonbValueFromContainer(JsonbContainer *container, uint32 flags,
 		Assert(key->type == jbvString);
 
 		return getKeyJsonValueFromContainer(container, key->val.string.val,
-											key->val.string.len, NULL);
+											key->val.string.len, res);
 	}
 
 	/* Not found */
 	return NULL;
 }
 
+/*
+ * Subroutine for findJsonbValueFromContainer
+ *
+ * Find scalar element in Jsonb array and return it.
+ */
+static JsonbValue *
+findJsonbElementInArray(JsonbContainer *container, JsonbValue *elem,
+						JsonbValue *res)
+{
+	JsonbValue *result;
+	JEntry	   *children = container->children;
+	int			count = JsonContainerSize(container);
+	char	   *baseAddr = (char *) (children + count);
+	uint32		offset = 0;
+	int			i;
+
+	result = res ? res : palloc(sizeof(*result));
+
+	for (i = 0; i < count; i++)
+	{
+		fillJsonbValue(container, i, baseAddr, offset, result);
+
+		if (elem->type == result->type &&
+			equalsJsonbScalarValue(elem, result))
+			return result;
+
+		JBE_ADVANCE_OFFSET(offset, children[i]);
+	}
+
+	if (!res)
+		pfree(result);
+
+	return NULL;
+}
+
 /*
  * Find value by key in Jsonb object and fetch it into 'res', which is also
  * returned.
@@ -450,9 +468,9 @@ getKeyJsonValueFromContainer(JsonbContainer *container,
  * Returns palloc()'d copy of the value, or NULL if it does not exist.
  */
 JsonbValue *
-getIthJsonbValueFromContainer(JsonbContainer *container, uint32 i)
+getIthJsonbValueFromContainer(JsonbContainer *container, uint32 i,
+							  JsonbValue *result)
 {
-	JsonbValue *result;
 	char	   *base_addr;
 	uint32		nelements;
 
@@ -465,7 +483,8 @@ getIthJsonbValueFromContainer(JsonbContainer *container, uint32 i)
 	if (i >= nelements)
 		return NULL;
 
-	result = palloc(sizeof(JsonbValue));
+	if (!result)
+		result = palloc(sizeof(JsonbValue));
 
 	fillJsonbValue(container, i, base_addr,
 				   getJsonbOffset(container, i),
@@ -1163,9 +1182,10 @@ JsonbDeepContains(JsonbIterator **val, JsonbIterator **mContained)
 
 			if (IsAJsonbScalar(&vcontained))
 			{
-				if (!findJsonbValueFromContainer((*val)->container,
-												 JB_FARRAY,
-												 &vcontained))
+				JsonbValue	elemBuf;
+
+				if (!findJsonbElementInArray((*val)->container, &vcontained,
+											 &elemBuf))
 					return false;
 			}
 			else
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 3553a304b8..f75d496102 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -787,6 +787,7 @@ jsonb_array_element(PG_FUNCTION_ARGS)
 	Jsonb	   *jb = PG_GETARG_JSONB_P(0);
 	int			element = PG_GETARG_INT32(1);
 	JsonbValue *v;
+	JsonbValue	vbuf;
 
 	if (!JB_ROOT_IS_ARRAY(jb))
 		PG_RETURN_NULL();
@@ -802,7 +803,7 @@ jsonb_array_element(PG_FUNCTION_ARGS)
 			element += nelements;
 	}
 
-	v = getIthJsonbValueFromContainer(&jb->root, element);
+	v = getIthJsonbValueFromContainer(&jb->root, element, &vbuf);
 	if (v != NULL)
 		PG_RETURN_JSONB_P(JsonbValueToJsonb(v));
 
@@ -830,6 +831,7 @@ jsonb_array_element_text(PG_FUNCTION_ARGS)
 	Jsonb	   *jb = PG_GETARG_JSONB_P(0);
 	int			element = PG_GETARG_INT32(1);
 	JsonbValue *v;
+	JsonbValue	vbuf;
 
 	if (!JB_ROOT_IS_ARRAY(jb))
 		PG_RETURN_NULL();
@@ -845,7 +847,7 @@ jsonb_array_element_text(PG_FUNCTION_ARGS)
 			element += nelements;
 	}
 
-	v = getIthJsonbValueFromContainer(&jb->root, element);
+	v = getIthJsonbValueFromContainer(&jb->root, element, &vbuf);
 
 	if (v != NULL && v->type != jbvNull)
 		PG_RETURN_TEXT_P(JsonbValueAsText(v));
@@ -1361,7 +1363,7 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text)
 		Assert(JB_ROOT_IS_ARRAY(jb) && JB_ROOT_IS_SCALAR(jb));
 		/* Extract the scalar value, if it is what we'll return */
 		if (npath <= 0)
-			jbvp = getIthJsonbValueFromContainer(container, 0);
+			jbvp = getIthJsonbValueFromContainer(container, 0, &jbvbuf);
 	}
 
 	/*
@@ -1430,7 +1432,7 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text)
 					index = nelements + lindex;
 			}
 
-			jbvp = getIthJsonbValueFromContainer(container, index);
+			jbvp = getIthJsonbValueFromContainer(container, index, &jbvbuf);
 		}
 		else
 		{
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index 565b00c426..290b011ef6 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -583,7 +583,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
 				key.val.string.val = jspGetString(jsp, &key.val.string.len);
 
 				v = findJsonbValueFromContainer(jb->val.binary.data,
-												JB_FOBJECT, &key);
+												JB_FOBJECT, &key, NULL);
 
 				if (v != NULL)
 				{
@@ -715,7 +715,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
 						else
 						{
 							v = getIthJsonbValueFromContainer(jb->val.binary.data,
-															  (uint32) index);
+															  (uint32) index, NULL);
 
 							if (v == NULL)
 								continue;
@@ -1909,7 +1909,7 @@ getJsonPathVariable(JsonPathExecContext *cxt, JsonPathItem *variable,
 	tmp.val.string.val = varName;
 	tmp.val.string.len = varNameLength;
 
-	v = findJsonbValueFromContainer(&vars->root, JB_FOBJECT, &tmp);
+	v = findJsonbValueFromContainer(&vars->root, JB_FOBJECT, &tmp, NULL);
 
 	if (v)
 	{
diff --git a/src/include/utils/jsonb.h b/src/include/utils/jsonb.h
index 35766e106a..2ee6d3f6b4 100644
--- a/src/include/utils/jsonb.h
+++ b/src/include/utils/jsonb.h
@@ -363,12 +363,13 @@ extern uint32 getJsonbLength(const JsonbContainer *jc, int index);
 extern int	compareJsonbContainers(JsonbContainer *a, JsonbContainer *b);
 extern JsonbValue *findJsonbValueFromContainer(JsonbContainer *sheader,
 											   uint32 flags,
-											   JsonbValue *key);
+											   JsonbValue *key,
+											   JsonbValue *res);
 extern JsonbValue *getKeyJsonValueFromContainer(JsonbContainer *container,
 												const char *keyVal, int keyLen,
 												JsonbValue *res);
 extern JsonbValue *getIthJsonbValueFromContainer(JsonbContainer *sheader,
-												 uint32 i);
+												 uint32 i, JsonbValue *res);
 extern JsonbValue *pushJsonbValue(JsonbParseState **pstate,
 								  JsonbIteratorToken seq, JsonbValue *jbval);
 extern JsonbIterator *JsonbIteratorInit(JsonbContainer *container);

--J/dobhs11T7y2rNN--