public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v43 1/2] Subscripting for jsonb
11+ messages / 6 participants
[nested] [flat]
* [PATCH v43 1/2] Subscripting for jsonb
@ 2020-12-18 16:19 Dmitrii Dolgov <[email protected]>
0 siblings, 0 replies; 11+ 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
---
doc/src/sgml/json.sgml | 48 ++++
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, 982 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..100d1a60f4 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,54 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
</para>
</sect2>
+ <sect2 id="jsonb-subscripting">
+ <title><type>jsonb</type> Subscripting</title>
+ <para>
+ <type>jsonb</type> data type supports array-style subscripting expressions
+ to extract or update particular elements. It's possible to use multiple
+ subscripting expressions to extract nested values. In this case, a chain of
+ subscripting expressions follows the same rules as the
+ <literal>path</literal> argument in <literal>jsonb_set</literal> function,
+ e.g. in case of arrays it is a 0-based operation or that negative integers
+ that appear in <literal>path</literal> count from the end of JSON arrays.
+ The result of subscripting expressions is always jsonb data type. An
+ example of subscripting syntax:
+<programlisting>
+-- Extract value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update value by key, note the single quotes - the assigned value
+-- needs to be of jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Select records using where clause with subscripting. Since the result of
+-- subscripting is jsonb and we basically want to compare two jsonb objects, we
+-- need to put the value in double quotes to be able to convert it to jsonb.
+SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
+</programlisting>
+
+ Subscripting for <type>jsonb</type> does not support slice expressions,
+ even if it contains an array.
+
+ In case if source <type>jsonb</type> is <literal>NULL</literal>, assignment
+ via subscripting will proceed as if it was an empty JSON object:
+<programlisting>
+-- If jsonb_field here is NULL, the result is {"a": 1}
+UPDATE table_name SET jsonb_field['a'] = '1';
+
+-- If jsonb_field here is NULL, the result is [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..306c37b5a6
--- /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 coercet 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..bb3f25ec3f 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 coercet 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
--6snw7vcvuujiswgu
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v43-0002-Filling-gaps-in-jsonb.patch"
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: run pgindent on a regular basis / scripted manner
@ 2023-01-20 09:43 Jelte Fennema <[email protected]>
2023-01-21 17:30 ` Re: run pgindent on a regular basis / scripted manner Bruce Momjian <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Jelte Fennema @ 2023-01-20 09:43 UTC (permalink / raw)
To: Magnus Hagander <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
As a not very frequent submitter, on all the patches that I submit I
keep running into this problem. I have two open patchsets for
libpq[1][2] both of which currently include the same initial "run
pgindent" patch in addition to the actual patch, just so I can
actually run it on my own patch because a9e9a9f caused formatting to
totally be off in the libpq directory. And there's lots of other
changes that pgindent wants to make, which are visible on the job that
Magnus has set up.
To me a master branch that pgindent never complains about sounds
amazing! And I personally think rejection of unindented pushes and
cfbot complaining about unindented patches would be a very good thing,
because that seems to be the only solution that could achieve that.
Having cfbot complain also doesn't sound like a crazy burden for
submitters. Many open-source projects have CI complaining if code
formatting does not pass automatic formatting tools. As long as there
is good documentation on how to install and run pgindent I don't think
it should be a big problem. A link to those docs could even be
included in the failing CI job its error message. A pre-commit hook
that submitters/committers could install would be super useful too.
Since right now I sometimes forget to run pgindent, especially since
there's no editor integration (that I know of) for pgindent.
Side-question: What's the reason why pgindent is used instead of some
more "modern" code formatter that doesn't require keeping
typedefs.list up to date for good looking output? (e.g. uncrustify or
clang-format) Because that would also allow for easy editor
integration.
[1]: https://commitfest.postgresql.org/41/3511/
[2]: https://commitfest.postgresql.org/41/3679/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: run pgindent on a regular basis / scripted manner
2023-01-20 09:43 Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
@ 2023-01-21 17:30 ` Bruce Momjian <[email protected]>
2023-01-21 20:21 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Bruce Momjian @ 2023-01-21 17:30 UTC (permalink / raw)
To: Jelte Fennema <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Fri, Jan 20, 2023 at 10:43:50AM +0100, Jelte Fennema wrote:
> Side-question: What's the reason why pgindent is used instead of some
> more "modern" code formatter that doesn't require keeping
> typedefs.list up to date for good looking output? (e.g. uncrustify or
> clang-format) Because that would also allow for easy editor
> integration.
One reason the typedef list is required is a quirk of the C syntax.
Most languages have a lexer/scanner, which tokenizes, and a parser,
which parses. The communication is usually one-way, lexer to parser.
For C, typedefs require the parser to feed new typedefs back into the
lexer:
http://calculist.blogspot.com/2009/02/c-typedef-parsing-problem.html
BSD indent doesn't have that feedback mechanism, probably because it
doesn't fully parse the C file. Therefore, we have to supply typedefs
manually, and for Postgres we pull them from debug-enabled binaries in
our buildfarm. The problem with that is you often import typedefs from
system headers, and the typedefs apply to all C files, not just the ones
were the typdefs are visible.
I don't see uncrustify or clang-format supporting typedef lists so maybe
they implemented this feedback loop. It would be good to see if we can
get either of these tools to match our formatting.
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Embrace your flaws. They make you human, rather than perfect,
which you will never be.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: run pgindent on a regular basis / scripted manner
2023-01-20 09:43 Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
2023-01-21 17:30 ` Re: run pgindent on a regular basis / scripted manner Bruce Momjian <[email protected]>
@ 2023-01-21 20:21 ` Peter Geoghegan <[email protected]>
2023-01-21 20:59 ` Re: run pgindent on a regular basis / scripted manner Tom Lane <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Peter Geoghegan @ 2023-01-21 20:21 UTC (permalink / raw)
To: Bruce Momjian <[email protected]>; +Cc: Jelte Fennema <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Sat, Jan 21, 2023 at 9:30 AM Bruce Momjian <[email protected]> wrote:
> I don't see uncrustify or clang-format supporting typedef lists so maybe
> they implemented this feedback loop. It would be good to see if we can
> get either of these tools to match our formatting.
I personally use clang-format for Postgres development work, since it
integrates nicely with my text editor, and can be configured to
produce approximately the same result as pgindent (certainly good
enough when working on a patch that's still far from a committable
state). I'm fairly sure that clang-format has access to a full AST
from the clang compiler, which is the ideal approach - at least in
theory.
In practice this approach tends to run into problems when the relevant
AST isn't available. For example, if there's code that only builds on
Windows, maybe it won't work at all (at least on my Linux system).
This doesn't really bother me currently, since I only rely on
clang-format as a first pass sort of thing. Maybe I could figure out a
better way to deal with such issues, but right now I don't have much
incentive to.
Another advantage of clang-format is that it's a known quantity. For
example there is direct support for it built into meson, with bells
and whistles such as CI support:
https://mesonbuild.com/Code-formatting.html
My guess is that moving to clang-format would require giving up some
flexibility, but getting much better integration with text editors and
tools like meson in return. It would probably make it practical to
have much stronger rules about how committed code must be indented --
rules that are practical, and can actually be enforced. That trade-off
seems likely to be worth it in my view, though it's not something that
I feel too strongly about.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: run pgindent on a regular basis / scripted manner
2023-01-20 09:43 Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
2023-01-21 17:30 ` Re: run pgindent on a regular basis / scripted manner Bruce Momjian <[email protected]>
2023-01-21 20:21 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
@ 2023-01-21 20:59 ` Tom Lane <[email protected]>
2023-01-21 22:05 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Tom Lane @ 2023-01-21 20:59 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: Bruce Momjian <[email protected]>; Jelte Fennema <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
Peter Geoghegan <[email protected]> writes:
> In practice this approach tends to run into problems when the relevant
> AST isn't available. For example, if there's code that only builds on
> Windows, maybe it won't work at all (at least on my Linux system).
Hmm, that could be a deal-breaker. It's not going to be acceptable
to have to pgindent different parts of the system on different platforms
... at least not unless we can segregate them on the file level, and
even that would have a large PITA factor.
Still, we won't know unless someone makes a serious experiment with it.
regards, tom lane
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: run pgindent on a regular basis / scripted manner
2023-01-20 09:43 Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
2023-01-21 17:30 ` Re: run pgindent on a regular basis / scripted manner Bruce Momjian <[email protected]>
2023-01-21 20:21 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-21 20:59 ` Re: run pgindent on a regular basis / scripted manner Tom Lane <[email protected]>
@ 2023-01-21 22:05 ` Peter Geoghegan <[email protected]>
2023-01-21 22:43 ` Re: run pgindent on a regular basis / scripted manner Andres Freund <[email protected]>
2023-01-22 04:08 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
0 siblings, 2 replies; 11+ messages in thread
From: Peter Geoghegan @ 2023-01-21 22:05 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Bruce Momjian <[email protected]>; Jelte Fennema <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Sat, Jan 21, 2023 at 12:59 PM Tom Lane <[email protected]> wrote:
> Hmm, that could be a deal-breaker. It's not going to be acceptable
> to have to pgindent different parts of the system on different platforms
> ... at least not unless we can segregate them on the file level, and
> even that would have a large PITA factor.
It's probably something that could be worked around. My remarks are
based on some dim memories of dealing with the tool before I arrived
at a configuration that works well enough for me. Importantly,
clang-format doesn't require you to futz around with Makefiles or
objdump or anything like that -- that's a huge plus. It doesn't seem
to impose any requirements on how I build Postgres at all (I generally
use gcc, not clang).
Even if these kinds of issues proved to be a problem for the person
tasked with running clang-format against the whole tree periodically,
they still likely wouldn't affect most of us. It's quite convenient to
use clang-format from an editor -- it can be invoked very
incrementally, against a small range of lines at a time. It's pretty
much something that I can treat like the built-in indent for my
editor. It's vastly different to the typical pgindent workflow.
> Still, we won't know unless someone makes a serious experiment with it.
There is one thing about clang-format that I find mildly infuriating:
it can indent function declarations in the way that I want it to, and
it can indent variable declarations in the way that I want it to. It
just can't do both at the same time, because they're both controlled
by AlignConsecutiveDeclarations.
Of course the way that I want to do things is (almost by definition)
the pgindent way, at least right now -- it's not necessarily about my
fixed preferences (though it can be hard to tell!). It's really not
surprising that clang-format cannot quite perfectly simulate pgindent.
How flexible can we be about stuff like that? Obviously there is no
clear answer right now.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: run pgindent on a regular basis / scripted manner
2023-01-20 09:43 Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
2023-01-21 17:30 ` Re: run pgindent on a regular basis / scripted manner Bruce Momjian <[email protected]>
2023-01-21 20:21 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-21 20:59 ` Re: run pgindent on a regular basis / scripted manner Tom Lane <[email protected]>
2023-01-21 22:05 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
@ 2023-01-21 22:43 ` Andres Freund <[email protected]>
2023-01-21 23:32 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
1 sibling, 1 reply; 11+ messages in thread
From: Andres Freund @ 2023-01-21 22:43 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: Tom Lane <[email protected]>; Bruce Momjian <[email protected]>; Jelte Fennema <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; pgsql-hackers
Hi,
On 2023-01-21 14:05:41 -0800, Peter Geoghegan wrote:
> On Sat, Jan 21, 2023 at 12:59 PM Tom Lane <[email protected]> wrote:
> > Hmm, that could be a deal-breaker. It's not going to be acceptable
> > to have to pgindent different parts of the system on different platforms
> > ... at least not unless we can segregate them on the file level, and
> > even that would have a large PITA factor.
Unless I miss something, I don't think clang-format actually does that level
of C parsing - you can't pass include paths etc, so it really can't.
> It's probably something that could be worked around. My remarks are
> based on some dim memories of dealing with the tool before I arrived
> at a configuration that works well enough for me.
Could you share your .clang-format?
> > Still, we won't know unless someone makes a serious experiment with it.
>
> There is one thing about clang-format that I find mildly infuriating:
> it can indent function declarations in the way that I want it to, and
> it can indent variable declarations in the way that I want it to. It
> just can't do both at the same time, because they're both controlled
> by AlignConsecutiveDeclarations.
>
> Of course the way that I want to do things is (almost by definition)
> the pgindent way, at least right now -- it's not necessarily about my
> fixed preferences (though it can be hard to tell!). It's really not
> surprising that clang-format cannot quite perfectly simulate pgindent.
> How flexible can we be about stuff like that? Obviously there is no
> clear answer right now.
I personally find the current indentation of variables assignment deeply
unhelpful - but changing it would be a very noisy change.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: run pgindent on a regular basis / scripted manner
2023-01-20 09:43 Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
2023-01-21 17:30 ` Re: run pgindent on a regular basis / scripted manner Bruce Momjian <[email protected]>
2023-01-21 20:21 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-21 20:59 ` Re: run pgindent on a regular basis / scripted manner Tom Lane <[email protected]>
2023-01-21 22:05 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-21 22:43 ` Re: run pgindent on a regular basis / scripted manner Andres Freund <[email protected]>
@ 2023-01-21 23:32 ` Peter Geoghegan <[email protected]>
2023-01-22 09:49 ` Re: run pgindent on a regular basis / scripted manner Andres Freund <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Peter Geoghegan @ 2023-01-21 23:32 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Bruce Momjian <[email protected]>; Jelte Fennema <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; pgsql-hackers
On Sat, Jan 21, 2023 at 2:43 PM Andres Freund <[email protected]> wrote:
> Unless I miss something, I don't think clang-format actually does that level
> of C parsing - you can't pass include paths etc, so it really can't.
It's hard to keep track of, since I also use clangd, which is
influenced by .clang-format for certain completions. It clearly does
plenty of stuff that requires an AST, since it requires a
compile_commands.json. You're the LLVM committer, not me.
Attached is my .clang-format, since you asked for it. It was
originally based on stuff that both you and Peter E posted several
years back, I believe. Plus the timescaledb one in one or two places.
I worked a couple of things out through trial and error. It's
relatively hard to follow the documentation, and there have been
features added to newer LLVM versions.
--
Peter Geoghegan
Attachments:
[application/octet-stream] clang-format (4.5K, ../../CAH2-Wzm0niYBPcN6eX3PzHABfj0dE3fwifo5bdturvMp=9FGqw@mail.gmail.com/2-clang-format)
download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: run pgindent on a regular basis / scripted manner
2023-01-20 09:43 Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
2023-01-21 17:30 ` Re: run pgindent on a regular basis / scripted manner Bruce Momjian <[email protected]>
2023-01-21 20:21 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-21 20:59 ` Re: run pgindent on a regular basis / scripted manner Tom Lane <[email protected]>
2023-01-21 22:05 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-21 22:43 ` Re: run pgindent on a regular basis / scripted manner Andres Freund <[email protected]>
2023-01-21 23:32 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
@ 2023-01-22 09:49 ` Andres Freund <[email protected]>
2023-01-22 17:20 ` Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Andres Freund @ 2023-01-22 09:49 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: Tom Lane <[email protected]>; Bruce Momjian <[email protected]>; Jelte Fennema <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; pgsql-hackers
Hi,
On 2023-01-21 15:32:45 -0800, Peter Geoghegan wrote:
> Attached is my .clang-format, since you asked for it. It was
> originally based on stuff that both you and Peter E posted several
> years back, I believe. Plus the timescaledb one in one or two places.
> I worked a couple of things out through trial and error. It's
> relatively hard to follow the documentation, and there have been
> features added to newer LLVM versions.
Reformatting with your clang-format end up with something like:
Peter's:
2234 files changed, 334753 insertions(+), 289772 deletions(-)
Jelte's:
2236 files changed, 357500 insertions(+), 306815 deletions(-)
Mine (modified to reduce this):
2226 files changed, 261538 insertions(+), 256039 deletions(-)
Which is all at least an order of magnitude too much.
Jelte's uncrustify:
1773 files changed, 121722 insertions(+), 125369 deletions(-)
better, but still not great. Also had to prevent a file files it choked on
from getting reindented.
I think the main issue with either is that our variable definition indentation
just can't be emulated by the tools as-is.
Some tools can indent variable definitions so that the variable name starts on
the same column. Some can limit that for too long type names. But so far I
haven't seen one that cn make that column be column +12. They all look to
other surrounding types.
I hate that variable name indentation with a fiery passion. But switching away
from that intermixed with a lot of other changes isn't going to be fun.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: run pgindent on a regular basis / scripted manner
2023-01-20 09:43 Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
2023-01-21 17:30 ` Re: run pgindent on a regular basis / scripted manner Bruce Momjian <[email protected]>
2023-01-21 20:21 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-21 20:59 ` Re: run pgindent on a regular basis / scripted manner Tom Lane <[email protected]>
2023-01-21 22:05 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-21 22:43 ` Re: run pgindent on a regular basis / scripted manner Andres Freund <[email protected]>
2023-01-21 23:32 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-22 09:49 ` Re: run pgindent on a regular basis / scripted manner Andres Freund <[email protected]>
@ 2023-01-22 17:20 ` Jelte Fennema <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Jelte Fennema @ 2023-01-22 17:20 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>; Bruce Momjian <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; pgsql-hackers
> But so far I haven't seen one that can make that
> column be column +12.
Thanks for clarifying what the current variable declaration indention
rule is. Indeed neither uncrustify or clang-format seem to support
that. Getting uncrustify to support it might not be too difficult, but
the question remains if we even want that.
> But switching away from that intermixed with a lot of other changes isn't going to be fun.
I don't think the amount of pain is really much lower if we reformat
10,000 or 300,000 lines of code, without automation both would be
quite painful. But the git commands I shared in my previous email
should alleviate most of that pain.
> I don't have a problem with the current pgindent alignment of function
> parameters, so not sure what you mean about that.
Function parameter alignment is fine with pgindent imho, but this +12
column variable declaration thing I personally think is quite weird.
> Really? I have been using 14, which is quite recent. Did you just
> figure this out recently? If this is true, then it's certainly
> discouraging.
It seems this was due to my Ubuntu 22.04 install having clang-format
14.0.0. After
updating it to 14.0.6 by using the official llvm provided packages, I
don't have this
issue on clang-format-14 anymore. To be clear this was an issue in alignment of
variable declarations not function parameters.
But I agree with Tom Lane that this makes clear that whatever tool we
pick we'll need
to pick a specific version, just like we do now with perltidy. And
indeed I'm not sure
how easy that is with clang. Installing a specific uncrustify version
is pretty easy btw,
the compilation from source is quite quick.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: run pgindent on a regular basis / scripted manner
2023-01-20 09:43 Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
2023-01-21 17:30 ` Re: run pgindent on a regular basis / scripted manner Bruce Momjian <[email protected]>
2023-01-21 20:21 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-21 20:59 ` Re: run pgindent on a regular basis / scripted manner Tom Lane <[email protected]>
2023-01-21 22:05 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
@ 2023-01-22 04:08 ` Peter Geoghegan <[email protected]>
1 sibling, 0 replies; 11+ messages in thread
From: Peter Geoghegan @ 2023-01-22 04:08 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Bruce Momjian <[email protected]>; Jelte Fennema <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Sat, Jan 21, 2023 at 2:05 PM Peter Geoghegan <[email protected]> wrote:
> There is one thing about clang-format that I find mildly infuriating:
> it can indent function declarations in the way that I want it to, and
> it can indent variable declarations in the way that I want it to. It
> just can't do both at the same time, because they're both controlled
> by AlignConsecutiveDeclarations.
Looks like I'm not the only one that doesn't like this behavior:
https://github.com/llvm/llvm-project/issues/55605
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2023-01-22 17:20 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-18 16:19 [PATCH v43 1/2] Subscripting for jsonb Dmitrii Dolgov <[email protected]>
2023-01-20 09:43 Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
2023-01-21 17:30 ` Re: run pgindent on a regular basis / scripted manner Bruce Momjian <[email protected]>
2023-01-21 20:21 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-21 20:59 ` Re: run pgindent on a regular basis / scripted manner Tom Lane <[email protected]>
2023-01-21 22:05 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-21 22:43 ` Re: run pgindent on a regular basis / scripted manner Andres Freund <[email protected]>
2023-01-21 23:32 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[email protected]>
2023-01-22 09:49 ` Re: run pgindent on a regular basis / scripted manner Andres Freund <[email protected]>
2023-01-22 17:20 ` Re: run pgindent on a regular basis / scripted manner Jelte Fennema <[email protected]>
2023-01-22 04:08 ` Re: run pgindent on a regular basis / scripted manner Peter Geoghegan <[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