($INBOX_DIR/description missing)help / color / mirror / Atom feed
[PATCH v30 3/6] Subscripting for jsonb 28+ messages / 7 participants [nested] [flat]
* [PATCH v30 3/6] Subscripting for jsonb @ 2019-02-01 10:41 erthalion <[email protected]> 0 siblings, 0 replies; 28+ messages in thread From: erthalion @ 2019-02-01 10:41 UTC (permalink / raw) Subscripting implementation for jsonb. 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. Reviewed-by: Tom Lane, Arthur Zakirov --- src/backend/utils/adt/jsonb.c | 27 ++- src/backend/utils/adt/jsonb_util.c | 76 ++++++- src/backend/utils/adt/jsonfuncs.c | 325 ++++++++++++++++++++-------- src/include/catalog/pg_proc.dat | 8 + src/include/catalog/pg_type.dat | 3 +- src/include/utils/jsonb.h | 2 + src/test/regress/expected/jsonb.out | 233 +++++++++++++++++++- src/test/regress/sql/jsonb.sql | 68 +++++- 8 files changed, 632 insertions(+), 110 deletions(-) diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c index b961d29472..ad364b4a0e 100644 --- a/src/backend/utils/adt/jsonb.c +++ b/src/backend/utils/adt/jsonb.c @@ -1134,23 +1134,34 @@ to_jsonb(PG_FUNCTION_ARGS) { Datum val = PG_GETARG_DATUM(0); Oid val_type = get_fn_expr_argtype(fcinfo->flinfo, 0); - JsonbInState result; - JsonbTypeCategory tcategory; - Oid outfuncoid; + JsonbValue *res = to_jsonb_worker(val, val_type, false); + PG_RETURN_POINTER(JsonbValueToJsonb(res)); +} - if (val_type == InvalidOid) +/* + * Do the actual conversion to jsonb for to_jsonb function. This logic is + * separated because it can be useful not only in here (e.g. we use it in + * jsonb subscripting) + */ +JsonbValue * +to_jsonb_worker(Datum source, Oid source_type, bool is_null) +{ + JsonbInState result; + JsonbTypeCategory tcategory; + Oid outfuncoid; + + if (source_type == InvalidOid) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("could not determine input data type"))); - jsonb_categorize_type(val_type, + jsonb_categorize_type(source_type, &tcategory, &outfuncoid); memset(&result, 0, sizeof(JsonbInState)); - datum_to_jsonb(val, false, &result, tcategory, outfuncoid, false); - - PG_RETURN_POINTER(JsonbValueToJsonb(result.res)); + datum_to_jsonb(source, is_null, &result, tcategory, outfuncoid, false); + return result.res; } /* diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c index 04b70c805b..adcf16acf1 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/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index f92861d8d2..538740aec9 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -21,12 +21,16 @@ #include "common/jsonapi.h" #include "fmgr.h" #include "funcapi.h" +#include "executor/execExpr.h" #include "lib/stringinfo.h" #include "mb/pg_wchar.h" #include "miscadmin.h" +#include "nodes/nodeFuncs.h" +#include "parser/parse_coerce.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/hsearch.h" +#include "utils/fmgroids.h" #include "utils/json.h" #include "utils/jsonb.h" #include "utils/jsonfuncs.h" @@ -460,18 +464,22 @@ 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 Datum jsonb_set_element(Datum datum, Datum *path, int path_len, + Datum sourceData, Oid source_type, bool is_null); +static Datum jsonb_get_element(Jsonb *jb, Datum *path, int npath, + bool *isnull, bool as_text); static 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); @@ -486,6 +494,15 @@ static void transform_string_values_object_field_start(void *state, char *fname, static void transform_string_values_array_element_start(void *state, bool isnull); static void transform_string_values_scalar(void *state, char *token, JsonTokenType tokentype); +static SubscriptingRef *jsonb_subscript_prepare(bool isAssignment, + SubscriptingRef *sbsref); + +static SubscriptingRef *jsonb_subscript_validate(bool isAssignment, + SubscriptingRef *sbsref, + ParseState *pstate); +static Datum jsonb_subscript_fetch(Datum containerSource, SubscriptingRefState *sbstate); +static Datum jsonb_subscript_assign(Datum containerSource, SubscriptingRefState *sbstate); + /* * pg_parse_json_or_ereport * @@ -1461,13 +1478,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 @@ -1482,9 +1495,28 @@ 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); +} + +static Datum +jsonb_get_element(Jsonb *jb, Datum *path, int npath, bool *isnull, bool as_text) +{ + Jsonb *res; + JsonbContainer *container = &jb->root; + JsonbValue *jbvp = NULL; + JsonbValue tv; + 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)) @@ -1509,7 +1541,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)))); } @@ -1525,22 +1557,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) { @@ -1558,7 +1593,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; } @@ -1568,11 +1606,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; @@ -1594,9 +1636,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 { @@ -1607,6 +1652,32 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text) } } +Datum +jsonb_set_element(Datum jsonbdatum, Datum *path, int path_len, + Datum sourceData, Oid source_type, bool is_null) +{ + Jsonb *jb = DatumGetJsonbP(jsonbdatum); + JsonbValue *newval, + *res; + JsonbParseState *state = NULL; + JsonbIterator *it; + bool *path_nulls = palloc0(path_len * sizeof(bool)); + + newval = to_jsonb_worker(sourceData, source_type, is_null); + + 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. */ @@ -4166,58 +4237,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) * @@ -4489,7 +4508,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; @@ -4646,7 +4666,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; @@ -4809,7 +4830,7 @@ IteratorConcat(JsonbIterator **it1, JsonbIterator **it2, static 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; @@ -4862,11 +4883,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]) @@ -4883,7 +4904,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++) @@ -4914,7 +4935,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; } @@ -4937,7 +4958,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); @@ -4969,7 +4990,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, @@ -5017,7 +5038,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; } @@ -5033,7 +5054,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 @@ -5044,7 +5065,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; } @@ -5078,12 +5099,138 @@ 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); } } } } +/* + * Perform an actual data extraction or modification for the jsonb + * subscripting. As a result the extracted Datum or the modified containers + * value will be returned. + */ +Datum +jsonb_subscript_fetch(Datum containerSource, SubscriptingRefState *sbstate) +{ + return jsonb_get_element(DatumGetJsonbP(containerSource), + sbstate->upperindex, + sbstate->numupper, + &sbstate->resnull, + false); +} + + + +/* + * Perform an actual data extraction or modification for the jsonb + * subscripting. As a result the extracted Datum or the modified containers + * value will be returned. + */ +Datum +jsonb_subscript_assign(Datum containerSource, SubscriptingRefState *sbstate) +{ + /* + * the original jsonb must be non-NULL, else we punt and return the + * original array. + */ + if (sbstate->resnull) + return containerSource; + + return jsonb_set_element(containerSource, + sbstate->upperindex, + sbstate->numupper, + sbstate->replacevalue, + sbstate->refelemtype, + sbstate->replacenull); +} + +/* + * Perform preparation for the jsonb subscripting. Since there are not any + * particular restrictions for this kind of subscripting, we will verify that + * it is not a slice operation. This function produces an expression that + * represents the result of extracting a single container element or the new + * container value with the source data inserted into the right part of the + * container. If you have read until this point, and will submit a meaningful + * review of this patch series, I'll owe you a beer at the next PGConfEU. + */ + +/* + * Handle jsonb-type subscripting logic. + */ +Datum +jsonb_subscript_handler(PG_FUNCTION_ARGS) +{ + SubscriptRoutines *sbsroutines = (SubscriptRoutines *) + palloc(sizeof(SubscriptRoutines)); + + sbsroutines->prepare = jsonb_subscript_prepare; + sbsroutines->validate = jsonb_subscript_validate; + sbsroutines->fetch = jsonb_subscript_fetch; + sbsroutines->assign = jsonb_subscript_assign; + + PG_RETURN_POINTER(sbsroutines); +} + +SubscriptingRef * +jsonb_subscript_prepare(bool isAssignment, SubscriptingRef *sbsref) +{ + if (isAssignment) + { + sbsref->refelemtype = exprType((Node *) sbsref->refassgnexpr); + sbsref->refassgntype = exprType((Node *) sbsref->refassgnexpr); + } + else + sbsref->refelemtype = JSONBOID; + + return sbsref; +} + +SubscriptingRef * +jsonb_subscript_validate(bool isAssignment, SubscriptingRef *sbsref, + ParseState *pstate) +{ + List *upperIndexpr = NIL; + ListCell *l; + + if (sbsref->reflowerindexpr != NIL) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("jsonb subscript does not support slices"), + parser_errposition(pstate, exprLocation( + ((Node *) linitial(sbsref->reflowerindexpr)))))); + + foreach(l, sbsref->refupperindexpr) + { + Node *subexpr = (Node *) lfirst(l); + + if (subexpr == NULL) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("jsonb subscript does not support slices"), + parser_errposition(pstate, exprLocation( + ((Node *) linitial(sbsref->refupperindexpr)))))); + + subexpr = coerce_to_target_type(pstate, + subexpr, exprType(subexpr), + TEXTOID, -1, + COERCION_ASSIGNMENT, + 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)))); + + upperIndexpr = lappend(upperIndexpr, subexpr); + } + + sbsref->refupperindexpr = upperIndexpr; + + return sbsref; +} + /* * Parse information about what elements of a jsonb document we want to iterate * in functions iterate_json(b)_values. This information is presented in jsonb diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 6de2dd29f1..36637afefb 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -10707,6 +10707,14 @@ proargnames => '{max_data_alignment,database_block_size,blocks_per_segment,wal_block_size,bytes_per_wal_segment,max_identifier_length,max_index_columns,max_toast_chunk_size,large_object_chunk_size,float8_pass_by_value,data_page_checksum_version}', prosrc => 'pg_control_init' }, +# type subscripting support +{ oid => '4191', + descr => 'Jsonb subscripting logic', + proname => 'jsonb_subscript_handler', + prorettype => 'internal', + proargtypes => 'internal', + prosrc => 'jsonb_subscript_handler' }, + { oid => '4192', descr => 'Array subscripting logic', proname => 'array_subscript_handler', diff --git a/src/include/catalog/pg_type.dat b/src/include/catalog/pg_type.dat index f868a6d52e..64c0c6d519 100644 --- a/src/include/catalog/pg_type.dat +++ b/src/include/catalog/pg_type.dat @@ -447,7 +447,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', + typsubshandler => '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..6e3b75d56a 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,6 @@ extern char *JsonbToCStringIndent(StringInfo out, JsonbContainer *in, extern bool JsonbExtractScalar(JsonbContainer *jbc, JsonbValue *res); extern const char *JsonbTypeName(JsonbValue *jb); +extern JsonbValue *to_jsonb_worker(Datum source, Oid source_type, bool is_null); #endif /* __JSONB_H__ */ diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out index a70cd0b7c1..04a146a7d0 100644 --- a/src/test/regress/expected/jsonb.out +++ b/src/test/regress/expected/jsonb.out @@ -4567,7 +4567,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 @@ -4697,6 +4697,237 @@ 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]; + jsonb +------- + +(1 row) + +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) + +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: 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) + -- 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 3e2b8f66df..12541e7e50 100644 --- a/src/test/regress/sql/jsonb.sql +++ b/src/test/regress/sql/jsonb.sql @@ -1172,7 +1172,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"'); @@ -1203,6 +1203,72 @@ 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]; + +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; + -- jsonb to tsvector select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb); -- 2.21.0 --g5st7yssq7xavhfb Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v30-0004-Subscripting-documentation.patch" ^ permalink raw reply [nested|flat] 28+ messages in thread
* [PATCH v28 3/5] Subscripting for jsonb @ 2019-02-01 10:41 erthalion <[email protected]> 0 siblings, 0 replies; 28+ messages in thread From: erthalion @ 2019-02-01 10:41 UTC (permalink / raw) Subscripting implementation for jsonb. 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. Reviewed-by: Tom Lane, Arthur Zakirov --- src/backend/utils/adt/jsonb.c | 27 ++- src/backend/utils/adt/jsonb_util.c | 76 +++++++-- src/backend/utils/adt/jsonfuncs.c | 324 ++++++++++++++++++++++++++---------- src/include/catalog/pg_proc.dat | 8 + src/include/catalog/pg_type.dat | 3 +- src/include/utils/jsonb.h | 2 + src/test/regress/expected/jsonb.out | 231 +++++++++++++++++++++++++ src/test/regress/sql/jsonb.sql | 66 ++++++++ 8 files changed, 629 insertions(+), 108 deletions(-) diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c index 74b4bbe..7af55fc 100644 --- a/src/backend/utils/adt/jsonb.c +++ b/src/backend/utils/adt/jsonb.c @@ -1134,23 +1134,34 @@ to_jsonb(PG_FUNCTION_ARGS) { Datum val = PG_GETARG_DATUM(0); Oid val_type = get_fn_expr_argtype(fcinfo->flinfo, 0); - JsonbInState result; - JsonbTypeCategory tcategory; - Oid outfuncoid; + JsonbValue *res = to_jsonb_worker(val, val_type, false); + PG_RETURN_POINTER(JsonbValueToJsonb(res)); +} - if (val_type == InvalidOid) +/* + * Do the actual conversion to jsonb for to_jsonb function. This logic is + * separated because it can be useful not only in here (e.g. we use it in + * jsonb subscripting) + */ +JsonbValue * +to_jsonb_worker(Datum source, Oid source_type, bool is_null) +{ + JsonbInState result; + JsonbTypeCategory tcategory; + Oid outfuncoid; + + if (source_type == InvalidOid) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("could not determine input data type"))); - jsonb_categorize_type(val_type, + jsonb_categorize_type(source_type, &tcategory, &outfuncoid); memset(&result, 0, sizeof(JsonbInState)); - datum_to_jsonb(val, false, &result, tcategory, outfuncoid, false); - - PG_RETURN_POINTER(JsonbValueToJsonb(result.res)); + datum_to_jsonb(source, is_null, &result, tcategory, outfuncoid, false); + return result.res; } /* diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c index 8b73903..7be928f 100644 --- a/src/backend/utils/adt/jsonb_util.c +++ b/src/backend/utils/adt/jsonb_util.c @@ -67,18 +67,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) @@ -562,6 +573,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) @@ -572,9 +607,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/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 1b0fb2a..ca9e134 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -20,12 +20,16 @@ #include "catalog/pg_type.h" #include "fmgr.h" #include "funcapi.h" +#include "executor/execExpr.h" #include "lib/stringinfo.h" #include "mb/pg_wchar.h" #include "miscadmin.h" +#include "nodes/nodeFuncs.h" +#include "parser/parse_coerce.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/hsearch.h" +#include "utils/fmgroids.h" #include "utils/json.h" #include "utils/jsonapi.h" #include "utils/jsonb.h" @@ -457,18 +461,22 @@ 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 Datum jsonb_set_element(Datum datum, Datum *path, int path_len, + Datum sourceData, Oid source_type, bool is_null); +static Datum jsonb_get_element(Jsonb *jb, Datum *path, int npath, + bool *isnull, bool as_text); static 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); @@ -483,6 +491,15 @@ static void transform_string_values_object_field_start(void *state, char *fname, static void transform_string_values_array_element_start(void *state, bool isnull); static void transform_string_values_scalar(void *state, char *token, JsonTokenType tokentype); +static SubscriptingRef *jsonb_subscript_prepare(bool isAssignment, + SubscriptingRef *sbsref); + +static SubscriptingRef *jsonb_subscript_validate(bool isAssignment, + SubscriptingRef *sbsref, + ParseState *pstate); +static Datum jsonb_subscript_fetch(Datum containerSource, SubscriptingRefState *sbstate); +static Datum jsonb_subscript_assign(Datum containerSource, SubscriptingRefState *sbstate); + /* * SQL function json_object_keys * @@ -1328,13 +1345,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 @@ -1349,9 +1362,28 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text) deconstruct_array(path, TEXTOID, -1, false, 'i', &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); +} + +static Datum +jsonb_get_element(Jsonb *jb, Datum *path, int npath, bool *isnull, bool as_text) +{ + Jsonb *res; + JsonbContainer *container = &jb->root; + JsonbValue *jbvp = NULL; + JsonbValue tv; + 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)) @@ -1376,7 +1408,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)))); } @@ -1392,22 +1424,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) { @@ -1425,7 +1460,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; } @@ -1435,11 +1473,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; @@ -1461,9 +1503,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 { @@ -1474,6 +1519,32 @@ get_jsonb_path_all(FunctionCallInfo fcinfo, bool as_text) } } +Datum +jsonb_set_element(Datum jsonbdatum, Datum *path, int path_len, + Datum sourceData, Oid source_type, bool is_null) +{ + Jsonb *jb = DatumGetJsonbP(jsonbdatum); + JsonbValue *newval, + *res; + JsonbParseState *state = NULL; + JsonbIterator *it; + bool *path_nulls = palloc0(path_len * sizeof(bool)); + + newval = to_jsonb_worker(sourceData, source_type, is_null); + + 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. */ @@ -4034,58 +4105,6 @@ jsonb_strip_nulls(PG_FUNCTION_ARGS) } /* - * 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) * * Pretty-printed text for the jsonb @@ -4356,7 +4375,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; @@ -4447,7 +4467,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; @@ -4610,7 +4631,7 @@ IteratorConcat(JsonbIterator **it1, JsonbIterator **it2, static 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; @@ -4663,11 +4684,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]) @@ -4684,7 +4705,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++) @@ -4715,7 +4736,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; } @@ -4738,7 +4759,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); @@ -4770,7 +4791,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, @@ -4818,7 +4839,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; } @@ -4834,7 +4855,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 @@ -4845,7 +4866,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; } @@ -4879,13 +4900,138 @@ 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); } } } } /* + * Perform an actual data extraction or modification for the jsonb + * subscripting. As a result the extracted Datum or the modified containers + * value will be returned. + */ +Datum +jsonb_subscript_fetch(Datum containerSource, SubscriptingRefState *sbstate) +{ + return jsonb_get_element(DatumGetJsonbP(containerSource), + sbstate->upperindex, + sbstate->numupper, + &sbstate->resnull, + false); +} + + + +/* + * Perform an actual data extraction or modification for the jsonb + * subscripting. As a result the extracted Datum or the modified containers + * value will be returned. + */ +Datum +jsonb_subscript_assign(Datum containerSource, SubscriptingRefState *sbstate) +{ + /* + * the original jsonb must be non-NULL, else we punt and return the + * original array. + */ + if (sbstate->resnull) + return containerSource; + + return jsonb_set_element(containerSource, + sbstate->upperindex, + sbstate->numupper, + sbstate->replacevalue, + sbstate->refelemtype, + sbstate->replacenull); +} + +/* + * Perform preparation for the jsonb subscripting. Since there are not any + * particular restrictions for this kind of subscripting, we will verify that + * it is not a slice operation. This function produces an expression that + * represents the result of extracting a single container element or the new + * container value with the source data inserted into the right part of the + * container. + */ + +/* + * Handle jsonb-type subscripting logic. + */ +Datum +jsonb_subscript_handler(PG_FUNCTION_ARGS) +{ + SubscriptRoutines *sbsroutines = (SubscriptRoutines *) + palloc(sizeof(SubscriptRoutines)); + + sbsroutines->prepare = jsonb_subscript_prepare; + sbsroutines->validate = jsonb_subscript_validate; + sbsroutines->fetch = jsonb_subscript_fetch; + sbsroutines->assign = jsonb_subscript_assign; + + PG_RETURN_POINTER(sbsroutines); +} + +SubscriptingRef * +jsonb_subscript_prepare(bool isAssignment, SubscriptingRef *sbsref) +{ + if (isAssignment) + { + sbsref->refelemtype = exprType((Node *) sbsref->refassgnexpr); + sbsref->refassgntype = exprType((Node *) sbsref->refassgnexpr); + } + else + sbsref->refelemtype = JSONBOID; + + return sbsref; +} + +SubscriptingRef * +jsonb_subscript_validate(bool isAssignment, SubscriptingRef *sbsref, + ParseState *pstate) +{ + List *upperIndexpr = NIL; + ListCell *l; + + if (sbsref->reflowerindexpr != NIL) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("jsonb subscript does not support slices"), + parser_errposition(pstate, exprLocation( + ((Node *) linitial(sbsref->reflowerindexpr)))))); + + foreach(l, sbsref->refupperindexpr) + { + Node *subexpr = (Node *) lfirst(l); + + if (subexpr == NULL) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("jsonb subscript does not support slices"), + parser_errposition(pstate, exprLocation( + ((Node *) linitial(sbsref->refupperindexpr)))))); + + subexpr = coerce_to_target_type(pstate, + subexpr, exprType(subexpr), + TEXTOID, -1, + COERCION_ASSIGNMENT, + 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)))); + + upperIndexpr = lappend(upperIndexpr, subexpr); + } + + sbsref->refupperindexpr = upperIndexpr; + + return sbsref; +} + +/* * Parse information about what elements of a jsonb document we want to iterate * in functions iterate_json(b)_values. This information is presented in jsonb * format, so that it can be easily extended in the future. diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 38474a7..b3358c7 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -10665,6 +10665,14 @@ proargnames => '{max_data_alignment,database_block_size,blocks_per_segment,wal_block_size,bytes_per_wal_segment,max_identifier_length,max_index_columns,max_toast_chunk_size,large_object_chunk_size,float4_pass_by_value,float8_pass_by_value,data_page_checksum_version}', prosrc => 'pg_control_init' }, +# type subscripting support +{ oid => '4191', + descr => 'Jsonb subscripting logic', + proname => 'jsonb_subscript_handler', + prorettype => 'internal', + proargtypes => 'internal', + prosrc => 'jsonb_subscript_handler' }, + { oid => '4192', descr => 'Array subscripting logic', proname => 'array_subscript_handler', diff --git a/src/include/catalog/pg_type.dat b/src/include/catalog/pg_type.dat index 3e3fa26..32500f7 100644 --- a/src/include/catalog/pg_type.dat +++ b/src/include/catalog/pg_type.dat @@ -447,7 +447,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', + typsubshandler => '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 5e81796..6081fab 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,6 @@ extern char *JsonbToCStringIndent(StringInfo out, JsonbContainer *in, extern bool JsonbExtractScalar(JsonbContainer *jbc, JsonbValue *res); extern const char *JsonbTypeName(JsonbValue *jb); +extern JsonbValue *to_jsonb_worker(Datum source, Oid source_type, bool is_null); #endif /* __JSONB_H__ */ diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out index a2a19f8..c7d7e30 100644 --- a/src/test/regress/expected/jsonb.out +++ b/src/test/regress/expected/jsonb.out @@ -4640,6 +4640,237 @@ 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]; + jsonb +------- + +(1 row) + +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) + +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: 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) + -- 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 efd4c45..934fa9d 100644 --- a/src/test/regress/sql/jsonb.sql +++ b/src/test/regress/sql/jsonb.sql @@ -1183,6 +1183,72 @@ 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]; + +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; + -- jsonb to tsvector select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb); -- 2.7.4 --------------3C456A3C89C0AEF059999FAB Content-Type: text/x-patch; name="v28-0004-Subscripting-documentation.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="v28-0004-Subscripting-documentation.patch" ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-07 14:24 Melanie Plageman <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Melanie Plageman @ 2024-04-07 14:24 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Sun, Apr 7, 2024 at 7:38 AM Tomas Vondra <[email protected]> wrote: > > > > On 4/7/24 06:17, Melanie Plageman wrote: > > On Sun, Apr 07, 2024 at 02:27:43AM +0200, Tomas Vondra wrote: > >> On 4/6/24 23:34, Melanie Plageman wrote: > >>> ... > >>>> > >>>> I realized it makes more sense to add a FIXME (I used XXX. I'm not when > >>>> to use what) with a link to the message where Andres describes why he > >>>> thinks it is a bug. If we plan on fixing it, it is good to have a record > >>>> of that. And it makes it easier to put a clear and accurate comment. > >>>> Done in 0009. > >>>> > >>>>> OK, thanks. If think 0001-0008 are ready to go, with some minor tweaks > >>>>> per above (tuple vs. tuples etc.), and the question about the recheck > >>>>> flag. If you can do these tweaks, I'll get that committed today and we > >>>>> can try to get a couple more patches in tomorrow. > >>> > >>> Attached v19 rebases the rest of the commits from v17 over the first > >>> nine patches from v18. All patches 0001-0009 are unchanged from v18. I > >>> have made updates and done cleanup on 0010-0021. > >>> > >> > >> I've pushed 0001-0005, I'll get back to this tomorrow and see how much > >> more we can get in for v17. > > > > Thanks! I thought about it a bit more, and I got worried about the > > > > Assert(scan->rs_empty_tuples_pending == 0); > > > > in heap_rescan() and heap_endscan(). > > > > I was worried if we don't complete the scan it could end up tripping > > incorrectly. > > > > I tried to come up with a query which didn't end up emitting all of the > > tuples on the page (using a LIMIT clause), but I struggled to come up > > with an example that qualified for the skip fetch optimization and also > > returned before completing the scan. > > > > I could work a bit harder tomorrow to try and come up with something. > > However, I think it might be safer to just change these to: > > > > scan->rs_empty_tuples_pending = 0 > > > > Hmmm, good point. I haven't tried, but wouldn't something like "SELECT 1 > FROM t WHERE column = X LIMIT 1" do the trick? Probably in a join, as a > correlated subquery? Unfortunately (or fortunately, I guess) that exact thing won't work because even constant values in the target list disqualify it for the skip fetch optimization. Being a bit too lazy to look at planner code this morning, I removed the target list requirement like this: - need_tuples = (node->ss.ps.plan->qual != NIL || - node->ss.ps.plan->targetlist != NIL); + need_tuples = (node->ss.ps.plan->qual != NIL); And can easily trip the assert with this: create table foo (a int); insert into foo select i from generate_series(1,10)i; create index on foo(a); vacuum foo; select 1 from (select 2 from foo limit 3); Anyway, I don't know if we could find a query that does actually hit this. The only bitmap heap scan queries in the regress suite that meet the BitmapHeapScanState->ss.ps.plan->targetlist == NIL condition are aggregates (all are count(*)). I'll dig a bit more later, but do you think this is worth adding an open item for? Even though I don't have a repro yet? - Melanie ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-07 14:41 Tomas Vondra <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 2 replies; 28+ messages in thread From: Tomas Vondra @ 2024-04-07 14:41 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On 4/7/24 16:24, Melanie Plageman wrote: > On Sun, Apr 7, 2024 at 7:38 AM Tomas Vondra > <[email protected]> wrote: >> >> >> >> On 4/7/24 06:17, Melanie Plageman wrote: >>> On Sun, Apr 07, 2024 at 02:27:43AM +0200, Tomas Vondra wrote: >>>> On 4/6/24 23:34, Melanie Plageman wrote: >>>>> ... >>>>>> >>>>>> I realized it makes more sense to add a FIXME (I used XXX. I'm not when >>>>>> to use what) with a link to the message where Andres describes why he >>>>>> thinks it is a bug. If we plan on fixing it, it is good to have a record >>>>>> of that. And it makes it easier to put a clear and accurate comment. >>>>>> Done in 0009. >>>>>> >>>>>>> OK, thanks. If think 0001-0008 are ready to go, with some minor tweaks >>>>>>> per above (tuple vs. tuples etc.), and the question about the recheck >>>>>>> flag. If you can do these tweaks, I'll get that committed today and we >>>>>>> can try to get a couple more patches in tomorrow. >>>>> >>>>> Attached v19 rebases the rest of the commits from v17 over the first >>>>> nine patches from v18. All patches 0001-0009 are unchanged from v18. I >>>>> have made updates and done cleanup on 0010-0021. >>>>> >>>> >>>> I've pushed 0001-0005, I'll get back to this tomorrow and see how much >>>> more we can get in for v17. >>> >>> Thanks! I thought about it a bit more, and I got worried about the >>> >>> Assert(scan->rs_empty_tuples_pending == 0); >>> >>> in heap_rescan() and heap_endscan(). >>> >>> I was worried if we don't complete the scan it could end up tripping >>> incorrectly. >>> >>> I tried to come up with a query which didn't end up emitting all of the >>> tuples on the page (using a LIMIT clause), but I struggled to come up >>> with an example that qualified for the skip fetch optimization and also >>> returned before completing the scan. >>> >>> I could work a bit harder tomorrow to try and come up with something. >>> However, I think it might be safer to just change these to: >>> >>> scan->rs_empty_tuples_pending = 0 >>> >> >> Hmmm, good point. I haven't tried, but wouldn't something like "SELECT 1 >> FROM t WHERE column = X LIMIT 1" do the trick? Probably in a join, as a >> correlated subquery? > > Unfortunately (or fortunately, I guess) that exact thing won't work > because even constant values in the target list disqualify it for the > skip fetch optimization. > > Being a bit too lazy to look at planner code this morning, I removed > the target list requirement like this: > > - need_tuples = (node->ss.ps.plan->qual != NIL || > - node->ss.ps.plan->targetlist != NIL); > + need_tuples = (node->ss.ps.plan->qual != NIL); > > And can easily trip the assert with this: > > create table foo (a int); > insert into foo select i from generate_series(1,10)i; > create index on foo(a); > vacuum foo; > select 1 from (select 2 from foo limit 3); > > Anyway, I don't know if we could find a query that does actually hit > this. The only bitmap heap scan queries in the regress suite that meet > the > BitmapHeapScanState->ss.ps.plan->targetlist == NIL > condition are aggregates (all are count(*)). > > I'll dig a bit more later, but do you think this is worth adding an > open item for? Even though I don't have a repro yet? > Try this: create table t (a int, b int) with (fillfactor=10); insert into t select mod((i/22),2), (i/22) from generate_series(0,1000) S(i); create index on t(a); vacuum analyze t; set enable_indexonlyscan = off; set enable_seqscan = off; explain (analyze, verbose) select 1 from (values (1)) s(x) where exists (select * from t where a = x); KABOOM! #2 0x000078a16ac5fafe in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #3 0x000078a16ac4887f in __GI_abort () at abort.c:79 #4 0x0000000000bb2c5a in ExceptionalCondition (conditionName=0xc42ba8 "scan->rs_empty_tuples_pending == 0", fileName=0xc429c8 "heapam.c", lineNumber=1090) at assert.c:66 #5 0x00000000004f68bb in heap_endscan (sscan=0x19af3a0) at heapam.c:1090 #6 0x000000000077a94c in table_endscan (scan=0x19af3a0) at ../../../src/include/access/tableam.h:1001 So yeah, this assert is not quite correct. It's not breaking anything at the moment, so we can fix it now or add it as an open item. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-07 14:54 Melanie Plageman <[email protected]> parent: Tomas Vondra <[email protected]> 1 sibling, 1 reply; 28+ messages in thread From: Melanie Plageman @ 2024-04-07 14:54 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Sun, Apr 7, 2024 at 10:42 AM Tomas Vondra <[email protected]> wrote: > > > > On 4/7/24 16:24, Melanie Plageman wrote: > >>> Thanks! I thought about it a bit more, and I got worried about the > >>> > >>> Assert(scan->rs_empty_tuples_pending == 0); > >>> > >>> in heap_rescan() and heap_endscan(). > >>> > >>> I was worried if we don't complete the scan it could end up tripping > >>> incorrectly. > >>> > >>> I tried to come up with a query which didn't end up emitting all of the > >>> tuples on the page (using a LIMIT clause), but I struggled to come up > >>> with an example that qualified for the skip fetch optimization and also > >>> returned before completing the scan. > >>> > >>> I could work a bit harder tomorrow to try and come up with something. > >>> However, I think it might be safer to just change these to: > >>> > >>> scan->rs_empty_tuples_pending = 0 > >>> > >> > >> Hmmm, good point. I haven't tried, but wouldn't something like "SELECT 1 > >> FROM t WHERE column = X LIMIT 1" do the trick? Probably in a join, as a > >> correlated subquery? > > > > Unfortunately (or fortunately, I guess) that exact thing won't work > > because even constant values in the target list disqualify it for the > > skip fetch optimization. > > > > Being a bit too lazy to look at planner code this morning, I removed > > the target list requirement like this: > > > > - need_tuples = (node->ss.ps.plan->qual != NIL || > > - node->ss.ps.plan->targetlist != NIL); > > + need_tuples = (node->ss.ps.plan->qual != NIL); > > > > And can easily trip the assert with this: > > > > create table foo (a int); > > insert into foo select i from generate_series(1,10)i; > > create index on foo(a); > > vacuum foo; > > select 1 from (select 2 from foo limit 3); > > > > Anyway, I don't know if we could find a query that does actually hit > > this. The only bitmap heap scan queries in the regress suite that meet > > the > > BitmapHeapScanState->ss.ps.plan->targetlist == NIL > > condition are aggregates (all are count(*)). > > > > I'll dig a bit more later, but do you think this is worth adding an > > open item for? Even though I don't have a repro yet? > > > > Try this: > > create table t (a int, b int) with (fillfactor=10); > insert into t select mod((i/22),2), (i/22) from generate_series(0,1000) > S(i); > create index on t(a); > vacuum analyze t; > > set enable_indexonlyscan = off; > set enable_seqscan = off; > explain (analyze, verbose) select 1 from (values (1)) s(x) where exists > (select * from t where a = x); > > KABOOM! Ooo fancy query! Good job. > #2 0x000078a16ac5fafe in __GI_raise (sig=sig@entry=6) at > ../sysdeps/posix/raise.c:26 > #3 0x000078a16ac4887f in __GI_abort () at abort.c:79 > #4 0x0000000000bb2c5a in ExceptionalCondition (conditionName=0xc42ba8 > "scan->rs_empty_tuples_pending == 0", fileName=0xc429c8 "heapam.c", > lineNumber=1090) at assert.c:66 > #5 0x00000000004f68bb in heap_endscan (sscan=0x19af3a0) at heapam.c:1090 > #6 0x000000000077a94c in table_endscan (scan=0x19af3a0) at > ../../../src/include/access/tableam.h:1001 > > So yeah, this assert is not quite correct. It's not breaking anything at > the moment, so we can fix it now or add it as an open item. I've added an open item [1], because what's one open item when you can have two? (me) - Melanie [1] https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items#Open_Issues ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-12 03:18 Richard Guo <[email protected]> parent: Tomas Vondra <[email protected]> 1 sibling, 0 replies; 28+ messages in thread From: Richard Guo @ 2024-04-12 03:18 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Sun, Apr 7, 2024 at 10:42 PM Tomas Vondra <[email protected]> wrote: > create table t (a int, b int) with (fillfactor=10); > insert into t select mod((i/22),2), (i/22) from generate_series(0,1000) > S(i); > create index on t(a); > vacuum analyze t; > > set enable_indexonlyscan = off; > set enable_seqscan = off; > explain (analyze, verbose) select 1 from (values (1)) s(x) where exists > (select * from t where a = x); > > KABOOM! FWIW, it seems to me that this assert could be triggered in cases where, during a join, not all inner tuples need to be scanned before skipping to next outer tuple. This can happen for 'single_match' or anti-join. The query provided by Tomas is an example of 'single_match' case. Here is a query for anti-join that can also trigger this assert. explain (analyze, verbose) select t1.a from t t1 left join t t2 on t2.a = 1 where t2.a is null; server closed the connection unexpectedly Thanks Richard ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-18 07:10 Michael Paquier <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Michael Paquier @ 2024-04-18 07:10 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Sun, Apr 07, 2024 at 10:54:56AM -0400, Melanie Plageman wrote: > I've added an open item [1], because what's one open item when you can > have two? (me) And this is still an open item as of today. What's the plan to move forward here? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-18 09:39 Tomas Vondra <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Tomas Vondra @ 2024-04-18 09:39 UTC (permalink / raw) To: Michael Paquier <[email protected]>; Melanie Plageman <[email protected]>; +Cc: Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On 4/18/24 09:10, Michael Paquier wrote: > On Sun, Apr 07, 2024 at 10:54:56AM -0400, Melanie Plageman wrote: >> I've added an open item [1], because what's one open item when you can >> have two? (me) > > And this is still an open item as of today. What's the plan to move > forward here? AFAIK the plan is to replace the asserts with actually resetting the rs_empty_tuples_pending field to 0, as suggested by Melanie a week ago. I assume she was busy with the post-freeze AM reworks last week, so this was on a back burner. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-22 17:01 Melanie Plageman <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Melanie Plageman @ 2024-04-22 17:01 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Thu, Apr 18, 2024 at 5:39 AM Tomas Vondra <[email protected]> wrote: > > On 4/18/24 09:10, Michael Paquier wrote: > > On Sun, Apr 07, 2024 at 10:54:56AM -0400, Melanie Plageman wrote: > >> I've added an open item [1], because what's one open item when you can > >> have two? (me) > > > > And this is still an open item as of today. What's the plan to move > > forward here? > > AFAIK the plan is to replace the asserts with actually resetting the > rs_empty_tuples_pending field to 0, as suggested by Melanie a week ago. > I assume she was busy with the post-freeze AM reworks last week, so this > was on a back burner. yep, sorry. Also I took a few days off. I'm just catching up today. I want to pop in one of Richard or Tomas' examples as a test, since it seems like it would add some coverage. I will have a patch soon. - Melanie ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-23 16:05 Melanie Plageman <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Melanie Plageman @ 2024-04-23 16:05 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Mon, Apr 22, 2024 at 1:01 PM Melanie Plageman <[email protected]> wrote: > > On Thu, Apr 18, 2024 at 5:39 AM Tomas Vondra > <[email protected]> wrote: > > > > On 4/18/24 09:10, Michael Paquier wrote: > > > On Sun, Apr 07, 2024 at 10:54:56AM -0400, Melanie Plageman wrote: > > >> I've added an open item [1], because what's one open item when you can > > >> have two? (me) > > > > > > And this is still an open item as of today. What's the plan to move > > > forward here? > > > > AFAIK the plan is to replace the asserts with actually resetting the > > rs_empty_tuples_pending field to 0, as suggested by Melanie a week ago. > > I assume she was busy with the post-freeze AM reworks last week, so this > > was on a back burner. > > yep, sorry. Also I took a few days off. I'm just catching up today. I > want to pop in one of Richard or Tomas' examples as a test, since it > seems like it would add some coverage. I will have a patch soon. The patch with a fix is attached. I put the test in src/test/regress/sql/join.sql. It isn't the perfect location because it is testing something exercisable with a join but not directly related to the fact that it is a join. I also considered src/test/regress/sql/select.sql, but it also isn't directly related to the query being a SELECT query. If there is a better place for a test of a bitmap heap scan edge case, let me know. One other note: there is some concurrency effect in the parallel schedule group containing "join" where you won't trip the assert if all the tests in that group in the parallel schedule are run. But, if you would like to verify that the test exercises the correct code, just reduce the group containing "join". - Melanie Attachments: [text/x-patch] v1-0001-BitmapHeapScan-Remove-incorrect-assert.patch (4.7K, ../../CAAKRu_YVQdGVYFAu7ng84GvHWXoSvpBtz+xktm1nUB7H48Pj-Q@mail.gmail.com/2-v1-0001-BitmapHeapScan-Remove-incorrect-assert.patch) download | inline diff: From 6ad777979c335f6cc16d3936defb634176a44995 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Tue, 23 Apr 2024 11:45:37 -0400 Subject: [PATCH v1] BitmapHeapScan: Remove incorrect assert 04e72ed617be pushed the skip fetch optimization (allowing bitmap heap scans to operate like index-only scans if none of the underlying data is needed) into heap AM-specific bitmap heap scan code. 04e72ed617be added an assert that all tuples in blocks eligible for the optimization had been NULL-filled and emitted by the end of the scan. This assert is incorrect when not all tuples need be scanned to execute the query; for example: a join in which not all inner tuples need to be scanned before skipping to the next outer tuple. Author: Melanie Plageman Reviewed-by: Richard Guo, Tomas Vondra Discussion: https://postgr.es/m/CAMbWs48orzZVXa7-vP9Nt7vQWLTE04Qy4PePaLQYsVNQgo6qRg%40mail.gmail.com --- src/backend/access/heap/heapam.c | 4 ++-- src/test/regress/expected/join.out | 36 ++++++++++++++++++++++++++++++ src/test/regress/sql/join.sql | 24 ++++++++++++++++++++ 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 4a4cf76269d..8906f161320 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1184,7 +1184,7 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params, scan->rs_vmbuffer = InvalidBuffer; } - Assert(scan->rs_empty_tuples_pending == 0); + scan->rs_empty_tuples_pending = 0; /* * The read stream is reset on rescan. This must be done before @@ -1216,7 +1216,7 @@ heap_endscan(TableScanDesc sscan) if (BufferIsValid(scan->rs_vmbuffer)) ReleaseBuffer(scan->rs_vmbuffer); - Assert(scan->rs_empty_tuples_pending == 0); + scan->rs_empty_tuples_pending = 0; /* * Must free the read stream before freeing the BufferAccessStrategy. diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 8b640c2fc2f..ce73939c267 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -8956,3 +8956,39 @@ where exists (select 1 from j3 (13 rows) drop table j3; +-- Check the case when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. +CREATE TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM GENERATE_SERIES(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; +SET enable_indexonlyscan = off; +set enable_indexscan = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + QUERY PLAN +--------------------------------------------------------- + Nested Loop Anti Join + -> Seq Scan on skip_fetch t1 + -> Materialize + -> Bitmap Heap Scan on skip_fetch t2 + Recheck Cond: (a = 1) + -> Bitmap Index Scan on skip_fetch_a_idx + Index Cond: (a = 1) +(7 rows) + +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + a +--- +(0 rows) + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_seqscan; +DROP TABLE skip_fetch; diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index c4c6c7b8ba2..700631cd938 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -3374,3 +3374,27 @@ where exists (select 1 from j3 and t1.unique1 < 1; drop table j3; + +-- Check the case when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. +CREATE TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM GENERATE_SERIES(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; + +SET enable_indexonlyscan = off; +set enable_indexscan = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_seqscan; +DROP TABLE skip_fetch; -- 2.40.1 ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-23 22:43 Tomas Vondra <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Tomas Vondra @ 2024-04-23 22:43 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On 4/23/24 18:05, Melanie Plageman wrote: > On Mon, Apr 22, 2024 at 1:01 PM Melanie Plageman > <[email protected]> wrote: >> >> On Thu, Apr 18, 2024 at 5:39 AM Tomas Vondra >> <[email protected]> wrote: >>> >>> On 4/18/24 09:10, Michael Paquier wrote: >>>> On Sun, Apr 07, 2024 at 10:54:56AM -0400, Melanie Plageman wrote: >>>>> I've added an open item [1], because what's one open item when you can >>>>> have two? (me) >>>> >>>> And this is still an open item as of today. What's the plan to move >>>> forward here? >>> >>> AFAIK the plan is to replace the asserts with actually resetting the >>> rs_empty_tuples_pending field to 0, as suggested by Melanie a week ago. >>> I assume she was busy with the post-freeze AM reworks last week, so this >>> was on a back burner. >> >> yep, sorry. Also I took a few days off. I'm just catching up today. I >> want to pop in one of Richard or Tomas' examples as a test, since it >> seems like it would add some coverage. I will have a patch soon. > > The patch with a fix is attached. I put the test in > src/test/regress/sql/join.sql. It isn't the perfect location because > it is testing something exercisable with a join but not directly > related to the fact that it is a join. I also considered > src/test/regress/sql/select.sql, but it also isn't directly related to > the query being a SELECT query. If there is a better place for a test > of a bitmap heap scan edge case, let me know. > I don't see a problem with adding this to join.sql - why wouldn't this count as something related to a join? Sure, it's not like this code matters only for joins, but if you look at join.sql that applies to a number of other tests (e.g. there are a couple btree tests). That being said, it'd be good to explain in the comment why we're testing this particular plan, not just what the plan looks like. > One other note: there is some concurrency effect in the parallel > schedule group containing "join" where you won't trip the assert if > all the tests in that group in the parallel schedule are run. But, if > you would like to verify that the test exercises the correct code, > just reduce the group containing "join". > That is ... interesting. Doesn't that mean that most test runs won't actually detect the problem? That would make the test a bit useless. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-24 20:46 Melanie Plageman <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 2 replies; 28+ messages in thread From: Melanie Plageman @ 2024-04-24 20:46 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Tue, Apr 23, 2024 at 6:43 PM Tomas Vondra <[email protected]> wrote: > > On 4/23/24 18:05, Melanie Plageman wrote: > > The patch with a fix is attached. I put the test in > > src/test/regress/sql/join.sql. It isn't the perfect location because > > it is testing something exercisable with a join but not directly > > related to the fact that it is a join. I also considered > > src/test/regress/sql/select.sql, but it also isn't directly related to > > the query being a SELECT query. If there is a better place for a test > > of a bitmap heap scan edge case, let me know. > > I don't see a problem with adding this to join.sql - why wouldn't this > count as something related to a join? Sure, it's not like this code > matters only for joins, but if you look at join.sql that applies to a > number of other tests (e.g. there are a couple btree tests). I suppose it's true that other tests in this file use joins to test other code. I guess if we limited join.sql to containing tests of join implementation, it would be rather small. I just imagined it would be nice if tests were grouped by what they were testing -- not how they were testing it. > That being said, it'd be good to explain in the comment why we're > testing this particular plan, not just what the plan looks like. You mean I should explain in the test comment why I included the EXPLAIN plan output? (because it needs to contain a bitmapheapscan to actually be testing anything) I do have a detailed explanation in my test comment of why this particular query exercises the code we want to test. > > One other note: there is some concurrency effect in the parallel > > schedule group containing "join" where you won't trip the assert if > > all the tests in that group in the parallel schedule are run. But, if > > you would like to verify that the test exercises the correct code, > > just reduce the group containing "join". > > > > That is ... interesting. Doesn't that mean that most test runs won't > actually detect the problem? That would make the test a bit useless. Yes, I should really have thought about it more. After further investigation, I found that the reason it doesn't trip the assert when the join test is run concurrently with other tests is that the SELECT query doesn't use the skip fetch optimization because the VACUUM doesn't set the pages all visible in the VM. In this case, it's because the tuples' xmins are not before VacuumCutoffs->OldestXmin (which is derived from GetOldestNonRemovableTransactionId()). After thinking about it more, I suppose we can't add a test that relies on the relation being all visible in the VM in a group in the parallel schedule. I'm not sure this edge case is important enough to merit its own group or an isolation test. What do you think? - Melanie ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-25 23:03 Melanie Plageman <[email protected]> parent: Melanie Plageman <[email protected]> 1 sibling, 2 replies; 28+ messages in thread From: Melanie Plageman @ 2024-04-25 23:03 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Wed, Apr 24, 2024 at 4:46 PM Melanie Plageman <[email protected]> wrote: > > On Tue, Apr 23, 2024 at 6:43 PM Tomas Vondra > <[email protected]> wrote: > > > > On 4/23/24 18:05, Melanie Plageman wrote: > > > One other note: there is some concurrency effect in the parallel > > > schedule group containing "join" where you won't trip the assert if > > > all the tests in that group in the parallel schedule are run. But, if > > > you would like to verify that the test exercises the correct code, > > > just reduce the group containing "join". > > > > > > > That is ... interesting. Doesn't that mean that most test runs won't > > actually detect the problem? That would make the test a bit useless. > > Yes, I should really have thought about it more. After further > investigation, I found that the reason it doesn't trip the assert when > the join test is run concurrently with other tests is that the SELECT > query doesn't use the skip fetch optimization because the VACUUM > doesn't set the pages all visible in the VM. In this case, it's > because the tuples' xmins are not before VacuumCutoffs->OldestXmin > (which is derived from GetOldestNonRemovableTransactionId()). > > After thinking about it more, I suppose we can't add a test that > relies on the relation being all visible in the VM in a group in the > parallel schedule. I'm not sure this edge case is important enough to > merit its own group or an isolation test. What do you think? Andres rightly pointed out to me off-list that if I just used a temp table, the table would only be visible to the testing backend anyway. I've done that in the attached v2. Now the test is deterministic. - Melanie Attachments: [text/x-patch] v2-0001-BitmapHeapScan-Remove-incorrect-assert.patch (5.0K, ../../CAAKRu_atCg2KXyTOzbDqq8FZNPDZj7sn6YvkeL=dzn=9d4Eaug@mail.gmail.com/2-v2-0001-BitmapHeapScan-Remove-incorrect-assert.patch) download | inline diff: From c5f28d12f75f5af84ede0db563fc5c0b53295c65 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Thu, 25 Apr 2024 18:50:14 -0400 Subject: [PATCH v2] BitmapHeapScan: Remove incorrect assert 04e72ed617be pushed the skip fetch optimization (allowing bitmap heap scans to operate like index-only scans if none of the underlying data is needed) into heap AM-specific bitmap heap scan code. 04e72ed617be added an assert that all tuples in blocks eligible for the optimization had been NULL-filled and emitted by the end of the scan. This assert is incorrect when not all tuples need be scanned to execute the query; for example: a join in which not all inner tuples need to be scanned before skipping to the next outer tuple. Author: Melanie Plageman Reviewed-by: Richard Guo, Tomas Vondra Discussion: https://postgr.es/m/CAMbWs48orzZVXa7-vP9Nt7vQWLTE04Qy4PePaLQYsVNQgo6qRg%40mail.gmail.com --- src/backend/access/heap/heapam.c | 4 ++-- src/test/regress/expected/join.out | 37 ++++++++++++++++++++++++++++++ src/test/regress/sql/join.sql | 25 ++++++++++++++++++++ 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 4a4cf76269d..8906f161320 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1184,7 +1184,7 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params, scan->rs_vmbuffer = InvalidBuffer; } - Assert(scan->rs_empty_tuples_pending == 0); + scan->rs_empty_tuples_pending = 0; /* * The read stream is reset on rescan. This must be done before @@ -1216,7 +1216,7 @@ heap_endscan(TableScanDesc sscan) if (BufferIsValid(scan->rs_vmbuffer)) ReleaseBuffer(scan->rs_vmbuffer); - Assert(scan->rs_empty_tuples_pending == 0); + scan->rs_empty_tuples_pending = 0; /* * Must free the read stream before freeing the BufferAccessStrategy. diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 8b640c2fc2f..4f0292c7285 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -8956,3 +8956,40 @@ where exists (select 1 from j3 (13 rows) drop table j3; +-- Check the case when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. Use a temp table so it is only visible to this backend and +-- vacuum may reliably mark all blocks in the table all visible in the +-- visibility map. +CREATE TEMP TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM generate_series(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; +SET enable_indexonlyscan = off; +set enable_indexscan = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + QUERY PLAN +--------------------------------------------------------- + Nested Loop Anti Join + -> Seq Scan on skip_fetch t1 + -> Materialize + -> Bitmap Heap Scan on skip_fetch t2 + Recheck Cond: (a = 1) + -> Bitmap Index Scan on skip_fetch_a_idx + Index Cond: (a = 1) +(7 rows) + +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + a +--- +(0 rows) + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_seqscan; diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index c4c6c7b8ba2..25743ec972a 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -3374,3 +3374,28 @@ where exists (select 1 from j3 and t1.unique1 < 1; drop table j3; + +-- Check the case when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. Use a temp table so it is only visible to this backend and +-- vacuum may reliably mark all blocks in the table all visible in the +-- visibility map. +CREATE TEMP TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM generate_series(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; + +SET enable_indexonlyscan = off; +set enable_indexscan = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_seqscan; -- 2.40.1 ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-25 23:28 Tom Lane <[email protected]> parent: Melanie Plageman <[email protected]> 1 sibling, 0 replies; 28+ messages in thread From: Tom Lane @ 2024-04-25 23:28 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> Melanie Plageman <[email protected]> writes: > On Wed, Apr 24, 2024 at 4:46 PM Melanie Plageman > <[email protected]> wrote: >> After thinking about it more, I suppose we can't add a test that >> relies on the relation being all visible in the VM in a group in the >> parallel schedule. I'm not sure this edge case is important enough to >> merit its own group or an isolation test. What do you think? > Andres rightly pointed out to me off-list that if I just used a temp > table, the table would only be visible to the testing backend anyway. > I've done that in the attached v2. Now the test is deterministic. Hmm, is that actually true? There's no more reason to think a tuple in a temp table is old enough to be visible to all other sessions than one in any other table. It could be all right if we had a special-case rule for setting all-visible in temp tables. Which indeed I thought we had, but I can't find any evidence of that in vacuumlazy.c, nor did a trawl of the commit log turn up anything promising. Am I just looking in the wrong place? regards, tom lane ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-25 23:57 Tom Lane <[email protected]> parent: Melanie Plageman <[email protected]> 1 sibling, 1 reply; 28+ messages in thread From: Tom Lane @ 2024-04-25 23:57 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> I wrote: > Hmm, is that actually true? There's no more reason to think a tuple > in a temp table is old enough to be visible to all other sessions > than one in any other table. It could be all right if we had a > special-case rule for setting all-visible in temp tables. Which > indeed I thought we had, but I can't find any evidence of that in > vacuumlazy.c, nor did a trawl of the commit log turn up anything > promising. Am I just looking in the wrong place? Ah, never mind that --- I must be looking in the wrong place. Direct experimentation proves that VACUUM will set all-visible bits for temp tables even in the presence of concurrent transactions. regards, tom lane ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-26 13:04 Melanie Plageman <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Melanie Plageman @ 2024-04-26 13:04 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Thu, Apr 25, 2024 at 7:57 PM Tom Lane <[email protected]> wrote: > > I wrote: > > Hmm, is that actually true? There's no more reason to think a tuple > > in a temp table is old enough to be visible to all other sessions > > than one in any other table. It could be all right if we had a > > special-case rule for setting all-visible in temp tables. Which > > indeed I thought we had, but I can't find any evidence of that in > > vacuumlazy.c, nor did a trawl of the commit log turn up anything > > promising. Am I just looking in the wrong place? > > Ah, never mind that --- I must be looking in the wrong place. > Direct experimentation proves that VACUUM will set all-visible bits > for temp tables even in the presence of concurrent transactions. If this seems correct to you, are you okay with the rest of the fix and test? We could close this open item once the patch is acceptable. - Melanie ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-04-30 12:07 Daniel Gustafsson <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Daniel Gustafsson @ 2024-04-30 12:07 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Tom Lane <[email protected]>; Tomas Vondra <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> > On 26 Apr 2024, at 15:04, Melanie Plageman <[email protected]> wrote: > If this seems correct to you, are you okay with the rest of the fix > and test? We could close this open item once the patch is acceptable. From reading the discussion and the patch this seems like the right fix to me. Does the test added here aptly cover 04e72ed617be in terms its functionality? -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-05-02 21:31 Tomas Vondra <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 0 replies; 28+ messages in thread From: Tomas Vondra @ 2024-05-02 21:31 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; Melanie Plageman <[email protected]>; +Cc: Tom Lane <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On 4/30/24 14:07, Daniel Gustafsson wrote: >> On 26 Apr 2024, at 15:04, Melanie Plageman <[email protected]> wrote: > >> If this seems correct to you, are you okay with the rest of the fix >> and test? We could close this open item once the patch is acceptable. > > From reading the discussion and the patch this seems like the right fix to me. I agree. > Does the test added here aptly cover 04e72ed617be in terms its functionality? > AFAIK the test fails without the fix and works with it, so I believe it does cover the relevant functionality. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-05-02 21:37 Tomas Vondra <[email protected]> parent: Melanie Plageman <[email protected]> 1 sibling, 1 reply; 28+ messages in thread From: Tomas Vondra @ 2024-05-02 21:37 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On 4/24/24 22:46, Melanie Plageman wrote: > On Tue, Apr 23, 2024 at 6:43 PM Tomas Vondra > <[email protected]> wrote: >> >> On 4/23/24 18:05, Melanie Plageman wrote: >>> The patch with a fix is attached. I put the test in >>> src/test/regress/sql/join.sql. It isn't the perfect location because >>> it is testing something exercisable with a join but not directly >>> related to the fact that it is a join. I also considered >>> src/test/regress/sql/select.sql, but it also isn't directly related to >>> the query being a SELECT query. If there is a better place for a test >>> of a bitmap heap scan edge case, let me know. >> >> I don't see a problem with adding this to join.sql - why wouldn't this >> count as something related to a join? Sure, it's not like this code >> matters only for joins, but if you look at join.sql that applies to a >> number of other tests (e.g. there are a couple btree tests). > > I suppose it's true that other tests in this file use joins to test > other code. I guess if we limited join.sql to containing tests of join > implementation, it would be rather small. I just imagined it would be > nice if tests were grouped by what they were testing -- not how they > were testing it. > >> That being said, it'd be good to explain in the comment why we're >> testing this particular plan, not just what the plan looks like. > > You mean I should explain in the test comment why I included the > EXPLAIN plan output? (because it needs to contain a bitmapheapscan to > actually be testing anything) > No, I meant that the comment before the test describes a couple requirements the plan needs to meet (no need to process all inner tuples, bitmapscan eligible for skip_fetch on outer side, ...), but it does not explain why we're testing that plan. I could get to that by doing git-blame to see what commit added this code, and then read the linked discussion. Perhaps that's enough, but maybe the comment could say something like "verify we properly discard tuples on rescans" or something like that? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-05-10 19:48 Melanie Plageman <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Melanie Plageman @ 2024-05-10 19:48 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Thu, May 2, 2024 at 5:37 PM Tomas Vondra <[email protected]> wrote: > > > > On 4/24/24 22:46, Melanie Plageman wrote: > > On Tue, Apr 23, 2024 at 6:43 PM Tomas Vondra > > <[email protected]> wrote: > >> > >> On 4/23/24 18:05, Melanie Plageman wrote: > >>> The patch with a fix is attached. I put the test in > >>> src/test/regress/sql/join.sql. It isn't the perfect location because > >>> it is testing something exercisable with a join but not directly > >>> related to the fact that it is a join. I also considered > >>> src/test/regress/sql/select.sql, but it also isn't directly related to > >>> the query being a SELECT query. If there is a better place for a test > >>> of a bitmap heap scan edge case, let me know. > >> > >> I don't see a problem with adding this to join.sql - why wouldn't this > >> count as something related to a join? Sure, it's not like this code > >> matters only for joins, but if you look at join.sql that applies to a > >> number of other tests (e.g. there are a couple btree tests). > > > > I suppose it's true that other tests in this file use joins to test > > other code. I guess if we limited join.sql to containing tests of join > > implementation, it would be rather small. I just imagined it would be > > nice if tests were grouped by what they were testing -- not how they > > were testing it. > > > >> That being said, it'd be good to explain in the comment why we're > >> testing this particular plan, not just what the plan looks like. > > > > You mean I should explain in the test comment why I included the > > EXPLAIN plan output? (because it needs to contain a bitmapheapscan to > > actually be testing anything) > > > > No, I meant that the comment before the test describes a couple > requirements the plan needs to meet (no need to process all inner > tuples, bitmapscan eligible for skip_fetch on outer side, ...), but it > does not explain why we're testing that plan. > > I could get to that by doing git-blame to see what commit added this > code, and then read the linked discussion. Perhaps that's enough, but > maybe the comment could say something like "verify we properly discard > tuples on rescans" or something like that? Attached is v3. I didn't use your exact language because the test wouldn't actually verify that we properly discard the tuples. Whether or not the empty tuples are all emitted, it just resets the counter to 0. I decided to go with "exercise" instead. - Melanie Attachments: [text/x-patch] v3-0001-BitmapHeapScan-Remove-incorrect-assert.patch (5.2K, ../../CAAKRu_amE9MxcXSmHfjLQkx4CtoqHyAJPvv3_iY3gfR6ZHZedg@mail.gmail.com/2-v3-0001-BitmapHeapScan-Remove-incorrect-assert.patch) download | inline diff: From 0f10d304fc7dcce99622f348183f36a8062e70c6 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Fri, 10 May 2024 14:52:34 -0400 Subject: [PATCH v3] BitmapHeapScan: Remove incorrect assert 04e72ed617be pushed the skip fetch optimization (allowing bitmap heap scans to operate like index-only scans if none of the underlying data is needed) into heap AM-specific bitmap heap scan code. 04e72ed617be added an assert that all tuples in blocks eligible for the optimization had been NULL-filled and emitted by the end of the scan. This assert is incorrect when not all tuples need be scanned to execute the query; for example: a join in which not all inner tuples need to be scanned before skipping to the next outer tuple. Author: Melanie Plageman Reviewed-by: Richard Guo, Tomas Vondra Discussion: https://postgr.es/m/CAMbWs48orzZVXa7-vP9Nt7vQWLTE04Qy4PePaLQYsVNQgo6qRg%40mail.gmail.com --- src/backend/access/heap/heapam.c | 4 ++-- src/test/regress/expected/join.out | 38 ++++++++++++++++++++++++++++++ src/test/regress/sql/join.sql | 26 ++++++++++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 4be0dee4de..8600c22515 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1184,7 +1184,7 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params, scan->rs_vmbuffer = InvalidBuffer; } - Assert(scan->rs_empty_tuples_pending == 0); + scan->rs_empty_tuples_pending = 0; /* * The read stream is reset on rescan. This must be done before @@ -1216,7 +1216,7 @@ heap_endscan(TableScanDesc sscan) if (BufferIsValid(scan->rs_vmbuffer)) ReleaseBuffer(scan->rs_vmbuffer); - Assert(scan->rs_empty_tuples_pending == 0); + scan->rs_empty_tuples_pending = 0; /* * Must free the read stream before freeing the BufferAccessStrategy. diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 0246d56aea..8829bd76e7 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -7924,3 +7924,41 @@ where exists (select 1 from j3 (13 rows) drop table j3; +-- Exercise the "skip fetch" Bitmap Heap Scan optimization when candidate +-- tuples are discarded. This may occur when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. Use a temp table so it is only visible to this backend and +-- vacuum may reliably mark all blocks in the table all visible in the +-- visibility map. +CREATE TEMP TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM generate_series(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; +SET enable_indexonlyscan = off; +set enable_indexscan = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + QUERY PLAN +--------------------------------------------------------- + Nested Loop Anti Join + -> Seq Scan on skip_fetch t1 + -> Materialize + -> Bitmap Heap Scan on skip_fetch t2 + Recheck Cond: (a = 1) + -> Bitmap Index Scan on skip_fetch_a_idx + Index Cond: (a = 1) +(7 rows) + +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + a +--- +(0 rows) + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_seqscan; diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 923e7c5549..b73ce67bd2 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2904,3 +2904,29 @@ where exists (select 1 from j3 and t1.unique1 < 1; drop table j3; + +-- Exercise the "skip fetch" Bitmap Heap Scan optimization when candidate +-- tuples are discarded. This may occur when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. Use a temp table so it is only visible to this backend and +-- vacuum may reliably mark all blocks in the table all visible in the +-- visibility map. +CREATE TEMP TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM generate_series(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; + +SET enable_indexonlyscan = off; +set enable_indexscan = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_seqscan; -- 2.34.1 ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-05-11 19:18 Tomas Vondra <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Tomas Vondra @ 2024-05-11 19:18 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On 5/10/24 21:48, Melanie Plageman wrote: > On Thu, May 2, 2024 at 5:37 PM Tomas Vondra > <[email protected]> wrote: >> >> >> >> On 4/24/24 22:46, Melanie Plageman wrote: >>> On Tue, Apr 23, 2024 at 6:43 PM Tomas Vondra >>> <[email protected]> wrote: >>>> >>>> On 4/23/24 18:05, Melanie Plageman wrote: >>>>> The patch with a fix is attached. I put the test in >>>>> src/test/regress/sql/join.sql. It isn't the perfect location because >>>>> it is testing something exercisable with a join but not directly >>>>> related to the fact that it is a join. I also considered >>>>> src/test/regress/sql/select.sql, but it also isn't directly related to >>>>> the query being a SELECT query. If there is a better place for a test >>>>> of a bitmap heap scan edge case, let me know. >>>> >>>> I don't see a problem with adding this to join.sql - why wouldn't this >>>> count as something related to a join? Sure, it's not like this code >>>> matters only for joins, but if you look at join.sql that applies to a >>>> number of other tests (e.g. there are a couple btree tests). >>> >>> I suppose it's true that other tests in this file use joins to test >>> other code. I guess if we limited join.sql to containing tests of join >>> implementation, it would be rather small. I just imagined it would be >>> nice if tests were grouped by what they were testing -- not how they >>> were testing it. >>> >>>> That being said, it'd be good to explain in the comment why we're >>>> testing this particular plan, not just what the plan looks like. >>> >>> You mean I should explain in the test comment why I included the >>> EXPLAIN plan output? (because it needs to contain a bitmapheapscan to >>> actually be testing anything) >>> >> >> No, I meant that the comment before the test describes a couple >> requirements the plan needs to meet (no need to process all inner >> tuples, bitmapscan eligible for skip_fetch on outer side, ...), but it >> does not explain why we're testing that plan. >> >> I could get to that by doing git-blame to see what commit added this >> code, and then read the linked discussion. Perhaps that's enough, but >> maybe the comment could say something like "verify we properly discard >> tuples on rescans" or something like that? > > Attached is v3. I didn't use your exact language because the test > wouldn't actually verify that we properly discard the tuples. Whether > or not the empty tuples are all emitted, it just resets the counter to > 0. I decided to go with "exercise" instead. > I did go over the v3 patch, did a bunch of tests, and I think it's fine and ready to go. The one thing that might need some minor tweaks is the commit message. 1) Isn't the subject "Remove incorrect assert" a bit misleading, as the patch does not simply remove an assert, but replaces it with a reset of the field the assert used to check? (The commit message does not mention this either, at least not explicitly.) 2) The "heap AM-specific bitmap heap scan code" sounds a bit strange to me, isn't the first "heap" unnecessary? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-05-13 14:05 Melanie Plageman <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Melanie Plageman @ 2024-05-13 14:05 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Sat, May 11, 2024 at 3:18 PM Tomas Vondra <[email protected]> wrote: > > On 5/10/24 21:48, Melanie Plageman wrote: > > Attached is v3. I didn't use your exact language because the test > > wouldn't actually verify that we properly discard the tuples. Whether > > or not the empty tuples are all emitted, it just resets the counter to > > 0. I decided to go with "exercise" instead. > > > > I did go over the v3 patch, did a bunch of tests, and I think it's fine > and ready to go. The one thing that might need some minor tweaks is the > commit message. > > 1) Isn't the subject "Remove incorrect assert" a bit misleading, as the > patch does not simply remove an assert, but replaces it with a reset of > the field the assert used to check? (The commit message does not mention > this either, at least not explicitly.) I've updated the commit message. > 2) The "heap AM-specific bitmap heap scan code" sounds a bit strange to > me, isn't the first "heap" unnecessary? bitmap heap scan has been used to refer to bitmap table scans, as the name wasn't changed from heap when the table AM API was added (e.g. BitmapHeapNext() is used by all table AMs doing bitmap table scans). 04e72ed617be specifically pushed the skip fetch optimization into heap implementations of bitmap table scan callbacks, so it was important to make this distinction. I've changed the commit message to say heap AM implementations of bitmap table scan callbacks. While looking at the patch again, I wondered if I should set enable_material=false in the test. It doesn't matter from the perspective of exercising the correct code; however, I wasn't sure if disabling materialization would make the test more resilient against future planner changes which could cause it to incorrectly fail. - Melanie Attachments: [text/x-patch] v4-0001-BitmapHeapScan-Replace-incorrect-assert-with-rein.patch (5.4K, ../../CAAKRu_a+5foybidkmh8FpFAV7iegxetPyPXQ5=++kqZ+ZDEUcg@mail.gmail.com/2-v4-0001-BitmapHeapScan-Replace-incorrect-assert-with-rein.patch) download | inline diff: From d3628d8d36b2be54b64c18402bdda80e1c8a436f Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Fri, 10 May 2024 14:52:34 -0400 Subject: [PATCH v4] BitmapHeapScan: Replace incorrect assert with reinitialization 04e72ed617be pushed the skip fetch optimization (allowing bitmap heap scans to operate like index-only scans if none of the underlying data is needed) into heap AM implementations of bitmap table scan callbacks. 04e72ed617be added an assert that all tuples in blocks eligible for the optimization had been NULL-filled and emitted by the end of the scan. This assert is incorrect when not all tuples need be scanned to execute the query; for example: a join in which not all inner tuples need to be scanned before skipping to the next outer tuple. Remove the assert and reset the field on which it previously asserted to avoid incorrectly emitting NULL-filled tuples from a previous scan on rescan. Author: Melanie Plageman Reviewed-by: Tomas Vondra Reported-by: Melanie Plageman Reproduced-by: Tomas Vondra, Richard Guo Discussion: https://postgr.es/m/CAMbWs48orzZVXa7-vP9Nt7vQWLTE04Qy4PePaLQYsVNQgo6qRg%40mail.gmail.com --- src/backend/access/heap/heapam.c | 4 ++-- src/test/regress/expected/join.out | 38 ++++++++++++++++++++++++++++++ src/test/regress/sql/join.sql | 26 ++++++++++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 4be0dee4de..8600c22515 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1184,7 +1184,7 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params, scan->rs_vmbuffer = InvalidBuffer; } - Assert(scan->rs_empty_tuples_pending == 0); + scan->rs_empty_tuples_pending = 0; /* * The read stream is reset on rescan. This must be done before @@ -1216,7 +1216,7 @@ heap_endscan(TableScanDesc sscan) if (BufferIsValid(scan->rs_vmbuffer)) ReleaseBuffer(scan->rs_vmbuffer); - Assert(scan->rs_empty_tuples_pending == 0); + scan->rs_empty_tuples_pending = 0; /* * Must free the read stream before freeing the BufferAccessStrategy. diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 0246d56aea..8829bd76e7 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -7924,3 +7924,41 @@ where exists (select 1 from j3 (13 rows) drop table j3; +-- Exercise the "skip fetch" Bitmap Heap Scan optimization when candidate +-- tuples are discarded. This may occur when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. Use a temp table so it is only visible to this backend and +-- vacuum may reliably mark all blocks in the table all visible in the +-- visibility map. +CREATE TEMP TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM generate_series(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; +SET enable_indexonlyscan = off; +set enable_indexscan = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + QUERY PLAN +--------------------------------------------------------- + Nested Loop Anti Join + -> Seq Scan on skip_fetch t1 + -> Materialize + -> Bitmap Heap Scan on skip_fetch t2 + Recheck Cond: (a = 1) + -> Bitmap Index Scan on skip_fetch_a_idx + Index Cond: (a = 1) +(7 rows) + +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + a +--- +(0 rows) + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_seqscan; diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 923e7c5549..b73ce67bd2 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2904,3 +2904,29 @@ where exists (select 1 from j3 and t1.unique1 < 1; drop table j3; + +-- Exercise the "skip fetch" Bitmap Heap Scan optimization when candidate +-- tuples are discarded. This may occur when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. Use a temp table so it is only visible to this backend and +-- vacuum may reliably mark all blocks in the table all visible in the +-- visibility map. +CREATE TEMP TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM generate_series(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; + +SET enable_indexonlyscan = off; +set enable_indexscan = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_seqscan; -- 2.34.1 ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-05-14 06:18 Michael Paquier <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Michael Paquier @ 2024-05-14 06:18 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Mon, May 13, 2024 at 10:05:03AM -0400, Melanie Plageman wrote: > Remove the assert and reset the field on which it previously asserted to > avoid incorrectly emitting NULL-filled tuples from a previous scan on > rescan. > - Assert(scan->rs_empty_tuples_pending == 0); > + scan->rs_empty_tuples_pending = 0; Perhaps this should document the reason why the reset is done in these two paths rather than let the reader guess it? And this is about avoiding emitting some tuples from a previous scan. > +SET enable_indexonlyscan = off; > +set enable_indexscan = off; > +SET enable_seqscan = off; Nit: adjusting the casing of the second SET here. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-05-14 17:42 Melanie Plageman <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Melanie Plageman @ 2024-05-14 17:42 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Tue, May 14, 2024 at 2:18 AM Michael Paquier <[email protected]> wrote: > > On Mon, May 13, 2024 at 10:05:03AM -0400, Melanie Plageman wrote: > > Remove the assert and reset the field on which it previously asserted to > > avoid incorrectly emitting NULL-filled tuples from a previous scan on > > rescan. > > > - Assert(scan->rs_empty_tuples_pending == 0); > > + scan->rs_empty_tuples_pending = 0; > > Perhaps this should document the reason why the reset is done in these > two paths rather than let the reader guess it? And this is about > avoiding emitting some tuples from a previous scan. I've added a comment to heap_rescan() in the attached v5. Doing so made me realize that we shouldn't bother resetting it in heap_endscan(). Doing so is perhaps more confusing, because it implies that field may somehow be used later. I've removed the reset of rs_empty_tuples_pending from heap_endscan(). > > +SET enable_indexonlyscan = off; > > +set enable_indexscan = off; > > +SET enable_seqscan = off; > > Nit: adjusting the casing of the second SET here. I've fixed this. I've also set enable_material off as I mentioned I might in my earlier mail. - Melanie Attachments: [text/x-patch] v5-0001-BitmapHeapScan-Remove-incorrect-assert-and-reset-.patch (5.6K, ../../CAAKRu_ZwjB7b+_FS3h1_bdr4wY9xGcy47iE3o7kdHzOh3ALPfA@mail.gmail.com/2-v5-0001-BitmapHeapScan-Remove-incorrect-assert-and-reset-.patch) download | inline diff: From d44679397e3aaa043afad2108ec904f68b7052b3 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Tue, 14 May 2024 13:36:42 -0400 Subject: [PATCH v5] BitmapHeapScan: Remove incorrect assert and reset field 04e72ed617be pushed the skip fetch optimization (allowing bitmap heap scans to operate like index-only scans if none of the underlying data is needed) into heap AM implementations of bitmap table scan callbacks. 04e72ed617be added an assert that all tuples in blocks eligible for the optimization had been NULL-filled and emitted by the end of the scan. This assert is incorrect when not all tuples need be scanned to execute the query; for example: a join in which not all inner tuples need to be scanned before skipping to the next outer tuple. Remove the assert and reset the field on which it previously asserted to avoid incorrectly emitting NULL-filled tuples from a previous scan on rescan. Author: Melanie Plageman Reviewed-by: Tomas Vondra, Michael Paquier Reported-by: Melanie Plageman Reproduced-by: Tomas Vondra, Richard Guo Discussion: https://postgr.es/m/CAMbWs48orzZVXa7-vP9Nt7vQWLTE04Qy4PePaLQYsVNQgo6qRg%40mail.gmail.com --- src/backend/access/heap/heapam.c | 9 ++++--- src/test/regress/expected/join.out | 39 ++++++++++++++++++++++++++++++ src/test/regress/sql/join.sql | 28 +++++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 4be0dee4de..82bb9cb33b 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1184,7 +1184,12 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params, scan->rs_vmbuffer = InvalidBuffer; } - Assert(scan->rs_empty_tuples_pending == 0); + /* + * Reset rs_empty_tuples_pending, a field only used by bitmap heap scan, + * to avoid incorrectly emitting NULL-filled tuples from a previous scan + * on rescan. + */ + scan->rs_empty_tuples_pending = 0; /* * The read stream is reset on rescan. This must be done before @@ -1216,8 +1221,6 @@ heap_endscan(TableScanDesc sscan) if (BufferIsValid(scan->rs_vmbuffer)) ReleaseBuffer(scan->rs_vmbuffer); - Assert(scan->rs_empty_tuples_pending == 0); - /* * Must free the read stream before freeing the BufferAccessStrategy. */ diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 0246d56aea..466be7b439 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -7924,3 +7924,42 @@ where exists (select 1 from j3 (13 rows) drop table j3; +-- Exercise the "skip fetch" Bitmap Heap Scan optimization when candidate +-- tuples are discarded. This may occur when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. Use a temp table so it is only visible to this backend and +-- vacuum may reliably mark all blocks in the table all visible in the +-- visibility map. +CREATE TEMP TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM generate_series(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; +SET enable_indexonlyscan = off; +SET enable_indexscan = off; +SET enable_material = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + QUERY PLAN +--------------------------------------------------- + Nested Loop Anti Join + -> Seq Scan on skip_fetch t1 + -> Bitmap Heap Scan on skip_fetch t2 + Recheck Cond: (a = 1) + -> Bitmap Index Scan on skip_fetch_a_idx + Index Cond: (a = 1) +(6 rows) + +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + a +--- +(0 rows) + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_material; +RESET enable_seqscan; diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 923e7c5549..1bd4ed8d29 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2904,3 +2904,31 @@ where exists (select 1 from j3 and t1.unique1 < 1; drop table j3; + +-- Exercise the "skip fetch" Bitmap Heap Scan optimization when candidate +-- tuples are discarded. This may occur when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. Use a temp table so it is only visible to this backend and +-- vacuum may reliably mark all blocks in the table all visible in the +-- visibility map. +CREATE TEMP TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM generate_series(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; + +SET enable_indexonlyscan = off; +SET enable_indexscan = off; +SET enable_material = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_material; +RESET enable_seqscan; -- 2.34.1 ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-05-14 18:33 Tomas Vondra <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Tomas Vondra @ 2024-05-14 18:33 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; Michael Paquier <[email protected]>; +Cc: Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On 5/14/24 19:42, Melanie Plageman wrote: > On Tue, May 14, 2024 at 2:18 AM Michael Paquier <[email protected]> wrote: >> >> On Mon, May 13, 2024 at 10:05:03AM -0400, Melanie Plageman wrote: >>> Remove the assert and reset the field on which it previously asserted to >>> avoid incorrectly emitting NULL-filled tuples from a previous scan on >>> rescan. >> >>> - Assert(scan->rs_empty_tuples_pending == 0); >>> + scan->rs_empty_tuples_pending = 0; >> >> Perhaps this should document the reason why the reset is done in these >> two paths rather than let the reader guess it? And this is about >> avoiding emitting some tuples from a previous scan. > > I've added a comment to heap_rescan() in the attached v5. Doing so > made me realize that we shouldn't bother resetting it in > heap_endscan(). Doing so is perhaps more confusing, because it implies > that field may somehow be used later. I've removed the reset of > rs_empty_tuples_pending from heap_endscan(). > +1 >>> +SET enable_indexonlyscan = off; >>> +set enable_indexscan = off; >>> +SET enable_seqscan = off; >> >> Nit: adjusting the casing of the second SET here. > > I've fixed this. I've also set enable_material off as I mentioned I > might in my earlier mail. > I'm not sure this (setting more and more GUCs to prevent hypothetical plan changes) is a good practice. Because how do you know the plan does not change for some other unexpected reason, possibly in the future? IMHO if the test requires a specific plan, it's better to do an actual "explain (rows off, costs off)" to check that. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-05-14 18:40 Melanie Plageman <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Melanie Plageman @ 2024-05-14 18:40 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Tue, May 14, 2024 at 2:33 PM Tomas Vondra <[email protected]> wrote: > > On 5/14/24 19:42, Melanie Plageman wrote: > > I've fixed this. I've also set enable_material off as I mentioned I > > might in my earlier mail. > > > > I'm not sure this (setting more and more GUCs to prevent hypothetical > plan changes) is a good practice. Because how do you know the plan does > not change for some other unexpected reason, possibly in the future? Sure. So, you think it is better not to have enable_material = false? > IMHO if the test requires a specific plan, it's better to do an actual > "explain (rows off, costs off)" to check that. When you say "rows off", do you mean do something to ensure that it doesn't return tuples? Because I don't see a ROWS option for EXPLAIN in the docs. - Melanie ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-05-14 18:44 Tomas Vondra <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Tomas Vondra @ 2024-05-14 18:44 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On 5/14/24 20:40, Melanie Plageman wrote: > On Tue, May 14, 2024 at 2:33 PM Tomas Vondra > <[email protected]> wrote: >> >> On 5/14/24 19:42, Melanie Plageman wrote: >>> I've fixed this. I've also set enable_material off as I mentioned I >>> might in my earlier mail. >>> >> >> I'm not sure this (setting more and more GUCs to prevent hypothetical >> plan changes) is a good practice. Because how do you know the plan does >> not change for some other unexpected reason, possibly in the future? > > Sure. So, you think it is better not to have enable_material = false? > Right. Unless it's actually needed to force the necessary plan. >> IMHO if the test requires a specific plan, it's better to do an actual >> "explain (rows off, costs off)" to check that. > > When you say "rows off", do you mean do something to ensure that it > doesn't return tuples? Because I don't see a ROWS option for EXPLAIN > in the docs. > Sorry, I meant to hide the cardinality estimates in the explain, but I got confused. "COSTS OFF" is enough. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-05-14 19:11 Melanie Plageman <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 0 replies; 28+ messages in thread From: Melanie Plageman @ 2024-05-14 19:11 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Nazir Bilal Yavuz <[email protected]> On Tue, May 14, 2024 at 2:44 PM Tomas Vondra <[email protected]> wrote: > > On 5/14/24 20:40, Melanie Plageman wrote: > > On Tue, May 14, 2024 at 2:33 PM Tomas Vondra > > <[email protected]> wrote: > >> > >> On 5/14/24 19:42, Melanie Plageman wrote: > >>> I've fixed this. I've also set enable_material off as I mentioned I > >>> might in my earlier mail. > >>> > >> > >> I'm not sure this (setting more and more GUCs to prevent hypothetical > >> plan changes) is a good practice. Because how do you know the plan does > >> not change for some other unexpected reason, possibly in the future? > > > > Sure. So, you think it is better not to have enable_material = false? > > > > Right. Unless it's actually needed to force the necessary plan. Attached v6 does not use enable_material = false (as it is not needed). - Melanie Attachments: [text/x-patch] v6-0001-BitmapHeapScan-Remove-incorrect-assert-and-reset-.patch (5.6K, ../../CAAKRu_Y36cRie7XurLGQerYQBev4Wphqe4U72cKtQS2Gu6Br=Q@mail.gmail.com/2-v6-0001-BitmapHeapScan-Remove-incorrect-assert-and-reset-.patch) download | inline diff: From 6dc674fff206dc62c64d348ff0042b1d20798511 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Tue, 14 May 2024 13:36:42 -0400 Subject: [PATCH v6] BitmapHeapScan: Remove incorrect assert and reset field 04e72ed617be pushed the skip fetch optimization (allowing bitmap heap scans to operate like index-only scans if none of the underlying data is needed) into heap AM implementations of bitmap table scan callbacks. 04e72ed617be added an assert that all tuples in blocks eligible for the optimization had been NULL-filled and emitted by the end of the scan. This assert is incorrect when not all tuples need be scanned to execute the query; for example: a join in which not all inner tuples need to be scanned before skipping to the next outer tuple. Remove the assert and reset the field on which it previously asserted to avoid incorrectly emitting NULL-filled tuples from a previous scan on rescan. Author: Melanie Plageman Reviewed-by: Tomas Vondra, Michael Paquier Reported-by: Melanie Plageman Reproduced-by: Tomas Vondra, Richard Guo Discussion: https://postgr.es/m/CAMbWs48orzZVXa7-vP9Nt7vQWLTE04Qy4PePaLQYsVNQgo6qRg%40mail.gmail.com --- src/backend/access/heap/heapam.c | 9 ++++--- src/test/regress/expected/join.out | 38 ++++++++++++++++++++++++++++++ src/test/regress/sql/join.sql | 26 ++++++++++++++++++++ 3 files changed, 70 insertions(+), 3 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 4be0dee4de..82bb9cb33b 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1184,7 +1184,12 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params, scan->rs_vmbuffer = InvalidBuffer; } - Assert(scan->rs_empty_tuples_pending == 0); + /* + * Reset rs_empty_tuples_pending, a field only used by bitmap heap scan, + * to avoid incorrectly emitting NULL-filled tuples from a previous scan + * on rescan. + */ + scan->rs_empty_tuples_pending = 0; /* * The read stream is reset on rescan. This must be done before @@ -1216,8 +1221,6 @@ heap_endscan(TableScanDesc sscan) if (BufferIsValid(scan->rs_vmbuffer)) ReleaseBuffer(scan->rs_vmbuffer); - Assert(scan->rs_empty_tuples_pending == 0); - /* * Must free the read stream before freeing the BufferAccessStrategy. */ diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 0246d56aea..df358f7605 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -7924,3 +7924,41 @@ where exists (select 1 from j3 (13 rows) drop table j3; +-- Exercise the "skip fetch" Bitmap Heap Scan optimization when candidate +-- tuples are discarded. This may occur when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. Use a temp table so it is only visible to this backend and +-- vacuum may reliably mark all blocks in the table all visible in the +-- visibility map. +CREATE TEMP TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM generate_series(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; +SET enable_indexonlyscan = off; +SET enable_indexscan = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + QUERY PLAN +--------------------------------------------------------- + Nested Loop Anti Join + -> Seq Scan on skip_fetch t1 + -> Materialize + -> Bitmap Heap Scan on skip_fetch t2 + Recheck Cond: (a = 1) + -> Bitmap Index Scan on skip_fetch_a_idx + Index Cond: (a = 1) +(7 rows) + +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + a +--- +(0 rows) + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_seqscan; diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 923e7c5549..d9199dfcad 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2904,3 +2904,29 @@ where exists (select 1 from j3 and t1.unique1 < 1; drop table j3; + +-- Exercise the "skip fetch" Bitmap Heap Scan optimization when candidate +-- tuples are discarded. This may occur when: +-- 1. A join doesn't require all inner tuples to be scanned for each outer +-- tuple, and +-- 2. The inner side is scanned using a bitmap heap scan, and +-- 3. The bitmap heap scan is eligible for the "skip fetch" optimization. +-- This optimization is usable when no data from the underlying table is +-- needed. Use a temp table so it is only visible to this backend and +-- vacuum may reliably mark all blocks in the table all visible in the +-- visibility map. +CREATE TEMP TABLE skip_fetch (a INT, b INT) WITH (fillfactor=10); +INSERT INTO skip_fetch SELECT i % 3, i FROM generate_series(0,30) i; +CREATE INDEX ON skip_fetch(a); +VACUUM (ANALYZE) skip_fetch; + +SET enable_indexonlyscan = off; +SET enable_indexscan = off; +SET enable_seqscan = off; +EXPLAIN (COSTS OFF) +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; +SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS NULL; + +RESET enable_indexonlyscan; +RESET enable_indexscan; +RESET enable_seqscan; -- 2.34.1 ^ permalink raw reply [nested|flat] 28+ messages in thread
end of thread, other threads:[~2024-05-14 19:11 UTC | newest] Thread overview: 28+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-02-01 10:41 [PATCH v28 3/5] Subscripting for jsonb erthalion <[email protected]> 2019-02-01 10:41 [PATCH v30 3/6] Subscripting for jsonb erthalion <[email protected]> 2024-04-07 14:24 Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-04-07 14:41 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-04-07 14:54 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-04-18 07:10 ` Re: BitmapHeapScan streaming read user and prelim refactoring Michael Paquier <[email protected]> 2024-04-18 09:39 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-04-22 17:01 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-04-23 16:05 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-04-23 22:43 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-04-24 20:46 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-04-25 23:03 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-04-25 23:28 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tom Lane <[email protected]> 2024-04-25 23:57 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tom Lane <[email protected]> 2024-04-26 13:04 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-04-30 12:07 ` Re: BitmapHeapScan streaming read user and prelim refactoring Daniel Gustafsson <[email protected]> 2024-05-02 21:31 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-05-02 21:37 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-05-10 19:48 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-05-11 19:18 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-05-13 14:05 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-05-14 06:18 ` Re: BitmapHeapScan streaming read user and prelim refactoring Michael Paquier <[email protected]> 2024-05-14 17:42 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-05-14 18:33 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-05-14 18:40 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-05-14 18:44 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-05-14 19:11 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-04-12 03:18 ` Re: BitmapHeapScan streaming read user and prelim refactoring Richard Guo <[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