public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v49 1/3] Subscripting for jsonb
8+ messages / 3 participants
[nested] [flat]
* [PATCH v49 1/3] Subscripting for jsonb
@ 2020-12-18 16:19 Dmitrii Dolgov <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Dmitrii Dolgov @ 2020-12-18 16:19 UTC (permalink / raw)
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule, Dian M Fay
---
doc/src/sgml/json.sgml | 51 ++++
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 ++++-
src/backend/utils/adt/jsonbsubs.c | 413 ++++++++++++++++++++++++++++
src/backend/utils/adt/jsonfuncs.c | 180 ++++++------
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +++++++++++++++++-
src/test/regress/sql/jsonb.sql | 84 +++++-
10 files changed, 985 insertions(+), 105 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..3ace5e444b 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,57 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
</para>
</sect2>
+ <sect2 id="jsonb-subscripting">
+ <title><type>jsonb</type> Subscripting</title>
+ <para>
+ The <type>jsonb</type> data type supports array-style subscripting expressions
+ to extract and modify elements. Nested values can be indicated by chaining
+ subscripting expressions, following the same rules as the <literal>path</literal>
+ argument in the <literal>jsonb_set</literal> function. If a <type>jsonb</type>
+ value is an array, numeric subscripts start at zero, and negative integers count
+ backwards from the last element of the array. Slice expressions are not supported.
+ The result of a subscripting expression is always of the jsonb data type.
+ </para>
+
+ <para>
+ An example of subscripting syntax:
+<programlisting>
+
+-- Extract object value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested object value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract array element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update object value by key. Note the quotes around '1': the assigned
+-- value must be of the jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Filter records using a WHERE clause with subscripting. Since the result of
+-- subscripting is jsonb, the value we compare it against must also be jsonb.
+-- The double quotes make "value" also a valid jsonb string.
+SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
+</programlisting>
+
+ <type>jsonb</type> assignment via subscripting handles a few edge cases
+ differently from <literal>jsonb_set</literal>. When a source <type>jsonb</type>
+ is <literal>NULL</literal>, assignment via subscripting will proceed as if
+ it was an empty JSON object:
+
+<programlisting>
+-- Where jsonb_field was NULL, it is now {"a": 1}
+UPDATE table_name SET jsonb_field['a'] = '1';
+
+-- Where jsonb_field was NULL, it is now [1]
+UPDATE table_name SET jsonb_field[0] = '1';
+</programlisting>
+
+ </para>
+ </sect2>
+
<sect2>
<title>Transforms</title>
diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index 82732146d3..279ff15ade 100644
--- a/src/backend/utils/adt/Makefile
+++ b/src/backend/utils/adt/Makefile
@@ -50,6 +50,7 @@ OBJS = \
jsonb_op.o \
jsonb_util.o \
jsonfuncs.o \
+ jsonbsubs.o \
jsonpath.o \
jsonpath_exec.o \
jsonpath_gram.o \
diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index 4eeffa1424..41a1c1f9bb 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -68,18 +68,29 @@ static JsonbValue *pushJsonbValueScalar(JsonbParseState **pstate,
JsonbIteratorToken seq,
JsonbValue *scalarVal);
+JsonbValue *
+JsonbToJsonbValue(Jsonb *jsonb)
+{
+ JsonbValue *val = (JsonbValue *) palloc(sizeof(JsonbValue));
+
+ val->type = jbvBinary;
+ val->val.binary.data = &jsonb->root;
+ val->val.binary.len = VARSIZE(jsonb) - VARHDRSZ;
+
+ return val;
+}
+
/*
* Turn an in-memory JsonbValue into a Jsonb for on-disk storage.
*
- * There isn't a JsonbToJsonbValue(), because generally we find it more
- * convenient to directly iterate through the Jsonb representation and only
- * really convert nested scalar values. JsonbIteratorNext() does this, so that
- * clients of the iteration code don't have to directly deal with the binary
- * representation (JsonbDeepContains() is a notable exception, although all
- * exceptions are internal to this module). In general, functions that accept
- * a JsonbValue argument are concerned with the manipulation of scalar values,
- * or simple containers of scalar values, where it would be inconvenient to
- * deal with a great amount of other state.
+ * Generally we find it more convenient to directly iterate through the Jsonb
+ * representation and only really convert nested scalar values.
+ * JsonbIteratorNext() does this, so that clients of the iteration code don't
+ * have to directly deal with the binary representation (JsonbDeepContains() is
+ * a notable exception, although all exceptions are internal to this module).
+ * In general, functions that accept a JsonbValue argument are concerned with
+ * the manipulation of scalar values, or simple containers of scalar values,
+ * where it would be inconvenient to deal with a great amount of other state.
*/
Jsonb *
JsonbValueToJsonb(JsonbValue *val)
@@ -563,6 +574,30 @@ pushJsonbValue(JsonbParseState **pstate, JsonbIteratorToken seq,
JsonbValue *res = NULL;
JsonbValue v;
JsonbIteratorToken tok;
+ int i;
+
+ if (jbval && (seq == WJB_ELEM || seq == WJB_VALUE) && jbval->type == jbvObject)
+ {
+ pushJsonbValue(pstate, WJB_BEGIN_OBJECT, NULL);
+ for (i = 0; i < jbval->val.object.nPairs; i++)
+ {
+ pushJsonbValue(pstate, WJB_KEY, &jbval->val.object.pairs[i].key);
+ pushJsonbValue(pstate, WJB_VALUE, &jbval->val.object.pairs[i].value);
+ }
+
+ return pushJsonbValue(pstate, WJB_END_OBJECT, NULL);
+ }
+
+ if (jbval && (seq == WJB_ELEM || seq == WJB_VALUE) && jbval->type == jbvArray)
+ {
+ pushJsonbValue(pstate, WJB_BEGIN_ARRAY, NULL);
+ for (i = 0; i < jbval->val.array.nElems; i++)
+ {
+ pushJsonbValue(pstate, WJB_ELEM, &jbval->val.array.elems[i]);
+ }
+
+ return pushJsonbValue(pstate, WJB_END_ARRAY, NULL);
+ }
if (!jbval || (seq != WJB_ELEM && seq != WJB_VALUE) ||
jbval->type != jbvBinary)
@@ -573,9 +608,30 @@ pushJsonbValue(JsonbParseState **pstate, JsonbIteratorToken seq,
/* unpack the binary and add each piece to the pstate */
it = JsonbIteratorInit(jbval->val.binary.data);
+
+ if ((jbval->val.binary.data->header & JB_FSCALAR) && *pstate)
+ {
+ tok = JsonbIteratorNext(&it, &v, true);
+ Assert(tok == WJB_BEGIN_ARRAY);
+ Assert(v.type == jbvArray && v.val.array.rawScalar);
+
+ tok = JsonbIteratorNext(&it, &v, true);
+ Assert(tok == WJB_ELEM);
+
+ res = pushJsonbValueScalar(pstate, seq, &v);
+
+ tok = JsonbIteratorNext(&it, &v, true);
+ Assert(tok == WJB_END_ARRAY);
+ Assert(it == NULL);
+
+ return res;
+ }
+
while ((tok = JsonbIteratorNext(&it, &v, false)) != WJB_DONE)
res = pushJsonbValueScalar(pstate, tok,
- tok < WJB_BEGIN_ARRAY ? &v : NULL);
+ tok < WJB_BEGIN_ARRAY ||
+ (tok == WJB_BEGIN_ARRAY &&
+ v.val.array.rawScalar) ? &v : NULL);
return res;
}
diff --git a/src/backend/utils/adt/jsonbsubs.c b/src/backend/utils/adt/jsonbsubs.c
new file mode 100644
index 0000000000..5237414be4
--- /dev/null
+++ b/src/backend/utils/adt/jsonbsubs.c
@@ -0,0 +1,413 @@
+/*-------------------------------------------------------------------------
+ *
+ * jsonbsubs.c
+ * Subscripting support functions for jsonb.
+ *
+ * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ * src/backend/utils/adt/jsonbsubs.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "executor/execExpr.h"
+#include "nodes/makefuncs.h"
+#include "nodes/nodeFuncs.h"
+#include "nodes/subscripting.h"
+#include "parser/parse_coerce.h"
+#include "parser/parse_expr.h"
+#include "utils/jsonb.h"
+#include "utils/jsonfuncs.h"
+#include "utils/builtins.h"
+#include "utils/lsyscache.h"
+
+
+/* SubscriptingRefState.workspace for jsonb subscripting execution */
+typedef struct JsonbSubWorkspace
+{
+ bool expectArray; /* jsonb root is expected to be an array */
+ Oid *indexOid; /* OID of coerced subscript expression,
+ could be only integer or text */
+ Datum *index; /* Subscript values in Datum format */
+} JsonbSubWorkspace;
+
+
+/*
+ * Finish parse analysis of a SubscriptingRef expression for a jsonb.
+ *
+ * Transform the subscript expressions, coerce them to text,
+ * and determine the result type of the SubscriptingRef node.
+ */
+static void
+jsonb_subscript_transform(SubscriptingRef *sbsref,
+ List *indirection,
+ ParseState *pstate,
+ bool isSlice,
+ bool isAssignment)
+{
+ List *upperIndexpr = NIL;
+ ListCell *idx;
+
+ /*
+ * Transform and convert the subscript expressions. Jsonb subscripting does
+ * not support slices, look only and the upper index.
+ */
+ foreach(idx, indirection)
+ {
+ A_Indices *ai = lfirst_node(A_Indices, idx);
+ Node *subExpr;
+
+ if (isSlice)
+ {
+ Node *expr = ai->uidx ? ai->uidx : ai->lidx;
+
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("jsonb subscript does not support slices"),
+ parser_errposition(pstate, exprLocation(expr))));
+ }
+
+ if (ai->uidx)
+ {
+ Oid subExprType = InvalidOid,
+ targetType = UNKNOWNOID;
+
+ subExpr = transformExpr(pstate, ai->uidx, pstate->p_expr_kind);
+ subExprType = exprType(subExpr);
+
+ if (subExprType != UNKNOWNOID)
+ {
+ Oid targets[2] = {INT4OID, TEXTOID};
+
+ /*
+ * Jsonb can handle multiple subscript types, but cases when a
+ * subscript could be coerced to multiple target types must be
+ * avoided, similar to overloaded functions. It could be
+ * possibly extend with jsonpath in the future.
+ */
+ for (int i = 0; i < 2; i++)
+ {
+ if (can_coerce_type(1, &subExprType, &targets[i], COERCION_IMPLICIT))
+ {
+ /*
+ * One type has already succeeded, it means there are
+ * two coercion targets possible, failure.
+ */
+ if (targetType != UNKNOWNOID)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("subscript type is not supported"),
+ errhint("Jsonb subscript must be coerced "
+ "only to one type, integer or text."),
+ parser_errposition(pstate, exprLocation(subExpr))));
+
+ targetType = targets[i];
+ }
+ }
+
+ /*
+ * No suitable types were found, failure.
+ */
+ if (targetType == UNKNOWNOID)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("subscript type is not supported"),
+ errhint("Jsonb subscript must be coerced to either integer or text"),
+ parser_errposition(pstate, exprLocation(subExpr))));
+ }
+ else
+ targetType = TEXTOID;
+
+ /*
+ * We known from can_coerce_type that coercion will succeed, so
+ * coerce_type could be used. Note the implicit coercion context,
+ * which is required to handle subscripts of different types,
+ * similar to overloaded functions.
+ */
+ subExpr = coerce_type(pstate,
+ subExpr, subExprType,
+ targetType, -1,
+ COERCION_IMPLICIT,
+ COERCE_IMPLICIT_CAST,
+ -1);
+ if (subExpr == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("jsonb subscript must have text type"),
+ parser_errposition(pstate, exprLocation(subExpr))));
+ }
+ else
+ {
+ /*
+ * Slice with omitted upper bound. Should not happen as we already
+ * errored out on slice earlier, but handle this just in case.
+ */
+ Assert(isSlice && ai->is_slice);
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("jsonb subscript does not support slices"),
+ parser_errposition(pstate, exprLocation(ai->uidx))));
+ }
+
+ upperIndexpr = lappend(upperIndexpr, subExpr);
+ }
+
+ /* store the transformed lists into the SubscriptRef node */
+ sbsref->refupperindexpr = upperIndexpr;
+ sbsref->reflowerindexpr = NIL;
+
+ /* Determine the result type of the subscripting operation; always jsonb */
+ sbsref->refrestype = JSONBOID;
+ sbsref->reftypmod = -1;
+}
+
+/*
+ * During execution, process the subscripts in a SubscriptingRef expression.
+ *
+ * The subscript expressions are already evaluated in Datum form in the
+ * SubscriptingRefState's arrays. Check and convert them as necessary.
+ *
+ * If any subscript is NULL, we throw error in assignment cases, or in fetch
+ * cases set result to NULL and return false (instructing caller to skip the
+ * rest of the SubscriptingRef sequence).
+ */
+static bool
+jsonb_subscript_check_subscripts(ExprState *state,
+ ExprEvalStep *op,
+ ExprContext *econtext)
+{
+ SubscriptingRefState *sbsrefstate = op->d.sbsref_subscript.state;
+ JsonbSubWorkspace *workspace = (JsonbSubWorkspace *) sbsrefstate->workspace;
+
+ /*
+ * In case if the first subscript is an integer, the source jsonb is
+ * expected to be an array. This information is not used directly, all such
+ * cases are handled within corresponding jsonb assign functions. But if
+ * the source jsonb is NULL the expected type will be used to construct an
+ * empty source.
+ */
+ if (sbsrefstate->numupper > 0 && sbsrefstate->upperprovided[0] &&
+ !sbsrefstate->upperindexnull[0] && workspace->indexOid[0] == INT4OID)
+ workspace->expectArray = true;
+
+ /* Process upper subscripts */
+ for (int i = 0; i < sbsrefstate->numupper; i++)
+ {
+ if (sbsrefstate->upperprovided[i])
+ {
+ /* If any index expr yields NULL, result is NULL or error */
+ if (sbsrefstate->upperindexnull[i])
+ {
+ if (sbsrefstate->isassignment)
+ ereport(ERROR,
+ (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
+ errmsg("jsonb subscript in assignment must not be null")));
+ *op->resnull = true;
+ return false;
+ }
+
+ /*
+ * For jsonb fetch and assign functions we need to provide path in
+ * text format. Convert if it's not already text.
+ */
+ if (workspace->indexOid[i] == INT4OID)
+ {
+ Datum datum = sbsrefstate->upperindex[i];
+ char *cs = DatumGetCString(DirectFunctionCall1(int4out, datum));
+ workspace->index[i] = CStringGetTextDatum(cs);
+ }
+ else
+ workspace->index[i] = sbsrefstate->upperindex[i];
+ }
+ }
+
+ return true;
+}
+
+/*
+ * Evaluate SubscriptingRef fetch for a jsonb element.
+ *
+ * Source container is in step's result variable (it's known not NULL, since
+ * we set fetch_strict to true).
+ */
+static void
+jsonb_subscript_fetch(ExprState *state,
+ ExprEvalStep *op,
+ ExprContext *econtext)
+{
+ SubscriptingRefState *sbsrefstate = op->d.sbsref.state;
+ JsonbSubWorkspace *workspace = (JsonbSubWorkspace *) sbsrefstate->workspace;
+ Jsonb *jsonbSource;
+
+ /* Should not get here if source jsonb (or any subscript) is null */
+ Assert(!(*op->resnull));
+
+ jsonbSource = DatumGetJsonbP(*op->resvalue);
+ *op->resvalue = jsonb_get_element(jsonbSource,
+ workspace->index,
+ sbsrefstate->numupper,
+ op->resnull,
+ false);
+}
+
+/*
+ * Evaluate SubscriptingRef assignment for a jsonb element assignment.
+ *
+ * Input container (possibly null) is in result area, replacement value is in
+ * SubscriptingRefState's replacevalue/replacenull.
+ */
+static void
+jsonb_subscript_assign(ExprState *state,
+ ExprEvalStep *op,
+ ExprContext *econtext)
+{
+ SubscriptingRefState *sbsrefstate = op->d.sbsref.state;
+ JsonbSubWorkspace *workspace = (JsonbSubWorkspace *) sbsrefstate->workspace;
+ Jsonb *jsonbSource;
+ JsonbValue *replacevalue;
+
+ if (sbsrefstate->replacenull)
+ {
+ replacevalue = (JsonbValue *) palloc(sizeof(JsonbValue));
+ replacevalue->type = jbvNull;
+ }
+ else
+ replacevalue =
+ JsonbToJsonbValue(DatumGetJsonbP(sbsrefstate->replacevalue));
+
+ /*
+ * In case if the input container is null, set up an empty jsonb and
+ * proceed with the assignment.
+ */
+ if (*op->resnull)
+ {
+ JsonbValue *newSource = (JsonbValue *) palloc(sizeof(JsonbValue));
+
+ /*
+ * To avoid any surprising results, set up an empty jsonb array in case
+ * of an array is expected (i.e. the first subscript is integer),
+ * otherwise jsonb object.
+ */
+ if (workspace->expectArray)
+ {
+ newSource->type = jbvArray;
+ newSource->val.array.nElems = 0;
+ newSource->val.array.rawScalar = false;
+ }
+ else
+ {
+ newSource->type = jbvObject;
+ newSource->val.object.nPairs = 0;
+ }
+
+ jsonbSource = JsonbValueToJsonb(newSource);
+ *op->resnull = false;
+ }
+ else
+ jsonbSource = DatumGetJsonbP(*op->resvalue);
+
+ *op->resvalue = jsonb_set_element(jsonbSource,
+ workspace->index,
+ sbsrefstate->numupper,
+ replacevalue);
+ /* The result is never NULL, so no need to change *op->resnull */
+}
+
+/*
+ * Compute old jsonb element value for a SubscriptingRef assignment
+ * expression. Will only be called if the new-value subexpression
+ * contains SubscriptingRef or FieldStore. This is the same as the
+ * regular fetch case, except that we have to handle a null jsonb,
+ * and the value should be stored into the SubscriptingRefState's
+ * prevvalue/prevnull fields.
+ */
+static void
+jsonb_subscript_fetch_old(ExprState *state,
+ ExprEvalStep *op,
+ ExprContext *econtext)
+{
+ SubscriptingRefState *sbsrefstate = op->d.sbsref.state;
+
+ if (*op->resnull)
+ {
+ /* whole jsonb is null, so any element is too */
+ sbsrefstate->prevvalue = (Datum) 0;
+ sbsrefstate->prevnull = true;
+ }
+ else
+ {
+ Jsonb *jsonbSource = DatumGetJsonbP(*op->resvalue);
+ sbsrefstate->prevvalue = jsonb_get_element(jsonbSource,
+ sbsrefstate->upperindex,
+ sbsrefstate->numupper,
+ &sbsrefstate->prevnull,
+ false);
+ }
+}
+
+/*
+ * Set up execution state for a jsonb subscript operation. Opposite to the
+ * arrays subscription, there is no limit for number of subscripts as jsonb
+ * type itself doesn't have nesting limits.
+ */
+static void
+jsonb_exec_setup(const SubscriptingRef *sbsref,
+ SubscriptingRefState *sbsrefstate,
+ SubscriptExecSteps *methods)
+{
+ JsonbSubWorkspace *workspace;
+ ListCell *lc;
+ int nupper = sbsref->refupperindexpr->length;
+ char *ptr;
+
+ /* Allocate type-specific workspace with space for per-subscript data */
+ workspace = palloc0(MAXALIGN(sizeof(JsonbSubWorkspace)) +
+ nupper * (sizeof(Datum) + sizeof(Oid)));
+ workspace->expectArray = false;
+ ptr = ((char *) workspace) + MAXALIGN(sizeof(JsonbSubWorkspace));
+ workspace->indexOid = (Oid *) ptr;
+ ptr += nupper * sizeof(Oid);
+ workspace->index = (Datum *) ptr;
+
+ sbsrefstate->workspace = workspace;
+
+ /* Collect subscript data types necessary at execution time */
+ foreach(lc, sbsref->refupperindexpr)
+ {
+ Node *expr = lfirst(lc);
+ int i = foreach_current_index(lc);
+
+ workspace->indexOid[i] = exprType(expr);
+ }
+
+ /*
+ * Pass back pointers to appropriate step execution functions.
+ */
+ methods->sbs_check_subscripts = jsonb_subscript_check_subscripts;
+ methods->sbs_fetch = jsonb_subscript_fetch;
+ methods->sbs_assign = jsonb_subscript_assign;
+ methods->sbs_fetch_old = jsonb_subscript_fetch_old;
+}
+
+/*
+ * jsonb_subscript_handler
+ * Subscripting handler for jsonb.
+ *
+ */
+Datum
+jsonb_subscript_handler(PG_FUNCTION_ARGS)
+{
+ static const SubscriptRoutines sbsroutines = {
+ .transform = jsonb_subscript_transform,
+ .exec_setup = jsonb_exec_setup,
+ .fetch_strict = true, /* fetch returns NULL for NULL inputs */
+ .fetch_leakproof = true, /* fetch returns NULL for bad subscript */
+ .store_leakproof = false /* ... but assignment throws error */
+ };
+
+ PG_RETURN_POINTER(&sbsroutines);
+}
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 69100feab7..5a0ba6b220 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -461,18 +461,18 @@ static Datum populate_domain(DomainIOData *io, Oid typid, const char *colname,
/* functions supporting jsonb_delete, jsonb_set and jsonb_concat */
static JsonbValue *IteratorConcat(JsonbIterator **it1, JsonbIterator **it2,
JsonbParseState **state);
-static JsonbValue *setPath(JsonbIterator **it, Datum *path_elems,
+extern JsonbValue *setPath(JsonbIterator **it, Datum *path_elems,
bool *path_nulls, int path_len,
- JsonbParseState **st, int level, Jsonb *newval,
+ JsonbParseState **st, int level, JsonbValue *newval,
int op_type);
static void setPathObject(JsonbIterator **it, Datum *path_elems,
bool *path_nulls, int path_len, JsonbParseState **st,
int level,
- Jsonb *newval, uint32 npairs, int op_type);
+ JsonbValue *newval, uint32 npairs, int op_type);
static void setPathArray(JsonbIterator **it, Datum *path_elems,
bool *path_nulls, int path_len, JsonbParseState **st,
- int level, Jsonb *newval, uint32 nelems, int op_type);
-static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb);
+ int level,
+ JsonbValue *newval, uint32 nelems, int op_type);
/* function supporting iterate_json_values */
static void iterate_values_scalar(void *state, char *token, JsonTokenType tokentype);
@@ -1448,13 +1448,9 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text)
ArrayType *path = PG_GETARG_ARRAYTYPE_P(1);
Datum *pathtext;
bool *pathnulls;
+ bool isnull;
int npath;
- int i;
- bool have_object = false,
- have_array = false;
- JsonbValue *jbvp = NULL;
- JsonbValue jbvbuf;
- JsonbContainer *container;
+ Datum res;
/*
* If the array contains any null elements, return NULL, on the grounds
@@ -1469,9 +1465,26 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text)
deconstruct_array(path, TEXTOID, -1, false, TYPALIGN_INT,
&pathtext, &pathnulls, &npath);
- /* Identify whether we have object, array, or scalar at top-level */
- container = &jb->root;
+ res = jsonb_get_element(jb, pathtext, npath, &isnull, as_text);
+ if (isnull)
+ PG_RETURN_NULL();
+ else
+ PG_RETURN_DATUM(res);
+}
+
+Datum
+jsonb_get_element(Jsonb *jb, Datum *path, int npath, bool *isnull, bool as_text)
+{
+ JsonbContainer *container = &jb->root;
+ JsonbValue *jbvp = NULL;
+ int i;
+ bool have_object = false,
+ have_array = false;
+
+ *isnull = false;
+
+ /* Identify whether we have object, array, or scalar at top-level */
if (JB_ROOT_IS_OBJECT(jb))
have_object = true;
else if (JB_ROOT_IS_ARRAY(jb) && !JB_ROOT_IS_SCALAR(jb))
@@ -1496,7 +1509,7 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text)
{
if (as_text)
{
- PG_RETURN_TEXT_P(cstring_to_text(JsonbToCString(NULL,
+ return PointerGetDatum(cstring_to_text(JsonbToCString(NULL,
container,
VARSIZE(jb))));
}
@@ -1512,22 +1525,25 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text)
if (have_object)
{
jbvp = getKeyJsonValueFromContainer(container,
- VARDATA(pathtext[i]),
- VARSIZE(pathtext[i]) - VARHDRSZ,
- &jbvbuf);
+ VARDATA(path[i]),
+ VARSIZE(path[i]) - VARHDRSZ,
+ NULL);
}
else if (have_array)
{
long lindex;
uint32 index;
- char *indextext = TextDatumGetCString(pathtext[i]);
+ char *indextext = TextDatumGetCString(path[i]);
char *endptr;
errno = 0;
lindex = strtol(indextext, &endptr, 10);
if (endptr == indextext || *endptr != '\0' || errno != 0 ||
lindex > INT_MAX || lindex < INT_MIN)
- PG_RETURN_NULL();
+ {
+ *isnull = true;
+ return PointerGetDatum(NULL);
+ }
if (lindex >= 0)
{
@@ -1545,7 +1561,10 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text)
nelements = JsonContainerSize(container);
if (-lindex > nelements)
- PG_RETURN_NULL();
+ {
+ *isnull = true;
+ return PointerGetDatum(NULL);
+ }
else
index = nelements + lindex;
}
@@ -1555,11 +1574,15 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text)
else
{
/* scalar, extraction yields a null */
- PG_RETURN_NULL();
+ *isnull = true;
+ return PointerGetDatum(NULL);
}
if (jbvp == NULL)
- PG_RETURN_NULL();
+ {
+ *isnull = true;
+ return PointerGetDatum(NULL);
+ }
else if (i == npath - 1)
break;
@@ -1581,9 +1604,12 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text)
if (as_text)
{
if (jbvp->type == jbvNull)
- PG_RETURN_NULL();
+ {
+ *isnull = true;
+ return PointerGetDatum(NULL);
+ }
- PG_RETURN_TEXT_P(JsonbValueAsText(jbvp));
+ return PointerGetDatum(JsonbValueAsText(jbvp));
}
else
{
@@ -1594,6 +1620,28 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text)
}
}
+Datum
+jsonb_set_element(Jsonb* jb, Datum *path, int path_len,
+ JsonbValue *newval)
+{
+ JsonbValue *res;
+ JsonbParseState *state = NULL;
+ JsonbIterator *it;
+ bool *path_nulls = palloc0(path_len * sizeof(bool));
+
+ if (newval->type == jbvArray && newval->val.array.rawScalar)
+ *newval = newval->val.array.elems[0];
+
+ it = JsonbIteratorInit(&jb->root);
+
+ res = setPath(&it, path, path_nulls, path_len, &state, 0,
+ newval, JB_PATH_CREATE);
+
+ pfree(path_nulls);
+
+ PG_RETURN_JSONB_P(JsonbValueToJsonb(res));
+}
+
/*
* Return the text representation of the given JsonbValue.
*/
@@ -4151,58 +4199,6 @@ jsonb_strip_nulls(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(JsonbValueToJsonb(res));
}
-/*
- * Add values from the jsonb to the parse state.
- *
- * If the parse state container is an object, the jsonb is pushed as
- * a value, not a key.
- *
- * This needs to be done using an iterator because pushJsonbValue doesn't
- * like getting jbvBinary values, so we can't just push jb as a whole.
- */
-static void
-addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb)
-{
- JsonbIterator *it;
- JsonbValue *o = &(*jbps)->contVal;
- JsonbValue v;
- JsonbIteratorToken type;
-
- it = JsonbIteratorInit(&jb->root);
-
- Assert(o->type == jbvArray || o->type == jbvObject);
-
- if (JB_ROOT_IS_SCALAR(jb))
- {
- (void) JsonbIteratorNext(&it, &v, false); /* skip array header */
- Assert(v.type == jbvArray);
- (void) JsonbIteratorNext(&it, &v, false); /* fetch scalar value */
-
- switch (o->type)
- {
- case jbvArray:
- (void) pushJsonbValue(jbps, WJB_ELEM, &v);
- break;
- case jbvObject:
- (void) pushJsonbValue(jbps, WJB_VALUE, &v);
- break;
- default:
- elog(ERROR, "unexpected parent of nested structure");
- }
- }
- else
- {
- while ((type = JsonbIteratorNext(&it, &v, false)) != WJB_DONE)
- {
- if (type == WJB_KEY || type == WJB_VALUE || type == WJB_ELEM)
- (void) pushJsonbValue(jbps, type, &v);
- else
- (void) pushJsonbValue(jbps, type, NULL);
- }
- }
-
-}
-
/*
* SQL function jsonb_pretty (jsonb)
*
@@ -4474,7 +4470,8 @@ jsonb_set(PG_FUNCTION_ARGS)
{
Jsonb *in = PG_GETARG_JSONB_P(0);
ArrayType *path = PG_GETARG_ARRAYTYPE_P(1);
- Jsonb *newval = PG_GETARG_JSONB_P(2);
+ Jsonb *newjsonb = PG_GETARG_JSONB_P(2);
+ JsonbValue *newval = JsonbToJsonbValue(newjsonb);
bool create = PG_GETARG_BOOL(3);
JsonbValue *res = NULL;
Datum *path_elems;
@@ -4632,7 +4629,8 @@ jsonb_insert(PG_FUNCTION_ARGS)
{
Jsonb *in = PG_GETARG_JSONB_P(0);
ArrayType *path = PG_GETARG_ARRAYTYPE_P(1);
- Jsonb *newval = PG_GETARG_JSONB_P(2);
+ Jsonb *newjsonb = PG_GETARG_JSONB_P(2);
+ JsonbValue *newval = JsonbToJsonbValue(newjsonb);
bool after = PG_GETARG_BOOL(3);
JsonbValue *res = NULL;
Datum *path_elems;
@@ -4787,10 +4785,10 @@ IteratorConcat(JsonbIterator **it1, JsonbIterator **it2,
* All path elements before the last must already exist
* whatever bits in op_type are set, or nothing is done.
*/
-static JsonbValue *
+JsonbValue *
setPath(JsonbIterator **it, Datum *path_elems,
bool *path_nulls, int path_len,
- JsonbParseState **st, int level, Jsonb *newval, int op_type)
+ JsonbParseState **st, int level, JsonbValue *newval, int op_type)
{
JsonbValue v;
JsonbIteratorToken r;
@@ -4843,11 +4841,11 @@ setPath(JsonbIterator **it, Datum *path_elems,
static void
setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
int path_len, JsonbParseState **st, int level,
- Jsonb *newval, uint32 npairs, int op_type)
+ JsonbValue *newval, uint32 npairs, int op_type)
{
- JsonbValue v;
int i;
- JsonbValue k;
+ JsonbValue k,
+ v;
bool done = false;
if (level >= path_len || path_nulls[level])
@@ -4864,7 +4862,7 @@ setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
newkey.val.string.val = VARDATA_ANY(path_elems[level]);
(void) pushJsonbValue(st, WJB_KEY, &newkey);
- addJsonbToParseState(st, newval);
+ (void) pushJsonbValue(st, WJB_VALUE, newval);
}
for (i = 0; i < npairs; i++)
@@ -4895,7 +4893,7 @@ setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
if (!(op_type & JB_PATH_DELETE))
{
(void) pushJsonbValue(st, WJB_KEY, &k);
- addJsonbToParseState(st, newval);
+ (void) pushJsonbValue(st, WJB_VALUE, newval);
}
done = true;
}
@@ -4918,7 +4916,7 @@ setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
newkey.val.string.val = VARDATA_ANY(path_elems[level]);
(void) pushJsonbValue(st, WJB_KEY, &newkey);
- addJsonbToParseState(st, newval);
+ (void) pushJsonbValue(st, WJB_VALUE, newval);
}
(void) pushJsonbValue(st, r, &k);
@@ -4950,7 +4948,7 @@ setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
static void
setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
int path_len, JsonbParseState **st, int level,
- Jsonb *newval, uint32 nelems, int op_type)
+ JsonbValue *newval, uint32 nelems, int op_type)
{
JsonbValue v;
int idx,
@@ -4998,7 +4996,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
(op_type & JB_PATH_CREATE_OR_INSERT))
{
Assert(newval != NULL);
- addJsonbToParseState(st, newval);
+ (void) pushJsonbValue(st, WJB_ELEM, newval);
done = true;
}
@@ -5014,7 +5012,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
r = JsonbIteratorNext(it, &v, true); /* skip */
if (op_type & (JB_PATH_INSERT_BEFORE | JB_PATH_CREATE))
- addJsonbToParseState(st, newval);
+ (void) pushJsonbValue(st, WJB_ELEM, newval);
/*
* We should keep current value only in case of
@@ -5025,7 +5023,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
(void) pushJsonbValue(st, r, &v);
if (op_type & (JB_PATH_INSERT_AFTER | JB_PATH_REPLACE))
- addJsonbToParseState(st, newval);
+ (void) pushJsonbValue(st, WJB_ELEM, newval);
done = true;
}
@@ -5059,7 +5057,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
if ((op_type & JB_PATH_CREATE_OR_INSERT) && !done &&
level == path_len - 1 && i == nelems - 1)
{
- addJsonbToParseState(st, newval);
+ (void) pushJsonbValue(st, WJB_ELEM, newval);
}
}
}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 139f4a08bd..feae8cc4b0 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -11239,6 +11239,10 @@
{ oid => '9256', descr => 'raw array subscripting support',
proname => 'raw_array_subscript_handler', prorettype => 'internal',
proargtypes => 'internal', prosrc => 'raw_array_subscript_handler' },
+# type subscripting support
+{ oid => '6098', descr => 'jsonb subscripting logic',
+ proname => 'jsonb_subscript_handler', prorettype => 'internal',
+ proargtypes => 'internal', prosrc => 'jsonb_subscript_handler' },
# collation management functions
{ oid => '3445', descr => 'import collations from operating system',
diff --git a/src/include/catalog/pg_type.dat b/src/include/catalog/pg_type.dat
index 62018f063a..4a530ca907 100644
--- a/src/include/catalog/pg_type.dat
+++ b/src/include/catalog/pg_type.dat
@@ -444,7 +444,8 @@
{ oid => '3802', array_type_oid => '3807', descr => 'Binary JSON',
typname => 'jsonb', typlen => '-1', typbyval => 'f', typcategory => 'U',
typinput => 'jsonb_in', typoutput => 'jsonb_out', typreceive => 'jsonb_recv',
- typsend => 'jsonb_send', typalign => 'i', typstorage => 'x' },
+ typsend => 'jsonb_send', typalign => 'i', typstorage => 'x',
+ typsubscript => 'jsonb_subscript_handler' },
{ oid => '4072', array_type_oid => '4073', descr => 'JSON path',
typname => 'jsonpath', typlen => '-1', typbyval => 'f', typcategory => 'U',
typinput => 'jsonpath_in', typoutput => 'jsonpath_out',
diff --git a/src/include/utils/jsonb.h b/src/include/utils/jsonb.h
index 5860011693..64f1ccbf77 100644
--- a/src/include/utils/jsonb.h
+++ b/src/include/utils/jsonb.h
@@ -392,6 +392,7 @@ extern JsonbValue *pushJsonbValue(JsonbParseState **pstate,
extern JsonbIterator *JsonbIteratorInit(JsonbContainer *container);
extern JsonbIteratorToken JsonbIteratorNext(JsonbIterator **it, JsonbValue *val,
bool skipNested);
+extern JsonbValue *JsonbToJsonbValue(Jsonb *jsonb);
extern Jsonb *JsonbValueToJsonb(JsonbValue *val);
extern bool JsonbDeepContains(JsonbIterator **val,
JsonbIterator **mContained);
@@ -407,5 +408,8 @@ extern char *JsonbToCStringIndent(StringInfo out, JsonbContainer *in,
extern bool JsonbExtractScalar(JsonbContainer *jbc, JsonbValue *res);
extern const char *JsonbTypeName(JsonbValue *jb);
-
+extern Datum jsonb_set_element(Jsonb *jb, Datum *path, int path_len,
+ JsonbValue *newval);
+extern Datum jsonb_get_element(Jsonb *jb, Datum *path, int npath,
+ bool *isnull, bool as_text);
#endif /* __JSONB_H__ */
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index 1e6c6ef200..46bf2e2353 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -4599,7 +4599,7 @@ select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment => 'use_
{"a": 1, "b": null}
(1 row)
-\pset null
+\pset null ''
-- jsonb_insert
select jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"');
jsonb_insert
@@ -4729,6 +4729,276 @@ HINT: Try using the function jsonb_set to replace key value.
select jsonb_insert('{"a": {"b": "value"}}', '{a, b}', '"new_value"', true);
ERROR: cannot replace existing key
HINT: Try using the function jsonb_set to replace key value.
+-- jsonb subscript
+select ('123'::jsonb)['a'];
+ jsonb
+-------
+
+(1 row)
+
+select ('123'::jsonb)[0];
+ jsonb
+-------
+
+(1 row)
+
+select ('123'::jsonb)[NULL];
+ jsonb
+-------
+
+(1 row)
+
+select ('{"a": 1}'::jsonb)['a'];
+ jsonb
+-------
+ 1
+(1 row)
+
+select ('{"a": 1}'::jsonb)[0];
+ jsonb
+-------
+
+(1 row)
+
+select ('{"a": 1}'::jsonb)['not_exist'];
+ jsonb
+-------
+
+(1 row)
+
+select ('{"a": 1}'::jsonb)[NULL];
+ jsonb
+-------
+
+(1 row)
+
+select ('[1, "2", null]'::jsonb)['a'];
+ jsonb
+-------
+
+(1 row)
+
+select ('[1, "2", null]'::jsonb)[0];
+ jsonb
+-------
+ 1
+(1 row)
+
+select ('[1, "2", null]'::jsonb)['1'];
+ jsonb
+-------
+ "2"
+(1 row)
+
+select ('[1, "2", null]'::jsonb)[1.0];
+ERROR: subscript type is not supported
+LINE 1: select ('[1, "2", null]'::jsonb)[1.0];
+ ^
+HINT: Jsonb subscript must be coerced to either integer or text
+select ('[1, "2", null]'::jsonb)[2];
+ jsonb
+-------
+ null
+(1 row)
+
+select ('[1, "2", null]'::jsonb)[3];
+ jsonb
+-------
+
+(1 row)
+
+select ('[1, "2", null]'::jsonb)[-2];
+ jsonb
+-------
+ "2"
+(1 row)
+
+select ('[1, "2", null]'::jsonb)[1]['a'];
+ jsonb
+-------
+
+(1 row)
+
+select ('[1, "2", null]'::jsonb)[1][0];
+ jsonb
+-------
+
+(1 row)
+
+select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb)['b'];
+ jsonb
+-------
+ "c"
+(1 row)
+
+select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb)['d'];
+ jsonb
+-----------
+ [1, 2, 3]
+(1 row)
+
+select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb)['d'][1];
+ jsonb
+-------
+ 2
+(1 row)
+
+select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb)['d']['a'];
+ jsonb
+-------
+
+(1 row)
+
+select ('{"a": {"a1": {"a2": "aaa"}}, "b": "bbb", "c": "ccc"}'::jsonb)['a']['a1'];
+ jsonb
+---------------
+ {"a2": "aaa"}
+(1 row)
+
+select ('{"a": {"a1": {"a2": "aaa"}}, "b": "bbb", "c": "ccc"}'::jsonb)['a']['a1']['a2'];
+ jsonb
+-------
+ "aaa"
+(1 row)
+
+select ('{"a": {"a1": {"a2": "aaa"}}, "b": "bbb", "c": "ccc"}'::jsonb)['a']['a1']['a2']['a3'];
+ jsonb
+-------
+
+(1 row)
+
+select ('{"a": ["a1", {"b1": ["aaa", "bbb", "ccc"]}], "b": "bb"}'::jsonb)['a'][1]['b1'];
+ jsonb
+-----------------------
+ ["aaa", "bbb", "ccc"]
+(1 row)
+
+select ('{"a": ["a1", {"b1": ["aaa", "bbb", "ccc"]}], "b": "bb"}'::jsonb)['a'][1]['b1'][2];
+ jsonb
+-------
+ "ccc"
+(1 row)
+
+-- slices are not supported
+select ('{"a": 1}'::jsonb)['a':'b'];
+ERROR: jsonb subscript does not support slices
+LINE 1: select ('{"a": 1}'::jsonb)['a':'b'];
+ ^
+select ('[1, "2", null]'::jsonb)[1:2];
+ERROR: jsonb subscript does not support slices
+LINE 1: select ('[1, "2", null]'::jsonb)[1:2];
+ ^
+select ('[1, "2", null]'::jsonb)[:2];
+ERROR: jsonb subscript does not support slices
+LINE 1: select ('[1, "2", null]'::jsonb)[:2];
+ ^
+select ('[1, "2", null]'::jsonb)[1:];
+ERROR: jsonb subscript does not support slices
+LINE 1: select ('[1, "2", null]'::jsonb)[1:];
+ ^
+select ('[1, "2", null]'::jsonb)[:];
+ERROR: jsonb subscript does not support slices
+create TEMP TABLE test_jsonb_subscript (
+ id int,
+ test_json jsonb
+);
+insert into test_jsonb_subscript values
+(1, '{}'), -- empty jsonb
+(2, '{"key": "value"}'); -- jsonb with data
+-- update empty jsonb
+update test_jsonb_subscript set test_json['a'] = '1' where id = 1;
+select * from test_jsonb_subscript;
+ id | test_json
+----+------------------
+ 2 | {"key": "value"}
+ 1 | {"a": 1}
+(2 rows)
+
+-- update jsonb with some data
+update test_jsonb_subscript set test_json['a'] = '1' where id = 2;
+select * from test_jsonb_subscript;
+ id | test_json
+----+--------------------------
+ 1 | {"a": 1}
+ 2 | {"a": 1, "key": "value"}
+(2 rows)
+
+-- replace jsonb
+update test_jsonb_subscript set test_json['a'] = '"test"';
+select * from test_jsonb_subscript;
+ id | test_json
+----+-------------------------------
+ 1 | {"a": "test"}
+ 2 | {"a": "test", "key": "value"}
+(2 rows)
+
+-- replace by object
+update test_jsonb_subscript set test_json['a'] = '{"b": 1}'::jsonb;
+select * from test_jsonb_subscript;
+ id | test_json
+----+---------------------------------
+ 1 | {"a": {"b": 1}}
+ 2 | {"a": {"b": 1}, "key": "value"}
+(2 rows)
+
+-- replace by array
+update test_jsonb_subscript set test_json['a'] = '[1, 2, 3]'::jsonb;
+select * from test_jsonb_subscript;
+ id | test_json
+----+----------------------------------
+ 1 | {"a": [1, 2, 3]}
+ 2 | {"a": [1, 2, 3], "key": "value"}
+(2 rows)
+
+-- use jsonb subscription in where clause
+select * from test_jsonb_subscript where test_json['key'] = '"value"';
+ id | test_json
+----+----------------------------------
+ 2 | {"a": [1, 2, 3], "key": "value"}
+(1 row)
+
+select * from test_jsonb_subscript where test_json['key_doesnt_exists'] = '"value"';
+ id | test_json
+----+-----------
+(0 rows)
+
+select * from test_jsonb_subscript where test_json['key'] = '"wrong_value"';
+ id | test_json
+----+-----------
+(0 rows)
+
+-- NULL
+update test_jsonb_subscript set test_json[NULL] = '1';
+ERROR: jsonb subscript in assignment must not be null
+update test_jsonb_subscript set test_json['another_key'] = NULL;
+select * from test_jsonb_subscript;
+ id | test_json
+----+-------------------------------------------------------
+ 1 | {"a": [1, 2, 3], "another_key": null}
+ 2 | {"a": [1, 2, 3], "key": "value", "another_key": null}
+(2 rows)
+
+-- NULL as jsonb source
+insert into test_jsonb_subscript values (3, NULL);
+update test_jsonb_subscript set test_json['a'] = '1' where id = 3;
+select * from test_jsonb_subscript;
+ id | test_json
+----+-------------------------------------------------------
+ 1 | {"a": [1, 2, 3], "another_key": null}
+ 2 | {"a": [1, 2, 3], "key": "value", "another_key": null}
+ 3 | {"a": 1}
+(3 rows)
+
+update test_jsonb_subscript set test_json = NULL where id = 3;
+update test_jsonb_subscript set test_json[0] = '1';
+select * from test_jsonb_subscript;
+ id | test_json
+----+---------------------------------------------------------------
+ 1 | {"0": 1, "a": [1, 2, 3], "another_key": null}
+ 2 | {"0": 1, "a": [1, 2, 3], "key": "value", "another_key": null}
+ 3 | [1]
+(3 rows)
+
-- jsonb to tsvector
select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb);
to_tsvector
diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index b6409767f6..20aa8fe0e2 100644
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -1177,7 +1177,7 @@ select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment => 'retu
select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment => 'delete_key') as delete_key;
select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment => 'use_json_null') as use_json_null;
-\pset null
+\pset null ''
-- jsonb_insert
select jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"');
@@ -1208,6 +1208,88 @@ select jsonb_insert('{"a": {"b": "value"}}', '{a, c}', '"new_value"', true);
select jsonb_insert('{"a": {"b": "value"}}', '{a, b}', '"new_value"');
select jsonb_insert('{"a": {"b": "value"}}', '{a, b}', '"new_value"', true);
+-- jsonb subscript
+select ('123'::jsonb)['a'];
+select ('123'::jsonb)[0];
+select ('123'::jsonb)[NULL];
+select ('{"a": 1}'::jsonb)['a'];
+select ('{"a": 1}'::jsonb)[0];
+select ('{"a": 1}'::jsonb)['not_exist'];
+select ('{"a": 1}'::jsonb)[NULL];
+select ('[1, "2", null]'::jsonb)['a'];
+select ('[1, "2", null]'::jsonb)[0];
+select ('[1, "2", null]'::jsonb)['1'];
+select ('[1, "2", null]'::jsonb)[1.0];
+select ('[1, "2", null]'::jsonb)[2];
+select ('[1, "2", null]'::jsonb)[3];
+select ('[1, "2", null]'::jsonb)[-2];
+select ('[1, "2", null]'::jsonb)[1]['a'];
+select ('[1, "2", null]'::jsonb)[1][0];
+select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb)['b'];
+select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb)['d'];
+select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb)['d'][1];
+select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb)['d']['a'];
+select ('{"a": {"a1": {"a2": "aaa"}}, "b": "bbb", "c": "ccc"}'::jsonb)['a']['a1'];
+select ('{"a": {"a1": {"a2": "aaa"}}, "b": "bbb", "c": "ccc"}'::jsonb)['a']['a1']['a2'];
+select ('{"a": {"a1": {"a2": "aaa"}}, "b": "bbb", "c": "ccc"}'::jsonb)['a']['a1']['a2']['a3'];
+select ('{"a": ["a1", {"b1": ["aaa", "bbb", "ccc"]}], "b": "bb"}'::jsonb)['a'][1]['b1'];
+select ('{"a": ["a1", {"b1": ["aaa", "bbb", "ccc"]}], "b": "bb"}'::jsonb)['a'][1]['b1'][2];
+
+-- slices are not supported
+select ('{"a": 1}'::jsonb)['a':'b'];
+select ('[1, "2", null]'::jsonb)[1:2];
+select ('[1, "2", null]'::jsonb)[:2];
+select ('[1, "2", null]'::jsonb)[1:];
+select ('[1, "2", null]'::jsonb)[:];
+
+create TEMP TABLE test_jsonb_subscript (
+ id int,
+ test_json jsonb
+);
+
+insert into test_jsonb_subscript values
+(1, '{}'), -- empty jsonb
+(2, '{"key": "value"}'); -- jsonb with data
+
+-- update empty jsonb
+update test_jsonb_subscript set test_json['a'] = '1' where id = 1;
+select * from test_jsonb_subscript;
+
+-- update jsonb with some data
+update test_jsonb_subscript set test_json['a'] = '1' where id = 2;
+select * from test_jsonb_subscript;
+
+-- replace jsonb
+update test_jsonb_subscript set test_json['a'] = '"test"';
+select * from test_jsonb_subscript;
+
+-- replace by object
+update test_jsonb_subscript set test_json['a'] = '{"b": 1}'::jsonb;
+select * from test_jsonb_subscript;
+
+-- replace by array
+update test_jsonb_subscript set test_json['a'] = '[1, 2, 3]'::jsonb;
+select * from test_jsonb_subscript;
+
+-- use jsonb subscription in where clause
+select * from test_jsonb_subscript where test_json['key'] = '"value"';
+select * from test_jsonb_subscript where test_json['key_doesnt_exists'] = '"value"';
+select * from test_jsonb_subscript where test_json['key'] = '"wrong_value"';
+
+-- NULL
+update test_jsonb_subscript set test_json[NULL] = '1';
+update test_jsonb_subscript set test_json['another_key'] = NULL;
+select * from test_jsonb_subscript;
+
+-- NULL as jsonb source
+insert into test_jsonb_subscript values (3, NULL);
+update test_jsonb_subscript set test_json['a'] = '1' where id = 3;
+select * from test_jsonb_subscript;
+
+update test_jsonb_subscript set test_json = NULL where id = 3;
+update test_jsonb_subscript set test_json[0] = '1';
+select * from test_jsonb_subscript;
+
-- jsonb to tsvector
select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb);
--
2.21.0
--3x42k255okwqcjpg
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v49-0002-Filling-gaps-in-jsonb.patch"
^ permalink raw reply [nested|flat] 8+ messages in thread
* injection points for hash aggregation
@ 2025-02-03 20:45 Jeff Davis <[email protected]>
2025-02-03 23:54 ` Re: injection points for hash aggregation Jeff Davis <[email protected]>
2025-02-04 00:21 ` Re: injection points for hash aggregation Michael Paquier <[email protected]>
0 siblings, 2 replies; 8+ messages in thread
From: Jeff Davis @ 2025-02-03 20:45 UTC (permalink / raw)
To: pgsql-hackers
Attached is a patch that adds a few injection points for hash
aggregation.
A couple questions on the injection points framework:
* The patch allows forcing the partition fan-out to one. I could
imagine forcing it to a specific value, is there a way to do that?
* The injection_points extension offers the concept of a "local"
injection point. While that makes sense for the callback function, it
doesn't make sense when using the "local_var = 123" style, because that
will happen regardless of the condition, right?
* Callbacks are given very little context from the injection point
itself, so it's hard for me to imagine what a callback might do other
than logging the name of the injection point or waiting (as the
extension implements). What else would callbacks be good for?
--
Jeff Davis
PostgreSQL Contributor Team - AWS
Attachments:
[text/x-patch] v1-0001-Add-injection-points-for-hash-aggregation.patch (8.0K, ../../[email protected]/2-v1-0001-Add-injection-points-for-hash-aggregation.patch)
download | inline diff:
From f98ee96920de46a5bba4da4079395d5e959472a4 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Sun, 2 Feb 2025 05:26:58 -0800
Subject: [PATCH v1] Add injection points for hash aggregation.
---
src/backend/executor/nodeAgg.c | 20 ++++++
src/test/modules/injection_points/Makefile | 2 +-
.../injection_points/expected/hashagg.out | 68 +++++++++++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../modules/injection_points/sql/hashagg.sql | 22 ++++++
5 files changed, 112 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/injection_points/expected/hashagg.out
create mode 100644 src/test/modules/injection_points/sql/hashagg.sql
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 3005b5c0e3b..267a3229145 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -269,6 +269,7 @@
#include "utils/datum.h"
#include "utils/dynahash.h"
#include "utils/expandeddatum.h"
+#include "utils/injection_point.h"
#include "utils/logtape.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -1489,6 +1490,14 @@ build_hash_tables(AggState *aggstate)
perhash->aggnode->numGroups,
memory);
+#ifdef USE_INJECTION_POINTS
+ if (IS_INJECTION_POINT_ATTACHED("hash-aggregate-oversize-table"))
+ {
+ nbuckets = memory / sizeof(TupleHashEntryData);
+ INJECTION_POINT_CACHED("hash-aggregate-oversize-table");
+ }
+#endif
+
build_hash_table(aggstate, setno, nbuckets);
}
@@ -1881,6 +1890,7 @@ hash_agg_check_limits(AggState *aggstate)
static void
hash_agg_enter_spill_mode(AggState *aggstate)
{
+ INJECTION_POINT("hash-aggregate-enter-spill-mode");
aggstate->hash_spill_mode = true;
hashagg_recompile_expressions(aggstate, aggstate->table_filled, true);
@@ -2652,6 +2662,7 @@ agg_refill_hash_table(AggState *aggstate)
*/
hashagg_recompile_expressions(aggstate, true, true);
+ INJECTION_POINT("hash-aggregate-process-batch");
for (;;)
{
TupleTableSlot *spillslot = aggstate->hash_spill_rslot;
@@ -2900,6 +2911,15 @@ hashagg_spill_init(HashAggSpill *spill, LogicalTapeSet *tapeset, int used_bits,
npartitions = hash_choose_num_partitions(input_groups, hashentrysize,
used_bits, &partition_bits);
+#ifdef USE_INJECTION_POINTS
+ if (IS_INJECTION_POINT_ATTACHED("hash-aggregate-single-partition"))
+ {
+ npartitions = 1;
+ partition_bits = 0;
+ INJECTION_POINT_CACHED("hash-aggregate-single-partition");
+ }
+#endif
+
spill->partitions = palloc0(sizeof(LogicalTape *) * npartitions);
spill->ntuples = palloc0(sizeof(int64) * npartitions);
spill->hll_card = palloc0(sizeof(hyperLogLogState) * npartitions);
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 4f0161fd33a..e680991f8d4 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -11,7 +11,7 @@ EXTENSION = injection_points
DATA = injection_points--1.0.sql
PGFILEDESC = "injection_points - facility for injection points"
-REGRESS = injection_points reindex_conc
+REGRESS = injection_points hashagg reindex_conc
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic inplace syscache-update-pruned
diff --git a/src/test/modules/injection_points/expected/hashagg.out b/src/test/modules/injection_points/expected/hashagg.out
new file mode 100644
index 00000000000..795a53210c4
--- /dev/null
+++ b/src/test/modules/injection_points/expected/hashagg.out
@@ -0,0 +1,68 @@
+-- Test for hash aggregation
+CREATE EXTENSION injection_points;
+SELECT injection_points_set_local();
+ injection_points_set_local
+----------------------------
+
+(1 row)
+
+SELECT injection_points_attach('hash-aggregate-enter-spill-mode', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_attach('hash-aggregate-process-batch', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_attach('hash-aggregate-single-partition', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+CREATE TABLE hashagg_ij(x INTEGER);
+INSERT INTO hashagg_ij SELECT g FROM generate_series(1,100000) g;
+SET max_parallel_workers=0;
+SET max_parallel_workers_per_gather=0;
+SET enable_sort=FALSE;
+SET work_mem='1MB';
+SET hash_mem_multiplier=1.0;
+SELECT COUNT(*) FROM (SELECT DISTINCT x FROM hashagg_ij) s;
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+ count
+--------
+ 100000
+(1 row)
+
+DROP TABLE hashagg_ij;
+DROP EXTENSION injection_points;
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 259045e5c2d..d61149712fd 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -35,6 +35,7 @@ tests += {
'regress': {
'sql': [
'injection_points',
+ 'hashagg',
'reindex_conc',
],
'regress_args': ['--dlpath', meson.build_root() / 'src/test/regress'],
diff --git a/src/test/modules/injection_points/sql/hashagg.sql b/src/test/modules/injection_points/sql/hashagg.sql
new file mode 100644
index 00000000000..c852c590771
--- /dev/null
+++ b/src/test/modules/injection_points/sql/hashagg.sql
@@ -0,0 +1,22 @@
+-- Test for hash aggregation
+CREATE EXTENSION injection_points;
+
+SELECT injection_points_set_local();
+
+SELECT injection_points_attach('hash-aggregate-enter-spill-mode', 'notice');
+SELECT injection_points_attach('hash-aggregate-process-batch', 'notice');
+SELECT injection_points_attach('hash-aggregate-single-partition', 'notice');
+
+CREATE TABLE hashagg_ij(x INTEGER);
+INSERT INTO hashagg_ij SELECT g FROM generate_series(1,100000) g;
+
+SET max_parallel_workers=0;
+SET max_parallel_workers_per_gather=0;
+SET enable_sort=FALSE;
+SET work_mem='1MB';
+SET hash_mem_multiplier=1.0;
+
+SELECT COUNT(*) FROM (SELECT DISTINCT x FROM hashagg_ij) s;
+
+DROP TABLE hashagg_ij;
+DROP EXTENSION injection_points;
--
2.34.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: injection points for hash aggregation
2025-02-03 20:45 injection points for hash aggregation Jeff Davis <[email protected]>
@ 2025-02-03 23:54 ` Jeff Davis <[email protected]>
2025-02-11 19:48 ` Re: injection points for hash aggregation Jeff Davis <[email protected]>
1 sibling, 1 reply; 8+ messages in thread
From: Jeff Davis @ 2025-02-03 23:54 UTC (permalink / raw)
To: pgsql-hackers
On Mon, 2025-02-03 at 12:45 -0800, Jeff Davis wrote:
> * The patch allows forcing the partition fan-out to one. I could
> imagine forcing it to a specific value, is there a way to do that?
I hit "send" too quickly and this caused test failures in CI. Attaching
v2.
Changes:
* a new injection point to force spilling at 1000 tuples so that the
test is deterministic (required some minor refactoring in
hash_agg_check_limits())
* added a branch to guard against a shift-by-32, which could not
happen in the code before, because the number of partitions was a
minimum of 4
* minor refactor of hash_agg_set_limits() to avoid an implicit
assumption. This is not directly related, so I added it as a separate
patch.
Regards,
Jeff Davis
Attachments:
[text/x-patch] v2-0001-Add-injection-points-for-hash-aggregation.patch (9.7K, ../../[email protected]/2-v2-0001-Add-injection-points-for-hash-aggregation.patch)
download | inline diff:
From 513889206e048abc2493d2902787404f86ca45d7 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Sun, 2 Feb 2025 05:26:58 -0800
Subject: [PATCH v2 1/2] Add injection points for hash aggregation.
---
src/backend/executor/nodeAgg.c | 62 ++++++++++++++---
src/test/modules/injection_points/Makefile | 2 +-
.../injection_points/expected/hashagg.out | 68 +++++++++++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../modules/injection_points/sql/hashagg.sql | 26 +++++++
5 files changed, 150 insertions(+), 9 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/hashagg.out
create mode 100644 src/test/modules/injection_points/sql/hashagg.sql
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 3005b5c0e3b..35cf18e5282 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -269,6 +269,7 @@
#include "utils/datum.h"
#include "utils/dynahash.h"
#include "utils/expandeddatum.h"
+#include "utils/injection_point.h"
#include "utils/logtape.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -1489,6 +1490,14 @@ build_hash_tables(AggState *aggstate)
perhash->aggnode->numGroups,
memory);
+#ifdef USE_INJECTION_POINTS
+ if (IS_INJECTION_POINT_ATTACHED("hash-aggregate-oversize-table"))
+ {
+ nbuckets = memory / sizeof(TupleHashEntryData);
+ INJECTION_POINT_CACHED("hash-aggregate-oversize-table");
+ }
+#endif
+
build_hash_table(aggstate, setno, nbuckets);
}
@@ -1860,17 +1869,36 @@ hash_agg_check_limits(AggState *aggstate)
true);
Size hashkey_mem = MemoryContextMemAllocated(aggstate->hashcontext->ecxt_per_tuple_memory,
true);
+ bool do_spill = false;
/*
- * Don't spill unless there's at least one group in the hash table so we
- * can be sure to make progress even in edge cases.
+ * Spill if exceeding memory limit. Don't spill unless there's at least
+ * one group in the hash table so we can be sure to make progress even in
+ * edge cases.
*/
- if (aggstate->hash_ngroups_current > 0 &&
- (meta_mem + hashkey_mem > aggstate->hash_mem_limit ||
- ngroups > aggstate->hash_ngroups_limit))
+ if (ngroups > 0 && (meta_mem + hashkey_mem > aggstate->hash_mem_limit))
+ do_spill = true;
+
+ /*
+ * Spill if exceeding ngroups limit. This is important if transition
+ * values may grow, e.g. for ARRAY_AGG().
+ */
+ if (ngroups > aggstate->hash_ngroups_limit)
+ do_spill = true;
+
+#ifdef USE_INJECTION_POINTS
+ if (ngroups > 1000)
{
- hash_agg_enter_spill_mode(aggstate);
+ if (IS_INJECTION_POINT_ATTACHED("hash-aggregate-spill-1000"))
+ {
+ do_spill = true;
+ INJECTION_POINT_CACHED("hash-aggregate-spill-1000");
+ }
}
+#endif
+
+ if (do_spill)
+ hash_agg_enter_spill_mode(aggstate);
}
/*
@@ -1881,6 +1909,7 @@ hash_agg_check_limits(AggState *aggstate)
static void
hash_agg_enter_spill_mode(AggState *aggstate)
{
+ INJECTION_POINT("hash-aggregate-enter-spill-mode");
aggstate->hash_spill_mode = true;
hashagg_recompile_expressions(aggstate, aggstate->table_filled, true);
@@ -2652,6 +2681,7 @@ agg_refill_hash_table(AggState *aggstate)
*/
hashagg_recompile_expressions(aggstate, true, true);
+ INJECTION_POINT("hash-aggregate-process-batch");
for (;;)
{
TupleTableSlot *spillslot = aggstate->hash_spill_rslot;
@@ -2900,6 +2930,15 @@ hashagg_spill_init(HashAggSpill *spill, LogicalTapeSet *tapeset, int used_bits,
npartitions = hash_choose_num_partitions(input_groups, hashentrysize,
used_bits, &partition_bits);
+#ifdef USE_INJECTION_POINTS
+ if (IS_INJECTION_POINT_ATTACHED("hash-aggregate-single-partition"))
+ {
+ npartitions = 1;
+ partition_bits = 0;
+ INJECTION_POINT_CACHED("hash-aggregate-single-partition");
+ }
+#endif
+
spill->partitions = palloc0(sizeof(LogicalTape *) * npartitions);
spill->ntuples = palloc0(sizeof(int64) * npartitions);
spill->hll_card = palloc0(sizeof(hyperLogLogState) * npartitions);
@@ -2908,7 +2947,10 @@ hashagg_spill_init(HashAggSpill *spill, LogicalTapeSet *tapeset, int used_bits,
spill->partitions[i] = LogicalTapeCreate(tapeset);
spill->shift = 32 - used_bits - partition_bits;
- spill->mask = (npartitions - 1) << spill->shift;
+ if (spill->shift < 32)
+ spill->mask = (npartitions - 1) << spill->shift;
+ else
+ spill->mask = 0;
spill->npartitions = npartitions;
for (int i = 0; i < npartitions; i++)
@@ -2957,7 +2999,11 @@ hashagg_spill_tuple(AggState *aggstate, HashAggSpill *spill,
tuple = ExecFetchSlotMinimalTuple(spillslot, &shouldFree);
- partition = (hash & spill->mask) >> spill->shift;
+ if (spill->shift < 32)
+ partition = (hash & spill->mask) >> spill->shift;
+ else
+ partition = 0;
+
spill->ntuples[partition]++;
/*
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 4f0161fd33a..e680991f8d4 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -11,7 +11,7 @@ EXTENSION = injection_points
DATA = injection_points--1.0.sql
PGFILEDESC = "injection_points - facility for injection points"
-REGRESS = injection_points reindex_conc
+REGRESS = injection_points hashagg reindex_conc
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic inplace syscache-update-pruned
diff --git a/src/test/modules/injection_points/expected/hashagg.out b/src/test/modules/injection_points/expected/hashagg.out
new file mode 100644
index 00000000000..cc4247af97d
--- /dev/null
+++ b/src/test/modules/injection_points/expected/hashagg.out
@@ -0,0 +1,68 @@
+-- Test for hash aggregation
+CREATE EXTENSION injection_points;
+SELECT injection_points_set_local();
+ injection_points_set_local
+----------------------------
+
+(1 row)
+
+SELECT injection_points_attach('hash-aggregate-enter-spill-mode', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_attach('hash-aggregate-process-batch', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+-- force partition fan-out to 1
+SELECT injection_points_attach('hash-aggregate-single-partition', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+-- force spilling after 1000 groups
+SELECT injection_points_attach('hash-aggregate-spill-1000', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+CREATE TABLE hashagg_ij(x INTEGER);
+INSERT INTO hashagg_ij SELECT g FROM generate_series(1,5100) g;
+SET max_parallel_workers=0;
+SET max_parallel_workers_per_gather=0;
+SET enable_sort=FALSE;
+SET work_mem='4MB';
+SELECT COUNT(*) FROM (SELECT DISTINCT x FROM hashagg_ij) s;
+NOTICE: notice triggered for injection point hash-aggregate-spill-1000
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-spill-1000
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-spill-1000
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-spill-1000
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+NOTICE: notice triggered for injection point hash-aggregate-spill-1000
+NOTICE: notice triggered for injection point hash-aggregate-enter-spill-mode
+NOTICE: notice triggered for injection point hash-aggregate-single-partition
+NOTICE: notice triggered for injection point hash-aggregate-process-batch
+ count
+-------
+ 5100
+(1 row)
+
+DROP TABLE hashagg_ij;
+DROP EXTENSION injection_points;
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 259045e5c2d..d61149712fd 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -35,6 +35,7 @@ tests += {
'regress': {
'sql': [
'injection_points',
+ 'hashagg',
'reindex_conc',
],
'regress_args': ['--dlpath', meson.build_root() / 'src/test/regress'],
diff --git a/src/test/modules/injection_points/sql/hashagg.sql b/src/test/modules/injection_points/sql/hashagg.sql
new file mode 100644
index 00000000000..51d814623fc
--- /dev/null
+++ b/src/test/modules/injection_points/sql/hashagg.sql
@@ -0,0 +1,26 @@
+-- Test for hash aggregation
+CREATE EXTENSION injection_points;
+
+SELECT injection_points_set_local();
+
+SELECT injection_points_attach('hash-aggregate-enter-spill-mode', 'notice');
+SELECT injection_points_attach('hash-aggregate-process-batch', 'notice');
+
+-- force partition fan-out to 1
+SELECT injection_points_attach('hash-aggregate-single-partition', 'notice');
+
+-- force spilling after 1000 groups
+SELECT injection_points_attach('hash-aggregate-spill-1000', 'notice');
+
+CREATE TABLE hashagg_ij(x INTEGER);
+INSERT INTO hashagg_ij SELECT g FROM generate_series(1,5100) g;
+
+SET max_parallel_workers=0;
+SET max_parallel_workers_per_gather=0;
+SET enable_sort=FALSE;
+SET work_mem='4MB';
+
+SELECT COUNT(*) FROM (SELECT DISTINCT x FROM hashagg_ij) s;
+
+DROP TABLE hashagg_ij;
+DROP EXTENSION injection_points;
--
2.34.1
[text/x-patch] v2-0002-Minor-refactor-of-hash_agg_set_limits.patch (2.2K, ../../[email protected]/3-v2-0002-Minor-refactor-of-hash_agg_set_limits.patch)
download | inline diff:
From c1a619500a41dee654868efe480b0b8a7c67dc4f Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Mon, 3 Feb 2025 15:13:03 -0800
Subject: [PATCH v2 2/2] Minor refactor of hash_agg_set_limits().
Avoid implicit assumption that input_groups is greater than or equal
to one. The assumption appears to be true, but it's easier to read
without it.
Also, a branch that avoided underflow while calculating a max
was hard to read; replace with ssize_t types and a Max().
---
src/backend/executor/nodeAgg.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 35cf18e5282..fb467ec9877 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1809,8 +1809,8 @@ hash_agg_set_limits(double hashentrysize, double input_groups, int used_bits,
int *num_partitions)
{
int npartitions;
- Size partition_mem;
- Size hash_mem_limit = get_hash_memory_limit();
+ ssize_t partition_mem;
+ ssize_t hash_mem_limit = get_hash_memory_limit();
/* if not expected to spill, use all of hash_mem */
if (input_groups * hashentrysize <= hash_mem_limit)
@@ -1818,7 +1818,7 @@ hash_agg_set_limits(double hashentrysize, double input_groups, int used_bits,
if (num_partitions != NULL)
*num_partitions = 0;
*mem_limit = hash_mem_limit;
- *ngroups_limit = hash_mem_limit / hashentrysize;
+ *ngroups_limit = Max(*mem_limit / hashentrysize, 1);
return;
}
@@ -1841,17 +1841,11 @@ hash_agg_set_limits(double hashentrysize, double input_groups, int used_bits,
/*
* Don't set the limit below 3/4 of hash_mem. In that case, we are at the
* minimum number of partitions, so we aren't going to dramatically exceed
- * work mem anyway.
+ * work mem anyway. Use ssize_t to avoid underflow during subtraction.
*/
- if (hash_mem_limit > 4 * partition_mem)
- *mem_limit = hash_mem_limit - partition_mem;
- else
- *mem_limit = hash_mem_limit * 0.75;
+ *mem_limit = Max(hash_mem_limit - partition_mem, hash_mem_limit * 0.75);
- if (*mem_limit > hashentrysize)
- *ngroups_limit = *mem_limit / hashentrysize;
- else
- *ngroups_limit = 1;
+ *ngroups_limit = Max(*mem_limit / hashentrysize, 1);
}
/*
--
2.34.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: injection points for hash aggregation
2025-02-03 20:45 injection points for hash aggregation Jeff Davis <[email protected]>
2025-02-03 23:54 ` Re: injection points for hash aggregation Jeff Davis <[email protected]>
@ 2025-02-11 19:48 ` Jeff Davis <[email protected]>
2025-02-18 03:42 ` Re: injection points for hash aggregation Michael Paquier <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Jeff Davis @ 2025-02-11 19:48 UTC (permalink / raw)
To: pgsql-hackers
On Mon, 2025-02-03 at 15:54 -0800, Jeff Davis wrote:
> Attaching
> v2.
I committed just the injection points part. It seemed simple and low-
risk.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: injection points for hash aggregation
2025-02-03 20:45 injection points for hash aggregation Jeff Davis <[email protected]>
2025-02-03 23:54 ` Re: injection points for hash aggregation Jeff Davis <[email protected]>
2025-02-11 19:48 ` Re: injection points for hash aggregation Jeff Davis <[email protected]>
@ 2025-02-18 03:42 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Michael Paquier @ 2025-02-18 03:42 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers
On Tue, Feb 11, 2025 at 11:48:03AM -0800, Jeff Davis wrote:
> I committed just the injection points part. It seemed simple and low-
> risk.
This has been committed and there was a CF entry:
https://commitfest.postgresql.org/patch/5560/
Switched it as committed now.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: injection points for hash aggregation
2025-02-03 20:45 injection points for hash aggregation Jeff Davis <[email protected]>
@ 2025-02-04 00:21 ` Michael Paquier <[email protected]>
2025-02-04 03:50 ` Re: injection points for hash aggregation Michael Paquier <[email protected]>
1 sibling, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2025-02-04 00:21 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers
On Mon, Feb 03, 2025 at 12:45:24PM -0800, Jeff Davis wrote:
> A couple questions on the injection points framework:
>
> * The patch allows forcing the partition fan-out to one. I could
> imagine forcing it to a specific value, is there a way to do that?
>
> * The injection_points extension offers the concept of a "local"
> injection point. While that makes sense for the callback function, it
> doesn't make sense when using the "local_var = 123" style, because that
> will happen regardless of the condition, right?
>
> * Callbacks are given very little context from the injection point
> itself, so it's hard for me to imagine what a callback might do other
> than logging the name of the injection point or waiting (as the
> extension implements). What else would callbacks be good for?
Being able to pass down some runtime states to a callback is something
I've drafted a patch for, as of:
https://www.postgresql.org/message-id/[email protected]
There was no real agreement if this facility was worth having or not,
because there were no use case for it. Well, perhaps until now based
on what you are saying.
If you want to force a callback to use a specific value or trigger
conditions based on that, you could add your own new callback in the
module injection_points and give it some custom data when attaching
the point, with a SQL function that uses InjectionPointAttach()'s
private_data. Then you could rely on injection_point_allowed(), for
example, to decide if a callback is run or not. So it depends on if
you want to use data known when attaching a callback or do something
based on a state given by a backend when it runs an attached callback.
Something worth mentioning. Heikki and Peter G. have mentioned to me
at the last pgconf.dev that it could be nice to allow files to hold
their own callbacks rather than have an extension do that (don't
recall there is a footprint about that on the lists, that was an
informal offline discussion). This would have the advantage to limit
the number of code-path-specific callbacks in the injection_points
module (or just limit the number of modules), with callbacks only
existing locally where they are used. IS_INJECTION_POINT_ATTACHED()
is what I see as a a simplified version of this idea.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: injection points for hash aggregation
2025-02-03 20:45 injection points for hash aggregation Jeff Davis <[email protected]>
2025-02-04 00:21 ` Re: injection points for hash aggregation Michael Paquier <[email protected]>
@ 2025-02-04 03:50 ` Michael Paquier <[email protected]>
2025-02-04 05:35 ` Re: injection points for hash aggregation Jeff Davis <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2025-02-04 03:50 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers
On Tue, Feb 04, 2025 at 09:21:49AM +0900, Michael Paquier wrote:
> Being able to pass down some runtime states to a callback is something
> I've drafted a patch for, as of:
> https://www.postgresql.org/message-id/[email protected]
Oops. I've posted an incorrect link here, I meant this one instead:
https://www.postgresql.org/message-id/[email protected]
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: injection points for hash aggregation
2025-02-03 20:45 injection points for hash aggregation Jeff Davis <[email protected]>
2025-02-04 00:21 ` Re: injection points for hash aggregation Michael Paquier <[email protected]>
2025-02-04 03:50 ` Re: injection points for hash aggregation Michael Paquier <[email protected]>
@ 2025-02-04 05:35 ` Jeff Davis <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Jeff Davis @ 2025-02-04 05:35 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers
On Tue, 2025-02-04 at 12:50 +0900, Michael Paquier wrote:
> On Tue, Feb 04, 2025 at 09:21:49AM +0900, Michael Paquier wrote:
> > Being able to pass down some runtime states to a callback is
> > something
> > I've drafted a patch for, as of:
> > https://www.postgresql.org/message-id/[email protected]
>
> Oops. I've posted an incorrect link here, I meant this one instead:
> https://www.postgresql.org/message-id/[email protected]
I replied in that thread. Thank you.
The injection point arguments are required for $SUBJECT, or at least
not yet, so we can proceed with this thread independently.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2025-02-18 03:42 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-18 16:19 [PATCH v49 1/3] Subscripting for jsonb Dmitrii Dolgov <[email protected]>
2025-02-03 20:45 injection points for hash aggregation Jeff Davis <[email protected]>
2025-02-03 23:54 ` Re: injection points for hash aggregation Jeff Davis <[email protected]>
2025-02-11 19:48 ` Re: injection points for hash aggregation Jeff Davis <[email protected]>
2025-02-18 03:42 ` Re: injection points for hash aggregation Michael Paquier <[email protected]>
2025-02-04 00:21 ` Re: injection points for hash aggregation Michael Paquier <[email protected]>
2025-02-04 03:50 ` Re: injection points for hash aggregation Michael Paquier <[email protected]>
2025-02-04 05:35 ` Re: injection points for hash aggregation Jeff Davis <[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