Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Add additional jsonpath string methods
- bd4f879a9cdd 19 (unreleased) landed
Rename jsonpath method arg tokens
- a35c9d524ed0 19 (unreleased) landed
Fix transient memory leakage in jsonpath evaluation.
- 5a2043bf7131 19 (unreleased) cited
Make jsonpath .string() be immutable for datetimes.
- cb599b9ddfcc 18.0 cited

PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2024-09-25T18:17:20Z

Hello hackers,

This patch is a follow-up and generalization to [0].

It adds the following jsonpath methods:  lower, upper, initcap, l/r/btrim,
replace, split_part.

It makes jsonpath able to support expressions like these:

select jsonb_path_query('"   hElLo WorlD "',
 '$.btrim().lower().upper().lower().replace("hello","bye") starts with
"bye"');
select jsonb_path_query('"abc~@~def~@~ghi"', '$.split_part("~@~", 2)')

They, of course, forward their implementation to the internal
pg_proc-registered function.

As a first wip/poc I've picked the functions I typically need to clean up
JSON data.
I've also added a README.jsonpath with documentation on how to add a new
jsonpath method.
If I had this available when I started, it would have saved me some time.
So, I am leaving it here for the next hacker.

This patch is not particularly intrusive to existing code:
Afaict, the only struct I've touched is JsonPathParseItem , where I added {
JsonPathParseItem *arg0, *arg1; } method_args.
Up until now, most of the jsonpath methods that accept arguments rely on
left/right operands,
which works, but it could be more convenient for future more complex
methods.
I've also added the appropriate jspGetArgX(JsonPathItem *v, JsonPathItem
*a).

Open items
- What happens if the jsonpath standard adds a new method by the same name?
A.D. mentioned this in [0] with the proposal of having a prefix like pg_ or
initial-upper letter.
- Still using the default collation like the rest of the jsonpath code.
- documentation N/A yet
- I do realize that the process of adding a new method sketches an
imaginary.
CREATE JSONPATH FUNCTION. This has been on the back of my mind for some
time now,
but I can't say I have an action plan for this yet.

GitHub PR view if you prefer:
https://github.com/Florents-Tselai/postgres/pull/18

[0]
https://www.postgresql.org/message-id/flat/185BF814-9225-46DB-B1A1-6468CF2C8B63%40justatheory.com#1850a37a98198974cf543aefe225ba56

All the best,
Flo

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-25T21:03:57Z

Florents Tselai <florents.tselai@gmail.com> writes:
> This patch is a follow-up and generalization to [0].
> It adds the following jsonpath methods:  lower, upper, initcap, l/r/btrim,
> replace, split_part.

How are you going to deal with the fact that this makes jsonpath
operations not guaranteed immutable?  (See commit cb599b9dd
for some context.)  Those are all going to have behavior that's
dependent on the underlying locale.

We have the kluge of having separate "_tz" functions to support
non-immutable datetime operations, but that way doesn't seem like
it's going to scale well to multiple sources of mutability.

			regards, tom lane

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Alexander Korotkov <aekorotkov@gmail.com> — 2024-09-26T10:55:44Z

On Thu, Sep 26, 2024 at 12:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Florents Tselai <florents.tselai@gmail.com> writes:
> > This patch is a follow-up and generalization to [0].
> > It adds the following jsonpath methods:  lower, upper, initcap, l/r/btrim,
> > replace, split_part.
>
> How are you going to deal with the fact that this makes jsonpath
> operations not guaranteed immutable?  (See commit cb599b9dd
> for some context.)  Those are all going to have behavior that's
> dependent on the underlying locale.
>
> We have the kluge of having separate "_tz" functions to support
> non-immutable datetime operations, but that way doesn't seem like
> it's going to scale well to multiple sources of mutability.

While inventing "_tz" functions I was thinking about jsonpath methods
and operators defined in standard then.  Now I see huge interest on
extending that.  I wonder if we can introduce a notion of flexible
mutability?  Imagine that jsonb_path_query() function (and others) has
another function which analyzes arguments and reports mutability.  If
jsonpath argument is constant and all methods inside are safe then
jsonb_path_query() is immutable otherwise it is stable.  I was
thinking about that back working on jsonpath, but that time problem
seemed too limited for this kind of solution.  Now, it's possibly time
to shake off the dust from this idea.  What do you think?

------
Regards,
Alexander Korotkov
Supabase

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Alexander Korotkov <aekorotkov@gmail.com> — 2024-09-26T10:57:25Z

Hi, Florents!

On Wed, Sep 25, 2024 at 9:18 PM Florents Tselai
<florents.tselai@gmail.com> wrote:
> This patch is a follow-up and generalization to [0].
>
> It adds the following jsonpath methods:  lower, upper, initcap, l/r/btrim, replace, split_part.
>
> It makes jsonpath able to support expressions like these:
>
> select jsonb_path_query('"   hElLo WorlD "',  '$.btrim().lower().upper().lower().replace("hello","bye") starts with "bye"');
> select jsonb_path_query('"abc~@~def~@~ghi"', '$.split_part("~@~", 2)')

Did you check if these new methods now in SQL standard or project of
SQL standard?

------
Regards,
Alexander Korotkov
Supabase

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2024-09-26T12:59:51Z

On Thu, Sep 26, 2024 at 1:55 PM Alexander Korotkov <aekorotkov@gmail.com>
wrote:

> On Thu, Sep 26, 2024 at 12:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Florents Tselai <florents.tselai@gmail.com> writes:
> > > This patch is a follow-up and generalization to [0].
> > > It adds the following jsonpath methods:  lower, upper, initcap,
> l/r/btrim,
> > > replace, split_part.
> >
> > How are you going to deal with the fact that this makes jsonpath
> > operations not guaranteed immutable?  (See commit cb599b9dd
> > for some context.)  Those are all going to have behavior that's
> > dependent on the underlying locale.
> >
> > We have the kluge of having separate "_tz" functions to support
> > non-immutable datetime operations, but that way doesn't seem like
> > it's going to scale well to multiple sources of mutability.
>
> While inventing "_tz" functions I was thinking about jsonpath methods
> and operators defined in standard then.  Now I see huge interest on
> extending that.  I wonder if we can introduce a notion of flexible
> mutability?  Imagine that jsonb_path_query() function (and others) has
> another function which analyzes arguments and reports mutability.  If
> jsonpath argument is constant and all methods inside are safe then
> jsonb_path_query() is immutable otherwise it is stable.  I was
> thinking about that back working on jsonpath, but that time problem
> seemed too limited for this kind of solution.  Now, it's possibly time
> to shake off the dust from this idea.  What do you think?
>
> ------
> Regards,
> Alexander Korotkov
> Supabase
>

In case you're having a deja vu, while researching this
I did come across [0] where disussing this back in 2019.

In this patch I've conveniently left jspIsMutable and jspIsMutableWalker
untouched and under the rug,
but for the few seconds I pondered over this,the best answer I came with
was
a simple heuristic to what Alexander says above:
if all elements are safe, then the whole jsp is immutable.

If we really want to tackle this and make jsonpath richer though,
I don't think we can avoid being a little more flexible/explicit wrt
mutability.

Speaking of extensible: the jsonpath standard does mention function
extensions [1] ,
so it looks like we're covered by the standard, and the mutability aspect
is an implementation detail. No?
And having said that, the whole jsonb/jsonpath parser/executor
infrastructure is extremely powerful
and kinda under-utilized if we use it "only" for jsonpath.
Tbh, I can see it supporting more specific DSLs and even offering hooks for
extensions.
And I know for certain I'm not the only one thinking about this.
See [2] for example where they've lifted, shifted and renamed the
jsonb/jsonpath infra to build a separate language for graphs

[0]
https://www.postgresql.org/message-id/CAPpHfdvDci4iqNF9fhRkTqhe-5_8HmzeLt56drH+_Rv2rNRqfg@mail.gmail.com
[1] https://www.rfc-editor.org/rfc/rfc9535.html#name-function-extensions
[2] https://github.com/apache/age/blob/master/src/include/utils/agtype.h

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2024-09-27T09:45:23Z

On Sep 26, 2024, at 13:59, Florents Tselai <florents.tselai@gmail.com> wrote:

> Speaking of extensible: the jsonpath standard does mention function extensions [1] ,
> so it looks like we're covered by the standard, and the mutability aspect is an implementation detail. No?

That’s not the standard used for Postgres jsonpath. Postgres follows the SQL/JSON standard in the SQL standard, which is not publicly available, but a few people on the list have copies they’ve purchased and so could provide some context.

In a previous post I wondered if the SQL standard had some facility for function extensions, but I suspect not. Maybe in the next iteration?

> And having said that, the whole jsonb/jsonpath parser/executor infrastructure is extremely powerful
> and kinda under-utilized if we use it "only" for jsonpath.
> Tbh, I can see it supporting more specific DSLs and even offering hooks for extensions.
> And I know for certain I'm not the only one thinking about this.
> See [2] for example where they've lifted, shifted and renamed the jsonb/jsonpath infra to build a separate language for graphs

I’m all for extensibility, though jsonpath does need to continue to comply with the SQL standard. Do you have some idea of the sorts of hooks that would allow extension authors to use some of that underlying capability?

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2024-09-27T10:28:21Z


