Re: pgsql: Add more SQL/JSON constructor functions - Mailing list pgsql-hackers
From | Amit Langote |
---|---|
Subject | Re: pgsql: Add more SQL/JSON constructor functions |
Date | |
Msg-id | CA+HiwqFP0X75gXLrxQb3bg4di9KrQWJFhCEpcmdLcsUbJLHMpA@mail.gmail.com Whole thread Raw |
In response to | Re: pgsql: Add more SQL/JSON constructor functions (Amit Langote <amitlangote09@gmail.com>) |
Responses |
Re: pgsql: Add more SQL/JSON constructor functions
|
List | pgsql-hackers |
On Tue, Jul 2, 2024 at 5:03 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Jul 2, 2024 at 3:19 PM jian he <jian.universality@gmail.com> wrote: > > On Mon, Jul 1, 2024 at 6:45 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > > > On Sun, Jun 30, 2024 at 3:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > > >> + /* > > > > >> + * For domains, consider the base type's typmod to decide whether to setup > > > > >> + * an implicit or explicit cast. > > > > >> + */ > > > > >> + if (get_typtype(returning->typid) == TYPTYPE_DOMAIN) > > > > >> + (void) getBaseTypeAndTypmod(returning->typid, &baseTypmod); > > > > > > > > > TBH I'm not super clear on why we decide on explicit or implicit cast > > > > > based on presence of a typmod. Why isn't it better to always use an > > > > > implicit one? > > > > > > > > Hmm ... there are a bunch of existing places that seem to have similar > > > > logic, but they are all in new-ish SQL/JSON functionality, and I would > > > > not be surprised if they are all wrong. parse_coerce.c is quite > > > > opinionated about what a domain's typtypmod means (see comments in > > > > coerce_type() for instance); see also the logic in coerce_to_domain: > > > > > > > > * If the domain applies a typmod to its base type, build the appropriate > > > > * coercion step. Mark it implicit for display purposes, because we don't > > > > * want it shown separately by ruleutils.c; but the isExplicit flag passed > > > > * to the conversion function depends on the manner in which the domain > > > > * coercion is invoked, so that the semantics of implicit and explicit > > > > * coercion differ. (Is that really the behavior we want?) > > > > > > > > I don't think that this SQL/JSON behavior quite matches that. > > > > > > The reason I decided to go for the implicit cast only when there is a > > > typmod is that the behavior with COERCION_EXPLICIT is only problematic > > > when there's a typmod because of this code in > > > build_coercion_expression: > > > > > > if (nargs == 3) > > > { > > > /* Pass it a boolean isExplicit parameter, too */ > > > cons = makeConst(BOOLOID, > > > -1, > > > InvalidOid, > > > sizeof(bool), > > > BoolGetDatum(ccontext == COERCION_EXPLICIT), > > > false, > > > true); > > > > > > args = lappend(args, cons); > > > } > > > > > > Yeah, we could have fixed that by always using COERCION_IMPLICIT for > > > SQL/JSON but, as Jian said, we don't have a bunch of casts that these > > > SQL/JSON functions need, which is why I guess we ended up with > > > COERCION_EXPLICIT here in the first place. > > > > > > One option I hadn't tried was using COERCION_ASSIGNMENT instead, which > > > seems to give coerceJsonFuncExpr() the casts it needs with the > > > behavior it wants, so how about applying the attached? > > > > you patched works. > > i think it's because of you mentioned build_coercion_expression ` if > > (nargs == 3)` related code > > and > > > > find_coercion_pathway: > > if (result == COERCION_PATH_NONE) > > { > > if (ccontext >= COERCION_ASSIGNMENT && > > TypeCategory(targetTypeId) == TYPCATEGORY_STRING) > > result = COERCION_PATH_COERCEVIAIO; > > else if (ccontext >= COERCION_EXPLICIT && > > TypeCategory(sourceTypeId) == TYPCATEGORY_STRING) > > result = COERCION_PATH_COERCEVIAIO; > > } > > > > functions: JSON_OBJECT,JSON_ARRAY, JSON_ARRAYAGG,JSON_OBJECTAGG, > > JSON_SERIALIZE > > the returning type can only be string type or json. json type already > > being handled in other code. > > so the targetTypeId category will be only TYPCATEGORY_STRING. > > Yes, thanks for confirming that. > > I checked other sites that use COERCION_ASSIGNMENT and I don't see a > reason why it can't be used in this context. > > I'll push the patch tomorrow unless there are objections. Sorry, I dropped the ball on this one. I've pushed the patch now. -- Thanks, Amit Langote
pgsql-hackers by date: