public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v27 4/9] Row pattern recognition patch (planner). 13+ messages / 4 participants [nested] [flat]
* [PATCH v27 4/9] Row pattern recognition patch (planner). @ 2024-12-30 23:53 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Tatsuo Ishii @ 2024-12-30 23:53 UTC (permalink / raw) --- src/backend/optimizer/plan/createplan.c | 22 ++++++++++++++---- src/backend/optimizer/plan/planner.c | 3 +++ src/backend/optimizer/plan/setrefs.c | 27 ++++++++++++++++++++++- src/backend/optimizer/prep/prepjointree.c | 9 ++++++++ src/include/nodes/plannodes.h | 19 ++++++++++++++++ 5 files changed, 75 insertions(+), 5 deletions(-) diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index b3e2294e84..e99ac4652e 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -291,8 +291,10 @@ static WindowAgg *make_windowagg(List *tlist, Index winref, int frameOptions, Node *startOffset, Node *endOffset, Oid startInRangeFunc, Oid endInRangeFunc, Oid inRangeColl, bool inRangeAsc, bool inRangeNullsFirst, - List *runCondition, List *qual, bool topWindow, - Plan *lefttree); + List *runCondition, RPSkipTo rpSkipTo, List *patternVariable, + List *patternRegexp, List *defineClause, + List *defineInitial, List *qual, + bool topWindow, Plan *lefttree); static Group *make_group(List *tlist, List *qual, int numGroupCols, AttrNumber *grpColIdx, Oid *grpOperators, Oid *grpCollations, Plan *lefttree); @@ -2700,6 +2702,11 @@ create_windowagg_plan(PlannerInfo *root, WindowAggPath *best_path) wc->inRangeAsc, wc->inRangeNullsFirst, best_path->runCondition, + wc->rpSkipTo, + wc->patternVariable, + wc->patternRegexp, + wc->defineClause, + wc->defineInitial, best_path->qual, best_path->topwindow, subplan); @@ -6708,8 +6715,10 @@ make_windowagg(List *tlist, Index winref, int ordNumCols, AttrNumber *ordColIdx, Oid *ordOperators, Oid *ordCollations, int frameOptions, Node *startOffset, Node *endOffset, Oid startInRangeFunc, Oid endInRangeFunc, - Oid inRangeColl, bool inRangeAsc, bool inRangeNullsFirst, - List *runCondition, List *qual, bool topWindow, Plan *lefttree) + Oid inRangeColl, bool inRangeAsc, bool inRangeNullsFirst, List *runCondition, + RPSkipTo rpSkipTo, List *patternVariable, List *patternRegexp, List *defineClause, + List *defineInitial, + List *qual, bool topWindow, Plan *lefttree) { WindowAgg *node = makeNode(WindowAgg); Plan *plan = &node->plan; @@ -6735,6 +6744,11 @@ make_windowagg(List *tlist, Index winref, node->inRangeAsc = inRangeAsc; node->inRangeNullsFirst = inRangeNullsFirst; node->topWindow = topWindow; + node->rpSkipTo = rpSkipTo, + node->patternVariable = patternVariable; + node->patternRegexp = patternRegexp; + node->defineClause = defineClause; + node->defineInitial = defineInitial; plan->targetlist = tlist; plan->lefttree = lefttree; diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 7468961b01..f123721e06 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -881,6 +881,9 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root, EXPRKIND_LIMIT); wc->endOffset = preprocess_expression(root, wc->endOffset, EXPRKIND_LIMIT); + wc->defineClause = (List *) preprocess_expression(root, + (Node *) wc->defineClause, + EXPRKIND_TARGET); } parse->limitOffset = preprocess_expression(root, parse->limitOffset, diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 6d23df108d..70c9733e3f 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -211,7 +211,6 @@ static List *set_windowagg_runcondition_references(PlannerInfo *root, List *runcondition, Plan *plan); - /***************************************************************************** * * SUBPLAN REFERENCES @@ -2492,6 +2491,32 @@ set_upper_references(PlannerInfo *root, Plan *plan, int rtoffset) NRM_EQUAL, NUM_EXEC_QUAL(plan)); + /* + * Modifies an expression tree in each DEFINE clause so that all Var + * nodes's varno refers to OUTER_VAR. + */ + if (IsA(plan, WindowAgg)) + { + WindowAgg *wplan = (WindowAgg *) plan; + + if (wplan->defineClause != NIL) + { + foreach(l, wplan->defineClause) + { + TargetEntry *tle = (TargetEntry *) lfirst(l); + + tle->expr = (Expr *) + fix_upper_expr(root, + (Node *) tle->expr, + subplan_itlist, + OUTER_VAR, + rtoffset, + NRM_EQUAL, + NUM_EXEC_QUAL(plan)); + } + } + } + pfree(subplan_itlist); } diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index adad7ea9a9..842738adc9 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -2298,6 +2298,15 @@ perform_pullup_replace_vars(PlannerInfo *root, parse->returningList = (List *) pullup_replace_vars((Node *) parse->returningList, rvcontext); + foreach(lc, parse->windowClause) + { + WindowClause *wc = lfirst_node(WindowClause, lc); + + if (wc->defineClause != NIL) + wc->defineClause = (List *) + pullup_replace_vars((Node *) wc->defineClause, rvcontext); + } + if (parse->onConflict) { parse->onConflict->onConflictSet = (List *) diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 4633121689..1e7cc2a94e 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -20,6 +20,7 @@ #include "lib/stringinfo.h" #include "nodes/bitmapset.h" #include "nodes/lockoptions.h" +#include "nodes/parsenodes.h" #include "nodes/primnodes.h" @@ -1099,6 +1100,24 @@ typedef struct WindowAgg /* nulls sort first for in_range tests? */ bool inRangeNullsFirst; + /* Row Pattern Recognition AFTER MACH SKIP clause */ + RPSkipTo rpSkipTo; /* Row Pattern Skip To type */ + + /* Row Pattern PATTERN variable name (list of String) */ + List *patternVariable; + + /* + * Row Pattern RPATTERN regular expression quantifier ('+' or ''. list of + * String) + */ + List *patternRegexp; + + /* Row Pattern DEFINE clause (list of TargetEntry) */ + List *defineClause; + + /* Row Pattern DEFINE variable initial names (list of String) */ + List *defineInitial; + /* * false for all apart from the WindowAgg that's closest to the root of * the plan -- 2.25.1 ----Next_Part(Tue_Dec_31_08_57_07_2024_963)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v27-0005-Row-pattern-recognition-patch-executor.patch" ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-16 22:12 Tom Lane <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Tom Lane @ 2025-04-16 22:12 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: [email protected] Daniel Gustafsson <[email protected]> writes: >> On 16 Apr 2025, at 23:42, Tom Lane <[email protected]> wrote: >> I'm not sure >> how other than giving up on stack allocation of JsonLexContexts, >> though, especially if we consider the jsonapi API frozen. But seeing >> that there are only three such call sites and none of them seem in the >> least performance-critical, maybe we should just do that? > I can't see any other option really, and there is no performance angle really > so that should be safe. Since I committed at least one of these, let me know > if you want me to tackle it. The only alternative I can see that might stop the warning is if we can find a way to make it clearer to the optimizer that the FREE() isn't reached. But I'm not sure about a trustworthy way to make that happen. Maybe it'd work to change the signature of freeJsonLexContext (or perhaps better, add a separate entry point) so that the caller is passing a bool constant that controls whether to free the struct. We could have an Assert that compares that to the state of the JSONLEX_FREE_STRUCT flag to catch mistakes. This seems kind of messy though. regards, tom lane ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-16 22:18 Daniel Gustafsson <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Daniel Gustafsson @ 2025-04-16 22:18 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected] > On 17 Apr 2025, at 00:12, Tom Lane <[email protected]> wrote: > > Daniel Gustafsson <[email protected]> writes: >>> On 16 Apr 2025, at 23:42, Tom Lane <[email protected]> wrote: >>> I'm not sure >>> how other than giving up on stack allocation of JsonLexContexts, >>> though, especially if we consider the jsonapi API frozen. But seeing >>> that there are only three such call sites and none of them seem in the >>> least performance-critical, maybe we should just do that? > >> I can't see any other option really, and there is no performance angle really >> so that should be safe. Since I committed at least one of these, let me know >> if you want me to tackle it. > > The only alternative I can see that might stop the warning is if we > can find a way to make it clearer to the optimizer that the FREE() > isn't reached. But I'm not sure about a trustworthy way to make that > happen. Maybe it'd work to change the signature of freeJsonLexContext > (or perhaps better, add a separate entry point) so that the caller is > passing a bool constant that controls whether to free the struct. > We could have an Assert that compares that to the state of the > JSONLEX_FREE_STRUCT flag to catch mistakes. This seems kind of messy > though. Yeah, that seems messy enough that someone down the line will go "why on earth" and we'll have to revisit this discussion. It can probably be made to work but I doubt it will be worth it compared to allocating on the heap. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-16 23:04 Tom Lane <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Tom Lane @ 2025-04-16 23:04 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: [email protected] Daniel Gustafsson <[email protected]> writes: > On 17 Apr 2025, at 00:12, Tom Lane <[email protected]> wrote: >> The only alternative I can see that might stop the warning is if we >> can find a way to make it clearer to the optimizer that the FREE() >> isn't reached. But I'm not sure about a trustworthy way to make that >> happen. > Yeah, that seems messy enough that someone down the line will go "why on earth" > and we'll have to revisit this discussion. It can probably be made to work but > I doubt it will be worth it compared to allocating on the heap. Looking through all of the callers of freeJsonLexContext, quite a lot of them use local JsonLexContext structs, and probably some of them are more performance-critical than these. So that raises the question of why are we seeing warnings for only these call sites? Maybe there is a more elegant way to suppress them. Still, I think that observation refutes my initial thought that we should rip out support for local JsonLexContext structs altogether. I'm inclined now to just do the minimal thing of changing these callers to use an allocated struct, and call it a day. (BTW, there seem to be only 2 places to change not 3; two of the warnings are pointing at the same variable.) regards, tom lane ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-16 23:09 Jacob Champion <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Jacob Champion @ 2025-04-16 23:09 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; [email protected] On Wed, Apr 16, 2025 at 4:04 PM Tom Lane <[email protected]> wrote: > Looking through all of the callers of freeJsonLexContext, quite > a lot of them use local JsonLexContext structs, and probably some > of them are more performance-critical than these. So that raises > the question of why are we seeing warnings for only these call > sites? Yeah, I had the same question... > Maybe there is a more elegant way to suppress them. Can we brute-force ignore this particular warning site with a #pragma (suggested in [1])? --Jacob [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98753 ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-16 23:28 Tom Lane <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Tom Lane @ 2025-04-16 23:28 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; [email protected] Jacob Champion <[email protected]> writes: > On Wed, Apr 16, 2025 at 4:04 PM Tom Lane <[email protected]> wrote: >> Looking through all of the callers of freeJsonLexContext, quite >> a lot of them use local JsonLexContext structs, and probably some >> of them are more performance-critical than these. So that raises >> the question of why are we seeing warnings for only these call >> sites? > Yeah, I had the same question... After making another pass through the callers of freeJsonLexContext, I observe that the warnings appear in callers that use a local variable *and* contain goto statements. So I'm betting that the presence of goto's causes the LTO optimizer to pull in its horns quite a bit and thereby fail to detect the flag correlation. >> Maybe there is a more elegant way to suppress them. > Can we brute-force ignore this particular warning site with a #pragma > (suggested in [1])? That's surely not elegant :-(. However, I don't especially want to rewrite away the goto's in these callers ... regards, tom lane ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-17 11:43 Daniel Gustafsson <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Daniel Gustafsson @ 2025-04-17 11:43 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Jacob Champion <[email protected]>; [email protected] > On 17 Apr 2025, at 01:28, Tom Lane <[email protected]> wrote: > > Jacob Champion <[email protected]> writes: >> On Wed, Apr 16, 2025 at 4:04 PM Tom Lane <[email protected]> wrote: >>> Looking through all of the callers of freeJsonLexContext, quite >>> a lot of them use local JsonLexContext structs, and probably some >>> of them are more performance-critical than these. So that raises >>> the question of why are we seeing warnings for only these call >>> sites? > >> Yeah, I had the same question... > > After making another pass through the callers of freeJsonLexContext, > I observe that the warnings appear in callers that use a local > variable *and* contain goto statements. So I'm betting that the > presence of goto's causes the LTO optimizer to pull in its horns > quite a bit and thereby fail to detect the flag correlation. That seems plausible given the selective warnings. >>> Maybe there is a more elegant way to suppress them. > >> Can we brute-force ignore this particular warning site with a #pragma >> (suggested in [1])? > > That's surely not elegant :-(. However, I don't especially want to > rewrite away the goto's in these callers ... Agreed, moving to heap allocated structures for these callsites seem much better. Something like the attached should be enough I think? -- Daniel Gustafsson Attachments: [application/octet-stream] 0001-Allocate-JsonLexContexts-on-the-heap-to-avoid-warnin.patch (5.4K, ../../[email protected]/2-0001-Allocate-JsonLexContexts-on-the-heap-to-avoid-warnin.patch) download | inline diff: From c5b661e24aff9d6ae54f922a289b92d39383322d Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson <[email protected]> Date: Thu, 17 Apr 2025 13:38:05 +0200 Subject: [PATCH] Allocate JsonLexContexts on the heap to avoid warnings The stack allocated JsonLexContexts, in combination with codepaths using goto, were causing warnings when compiling with LTO enabled as the optimizer is unable to figure out that is safe. Rather than contort the code with workarounds for this simply heap allocate the structs instead as these are not in any performance critical paths. Reported-by: Tom Lane <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/interfaces/libpq/fe-auth-oauth.c | 23 ++++++++++++++----- .../test_json_parser_incremental.c | 22 ++++++++++-------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c index cf1a25e2ccc..b4d69874c83 100644 --- a/src/interfaces/libpq/fe-auth-oauth.c +++ b/src/interfaces/libpq/fe-auth-oauth.c @@ -476,7 +476,7 @@ issuer_from_well_known_uri(PGconn *conn, const char *wkuri) static bool handle_oauth_sasl_error(PGconn *conn, const char *msg, int msglen) { - JsonLexContext lex = {0}; + JsonLexContext *lex; JsonSemAction sem = {0}; JsonParseErrorType err; struct json_ctx ctx = {0}; @@ -504,8 +504,19 @@ handle_oauth_sasl_error(PGconn *conn, const char *msg, int msglen) return false; } - makeJsonLexContextCstringLen(&lex, msg, msglen, PG_UTF8, true); - setJsonLexContextOwnsTokens(&lex, true); /* must not leak on error */ + /* + * Later error paths need to go via the cleanup label but since ctx hasn't + * been initiated yet we return immediately here. + */ + lex = calloc(1, sizeof(JsonLexContext)); + if (!lex) + { + libpq_append_conn_error(conn, "out of memory"); + return false; + } + + makeJsonLexContextCstringLen(lex, msg, msglen, PG_UTF8, true); + setJsonLexContextOwnsTokens(lex, true); /* must not leak on error */ initPQExpBuffer(&ctx.errbuf); sem.semstate = &ctx; @@ -516,7 +527,7 @@ handle_oauth_sasl_error(PGconn *conn, const char *msg, int msglen) sem.array_start = oauth_json_array_start; sem.scalar = oauth_json_scalar; - err = pg_parse_json(&lex, &sem); + err = pg_parse_json(lex, &sem); if (err == JSON_SEM_ACTION_FAILED) { @@ -535,7 +546,7 @@ handle_oauth_sasl_error(PGconn *conn, const char *msg, int msglen) } } else if (err != JSON_SUCCESS) - errmsg = json_errdetail(err, &lex); + errmsg = json_errdetail(err, lex); if (errmsg) libpq_append_conn_error(conn, @@ -544,7 +555,7 @@ handle_oauth_sasl_error(PGconn *conn, const char *msg, int msglen) /* Don't need the error buffer or the JSON lexer anymore. */ termPQExpBuffer(&ctx.errbuf); - freeJsonLexContext(&lex); + freeJsonLexContext(lex); if (errmsg) goto cleanup; diff --git a/src/test/modules/test_json_parser/test_json_parser_incremental.c b/src/test/modules/test_json_parser/test_json_parser_incremental.c index a529ee47e9b..357152e0ae0 100644 --- a/src/test/modules/test_json_parser/test_json_parser_incremental.c +++ b/src/test/modules/test_json_parser/test_json_parser_incremental.c @@ -84,7 +84,7 @@ main(int argc, char **argv) char buff[BUFSIZE]; FILE *json_file; JsonParseErrorType result; - JsonLexContext lex; + JsonLexContext *lex; StringInfoData json; int n_read; size_t chunk_size = DEFAULT_CHUNK_SIZE; @@ -98,6 +98,10 @@ main(int argc, char **argv) pg_logging_init(argv[0]); + lex = calloc(1, sizeof(JsonLexContext)); + if (!lex) + pg_fatal("out of memory"); + while ((c = getopt(argc, argv, "c:os")) != -1) { switch (c) @@ -113,7 +117,7 @@ main(int argc, char **argv) case 's': /* do semantic processing */ testsem = &sem; sem.semstate = palloc(sizeof(struct DoState)); - ((struct DoState *) sem.semstate)->lex = &lex; + ((struct DoState *) sem.semstate)->lex = lex; ((struct DoState *) sem.semstate)->buf = makeStringInfo(); need_strings = true; break; @@ -131,8 +135,8 @@ main(int argc, char **argv) exit(1); } - makeJsonLexContextIncremental(&lex, PG_UTF8, need_strings); - setJsonLexContextOwnsTokens(&lex, lex_owns_tokens); + makeJsonLexContextIncremental(lex, PG_UTF8, need_strings); + setJsonLexContextOwnsTokens(lex, lex_owns_tokens); initStringInfo(&json); if ((json_file = fopen(testfile, PG_BINARY_R)) == NULL) @@ -165,12 +169,12 @@ main(int argc, char **argv) bytes_left -= n_read; if (bytes_left > 0) { - result = pg_parse_json_incremental(&lex, testsem, + result = pg_parse_json_incremental(lex, testsem, json.data, n_read, false); if (result != JSON_INCOMPLETE) { - fprintf(stderr, "%s\n", json_errdetail(result, &lex)); + fprintf(stderr, "%s\n", json_errdetail(result, lex)); ret = 1; goto cleanup; } @@ -178,12 +182,12 @@ main(int argc, char **argv) } else { - result = pg_parse_json_incremental(&lex, testsem, + result = pg_parse_json_incremental(lex, testsem, json.data, n_read, true); if (result != JSON_SUCCESS) { - fprintf(stderr, "%s\n", json_errdetail(result, &lex)); + fprintf(stderr, "%s\n", json_errdetail(result, lex)); ret = 1; goto cleanup; } @@ -195,7 +199,7 @@ main(int argc, char **argv) cleanup: fclose(json_file); - freeJsonLexContext(&lex); + freeJsonLexContext(lex); free(json.data); return ret; -- 2.39.3 (Apple Git-146) ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-17 15:48 Jacob Champion <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Jacob Champion @ 2025-04-17 15:48 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; [email protected] On Thu, Apr 17, 2025 at 8:20 AM Tom Lane <[email protected]> wrote: > I confirm this silences those warnings on my Fedora 41 box. Instead of doing lex = calloc(...); /* (error out on NULL return) */ makeJsonLexContextCstringLen(lex, ...); we need to do lex = makeJsonLexContextCstringLen(NULL, ...); /* (error out on NULL return) */ so that JSONLEX_FREE_STRUCT is set correctly. Otherwise we'll leak the main allocation: ==12550==ERROR: LeakSanitizer: detected memory leaks Direct leak of 120 byte(s) in 1 object(s) allocated from: #0 0xaaaae34d2a84 in __interceptor_calloc (/home/jacob/src/postgres/worktree-oauth/build-clang/src/interfaces/libpq/fuzz_libpq_handle_oauth_sasl_error+0x112a84) (BuildId: 359bf20b63a97771ccb3bd2c238485920485521f) #1 0xaaaae3510ff0 in handle_oauth_sasl_error /home/jacob/src/postgres/worktree-oauth/build-clang/../src/interfaces/libpq/fe-auth-oauth.c:511:8 > I'm content to do it like this, but maybe Jacob wants to > investigate alternatives? I was more worried about it when you said you wanted to get rid of the stack allocation API. (I like having the flexibility to choose between the two forms, not just for performance but also for struct embedding.) But I'm perfectly happy with just adjusting these sites. Thanks! --Jacob ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-21 15:33 Jacob Champion <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Jacob Champion @ 2025-04-21 15:33 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Developers <[email protected]> On Sat, Apr 19, 2025 at 2:15 PM Daniel Gustafsson <[email protected]> wrote: > Since there is no way to determine if the allocation succeeded from outside of > the JSON api it might be better to keep the calloc and explicitly free it? I don't think so; pg_parse_json() will error out quickly, so I don't see much advantage to the extra code. Raw performance isn't much of a concern for the out-of-memory case, IMO. > (Could a JsonLexContextBroken() function be useful perhaps?) Maybe if there's ever a client that absolutely must initialize a context before doing a bunch of independent work? But I don't think that's true here. (Any callers we're converting from the stack API are going to have short-lived contexts.) --Jacob ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-21 18:20 Daniel Gustafsson <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Daniel Gustafsson @ 2025-04-21 18:20 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Developers <[email protected]> > On 21 Apr 2025, at 17:33, Jacob Champion <[email protected]> wrote: > > On Sat, Apr 19, 2025 at 2:15 PM Daniel Gustafsson <[email protected]> wrote: >> Since there is no way to determine if the allocation succeeded from outside of >> the JSON api it might be better to keep the calloc and explicitly free it? > > I don't think so; pg_parse_json() will error out quickly, so I don't > see much advantage to the extra code. Raw performance isn't much of a > concern for the out-of-memory case, IMO. Sure, but I fear we'll get an endless stream of static analysis reports for the allocation leaking if we don't free it. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-21 18:28 Jacob Champion <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Jacob Champion @ 2025-04-21 18:28 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Developers <[email protected]> On Mon, Apr 21, 2025 at 11:20 AM Daniel Gustafsson <[email protected]> wrote: > Sure, but I fear we'll get an endless stream of static analysis reports for the > allocation leaking if we don't free it. But we do free it, in freeJsonLexContext(). That usage of the API goes back to 2023, with 1c99cde2f344. Or am I misunderstanding? --Jacob ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-21 18:58 Jacob Champion <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Jacob Champion @ 2025-04-21 18:58 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Developers <[email protected]> On Mon, Apr 21, 2025 at 11:46 AM Daniel Gustafsson <[email protected]> wrote: > We do, but with the current coding we call setJsonLexContextOwnsTokens > immediately after creation which derefences the pointer without checkinf for > allocation failure. Right. (The flag does nothing on the OOM sentinel.) > This means we dereference the pointer before we can check > for an OOM return from pg_parse_json which even if safe seems to violate code > readability no? Personally, I'm fine with can't-fail APIs, as long as they're documented as such. (I think the deferred error API was probably chosen so that src/common JSON clients could be written without a lot of pain?) --Jacob ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled @ 2025-04-23 00:01 Jacob Champion <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Jacob Champion @ 2025-04-23 00:01 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Developers <[email protected]> On Tue, Apr 22, 2025 at 3:10 AM Daniel Gustafsson <[email protected]> wrote: > My preference is that no operation can silently work on a failed object, but > it's not a hill (even more so given where we are in the cycle). Hm, okay. Something to talk about with Andrew and Peter, maybe. > The attached > v3 allocates via the JSON api, no specific error handling should be required as > it's already handled today. pgindent shows one whitespace change on my machine (comment indentation), but other than that, LGTM! Fuzzers are happy too. Thanks, --Jacob ^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2025-04-23 00:01 UTC | newest] Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-12-30 23:53 [PATCH v27 4/9] Row pattern recognition patch (planner). Tatsuo Ishii <[email protected]> 2025-04-16 22:12 Re: jsonapi: scary new warnings with LTO enabled Tom Lane <[email protected]> 2025-04-16 22:18 ` Re: jsonapi: scary new warnings with LTO enabled Daniel Gustafsson <[email protected]> 2025-04-16 23:04 ` Re: jsonapi: scary new warnings with LTO enabled Tom Lane <[email protected]> 2025-04-16 23:09 ` Re: jsonapi: scary new warnings with LTO enabled Jacob Champion <[email protected]> 2025-04-16 23:28 ` Re: jsonapi: scary new warnings with LTO enabled Tom Lane <[email protected]> 2025-04-17 11:43 ` Re: jsonapi: scary new warnings with LTO enabled Daniel Gustafsson <[email protected]> 2025-04-17 15:48 ` Re: jsonapi: scary new warnings with LTO enabled Jacob Champion <[email protected]> 2025-04-21 15:33 ` Re: jsonapi: scary new warnings with LTO enabled Jacob Champion <[email protected]> 2025-04-21 18:20 ` Re: jsonapi: scary new warnings with LTO enabled Daniel Gustafsson <[email protected]> 2025-04-21 18:28 ` Re: jsonapi: scary new warnings with LTO enabled Jacob Champion <[email protected]> 2025-04-21 18:58 ` Re: jsonapi: scary new warnings with LTO enabled Jacob Champion <[email protected]> 2025-04-23 00:01 ` Re: jsonapi: scary new warnings with LTO enabled Jacob Champion <[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