> On 27 Sep 2024, at 12:45 PM, David E. Wheeler <david@justatheory.com> wrote:
> 
> On Sep 26, 2024, at 13:59, Florents Tselai <florents.tselai@gmail.com> wrote:
> 
>> Speaking of extensible: the jsonpath standard does mention function extensions [1] ,
>> so it looks like we're covered by the standard, and the mutability aspect is an implementation detail. No?
> 
> That’s not the standard used for Postgres jsonpath. Postgres follows the SQL/JSON standard in the SQL standard, which is not publicly available, but a few people on the list have copies they’ve purchased and so could provide some context.
> 
> In a previous post I wondered if the SQL standard had some facility for function extensions, but I suspect not. Maybe in the next iteration?
> 
>> And having said that, the whole jsonb/jsonpath parser/executor infrastructure is extremely powerful
>> and kinda under-utilized if we use it "only" for jsonpath.
>> Tbh, I can see it supporting more specific DSLs and even offering hooks for extensions.
>> And I know for certain I'm not the only one thinking about this.
>> See [2] for example where they've lifted, shifted and renamed the jsonb/jsonpath infra to build a separate language for graphs
> 
> I’m all for extensibility, though jsonpath does need to continue to comply with the SQL standard. Do you have some idea of the sorts of hooks that would allow extension authors to use some of that underlying capability?

Re-tracing what I had to do

1. Define a new JsonPathItemType jpiMyExtType and map it to a JsonPathKeyword
2. Add a new JsonPathKeyword and make the lexer and parser aware of that,
3. Tell the main executor executeItemOptUnwrapTarget what to do when the new type is matched.

I think 1, 2 are the trickiest because they require hooks to  jsonpath_scan.l and parser jsonpath_gram.y 

3. is the meat of a potential hook, which would be something like 
extern JsonPathExecResult executeOnMyJsonpathItem(JsonPathExecContext *cxt, JsonbValue *jb, JsonValueList *found);
This should be called by the main executor executeItemOptUnwrapTarget when it encounters case jpiMyExtType

It looks like quite an endeavor, to be honest.

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2025-03-05T19:29:50Z

On Thu, Sep 26, 2024 at 1:55 PM Alexander Korotkov <aekorotkov@gmail.com>
wrote:

> On Thu, Sep 26, 2024 at 12:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Florents Tselai <florents.tselai@gmail.com> writes:
> > > This patch is a follow-up and generalization to [0].
> > > It adds the following jsonpath methods:  lower, upper, initcap,
> l/r/btrim,
> > > replace, split_part.
> >
> > How are you going to deal with the fact that this makes jsonpath
> > operations not guaranteed immutable?  (See commit cb599b9dd
> > for some context.)  Those are all going to have behavior that's
> > dependent on the underlying locale.
> >
> > We have the kluge of having separate "_tz" functions to support
> > non-immutable datetime operations, but that way doesn't seem like
> > it's going to scale well to multiple sources of mutability.
>
> While inventing "_tz" functions I was thinking about jsonpath methods
> and operators defined in standard then.  Now I see huge interest on
> extending that.  I wonder if we can introduce a notion of flexible
> mutability?  Imagine that jsonb_path_query() function (and others) has
> another function which analyzes arguments and reports mutability.  If
> jsonpath argument is constant and all methods inside are safe then
> jsonb_path_query() is immutable otherwise it is stable.  I was
> thinking about that back working on jsonpath, but that time problem
> seemed too limited for this kind of solution.  Now, it's possibly time
> to shake off the dust from this idea.  What do you think?
>

I was thinking about taking another stab at this.
Would someone more versed in the inner workings of jsonpath like to weigh
in on the immutability wrt locale?

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Robert Haas <robertmhaas@gmail.com> — 2025-05-09T19:50:27Z

On Wed, Mar 5, 2025 at 2:30 PM Florents Tselai
<florents.tselai@gmail.com> wrote:
> I was thinking about taking another stab at this.
> Would someone more versed in the inner workings of jsonpath like to weigh in on the immutability wrt locale?

I'm not sure the issues with immutability here are particularly
related to jsonpath -- I think they may just be general problems with
our framework for immutability.

I always struggle a bit to remember our policy on these issues -- to
the best of my knowledge, we haven't documented it anywhere, and I
think we probably should. I believe the way it works is that whenever
a function depends on the operating system's timestamp or locale
definitions, we decide it has to be stable, not immutable. We don't
expect those things to be updated very often, but we know sometimes
they do get updated.

Now apparently what we've done for time zones is we have both
json_path_exists and json_path_exists_tz, and the former only supports
things that are truly immutable while the latter additionally supports
things that depend on time zone, and are thus marked stable.

I suppose we could just add support for these locale-dependent
operations to the "tz" version and have them error out in the non-tz
version. After all, the effect of depending on time zone is, as far as
I know, the same as the effect of depending on locale: the function
can't be immutable any more. The only real problem with that idea, at
least to my knowledge, is that the function naming makes you think
that it's just about time zones and not about anything else. Maybe
that's a wart we can live with?

Tom writes earlier in the thread that:

# We have the kluge of having separate "_tz" functions to support
# non-immutable datetime operations, but that way doesn't seem like
# it's going to scale well to multiple sources of mutability.

But I'm not sure I understand why it matters that there are multiple
sources of mutability here. Maybe I'm missing a piece of the puzzle
here.

--
Robert Haas
EDB: http://www.enterprisedb.com

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-05-13T18:07:10Z

On May 9, 2025, at 15:50, Robert Haas <robertmhaas@gmail.com> wrote:

> # We have the kluge of having separate "_tz" functions to support
> # non-immutable datetime operations, but that way doesn't seem like
> # it's going to scale well to multiple sources of mutability.
> 
> But I'm not sure I understand why it matters that there are multiple
> sources of mutability here. Maybe I'm missing a piece of the puzzle
> here.

I read that to mean “we’re not going to add another json_path_exists_* function for every potentially immutable JSONPath function. But I take your point that it could be generalized for *any* mutable function. In which case maybe it should be renamed?

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2025-05-13T20:24:11Z

> On 13 May 2025, at 2:07 PM, David E. Wheeler <david@justatheory.com> wrote:
> 
> On May 9, 2025, at 15:50, Robert Haas <robertmhaas@gmail.com> wrote:
> 
>> # We have the kluge of having separate "_tz" functions to support
>> # non-immutable datetime operations, but that way doesn't seem like
>> # it's going to scale well to multiple sources of mutability.
>> 
>> But I'm not sure I understand why it matters that there are multiple
>> sources of mutability here. Maybe I'm missing a piece of the puzzle
>> here.
> 
> I read that to mean “we’re not going to add another json_path_exists_* function for every potentially immutable JSONPath function. But I take your point that it could be generalized for *any* mutable function. In which case maybe it should be renamed?
> 
> Best,
> 
> David
> 

We discussed this a bit during the APFS:

As Robert said—and I agree—renaming the existing _tz family would be more trouble than it’s worth, given the need for deprecations, migration paths, etc. If we were designing this today, suffixes like _stable or _volatile might have been more appropriate, but at this point, we’re better off staying consistent with the _tz family.

So the path forward seems to be:

- Put these new functions under the jsonb_path_*_tz family.

- Raise an error if they’re used in the non-_tz versions.

- Document this behavior clearly.

I’ll make sure to follow the patterns in the existing _tz functions closely.

Other thoughts and head’s up are, of course, welcome.

Patch CF entry: https://commitfest.postgresql.org/patch/5270/

Last updated Sept 24, so it will also need a rebase to account for changes in jsonpath_scan.l. I’ll get to that shortly.

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-05-14T03:00:34Z

On May 13, 2025, at 16:24, Florents Tselai <florents.tselai@gmail.com> wrote:

> As Robert said—and I agree—renaming the existing _tz family would be more trouble than it’s worth, given the need for deprecations, migration paths, etc. If we were designing this today, suffixes like _stable or _volatile might have been more appropriate, but at this point, we’re better off staying consistent with the _tz family.

I get the pragmatism, and don’t want to over-bike-shed, but what a wart to live with. [I just went back and re-read Robert’s post, and didn’t realize he used exactly the same expression!] Would it really be too effortful to create _stable or _volatile functions and leave the _tz functions as a sort of legacy?

Or maybe there’s a nice backronym we could come up with for _tz. 

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2025-05-14T15:33:18Z


> On 13 May 2025, at 11:00 PM, David E. Wheeler <david@justatheory.com> wrote:
> 
> On May 13, 2025, at 16:24, Florents Tselai <florents.tselai@gmail.com> wrote:
> 
>> As Robert said—and I agree—renaming the existing _tz family would be more trouble than it’s worth, given the need for deprecations, migration paths, etc. If we were designing this today, suffixes like _stable or _volatile might have been more appropriate, but at this point, we’re better off staying consistent with the _tz family.
> 
> I get the pragmatism, and don’t want to over-bike-shed, but what a wart to live with. [I just went back and re-read Robert’s post, and didn’t realize he used exactly the same expression!] Would it really be too effortful to create _stable or _volatile functions and leave the _tz functions as a sort of legacy?

Thinking about it a second time, you may be right. 
Especially if more people are interested in adding even more methods there.

Here’s a patch just merging the latest changes in the jsonpath tooling; 
no substantial changes to v1; mainly for CFbot to pick this up.

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Robert Haas <robertmhaas@gmail.com> — 2025-05-21T18:06:17Z

On Tue, May 13, 2025 at 11:00 PM David E. Wheeler <david@justatheory.com> wrote:
> On May 13, 2025, at 16:24, Florents Tselai <florents.tselai@gmail.com> wrote:
> > As Robert said—and I agree—renaming the existing _tz family would be more trouble than it’s worth, given the need for deprecations, migration paths, etc. If we were designing this today, suffixes like _stable or _volatile might have been more appropriate, but at this point, we’re better off staying consistent with the _tz family.
>
> I get the pragmatism, and don’t want to over-bike-shed, but what a wart to live with. [I just went back and re-read Robert’s post, and didn’t realize he used exactly the same expression!] Would it really be too effortful to create _stable or _volatile functions and leave the _tz functions as a sort of legacy?

No, that wouldn't be too much work, but the issue is that people will
keep using the _tz versions and when we eventually try to remove them
those people will complain no matter how prominent we make the
deprecation notice. If we want to go this route, maybe we should do
something like:

1. Add the new versions with a _s suffix or whatever.

2. Invent a GUC jsonb_tz_warning = { on | off } that advises you to
use the new functions instead, whenever you use the old ones.

3. After N years, flip the default value from off to on.

4. After M additional years, remove the old functions and the GUC.

5. Still get complaints.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-05-21T18:13:28Z

On May 21, 2025, at 14:06, Robert Haas <robertmhaas@gmail.com> wrote:

> No, that wouldn't be too much work, but the issue is that people will
> keep using the _tz versions and when we eventually try to remove them
> those people will complain no matter how prominent we make the
> deprecation notice. If we want to go this route, maybe we should do
> something like:
> 
> 1. Add the new versions with a _s suffix or whatever.
> 
> 2. Invent a GUC jsonb_tz_warning = { on | off } that advises you to
> use the new functions instead, whenever you use the old ones.
> 
> 3. After N years, flip the default value from off to on.
> 
> 4. After M additional years, remove the old functions and the GUC.
> 
> 5. Still get complaints.

Complainers gonna complain. 🫠

Any idea how widespread the use of the function is? It was added in 17, and I’ve met few who have really dug into the jonpath stuff yet, let alone needed the time zone conversion functionality.

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-21T18:31:27Z

"David E. Wheeler" <david@justatheory.com> writes:
> On May 21, 2025, at 14:06, Robert Haas <robertmhaas@gmail.com> wrote:
>> If we want to go this route, maybe we should do
>> something like:
>> ...
>> 5. Still get complaints.

> Complainers gonna complain.

Yeah.  I do not see the point of that amount of effort.

> Any idea how widespread the use of the function is? It was added in 17, and I’ve met few who have really dug into the jonpath stuff yet, let alone needed the time zone conversion functionality.

That's a good point.  We should also remember that if somebody really
really doesn't want to fix their app, they can trivially create a
wrapper function with the old name.

Having said that, what's wrong with inventing some improved function
names and never removing the old ones?

			regards, tom lane

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Robert Haas <robertmhaas@gmail.com> — 2025-05-22T14:05:45Z

On Wed, May 21, 2025 at 2:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Having said that, what's wrong with inventing some improved function
> names and never removing the old ones?

I don't particularly like the clutter, but if the consensus is that
the clutter doesn't matter, fair enough.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2025-05-22T16:38:02Z

> On 22 May 2025, at 5:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> On Wed, May 21, 2025 at 2:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Having said that, what's wrong with inventing some improved function
>> names and never removing the old ones?
> 
> I don't particularly like the clutter, but if the consensus is that
> the clutter doesn't matter, fair enough.
> 

It depends really on how much future work we expect in adding more methods in jsonpath.
I think there’s a lot of potential there, but that’s a guess really.

On David’s point about popularity: 
In my experience timestamp related stuff from jsonb documents end up in a generated column,
and are indexed & queried there.
I expect that to continue in PG18 onwards as we’ll have virtual gen columns too.

Just to be clear, though, adding another version of these functions means
we’ll have an additional (now third) set of the same 5 functions:

The vanilla versions are considered stable and the suffixed *_tz or *_volatile (?) 

jsonb_path_exists
jsonb_path_query
jsonb_path_query_array
jsonb_path_query_first
jsonb_path_match

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-05-22T18:08:02Z

On May 22, 2025, at 12:38, Florents Tselai <florents.tselai@gmail.com> wrote:

> In my experience timestamp related stuff from jsonb documents end up in a generated column,
> and are indexed & queried there.

Have you seen this in the wild using the _tz functions? I wouldn’t think they were indexable, given the volatility.

D

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Peter Eisentraut <peter@eisentraut.org> — 2025-05-22T20:56:25Z

On 09.05.25 21:50, Robert Haas wrote:
> I always struggle a bit to remember our policy on these issues -- to
> the best of my knowledge, we haven't documented it anywhere, and I
> think we probably should. I believe the way it works is that whenever
> a function depends on the operating system's timestamp or locale
> definitions, we decide it has to be stable, not immutable. We don't
> expect those things to be updated very often, but we know sometimes
> they do get updated.

I don't understand how this discussion got to the conclusion that 
functions that depend on the locale cannot be immutable.  Note that the 
top-level functions lower, upper, and initcap themselves are immutable.

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Robert Haas <robertmhaas@gmail.com> — 2025-05-23T13:56:11Z

On Thu, May 22, 2025 at 4:56 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> I don't understand how this discussion got to the conclusion that
> functions that depend on the locale cannot be immutable.  Note that the
> top-level functions lower, upper, and initcap themselves are immutable.

Oh, well that was what Tom said last September and I just assumed he
was right about the policy. If not, well then that's different.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2025-05-23T17:06:25Z


> On 22 May 2025, at 11:56 PM, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> On 09.05.25 21:50, Robert Haas wrote:
>> I always struggle a bit to remember our policy on these issues -- to
>> the best of my knowledge, we haven't documented it anywhere, and I
>> think we probably should. I believe the way it works is that whenever
>> a function depends on the operating system's timestamp or locale
>> definitions, we decide it has to be stable, not immutable. We don't
>> expect those things to be updated very often, but we know sometimes
>> they do get updated.
> 
> I don't understand how this discussion got to the conclusion that functions that depend on the locale cannot be immutable.  Note that the top-level functions lower, upper, and initcap themselves are immutable.

I assume you mean that they’re set at initdb time, so there’s no mutability concern?

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-23T17:52:01Z

Florents Tselai <florents.tselai@gmail.com> writes:
> On 22 May 2025, at 11:56 PM, Peter Eisentraut <peter@eisentraut.org> wrote:
>> I don't understand how this discussion got to the conclusion that functions that depend on the locale cannot be immutable.  Note that the top-level functions lower, upper, and initcap themselves are immutable.

> I assume you mean that they’re set at initdb time, so there’s no mutability concern? 

Yeah, I think Peter's right and I'm wrong.  Obviously this ties into
our philosophical debate about how immutable is immutable.  But as
long as the functions only depend on locale settings that are fixed
at database creation, I think it's okay to consider them immutable.

If you were, say, depending on LC_NUMERIC, it would clearly be unsafe
to consider that immutable, so I'm not quite sure if this is the end
of the discussion.  But for what's mentioned in the thread title,
I think we only care about LC_CTYPE.

			regards, tom lane

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-05-24T16:08:18Z

On May 23, 2025, at 13:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:

>> I assume you mean that they’re set at initdb time, so there’s no mutability concern? 
> 
> Yeah, I think Peter's right and I'm wrong.  Obviously this ties into
> our philosophical debate about how immutable is immutable.  But as
> long as the functions only depend on locale settings that are fixed
> at database creation, I think it's okay to consider them immutable.
> 
> If you were, say, depending on LC_NUMERIC, it would clearly be unsafe
> to consider that immutable, so I'm not quite sure if this is the end
> of the discussion.  But for what's mentioned in the thread title,
> I think we only care about LC_CTYPE.

Oh, so maybe all this is moot, and Florents can go ahead and add support for the functions to the non-_tz functions?

Should there be some sort of inventory of what functions can be used in what contexts?

D

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2025-05-24T16:51:32Z


> On 24 May 2025, at 7:08 PM, David E. Wheeler <david@justatheory.com> wrote:
> 
> On May 23, 2025, at 13:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
>>> I assume you mean that they’re set at initdb time, so there’s no mutability concern?
>> 
>> Yeah, I think Peter's right and I'm wrong.  Obviously this ties into
>> our philosophical debate about how immutable is immutable.  But as
>> long as the functions only depend on locale settings that are fixed
>> at database creation, I think it's okay to consider them immutable.
>> 
>> If you were, say, depending on LC_NUMERIC, it would clearly be unsafe
>> to consider that immutable, so I'm not quite sure if this is the end
>> of the discussion.  But for what's mentioned in the thread title,
>> I think we only care about LC_CTYPE.
> 
> Oh, so maybe all this is moot, and Florents can go ahead and add support for the functions to the non-_tz functions?
> 


I think the patch is still in reasonably good shape and hasn’t changed much since September 24.
So yes, I’d hope there are still some valid points to consider or improve.
Otherwise, I’ll have only myself to blame for not pushing harder before the feature freeze. 😅

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-05-24T21:39:01Z

On May 24, 2025, at 12:51, Florents Tselai <florents.tselai@gmail.com> wrote:

> I think the patch is still in reasonably good shape and hasn’t changed much since September 24.So yes, I’d hope there are still some valid points to consider or improve.

Okay, here’s a review.

Patch applies cleanly.

All tests pass.

I'm curious why you added the `arg0` and `arg1` fields to the `method_args` union. Is there some reason that the existing `left` and `right` fields wouldn't work? Admittedly these are not formally binary operators, but I don't see that it matters much.

The existing string() method operates on a "JSON boolean, number, string, or datetime"; should these functions also operate on all those data types?

The argument to the trim methods appears to be ignored:

```
postgres=# select jsonb_path_query('"zzzytest"', '$.ltrim("xyz")');
 jsonb_path_query 
------------------
 "zzzytest"
```

I'm wondering if the issue is the use of the opt_datetime_template in the grammar?

```
	| '.' STR_LTRIM_P '(' opt_datetime_template ')'
		{ $$ = makeItemUnary(jpiStrLtrimFunc, $4); }
	| '.' STR_RTRIM_P '(' opt_datetime_template ')'
		{ $$ = makeItemUnary(jpiStrRtrimFunc, $4); }
	| '.' STR_BTRIM_P '(' opt_datetime_template ')'
		{ $$ = makeItemUnary(jpiStrBtrimFunc, $4); }
```

I realize it resolves to a string, but for some reason it doesn't get picked up. But also, do you want to support variables for either of these arguments? If so, maybe rename and use starts_with_initial:

```
starts_with_initial:
	STRING_P						{ $$ = makeItemString(&$1); }
	| VARIABLE_P					{ $$ = makeItemVariable(&$1); }
	;
```

split_part() does not support a negative n value:

```
postgres=# select split_part('abc,def,ghi,jkl', ',', -2) ;
 split_part 
------------
 ghi

select jsonb_path_query('"abc,def,ghi,jkl"', '$.split_part(",", -2)');
ERROR:  syntax error at or near "-" of jsonpath input
LINE 1: select jsonb_path_query('"abc,def,ghi,jkl"', '$.split_part("...
```

Nor does a `+` work. I think you’d be better served using `csv_elem`, something like:


```
    | '.' STR_SPLIT_PART_P '(' STRING_P csv_elem ‘)’
```

I'm not sure how well these functions comply with the SQL spec. Does it have a provision for implementation-specific methods? I *think* all existing methods are defined by the spec, but someone with access to its contents would have to say for sure. And maybe we don't care, consider this a natural extension?

I’ve attached a new patch with docs.

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-05-24T21:46:49Z

On May 24, 2025, at 17:39, David E. Wheeler <david@justatheory.com> wrote:

> I’ve attached a new patch with docs.

Oh, and I forgot to mention, src/backend/utils/adt/jsonpath_scan.l looks like it has a ton of duplication in it. Shouldn’t it just add the new keywords, something like:

```
@@ -415,6 +415,11 @@ static const JsonPathKeyword keywords[] = {
    {4, false, WITH_P, "with"},
    {5, true, FALSE_P, "false"},
    {5, false, FLOOR_P, "floor"},
+   {5, false, STR_BTRIM_P, "btrim"},
+   {5, false, STR_LOWER_P, "lower"},
+   {5, false, STR_LTRIM_P, "ltrim"},
+   {5, false, STR_RTRIM_P, "rtrim"},
+   {5, false, STR_UPPER_P, "upper"},
    {6, false, BIGINT_P, "bigint"},
    {6, false, DOUBLE_P, "double"},
    {6, false, EXISTS_P, "exists"},
@@ -428,10 +433,13 @@ static const JsonPathKeyword keywords[] = {
    {7, false, INTEGER_P, "integer"},
    {7, false, TIME_TZ_P, "time_tz"},
    {7, false, UNKNOWN_P, "unknown"},
+   {7, false, STR_INITCAP_P, "initcap"},
+   {7, false, STR_REPLACEFUNC_P, "replace"},
    {8, false, DATETIME_P, "datetime"},
    {8, false, KEYVALUE_P, "keyvalue"},
    {9, false, TIMESTAMP_P, "timestamp"},
    {10, false, LIKE_REGEX_P, "like_regex"},
+   {10, false, STR_SPLIT_PART_P, "split_part"},
    {12, false, TIMESTAMP_TZ_P, "timestamp_tz"},
```

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-05-24T21:55:36Z

On May 24, 2025, at 17:46, David E. Wheeler <david@justatheory.com> wrote:

> Oh, and I forgot to mention, src/backend/utils/adt/jsonpath_scan.l looks like it has a ton of duplication in it. Shouldn’t it just add the new keywords, something like:

And now I see my patch broke the grammar because I left some of my fiddling in there. Apologies. Here’s an updated patch with the updated keyword map, too.

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-05-24T22:01:21Z

On May 24, 2025, at 17:55, David E. Wheeler <david@justatheory.com> wrote:

> And now I see my patch broke the grammar because I left some of my fiddling in there. Apologies. Here’s an updated patch with the updated keyword map, too.

No, really :sigh:

D

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2025-05-25T04:16:57Z

> On 25 May 2025, at 1:01 AM, David E. Wheeler <david@justatheory.com> wrote:
> 
> On May 24, 2025, at 17:55, David E. Wheeler <david@justatheory.com> wrote:
> 
>> And now I see my patch broke the grammar because I left some of my fiddling in there. Apologies. Here’s an updated patch with the updated keyword map, too.
> 
> No, really :sigh:
> 
> D
> 
> <v5-0001-Add-additional-jsonpath-string-methods.patch>

The most important problem in jsonpath_scan.l now is the fact that I broke the alphabetical ordering of keywords in v2 ,
and you followed that too.

> I'm curious why you added the `arg0` and `arg1` fields to the `method_args` union. Is there some reason that the existing `left` and `right` fields wouldn't work? 

The left-right ended-up being more of a brain teaser to work with in jsonpath_exec.
Until before these methods, the opt_datetime_template was the only argument passed in existing jsonpath functions, 
So initially I used that as a template to add to the scann-parser infra,
but then realized it may make morese sense to have a way to access indexed-args.
IIRC, with an eye in the future I found it much more convenient - less of the  to work with indexed-args.
I should have gone back and use them for *TRIM_P too
But you may be onto something with the split_part thing.

> The existing string() method operates on a "JSON boolean, number, string, or datetime"; should these functions also operate on all those data types?

You mean implicitely conversion to string first?
I don’t think so: I’d expect to work like ‘$…string().replace()…'

> I'm not sure how well these functions comply with the SQL spec.

The fact that Peter hasn’t raized this as an issue, makes me think it's not one

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-05-26T22:00:34Z

On May 25, 2025, at 00:16, Florents Tselai <florents.tselai@gmail.com> wrote:

> The most important problem in jsonpath_scan.l now is the fact that I broke the alphabetical ordering of keywords in v2 ,
> and you followed that too.

Oh. They have been organized by length; I didn’t notice they were also alphabetical.

> But you may be onto something with the split_part thing.

Yes, I think it would be best if the grammar was a bit stricter --- and therefore more self-explanatory --- by making the args closer to what the functions actually expect.

>> The existing string() method operates on a "JSON boolean, number, string, or datetime"; should these functions also operate on all those data types?
> 
> You mean implicitely conversion to string first?
> I don’t think so: I’d expect to work like ‘$…string().replace()…'

Yes. Each of the existing methods has well-defined rules for what types of values they operate on, and many accept multiple types. Looking again, though, it appears that all the date/time methods operate only on strings, so I think you’re correct to follow that precedent and people can use `.string()` if they need it. We can also loosen it up later if use cases demand it.

>> I'm not sure how well these functions comply with the SQL spec.
> 
> The fact that Peter hasn’t raized this as an issue, makes me think it's not one

Fair enough.

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-06-03T19:02:07Z

Hackers,

On May 26, 2025, at 18:00, David E. Wheeler <david@justatheory.com> wrote:

> Yes, I think it would be best if the grammar was a bit stricter --- and therefore more self-explanatory --- by making the args closer to what the functions actually expect.

I chatted with Florents and went ahead and simplified the grammar and fixed the other issues I identified in my original review. Note that there are two commits, now:

`v6-0001-Rename-jsonpath-method-arg-tokens.patch` Renames some of the symbols in the jsonpath grammar so that they’re less generic (`csv*`) and more specific to their contents. This is with the expectation that they will be used by other methods in the next patch and in the future. I thought it best to separate this refactoring from the feature patch.

`v6-0002-Add-additional-jsonpath-string-methods.patch` is that feature patch. The grammar now parses the exact number and types of each method argument, eliminating the need for explicit error checking. It also uses the existing patterns for handling methods with two parameters, removing a bunch of duplicate code.

Overall I think this is ready for committer review, although now that I’m not just reviewing but hacking on this thing, maybe someone else should review it first.

Patches attached, GitHub PR here:

  https://github.com/theory/postgres/pull/12

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-06-03T19:10:28Z

On Jun 3, 2025, at 15:02, David E. Wheeler <david@justatheory.com> wrote:

> Patches attached, GitHub PR here:
> 
>  https://github.com/theory/postgres/pull/12

Found a little more unnecessary code to remove. Updated patches attached.

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-06-04T15:27:18Z

On Jun 3, 2025, at 15:10, David E. Wheeler <david@justatheory.com> wrote:

>> https://github.com/theory/postgres/pull/12
> 
> Found a little more unnecessary code to remove. Updated patches attached.

And these should fix the CI failure. I also ran pgindent.

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-06-14T15:08:07Z

On Jun 4, 2025, at 11:27, David E. Wheeler <david@justatheory.com> wrote:

> And these should fix the CI failure. I also ran pgindent.

Here’s a quick rebase. I think it’s ready for committer review, but since I’ve poked at it quite a bit myself, I updated the Commitfest item [1] to “Needs Review”.

Best,

David

[1]: https://commitfest.postgresql.org/patch/5270/

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2025-07-10T17:41:11Z


> On 14 Jun 2025, at 6:08 PM, David E. Wheeler <david@justatheory.com> wrote:
> 
> On Jun 4, 2025, at 11:27, David E. Wheeler <david@justatheory.com> wrote:
> 
>> And these should fix the CI failure. I also ran pgindent.
> 
> Here’s a quick rebase. I think it’s ready for committer review, but since I’ve poked at it quite a bit myself, I updated the Commitfest item [1] to “Needs Review”.
> 
> Best,
> 
> David
> 
> [1]: https://commitfest.postgresql.org/patch/5270/
> 
> <v9-0001-Rename-jsonpath-method-arg-tokens.patch><v9-0002-Add-additional-jsonpath-string-methods.patch>
> 

The basic problem I see with these latest revisions/refactorings is that they fail for pg_upgrade afaict.
Probably this means that some of the rearrangements on the parser/scanner are not that flexible.

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-07-10T18:13:13Z

On Jul 10, 2025, at 13:41, Florents Tselai <florents.tselai@gmail.com> wrote:

> The basic problem I see with these latest revisions/refactorings is that they fail for pg_upgrade afaict.
> Probably this means that some of the rearrangements on the parser/scanner are not that flexible.

Oh, is that what’s happening? What needs to happen to properly support pg_upgrade?

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-07-10T22:40:43Z

On Jul 10, 2025, at 14:13, David E. Wheeler <david@justatheory.com> wrote:

> Oh, is that what’s happening? What needs to happen to properly support pg_upgrade?

Turns out there was an assertion failure that David Johnson spotted in the core dump of the test output and then in the regress log. Turns out I wasn’t using `--enable-assert` in my testing. With that I was able to replicate it and find the core dump in the “Crash Reports” tab of the macOS Console.app with this line:

{"imageOffset":8079100,"sourceLine":1265,"sourceFile":"jsonpath.c","symbol":"jspGetLeftArg","imageIndex":0,"symbolLocation":348},

When I switched to using jspGetLeftArg and jspGetRightArg in the last patch, I forgot to add the assertions you originally had in your patch, Florents. Resolved in the attached, which now passes `make check-world` for me.

Also available as a pull request[1].

Best,

David

[1] https://github.com/theory/postgres/pull/12/files

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-07-10T23:23:57Z

On Jul 10, 2025, at 18:40, David E. Wheeler <david@justatheory.com> wrote:

> Resolved in the attached, which now passes `make check-world` for me.
> 
> Also available as a pull request[1].

Now with the `ISO C90 forbids mixed declarations and code` warning cleared up.

Weird that there’s a failure on Bookworm with Meson [1] (pg_regress diffs [2]) but not Bookworm with Configure [3]. Collation issue, perhaps?

Best,

David

[1]: https://cirrus-ci.com/task/5363472541679616
[2]: https://api.cirrus-ci.com/v1/artifact/task/5363472541679616/testrun/build-32/testrun/regress/regress/regression.diffs
[3]: https://cirrus-ci.com/task/5926422495100928

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-07-11T18:48:46Z

On Jul 10, 2025, at 19:23, David E. Wheeler <david@justatheory.com> wrote:

> Now with the `ISO C90 forbids mixed declarations and code` warning cleared up.
> 
> Weird that there’s a failure on Bookworm with Meson [1] (pg_regress diffs [2]) but not Bookworm with Configure [3]. Collation issue, perhaps?

David Johnson noticed that this build is 32-bit. I looked at the split_path function and after trying a couple of things, realized that it was passing an int8 when the SQL function in Marlena.c passes an int4. This change got the test passing in my clone (indentation reduced):


```patch
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2959,7 +2959,7 @@ executeStringInternalMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
 	C_COLLATION_OID,
 	CStringGetTextDatum(tmp),
 	CStringGetTextDatum(from_str),
-	DirectFunctionCall1(numeric_int8, NumericGetDatum(n))));
+	DirectFunctionCall1(numeric_int4, NumericGetDatum(n))));
 				break;
 			}
 		default:
```

v12 attached.


Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2025-07-12T04:07:08Z

On Fri, Jul 11, 2025 at 9:48 PM David E. Wheeler <david@justatheory.com>
wrote:

> On Jul 10, 2025, at 19:23, David E. Wheeler <david@justatheory.com> wrote:
>
> > Now with the `ISO C90 forbids mixed declarations and code` warning
> cleared up.
> >
> > Weird that there’s a failure on Bookworm with Meson [1] (pg_regress
> diffs [2]) but not Bookworm with Configure [3]. Collation issue, perhaps?
>
> David Johnson noticed that this build is 32-bit. I looked at the
> split_path function and after trying a couple of things, realized that it
> was passing an int8 when the SQL function in Marlena.c passes an int4. This
> change got the test passing in my clone (indentation reduced):


Occasionally I've noticed myself some inconsistencies wrt to compiler
warnings between meson & make .
But cirrus seems generally happy now
https://cirrus-ci.com/build/4964687915253760

To recap so far;

- I like your changes and renames on the parser/lexer; it indeed looks much
cleaner now and will help with future improvements.
- I also like the addition of executeStringInternalMethod ; it'll help us
add more stuff in the future (reminder that for the original patch I
implemented the methods I'd like more, but string operations are quite
more).

- AFAICT no test cases / results have changed with your versions; is this
correct ?

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-07-12T18:24:25Z

On Jul 12, 2025, at 00:07, Florents Tselai <florents.tselai@gmail.com> wrote:

> To recap so far; 
> 
> - I like your changes and renames on the parser/lexer; it indeed looks much cleaner now and will help with future improvements.

Thanks!

> - I also like the addition of executeStringInternalMethod ; it'll help us add more stuff in the future (reminder that for the original patch I implemented the methods I'd like more, but string operations are quite more).

Agreed.

> - AFAICT no test cases / results have changed with your versions; is this correct ?

I made some minor changes, notably to test alternate trim values and a negative position passed to split_part():

```patch
--- a/src/test/regress/sql/jsonb_jsonpath.sql
+++ b/src/test/regress/sql/jsonb_jsonpath.sql
@@ -627,7 +627,7 @@ rollback;
 select jsonb_path_query('"   hello   "', '$.ltrim(" ")');
 select jsonb_path_query('"   hello   "', '$.ltrim(" ")');
 select jsonb_path_query('"   hello   "', '$.ltrim()');
-select jsonb_path_query('"   hello   "', '$.ltrim()');
+select jsonb_path_query('"zzzytest"', '$.ltrim("xyz")');
 select jsonb_path_query('null', '$.ltrim()');
 select jsonb_path_query('null', '$.ltrim()', silent => true);
 select jsonb_path_query('[]', '$.ltrim()');
@@ -647,13 +647,13 @@ select jsonb_path_query_array('["  maybe  ", "  yes", "  no"]', '$[*].ltrim().ty
   -- test .rtrim()
 select jsonb_path_query('"   hello   "', '$.rtrim(" ")');
-select jsonb_path_query('"   hello   "', '$.rtrim(" ")');
+select jsonb_path_query('"testxxzx"', '$.rtrim("xyz")');
 select jsonb_path_query('"   hello   "', '$.rtrim()');
 select jsonb_path_query('"   hello   "', '$.rtrim()');
   -- test .btrim()
 select jsonb_path_query('"   hello   "', '$.btrim(" ")');
-select jsonb_path_query('"   hello   "', '$.btrim(" ")');
+select jsonb_path_query('"xyxtrimyyx"', '$.btrim("xyz")');
 select jsonb_path_query('"   hello   "', '$.btrim()');
 select jsonb_path_query('"   hello   "', '$.btrim()');
  @@ -723,6 +723,7 @@ select jsonb_path_query('"hello world"', '$.replace("hello","bye") starts with "
   -- Test .split_part()
 select jsonb_path_query('"abc~@~def~@~ghi"', '$.split_part("~@~", 2)');
+select jsonb_path_query('"abc,def,ghi,jkl"', '$.split_part(",", -2)');
   -- Test string methods play nicely together
 select jsonb_path_query('"hello world"', '$.replace("hello","bye").upper()');
```

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Álvaro Herrera <alvherre@kurilemu.de> — 2025-10-21T08:05:37Z

This was lacking rebase after the func.sgml changes.  Here it is again.
I have not reviewed it.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Chao Li <li.evan.chao@gmail.com> — 2025-10-23T02:43:13Z

Hi Alvora,

I have reviewed and tested the patch, and got a few comments.

> On Oct 21, 2025, at 16:05, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 
> This was lacking rebase after the func.sgml changes.  Here it is again.
> I have not reviewed it.
> 
> -- 
> Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
> <v13-0001-Rename-jsonpath-method-arg-tokens.patch><v13-0002-Add-additional-jsonpath-string-methods.patch>

1 - jsonpath.c
```
 		case jpiStringFunc:
 			return "string";
+		case jpiStrReplace:
+			return "replace";
+		case jpiStrLower:
+			return "lower";
+		case jpiStrUpper:
+			return "upper";
 		case jpiTime:
 			return "time";
 		case jpiTimeTz:
@@ -914,6 +982,16 @@ jspOperationName(JsonPathItemType type)
 			return "timestamp";
 		case jpiTimestampTz:
 			return "timestamp_tz";
+		case jpiStrLtrim:
+			return "ltrim";
+		case jpiStrRtrim:
+			return "rtrim";
+		case jpiStrBtrim:
+			return "btrim";
+		case jpiStrInitcap:
+			return "initcap";
+		case jpiStrSplitPart:
+			return "split_part";
 		default:
 			elog(ERROR, "unrecognized jsonpath item type: %d", type);
 			return NULL;
```

I wonder if there is some consideration for the order? Feels that jpiSttLtrim and the following jpiStrXXX should be placed above jpiTimeXXX.

2 - jsonpath.c
```
+			if (v->content.arg)
+			{
+				jspGetArg(v, &elem);
+				printJsonPathItem(buf, &elem, false, false);
+			}
```

As there is jspGetArg(), for v->content.arg, does it make sense to add a macro or inline function of jspHasArg(v)?

3 - jsonpath.c
```
+		case jpiStrLtrim:
+			return "ltrim";
+		case jpiStrRtrim:
+			return "rtrim";
+		case jpiStrBtrim:
+			return "btrim";
```

I know “b” in “btrim” stands for “both”, just curious why trim both side function is named “btrim()”? In most of programming languages I am aware of, trim() is the choice. 

4 - jsonpath_exec.c
```
        default:
;
/* cant' happen */
```

Typo: cant’ -> can’t 

5 - jsonpath_exec.c
```
+	/* Create the appropriate jb value to return */
+	switch (jsp->type)
+	{
+			/* Cases for functions that return text */
+		case jpiStrLower:
+		case jpiStrUpper:
+		case jpiStrReplace:
+		case jpiStrLtrim:
+		case jpiStrRtrim:
+		case jpiStrBtrim:
+		case jpiStrInitcap:
+		case jpiStrSplitPart:
+			jb->type = jbvString;
+			jb->val.string.val = resStr;
+			jb->val.string.len = strlen(jb->val.string.val);
+		default:
+			;
+			/* cant' happen */
+	}
``` 

As “default” clause has a comment “can’t happen”, I believe “break” is missing in the case clause.

Also, do we want to add an assert in default to report a message in case it happens?

6 - jsonpath_exec.c
```
+				resStr = TextDatumGetCString(DirectFunctionCall3Coll(replace_text,
+																	 C_COLLATION_OID,
+																	 CStringGetTextDatum(tmp),
+																	 CStringGetTextDatum(from_str),
+																	 CStringGetTextDatum(to_str)));
```

For trim functions, DEFAULT_COLLATION_OID used. Why C_COLLATION_OID is used for replace and split_part? I don’t see anything mentioned in your changes to the doc (func-json.sgml).

7 - jsonpath_exec.c
```
+	if (!(jb = getScalar(jb, jbvString)))
+		RETURN_ERROR(ereport(ERROR,
+							 (errcode(ERRCODE_INVALID_ARGUMENT_FOR_SQL_JSON_DATETIME_FUNCTION),
+							  errmsg("jsonpath item method .%s() can only be applied to a string",
+									 jspOperationName(jsp->type)))));
```

ERRCODE_INVALID_ARGUMENT_FOR_SQL_JSON_DATETIME_FUNCTION seems wrong, this is a string function, not a date time function.

8 - jsonpath_exec.c
```
+	switch (jsp->type)
+	{
+		case jpiStrLtrim:
+		case jpiStrRtrim:
+		case jpiStrBtrim:
+			{
+				char	   *characters_str;
+				int			characters_len;
+				PGFunction	func = NULL;
+
+				if (jsp->content.arg)
+				{
+					switch (jsp->type)
+					{
+						case jpiStrLtrim:
+							func = ltrim;
+							break;
+						case jpiStrRtrim:
+							func = rtrim;
+							break;
+						case jpiStrBtrim:
+							func = btrim;
+							break;
+						default:;
+					}
+					jspGetArg(jsp, &elem);
+					if (elem.type != jpiString)
+						elog(ERROR, "invalid jsonpath item type for .%s() argument", jspOperationName(jsp->type));
+
+					characters_str = jspGetString(&elem, &characters_len);
+					resStr = TextDatumGetCString(DirectFunctionCall2Coll(func,
+																		 DEFAULT_COLLATION_OID, str,
+																		 CStringGetTextDatum(characters_str)));
+					break;
+				}
+
+				switch (jsp->type)
+				{
+					case jpiStrLtrim:
+						func = ltrim1;
+						break;
+					case jpiStrRtrim:
+						func = rtrim1;
+						break;
+					case jpiStrBtrim:
+						func = btrim1;
+						break;
+					default:;
+				}
+				resStr = TextDatumGetCString(DirectFunctionCall1Coll(func,
+																	 DEFAULT_COLLATION_OID, str));
+				break;
+			}
```

The two nested “switch (jsp->type)” are quit redundant. We can pull up the second one, and simplify the first one, something like:

```
Switch (jsp->)
{
  Case:
    ..
}

If (jsp->content.arg)
{
    jspGetArg(jsp, &elem);
    ...
}
```

9 - jsonpath_exec.c
```
+				if (elem.type != jpiString)
+					elog(ERROR, "invalid jsonpath item type for .replace() from");
+
+				from_str = jspGetString(&elem, &from_len);
+
+				jspGetRightArg(jsp, &elem);
+				if (elem.type != jpiString)
+					elog(ERROR, "invalid jsonpath item type for .replace() to");
```

In these two elog(), do we want to log the invalid type? As I see in the “default” clause, jsp->type is logged:
```
+		default:
+			elog(ERROR, "unsupported jsonpath item type: %d", jsp->type);
```

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-10-28T19:38:47Z

On Oct 22, 2025, at 22:43, Chao Li <li.evan.chao@gmail.com> wrote:

> I wonder if there is some consideration for the order? Feels that jpiSttLtrim and the following jpiStrXXX should be placed above jpiTimeXXX.

I wouldn’t think the order would matter.

> I know “b” in “btrim” stands for “both”, just curious why trim both side function is named “btrim()”? In most of programming languages I am aware of, trim() is the choice.

This patch uses existing Postgres functions, of which btrim is one[1].

> + default:
> + ;
> + /* cant' happen */
> + }
> ``` 
> 
> As “default” clause has a comment “can’t happen”, I believe “break” is missing in the case clause.
> 
> Also, do we want to add an assert in default to report a message in case it happens?

Good call, will change.

> 6 - jsonpath_exec.c
> ```
> + resStr = TextDatumGetCString(DirectFunctionCall3Coll(replace_text,
> + C_COLLATION_OID,
> + CStringGetTextDatum(tmp),
> + CStringGetTextDatum(from_str),
> + CStringGetTextDatum(to_str)));
> ```
> 
> For trim functions, DEFAULT_COLLATION_OID used. Why C_COLLATION_OID is used for replace and split_part? I don’t see anything mentioned in your changes to the doc (func-json.sgml).

Intuitively that makes sense to me. Tests pass if I change it. Will update the patch.


> 7 - jsonpath_exec.c
> ```
> + if (!(jb = getScalar(jb, jbvString)))
> + RETURN_ERROR(ereport(ERROR,
> + (errcode(ERRCODE_INVALID_ARGUMENT_FOR_SQL_JSON_DATETIME_FUNCTION),
> +  errmsg("jsonpath item method .%s() can only be applied to a string",
> + jspOperationName(jsp->type)))));
> ```
> 
> ERRCODE_INVALID_ARGUMENT_FOR_SQL_JSON_DATETIME_FUNCTION seems wrong, this is a string function, not a date time function.

Yes. Maybe `ERRCODE_INVALID_PARAMETER_VALUE`? There’s also `ERRCODE_INVALID_JSON_TEXT`, but I think that’s about invalid bytes in a JSON string.

> The two nested “switch (jsp->type)” are quit redundant. We can pull up the second one, and simplify the first one, something like:

Well they assign different values to `func`: ltrim, rtrim, btrim when no arg vs ltrim1, rtrim1, btrim1 when there is an argument.

> 9 - jsonpath_exec.c
> ```
> + if (elem.type != jpiString)
> + elog(ERROR, "invalid jsonpath item type for .replace() from");
> +
> + from_str = jspGetString(&elem, &from_len);
> +
> + jspGetRightArg(jsp, &elem);
> + if (elem.type != jpiString)
> + elog(ERROR, "invalid jsonpath item type for .replace() to");
> ```
> 
> In these two elog(), do we want to log the invalid type? As I see in the “default” clause, jsp->type is logged:
> ```
> + default:
> + elog(ERROR, "unsupported jsonpath item type: %d", jsp->type);
> ```

I think it’s going on precedents such as

```
if (elem.type != jpiNumeric)
	elog(ERROR, "invalid jsonpath item type for .decimal() precision");
```

And also the date time method execution:

```
(errcode(ERRCODE_INVALID_ARGUMENT_FOR_SQL_JSON_DATETIME_FUNCTION),
errmsg("jsonpath item method .%s() can only be applied to a string",
		jspOperationName(jsp->type)))));
