Re: appendBinaryStringInfo stuff

On 10.02.23 20:08, Corey Huinker wrote: On Fri, Feb 10, 2023 at 7:16 AM Peter Eisentraut > wrote: On 19.12.22 07:13, Peter Eisentraut wrote: > Also, the argument type of appendBinaryStringInfo() is char *. There is > some code

Re: appendBinaryStringInfo stuff

On Fri, Feb 10, 2023 at 7:16 AM Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > On 19.12.22 07:13, Peter Eisentraut wrote: > > Also, the argument type of appendBinaryStringInfo() is char *. There is > > some code that uses this function to assemble some kind of packed binary > >

Re: appendBinaryStringInfo stuff

On 19.12.22 07:13, Peter Eisentraut wrote: Also, the argument type of appendBinaryStringInfo() is char *.  There is some code that uses this function to assemble some kind of packed binary layout, which requires a bunch of casts because of this.  I think functions taking binary data plus

Re: appendBinaryStringInfo stuff

On 23.12.22 14:01, David Rowley wrote: Maybe if there's concern that inlining appendStringInfoString is going to bloat the binary too much, then how about we just invent an inlined version of it using some other name that we can use when performance matters? We could then safely replace the

Re: appendBinaryStringInfo stuff

On 23.12.22 10:04, Peter Eisentraut wrote: On 19.12.22 23:48, David Rowley wrote: On Tue, 20 Dec 2022 at 11:42, Tom Lane wrote: I think Peter is entirely right to question whether *this* type's output function is performance-critical.  Who's got large tables with jsonpath columns?  It seems

Re: appendBinaryStringInfo stuff

On Fri, 23 Dec 2022 at 22:04, Peter Eisentraut wrote: > > On 19.12.22 23:48, David Rowley wrote: > > On Tue, 20 Dec 2022 at 11:42, Tom Lane wrote: > >> I think Peter is entirely right to question whether *this* type's > >> output function is performance-critical. Who's got large tables with >

Re: appendBinaryStringInfo stuff

On 19.12.22 23:48, David Rowley wrote: On Tue, 20 Dec 2022 at 11:42, Tom Lane wrote: I think Peter is entirely right to question whether *this* type's output function is performance-critical. Who's got large tables with jsonpath columns? It seems to me the type would mostly only exist as

Re: appendBinaryStringInfo stuff

On Thu, Dec 22, 2022 at 4:19 PM David Rowley wrote: > > Test 1 (from earlier) > > master + escape_json using appendStringInfoCharMacro > $ pgbench -n -T 60 -f bench.sql -M prepared postgres | grep latency > latency average = 1.807 ms > latency average = 1.800 ms > latency average = 1.812 ms

Re: appendBinaryStringInfo stuff

On Thu, 22 Dec 2022 at 20:56, Tom Lane wrote: > > David Rowley writes: > > 22.57% postgres [.] escape_json > > Hmm ... shouldn't we do something like > > -appendStringInfoString(buf, "\\b"); > +appendStringInfoCharMacro(buf, '\\'); > +

Re: appendBinaryStringInfo stuff

David Rowley writes: > 22.57% postgres [.] escape_json Hmm ... shouldn't we do something like -appendStringInfoString(buf, "\\b"); +appendStringInfoCharMacro(buf, '\\'); +appendStringInfoCharMacro(buf, 'b'); and so on in that

Re: appendBinaryStringInfo stuff

On Tue, 20 Dec 2022 at 11:26, David Rowley wrote: > It would be good to see some measurements to find out how much adding > the strlen calls back in would cost us. I tried this out. I'm not pretending I found the best test which highlights how much the performance will change in a real-world

Re: appendBinaryStringInfo stuff

On Tue, Dec 20, 2022 at 10:47 AM Andrew Dunstan wrote: > There are 5 uses in the jsonb code where the length param is a compile > time constant: > > andrew@ub22:adt $ grep appendBinary.*[0-9] jsonb* > jsonb.c:appendBinaryStringInfo(out, "null", 4); > jsonb.c:

Re: appendBinaryStringInfo stuff

On Wed, 21 Dec 2022 at 04:47, Andrew Dunstan wrote: > jsonb.c:appendBinaryStringInfo(out, "", 4); > > None of these really bother me much, TBH. In fact the last one is > arguably nicer because it tells you without counting how many spaces > there are. appendStringInfoSpaces()

Re: appendBinaryStringInfo stuff

Hi, On 2022-12-19 21:29:10 +1300, David Rowley wrote: > On Mon, 19 Dec 2022 at 21:12, Andres Freund wrote: > > Perhaps we should make appendStringInfoString() a static inline function > > - most compilers can compute strlen() of a constant string at compile > > time. > > I had wondered about

Re: appendBinaryStringInfo stuff

On 2022-12-19 Mo 17:48, David Rowley wrote: > On Tue, 20 Dec 2022 at 11:42, Tom Lane wrote: >> I think Peter is entirely right to question whether *this* type's >> output function is performance-critical. Who's got large tables with >> jsonpath columns? It seems to me the type would mostly

Re: appendBinaryStringInfo stuff

On Tue, 20 Dec 2022 at 11:42, Tom Lane wrote: > I think Peter is entirely right to question whether *this* type's > output function is performance-critical. Who's got large tables with > jsonpath columns? It seems to me the type would mostly only exist > as constants within queries. The patch

Re: appendBinaryStringInfo stuff

David Rowley writes: > On Tue, 20 Dec 2022 at 09:23, Peter Eisentraut > wrote: >> AFAICT, the code in question is for the text output of the jsonpath >> type, which is used ... for barely anything? > I think the performance of a type's output function is quite critical. I think Peter is

Re: appendBinaryStringInfo stuff

On Tue, 20 Dec 2022 at 09:23, Peter Eisentraut wrote: > AFAICT, the code in question is for the text output of the jsonpath > type, which is used ... for barely anything? I think the performance of a type's output function is quite critical. I've seen huge performance gains in COPY TO

Re: appendBinaryStringInfo stuff

On 19.12.22 09:12, Andres Freund wrote: There are a bunch of places in the json code that use appendBinaryStringInfo() where appendStringInfoString() could be used, e.g., appendBinaryStringInfo(buf, ".size()", 7); Is there a reason for this? Are we that stretched for performance?

Re: appendBinaryStringInfo stuff

David Rowley writes: > I'm currently not sure why the macro trick increases the binary at > all. I understand why the inline function does. In the places where it changes the code at all, you're replacing appendStringInfoString(buf, s); with appendBinaryStringInfo(buf, s, n);

Re: appendBinaryStringInfo stuff

On Mon, 19 Dec 2022 at 21:12, Andres Freund wrote: > Perhaps we should make appendStringInfoString() a static inline function > - most compilers can compute strlen() of a constant string at compile > time. I had wondered about that, but last time I looked into it there was a small increase in

Re: appendBinaryStringInfo stuff

Hi, On 2022-12-19 07:13:40 +0100, Peter Eisentraut wrote: > I found a couple of adjacent weird things: > > There are a bunch of places in the json code that use > appendBinaryStringInfo() where appendStringInfoString() could be used, e.g., > > appendBinaryStringInfo(buf, ".size()", 7); > >

appendBinaryStringInfo stuff

I found a couple of adjacent weird things: There are a bunch of places in the json code that use appendBinaryStringInfo() where appendStringInfoString() could be used, e.g., appendBinaryStringInfo(buf, ".size()", 7); Is there a reason for this? Are we that stretched for performance? I