public inbox for [email protected]
help / color / mirror / Atom feedRe: jsonapi: scary new warnings with LTO enabled
11+ messages / 3 participants
[nested] [flat]
* Re: jsonapi: scary new warnings with LTO enabled
@ 2025-04-16 22:18 Daniel Gustafsson <[email protected]>
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled
@ 2025-04-16 23:09 Jacob Champion <[email protected]>
parent: Daniel Gustafsson <[email protected]>
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled
@ 2025-04-17 15:48 Jacob Champion <[email protected]>
parent: Jacob Champion <[email protected]>
0 siblings, 1 reply; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled
@ 2025-04-22 10:10 Daniel Gustafsson <[email protected]>
parent: Jacob Champion <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Gustafsson @ 2025-04-22 10:10 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Developers <[email protected]>
> On 21 Apr 2025, at 20:58, Jacob Champion <[email protected]> wrote:
> 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?)
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). The attached
v3 allocates via the JSON api, no specific error handling should be required as
it's already handled today.
--
Daniel Gustafsson
Attachments:
[application/octet-stream] v3-0001-Allocate-JsonLexContexts-on-the-heap-to-avoid-war.patch (5.3K, ../../[email protected]/2-v3-0001-Allocate-JsonLexContexts-on-the-heap-to-avoid-war.patch)
download | inline diff:
From fe421d657aaa361dfb796e28c96975bed97f79f5 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <[email protected]>
Date: Thu, 17 Apr 2025 23:18:58 +0200
Subject: [PATCH v3] 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]>
Reviewed-by: Jacob Champion <[email protected]>
Reviewed-by: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/interfaces/libpq/fe-auth-oauth.c | 12 +++++-----
.../test_json_parser_incremental.c | 23 +++++++++++--------
2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c
index cf1a25e2ccc..52d2b286625 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,8 @@ 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 */
+ lex = makeJsonLexContextCstringLen(NULL, msg, msglen, PG_UTF8, true);
+ setJsonLexContextOwnsTokens(lex, true); /* must not leak on error */
initPQExpBuffer(&ctx.errbuf);
sem.semstate = &ctx;
@@ -516,7 +516,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 +535,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 +544,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..d1e3e4ab4ea 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,8 +199,9 @@ main(int argc, char **argv)
cleanup:
fclose(json_file);
- freeJsonLexContext(&lex);
+ freeJsonLexContext(lex);
free(json.data);
+ free(lex);
return ret;
}
--
2.39.3 (Apple Git-146)
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled
@ 2025-04-23 00:01 Jacob Champion <[email protected]>
parent: Daniel Gustafsson <[email protected]>
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: jsonapi: scary new warnings with LTO enabled
@ 2025-04-23 09:35 Daniel Gustafsson <[email protected]>
parent: Jacob Champion <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Gustafsson @ 2025-04-23 09:35 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Developers <[email protected]>
> On 23 Apr 2025, at 02:01, Jacob Champion <[email protected]> wrote:
> On Tue, Apr 22, 2025 at 3:10 AM Daniel Gustafsson <[email protected]> wrote:
>> 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 for confirming, I've pushed this now.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Support EXCEPT for TABLES IN SCHEMA publications
@ 2026-07-10 11:37 Nisha Moond <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Nisha Moond @ 2026-07-10 11:37 UTC (permalink / raw)
To: shveta malik <[email protected]>; +Cc: Peter Smith <[email protected]>; Amit Kapila <[email protected]>; Zsolt Parragi <[email protected]>; [email protected]
On Thu, Jul 9, 2026 at 2:53 PM shveta malik <[email protected]> wrote:
>
> >
> > To reduce duplicate code, I moved the schema
> > qualification and eligibility checks for both cases to execution time
>> and removed the corresponding processing from gram.y.
> >
> > Since this changes a fair amount of code, I've kept it as a separate
> > patch (v19-0002) to make review easier. Depending on the feedback, we
> > can optimize the implementation or move parts of it back into gram.y
> > before merging it into patch 0001.
> > ~~~
> >
>
> Nisha,
>
> - * Also flattens except_tables from TABLES IN SCHEMA nodes into the list so
> - * that ObjectsInPublicationToOids() sees them as top-level
> EXCEPT_TABLE entries.
> + * The except_tables attached to TABLES IN SCHEMA nodes are left in place;
> + * ObjectsInPublicationToOids() qualifies names, validates schema membership,
> + * and flattens them once the schema OID is known.
>
> Can you please explain what does 'flatten' mean here.
>
Each TABLES IN SCHEMA ... EXCEPT (...) entry stores its own nested
list of EXCEPT tables. Flatten means collecting the PublicationTable
from each nested list into a single top-level except_pubtables list,
giving it the same structure as a FOR TABLE list that can be passed
directly to OpenTableList().
I've reworded the comment. We can also remove it if it seems unnecessary here.
> The flatten logic here is added by your patch001 only, it is not an
> existing logic. To me (in my initial round of review), it seems okay
> if we moved it to ObjectsInPublicationToOids(). The only thing is that
> the one error (table in EXCEPT clause does not belong to schema) which
> is possible to get at the parsing stage will now be emitted during
> pub-creation stage.
Yes, the logic was introduced in patch 0001 itself. I just highlighted
it for the case you mentioned, where the error is deferred instead of
being raised at parse time.
> But since it will keep the entire logic of
> except-list verification at one place and considering that
> creat/alter-pub is not so frequent operation, I am fine with it. But
> lets see if others have any comment here.
>
My thought was the same: to keep the logic in one place for now. Let's
see if others have any opinions on it as well.
> ~~
>
> I had a quick look at the changes:
>
> + * Include the ONLY-ness of this entry in its signature. EXCEPT
> + * (TABLE parent) excludes parent and its inheritance children,
> + * while EXCEPT (TABLE ONLY parent) excludes only parent itself.
>
> I think this will unconditionally apply to paritioned tables as well
> but for paritioned table, EXCEPT doc says that ONLY and * has no
> effect, but here it shows the effect:
>
> Thsi gives error:
> postgres=# CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA public EXCEPT
> (TABLE ONLY tab_root), TABLES IN SCHEMA public EXCEPT (TABLE
> tab_root);
> ERROR: 42P17: schema "public" specified with conflicting EXCEPT lists
> LOCATION: ProcessSchemaExceptTables, publicationcmds.c:299
>
> While this does not:
> postgres=# CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA public EXCEPT
> (TABLE tab_root), TABLES IN SCHEMA public EXCEPT (TABLE tab_root);
> CREATE PUBLICATION
>
> But I also checked column-list errors and found that even if columns
> are exact sames, we get error:
>
> postgres=# CREATE PUBLICATION PUB4 FOR TABLE T1(i), T1(i);
> ERROR: 42710: conflicting or redundant column lists for table "t1"
> LOCATION: OpenTableList, publicationcmds.c:2312
>
> While this works:
> postgres=# CREATE PUBLICATION PUB4 FOR TABLE T1, T1;
> CREATE PUBLICATION
>
> This makes me think that even introducing the smartness of identifying
> ONLY in case of inherited tables and distinguishing it from
> parititoned tables is not required. We can just change your error
> message to 'conflicting or redundant EXCEPT lists for schmea "s1"' and
> throw error in all cases where schmea with except list repeats without
> actually comparing them. Does this sound reasonable?
>
Yes, that works for me as we have FOR TABLES example to follow. I've
made the changes in v20.
--
Thanks,
Nisha
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2026-07-10 11:37 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-04-16 22:18 Re: jsonapi: scary new warnings with LTO enabled Daniel Gustafsson <[email protected]>
2025-04-16 23:09 ` Jacob Champion <[email protected]>
2025-04-17 15:48 ` Jacob Champion <[email protected]>
2025-04-21 15:33 ` Jacob Champion <[email protected]>
2025-04-21 18:20 ` Daniel Gustafsson <[email protected]>
2025-04-21 18:28 ` Jacob Champion <[email protected]>
2025-04-21 18:58 ` Jacob Champion <[email protected]>
2025-04-22 10:10 ` Daniel Gustafsson <[email protected]>
2025-04-23 00:01 ` Jacob Champion <[email protected]>
2025-04-23 09:35 ` Daniel Gustafsson <[email protected]>
2026-07-10 11:37 Re: Support EXCEPT for TABLES IN SCHEMA publications Nisha Moond <[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