```

I see types mentioned only in the context of failed numeric conversions (ERRCODE_NON_NUMERIC_SQL_JSON_ITEM).

Updated patches attached.

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-10-28T19:51:55Z

On Oct 28, 2025, at 15:38, David E. Wheeler <david@justatheory.com> wrote:

> This patch uses existing Postgres functions, of which btrim is one[1].

Accidentally cut out my link before sending!

[1]: https://www.postgresql.org/docs/current/functions-string.html#FUNCTIONS-STRING

D

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

jian he <jian.universality@gmail.com> — 2025-11-28T10:29:17Z

On Wed, Oct 29, 2025 at 3:39 AM David E. Wheeler <david@justatheory.com> wrote:
>

> Updated patches attached.
>
> Best,
>
> David
>
hi.

/*
 * All node's type of jsonpath expression
 *
 * These become part of the on-disk representation of the jsonpath type.
 * Therefore, to preserve pg_upgradability, the order must not be changed, and
 * new values must be added at the end.
 *
 * It is recommended that switch cases etc. in other parts of the code also
 * use this order, to maintain some consistency.
 */
typedef enum JsonPathItemType

some "switch" in the attached patch does not preserve the JsonPathItemType order
consistency, like executeItemOptUnwrapTarget.

--
jian
https://www.enterprisedb.com/

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2025-11-30T21:16:34Z

On Nov 28, 2025, at 05:29, jian he <jian.universality@gmail.com> wrote:

> some "switch" in the attached patch does not preserve the JsonPathItemType order
> consistency, like executeItemOptUnwrapTarget.

Well-spotted, thank you! Fixed in v15, attached.

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

jian he <jian.universality@gmail.com> — 2025-12-04T03:56:31Z

On Mon, Dec 1, 2025 at 5:16 AM David E. Wheeler <david@justatheory.com> wrote:
>
> On Nov 28, 2025, at 05:29, jian he <jian.universality@gmail.com> wrote:
>
> > some "switch" in the attached patch does not preserve the JsonPathItemType order
> > consistency, like executeItemOptUnwrapTarget.
>
> Well-spotted, thank you! Fixed in v15, attached.
>
hi.

seems no deparse regress tests, like:
create view vj as select jsonb_path_query('"   hello   "', '$.ltrim(" ")') as a;
\sv vj

that mean the changes in printJsonPathItem are not tested?


+ /* Create the appropriate jb value to return */
+ switch (jsp->type)
+ {
+ /* Cases for functions that return text */
+ case jpiStrReplace:
comments indentation should align with the word "case"?

executeStringInternalMethod:
+ tmp = pnstrdup(jb->val.string.val, jb->val.string.len);
+ str = CStringGetTextDatum(tmp);
+
+ /* Internal string functions that accept no arguments */
+ switch (jsp->type)
+ {
+ case jpiStrReplace:
+ {
+ char   *from_str,
+   *to_str;
+ int from_len,
+ to_len;
+
+ jspGetLeftArg(jsp, &elem);
+ if (elem.type != jpiString)
+ elog(ERROR, "invalid jsonpath item type for .replace() from");
+
+ from_str = jspGetString(&elem, &from_len);
+
+ jspGetRightArg(jsp, &elem);
+ if (elem.type != jpiString)
+ elog(ERROR, "invalid jsonpath item type for .replace() to");
+
+ to_str = jspGetString(&elem, &to_len);
+
+ resStr = TextDatumGetCString(DirectFunctionCall3Coll(replace_text,
+ DEFAULT_COLLATION_OID,
+ CStringGetTextDatum(tmp),
+ CStringGetTextDatum(from_str),
+ CStringGetTextDatum(to_str)));
+

pnstrdup, CStringGetTextDatum copied twice for the same contend?
I think you can just
``
text *tmp = cstring_to_text_with_len(jb->val.string.val, jb->val.string.len);
Datum str = PointerGetDatum(tmp)
``
In the first main switch block, there's no need to call
``CStringGetTextDatum(tmp)``
because str is already a Datum. We can simply use str directly.

I noticed that almost all of them use DEFAULT_COLLATION_OID,
but jpiStrSplitPart uses C_COLLATION_OID.


--
jian
https://www.enterprisedb.com

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

jian he <jian.universality@gmail.com> — 2025-12-04T04:22:19Z

On Thu, Dec 4, 2025 at 11:56 AM jian he <jian.universality@gmail.com> wrote:
>
> On Mon, Dec 1, 2025 at 5:16 AM David E. Wheeler <david@justatheory.com> wrote:
> >
> > Well-spotted, thank you! Fixed in v15, attached.
> >
>
> seems no deparse regress tests, like:
> create view vj as select jsonb_path_query('"   hello   "', '$.ltrim(" ")') as a;
> \sv vj
>
> that mean the changes in printJsonPathItem are not tested?
>

hi.
seems no tests for the changes in jspIsMutableWalker too.
we can make some simple dummy tests like:

create table tjs(a jsonb);
create index on tjs(jsonb_path_match(a, '$.ltrim(" ")'));

--
jian
https://www.enterprisedb.com/

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

jian he <jian.universality@gmail.com> — 2025-12-07T11:21:47Z

hi.
       <para>
        <literal>jsonb_path_query('"abc,def,ghi,jkl"',
'$.split_part(",", 2)')</literal>
        <returnvalue>"ghi"</returnvalue>
       </para></entry>
the return value should be
"def"
?

      <row>
       <entry role="func_table_entry"><para role="func_signature">
        <replaceable>string</replaceable> <literal>.</literal>
<literal>ltrim(<replaceable>characters</replaceable>)</literal>
        <returnvalue><replaceable>string</replaceable></returnvalue>
       </para>
       <para>
        String with the longest string containing only spaces or the
        characters in <replaceable>characters</replaceable> removed from the
        start of <replaceable>string</replaceable>
       </para>
       <para>
        <literal> jsonb_path_query('"  hello"', '$.ltrim()')</literal>
        <returnvalue>"hello"</returnvalue>
       </para>
       <para>
        <literal>jsonb_path_query('"zzzytest"', '$.ltrim("xyz")')</literal>
        <returnvalue>"test"</returnvalue>
       </para></entry>
      </row>

The actual signature:
<replaceable>characters</replaceable> part is optional, but we didn't
use square brackets
to indicate it's optional.

personally I would prefer list two seperate function signature, like:
       <entry role="func_table_entry"><para role="func_signature">
        <replaceable>string</replaceable> <literal>.</literal>
<literal>ltrim()</literal>
        <returnvalue><replaceable>string</replaceable></returnvalue>
       </para>
       <para role="func_signature">
        <replaceable>string</replaceable> <literal>.</literal>
<literal>ltrim(<replaceable>characters</replaceable>)</literal>
        <returnvalue><replaceable>string</replaceable></returnvalue>
       </para>

similarly:
string . rtrim([ characters ]) → string
change to
```
string . rtrim() → string
string . rtrim(characters ) → string
```

string . btrim([ characters ]) → string
change to
```
string . btrim() → string
string . btrim([ characters ]) → string
``
would improve the readability, I think.


--
jian
https://www.enterprisedb.com/

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2026-01-04T20:51:41Z

On Dec 3, 2025, at 22:56, jian he <jian.universality@gmail.com> wrote:

> seems no deparse regress tests, like:
> create view vj as select jsonb_path_query('"   hello   "', '$.ltrim(" ")') as a;
> \sv vj
> 
> that mean the changes in printJsonPathItem are not tested?

I’m afraid I don’t understand. All of the new tests in src/test/regress/sql/jsonpath.sql exercise the printJsonPathItem changes.

