public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v42 1/2] Subscripting for jsonb 6+ messages / 4 participants [nested] [flat]
* [PATCH v42 1/2] Subscripting for jsonb @ 2020-12-18 16:19 Dmitrii Dolgov <[email protected]> 0 siblings, 0 replies; 6+ 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 --2lozexjxkmbehzdg Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v42-0002-Filling-gaps-in-jsonb.patch" ^ permalink raw reply [nested|flat] 6+ messages in thread
* Make --help output fit within 80 columns per line @ 2023-07-05 01:47 torikoshia <[email protected]> 2023-08-21 04:08 ` Re: Make --help output fit within 80 columns per line Masahiro Ikeda <[email protected]> 2023-09-13 17:46 ` Re: Make --help output fit within 80 columns per line Greg Sabino Mullane <[email protected]> 0 siblings, 2 replies; 6+ messages in thread From: torikoshia @ 2023-07-05 01:47 UTC (permalink / raw) To: pgsql-hackers Hi, As discussed in [1], outputs of --help for some commands fits into 80 columns per line, while others do not. Since it seems preferable to have consistent line break policy and some people use 80-column terminal, wouldn't it be better to make all commands in 80 columns per line? Attached patch which does this for src/bin commands. If this is the way to go, I'll do same things for contrib commands. [1] https://www.postgresql.org/message-id/3fe4af5a0a81fc6a2ec01cb484c0a487%40oss.nttdata.com -- Regards, -- Atsushi Torikoshi NTT DATA CORPORATION Attachments: [text/x-diff] v1-0001-Make-help-output-fit-within-80-columns-per-line.patch (24.6K, ../../[email protected]/2-v1-0001-Make-help-output-fit-within-80-columns-per-line.patch) download | inline diff: From 4505bbcf0efe199e34159696d64bd9f8cc60fb37 Mon Sep 17 00:00:00 2001 From: Atsushi Torikoshi <[email protected]> Date: Wed, 5 Jul 2023 10:14:58 +0900 Subject: [PATCH v1] Make --help output fit within 80 columns per line Outputs of --help for some commands fits into 80 columns per line, while others do not. For the consistency and for 80-column terminal, this patch makes them fit within 80 columns per line. --- src/bin/initdb/initdb.c | 6 ++++-- src/bin/pg_amcheck/pg_amcheck.c | 21 ++++++++++++------- src/bin/pg_archivecleanup/pg_archivecleanup.c | 2 +- src/bin/pg_basebackup/pg_receivewal.c | 12 +++++++---- src/bin/pg_basebackup/pg_recvlogical.c | 18 ++++++++++------ src/bin/pg_checksums/pg_checksums.c | 3 ++- src/bin/pg_dump/pg_dump.c | 3 ++- src/bin/pg_dump/pg_dumpall.c | 3 ++- src/bin/pg_dump/pg_restore.c | 6 ++++-- src/bin/pg_upgrade/option.c | 9 +++++--- src/bin/pgbench/pgbench.c | 6 ++++-- src/bin/psql/help.c | 9 +++++--- src/bin/scripts/createdb.c | 3 ++- src/bin/scripts/pg_isready.c | 3 ++- src/bin/scripts/vacuumdb.c | 21 ++++++++++++------- 15 files changed, 83 insertions(+), 42 deletions(-) diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index fc1fb363e7..ccdd093e24 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -2429,8 +2429,10 @@ usage(const char *progname) printf(_(" %s [OPTION]... [DATADIR]\n"), progname); printf(_("\nOptions:\n")); printf(_(" -A, --auth=METHOD default authentication method for local connections\n")); - printf(_(" --auth-host=METHOD default authentication method for local TCP/IP connections\n")); - printf(_(" --auth-local=METHOD default authentication method for local-socket connections\n")); + printf(_(" --auth-host=METHOD default authentication method for local TCP/IP\n" + " connections\n")); + printf(_(" --auth-local=METHOD default authentication method for local-socket\n" + " connections\n")); printf(_(" [-D, --pgdata=]DATADIR location for this database cluster\n")); printf(_(" -E, --encoding=ENCODING set default encoding for new databases\n")); printf(_(" -g, --allow-group-access allow group read/execute on data directory\n")); diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c index 68f8180c19..754a2723ce 100644 --- a/src/bin/pg_amcheck/pg_amcheck.c +++ b/src/bin/pg_amcheck/pg_amcheck.c @@ -1150,17 +1150,23 @@ help(const char *progname) printf(_(" -S, --exclude-schema=PATTERN do NOT check matching schema(s)\n")); printf(_(" -t, --table=PATTERN check matching table(s)\n")); printf(_(" -T, --exclude-table=PATTERN do NOT check matching table(s)\n")); - printf(_(" --no-dependent-indexes do NOT expand list of relations to include indexes\n")); - printf(_(" --no-dependent-toast do NOT expand list of relations to include TOAST tables\n")); + printf(_(" --no-dependent-indexes do NOT expand list of relations to include\n" + " indexes\n")); + printf(_(" --no-dependent-toast do NOT expand list of relations to include\n" + " TOAST tables\n")); printf(_(" --no-strict-names do NOT require patterns to match objects\n")); printf(_("\nTable checking options:\n")); printf(_(" --exclude-toast-pointers do NOT follow relation TOAST pointers\n")); printf(_(" --on-error-stop stop checking at end of first corrupt page\n")); - printf(_(" --skip=OPTION do NOT check \"all-frozen\" or \"all-visible\" blocks\n")); - printf(_(" --startblock=BLOCK begin checking table(s) at the given block number\n")); - printf(_(" --endblock=BLOCK check table(s) only up to the given block number\n")); + printf(_(" --skip=OPTION do NOT check \"all-frozen\" or \"all-visible\"\n" + " blocks\n")); + printf(_(" --startblock=BLOCK begin checking table(s) at the given block\n" + " number\n")); + printf(_(" --endblock=BLOCK check table(s) only up to the given block\n" + " number\n")); printf(_("\nB-tree index checking options:\n")); - printf(_(" --heapallindexed check that all heap tuples are found within indexes\n")); + printf(_(" --heapallindexed check that all heap tuples are found within\n" + " indexes\n")); printf(_(" --parent-check check index parent/child relationships\n")); printf(_(" --rootdescend search from root page to refind tuples\n")); printf(_("\nConnection options:\n")); @@ -1172,7 +1178,8 @@ help(const char *progname) printf(_(" --maintenance-db=DBNAME alternate maintenance database\n")); printf(_("\nOther options:\n")); printf(_(" -e, --echo show the commands being sent to the server\n")); - printf(_(" -j, --jobs=NUM use this many concurrent connections to the server\n")); + printf(_(" -j, --jobs=NUM use this many concurrent connections to the\n" + " server\n")); printf(_(" -P, --progress show progress information\n")); printf(_(" -v, --verbose write a lot of output\n")); printf(_(" -V, --version output version information, then exit\n")); diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c index fc0dca9856..52b29e5ca7 100644 --- a/src/bin/pg_archivecleanup/pg_archivecleanup.c +++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c @@ -267,7 +267,7 @@ usage(void) printf(_("\n" "Or for use as a standalone archive cleaner:\n" "e.g.\n" - " pg_archivecleanup /mnt/server/archiverdir 000000010000000000000010.00000020.backup\n")); + " pg_archivecleanup archiverdir 000000010000000000000010.00000020.backup\n")); printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT); printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL); } diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c index d0a4079d50..39a40d525e 100644 --- a/src/bin/pg_basebackup/pg_receivewal.c +++ b/src/bin/pg_basebackup/pg_receivewal.c @@ -81,11 +81,13 @@ usage(void) printf(_("\nOptions:\n")); printf(_(" -D, --directory=DIR receive write-ahead log files into this directory\n")); printf(_(" -E, --endpos=LSN exit after receiving the specified LSN\n")); - printf(_(" --if-not-exists do not error if slot already exists when creating a slot\n")); + printf(_(" --if-not-exists do not error if slot already exists when creating a\n" + " slot\n")); printf(_(" -n, --no-loop do not loop on connection lost\n")); printf(_(" --no-sync do not wait for changes to be written safely to disk\n")); printf(_(" -s, --status-interval=SECS\n" - " time between status packets sent to server (default: %d)\n"), (standby_message_timeout / 1000)); + " time between status packets sent to server\n" + " (default: %d)\n"), (standby_message_timeout / 1000)); printf(_(" -S, --slot=SLOTNAME replication slot to use\n")); printf(_(" --synchronous flush write-ahead log immediately after writing\n")); printf(_(" -v, --verbose output verbose messages\n")); @@ -101,8 +103,10 @@ usage(void) printf(_(" -w, --no-password never prompt for password\n")); printf(_(" -W, --password force password prompt (should happen automatically)\n")); printf(_("\nOptional actions:\n")); - printf(_(" --create-slot create a new replication slot (for the slot's name see --slot)\n")); - printf(_(" --drop-slot drop the replication slot (for the slot's name see --slot)\n")); + printf(_(" --create-slot create a new replication slot (for the slot's name see\n" + " --slot)\n")); + printf(_(" --drop-slot drop the replication slot (for the slot's name see\n" + " --slot)\n")); printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT); printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL); } diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index f3c7937a1d..33e1110da6 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -76,15 +76,19 @@ usage(void) printf(_("Usage:\n")); printf(_(" %s [OPTION]...\n"), progname); printf(_("\nAction to be performed:\n")); - printf(_(" --create-slot create a new replication slot (for the slot's name see --slot)\n")); - printf(_(" --drop-slot drop the replication slot (for the slot's name see --slot)\n")); - printf(_(" --start start streaming in a replication slot (for the slot's name see --slot)\n")); + printf(_(" --create-slot create a new replication slot (for the slot's name see\n" + " --slot)\n")); + printf(_(" --drop-slot drop the replication slot (for the slot's name see\n" + " --slot)\n")); + printf(_(" --start start streaming in a replication slot (for the slot's\n" + " name see --slot)\n")); printf(_("\nOptions:\n")); printf(_(" -E, --endpos=LSN exit after receiving the specified LSN\n")); printf(_(" -f, --file=FILE receive log into this file, - for stdout\n")); printf(_(" -F --fsync-interval=SECS\n" " time between fsyncs to the output file (default: %d)\n"), (fsync_interval / 1000)); - printf(_(" --if-not-exists do not error if slot already exists when creating a slot\n")); + printf(_(" --if-not-exists do not error if slot already exists when creating a\n" + " slot\n")); printf(_(" -I, --startpos=LSN where in an existing slot should the streaming start\n")); printf(_(" -n, --no-loop do not loop on connection lost\n")); printf(_(" -o, --option=NAME[=VALUE]\n" @@ -92,9 +96,11 @@ usage(void) " output plugin\n")); printf(_(" -P, --plugin=PLUGIN use output plugin PLUGIN (default: %s)\n"), plugin); printf(_(" -s, --status-interval=SECS\n" - " time between status packets sent to server (default: %d)\n"), (standby_message_timeout / 1000)); + " time between status packets sent to server\n" + " (default: %d)\n"), (standby_message_timeout / 1000)); printf(_(" -S, --slot=SLOTNAME name of the logical replication slot\n")); - printf(_(" -t, --two-phase enable decoding of prepared transactions when creating a slot\n")); + printf(_(" -t, --two-phase enable decoding of prepared transactions when creating\n" + " a slot\n")); printf(_(" -v, --verbose output verbose messages\n")); printf(_(" -V, --version output version information, then exit\n")); printf(_(" -?, --help show this help, then exit\n")); diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index 19eb67e485..2b42ab7e0a 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -76,7 +76,8 @@ static pg_time_t last_progress_report = 0; static void usage(void) { - printf(_("%s enables, disables, or verifies data checksums in a PostgreSQL database cluster.\n\n"), progname); + printf(_("%s enables, disables, or verifies data checksums in a PostgreSQL\n" + "database cluster.\n\n"), progname); printf(_("Usage:\n")); printf(_(" %s [OPTION]... [DATADIR]\n"), progname); printf(_("\nOptions:\n")); diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 5dab1ba9ea..44156ee85e 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -1067,7 +1067,8 @@ help(const char *progname) printf(_(" -Z, --compress=METHOD[:DETAIL]\n" " compress as specified\n")); printf(_(" --lock-wait-timeout=TIMEOUT fail after waiting TIMEOUT for a table lock\n")); - printf(_(" --no-sync do not wait for changes to be written safely to disk\n")); + printf(_(" --no-sync do not wait for changes to be written safely to\n" + " disk\n")); printf(_(" -?, --help show this help, then exit\n")); printf(_("\nOptions controlling the output content:\n")); diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index 3627b69e2a..76653d6ec8 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -661,7 +661,8 @@ help(void) printf(_(" --no-role-passwords do not dump passwords for roles\n")); printf(_(" --no-security-labels do not dump security label assignments\n")); printf(_(" --no-subscriptions do not dump subscriptions\n")); - printf(_(" --no-sync do not wait for changes to be written safely to disk\n")); + printf(_(" --no-sync do not wait for changes to be written safely to\n" + " disk\n")); printf(_(" --no-table-access-method do not dump table access methods\n")); printf(_(" --no-tablespaces do not dump tablespace assignments\n")); printf(_(" --no-toast-compression do not dump TOAST compression methods\n")); diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c index 049a100634..fbbb9f93f8 100644 --- a/src/bin/pg_dump/pg_restore.c +++ b/src/bin/pg_dump/pg_restore.c @@ -459,7 +459,8 @@ usage(const char *progname) printf(_(" -S, --superuser=NAME superuser user name to use for disabling triggers\n")); printf(_(" -t, --table=NAME restore named relation (table, view, etc.)\n")); printf(_(" -T, --trigger=NAME restore named trigger\n")); - printf(_(" -x, --no-privileges skip restoration of access privileges (grant/revoke)\n")); + printf(_(" -x, --no-privileges skip restoration of access privileges\n" + " (grant/revoke)\n")); printf(_(" -1, --single-transaction restore as a single transaction\n")); printf(_(" --disable-triggers disable triggers during data-only restore\n")); printf(_(" --enable-row-security enable row security\n")); @@ -472,7 +473,8 @@ usage(const char *progname) printf(_(" --no-subscriptions do not restore subscriptions\n")); printf(_(" --no-table-access-method do not restore table access methods\n")); printf(_(" --no-tablespaces do not restore tablespace assignments\n")); - printf(_(" --section=SECTION restore named section (pre-data, data, or post-data)\n")); + printf(_(" --section=SECTION restore named section (pre-data, data, or\n" + " post-data)\n")); printf(_(" --strict-names require table and/or schema include patterns to\n" " match at least one entity each\n")); printf(_(" --use-set-session-authorization\n" diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index 640361009e..58c86309a1 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -275,9 +275,11 @@ usage(void) printf(_(" -c, --check check clusters only, don't change any data\n")); printf(_(" -d, --old-datadir=DATADIR old cluster data directory\n")); printf(_(" -D, --new-datadir=DATADIR new cluster data directory\n")); - printf(_(" -j, --jobs=NUM number of simultaneous processes or threads to use\n")); + printf(_(" -j, --jobs=NUM number of simultaneous processes or threads to\n" + " use\n")); printf(_(" -k, --link link instead of copying files to new cluster\n")); - printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n")); + printf(_(" -N, --no-sync do not wait for changes to be written safely to\n" + " disk\n")); printf(_(" -o, --old-options=OPTIONS old cluster options to pass to the server\n")); printf(_(" -O, --new-options=OPTIONS new cluster options to pass to the server\n")); printf(_(" -p, --old-port=PORT old cluster port number (default %d)\n"), old_cluster.port); @@ -303,7 +305,8 @@ usage(void) " the \"bin\" directory for the new version (-B BINDIR)\n")); printf(_("\n" "For example:\n" - " pg_upgrade -d oldCluster/data -D newCluster/data -b oldCluster/bin -B newCluster/bin\n" + " pg_upgrade -d oldCluster/data -D newCluster/data\n" + " -b oldCluster/bin -B newCluster/bin\n" "or\n")); #ifndef WIN32 printf(_(" $ export PGDATAOLD=oldCluster/data\n" diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 1d1670d4c2..611ae77967 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -888,8 +888,10 @@ usage(void) " --index-tablespace=TABLESPACE\n" " create indexes in the specified tablespace\n" " --partition-method=(range|hash)\n" - " partition pgbench_accounts with this method (default: range)\n" - " --partitions=NUM partition pgbench_accounts into NUM parts (default: 0)\n" + " partition pgbench_accounts with this method\n" + " (default: range)\n" + " --partitions=NUM partition pgbench_accounts into NUM parts\n" + " (default: 0)\n" " --tablespace=TABLESPACE create tables in the specified tablespace\n" " --unlogged-tables create tables as unlogged tables\n" "\nOptions to select what to run:\n" diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c index 0ff595e7ee..48ba9e5617 100644 --- a/src/bin/psql/help.c +++ b/src/bin/psql/help.c @@ -118,19 +118,22 @@ usage(unsigned short int pager) HELP0(" -H, --html HTML table output mode\n"); HELP0(" -P, --pset=VAR[=ARG] set printing option VAR to ARG (see \\pset command)\n"); HELP0(" -R, --record-separator=STRING\n" - " record separator for unaligned output (default: newline)\n"); + " record separator for unaligned output\n" + " (default: newline)\n"); HELP0(" -t, --tuples-only print rows only\n"); HELP0(" -T, --table-attr=TEXT set HTML table tag attributes (e.g., width, border)\n"); HELP0(" -x, --expanded turn on expanded table output\n"); HELP0(" -z, --field-separator-zero\n" " set field separator for unaligned output to zero byte\n"); HELP0(" -0, --record-separator-zero\n" - " set record separator for unaligned output to zero byte\n"); + " set record separator for unaligned output to zero\n" + " byte\n"); HELP0("\nConnection options:\n"); /* Display default host */ env = getenv("PGHOST"); - HELPN(" -h, --host=HOSTNAME database server host or socket directory (default: \"%s\")\n", + HELPN(" -h, --host=HOSTNAME database server host or socket directory\n" + " (default: \"%s\")\n", env ? env : _("local socket")); /* Display default port */ env = getenv("PGPORT"); diff --git a/src/bin/scripts/createdb.c b/src/bin/scripts/createdb.c index 9ca86a3e53..fa1f6d95e5 100644 --- a/src/bin/scripts/createdb.c +++ b/src/bin/scripts/createdb.c @@ -297,7 +297,8 @@ help(const char *progname) printf(_(" --icu-locale=LOCALE ICU locale setting for the database\n")); printf(_(" --icu-rules=RULES ICU rules setting for the database\n")); printf(_(" --locale-provider={libc|icu}\n" - " locale provider for the database's default collation\n")); + " locale provider for the database's default\n" + " collation\n")); printf(_(" -O, --owner=OWNER database user to own the new database\n")); printf(_(" -S, --strategy=STRATEGY database creation strategy wal_log or file_copy\n")); printf(_(" -T, --template=TEMPLATE template database to copy\n")); diff --git a/src/bin/scripts/pg_isready.c b/src/bin/scripts/pg_isready.c index 64bbffb0b2..a5e617ba1b 100644 --- a/src/bin/scripts/pg_isready.c +++ b/src/bin/scripts/pg_isready.c @@ -235,7 +235,8 @@ help(const char *progname) printf(_("\nConnection options:\n")); printf(_(" -h, --host=HOSTNAME database server host or socket directory\n")); printf(_(" -p, --port=PORT database server port\n")); - printf(_(" -t, --timeout=SECS seconds to wait when attempting connection, 0 disables (default: %s)\n"), DEFAULT_CONNECT_TIMEOUT); + printf(_(" -t, --timeout=SECS seconds to wait when attempting connection,\n" + " 0 disables (default: %s)\n"), DEFAULT_CONNECT_TIMEOUT); printf(_(" -U, --username=USERNAME user name to connect as\n")); printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT); printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL); diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c index 4b17a07089..7fffdad17b 100644 --- a/src/bin/scripts/vacuumdb.c +++ b/src/bin/scripts/vacuumdb.c @@ -1134,19 +1134,26 @@ help(const char *progname) printf(_(" -e, --echo show the commands being sent to the server\n")); printf(_(" -f, --full do full vacuuming\n")); printf(_(" -F, --freeze freeze row transaction information\n")); - printf(_(" --force-index-cleanup always remove index entries that point to dead tuples\n")); + printf(_(" --force-index-cleanup always remove index entries that point to dead\n" + " tuples\n")); printf(_(" -j, --jobs=NUM use this many concurrent connections to vacuum\n")); printf(_(" --min-mxid-age=MXID_AGE minimum multixact ID age of tables to vacuum\n")); printf(_(" --min-xid-age=XID_AGE minimum transaction ID age of tables to vacuum\n")); - printf(_(" --no-index-cleanup don't remove index entries that point to dead tuples\n")); + printf(_(" --no-index-cleanup don't remove index entries that point to dead\n" + " tuples\n")); printf(_(" --no-process-main skip the main relation\n")); - printf(_(" --no-process-toast skip the TOAST table associated with the table to vacuum\n")); - printf(_(" --no-truncate don't truncate empty pages at the end of the table\n")); + printf(_(" --no-process-toast skip the TOAST table associated with the table\n" + " to vacuum\n")); + printf(_(" --no-truncate don't truncate empty pages at the end of the\n" + " table\n")); printf(_(" -n, --schema=PATTERN vacuum tables in the specified schema(s) only\n")); - printf(_(" -N, --exclude-schema=PATTERN do not vacuum tables in the specified schema(s)\n")); - printf(_(" -P, --parallel=PARALLEL_WORKERS use this many background workers for vacuum, if available\n")); + printf(_(" -N, --exclude-schema=PATTERN do not vacuum tables in the specified\n" + " schema(s)\n")); + printf(_(" -P, --parallel=PARALLEL_WORKERS use this many background workers for vacuum,\n" + " if available\n")); printf(_(" -q, --quiet don't write any messages\n")); - printf(_(" --skip-locked skip relations that cannot be immediately locked\n")); + printf(_(" --skip-locked skip relations that cannot be immediately\n" + " locked\n")); printf(_(" -t, --table='TABLE[(COLUMNS)]' vacuum specific table(s) only\n")); printf(_(" -v, --verbose write a lot of output\n")); printf(_(" -V, --version output version information, then exit\n")); -- 2.39.2 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Make --help output fit within 80 columns per line 2023-07-05 01:47 Make --help output fit within 80 columns per line torikoshia <[email protected]> @ 2023-08-21 04:08 ` Masahiro Ikeda <[email protected]> 2023-08-22 13:57 ` Re: Make --help output fit within 80 columns per line torikoshia <[email protected]> 1 sibling, 1 reply; 6+ messages in thread From: Masahiro Ikeda @ 2023-08-21 04:08 UTC (permalink / raw) To: torikoshia <[email protected]>; +Cc: pgsql-hackers On 2023-07-05 10:47, torikoshia wrote: > Hi, > > As discussed in [1], outputs of --help for some commands fits into 80 > columns > per line, while others do not. > > Since it seems preferable to have consistent line break policy and some > people > use 80-column terminal, wouldn't it be better to make all commands in > 80 > columns per line? > > Attached patch which does this for src/bin commands. > > If this is the way to go, I'll do same things for contrib commands. > > [1] > https://www.postgresql.org/message-id/3fe4af5a0a81fc6a2ec01cb484c0a487%40oss.nttdata.com Thanks for making the patches! I have some comments to v1 patch. (1) Why don't you add test for the purpose? It could be overkill... I though the following function is the best place. diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm index 617caa022f..1bdb81ac56 100644 --- a/src/test/perl/PostgreSQL/Test/Utils.pm +++ b/src/test/perl/PostgreSQL/Test/Utils.pm @@ -843,6 +843,10 @@ sub program_help_ok ok($result, "$cmd --help exit code 0"); isnt($stdout, '', "$cmd --help goes to stdout"); is($stderr, '', "$cmd --help nothing to stderr"); + foreach my $line (split /\n/, $stdout) + { + ok(length($line) <= 80, "$cmd --help output fit within 80 columns per line"); + } return; } (2) Is there any reason that only src/bin commands are targeted? I found that we also need to fix vacuumlo with the above test. I think it's better to fix it because it's a contrib module. $ vacuumlo --help | while IFS='' read line; do echo $((`echo $line | wc -m` - 1)) $line; done | sort -n -r | head -n 2 84 -n, --dry-run don't remove large objects, just show what would be done 74 -l, --limit=LIMIT commit after removing each LIMIT large objects (3) Is to delete '/mnt/server' intended? I though it better to leave it as is since archive_cleanup_command example uses the absolute path. - " pg_archivecleanup /mnt/server/archiverdir 000000010000000000000010.00000020.backup\n")); + " pg_archivecleanup archiverdir 000000010000000000000010.00000020.backup\n")); I will confirmed that the --help text are not changed and only the line breaks are changed. But, currently the above change break it. (4) I found that some binaries, for example ecpg, are not tested with program_help_ok(). Is it better to add tests in the patch? BTW, I check the difference with the following commands # files include "--help" $ find -name "*.c" | xargs -I {} sh -c 'if [ `grep -e --help {} | wc -l` -gt 0 ]; then echo {}; fi' # programs which is tested with program_help_ok $ find -name "*.pl" | xargs -I {} sh -c 'grep -e program_help_ok {}' Regards, -- Masahiro Ikeda NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Make --help output fit within 80 columns per line 2023-07-05 01:47 Make --help output fit within 80 columns per line torikoshia <[email protected]> 2023-08-21 04:08 ` Re: Make --help output fit within 80 columns per line Masahiro Ikeda <[email protected]> @ 2023-08-22 13:57 ` torikoshia <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: torikoshia @ 2023-08-22 13:57 UTC (permalink / raw) To: Masahiro Ikeda <[email protected]>; +Cc: pgsql-hackers On 2023-08-21 13:08, Masahiro Ikeda wrote: Thanks for your review! > (1) > > Why don't you add test for the purpose? It could be overkill... > I though the following function is the best place. > > diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm > b/src/test/perl/PostgreSQL/Test/Utils.pm > index 617caa022f..1bdb81ac56 100644 > --- a/src/test/perl/PostgreSQL/Test/Utils.pm > +++ b/src/test/perl/PostgreSQL/Test/Utils.pm > @@ -843,6 +843,10 @@ sub program_help_ok > ok($result, "$cmd --help exit code 0"); > isnt($stdout, '', "$cmd --help goes to stdout"); > is($stderr, '', "$cmd --help nothing to stderr"); > + foreach my $line (split /\n/, $stdout) > + { > + ok(length($line) <= 80, "$cmd --help output fit within > 80 columns per line"); > + } > return; > } Agreed. > (2) > > Is there any reason that only src/bin commands are targeted? I found > that > we also need to fix vacuumlo with the above test. I think it's better > to > fix it because it's a contrib module. > > $ vacuumlo --help | while IFS='' read line; do echo $((`echo $line | > wc -m` - 1)) $line; done | sort -n -r | head -n 2 > 84 -n, --dry-run don't remove large objects, just show > what would be done > 74 -l, --limit=LIMIT commit after removing each LIMIT large > objects This is because I wasn't sure making all --help outputs fit into 80 columns per line is right thing to do as described below: | If this is the way to go, I'll do same things for contrib commands. If there are no objection, I'm going to make other commands fit within 80 columns per line including (4). > (3) > > Is to delete '/mnt/server' intended? I though it better to leave it as > is since archive_cleanup_command example uses the absolute path. > > - " pg_archivecleanup /mnt/server/archiverdir > 000000010000000000000010.00000020.backup\n")); > + " pg_archivecleanup archiverdir > 000000010000000000000010.00000020.backup\n")); > > I will confirmed that the --help text are not changed and only > the line breaks are changed. But, currently the above change > break it. Yes, it is intended as described in the thread. https://www.postgresql.org/message-id/20230615.152036.1556630042388070221.horikyota.ntt%40gmail.com | We could shorten it by removing the "/mnt/server" portion, but I'm not sure if it's worth doing. However, I feel it is acceptable to make an exception and exceed 80 characters for this line. > (4) > > I found that some binaries, for example ecpg, are not tested with > program_help_ok(). Is it better to add tests in the patch? > Agreed. > BTW, I check the difference with the following commands > # files include "--help" > $ find -name "*.c" | xargs -I {} sh -c 'if [ `grep -e --help {} | wc > -l` -gt 0 ]; then echo {}; fi' > > # programs which is tested with program_help_ok > $ find -name "*.pl" | xargs -I {} sh -c 'grep -e program_help_ok {}' Thanks for sharing your procedure! -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Make --help output fit within 80 columns per line 2023-07-05 01:47 Make --help output fit within 80 columns per line torikoshia <[email protected]> @ 2023-09-13 17:46 ` Greg Sabino Mullane <[email protected]> 2023-09-15 15:10 ` Re: Make --help output fit within 80 columns per line torikoshia <[email protected]> 1 sibling, 1 reply; 6+ messages in thread From: Greg Sabino Mullane @ 2023-09-13 17:46 UTC (permalink / raw) To: torikoshia <[email protected]>; +Cc: pgsql-hackers On Tue, Jul 4, 2023 at 9:47 PM torikoshia <[email protected]> wrote: > Since it seems preferable to have consistent line break policy and some > people use 80-column terminal, wouldn't it be better to make all commands > in 80 > columns per line? > All this seems an awful lot of work to support this mythical 80-column terminal user. It's 2023, perhaps it's time to widen the default assumption past 80 characters? Cheers, Greg ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Make --help output fit within 80 columns per line 2023-07-05 01:47 Make --help output fit within 80 columns per line torikoshia <[email protected]> 2023-09-13 17:46 ` Re: Make --help output fit within 80 columns per line Greg Sabino Mullane <[email protected]> @ 2023-09-15 15:10 ` torikoshia <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: torikoshia @ 2023-09-15 15:10 UTC (permalink / raw) To: Greg Sabino Mullane <[email protected]>; [email protected]; +Cc: pgsql-hackers On 2023-09-12 15:27, Peter Eisentraut wrote: > Also, it would be very useful if the TAP test function could print out > the violating lines if a test fails. (Similar to how is() and like() > print the failing values.) Attached patch for this. Below are the the outputs when test failed: ``` $ cd contrib/vacuumlo $ make check ...(snip)... t/001_basic.pl .. 1/? # Failed test ' -n, --dry-run don't remove large objects, just show what would be done' # at /home/atorik/postgres/contrib/vacuumlo/../../src/test/perl/PostgreSQL/Test/Utils.pm line 850. # Looks like you failed 1 test of 21. # Failed test 'vacuumlo --help outputs fit within 80 columns per line' # at t/001_basic.pl line 10. # Looks like you failed 1 test of 9. t/001_basic.pl .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/9 subtests Test Summary Report ------------------- t/001_basic.pl (Wstat: 256 (exited 1) Tests: 9 Failed: 1) Failed test: 4 Non-zero exit status: 1 Files=1, Tests=9, 0 wallclock secs ( 0.01 usr 0.01 sys + 0.04 cusr 0.01 csys = 0.07 CPU) Result: FAIL ``` ``` $ cat tmp_check/log/regress_log_001_basic # Running: vacuumlo --help [23:11:10.378](0.230s) ok 1 - vacuumlo --help exit code 0 [23:11:10.379](0.001s) ok 2 - vacuumlo --help goes to stdout [23:11:10.379](0.000s) ok 3 - vacuumlo --help nothing to stderr [23:11:10.380](0.000s) # Subtest: vacuumlo --help outputs fit within 80 columns per line [23:11:10.380](0.001s) ok 1 - vacuumlo removes unreferenced large objects from databases. [23:11:10.380](0.000s) ok 2 - [23:11:10.381](0.000s) ok 3 - Usage: [23:11:10.381](0.000s) ok 4 - vacuumlo [OPTION]... DBNAME... [23:11:10.381](0.000s) ok 5 - [23:11:10.381](0.000s) ok 6 - Options: [23:11:10.381](0.000s) ok 7 - -l, --limit=LIMIT commit after removing each LIMIT large objects [23:11:10.382](0.000s) ok 20 - Report bugs to <[email protected]>. [23:11:10.382](0.000s) ok 21 - PostgreSQL home page: <https://www.postgresql.org/; [23:11:10.382](0.000s) 1..21 [23:11:10.382](0.000s) # Looks like you failed 1 test of 21. [23:11:10.382](0.000s) not ok 4 - vacuumlo --help outputs fit within 80 columns per line [23:11:10.382](0.000s) [23:11:10.382](0.000s) # Failed test 'vacuumlo --help outputs fit within 80 columns per line' # at t/001_basic.pl line 10. # Running: vacuumlo --version [23:11:10.388](0.005s) ok 5 - vacuumlo --version exit code 0 [23:11:10.388](0.000s) ok 6 - vacuumlo --version goes to stdout [23:11:10.388](0.000s) ok 7 - vacuumlo --version nothing to stderr # Running: vacuumlo --not-a-valid-option [23:11:10.391](0.003s) ok 8 - vacuumlo with invalid option nonzero exit code [23:11:10.391](0.000s) ok 9 - vacuumlo with invalid option prints error message [23:11:10.391](0.000s) 1..9 [23:11:10.391](0.000s) # Looks like you failed 1 test of 9. ``` I feel using subtest in Test::More improves readability. On 2023-09-14 02:46, Greg Sabino Mullane wrote: > All this seems an awful lot of work to support this mythical 80-column > terminal user. > It's 2023, perhaps it's time to widen the default assumption past 80 > characters? That may be a good idea. However, from what I have seen some basic commands like `ls` in my Linux environments, the man command has over 100 characters per line, while the output of the --help option seems to be within 80 characters per line. Also, the current PostgreSQL commands follow the "no more than 80 characters per line". I do not intend to adhere to this rule(my terminals are usually bigger than 80 chars per line), but wouldn't it be a not bad direction to use 80 characters for all commands? Thoughts? -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation Attachments: [text/x-diff] v1-0001-Added-a-test-for-checking-the-length-of-help-outp.patch (923B, ../../[email protected]/2-v1-0001-Added-a-test-for-checking-the-length-of-help-outp.patch) download | inline diff: From 3dbfdb79680a0c1b68d4f742ae408810a1ee999d Mon Sep 17 00:00:00 2001 From: Atsushi Torikoshi <[email protected]> Date: Fri, 15 Sep 2023 23:29:23 +0900 Subject: [PATCH v1] Added a test for checking the length of --help output --- src/test/perl/PostgreSQL/Test/Utils.pm | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm index 617caa022f..989f369ae7 100644 --- a/src/test/perl/PostgreSQL/Test/Utils.pm +++ b/src/test/perl/PostgreSQL/Test/Utils.pm @@ -843,6 +843,13 @@ sub program_help_ok ok($result, "$cmd --help exit code 0"); isnt($stdout, '', "$cmd --help goes to stdout"); is($stderr, '', "$cmd --help nothing to stderr"); + + subtest "$cmd --help outputs fit within 80 columns per line" => sub { + foreach my $line (split /\n/, $stdout) + { + ok(length($line) <= 80, "$line"); + } +}; return; } -- 2.39.2 ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-09-15 15:10 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-12-18 16:19 [PATCH v42 1/2] Subscripting for jsonb Dmitrii Dolgov <[email protected]> 2023-07-05 01:47 Make --help output fit within 80 columns per line torikoshia <[email protected]> 2023-08-21 04:08 ` Re: Make --help output fit within 80 columns per line Masahiro Ikeda <[email protected]> 2023-08-22 13:57 ` Re: Make --help output fit within 80 columns per line torikoshia <[email protected]> 2023-09-13 17:46 ` Re: Make --help output fit within 80 columns per line Greg Sabino Mullane <[email protected]> 2023-09-15 15:10 ` Re: Make --help output fit within 80 columns per line torikoshia <[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