When is int32 not an int32?
Hell Hackers, long time no email! I got a bug report for the semver extension: https://github.com/theory/pg-semver/issues/58 It claims that a test unexpected passes. That is, Test #31 is expected to fail, because it intentionally tests a version in which its parts overflow the int32[3] they’re stored in, with the expectation that one day we can refactor the type to handle larger version parts. I can’t imagine there would be any circumstance under which int32 would somehow be larger than a signed 32-bit integer, but perhaps there is? Scroll to the bottom of these pages to see the unexpected passes on i386 and armhf: https://ci.debian.net/data/autopkgtest/unstable/i386/p/postgresql-semver/15208658/log.gz https://ci.debian.net/data/autopkgtest/unstable/armhf/p/postgresql-semver/15208657/log.gz Here’s the Postgres build output for those two platforms, as well, though nothing jumps out at me: https://buildd.debian.org/status/fetch.php?pkg=postgresql-13&arch=i386&ver=13.4-3&stamp=1630408269&raw=0 https://buildd.debian.org/status/fetch.php?pkg=postgresql-13&arch=armhf&ver=13.4-3&stamp=1630412028&raw=0 Thanks, David signature.asc Description: Message signed with OpenPGP
Re: When is int32 not an int32?
On Sep 26, 2021, at 18:31, Tom Lane wrote: > I'd bet more along the lines of "your overflow check is less portable than > you thought”. Oh well now that you mention it and I look past things, I see we’re using INT_MAX, but should probably use INT32_MAX. https://github.com/theory/pg-semver/blob/87cc30cbe80aa3992a4af6f19a35a944a86c/src/semver.c#L145-L149 And also that the destination value we’re storing it in is an int parts[], not int32 parts[]. Which we do so we can parse numbers up to int size. But to Fetter’s point, we’re not properly handling something greater than int (usually int64, presumably). Not sure what changes are required to improve memory safety over and above using INT32_MAX instead of INT_MAX. Thanks, Daavid signature.asc Description: Message signed with OpenPGP
Re: When is int32 not an int32?
On Sep 26, 2021, at 19:25, Tom Lane wrote: > More to the point, you should be checking whether strtol reports overflow. > Having now seen your code, I'll opine that the failing platforms have > 32-bit long. Thanks for the pointer, Tom. I believe this fixes that particular issue. https://github.com/theory/pg-semver/commit/4d79dcc Best, David signature.asc Description: Message signed with OpenPGP
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Jan 19, 2024, at 21:46, Erik Wienhold wrote: > Interesting... copy-pasting the entire \set command works for me with > psql 16.1 in gnome-terminal and tmux. Typing it out manually gives me > the "unterminated quoted string" error. Maybe has to do with my stty > settings. Yes, same on macOS Terminal.app and 16.1 compiled with readline. I didn’t realize that \set didn’t support newlines, because it works fine when you paste something with newlines. Curious. >> I experimented with >> >> SELECT ' >> ... multiline json value ... >> ' AS json >> \gexec >> >> but that didn't seem to work either. Anybody have a better idea? > > Fine with me (the \gset variant). Much cleaner TBH. david=# select '{ "track": { "segments": [ { "location": [ 47.763, 13.4034 ], "start time": "2018-10-14 10:05:14", "HR": 73 }, { "location": [ 47.706, 13.2635 ], "start time": "2018-10-14 10:39:21", "HR": 135 } ] } }'::jsonb as json; json {"track": {"segments": [{"HR": 73, "location": [47.763, 13.4034], "start time": "2018-10-14 10:05:14"}, {"HR": 135, "location": [47.706, 13.2635], "start time": "2018-10-14 10:39:21"}]}} (1 row) david=# \gset david=# select :'json'::jsonb; jsonb {"track": {"segments": [{"HR": 73, "location": [47.763, 13.4034], "start time": "2018-10-14 10:05:14"}, {"HR": 135, "location": [47.706, 13.2635], "start time": "2018-10-14 10:39:21"}]}} (1 row) So great! While you’re in there, Tom, would it make sense to fold in something like [this patch][1] I posted last month to clarify which JSONPath comparison operators can take advantage of a index? --- a/doc/src/sgml/json.sgml +++ b/doc/src/sgml/json.sgml @@ -513,7 +513,7 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @@ '$.tags[*] == "qui"'; For these operators, a GIN index extracts clauses of the form accessors_chain -= constant out of +== constant out of the jsonpath pattern, and does the index search based on the keys and values mentioned in these clauses. The accessors chain may include .key, @@ -522,6 +522,9 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @@ '$.tags[*] == "qui"'; The jsonb_ops operator class also supports .* and .** accessors, but the jsonb_path_ops operator class does not. +Only the == and != SQL/JSON Path Operators +can use the index. Best, David [1]: https://www.postgresql.org/message-id/0ece6b9c-cdde-4b65-be5a-49d737204...@justatheory.com
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Jan 20, 2024, at 12:34, Tom Lane wrote: > Surely we're not helping anybody by leaving that behavior in place. > Making it do something useful, throwing an error, or returning NULL > all seem superior to this. I observe that @@ returns NULL for the > path type it doesn't like, so maybe that's what to do here. I agree it would be far better for the behavior to be consistent, but frankly would like to see them raise an error. Ideally the hit would suggest the proper alternative operator or function to use, and maybe link to the docs that describe the difference between SQL-standard JSONPath and "predicate check expressions”, and how they have separate operators and functions. I think of them as practically different data types (and wish they were, TBH). It makes sense that passing a JSON containment expression[1] would raise an error; so should passing the wrong flavor of JSONPath. > BTW, jsonb_path_query_array and jsonb_path_query_first seem to > take both types of path, like jsonb_path_query, so ISTM they need > docs changes too. Happy to update the patch, either to add those docs or, if we change the behavior to return a NULL or raise an error, then with that information, instead. Best, David [1]: https://www.postgresql.org/docs/current/datatype-json.html#JSON-CONTAINMENT
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Jan 20, 2024, at 11:45, Tom Lane wrote: > You sure about that? It would surprise me if we could effectively use > a not-equal condition with an index. If it is only == that works, > then the preceding statement seems sufficient. I’m not! I just assumed it in the same way creating an SQL = operator automatically respects NOT syntax (or so I recall). In fiddling a bit, I can’t get it to use an index: CREATE TABLE MOVIES (id SERIAL PRIMARY KEY, movie JSONB NOT NULL); \copy movies(movie) from PROGRAM 'curl -s https://raw.githubusercontent.com/prust/wikipedia-movie-data/master/movies.json | jq -c ".[]" | sed "s|||g"'; create index on movies using gin (movie); analyze movies; david=# explain analyze select id from movies where movie @? '$ ?(@.genre[*] != "Teen")'; QUERY PLAN - Seq Scan on movies (cost=0.00..3741.41 rows=4 width=4) (actual time=19.222..19.223 rows=0 loops=1) Filter: (movie @? '$?(@."genre"[*] != "Teen")'::jsonpath) Rows Removed by Filter: 36273 Planning Time: 1.242 ms Execution Time: 19.247 ms (5 rows) But that might be because the planner knows that the query is going to fetch most records, anyway. If I set most records to a single value: david=# update movies set movie = jsonb_set(movie, '{year}', '2020'::jsonb) where id < 3600; UPDATE 3599 david=# analyze movies; ANALYZE david=# explain analyze select id from movies where movie @? '$ ?(@.year != 2020)'; QUERY PLAN Seq Scan on movies (cost=0.00..3884.41 rows=32609 width=4) (actual time=0.065..43.730 rows=32399 loops=1) Filter: (movie @? '$?(@."year" != 2020)'::jsonpath) Rows Removed by Filter: 3874 Planning Time: 1.759 ms Execution Time: 45.368 ms (5 rows) Looks like it still doesn’t use the index with !=. Pity. Best, David
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Jan 20, 2024, at 12:34, Tom Lane wrote: > It will take a predicate, but seems to always return true: > > regression=# select '{"a":[1,2,3,4,5]}'::jsonb @? '$.a[*] < 5' ; > ?column? > -- > t > (1 row) > > regression=# select '{"a":[1,2,3,4,5]}'::jsonb @? '$.a[*] > 5' ; > ?column? > -- > t > (1 row) Just for the sake of clarity, this return value is “correct,” because @? and other functions and operators that expect SQL standard statements evaluate the SET returned by the JSONPath statement, but predicate check expressions don’t return a set, but a always a single scalar value (true, false, or null). From the POV of the code expecting SQL standard JSONPath results, that’s a set of one. @? sees that the set is not empty so returns true. Best, David
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Jan 21, 2024, at 14:43, Tom Lane wrote: > I don't entirely buy this argument --- if that is the interpretation, > of what use are predicate check expressions? It seems to me that we > have to consider them as being a shorthand notation for filter > expressions, or else they simply do not make sense as jsonpath. I believe it becomes pretty apparent when using jsonb_path_query(). The filter expression returns a set (using the previous \gset example): david=# select jsonb_path_query(:'json', '$.track.segments[*].HR ? (@ > 10)'); jsonb_path_query -- 73 135 (2 rows) The predicate check returns a boolean: david=# select jsonb_path_query(:'json', '$.track.segments[*].HR > 10'); jsonb_path_query -- true (1 row) This is the only way the different behaviors make sense to me. @? expects a set, not a boolean, sees there is an item in the set, so returns true: david=# select jsonb_path_query(:'json', '$.track.segments[*].HR > 1000'); jsonb_path_query -- false (1 row) david=# select :'json'::jsonb @? '$.track.segments[*].HR > 1000'; ?column? -- t (1 row) Best, David
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Jan 21, 2024, at 14:52, David E. Wheeler wrote: > This is the only way the different behaviors make sense to me. @? expects a > set, not a boolean, sees there is an item in the set, so returns true: I make this interpretation based on this bit of the docs: PostgreSQL's implementation of the SQL/JSON path language has the following deviations from the SQL/JSON standard. Boolean Predicate Check Expressions As an extension to the SQL standard, a PostgreSQL path expression can be a Boolean predicate, whereas the SQL standard allows predicates only in filters. Where SQL standard path expressions return the relevant contents of the queried JSON value, predicate check expressions return the three-valued result of the predicate: true, false, or unknown. Compare this filter jsonpath expression: => select jsonb_path_query(:'json', '$.track.segments ?(@[*].HR > 130)'); jsonb_path_query - {"HR": 135, "location": [47.706, 13.2635], "start time": "2018-10-14 10:39:21"} To a predicate expression, which returns true => select jsonb_path_query(:'json', '$.track.segments[*].HR > 130'); jsonb_path_query -- true Best, David
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Jan 21, 2024, at 14:58, David E. Wheeler wrote: > I make this interpretation based on this bit of the docs: Sorry, that’s from my branch. Here it is in master: A path expression can be a Boolean predicate, although the SQL/JSON standard allows predicates only in filters. This is necessary for implementation of the @@ operator. For example, the following jsonpath expression is valid in PostgreSQL: $.track.segments[*].HR < 70 In any event, something to do with @@, perhaps to have some compatibility with `jsonb @> jsonb`? I don’t know why @@ was important to have. David
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Jan 24, 2024, at 16:32, Tom Lane wrote: > "David E. Wheeler" writes: > >> In any event, something to do with @@, perhaps to have some compatibility >> with `jsonb @> jsonb`? I don’t know why @@ was important to have. > > Yeah, that's certainly under-explained. But it seems like I'm not > getting traction for the idea of changing the behavior, so let's > go back to just documenting it. Curious about those discussions. On the one hand I find the distinction between the two behaviors to be odd, and to produce unexpected results when they’re not used in the proper context. It’s reminds me of the Perl idea of context, where functions behave differently in scalar and list context, and if you expect list behavior on scalar context you’re gonna get a surprise. This is a bit of a challenge for those new to the language, as they’re not necessarily aware of the context. > I spent some time going over your > text and also cleaning up nearby shaky English, and ended with v8 > attached. I'd be content to commit this if it looks good to you. This looks very nice, thank you. A couple of comments. > + > + Predicate check expressions are required in the > + @@ operator (and the > + jsonb_path_match function), and should not be > used > + with the @? operator (or the > + jsonb_path_exists function). > + > + > + I had this bit here: Conversely, non-predicate jsonpath expressions should not be used with the @@ operator (or the jsonb_path_match function). I think it’s important to let people know what the difference is in the behavior of the two forms, in every spot it’s likely to come up. SQL-standard JSON Path expressions should never be used in contexts (functions, operators) only designed to work with predicate check expressions, and the docs should say so IMO. > > -The lax mode facilitates matching of a JSON document structure and path > -expression if the JSON data does not conform to the expected schema. > +The lax mode facilitates matching of a JSON document and path > +expression when the JSON data does not conform to the expected schema. What do you think of also dropping the article from all the references to “the strict mode” or “the lax mode”, to make them “strict mode” and “lax mode”, respectively? Thanks for the review! Best, David
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Jan 25, 2024, at 11:03, Tom Lane wrote: > I changed the preceding para to say "... check expressions are > required in ...", which I thought was sufficient to cover that. > Also, the tabular description of the operator tells you not to do it. Yeah, that’s good. I was perhaps leaning into being over-explicit after it took me a while to even figure out that there was a difference, let alone where matters. >> What do you think of also dropping the article from all the references to >> “the strict mode” or “the lax mode”, to make them “strict mode” and “lax >> mode”, respectively? > > Certainly most of 'em don't need it. I'll make it so. Nice, thanks! David
Bug: The "directory" control parameter does not work
Hackers, I wanted to try to customize the subdirectory for an extension. The docs[1] say: > directory (string) > > The directory containing the extension's SQL script file(s). Unless an > absolute path is given, the name is relative to the installation's SHAREDIR > directory. The default behavior is equivalent to specifying directory = > 'extension'. [1]: https://www.postgresql.org/docs/16/extend-extensions.html But it doesn’t seem to work. I tried this experiment: ❯ pg_config --version PostgreSQL 16.1 ❯ git clone g...@github.com:theory/kv-pair.git ❯ cd kv-pair ❯ echo directory = 'blah' >> pair.control ❯ cat pair.control # pair extension comment = 'A key/value pair data type' default_version = '0.1.2' module_pathname = '$libdir/pair' relocatable = true directory = blah ❯ make install cp sql/pair.sql sql/pair--0.1.2.sql /opt/homebrew/bin/gmkdir -p '/Users/david/.pgenv/pgsql-16.1/share/extension' /opt/homebrew/bin/gmkdir -p '/Users/david/.pgenv/pgsql-16.1/share/extension' /opt/homebrew/bin/gmkdir -p '/Users/david/.pgenv/pgsql-16.1/share/doc/extension' /opt/homebrew/bin/ginstall -c -m 644 .//pair.control '/Users/david/.pgenv/pgsql-16.1/share/extension/' /opt/homebrew/bin/ginstall -c -m 644 .//sql/pair--unpackaged--0.1.2.sql '/Users/david/.pgenv/pgsql-16.1/share/extension/' /opt/homebrew/bin/ginstall -c -m 644 .//doc/pair.md '/Users/david/.pgenv/pgsql-16.1/share/doc/extension/‘ ❯ find ~/.pgenv/pgsql-16.1/ -name '*pair*.*' /Users/david/.pgenv/pgsql-16.1//include/server/lib/pairingheap.h /Users/david/.pgenv/pgsql-16.1//share/extension/pair--unpackaged--0.1.2.sql /Users/david/.pgenv/pgsql-16.1//share/extension/pair.control /Users/david/.pgenv/pgsql-16.1//share/doc/extension/pair.md I see two problems: 1. pair--0.1.2.sql is not installed. It does install without `directory` set. 2. pair--unpackaged--0.1.2.sql is installed, but not in a `blah` directory. Shouldn’t it be? Am I missing something? Best, David
Re: Bug: The "directory" control parameter does not work
On Jan 26, 2024, at 4:40 PM, David E. Wheeler wrote: > But it doesn’t seem to work. I tried this experiment: I made a simpler test case here: https://github.com/theory/test-extension-directory Not the difference between actual and expected output. Best, David
Re: Bug: The "directory" control parameter does not work
On Jan 26, 2024, at 5:05 PM, David E. Wheeler wrote: > I made a simpler test case here: > > https://github.com/theory/test-extension-directory > > Not the difference between actual and expected output. Bah! Need to also set `MODULEDIR = extension/click` and then it works. D
Re: to_regtype() Raises Error
Hey there, coming back to this. I poked at the logs in the master branch and saw no mention of to_regtype; did I miss it? On Sep 17, 2023, at 10:58 PM, David G. Johnston wrote: > Parses a string of text, extracts a potential type name from it, and > translates that name into an OID. Failure to extract a valid potential type > name results in an error while a failure to determine that the extracted name > is known to the system results in a null output. > > I take specific exception to describing your example as a “textual type name”. More docs seem like a reasonable compromise. Perhaps it’d be useful to also describe when an error is likely and when it’s not. Best, David
Re: to_regtype() Raises Error
On Feb 2, 2024, at 3:23 PM, David G. Johnston wrote: > Seems like most just want to leave well enough alone and deal with the rare > question for oddball input on the mailing list. If you are interested enough > to come back after 4 months I'd suggest you write up and submit a patch. I'm > happy to review it and see if that is enough to get a committer to respond. LOL, “interested enough” is less the right term than “triaging email backlog and following up on a surprisingly controversial question.” I also just like to see decisions made and issues closed one way or another. Anyway, I’m happy to submit a documentation patch along the lines you suggested. D
Patch: Add parse_type Function
Hackers, Attached is a patch to add a new function, `parse_type()`. It parses a type string and returns a record with the typid and typmod for the type, or raises an error if the type is invalid. It’s effectively a thin layer over the parser’s parseTypeString() function. The purpose of this function is to allow uses to resolve any type name, including aliases, to its formal name as rendered by, for example, pg_dump. An example: david=# WITH s(s) AS ( SELECT * FROM unnest(ARRAY[ 'timestamp(4)', 'interval(0)', 'interval second(0)', 'timestamptz', 'timestamptz(6)', 'varchar', 'varchar(128)' ]) ), p(type, typid, typmod) AS ( SELECT s, ((parse_type(s)).*) FROM s ) SELECT type, format_type(typid, typmod) FROM p; type| format_type + timestamp(4) | timestamp(4) without time zone interval(0)| interval(0) interval second(0) | interval second(0) timestamptz| timestamp with time zone timestamptz(6) | timestamp(6) with time zone varchar| character varying varchar(128) | character varying(128) (7 rows) The use case leading to this patch was pgTAP column data type assertions. pgTAP traditionally required full type names for testing data types, but recently added support for aliases. This broke for some types, because it was difficult to extract the typmod correctly, and even when we did, it failed for types such as `interval second(0)`, where `second (0)` is the typmod, and there was no sensible way to access the bit mask to generate the proper typmod to pass to `format_type()`. Erik Wienhold wrote this function to solve the problem, and I volunteered to submit a patch. Potential issues and questions for this patch: * Is `parse_type()` the right name to use? I can see arguments for `parse_type_string()`, `pg_parse_type()`, and `pg_parse_type_string()`. Whatever the consensus is works for me. * The function returns a record, which is a little fussy. I could see adding a `format_type_string()` function that just contains `SELECT format_type(p.typmod, p.typid) FROM parse_type($1) p` and returns the formatted type. But I think it might also be useful to have access to the typmod and typid directly via this method, since they’re already exposed as `format_type()`’s arguments. * Is format_type.c the right home for the function? I put it there because I closely associate it with format_type(), but happy to move it to a more appropriate location. * I tried to link to `format_type` from the docs entry for `parse_type`, and added an ID for the former, but I’m not sure it resolves. Best, David v1-0001-Add-parse_type-SQL-function.patch Description: Binary data
Re: Patch: Add parse_type Function
On Feb 4, 2024, at 13:02, Pavel Stehule wrote: > I thinks so proposed functionality can be useful Great, thank you! Is that a review? :-) D
Re: Patch: Add parse_type Function
On Feb 4, 2024, at 13:52, Pavel Stehule wrote: > not yet, but I'll do it Nice, thank you. I put it into the Commitfest. https://commitfest.postgresql.org/47/4807/ Best, David
Re: to_regtype() Raises Error
On Feb 2, 2024, at 15:33, David E. Wheeler wrote: > Anyway, I’m happy to submit a documentation patch along the lines you > suggested. How’s this? --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -25460,11 +25460,12 @@ SELECT collation for ('foo' COLLATE "de_DE"); regtype -Translates a textual type name to its OID. A similar result is +Parses a string of text, extracts a potential type name from it, and +translates that name into an OID. A similar result is obtained by casting the string to type regtype (see -); however, this function will return -NULL rather than throwing an error if the name is -not found. +). Failure to extract a valid potential +type name results in an error; however, if the extracted names is not +known to the system, this function will return NULL. Does similar wording need to apply to other `to_reg*` functions? Best, David v1-0001-Update-to_regtype-docs-regarding-error-condition.patch Description: Binary data
Re: to_regtype() Raises Error
On Feb 4, 2024, at 19:11, Erik Wienhold wrote: > Here "extracted names" should be "extracted name" (singular). > Otherwise, the text looks good. Ah, thank you. Updated patch attached. Best, David v2-0001-Update-to_regtype-docs-regarding-error-condition.patch Description: Binary data
Re: Patch: Add parse_type Function
On Feb 5, 2024, at 08:02, Jim Jones wrote: > There are a few issues though: Excellent, thank you very much! Updated patch attached. Best, David v2-0001-Add-parse_type-SQL-function.patch Description: Binary data
Re: Patch: Add parse_type Function
On Feb 5, 2024, at 09:49, Jim Jones wrote: > Yes, if the function was strict (which in the current design is not the > case) there is no need to check for nulls. Right, done, and cleaned up the redundant comments. Best, David v3-0001-Add-parse_type-SQL-function.patch Description: Binary data
Re: Patch: Add parse_type Function
On Feb 5, 2024, at 10:47 AM, Jim Jones wrote: > The patch applies cleanly and it no longer crashes with NULL parameters. > Docs render fine and regression tests pass. I have nothing more to add. > Let's now wait for Pavel's review. Much obliged! D
Re: Patch: Add parse_type Function
On Feb 10, 2024, at 20:52, Erik Wienhold wrote: > > Let me comment on some issues since I wrote the very first version of > parse_type() on which David's patch is based. Thanks Erik. >> On 2024-02-01 01:00 +0100, jian he wrote: >> if you are wondering around other code deal with NULL, ErrorSaveContext. >> NULL would be more correct? >> `(void) parseTypeString(type, &typid, &typmod, NULL);` Fixed. >> also parseTypeString already explained the third argument, our >> comments can be simplified as: >> /* >> * Parse type-name argument to obtain type OID and encoded typmod. We don't >> * need to handle parseTypeString failure, just let the error be >> * raised. >> */ Thanks, adopted that language. >> cosmetic issue. Looking around other error handling code, the format >> should be something like: >> ` >> if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) >>ereport(ERROR, >> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), >>errmsg("function returning record called in" >> "context that cannot accept type record"))); >> ` > > +1 I poked around and found some other examples, yes. I went with a single long line for errmsg following the example in adminpack.c[1] >> `PG_FUNCTION_INFO_V1(parse_type);` >> is unnecessary? >> based on the error information: https://cirrus-ci.com/task/5899457502380032 >> not sure why it only fails on windows. > > Yeah, it's not necessary for internal functions per [1]. It's a > leftover from when this function was part of the pgtap extension. Removed. >> +#define PARSE_TYPE_STRING_COLS 2 /* Returns two columns. */ >> +#undef PARSE_TYPE_STRING_COLS >> Are these necessary? >> given that the comments already say return the OID and type modifier. > > Not sure what the convention here is but I found this in a couple of > places, maybe even a tutorial on writing C functions. See > `git grep '_COLS\s\+[1-9]'` for those instances. It's in line with my > habit of avoiding magic constants. Left in place for now. > >> +( typid oid, >> + typmod int4 ) >> here `int4` should be `integer`? > > +1 Fixed. > Yes, the sentence is rather handwavy. What is meant here is that this > function also resolves types whose typmod is determined by the SQL > parser or some step after that. There are types whose typmod is > whatever value is found inside the parenthesis, e.g. bit(13) has typmod > 13. Our first idea before coming up with that function was to do some > string manipulation and extract the typmod from inside the parenthesis. > But we soon found out that other typmods don't translate one to one, > e.g. varchar(13) has typmod 17. The interval type is also special > because the typmod is some bitmask encoding of e.g. 'second(0)'. Hence > the mention of the SQL grammar. I adopted some of your language here --- and fixed the spelling errors :-) >> can be simplified: >> + >> +Parses a string representing an SQL data type, optionally >> schema-qualified. >> +Returns a record with two fields, typid and >> +typmod, representing the OID and >> modifier for the >> +type. These correspond to the parameters to pass to the >> +format_type >> function. >> + >> (I don't have a strong opinion though) > > Sounds better. The CREATE TABLE reference is superfluous. Done. Best, David [1] https://github.com/postgres/postgres/blob/09eb633e1baa3b7cd7929f3cc77f9c46f63c20b1/contrib/adminpack/adminpack.c#L508-L511 v4-0001-Add-parse_type-SQL-function.patch Description: Binary data
Re: Patch: Add parse_type Function
On Feb 12, 2024, at 12:53 PM, Tom Lane wrote: > "David E. Wheeler" writes: >> [ v4-0001-Add-parse_type-SQL-function.patch ] > > It strikes me that this is basically to_regtype() with the additional > option to return the typmod. That leads to some questions: Huh. I saw it more on a par with format_type(), at least in the output I expect. > * Should we choose a name related to to_regtype? I don't have any > immediate suggestions, but since you didn't seem entirely set on > parse_type, maybe it's worth thinking along those lines. OTOH, > to the extent that people would use this with format_type, maybe > parse_type is fine. Yeah, and the `to_*` functions return a single OID, not two fields. > * Perhaps the type OID output argument should be declared regtype > not plain OID? It wouldn't make any difference for passing it to > format_type, but in other use-cases perhaps regtype would be more > readable. It's not a deal-breaker either way of course, since > you can cast oid to regtype or vice versa. Sure. I used `oid` because that’s exactly what the argument to format_type() is called, so thought the parity there more appropriate. Maybe its argument should be renamed to regtype? > * Maybe the code should be in adt/regproc.c not format_type.c. Happy to move it wherever makes the most sense. > * Experience with to_regtype suggests strongly that people would > prefer "return NULL" to failure for an unrecognized type name. Could do, but as with to_regtype() there’s an issue with error handling when it descends into the SQL parser. > Skimming the patch, I notice that the manual addition to > builtins.h should be unnecessary: the pg_proc.dat entry > should be enough to create an extern in fmgrprotos.h. Confirmed, will remove in next patch. > Also, I'm pretty sure that reformat_dat_file.pl will > think your pg_proc.dat entry is overly verbose. See > https://www.postgresql.org/docs/devel/system-catalog-initial-data.html There are a bunch of things it reformats: ❯ git diff --name-only src/include/catalog/pg_aggregate.dat src/include/catalog/pg_database.dat src/include/catalog/pg_proc.dat src/include/catalog/pg_type.dat src/include/utils/builtins.h And even in pg_proc.dat there are 13 blocks it reformats. I can submit with just my block formatted if you’d prefer. > BTW, another way that this problem could be approached is to use > to_regtype() as-is, with a separate function to obtain the typmod: > > select format_type(to_regtype('timestamp(4)'), to_regtypmod('timestamp(4)')); Oh interesting. > This is intellectually ugly, since it implies parsing the same > typename string twice. But on the other hand it avoids the notational > pain and runtime overhead involved in using a record-returning > function. So I think it might be roughly a wash for performance. > Question to think about is which way is easier to use. I don't > have an opinion particularly; just throwing the idea out there. Honestly I haven’t cared for the fact that parse_type() returns a record; it makes it a bit clunky. But yeah, so does this to pass the same value to two function calls. Perhaps it makes sense to add to_regtypmod() as you suggest, but then also something like: CREATE FUNCTION format_type_string(text) RETURNS TEXT AS $$ SELECT format_type(to_regtype($1), to_regtypmod($1)) $$; Best, David
Re: Patch: Add parse_type Function
On Feb 12, 2024, at 15:55, David E. Wheeler wrote: > Happy to move it wherever makes the most sense. Update with a bunch of the suggested changes. Some questions still open from the previous post, though. Best, David v5-0001-Add-parse_type-SQL-function.patch Description: Binary data
Re: Patch: Add parse_type Function
On Feb 18, 2024, at 15:55, Erik Wienhold wrote: >> The overhead of parse_type_and_format can be related to higher planning >> time. PL/pgSQL can assign composite without usage FROM clause. > > Thanks, didn't know that this makes a difference. In that case both > variants are on par. Presumably this is a side-effect of the use of a RECORD here, which requires a FROM clause in pure SQL, yes? The only way I can think of to avoid that is to: 1. Add a to_regtypmod() for those who just want the typemod. 2. Add a format_type_string() function that returns a string, the equivalent of this function but in C: CREATE FUNCTION format_type_string(text) RETURNS TEXT AS $$ SELECT format_type(to_regtype($1), to_regtypmod($1)) $$; We could also keep the record-returning function for users who want both the regtype and the regytypemod at once, but with the other two I consider it less essential. Thoughts? Best, David
Re: Patch: Add parse_type Function
On Feb 19, 2024, at 15:47, Tom Lane wrote: >> 1. Add a to_regtypmod() for those who just want the typemod. > > Seems like there's a good case for doing that. I’ll work on that. > I'm less thrilled about that, mainly because I can't think of > a good name for it ("format_type_string" is certainly not that). > Is the use-case for this functionality really strong enough that > we need to provide it as a single function rather than something > assembled out of spare parts? Not essential for pgTAP, no, as we can just use the parts. It was the typmod bit that was missing. On Feb 19, 2024, at 17:13, Tom Lane wrote: > After some time ruminating, a couple of possibilities occurred to > me: > reformat_type(text) > canonical_type_name(text) I was just thinking about hitting the thesaurus entry for “canonical”, but I quite like reformat_type. It’s super clear and draws the parallel to format_type() more clearly. Will likely steal the name for pgTAP if we don’t add it to core. :-) > I'm still unconvinced about that, though. I guess the questions are: * What are the other use cases for it? * How obvious is it how to do it? For the latter, it could easily be an example in the docs. Best, David
Re: Patch: Add parse_type Function
On Feb 19, 2024, at 21:58, Erik Wienhold wrote: > See the patch I wrote for my benchmarks. But it's pretty easy anyway to > cut down parse_type() ;) LOL, I missed that, just wrote it myself in the last hour. :-) v6 attached. > But you don't actually need reformat_type() in pgTAP. You can just get > the type OID and modifier of the want_type and have_type and compare > those. Then use format_type() for the error message. Looks a bit > cleaner to me than doing the string comparison. Fair. > On second thought, I guess comparing the reformatted type names is > necessary in order to have a uniform API on older Postgres releases > where pgTAP has to provide its own to_regtypmod() based on typmodin > functions. Maybe. Worth playing with. >> For the latter, it could easily be an example in the docs. > > Can be mentioned right under format_type(). Well I included it in the to_regtypemod docs here, but could so either. Best, David v6-0001-Add-to_regtypemod-SQL-function.patch Description: Binary data
Re: Patch: Add parse_type Function
On Feb 20, 2024, at 01:30, jian he wrote: > the second hint `-- grammar error expected` seems to contradict with > the results? Quite right, thank you, that’s actually a trapped error. I’ve tweaked the comments and their order in v7, attached. This goes back to the discussion of the error raising of to_regtype[1], so I’ve incorporated the patch from that thread into this patch, and set up the docs for to_regtypemod() with similar information. The wording is still a little opaque for my taste, though, written more for someone who knows a bit about the internals, but it’s a start. I’ve also fixed the wayward parameter in the function signature in the docs, and added a note about why I’ve also patched genbki.pl. Best, David [1] https://www.postgresql.org/message-id/flat/57E1FDDC-5A38-452D-82D7-A44DA2E13862%40justatheory.com#1ae0b11634bc33c7ad3cd728e43d504e v7-0001-Add-to_regtypemod-SQL-function.patch Description: Binary data
Re: to_regtype() Raises Error
On Feb 5, 2024, at 09:01, David E. Wheeler wrote: > Ah, thank you. Updated patch attached. I’ve moved this patch into the to_regtype patch thread[1], since it exhibits the same behavior. Best, David [1] https://www.postgresql.org/message-id/60ef4e11-bc1c-4034-b37b-695662d28...@justatheory.com
Re: Patch: Add parse_type Function
On Feb 20, 2024, at 21:09, Erik Wienhold wrote: > The references are printed as "???" right now. Can be fixed with > xreflabel="format_type" and xreflabel="to_regtype" on those > elements. Thanks. > The docs show parameter name "type" but pg_proc.data does not define > proargnames. So the to_regtypemod() cannot be called using named > notation: > > test=> select to_regtypemod(type => 'int'::text); > ERROR: function to_regtypemod(type => text) does not exist Yes, this format is identical to that of to_regtype(): david=# select to_regtype(type => 'int'::text); ERROR: function to_regtype(type => text) does not exist > +Parses a string of text, extracts a potential type name from it, and >> +translates its typmod, the modifier for the type, if any. Failure to > > s/typmod, the modifier of the type/type modifier/ > > Because format_type() already uses "type modifier" and I think it helps > searchability to use the same wording. Yes, definitely better wording, thanks. V8 attached. Best, David v8-0001-Add-to_regtypemod-SQL-function.patch Description: application/applefile
Re: Patch: Add parse_type Function
On Feb 21, 2024, at 11:18, Erik Wienhold wrote: > Thanks. But it's an applefile again, not a patch :P WTF. I still have that feature disabled. Oh, I think I deleted the file after dragged it into Mail but before sending, because it’s empty everywhere I look. 🤦🏻♂️ Let’s try that again. Best, David v8-0001-Add-to_regtypemod-SQL-function.patch Description: Binary data
Re: to_regtype() Raises Error
On Feb 21, 2024, at 11:54 AM, David Wheeler wrote: > Merged this change into the [to_regtypemod > patch](https://commitfest.postgresql.org/47/4807/), which has exactly the > same issue. > > The new status of this patch is: Needs review Bah, withdrawn. D
Re: Patch: Add parse_type Function
On Feb 21, 2024, at 16:43, Erik Wienhold wrote: > The docs still state that to_regtypemod() has a named parameter, which > is not the case per pg_proc.dat. Bah, I cleaned it up before but somehow put it back. Thanks for the catch. Fixed. Best, David v9-0001-Add-to_regtypemod-SQL-function.patch Description: Binary data
Re: Patch: Add parse_type Function
On Feb 21, 2024, at 17:19, Erik Wienhold wrote: > Thanks David! LGTM. Thanks. Anyone else? Or is it ready for committer? Best, David
Re: Patch: Add parse_type Function
On Feb 21, 2024, at 19:13, David E. Wheeler wrote: > Thanks. Anyone else? Or is it ready for committer? What’s the protocol for marking a patch ready for committer? Thanks, David
Re: Patch: Add parse_type Function
On Feb 24, 2024, at 19:11, Jim Jones wrote: >> What’s the protocol for marking a patch ready for committer? > > I guess after the review of the last assigned reviewer Oh, I didn’t realize someone was assigned. :-) > The fact that a completely invalid type returns NULL .. > > SELECT to_regtypemod('foo'); > to_regtypemod > --- > > (1 row) > > > .. but a "partially" valid one returns an error might be confusing > > postgres=# SELECT to_regtypemod('timestamp(-4)'); > ERROR: syntax error at or near "-" > LINE 1: SELECT to_regtypemod('timestamp(-4)'); > ^ > CONTEXT: invalid type name "timestamp(-4)" > > postgres=# SELECT to_regtypemod('text(-4)'); > ERROR: type modifier is not allowed for type "text" Yeah, there was quite a bit of discussion of this issue back in September[1]. > This behaviour is mentioned in the documentation, so I'd say it is ok. This is my attempt to make it clearer that it can return an error, but I don’t love the wording TBH. > I would personally prefer either NULL or an error in both cases, but I > can totally live with the current design. SAME. Best, David [1] https://www.postgresql.org/message-id/flat/09f9cad6-5096-43cc-b6a7-685703e47...@justatheory.com
Re: An improved README experience for PostgreSQL
On Feb 28, 2024, at 1:51 PM, Alvaro Herrera wrote: > *IF* people don't go overboard, yes. I agree, but let's keep an eye so > that it doesn't become an unreadable mess. I've seen some really > horrible markdown files that I'm sure most of you would object to. Markdown++ IME the keys to decent-looking Markdown are: 1. Wrapping lines to a legible width (76-80 chars) 2. Link references rather than inline links I try to follow these for my blog; posts end up looking like this: https://justatheory.com/2024/02/extension-metadata-typology.text (Append `.text` to any post to see the raw(ish) Markdown. Best, David
Re: Patch: Add parse_type Function
Hello Hackers, On Feb 25, 2024, at 13:00, David E. Wheeler wrote: >> postgres=# SELECT to_regtypemod('timestamp(-4)'); >> ERROR: syntax error at or near "-" >> LINE 1: SELECT to_regtypemod('timestamp(-4)'); >> ^ >> CONTEXT: invalid type name "timestamp(-4)" >> >> postgres=# SELECT to_regtypemod('text(-4)'); >> ERROR: type modifier is not allowed for type "text" > > Yeah, there was quite a bit of discussion of this issue back in September[1]. > >> This behaviour is mentioned in the documentation, so I'd say it is ok. > > This is my attempt to make it clearer that it can return an error, but I > don’t love the wording TBH. I’ve rebased the patch and, in an attempt to clarify this behavior, added a couple of examples to the docs for to_regtype. Updated patch attached. Best, David v10-0001-Add-to_regtypemod-SQL-function.patch Description: Binary data
Re: Patch: Add parse_type Function
On Mar 7, 2024, at 23:39, Erik Wienhold wrote: > I think you need to swap the examples. The text mentions the error case > first and the NULL case second. 🤦🏻♂️ Thanks, fixed in the attached patch. David v11-0001-Add-to_regtypemod-SQL-function.patch Description: Binary data
Q: Escapes in jsonpath Idents
Hackers, The jsonpath doc[1] has an excellent description of the format of strings, but for unquoted path keys, it simply says: > Member accessor that returns an object member with the specified key. If the > key name matches some named variable starting with $ or does not meet the > JavaScript rules for an identifier, it must be enclosed in double quotes to > make it a string literal. I went looking for the JavaScript rules for an identifier and found this in the MDN docs[2]: > In JavaScript, identifiers can contain Unicode letters, $, _, and digits > (0-9), but may not start with a digit. An identifier differs from a string in > that a string is data, while an identifier is part of the code. In > JavaScript, there is no way to convert identifiers to strings, but sometimes > it is possible to parse strings into identifiers. However, the Postgres parsing of jsonpath keys appears to follow the same rules as strings, allowing backslash escapes: david=# select '$.fo\u00f8 == $x'::jsonpath; jsonpath --- ($."foø" == $"x") This would seem to contradict the documentation. Is this behavior required by the SQL standard? Do the docs need updating? Or should the code actually follow the JSON identifier behavior? Thanks, David PS: Those excellent docs on strings mentions support for \v, but the grammar in the right nav of https://www.json.org/json-en.html does not. Another bonus feature? [1]: https://www.postgresql.org/docs/16/datatype-json.html#DATATYPE-JSONPATH [2]: https://developer.mozilla.org/en-US/docs/Glossary/Identifier
Re: Q: Escapes in jsonpath Idents
On Mar 16, 2024, at 14:39, David E. Wheeler wrote: > I went looking for the JavaScript rules for an identifier and found this in > the MDN docs[2]: > >> In JavaScript, identifiers can contain Unicode letters, $, _, and digits >> (0-9), but may not start with a digit. An identifier differs from a string >> in that a string is data, while an identifier is part of the code. In >> JavaScript, there is no way to convert identifiers to strings, but sometimes >> it is possible to parse strings into identifiers. Coda: Dollar signs don’t work at all outside double-quoted string identifiers: david=# select '$.$foo'::jsonpath; ERROR: syntax error at or near "$foo" of jsonpath input LINE 1: select '$.$foo'::jsonpath; ^ david=# select '$.f$oo'::jsonpath; ERROR: syntax error at or near "$oo" of jsonpath input LINE 1: select '$.f$oo'::jsonpath; ^ david=# select '$."$foo"'::jsonpath; jsonpath -- $."$foo" This, too, contradicts the MDM definition an identifier (and some quick browser tests). Best, David
Re: Q: Escapes in jsonpath Idents
On Mar 17, 2024, at 15:12, Erik Wienhold wrote: > Hi David, Hey Erik. Thanks for the detailed reply and patch! > So I think it makes sense to reword the entire backslash part of the > paragraph and remove references to JSON entirely. The attached patch > does that and also formats the backslash escapes as a bulleted list for > readability. Ah, it’s JavaScript format, not JSON! This does clarify things quite nicely, thank you. Happy to add my review once it’s in a commit fest. > The first case ($.$foo) is in line with the restriction on member > accessors that you quoted first. Huh, that’s now how I read it. Here it is again: >> Member accessor that returns an object member with the specified >> key. If the key name matches some named variable starting with $ or >> does not meet the JavaScript rules for an identifier, it must be >> enclosed in double quotes to make it a string literal. Note that in my example `$foo` does not match a variable. I mean it looks like a variable, but none is used here. I guess it’s being conservative because it might be used in one of the functions, like jsonb_path_exists(), to which variables might be passed. > The error message 'syntax error at or near "$oo" of jsonpath input' for > the second case ($.f$oo), however, looks as if the scanner identifies > '$oo' as a variable instead of contiuing the scan of identifier (f$oo) > for the member accessor. Looks like a bug to me because a variable > doesn't even make sense in that place. Right. Maybe the docs should be updated to say that a literal dollar sign isn’t supported in identifiers, unlike in JavaScript, except through escapes like this: > What works though, besides double quoting, is escaping the dollar sign: > > regress=# select '$.\u0024foo'::jsonpath; > jsonpath > -- > $."$foo" > (1 row) > > And we've come full circle :) 🎉 Best, David
Re: JSONPath Child Operator?
On Jan 30, 2023, at 08:17, Filipp Krylov wrote: >> My question: Are there plans to support square bracket syntax for JSON >> object field name strings like this? Or to update to follow the standard as >> it’s finalized? > > This syntax is a part of "jsonpath syntax extensions" patchset: > https://www.postgresql.org/message-id/e0fe4f7b-da0b-471c-b3da-d8adaf314357%40postgrespro.ru Nice, thanks. I learned since sending this email that SQL/JSON Path is not at all the same as plain JSON Path, so now I’m less concerned bout it. I like the new object subscript syntax, though, I’ve been thinking about this myself. D
Re: JSON Path and GIN Questions
On Sep 17, 2023, at 18:09, David E. Wheeler wrote: > I think this is useful, but also that it’s worth calling out explicitly that > functions do not count as indexable operators. True by definition, of course, > but I at least had assumed that since an operator is, in a sense, syntax > sugar for a function call, they are in some sense the same thing. > > A header might be useful, something like “What Counts as an indexable > expression”. Hey Tom, are you still thinking about adding this bit to the docs? I took a quick look at master and didn’t see it there. Thanks, David
GIN-Indexable JSON Patterns
Hey Hackers, Quick follow-up to my slew of questions back in [September][1]. I wanted to update [my patch][2] to note that only JSON Path equality operators are supported by indexes, as [previously discussed][3]. I thought perhaps adding a note to this bit of the docs would be useful: > For these operators, a GIN index extracts clauses of the form accessors_chain > = constant out of the jsonpath pattern, and does the index search based on > the keys and values mentioned in these clauses. The accessors chain may > include .key, [*], and [index] accessors. The jsonb_ops operator class also > supports .* and .** accessors, but the jsonb_path_ops operator class does not. But perhaps that’s what `accessors_chain = constant` is supposed to mean? I’m not super clear on it, though, since the operator is `==` and not `=` (and I would presume that `!=` would use the index, as well. Is that correct? If so, how would you feel about something like this? --- a/doc/src/sgml/json.sgml +++ b/doc/src/sgml/json.sgml @@ -513,7 +513,7 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @@ '$.tags[*] == "qui"'; For these operators, a GIN index extracts clauses of the form accessors_chain -= constant out of +== constant out of the jsonpath pattern, and does the index search based on the keys and values mentioned in these clauses. The accessors chain may include .key, @@ -522,6 +522,9 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @@ '$.tags[*] == "qui"'; The jsonb_ops operator class also supports .* and .** accessors, but the jsonb_path_ops operator class does not. +Only the == and != SQL/JSON Path Operators +can use the index. Best, David [1]: https://www.postgresql.org/message-id/15dd78a5-b5c4-4332-acfe-55723259c...@justatheory.com [2]: https://commitfest.postgresql.org/45/4624/ [3]: https://www.postgresql.org/message-id/973d6495-cf28-4d06-7d46-758bd2615...@xs4all.nl [4]: https://www.postgresql.org/docs/current/datatype-json.html#JSON-INDEXING
Re: JSON Path and GIN Questions
On Dec 17, 2023, at 16:08, Tom Lane wrote: > I'd waited because the discussion was still active, and then it > kind of slipped off the radar. I'll take another look and push > some form of what I suggested. Right on. > That doesn't really address the > jsonpath oddities you were on about, though. No, I attempted to address those in [a patch][1]. [1]: https://commitfest.postgresql.org/45/4624/ Best, David
Re: GIN-Indexable JSON Patterns
On Dec 17, 2023, at 13:10, David E. Wheeler wrote: > Quick follow-up to my slew of questions back in [September][1]. I wanted to > update [my patch][2] to note that only JSON Path equality operators are > supported by indexes, as [previously discussed][3]. Should I just add it to the patch and let the reviews fall where they may? :-) Best, David [1]: https://www.postgresql.org/message-id/15dd78a5-b5c4-4332-acfe-55723259c...@justatheory.com [2]: https://commitfest.postgresql.org/45/4624/ [3]: https://www.postgresql.org/message-id/973d6495-cf28-4d06-7d46-758bd2615...@xs4all.nl
JSONPath Child Operator?
Greetings! Long time no see, I know. How are you, Hackers? I notice from the docs in the Postgres JSONPath type, brackets are described as: > • Square brackets ([]) are used for array access. https://www.postgresql.org/docs/current/datatype-json.html#DATATYPE-JSONPATH Notably they are not used for object field path specifications, in contrast to the original JSON Path design, which says: > JSONPath expressions can use the dot–notation > > $.store.book[0].title > > or the bracket–notation > > $['store']['book'][0]['title'] https://goessner.net/articles/JsonPath/index.html#e2 Similarly, the current IETF RFC Draft says: > JSONPath expressions use the _bracket notation_, for example: > >$['store']['book'][0]['title'] > >or the more compact _dot notation_, for example: > >$.store.book[0].title https://datatracker.ietf.org/doc/draft-ietf-jsonpath-base/ My question: Are there plans to support square bracket syntax for JSON object field name strings like this? Or to update to follow the standard as it’s finalized? Thanks, David
Re: JSON Path and GIN Questions
On Sep 12, 2023, at 21:00, Erik Wienhold wrote: >> I posted this question on Stack Overflow >> (https://stackoverflow.com/q/77046554/79202), >> and from the suggestion I got there, it seems that @@ expects a boolean to be >> returned by the path query, while @? wraps it in an implicit exists(). Is >> that >> right? > > That's also my understanding. We had a discussion about the docs on @@, @?, > and > jsonb_path_query on -general a while back [1]. Maybe it's useful also. Hi, finally getting back to this, still fiddling to figure out the differences. From the thread you reference [1], is the point that @@ and jsonb_path_match() can only be properly used with a JSON Path expression that’s a predicate check? If so, as far as I can tell, only exists() around the entire path query, or the deviation from the SQL standard that allows an expression to be a predicate? This suggest to me that the "Only the first item of the result is taken into account” bit from the docs may not be quite right. Consider this example: david=# select jsonb_path_query('{"a":[false,true,false]}', '$.a ?(@[*] == false)'); jsonb_path_query -- false false (2 rows) david=# select jsonb_path_match('{"a":[false,true,false]}', '$.a ?(@[*] == false)'); ERROR: single boolean result is expected jsonb_path_match(), it turns out, only wants a single result. But furthermore perhaps the use of a filter predicate rather than a predicate expression for the entire path query is an error? Curiously, @@ seems okay with it: david=# select '{"a":[false,true,false]}'@@ '$.a ?(@[*] == false)'; ?column? -- t Not a predicate query, and somehow returns true even though the first item of the result is false? Is that how it should be? Best, David [1] https://www.postgresql.org/message-id/CACJufxE01sxgvtG4QEvRZPzs_roggsZeVvBSGpjM5tzE5hMCLA%40mail.gmail.com
Re: JSON Path and GIN Questions
Thanks for the reply, Erik. Have appreciated collaborating with you on a few different things lately! > On Oct 13, 2023, at 22:50, Erik Wienhold wrote: >> Hi, finally getting back to this, still fiddling to figure out the >> differences. From the thread you reference [1], is the point that @@ >> and jsonb_path_match() can only be properly used with a JSON Path >> expression that’s a predicate check? > > I think so. That's also supported by the existing docs which only > mention "JSON path predicate" for @@ and jsonb_path_match(). Okay, good. >> If so, as far as I can tell, only exists() around the entire path >> query, or the deviation from the SQL standard that allows an >> expression to be a predicate? > > Looks like that. But note that exists() is also a filter expression. > So wrapping the entire jsonpath in exists() is also a deviation from the > SQL standard which only allows predicates in filter expressions, i.e. > ' ? ()'. Yeah. I’m starting to get the sense that the Postgres extension of the standard to allow predicates without filters is almost a different thing, like there are two Pg SQL/JSON Path languages: 1. SQL Standard path language for selecting values and includes predicates. Returns the selected value(s). Supported by `@?` and jsonb_path_exists(). 2. The Postgres predicate path language which returns a boolean, akin to a WHERE expression. Supported by `@@` and jsonb_path_match() Both are supported by jsonb_path_query(), but if you use a standard path you get the values and if you use a predicate path you get a boolean. This feels a big overloaded to me, TBH; I find myself wanting them to be separate types since the behaviors vary quite a bit! >> This suggest to me that the "Only the first item of the result is >> taken into account” bit from the docs may not be quite right. > > Yes, this was also the issue in the referenced thread[1]. I think my > suggesstion in [2] explains it (as far as I understand it). Yeah, lax vs. strict mode stuff definitely creates some added complexity. I see now I missed the rest of that thread; seeing the entire thread on one page[1] really helps. I’d like to take a stab at the doc improvements Tom suggests[2]. >> jsonb_path_match(), it turns out, only wants a single result. But >> furthermore perhaps the use of a filter predicate rather than a >> predicate expression for the entire path query is an error? > > Yes, I think @@ and jsonb_path_match() should not be used with filter > expressions because the jsonpath returns whatever the path expression > yields (which may be an actual boolean value in the jsonb). The filter > expression only filters (as the name suggests) what the path expression > yields. Agreed. It only gets worse with a filter expression that selects a single value: david=# select jsonb_path_match('{"a":[false,true]}', '$.a ?(@[*] == false)'); jsonb_path_match -- f Presumably it returns false because the value selected is JSON `false`: david=# select jsonb_path_query('{"a":[false,true]}', '$.a ?(@[*] == false)'); jsonb_path_query -- false Which seems misleading, frankly. Would it be possible to update jsonb_path_match and @@ to raise an error when the path expression is not a predicate? >> Curiously, @@ seems okay with it: >> >> david=# select '{"a":[false,true,false]}'@@ '$.a ?(@[*] == false)'; >> ?column? >> -- >> t >> >> Not a predicate query, and somehow returns true even though the first >> item of the result is false? Is that how it should be? > > Your example does a text search equivalent to: > > select to_tsvector('{"a":[false,true,false]}') @@ plainto_tsquery('$.a ? > (@[*] == true)') > > You forgot the cast to jsonb. Oh good grief 🤦🏻♂️ > jsonb @@ jsonpath actually returns null: > > test=# select '{"a":[false,true,false]}'::jsonb @@ '$.a ? (@[*] == false)'; > ?column? > -- > > (1 row) Yes, much better, though see the result above that returns a single `false` and confuses things. > This matches the note right after the docs for @@: Yeah, that makes sense. But here’s a bit about lax mode[3] that confuses me: > The lax mode facilitates matching of a JSON document structure and path > expression if the JSON data does not conform to the expected schema. If an > operand does not match the requirements of a particular operation, it can be > automatically wrapped as an SQL/JSON array or unwrapped by converting its > elements into an SQL/JSON sequence before performing this operation. Besides, > comparison operators automatically unwrap their operands in the lax mode, so > you can compare SQL/JSON arrays out-of-the-box. This automatic flattening in lax mode seems odd, because it means you get different results in strict and lax mode where there are no errors. In lax mode, you get a set: david=# select jsonb_path_query('{"a":[1,2,3,4,5]}', '$.a ?(@[*] > 2)'); jsonb_path_query -- 3 4 5 (3 rows) But i
Patch: Improve Boolean Predicate JSON Path Docs
Hackers, Following up from a suggestion from Tom Lane[1] to improve the documentation of boolean predicate JSON path expressions, please find enclosed a draft patch to do so. It does three things: 1. Converts all of the example path queries to use jsonb_path_query() and show the results, to make it clearer what the behaviors are. 2. Replaces the list of deviations from the standards with a new subsection, with each deviation in its own sub-subsection. The regex section is unchanged, but I’ve greatly expanded the boolean expression JSON path section with examples comparing standard filter expressions and nonstandard boolean predicates. I’ve also added an exhortation not use boolean expressions with @? or standard path expressions with @@. 3. While converting the modes section to use jsonb_path_query() and show the results, I also added an example of strict mode returning an error. Follow-ups I’d like to make: 1. Expand the modes section to show how the types of results can vary depending on the mode, thanks to the flattening. Examples: david=# select jsonb_path_query('{"a":[1,2,3,4,5]}', '$.a ?(@[*] > 2)'); jsonb_path_query -- 3 4 5 (3 rows) david=# select jsonb_path_query('{"a":[1,2,3,4,5]}', 'strict $.a ?(@[*] > 2)'); jsonb_path_query -- [1, 2, 3, 4, 5] 2. Improve the descriptions and examples for @?/jsonb_path_exists() and @@/jsonb_path_match(). Best, David [1] https://www.postgresql.org/message-id/1229727.1680535592%40sss.pgh.pa.us jsonpath-pred-docs.patch Description: Binary data
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Oct 14, 2023, at 16:40, David E. Wheeler wrote: > Following up from a suggestion from Tom Lane[1] to improve the documentation > of boolean predicate JSON path expressions, please find enclosed a draft > patch to do so. And now I see I can’t spell “Deviations”. Will fix along with any other requested revisions. GitHub diff here if you’re into that sort of thing: https://github.com/postgres/postgres/compare/master...theory:postgres:jsonpath-pred-docs Best, David
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Oct 14, 2023, at 19:51, Erik Wienhold wrote: > Thanks for putting this together. See my review at the end. Appreciate the speedy review! > Nice. This really does help to make some sense of it. I checked all > queries and they do work out except for two queries where the path > expression string is not properly quoted (but the intended output is > still correct). 🤦🏻♂️ >> Follow-ups I’d like to make: >> >> 1. Expand the modes section to show how the types of results can vary >> depending on the mode, thanks to the flattening. Examples: >> >> david=# select jsonb_path_query('{"a":[1,2,3,4,5]}', '$.a ?(@[*] > 2)'); >> jsonb_path_query >> -- >> 3 >> 4 >> 5 >> (3 rows) >> >> david=# select jsonb_path_query('{"a":[1,2,3,4,5]}', 'strict $.a ?(@[*] > >> 2)'); >> jsonb_path_query >> -- >> [1, 2, 3, 4, 5] >> >> 2. Improve the descriptions and examples for @?/jsonb_path_exists() >> and @@/jsonb_path_match(). > > +1 I planned to submit these changes in a separate patch, based on Tom Lane’s suggestion[1]. Would it be preferred to add them to this patch? > Perhaps make it explicit that the reader must run this in psql in order > to use \set and :'json' in the ensuing samples? Some of the existing > examples already use psql output but they do not rely on any psql > features. Good call, done. > This should use , , and if it shows > a psql session, e.g.: > > > select jsonb_path_query(:'json', '$.track.segments'); > > > jsonb_path_query > --- > [{"HR": 73, "location": [47.763, 13.4034], "start time": "2018-10-14 > 10:05:14"}, {"HR": 135, "location": [47.706, 13.2635], "start time": > "2018-10-14 10:39:21"}] > > I pokwds around, and it appears the computeroutput bit is used for function output. So I followed the precedent in queries.sgml[2] and omitted the computeroutput tags but added prompt, e.g., => select jsonb_path_query(:'json', 'strict $.**.HR'); jsonb_path_query -- 73 135 > Also the cast to jsonb is not necessary and only adds clutter IMO. Right, removed them all in function calls. >> + >> + Predicate-only path expressions are necessary for implementation of >> the >> + @@ operator (and the >> + jsonb_path_match function), and should not be >> used >> + with the @? operator (or >> + jsonb_path_exists function). >> + >> + >> + >> + Conversely, non-predicate jsonpath expressions should >> not be >> + used with the @@ operator (or the >> + jsonb_path_match function). >> + >> + > > Both paras should be wrapped in a single so that they stand out > from the rest of the text. Maybe even , but is already > used on this page for things that I'd consider warnings. Agreed. Would be good if we could teach these functions and operators to reject path expressions they don’t support. >> + >> +Regular Expression Interpretation >> + >> + There are minor differences in the interpretation of regular >> + expression patterns used in like_regex filters, as >> + described in . >> + >> + > > should be closed here, > otherwise the docs won't build. This can be checked with > `make -C doc/src/sgml check`. Thanks. That produces a bunch of warnings for postgres.sgml and legal.sgml (and a failure to load the docbook DTD), but func.sgml is clean now. > `git diff --check` shows a couple of lines with trailing whitespace > (mostly psql output). I must’ve cleaned those after I sent the patch, good now. Updated patch attached, this time created by `git format-patch -v2`. Best, David [1] https://www.postgresql.org/message-id/1229727.1680535592%40sss.pgh.pa.us [2] https://www.postgresql.org/docs/current/queries-table-expressions.html#QUERIES-JOIN v2-0001-Improve-boolean-predicate-JSON-Path-docs.patch Description: Binary data
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Oct 15, 2023, at 23:03, Erik Wienhold wrote: > Your call but I'm not against including it in this patch because it > already touches the modes section. Okay, added, let’s just put all our cards on the table. :-) >> Agreed. Would be good if we could teach these functions and operators >> to reject path expressions they don’t support. > > Right, you mentioned that idea in [1] (separate types). Not sure what > the best strategy here is but it's likely to break existing queries. > Maybe deprecating unsupported path expressions in the next major release > and changing that to an error in the major release after that. Well if the functions have a JsonPathItem struct, they can check its type attribute and reject those with a root type that’s a predicate in @? and reject it if it’s not a predicate in @@. Example of checking type here: https://github.com/postgres/postgres/blob/54b208f90963cb8b48b9794a5392b2fae4b40a98/src/backend/utils/adt/jsonpath_exec.c#L622 >>> This can be checked with `make -C doc/src/sgml check`. >> >> Thanks. That produces a bunch of warnings for postgres.sgml and >> legal.sgml (and a failure to load the docbook DTD), but func.sgml is >> clean now. > > Hmm... I get no warnings on 1f89b73c4e. Did you install all tools as > described in [2]? The DTD needs to be installed as well. Thanks, got it down to one: postgres.sgml:112: element sect4: validity error : Element sect4 content does not follow the DTD, expecting (sect4info? , (title , subtitle? , titleabbrev?) , (toc | lot | index | glossary | bibliography)* , (((calloutlist | glosslist | bibliolist | itemizedlist | orderedlist | segmentedlist | simplelist | variablelist | caution | important | note | tip | warning | literallayout | programlisting | programlistingco | screen | screenco | screenshot | synopsis | cmdsynopsis | funcsynopsis | classsynopsis | fieldsynopsis | constructorsynopsis | destructorsynopsis | methodsynopsis | formalpara | para | simpara | address | blockquote | graphic | graphicco | mediaobject | mediaobjectco | informalequation | informalexample | informalfigure | informaltable | equation | example | figure | table | msgset | procedure | sidebar | qandaset | task | anchor | bridgehead | remark | highlights | abstract | authorblurb | epigraph | indexterm | beginpage)+ , (refentry* | sect5* | simplesect*)) | refentry+ | sect5+ | simplesect+) , (toc | lot | index | glossary | bibliography)*), got (para para ) &func; David v3-0001-Improve-boolean-predicate-JSON-Path-docs.patch Description: application/applefile
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Oct 16, 2023, at 18:07, Erik Wienhold wrote: >> Okay, added, let’s just put all our cards on the table. :-) > > I'll have a look but the attached v3 is not a patch but some applefile. Weird, should be no different from previous attachments. I believe Apple Mail always uses application/octet-stream for attachments it doesn’t recognize, which includes .patch and .diff files, sadly. > One of the added is invalid by the looks of it. Maybe > is missing because it says "got (para para )" at the end. Oh, I thought it would report issues from the files they were found in. You’re right, I forgot a title. Fixed in v4. David v4-0001-Improve-boolean-predicate-JSON-Path-docs.patch Description: Binary data
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Oct 19, 2023, at 01:22, jian he wrote: > "Do not use with non-predicate", double negative is not easy to > comprehend. Maybe we can simplify it. > > 16933: value. Use only SQL-standard JSON path expressions, not not > there are two "not". > > 15842: SQL-standard JSON path expressions, not not > there are two "not”. Thank you, jian. Updated patch attached and also on GitHub. https://github.com/postgres/postgres/compare/master...theory:postgres:jsonpath-pred-docs Best, David v5-0001-Improve-boolean-predicate-JSON-Path-docs.patch Description: application/applefile
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Oct 19, 2023, at 10:49 PM, Erik Wienhold wrote: > Just wanted to take a look at v5. But it's an applefile again :P I don’t get it. It was the other times too! Are you able to save it with a .patch suffix? D
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Oct 22, 2023, at 20:36, Erik Wienhold wrote: > That's an AppleSingle file according to [1][2]. It only contains the > resource fork and file name but no data fork. Ah, I had “Send large attachments with Mail Drop” enabled. To me 20K is not big but whatever. Let’s see if turning it off fixes the issue. > Any reason for calling it "predicate check expressions" (e.g. the link > text) and sometimes "predicate path expressions" (e.g. the linked > section title)? I think it should be named consistently to avoid > confusion and also to simplify searching. I think "predicate path expressions” is more descriptive, but "predicate check expressions” is what was in the docs before, so let’s stick with that. > Linking the same section twice in the same paragraph seems excessive. Fair. Will link the second one. >> +=> select jsonb_path_query(:'json', >> '$.track.segments'); >> +select jsonb_path_query(:'json', '$.track.segments'); > > Please remove the second SELECT. Done. >> +=> select jsonb_path_query(:'json', 'strict >> $.track.segments[0].location'); >> + jsonb_path_query >> +--- >> + [47.763, 13.4034] > > Strict mode is unnecessary to get that result and I'd omit it because > the different modes are not introduced yet at this point. Yep, pasto. > Strict mode is unnecessary here as well. Fixed. >> + using the lax mode. To avoid surprising results, we recommend using >> + the .** accessor only in the strict mode. The > > Please change to "in strict mode" (without "the"). Hrm, I prefer it without the article, too, but it is consistently used that way elsewhere, like here: https://github.com/postgres/postgres/blob/5b36e8f/doc/src/sgml/func.sgml#L17401 I’d be happy to change them all, but was keeping it consistent for now. Updated patch attached, thank you! David v6-0001-Improve-boolean-predicate-JSON-Path-docs.patch Description: Binary data
Re: Patch: Improve Boolean Predicate JSON Path Docs
On Oct 23, 2023, at 20:20, Erik Wienhold wrote: > I thought that you may have missed that one because I saw this change > that removes the article: > >> -In the strict mode, the specified path must exactly match the structure >> of >> +In strict mode, the specified path must exactly match the structure of Oh, didn’t realize. Fixed. > LGTM. Would you create a commitfest entry? I'll set the status to RfC. Done. https://commitfest.postgresql.org/45/4624/ Best, David v7-0001-Improve-boolean-predicate-JSON-Path-docs.patch Description: Binary data
JSON Path and GIN Questions
Greetings Hackers, Been a while! I’m working on some experiments with JSONB columns and GIN indexes, and have operated on the assumption that JSON Path operations would take advantage of GIN indexes, with json_path_ops as a nice optimization. But I’ve run into what appear to be some inconsistencies and oddities I’m hoping to figure out with your help. For the examples in this email, I’m using this simple table: CREATE TABLE MOVIES (id SERIAL PRIMARY KEY, movie JSONB NOT NULL); \copy movies(movie) from PROGRAM 'curl -s https://raw.githubusercontent.com/prust/wikipedia-movie-data/master/movies.json | jq -c ".[]" | sed "s|||g"'; create index on movies using gin (movie); analyze movies; That gives me a simple table with around 3600 rows. Not a lot of data, but hopefully enough to demonstrate the issues. Issue 1: @@ vs @? - I have been confused as to the difference between @@ vs @?: Why do these return different results? david=# select id from movies where movie @@ '$ ?(@.title == "New Life Rescue")'; id (0 rows) david=# select id from movies where movie @? '$ ?(@.title == "New Life Rescue")'; id 10 (1 row) I posted this question on Stack Overflow (https://stackoverflow.com/q/77046554/79202), and from the suggestion I got there, it seems that @@ expects a boolean to be returned by the path query, while @? wraps it in an implicit exists(). Is that right? If so, I’d like to submit a patch to the docs talking about this, and suggesting the use of jsonb_path_query() to test paths to see if they return a boolean or not. Issue 2: @? Index Use - From Oleg’s (happy belated birthday!) notes (https://github.com/obartunov/sqljsondoc/blob/master/jsonpath.md#jsonpath-operators): > Operators @? and @@ are interchangeable: > > js @? '$.a' <=> js @@ 'exists($.a)’ > js @@ '$.a == 1' <=> js @? '$ ? ($.a == 1)’ For the purposes of the above example, this appears to hold true: if I wrap the path query in exists(), @@ returns a result: david=# select id from movies where movie @@ 'exists($ ?(@.title == "New Life Rescue"))'; id 10 (1 row) Yay! However, @@ and @? don’t seem to use an index the same way: @@ uses a GIN index while @? does not. Or, no, fiddling with it again just now, I think I have still been confusing these operators! @@ was using the index with an an explicit exists(), but @? was not…because I was still using an explicit exists. In other words: * @@ 'exists($ ?($.year == 1944))' Uses the index * @? '$ ?(@.year == 1944)' Uses the index * @? 'exists($ ?($.year == 1944))' Does not use the index That last one presumably doesn’t work, because there is an implicit exists() around the exists(), making it `exists(exists($ ?($.year == 1944)))`, which returns true for every row (true and false both exists)! 🤦🏻♂️. Anyway, if I have this right, I’d like to flesh out the docs a bit. Issue 3: Index Use for Comparison - From the docs (https://www.postgresql.org/docs/current/datatype-json.html#JSON-INDEXING), I had assumed any JSON Path query would be able to use the GIN index. However while the use of the == JSON Path operator is able to take advantage of the GIN index, apparently the >= operator cannot: david=# explain analyze select id from movies where movie @? '$ ?($.year >= 2023)'; QUERY PLAN - Seq Scan on movies (cost=0.00..3741.41 rows=366 width=4) (actual time=34.815..36.259 rows=192 loops=1) Filter: (movie @? '$?($."year" >= 2023)'::jsonpath) Rows Removed by Filter: 36081 Planning Time: 1.864 ms Execution Time: 36.338 ms (5 rows) Is this expected? Originally I tried with json_path_ops, which I can understand not working, since it stores hashes of paths, which would allow only exact matches. But a plain old GIN index doesn’t appear to work, either. Should it? Is there perhaps some other op class that would allow it to work? Or would I have to create a separate BTREE index on `movie -> 'year'`? Thanks your your patience with my questions! Best, David signature.asc Description: Message signed with OpenPGP
Re: JSON Path and GIN Questions
On Sep 13, 2023, at 01:11, Erik Rijkers wrote: > "All use of json*() functions preclude index usage." > > That sentence is missing from the documentation. Where did that come from? Why wouldn’t JSON* functions use indexes? I see that the docs only mention operators; why would the corresponding functions behave the same? D signature.asc Description: Message signed with OpenPGP
Re: JSON Path and GIN Questions
On Sep 14, 2023, at 00:41, Tom Lane wrote: > As far as json in particular is concerned, 8.14.4 jsonb Indexing [4] > is pretty clear about what is or is not supported. How do you feel about this note, then? diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml index b6c2ddbf55..7dda727f0d 100644 --- a/doc/src/sgml/json.sgml +++ b/doc/src/sgml/json.sgml @@ -413,6 +413,13 @@ SELECT doc->'site_name' FROM websites Two GIN operator classes are provided, offering different performance and flexibility trade-offs. + + +As with all indexes, only operators on indexed columns are considered as +index qualifications. In other words, only jsonb operators can +take advantage of GIN indexes; jsonb functions cannot. + + The default GIN operator class for jsonb supports queries with the key-exists operators ?, ?| Best, David signature.asc Description: Message signed with OpenPGP
Re: JSON Path and GIN Questions
On Sep 12, 2023, at 21:00, Erik Wienhold wrote: > That's also my understanding. We had a discussion about the docs on @@, @?, > and > jsonb_path_query on -general a while back [1]. Maybe it's useful also. Okay, I’ll take a pass at expanding the docs on this. I think a little mini-tutorial on these two operators would be useful. Meanwhile, I’d like to re-up this question about the index qualification of non-equality JSON Path operators. On Sep 12, 2023, at 20:16, David E. Wheeler wrote: > Issue 3: Index Use for Comparison > - > > From the docs > (https://www.postgresql.org/docs/current/datatype-json.html#JSON-INDEXING), I > had assumed any JSON Path query would be able to use the GIN index. However > while the use of the == JSON Path operator is able to take advantage of the > GIN index, apparently the >= operator cannot: > > david=# explain analyze select id from movies where movie @? '$ ?($.year >= > 2023)'; > QUERY PLAN > > - > Seq Scan on movies (cost=0.00..3741.41 rows=366 width=4) (actual > time=34.815..36.259 rows=192 loops=1) > Filter: (movie @? '$?($."year" >= 2023)'::jsonpath) > Rows Removed by Filter: 36081 > Planning Time: 1.864 ms > Execution Time: 36.338 ms > (5 rows) > > Is this expected? Originally I tried with json_path_ops, which I can > understand not working, since it stores hashes of paths, which would allow > only exact matches. But a plain old GIN index doesn’t appear to work, either. > Should it? Is there perhaps some other op class that would allow it to work? > Or would I have to create a separate BTREE index on `movie -> 'year'`? Thanks, David signature.asc Description: Message signed with OpenPGP
Re: JSON Path and GIN Questions
On Sep 15, 2023, at 20:36, Tom Lane wrote: > I think that that indicates that you're putting the info in the > wrong place. Perhaps the right answer is to insert something > more explicit in section 11.2, which is the first place where > we really spend any effort discussing what can be indexed. Fair enough. How ’bout this? --- a/doc/src/sgml/indices.sgml +++ b/doc/src/sgml/indices.sgml @@ -120,7 +120,7 @@ CREATE INDEX test1_id_index ON test1 (id); B-tree, Hash, GiST, SP-GiST, GIN, BRIN, and the extension bloom. Each index type uses a different - algorithm that is best suited to different types of queries. + algorithm that is best suited to different types of queries and operators. By default, the CREATE INDEX command creates B-tree indexes, which fit the most common situations. @@ -132,6 +132,14 @@ CREATE INDEX name ON table + + +Only operators on indexed columns are considered as index qualifications. +Functions never qualify for index usage, aside from +indexes on expressions. + + + B-Tree signature.asc Description: Message signed with OpenPGP
Re: JSON Path and GIN Questions
On Sep 15, 2023, at 23:59, Erik Rijkers wrote: > movie @? '$ ?($.year >= 2023)' > > I believe it is indeed not possible to have such a unequality-search use the > GIN index. It is another weakness of JSON that can be unexpected to those > not in the fullness of Knowledge of the manual. Yes, this too would be good > to explain in the doc where JSON indexes are explained. Is that a limitation of GIN indexes in general? Or could there be opclass improvements in the future that would enable such comparisons? Thanks, David signature.asc Description: Message signed with OpenPGP
Re: JSON Path and GIN Questions
On Sep 12, 2023, at 21:00, Erik Wienhold wrote: >> If so, I’d like to submit a patch to the docs talking about this, and >> suggesting the use of jsonb_path_query() to test paths to see if they return >> a boolean or not. > > +1 I’ve started work on this; there’s so much to learn! Here’s a new example that surprised me a bit. Using the GPS tracker example from the docs [1] loaded into a `:json` psql variable, this output of this query makes perfect sense to me: david=# select jsonb_path_query(:'json', '$.track.segments.location[*] ? (@ < 14)'); jsonb_path_query -- 13.4034 13.2635 Because `[*]` selects all the values. This, however, I did not expect: david=# select jsonb_path_query(:'json', '$.track.segments.location ? (@[*] < 14)'); jsonb_path_query -- 13.4034 13.2635 (2 rows) I had expected it to return two single-value arrays, instead: [13.4034] [13.2635] It appears that the filter expression is doing some sub-selection, too. Is that expected? Best, David [1]: https://www.postgresql.org/docs/current/functions-json.html#FUNCTIONS-SQLJSON-PATH signature.asc Description: Message signed with OpenPGP
Re: JSON Path and GIN Questions
On Sep 16, 2023, at 16:50, Erik Wienhold wrote: > "For these operators, a GIN index extracts clauses of the form > **accessors_chain = constant** out of the jsonpath pattern, and does the > index search based on the keys and values mentioned in these clauses." > > I don't know if this is a general limitation of GIN indexes or just how these > operators are implemented right now. > > [1] https://www.postgresql.org/docs/current/datatype-json.html#JSON-INDEXING The detail that jumps out at me is this one on jsonb_path_ops: “Basically, each jsonb_path_ops index item is a hash of the value and the key(s) leading to it” Because jsonb_path_ops indexes hashes, I would assume it would only support path equality. But it’s not clear to me from these docs that jsonb_ops also indexes hashes. Does it? Best, D signature.asc Description: Message signed with OpenPGP
Re: JSON Path and GIN Questions
On Sep 16, 2023, at 18:13, Erik Wienhold wrote: > Looks like the effect of lax mode which may unwrap arrays when necessary [1]. > The array unwrapping looks like the result of jsonb_array_elements(). > > It kinda works in strict mode: > > SELECT jsonb_path_query(:'json', 'strict $.track.segments[*].location ? (@[*] > < 14)'); > >jsonb_path_query > --- > [47.763, 13.4034] > [47.706, 13.2635] > (2 rows) > > But it does not remove elements from the matching arrays. Which I don't even > expect here because the path specifies the location array as the object to be > returned. The filter expression then only decides whether to return the > location array or not. Nowhere in the docs does it say that the filter > expression itself removes any elements from a matched array. Yes, this is what I expected. It means “select the location array if any of its contents is less that 14.” I don’t understand why it’s different in lax mode, though, as `@[*]` is not a structural error; it confirms to the schema, as the docs say. The flattening in this case seems weird. Ah, here’s why:, from the docs: "Besides, comparison operators automatically unwrap their operands in the lax mode, so you can compare SQL/JSON arrays out-of-the-box.” There follow some discussion of the need to specify `[*]` on segments in strict mode, but since that’s exactly what my example does (and the same for the locations array inside the filter), it doesn’t seem right to me that it would be unwrapped here. > Here's a query that filter's out individual array elements. It's quite a > mouthful (especially to preserve the order of array elements): Wow fun, and yeah, it makes sense to take things apart in SQL for this sort of thing! Best, David signature.asc Description: Message signed with OpenPGP
Re: JSON Path and GIN Questions
On Sep 17, 2023, at 12:20, Tom Lane wrote: > After thinking about it for awhile, I think we need some more > discursive explanation of what's allowed, perhaps along the lines > of the attached. (I still can't shake the feeling that this is > duplicative; but I can't find anything comparable until you get > into the weeds in Section V.) > > I put the new text at the end of section 11.1, but perhaps it > belongs a little further up in that section; it seems more > important than some of the preceding paras. I think this is useful, but also that it’s worth calling out explicitly that functions do not count as indexable operators. True by definition, of course, but I at least had assumed that since an operator is, in a sense, syntax sugar for a function call, they are in some sense the same thing. A header might be useful, something like “What Counts as an indexable expression”. Best, David signature.asc Description: Message signed with OpenPGP
to_regtype() Raises Error
The docs for `to_regtype()` say, “this function will return NULL rather than throwing an error if the name is not found.” And it’s true most of the time: david=# select to_regtype('foo'), to_regtype('clam'); to_regtype | to_regtype + [null] | [null] But not others: david=# select to_regtype('inteval second'); ERROR: syntax error at or near "second" LINE 1: select to_regtype('inteval second'); ^ CONTEXT: invalid type name "inteval second” I presume this has something to do with not catching errors from the parser? david=# select to_regtype('clam bake'); ERROR: syntax error at or near "bake" LINE 1: select to_regtype('clam bake'); ^ CONTEXT: invalid type name "clam bake" Best, David signature.asc Description: Message signed with OpenPGP
Re: to_regtype() Raises Error
On Sep 17, 2023, at 19:28, Tom Lane wrote: >> No, that is precisely the point. The result should be null instead of >> an error. > > Yeah, ideally so, but the cost/benefit of making it happen seems > pretty unattractive for now. See the soft-errors thread at [1], > particularly [2] (but searching in that thread for references to > regclassin, regtypein, and to_reg* will find additional detail). For my purposes I’m wrapping it in an exception-catching PL/pgSQL function, but it might be worth noting the condition in which it *will* raise an error on the docs. Best, David signature.asc Description: Message signed with OpenPGP
Re: [HACKERS] Look-behind regular expressions
On Jul 7, 2020, at 21:32, Andreas Karlsson wrote: > On 7/7/20 6:51 PM, Thom Brown wrote: > >> 10 years later, and I've noticed that both look-behind and negative >> look-behind have been implemented. >> Thanks to whomever did this. > > I think that it is Tom Lane that you should thank for this. > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=12c9a04008870c283931d6b3b648ee21bbc2cfda In 2015! Thank you, Tom. I’ll have to find a use for it one of these days. D signature.asc Description: Message signed with OpenPGP
Re: Q: Escapes in jsonpath Idents
On Apr 24, 2024, at 05:51, Peter Eisentraut wrote: >A is classified as follows. > >Case: > >a) A that is a is acontext variable>. > >b) A that begins with is a > . > >c) Otherwise, a is a . > > Does this help? I wasn't following all the discussion to see if there is > anything wrong with the implementation. Yes, it does, as it ties the special meaning of the dollar sign to the *beginning* of an expression. So it makes sense that this would be an error: david=# select '$.$foo'::jsonpath; ERROR: syntax error at or near "$foo" of jsonpath input LINE 1: select '$.$foo'::jsonpath; ^ But I’m less sure when a dollar sign is used in the *middle* (or end) of a json path identifier: david=# select '$.xx$foo'::jsonpath; ERROR: syntax error at or near "$foo" of jsonpath input LINE 1: select '$.xx$foo'::jsonpath; ^ Perhaps that should be valid? Best, David
Re: Q: Escapes in jsonpath Idents
On Apr 24, 2024, at 05:46, Peter Eisentraut wrote: > I have committed this patch, and backpatched it, as a bug fix, because the > existing description was wrong. To keep the patch minimal for backpatching, > I didn't do the conversion to a list. I'm not sure I like that anyway, > because it tends to draw more attention to that part over the surrounding > parts, which didn't seem appropriate in this case. But anyway, if you have > any more non-bug-fix editing in this area, which would then target PG18, > please send more patches. Makes sense, that level of detail gets into the weeks so maybe doesn’t need to be quite so prominent as a list. Thank you! David
Re: Q: Escapes in jsonpath Idents
On Apr 24, 2024, at 3:22 PM, Erik Wienhold wrote: > Thanks Peter! But what is the definition of the entire path expression? > Perhaps something like: > > ::= { "." } > > That would imply that "$.$foo" is a valid path that accesses a variable > member (but I guess the path evaluation is also specified somewhere). I read it as “if it starts with a dollar sign, it’s a variable and not a path identifier”, and I assume any `.foo` expression is a path identifier. > What bugs me about this description, after reading it a couple of times, > is that it's not clear what is meant by ."$varname". It could mean two > things: (1) the double-quoting masks $varname in order to not interpret > those characters as a variable or (2) an interpolated string that > resolves $varname and yields a dynamic member accessor. My understanding is that if it’s in double quotes it’s never anything other than a string (whether a string literal or a path identifier string literal). IOW, variables don’t interpolate inside strings. > Under case (2) I'd expect that query to return 456 (because $foo > resolves to "bar"). (Similar to how psql would resolve :'foo' to > 'bar'.) Yes, I suspect this is the correct interpretation, but agree the wording could use some massaging, especially since path identifiers cannot start with a dollar sign anyway. Perhaps: "If the key name starts with $ or does not meet the JavaScript rules for an identifier, it must be enclosed in double quotes to make it a string literal." > Variables already work in array accessors and table 8.25 says that "The > specified index can be an integer, as well as an expression returning a > single numeric value [...]". A variable is such an expression. > >=> select jsonb_path_query('[2,3,5]', '$[$i]', '{"i":1}'); > jsonb_path_query >-- > 3 >(1 row) > > So I'd expect a similar behavior for member accessors as well when > seeing ."$varname" in the same table. Oh, interesting point! Now I wonder if the standard has this inconsistency (and is aware of it). > Yes, I think so. That would be case C in the spec excerpt provided by > Peter. So it's just a key name that happens to contain (but not start > with) the dollar sign. Exactly. It also matches the doc you quote above. Something would have to change in src/backend/utils/adt/jsonpath_scan.l to fix that, but that file makes my eyes water, so I’m not gonna take a stab at it. :-) D
Re: New committers: Melanie Plageman, Richard Guo
On Apr 26, 2024, at 07:54, Jonathan S. Katz wrote: > The Core Team would like to extend our congratulations to Melanie Plageman > and Richard Guo, who have accepted invitations to become our newest > PostgreSQL committers. > > Please join us in wishing them much success and few reverts! Great news, congratulations! Best, David
jsonpath Time and Timestamp Special Cases
Hello hackers, I noticed that the jsonpath date/time functions (.time() and timestamp(), et al.) don’t support some valid but special-case PostgreSQL values, notably `infinity`, `-infinity`, and, for times, '24:00:00`: ❯ psql psql (17devel) Type "help" for help. david=# select jsonb_path_query(to_jsonb('24:00:00'::time), '$.time()'); ERROR: time format is not recognized: "24:00:00" david=# select jsonb_path_query(to_jsonb('infinity'::timestamptz), '$.timestamp_tz()'); ERROR: timestamp_tz format is not recognized: "infinity" I assume this is because the standard doesn’t support these, or references JavaScript-only values or some such. Is that right? Best, David
Proposal: Document ABI Compatibility
Hackers, At the PGConf Unconference session on improving extension support in core, we talked quite a bit about the recent anxiety among extension developers about a lack of an ABI compatibility guarantee in Postgres. Yurii Rashkovskii did a little header file spelunking and talked[1] about a few changes in minor version releases, including to apparent field order in structs. Jeremy Schneider posted the example on Twitter[2], and Peter G replied[3]: > You must be referring to my commit 714780dc. The new field is stored within > alignment padding (though only on back branches). Has this been tied to a > known problem? At the Unconference, Tom Lane said that this approach is pretty well drilled into the heads of every committer, and new ones pick it up through experience. The goal, IIUC, is to never introduce binary incompatibilities into the C APIs in minor releases. This standard would be good to document, to let extension developers know exactly what the guarantees are. I’m happy to start a doc patch to add an ABI compatibility guarantee (presumably in xfunc.sgml), but want to be sure the details are right, namely: * There are no source or binary compatibility guarantees for major releases. * The ABI is guaranteed to change only in backward compatible ways in minor releases. If for some reason it doesn’t it’s a bug that will need to be fixed. This ensures that an extension compiled against an earlier minor release will continue to work without recompilation on later minor releases of the same major version. But if I understand correctly, the use of techniques like adding a new field in padding does not mean that extensions compiled on later minor releases will work on earlier minor releases of the same major version. Unless, that is, we can provide a complete list of things not to do (like make use of padding) to avoid it. Is that feasible? Are there other details it should include? Thanks, David [1]: https://www.pgevents.ca/events/pgconfdev2024/schedule/session/14 [2]: https://x.com/jer_s/status/1785717368804815026 [3]: https://x.com/petervgeoghegan/status/1785720228237717627
Re: Proposal: Document ABI Compatibility
On Jun 3, 2024, at 14:58, Andres Freund wrote: > Hi, Hello Andres. > Are there notes for the session? Yes, but not posted yet. Here’s what Andreas 'ads' Scherbaum sent me for that bit of the conversation: * Core is focused on core ABI stability * David: No "statement of stability" in Core * David/Jeremy/Tom: coding guidelines, style guidelines *useful to have docs in core about what's stable and what's not, what you should compile against or not, and ABI guarantees * Abigale: there are hooks, but no overall concept for extensions * Tom: Peter Eisentraut is working on tests for extensions stability * Jeremy: nothing is preventing people from installing incompatible versions > It'd be nice if the slides for the talk could be uploaded... He also talked about it at Mini-Summit Five[1], for which I posted slides[2] (slides 11-14) and video[3] (starting at 29:08). > I don't think we can really make this a hard guarantee. Yes, we try hard to > avoid ABI breaks, but there IIRC have been a few cases over the years where > that wasn't practical for some reason. If we have to decide between a bad bug > and causing an ABI issue that's unlikely to affect anybody, we'll probably > choose the ABI issue. We can document that too, and perhaps a policy for letting people know. I thought I recalled something like this in the past, but Rob Treat did some spelunking through change logs and found only CVEs that needed repairs by manually running some SQL. So some sense of its rarity would also be useful. > Thus I am not really on board with this statement as-is. That’s fine, we should get it to where there’s consensus on the ordering and agreement on what, exactly, it should be. > Extensions in general can do lots of stuff, guaranteeing that bug fixes don't > cause any problems is just not feasible. > > It'd be interesting to see a few examples of actual minor-version-upgrade > extension breakages, so we can judge what caused them. In the community Slack[4], Matthias van de Meent writes[5]: > Citus’ pg_version_compat.h[7] where it re-implements a low-level function > that was newly introduced in PG14.7. If you build against PG14.7+ headers, > you may get random crashes when running on 14.6. I suppose it would work fine on 14.7 if compiled on 14.6 though. I suspect there aren’t many examples, though, mostly just a lot of anxiety, and some have decided that extensions must be recompiled for every minor release in order to avoid the issue. StackGres[7] is one example, but I suspect Omni (Yurii’s company) may follow. > I don't think it's common for such new-fields-in-padding to cause problems > when using an earlier minor PG version. For that the extension would need to > actually rely on the presence of the new field, but typically that'd not be > the case when we introduce a new field in a minor version. That’s what I was thinking, so “compatibility assured only if you don’t use padding yourself” would be super helpful. >> Unless, that is, we can provide a complete list of things not to do (like >> make use of padding) to avoid it. Is that feasible? > > You can't really rely on the contents of padding, in general. So I don't think > this is really something that needs to be called out. Sure, probably not a problem, but if that’s the sole qualifier for making binary changes, I think it’s worth saying, as opposed to “we don’t make any”. Something like “Only changes to padding, which you never used anyway, right?” :-) D [1]: https://justatheory.com/2024/05/mini-summit-five/ [2]: https://justatheory.com/shared/extension-ecosystem-summit/omni-universally-buildable-extensions.pdf [3]: https://youtu.be/R5ijx8IJyaM [4]: https://pgtreats.info/slack-invite [5]: https://postgresteam.slack.com/archives/C056ZA93H1A/p1716502630690559?thread_ts=1716500801.036709&cid=C056ZA93H1A [6] https://github.com/citusdata/citus/blob/main/src/include/pg_version_compat.h#L236-L248 [7]: https://stackgres.io/doc/latest/intro/extensions/
Re: Proposal: Document ABI Compatibility
On Jun 3, 2024, at 5:56 PM, Andres Freund wrote: > I don't see how this would trigger random crashes. > > Unfortunately [4] doesn't seem to take me to a relevant message (pruned chat > history?), so I can't infer more from that context. You can use [4] to join the Slack (if you haven’t already) and [5] for the relevant post. > Regardless of ABI issues, it's probably a good idea to continually run tests > against in-development minor versions, just to prevent something breaking from > creeping in. IIRC there were a handful of cases where we accidentally broke > some extension, because they relied on some implementation details. Oh yeah, I run regular tests against the latest minor release of all supported Postgres version for my extensions, using pgxn-tools[6], which looks like this[7]. Which I consider absolutely essential. But it doesn’t mean that something compiled against .4 will work with .3 and vice versa. That’s what we could use the guidance/guarantees on. >> Sure, probably not a problem, but if that’s the sole qualifier for making >> binary changes, I think it’s worth saying, as opposed to “we don’t make >> any”. Something like “Only changes to padding, which you never used anyway, >> right?” :-) > > IDK, to me something like this seems to promise more than we actually can. What I’d like to do is figure out exactly what we *can* promise and perhaps some guidelines, and start with that. Best, David [4]: https://pgtreats.info/slack-invite [5]: https://postgresteam.slack.com/archives/C056ZA93H1A/p1716502630690559?thread_ts=1716500801.036709&cid=C056ZA93H1A [6]: https://github.com/pgxn/docker-pgxn-tools [7]: https://github.com/pgxn/docker-pgxn-tools/actions/runs/9351752462
Patch bug: Fix jsonpath .* on Arrays
Hackers, The behavior of the .* jpiAnyKey jsonpath selector seems incorrect. ``` select jsonb_path_query('[1,2,3]', '$.*'); jsonb_path_query -- (0 rows) select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', '$.*'); jsonb_path_query -- [3, 4, 5] ``` The first example might be expected, since .* is intended for object keys, but the handing of `jpiAnyKey` has a branch for unwrapping arrays. The second example, however, just seems weird: this is .*, not .**. The attached patch fixes it by passing the next node to `executeAnyItem()` (via `executeItemUnwrapTargetArray()`) and then properly setting `jperOk` when `executeAnyItem()` finds values and there is no current (next) node. I took this approach given what appears to be the intended behavior or $* on arrays in lax mode. However, I could see an argument that .* should not apply to arrays at all. If so, I can submit a new patch removing the branch that unwraps an array with .*. Best, David 0001-Fix-behavior-of-jsonpath-.-on-arrays.patch Description: Binary data
Re: Patch bug: Fix jsonpath .* on Arrays
On Jun 4, 2024, at 12:28 PM, David G. Johnston wrote: > This seems to be working correctly. Lax mode causes the first array level to > unwrap and produce new context item values. Then the wildcard member > accessor is applied to each. Numbers don’t have members so no matches exist > in the first example. The object in the second indeed has a single member > and so matches the wildcard and its value, the array, is returned. Oh FFS, unwrapping still breaks my brain. You’re right, of course. Here’s a new patch that demonstrates that behavior, since that code path is not currently represented in tests AFAICT (I would have expected to have broken it with this patch). D 0001-Add-tests-for-jsonpath-.-on-arrays.patch Description: Binary data
Re: Patch bug: Fix jsonpath .* on Arrays
On Jun 4, 2024, at 20:45, David E. Wheeler wrote: > Here’s a new patch that demonstrates that behavior, since that code path is > not currently represented in tests AFAICT (I would have expected to have > broken it with this patch). Commitfest link: https://commitfest.postgresql.org/48/5017/ D
Re: Patch bug: Fix jsonpath .* on Arrays
On Jun 4, 2024, at 20:45, David E. Wheeler wrote: > Oh FFS, unwrapping still breaks my brain. You’re right, of course. Here’s a > new patch that demonstrates that behavior, since that code path is not > currently represented in tests AFAICT (I would have expected to have broken > it with this patch). Rebased and moved the new tests to the end of the file. D v2-0001-Add-tests-for-jsonpath-.-on-arrays.patch Description: Binary data
Re: Patch bug: Fix jsonpath .* on Arrays
On Jun 7, 2024, at 10:23, David E. Wheeler wrote: > Rebased and moved the new tests to the end of the file. Bah, sorry, that was the previous patch. Here’s v3. D v3-0001-Add-tests-for-jsonpath-.-on-arrays.patch Description: Binary data
Shouldn't jsonpath .string() Unwrap?
Hackers, Most of the jsonpath methods auto-unwrap in lax mode: david=# select jsonb_path_query('[-2,5]', '$.abs()'); jsonb_path_query -- 2 5 (2 rows) The obvious exceptions are size() and type(), which apply directly to arrays, so no need to unwrap: david=# select jsonb_path_query('[-2,5]', '$.size()'); jsonb_path_query -- 2 (1 row) david=# select jsonb_path_query('[-2,5]', '$.type()'); jsonb_path_query -- "array" But what about string()? Is there some reason it doesn’t unwrap? david=# select jsonb_path_query('[-2,5]', '$.string()'); ERROR: jsonpath item method .string() can only be applied to a bool, string, numeric, or datetime value What I expect: david=# select jsonb_path_query('[-2,5]', '$.string()'); jsonb_path_query — "2" "5" (2 rows) However, I do see a test[1] for this behavior, so maybe there’s a reason for it? Best, David [1]: https://github.com/postgres/postgres/blob/REL_17_BETA1/src/test/regress/expected/jsonb_jsonpath.out#L2527-L2530
Re: Proposal: Document ABI Compatibility
On Jun 4, 2024, at 03:18, Peter Eisentraut wrote: > This could possibly be avoided by renaming the symbol in backbranches. Maybe > something like > > #define InitResultRelInfo InitResultRelInfo2 > > Then you'd get a specific error message when loading the module, rather than > a crash. That sounds more useful, yes. Is that a practice the project would consider adopting? There’s also oracle_fdw@d137d15[1], which says: > An API break in PostgreSQL 10.4 and 9.6.9 makes it impossible > to use these versions: the "extract_actual_join_clauses" function > gained an additional parameter. The 10.4 commit is 68fab04, and it does indeed add a new function: ``` patch --- a/src/include/optimizer/restrictinfo.h +++ b/src/include/optimizer/restrictinfo.h @@ -36,6 +36,7 @@ extern List *get_actual_clauses(List *restrictinfo_list); extern List *extract_actual_clauses(List *restrictinfo_list, bool pseudoconstant); extern void extract_actual_join_clauses(List *restrictinfo_list, + Relids joinrelids, List **joinquals, List **otherquals); extern bool join_clause_is_movable_to(RestrictInfo *rinfo, RelOptInfo *baserel); ``` I wonder if that sort of change could be avoided in backpatches, maybe by adding and using a `extract_actual_join_clauses_compat` function and using that internally instead? Or, to David C’s point, perhaps it would be better to say there are some categories of APIs that are not subject to any guarantees in minor releases? Best, David [1]: https://github.com/laurenz/oracle_fdw/commit/d137d15edca8c67df1e5a01f417f4833b028 [2]: https://github.com/postgres/postgres/commit/68fab04f7c2a07c5308e3d2957198ccd7a80ebc5#diff-bb6fa74cb115e19684092f0938131cd5d99b26fa2d49480f7ea7f28e937a7fb4
Re: Proposal: Document ABI Compatibility
On Jun 10, 2024, at 15:39, Andres Freund wrote: > That's 6 years ago, not sure we can really learn that much from that. > > And it's not like it's actually impossible, #ifdefs aren't great, but they are > better than nothing. Right, it’s just that extension authors could use some notification that such a change is coming so they can update their code, if necessary. >> Or, to David C’s point, perhaps it would be better to say there are some >> categories of APIs that are not subject to any guarantees in minor releases? > > I'm honestly very dubious that this is a good point to introduce a bunch of > formalism. It's a already a lot of work to maintain them, if we make it even > harder we'll end up more fixes not being backported, because it's not worth > the pain. Well it’s a matter of distributing the work. I don’t want to increase anyone’s workload unnecessarily, but as it is stuff like this can be surprising to extension maintainers with some expectation of minor release stability who had no warning of the change. That kind of thing can dissuade some people from deciding to write or maintain extensions, and lead others to recompile and distribute binaries for every single minor release. > To be blunt, the number of examples raised here doesn't seem to indicate that > this is an area where we need to invest additional resources. We are already > severely constrained as a project by committer bandwidth, there are plenty > other things that seem more important to focus on. So my question is, what’s the least onerous thing for committers to commit to doing that we can write down to properly set expectations? That’s where I want to start: can we publish a policy that reflects what committers already adhere to? And is there some way to let people know that an incompatible change is being released? Even if it just starts out in the release notes? Based on this thread, I’ve drafted the sort of policy I have in mind. Please don’t assume I’m advocating for exactly the wording here! Let’s workshop this until it’s something the committers and core team can agree to. (At that point I’ll turn it into a doc patch) Have a look and let me know what you think. ``` md ABI Policy == The PostgreSQL core team maintains two application binary interface (ABI) guarantees: one for major releases and one for minor releases. Major Releases -- Applications that use the PostgreSQL APIs must be compiled for each major release supported by the application. The inclusion of `PG_MODULE_MAGIC` ensures that code compiled for one major version will rejected by other major versions. Furthermore, new releases may make API changes that require code changes. Use the `PG_VERSION_NUM` constant to adjust code in a backwards compatible way: ``` c #if PG_VERSION_NUM >= 16 #include "varatt.h" #endif ``` PostgreSQL avoids unnecessary API changes in major releases, but usually ships a few necessary API changes, including deprecation, renaming, and argument variation. In such cases the incompatible changes will be listed in the Release Notes. Minor Releases -- PostgreSQL makes every effort to avoid both API and ABI breaks in minor releases. In general, an application compiled against any minor release will work with any other minor release, past or future. When a change *is* required, PostgreSQL will choose the least invasive way possible, for example by squeezing a new field into padding space or appending it to the end of a struct. This sort of change should not impact dependent applications unless they use `sizeof(the struct)` or create their own instances of such structs --- patterns best avoided. In rare cases, however, even such non-invasive changes may be impractical or impossible. In such an event, the change will be documented in the Release Notes, and details on the issue will also be posted to [TBD; mail list? Blog post? News item?]. The project strongly recommends that developers adopt continuous integration testing at least for the latest minor release all major versions of Postgres they support. ``` Best, David
Re: Proposal: Document ABI Compatibility
On Jun 12, 2024, at 8:43 AM, Peter Eisentraut wrote: >> Right, it’s just that extension authors could use some notification that >> such a change is coming so they can update their code, if necessary. > > I think since around 6 years ago we have been much more vigilant about > avoiding ABI breaks. So if there aren't any more recent examples of > breakage, then maybe that was ultimately successful, and the upshot is, > continue to be vigilant at about the same level? That sounds great to me. I’d like to get it documented, though, so that extension and other third party developers are aware of it, and not just making wild guesses and scaring each other over (perhaps) misconceptions. D
Re: Proposal: Document ABI Compatibility
On Jun 12, 2024, at 8:58 AM, Jelte Fennema-Nio wrote: > While not strictly an ABI break I guess, the backport of 32d5a4974c81 > broke building Citus against 13.10 and 14.7[1]. > > [1]: https://github.com/citusdata/citus/pull/6711 Interesting one. We might want to advise projects to use deferent names if they copy code from the core, use an extension-specific prefix perhaps. That way if it gets backported by the core, as in this example, it won’t break anything, and the extension can choose to switch. D
Re: Proposal: Document ABI Compatibility
On Jun 12, 2024, at 10:47, Robert Haas wrote: > What I think would be useful to document is our usual practices e.g. > adding new struct members at the end of structs, trying to avoid > changing public function signatures. If we document promises to > extension authors, I don't know how much difference that will make: > we'll probably end up needing to violate them at some point for one > reason or another. I think that’s fine if there is some sort of notification process. The policy I drafted upthread starts with making sure the such a break is mentioned in the release notes. > But if we document what committers should do, then > we might do better than we're now, because committers will be more > likely to do it right, and extension authors can also read those > instructions to understand what our practices are. Yes, this, thank you! D
Re: Shouldn't jsonpath .string() Unwrap?
On Jun 12, 2024, at 4:02 PM, David G. Johnston wrote: > Adding Andrew. Thank you. > I'm willing to call this an open item against this feature as I don't see any > documentation explaining that string() behaves differently than the others. Maybe there’s some wording in the standard on this topic? I’m happy to provide a patch to auto-unwrap .string() in lax mode. Seems pretty straightforward. D
jsonpath: Missing Binary Execution Path?
Hackers, Another apparent inconsistency I’ve noticed in jsonpath queries is the treatment of the && and || operators: They can’t operate on scalar functions, only on other expressions. Some examples: david=# select jsonb_path_query('true', '$ && $'); ERROR: syntax error at or near "&&" of jsonpath input LINE 1: select jsonb_path_query('true', '$ && $'); ^ david=# select jsonb_path_query('true', '$.boolean() && $.boolean()'); ERROR: syntax error at or near "&&" of jsonpath input LINE 1: select jsonb_path_query('true', '$.boolean() && $.boolean()'... ^ The only place I’ve seen them work is inside filters with binary or unary operands: jsonb_path_query('[1, 3, 7]', '$[*] ? (@ > 1 && @ < 5)'); jsonb_path_query -- 3 It doesn’t even work with boolean methods! david=# select jsonb_path_query('[1, 3, 7]', '$[*] ? (@.boolean() && @.boolean())'); ERROR: syntax error at or near "&&" of jsonpath input LINE 1: select jsonb_path_query('[1, 3, 7]', '$[*] ? (@.boolean() &&... ^ Other binary operators work just fine in these sorts of contexts: david=# select jsonb_path_query('1', '$ >= 1'); jsonb_path_query -- true (1 row) david=# select jsonb_path_query('[1, 3, 7]', '$[*] ? (@ > 1)'); jsonb_path_query -- 3 7 (2 rows) Should && and || not also work on scalar operands? Best, David
Re: jsonpath: Missing Binary Execution Path?
On Jun 13, 2024, at 11:32, David E. Wheeler wrote: > Should && and || not also work on scalar operands? I see the same issue for unary !, too: david=# select jsonb_path_query('true', '!$'); ERROR: syntax error at or near "$" of jsonpath input LINE 1: select jsonb_path_query('true', '!$'); ^ david=# select jsonb_path_query('[1, 3, 7]', '$[*] ? (!true)'); ERROR: syntax error at end of jsonpath input LINE 1: select jsonb_path_query('[1, 3, 7]', '$[*] ? (!true)'); ^ david=# select jsonb_path_query('[1, 3, 7]', '$[*] ? (!@.boolean())'); ERROR: syntax error at or near "@" of jsonpath input LINE 1: select jsonb_path_query('[1, 3, 7]', '$[*] ? (!@.boolean())'... ^ Best, David
Re: jsonpath: Missing Binary Execution Path?
On Jun 13, 2024, at 3:33 PM, Andrew Dunstan wrote: > What does the spec say about these? What do other implementations do? Paging Mr. Eisentraut! :-) D
Re: Shouldn't jsonpath .string() Unwrap?
On Jun 13, 2024, at 3:53 PM, Andrew Dunstan wrote: > Hmm. You might be right. Many of these items have this code, but the string() > branch does not: > if (unwrap && JsonbType(jb) == jbvArray) >return executeItemUnwrapTargetArray(cxt, jsp, jb, found, >false); Exactly, would be pretty easy to add. I can work up a patch this weekend. D