Re: remaining sql/json patches
От | Amit Langote |
---|---|
Тема | Re: remaining sql/json patches |
Дата | |
Msg-id | CA+HiwqGN9nMNTMtTB68wBW0fkZE_e_CFFMihtGQ=m4yt5L+vhw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: remaining sql/json patches (Alvaro Herrera <alvherre@alvh.no-ip.org>) |
Ответы |
Re: remaining sql/json patches
|
Список | pgsql-hackers |
On Thu, Dec 7, 2023 at 12:26 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2023-Dec-06, Amit Langote wrote: > > I think I'm inclined toward adapting the LA-token fix (attached 0005), > > because we've done that before with SQL/JSON constructors patch. > > Also, if I understand the concerns that Tom mentioned at [1] > > correctly, maybe we'd be better off not assigning precedence to > > symbols as much as possible, so there's that too against the approach > > #1. > > Sounds ok to me, but I'm happy for this decision to be overridden by > others with more experience in parser code. OK, I'll wait to hear from others. > > Also I've attached 0006 to add news tests under ECPG for the SQL/JSON > > query functions, which I haven't done so far but realized after you > > mentioned ECPG. It also includes the ECPG variant of the LA-token > > fix. I'll eventually merge it into 0003 and 0004 after expanding the > > test cases some more. I do wonder what kinds of tests we normally add > > to ECPG suite but not others? > > Well, I only added tests to the ecpg suite in the previous round of > SQL/JSON deeds because its grammar was being modified, so it seemed > possible that it'd break. Because you're also going to modify its > parser.c, it seems reasonable to expect tests to be added. I wouldn't > expect to have to do this for other patches, because it should behave > like straight SQL usage. Ah, ok, so ecpg tests are only needed in the JSON_TABLE patch. > Looking at 0002 I noticed that populate_array_assign_ndims() is called > in some places and its return value is not checked, so we'd ultimately > return JSON_SUCCESS even though there's actually a soft error stored > somewhere. I don't know if it's possible to hit this in practice, but > it seems odd. Indeed, fixed. I think I missed the callbacks in JsonSemAction because I only looked at functions directly reachable from json_populate_record() or something. > Looking at get_json_object_as_hash(), I think its comment is not > explicit enough about its behavior when an error is stored in escontext, > so its hard to judge whether its caller is doing the right thing (I > think it is). I've modified get_json_object_as_hash() to return NULL if pg_parse_json_or_errsave() returns false because of an error. Maybe that's an overkill but that's at least a bit clearer than a hash table of indeterminate state. Added a comment too. > OTOH, populate_record seems to have the same issue, but > callers of that definitely seem to be doing the wrong thing -- namely, > not checking whether an error was saved; particularly populate_composite > seems to rely on the returned tuple, even though an error might have > been reported. Right, populate_composite() should return NULL after checking escontext. Fixed. On Thu, Dec 7, 2023 at 12:11 PM jian he <jian.universality@gmail.com> wrote: > typo: > + * If a soft-error occurs, it will be checked by EEOP_JSONEXPR_COECION_FINISH Fixed. > json_exists no RETURNING clause. > so the following part in src/backend/parser/parse_expr.c can be removed? > > + else if (jsexpr->returning->typid != BOOLOID) > + { > + Node *coercion_expr; > + CaseTestExpr *placeholder = makeNode(CaseTestExpr); > + int location = exprLocation((Node *) jsexpr); > + > + /* > + * We abuse CaseTestExpr here as placeholder to pass the > + * result of evaluating JSON_EXISTS to the coercion > + * expression. > + */ > + placeholder->typeId = BOOLOID; > + placeholder->typeMod = -1; > + placeholder->collation = InvalidOid; > + > + coercion_expr = > + coerce_to_target_type(pstate, (Node *) placeholder, BOOLOID, > + jsexpr->returning->typid, > + jsexpr->returning->typmod, > + COERCION_EXPLICIT, > + COERCE_IMPLICIT_CAST, > + location); > + > + if (coercion_expr == NULL) > + ereport(ERROR, > + (errcode(ERRCODE_CANNOT_COERCE), > + errmsg("cannot cast type %s to %s", > + format_type_be(BOOLOID), > + format_type_be(jsexpr->returning->typid)), > + parser_coercion_errposition(pstate, location, (Node *) jsexpr))); > + > + if (coercion_expr != (Node *) placeholder) > + jsexpr->result_coercion = coercion_expr; > + } This is needed in the JSON_TABLE patch as explained in [1]. Moved this part into patch 0004. > Similarly, since JSON_EXISTS has no RETURNING clause, the following > also needs to be refactored? > > + /* > + * Disallow FORMAT specification in the RETURNING clause of JSON_EXISTS() > + * and JSON_VALUE(). > + */ > + if (func->output && > + (func->op == JSON_VALUE_OP || func->op == JSON_EXISTS_OP)) > + { > + JsonFormat *format = func->output->returning->format; > + > + if (format->format_type != JS_FORMAT_DEFAULT || > + format->encoding != JS_ENC_DEFAULT) > + ereport(ERROR, > + (errcode(ERRCODE_SYNTAX_ERROR), > + errmsg("cannot specify FORMAT in RETURNING clause of %s", > + func->op == JSON_VALUE_OP ? "JSON_VALUE()" : > + "JSON_EXISTS()"), > + parser_errposition(pstate, format->location))); This one needs to be fixed, so done. On Thu, Dec 7, 2023 at 5:25 PM Peter Eisentraut <peter@eisentraut.org> wrote: > Here are a couple of small patches to tidy up the parser a bit in your > v28-0004 (JSON_TABLE) patch. It's not a lot; the rest looks okay to me. Thanks Peter. I've merged these into 0004. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] https://www.postgresql.org/message-id/CA%2BHiwqGsByGXLUniPxBgZjn6PeDr0Scp0jxxQOmBXy63tiJ60A%40mail.gmail.com
Вложения
В списке pgsql-hackers по дате отправления:
Следующее
От: Dominik Michael RauhДата:
Сообщение: Configure problem when cross-compiling PostgreSQL 16.1