> + /* Create the appropriate jb value to return */
> + switch (jsp->type)
> + {
> + /* Cases for functions that return text */
> + case jpiStrReplace:
> comments indentation should align with the word "case"?

Make sense to me; I thought this was indented by pgindent, but I just re-ran it and it didn’t complain.

> pnstrdup, CStringGetTextDatum copied twice for the same contend?
> I think you can just
> ``
> text *tmp = cstring_to_text_with_len(jb->val.string.val, jb->val.string.len);
> Datum str = PointerGetDatum(tmp)
> ``
> In the first main switch block, there's no need to call
> ``CStringGetTextDatum(tmp)``
> because str is already a Datum. We can simply use str directly.

Very close reading, appreciated. I removed tmp altogether, creating this line:

```c
str = PointerGetDatum(cstring_to_text_with_len(jb->val.string.val, jb->val.string.len));
```

And replacing the use of `CStringGetTextDatum(tmp)` in both the `jpiStrReplace` and the `jpiStrSplitPart` cases.

> I noticed that almost all of them use DEFAULT_COLLATION_OID,
> but jpiStrSplitPart uses C_COLLATION_OID.

Right, fixed.

On Dec 3, 2025, at 23:22, jian he <jian.universality@gmail.com> wrote:

> seems no tests for the changes in jspIsMutableWalker too.
> we can make some simple dummy tests like:
> 
> create table tjs(a jsonb);
> create index on tjs(jsonb_path_match(a, '$.ltrim(" ")'));

Added to the existing `create index` tests to exercise the new functions.

On Dec 7, 2025, at 06:21, jian he <jian.universality@gmail.com> wrote:

> the return value should be
> "def"

Don’t know how that slipped by us; thank you! Fixed by changing the index number to 3.

> The actual signature:
> <replaceable>characters</replaceable> part is optional, but we didn't
> use square brackets
> to indicate it's optional.

Right, fixed.

> similarly:
> string . rtrim([ characters ]) → string
> change to
> ```
> string . rtrim() → string
> string . rtrim(characters ) → string
> ```
> 
> string . btrim([ characters ]) → string
> change to
> ```
> string . btrim() → string
> string . btrim([ characters ]) → string
> ``
> would improve the readability, I think.

I can see that, but the syntax here was borrowed from the existing trim functions, which use the [brackets] for the optional args[1]:

<function>rtrim</function> ( <parameter>string</parameter> <type>text</type>
 <optional>, <parameter>characters</parameter> <type>text</type> </optional> )
<returnvalue>text</returnvalue>

Updated and rebased patch attached.

Best,

David

[1]: https://github.com/postgres/postgres/blob/ac94ce8/doc/src/sgml/func/func-string.sgml#L383

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2026-01-31T14:44:13Z

On Jan 4, 2026, at 15:51, David E. Wheeler <david@justatheory.com> wrote:

> Updated and rebased patch attached.

New rebase on 6918434. See also the PR[0].

The Commitfest app still shows this patch as “Needs review”[1]. It has had a number of reviews, most recently from jian. Any objection to changing it to “Ready for Committer”?

Best,

David

[0] https://github.com/theory/postgres/pull/12
[1] https://commitfest.postgresql.org/patch/5270/

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2026-01-31T16:23:27Z

On Sat, Jan 31, 2026 at 4:44 PM David E. Wheeler <david@justatheory.com>
wrote:

> On Jan 4, 2026, at 15:51, David E. Wheeler <david@justatheory.com> wrote:
>
> > Updated and rebased patch attached.
>

I think your attachment was left behind. Attaching it myself (that's v17,
right? ) checked out from your PR

New rebase on 6918434. See also the PR[0].

> The Commitfest app still shows this patch as “Needs review”[1]. It has had
> a number of reviews, most recently from jian. Any objection to changing it
> to “Ready for Committer”?

With the refactoring you’ve done across the parser and executor,
I’m already tempted to slip in a few more string methods - but I think we
should leave that for a future iteration.
Functionally, it’d just mean adding another branch in
executeStringInternalMethod,
and I’d rather keep this patch focused.

At this point, I’d say it’s ready for committer review.

That said, I do expect someone to raise (again) the question of
how we want to handle potential future conflicts with the SQL/JSON
standard.
That’s a broader design topic, and it’s going to come up every time we
extend the JSONPath language.

And yes, I’m obviously biased here - I already have two other patches in
the same spirit queued up, namely
https://commitfest.postgresql.org/patch/6429/
https://commitfest.postgresql.org/patch/6436/

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2026-01-31T16:31:08Z

On Jan 31, 2026, at 11:23, Florents Tselai <florents.tselai@gmail.com> wrote:

> I think your attachment was left behind. Attaching it myself (that's v17, right? ) checked out from your PR

Bah! Yes, thank you.

> With the refactoring you’ve done across the parser and executor, 
> I’m already tempted to slip in a few more string methods - but I think we should leave that for a future iteration.
> Functionally, it’d just mean adding another branch in executeStringInternalMethod, 
> and I’d rather keep this patch focused.  

Agreed.

> At this point, I’d say it’s ready for committer review. 
> 
> That said, I do expect someone to raise (again) the question of 
> how we want to handle potential future conflicts with the SQL/JSON standard. 
> That’s a broader design topic, and it’s going to come up every time we extend the JSONPath language.

Yup, continual challenge, part of the deal IME.

> And yes, I’m obviously biased here - I already have two other patches in the same spirit queued up, namely
> https://commitfest.postgresql.org/patch/6429/ 
> https://commitfest.postgresql.org/patch/6436/

Ha! Message received, I’ll try to find some time to look at them.

D

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2026-02-01T15:23:58Z

On Jan 31, 2026, at 11:31, David E. Wheeler <david@justatheory.com> wrote:

>> At this point, I’d say it’s ready for committer review. 

Me too, I’ve updated the status[0].

  [0]: https://commitfest.postgresql.org/patch/5270/

Best,

David

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Andrew Dunstan <andrew@dunslane.net> — 2026-03-27T19:24:49Z

On 2026-02-01 Su 10:23 AM, David E. Wheeler wrote:
> On Jan 31, 2026, at 11:31, David E. Wheeler<david@justatheory.com> wrote:
>
>>> At this point, I’d say it’s ready for committer review.
> Me too, I’ve updated the status[0].
>
>    [0]:https://commitfest.postgresql.org/patch/5270/


Patch needs rework following commit 5a2043bf713


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Andrew Dunstan <andrew@dunslane.net> — 2026-03-28T10:04:52Z

On 2026-03-27 Fr 3:24 PM, Andrew Dunstan wrote:
>
>
> On 2026-02-01 Su 10:23 AM, David E. Wheeler wrote:
>> On Jan 31, 2026, at 11:31, David E. Wheeler<david@justatheory.com> wrote:
>>
>>>> At this point, I’d say it’s ready for committer review.
>> Me too, I’ve updated the status[0].
>>
>>    [0]:https://commitfest.postgresql.org/patch/5270/
>
>
> Patch needs rework following commit 5a2043bf713
>



I have fixed that. Some key_name entries were missing, which was an 
issue. I also removed some unused variables and some duplicate tests, 
and did some general tidying.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2026-03-28T14:30:42Z

On Mar 28, 2026, at 06:04, Andrew Dunstan <andrew@dunslane.net> wrote:

> I have fixed that. Some key_name entries were missing, which was an issue. I also removed some unused variables and some duplicate tests, and did some general tidying.

Looks good to me! I’m curious what the implication of missing `key_name`s was, since the tests passed and all the functions worked. Can we create additional tests that would fail when the keys were missing?

D

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Andrew Dunstan <andrew@dunslane.net> — 2026-03-28T15:02:13Z

On 2026-03-28 Sa 10:30 AM, David E. Wheeler wrote:
> On Mar 28, 2026, at 06:04, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>> I have fixed that. Some key_name entries were missing, which was an issue. I also removed some unused variables and some duplicate tests, and did some general tidying.
> Looks good to me! I’m curious what the implication of missing `key_name`s was, since the tests passed and all the functions worked. Can we create additional tests that would fail when the keys were missing?

The key_name production is what allows a keyword to also be used as an 
object key in $.keyname syntax. The trim keywords (ltrim, rtrim, btrim) 
were added there, so $.ltrim as a key works. But $.lower,
$.upper, $.initcap, $.replace, and $.split_part as keys would all break.

There are tests added for it.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

David E. Wheeler <david@justatheory.com> — 2026-03-28T15:58:33Z

On Mar 28, 2026, at 11:02, Andrew Dunstan <andrew@dunslane.net> wrote:

> The key_name production is what allows a keyword to also be used as an object key in $.keyname syntax. The trim keywords (ltrim, rtrim, btrim) were added there, so $.ltrim as a key works. But $.lower,
> $.upper, $.initcap, $.replace, and $.split_part as keys would all break.

Ooh, right. It has been so long since I looked at this stuff that I forgot. Thanks for the reminder.

> There are tests added for it.

And for this!

D

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Andrew Dunstan <andrew@dunslane.net> — 2026-04-02T19:21:54Z

On 2026-03-28 Sa 11:58 AM, David E. Wheeler wrote:
> On Mar 28, 2026, at 11:02, Andrew Dunstan<andrew@dunslane.net> wrote:
>
>> The key_name production is what allows a keyword to also be used as an object key in $.keyname syntax. The trim keywords (ltrim, rtrim, btrim) were added there, so $.ltrim as a key works. But $.lower,
>> $.upper, $.initcap, $.replace, and $.split_part as keys would all break.
> Ooh, right. It has been so long since I looked at this stuff that I forgot. Thanks for the reminder.
>
>> There are tests added for it.
> And for this!
>


Committed.


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Florents Tselai <florents.tselai@gmail.com> — 2026-04-02T19:57:28Z

On Thu, Apr 2, 2026 at 10:21 PM Andrew Dunstan <andrew@dunslane.net> wrote:

>
> On 2026-03-28 Sa 11:58 AM, David E. Wheeler wrote:
>
> On Mar 28, 2026, at 11:02, Andrew Dunstan <andrew@dunslane.net> <andrew@dunslane.net> wrote:
>
>
> The key_name production is what allows a keyword to also be used as an object key in $.keyname syntax. The trim keywords (ltrim, rtrim, btrim) were added there, so $.ltrim as a key works. But $.lower,
> $.upper, $.initcap, $.replace, and $.split_part as keys would all break.
>
> Ooh, right. It has been so long since I looked at this stuff that I forgot. Thanks for the reminder.
>
>
> There are tests added for it.
>
> And for this!
>
>
>
>
> Committed.
>
Boom! Thanks so much for this.

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Marcos Pegoraro <marcos@f10.com.br> — 2026-04-02T21:10:59Z

Em qui., 2 de abr. de 2026 às 16:22, Andrew Dunstan <andrew@dunslane.net>
escreveu:

> Committed.
>
There is a typo here.
<literal>jsonb_path_query('"xyxtrimyyx"', '$.btrim("xyz")')</literal>
should be
<literal>jsonb_path_query('"xyztrimxyz"', '$.btrim("xyz")')</literal>

regards
Marcos

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Andrew Dunstan <andrew@dunslane.net> — 2026-04-03T11:51:46Z

On 2026-04-02 Th 5:10 PM, Marcos Pegoraro wrote:
> Em qui., 2 de abr. de 2026 às 16:22, Andrew Dunstan 
> <andrew@dunslane.net> escreveu:
>
>     Committed.
>
> There is a typo here.
> <literal>jsonb_path_query('"xyxtrimyyx"', '$.btrim("xyz")')</literal>
> should be
> <literal>jsonb_path_query('"xyztrimxyz"', '$.btrim("xyz")')</literal>
>
>

Good catch, will fix.


Thanks


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Andrew Dunstan <andrew@dunslane.net> — 2026-04-03T13:36:31Z

On 2026-04-03 Fr 7:51 AM, Andrew Dunstan wrote:
>
>
> On 2026-04-02 Th 5:10 PM, Marcos Pegoraro wrote:
>> Em qui., 2 de abr. de 2026 às 16:22, Andrew Dunstan 
>> <andrew@dunslane.net> escreveu:
>>
>>     Committed.
>>
>> There is a typo here.
>> <literal>jsonb_path_query('"xyxtrimyyx"', '$.btrim("xyz")')</literal>
>> should be
>> <literal>jsonb_path_query('"xyztrimxyz"', '$.btrim("xyz")')</literal>
>>
>>
>
> Good catch, will fix.
>
>
>

On second thoughts, I don't think this is a typo. btrim() removes 
leading and trailing sequences of any characters in the argument string.:


andrew@~=# select jsonb_path_query('"xyxtrimyyx"', '$.btrim("xyz")');
  jsonb_path_query
------------------
  "trim"
(1 row)


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: PATCH: jsonpath string methods: lower, upper, initcap, l/r/btrim, replace, split_part

Marcos Pegoraro <marcos@f10.com.br> — 2026-04-03T14:34:57Z

Em sex., 3 de abr. de 2026 às 10:36, Andrew Dunstan <andrew@dunslane.net>
escreveu:

> On second thoughts, I don't think this is a typo. btrim() removes leading
> and trailing sequences of any characters in the argument string.
>
Regression has this, so you are correct.
select jsonb_path_query('"zzzytest"', '$.ltrim("xyz")');
 jsonb_path_query
------------------
 "test"

I always thought that only those with exactly the same string would be
removed, but thinking char by char, then it makes perfect sense.
And it works like string trim functions, so everything's fine.
Sorry for the noise.

regards
Marcos