Re: Sort functions with specialized comparators
Hello all. I am interested in the proposed patch and would like to propose some additional changes that would complement it. My changes would introduce similar optimizations when working with a list of integers or object identifiers. Additionally, my patch includes an extension for benchmarking, which shows an average speedup of 30-40%. postgres=# SELECT bench_oid_sort(100); bench_oid_sort Time taken by list_sort: 116990848 ns, Time taken by list_oid_sort: 80446640 ns, Percentage difference: 31.24% (1 row) postgres=# SELECT bench_int_sort(100); bench_int_sort Time taken by list_sort: 118168506 ns, Time taken by list_int_sort: 80523373 ns, Percentage difference: 31.86% (1 row) What do you think about these changes? Best regards, Stepan Neretin. On Fri, Jun 7, 2024 at 11:08 PM Andrey M. Borodin wrote: > Hi! > > In a thread about sorting comparators[0] Andres noted that we have > infrastructure to help compiler optimize sorting. PFA attached PoC > implementation. I've checked that it indeed works on the benchmark from > that thread. > > postgres=# CREATE TABLE arrays_to_sort AS >SELECT array_shuffle(a) arr >FROM >(SELECT ARRAY(SELECT generate_series(1, 100)) a), >generate_series(1, 10); > > postgres=# SELECT (sort(arr))[1] FROM arrays_to_sort; -- original > Time: 990.199 ms > postgres=# SELECT (sort(arr))[1] FROM arrays_to_sort; -- patched > Time: 696.156 ms > > The benefit seems to be on the order of magnitude with 30% speedup. > > There's plenty of sorting by TransactionId, BlockNumber, OffsetNumber, Oid > etc. But this sorting routines never show up in perf top or something like > that. > > Seems like in most cases we do not spend much time in sorting. But > specialization does not cost us much too, only some CPU cycles of a > compiler. I think we can further improve speedup by converting inline > comparator to value extractor: more compilers will see what is actually > going on. But I have no proofs for this reasoning. > > What do you think? > > > Best regards, Andrey Borodin. > > [0] > https://www.postgresql.org/message-id/flat/20240209184014.sobshkcsfjix6u4r%40awork3.anarazel.de#fc23df2cf314bef35095b632380b4a59 > From 74bad4bbcff9ea4a9a68f91618c84854dab24701 Mon Sep 17 00:00:00 2001 From: Stepan Neretin Date: Sat, 8 Jun 2024 01:29:42 +0700 Subject: [PATCH v42 6/6] Implemented benchmarking for optimized sorting This commit adds benchmarking functions to compare the performance of two list sorting operations: bench_int_sort and bench_oid_sort. These functions measure the execution time of sorting lists of integers and OIDs, respectively, using different algorithms (list_sort and custom sorting functions). Random lists of specified sizes are generated, sorted using both methods, and their execution times are recorded. The percentage difference in execution time between the two methods is also calculated. This commit aims to provide insights into the efficiency of the sorting algorithms used. --- contrib/Makefile | 1 + contrib/bench_sort_improvements/Makefile | 20 contrib/bench_sort_improvements/bench.c | 105 ++ .../bench_sort_improvements--1.0.sql | 3 + .../bench_sort_improvements.control | 5 + 5 files changed, 134 insertions(+) create mode 100644 contrib/bench_sort_improvements/Makefile create mode 100644 contrib/bench_sort_improvements/bench.c create mode 100644 contrib/bench_sort_improvements/bench_sort_improvements--1.0.sql create mode 100644 contrib/bench_sort_improvements/bench_sort_improvements.control diff --git a/contrib/Makefile b/contrib/Makefile index abd780f277..a1ee9defc2 100644 --- a/contrib/Makefile +++ b/contrib/Makefile @@ -10,6 +10,7 @@ SUBDIRS = \ auto_explain \ basic_archive \ basebackup_to_shell \ + bench_sort_improvements \ bloom \ btree_gin \ btree_gist \ diff --git a/contrib/bench_sort_improvements/Makefile b/contrib/bench_sort_improvements/Makefile new file mode 100644 index 00..46458ee76c --- /dev/null +++ b/contrib/bench_sort_improvements/Makefile @@ -0,0 +1,20 @@ +MODULE_big = bench_sort_improvements + +OBJS = \ + $(WIN32RES) \ + bench.o + +EXTENSION = bench_sort_improvements + +DATA = bench_sort_improvements--1.0.sql + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = contrib/bench_sort_improvements +top_builddir = ../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-g
Re: Sort functions with specialized comparators
On Sat, Jun 8, 2024 at 1:50 AM Stepan Neretin wrote:
> Hello all.
>
> I am interested in the proposed patch and would like to propose some
> additional changes that would complement it. My changes would introduce
> similar optimizations when working with a list of integers or object
> identifiers. Additionally, my patch includes an extension for benchmarking,
> which shows an average speedup of 30-40%.
>
> postgres=# SELECT bench_oid_sort(100);
> bench_oid_sort
>
>
>
> Time taken by list_sort: 116990848 ns, Time taken by list_oid_sort:
> 80446640 ns, Percentage difference: 31.24%
> (1 row)
>
> postgres=# SELECT bench_int_sort(100);
> bench_int_sort
>
>
>
> Time taken by list_sort: 118168506 ns, Time taken by list_int_sort:
> 80523373 ns, Percentage difference: 31.86%
> (1 row)
>
> What do you think about these changes?
>
> Best regards, Stepan Neretin.
>
> On Fri, Jun 7, 2024 at 11:08 PM Andrey M. Borodin
> wrote:
>
>> Hi!
>>
>> In a thread about sorting comparators[0] Andres noted that we have
>> infrastructure to help compiler optimize sorting. PFA attached PoC
>> implementation. I've checked that it indeed works on the benchmark from
>> that thread.
>>
>> postgres=# CREATE TABLE arrays_to_sort AS
>>SELECT array_shuffle(a) arr
>>FROM
>>(SELECT ARRAY(SELECT generate_series(1, 100)) a),
>>generate_series(1, 10);
>>
>> postgres=# SELECT (sort(arr))[1] FROM arrays_to_sort; -- original
>> Time: 990.199 ms
>> postgres=# SELECT (sort(arr))[1] FROM arrays_to_sort; -- patched
>> Time: 696.156 ms
>>
>> The benefit seems to be on the order of magnitude with 30% speedup.
>>
>> There's plenty of sorting by TransactionId, BlockNumber, OffsetNumber,
>> Oid etc. But this sorting routines never show up in perf top or something
>> like that.
>>
>> Seems like in most cases we do not spend much time in sorting. But
>> specialization does not cost us much too, only some CPU cycles of a
>> compiler. I think we can further improve speedup by converting inline
>> comparator to value extractor: more compilers will see what is actually
>> going on. But I have no proofs for this reasoning.
>>
>> What do you think?
>>
>>
>> Best regards, Andrey Borodin.
>>
>> [0]
>> https://www.postgresql.org/message-id/flat/20240209184014.sobshkcsfjix6u4r%40awork3.anarazel.de#fc23df2cf314bef35095b632380b4a59
>
>
Hello all.
I have decided to explore more areas in which I can optimize and have added
two new benchmarks. Do you have any thoughts on this?
postgres=# select bench_int16_sort(100);
bench_int16_sort
-
Time taken by usual sort: 66354981 ns, Time taken by optimized sort:
52151523 ns, Percentage difference: 21.41%
(1 row)
postgres=# select bench_float8_sort(1000000);
bench_float8_sort
------
Time taken by usual sort: 121475231 ns, Time taken by optimized sort:
74458545 ns, Percentage difference: 38.70%
(1 row)
postgres=#
Best regards, Stepan Neretin.
From a3c03ee1b4f6d94d6bf2dc8700c30349271ef9b3 Mon Sep 17 00:00:00 2001
From: Stepan Neretin
Date: Tue, 11 Jun 2024 12:02:48 +0700
Subject: [PATCH v42 08/12] Refactor sorting of attribute numbers in
pg_publication.c and statscmds.c
This commit refactors the sorting of attribute numbers in two modules:
pg_publication.c and statscmds.c. Instead of using the qsort function
with a custom compare function, it utilizes the recently introduced
sort_int_16_arr function, which offers enhanced performance for sorting
int16 arrays.
Details:
- Replaced qsort calls with sort_int_16_arr for sorting attribute numbers.
- Improved efficiency by utilizing the sorting template for int16 arrays.
This change ensures consistency in sorting methods and enhances sorting
performance in relevant modules.
---
src/backend/catalog/pg_publication.c | 2 +-
src/backend/commands/statscmds.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 8518582b76..196f696c1e 100644
--- a/src/backend/catalog/pg_publ
Re: New function normal_rand_array function to contrib/tablefunc.
It looks useful, for example, it can be used in sorting tests to make them more interesting. I just have one question. Why are you using SRF_IS_FIRST CALL and not _PG_init? Best regards, Stepan Neretin.
Re: strange context message in spi.c?
Hi! Looks good to me! Best regards, Stepan Neretin.
Re: sql/json miscellaneous issue
On Mon, Jun 24, 2024 at 5:05 PM jian he wrote: > hi. > the following two queries should return the same result? > > SELECT * FROM JSON_query (jsonb 'null', '$' returning jsonb); > SELECT * FROM JSON_value (jsonb 'null', '$' returning jsonb); > > I've tried a patch to implement it. > (i raised the issue at > > https://www.postgresql.org/message-id/CACJufxFWiCnG3Q7f0m_GdrytPbv29A5OWngCDwKVjcftwzHbTA%40mail.gmail.com > i think a new thread would be more appropriate). > > > > current json_value doc: > "Note that scalar strings returned by json_value always have their > quotes removed, equivalent to specifying OMIT QUOTES in json_query." > > i think there are two exceptions: when the returning data types are > jsonb or json. > > > Hi! I also noticed a very strange difference in behavior in these two queries, it seems to me that although it returns a string by default, for the boolean operator it is necessary to return true or false SELECT * FROM JSON_value (jsonb '1', '$ == "1"' returning jsonb); json_value -------- (1 row) SELECT * FROM JSON_value (jsonb 'null', '$ == "1"' returning jsonb); json_value false (1 row) Best regards, Stepan Neretin.
Re: thread-safety: gmtime_r(), localtime_r()
On Thu, Jun 27, 2024 at 1:42 AM Peter Eisentraut wrote: > Here is a patch for using gmtime_r() and localtime_r() instead of > gmtime() and localtime(), for thread-safety. > > There are a few affected calls in libpq and ecpg's libpgtypes, which are > probably effectively bugs, because those libraries already claim to be > thread-safe. > > There is one affected call in the backend. Most of the backend > otherwise uses the custom functions pg_gmtime() and pg_localtime(), > which are implemented differently. > > Some portability fun: gmtime_r() and localtime_r() are in POSIX but are > not available on Windows. Windows has functions gmtime_s() and > localtime_s() that can fulfill the same purpose, so we can add some > small wrappers around them. (Note that these *_s() functions are also > different from the *_s() functions in the bounds-checking extension of > C11. We are not using those here.) > > MinGW exposes neither *_r() nor *_s() by default. You can get at the > POSIX-style *_r() functions by defining _POSIX_C_SOURCE appropriately > before including . (There is apparently probably also a way to > get at the Windows-style *_s() functions by supplying some additional > options or defines. But we might as well just use the POSIX ones.) > > Hi! Looks good to me. But why you don`t change localtime function at all places? For example: src/bin/pg_controldata/pg_controldata.c src/bin/pg_dump/pg_backup_archiver.c src/bin/initdb/findtimezone.c Best regards, Stepan Neretin.
Re: Patch bug: Fix jsonpath .* on Arrays
On Thu, Jun 27, 2024 at 1:16 AM David E. Wheeler
wrote:
> On Jun 25, 2024, at 13:48, David E. Wheeler wrote:
>
> > I have since realized it’s not a complete fix for the issue, and hacked
> around it in my Go version. Would be fine to remove that bit, but IIRC this
> was the only execution function that would return `jperNotFound` when it in
> fact adds items to the `found` list. The current implementation only looks
> at one or the other, so it’s not super important, but I found the
> inconsistency annoying and sometimes confusing.
>
> I’ve removed this change.
>
> >> [1] select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*');
> >> I propose adding a similar test with explicitly specified lax mode:
> select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*'); to show what
> lax mode is set by default.
> >
> > Very few of the other tests do so; I can add it if it’s important for
> this case, though.
>
> Went ahead and added lax.
>
> > @? suppresses a number of errors. Perhaps I should add a variant of the
> error-raising query that passes the silent arg, which would also suppress
> the error.
>
> Added a variant where the silent param suppresses the error, too.
>
> V2 attached and the PR updated:
>
> https://github.com/theory/postgres/pull/4/files
>
> Best,
>
> David
>
>
>
>
HI! Now it looks good for me.
Best regards, Stepan Neretin.
Re: stale comments about fastgetattr and heap_getattr
Hi! Looks good to me. Please, register it in CF. Best regards, Stepan Neretin. On Fri, Jun 28, 2024 at 10:05 AM Junwang Zhao wrote: > fastgetattr and heap_getattr are converted to inline functions > in e27f4ee0a701, while some comments still referring them as macros. > > -- > Regards > Junwang Zhao >
Re: gamma() and lgamma() functions
On Mon, Jul 1, 2024 at 5:33 PM Dean Rasheed wrote: > Attached is a patch adding support for the gamma and log-gamma > functions. See, for example: > > https://en.wikipedia.org/wiki/Gamma_function > > I think these are very useful general-purpose mathematical functions. > They're part of C99, and they're commonly included in other > mathematical libraries, such as the python math module, so I think > it's useful to make them available from SQL. > > The error-handling for these functions seems to be a little trickier > than most, so that might need further discussion. > > Regards, > Dean > Hi! The patch file seems broken. git apply gamma-and-lgamma.patch error: git apply: bad git-diff — exptec /dev/null in line 2 Best regards, Stepan Neretin.
Re: gamma() and lgamma() functions
On Mon, Jul 1, 2024 at 5:33 PM Dean Rasheed wrote: > Attached is a patch adding support for the gamma and log-gamma > functions. See, for example: > > https://en.wikipedia.org/wiki/Gamma_function > > I think these are very useful general-purpose mathematical functions. > They're part of C99, and they're commonly included in other > mathematical libraries, such as the python math module, so I think > it's useful to make them available from SQL. > > The error-handling for these functions seems to be a little trickier > than most, so that might need further discussion. > > Regards, > Dean > I tried to review the patch without applying it. It looks good to me, but I have one notice: ERROR: value out of range: overflow. I think we need to add information about the available ranges in the error message
Re: Extension for PostgreSQL cast jsonb to hstore WIP
On Mon, Jul 15, 2024 at 12:44 PM Антуан Виолин
wrote:
> On 2024-04-03 Wn 04:21, Andrew Dunstan
>
>> I don't think a cast that doesn't cater for all the forms json can take
>> is going to work very well. At the very least you would need to error out
>> in cases you didn't want to cover, and have tests for all of those errors.
>> But the above is only a tiny fraction of those. If the error cases are
>> going to be so much more than the cases that work it seems a bit pointless.
>>
> Hi everyone
> I changed my mail account to be officially displayed in the correspondence.
> I also made an error conclusion if we are given an incorrect value. I
> believe that such a cast is needed by PostgreSQL since we already have
> several incomplete casts, but they perform their duties well and help in
> the right situations.
>
> cheers
> Antoine Violin
>
> Antoine
>
> On Mon, Jul 15, 2024 at 12:42 PM Andrew Dunstan
> wrote:
>
>>
>> On 2024-04-02 Tu 11:43, ShadowGhost wrote:
>>
>> At the moment, this cast supports only these structures, as it was enough
>> for my tasks:
>> {str:numeric}
>> {str:str}
>> {str:bool}
>> {str:null}
>> But it's a great idea and I'll think about implementing it.
>>
>>
>> Please don't top-post on the PostgreSQL lists. See
>>
>>
>>
>> I don't think a cast that doesn't cater for all the forms json can take
>> is going to work very well. At the very least you would need to error out
>> in cases you didn't want to cover, and have tests for all of those errors.
>> But the above is only a tiny fraction of those. If the error cases are
>> going to be so much more than the cases that work it seems a bit pointless.
>>
>>
>> cheers
>>
>>
>> andrew
>>
>> --
>> Andrew Dunstan
>> EDB: https://www.enterprisedb.com
>>
>>
Hi! I agree in some cases this cast can be useful.
I Have several comments about the patch:
1)I think we should call pfree on pairs(now we call palloc, but not pfree)
2)I think we should add error handling of load_external_function or maybe
rewrite using of DirectFunctionCall
3)i think we need replace all strdup occurences to pstrdup
4)why such a complex system , you first make global variables there to load
a link to functions there, and then wrap this pointer to a function through
a define?
5) postgres=# SELECT '{"aaa": "first_value", "aaa":
"second_value"}'::jsonb::hstore;
hstore
---
"aaa"=>"second_value"
(1 row)
is it documented behaviour?
Re: Sort functions with specialized comparators
On Mon, Jul 15, 2024 at 12:23 PM Антуан Виолин wrote: > Hello all. >> >> I have decided to explore more areas in which I can optimize and have >> added >> two new benchmarks. Do you have any thoughts on this? >> >> postgres=# select bench_int16_sort(100); >> bench_int16_sort >> >> >> - >> Time taken by usual sort: 66354981 ns, Time taken by optimized sort: >> 52151523 ns, Percentage difference: 21.41% >> (1 row) >> >> postgres=# select bench_float8_sort(100); >> bench_float8_sort >> >> >> -- >> Time taken by usual sort: 121475231 ns, Time taken by optimized sort: >> 74458545 ns, Percentage difference: 38.70% >> (1 row) >> > > Hello all > We would like to see the relationship between the length of the sorted > array and the performance gain, perhaps some graphs. We also want to see > to set a "worst case" test, sorting the array in ascending order when it > is initially descending > > Best, regards, Antoine Violin > > postgres=# >> > > On Mon, Jul 15, 2024 at 10:32 AM Stepan Neretin wrote: > >> >> >> On Sat, Jun 8, 2024 at 1:50 AM Stepan Neretin wrote: >> >>> Hello all. >>> >>> I am interested in the proposed patch and would like to propose some >>> additional changes that would complement it. My changes would introduce >>> similar optimizations when working with a list of integers or object >>> identifiers. Additionally, my patch includes an extension for >>> benchmarking, which shows an average speedup of 30-40%. >>> >>> postgres=# SELECT bench_oid_sort(100); >>> bench_oid_sort >>> >>> >>> >>> Time taken by list_sort: 116990848 ns, Time taken by list_oid_sort: >>> 80446640 ns, Percentage difference: 31.24% >>> (1 row) >>> >>> postgres=# SELECT bench_int_sort(100); >>> bench_int_sort >>> >>> >>> >>> Time taken by list_sort: 118168506 ns, Time taken by list_int_sort: >>> 80523373 ns, Percentage difference: 31.86% >>> (1 row) >>> >>> What do you think about these changes? >>> >>> Best regards, Stepan Neretin. >>> >>> On Fri, Jun 7, 2024 at 11:08 PM Andrey M. Borodin >>> wrote: >>> >>>> Hi! >>>> >>>> In a thread about sorting comparators[0] Andres noted that we have >>>> infrastructure to help compiler optimize sorting. PFA attached PoC >>>> implementation. I've checked that it indeed works on the benchmark from >>>> that thread. >>>> >>>> postgres=# CREATE TABLE arrays_to_sort AS >>>>SELECT array_shuffle(a) arr >>>>FROM >>>>(SELECT ARRAY(SELECT generate_series(1, 100)) a), >>>>generate_series(1, 10); >>>> >>>> postgres=# SELECT (sort(arr))[1] FROM arrays_to_sort; -- original >>>> Time: 990.199 ms >>>> postgres=# SELECT (sort(arr))[1] FROM arrays_to_sort; -- patched >>>> Time: 696.156 ms >>>> >>>> The benefit seems to be on the order of magnitude with 30% speedup. >>>> >>>> There's plenty of sorting by TransactionId, BlockNumber, OffsetNumber, >>>> Oid etc. But this sorting routines never show up in perf top or something >>>> like that. >>>> >>>> Seems like in most cases we do not spend much time in sorting. But >>>> specialization does not cost us much too, only some CPU cycles of a >>>> compiler. I think we can further improve speedup by converting inline >>>> comparator to value extractor: more compilers will see what is actually >>>> going on. But I have no proofs for this reasoning. >>>> >>>> What do you think? >>>> >>>> >>>> Best regards, Andrey Borodin. >>>> >>>> [0] >>>> https://www.postgresql.org/message-id/flat/20240209184014.sobshkcsfjix6u4r%40awor
Re: Sort functions with specialized comparators
On Mon, Jul 15, 2024 at 4:52 PM Stepan Neretin wrote: > > > On Mon, Jul 15, 2024 at 12:23 PM Антуан Виолин > wrote: > >> Hello all. >>> >>> I have decided to explore more areas in which I can optimize and have >>> added >>> two new benchmarks. Do you have any thoughts on this? >>> >>> postgres=# select bench_int16_sort(100); >>> bench_int16_sort >>> >>> >>> - >>> Time taken by usual sort: 66354981 ns, Time taken by optimized sort: >>> 52151523 ns, Percentage difference: 21.41% >>> (1 row) >>> >>> postgres=# select bench_float8_sort(100); >>> bench_float8_sort >>> >>> >>> -- >>> Time taken by usual sort: 121475231 ns, Time taken by optimized sort: >>> 74458545 ns, Percentage difference: 38.70% >>> (1 row) >>> >> >> Hello all >> We would like to see the relationship between the length of the sorted >> array and the performance gain, perhaps some graphs. We also want to see >> to set a "worst case" test, sorting the array in ascending order when it >> is initially descending >> >> Best, regards, Antoine Violin >> >> postgres=# >>> >> >> On Mon, Jul 15, 2024 at 10:32 AM Stepan Neretin >> wrote: >> >>> >>> >>> On Sat, Jun 8, 2024 at 1:50 AM Stepan Neretin wrote: >>> >>>> Hello all. >>>> >>>> I am interested in the proposed patch and would like to propose some >>>> additional changes that would complement it. My changes would introduce >>>> similar optimizations when working with a list of integers or object >>>> identifiers. Additionally, my patch includes an extension for >>>> benchmarking, which shows an average speedup of 30-40%. >>>> >>>> postgres=# SELECT bench_oid_sort(100); >>>> bench_oid_sort >>>> >>>> >>>> >>>> Time taken by list_sort: 116990848 ns, Time taken by list_oid_sort: >>>> 80446640 ns, Percentage difference: 31.24% >>>> (1 row) >>>> >>>> postgres=# SELECT bench_int_sort(100); >>>> bench_int_sort >>>> >>>> >>>> >>>> Time taken by list_sort: 118168506 ns, Time taken by list_int_sort: >>>> 80523373 ns, Percentage difference: 31.86% >>>> (1 row) >>>> >>>> What do you think about these changes? >>>> >>>> Best regards, Stepan Neretin. >>>> >>>> On Fri, Jun 7, 2024 at 11:08 PM Andrey M. Borodin >>>> wrote: >>>> >>>>> Hi! >>>>> >>>>> In a thread about sorting comparators[0] Andres noted that we have >>>>> infrastructure to help compiler optimize sorting. PFA attached PoC >>>>> implementation. I've checked that it indeed works on the benchmark from >>>>> that thread. >>>>> >>>>> postgres=# CREATE TABLE arrays_to_sort AS >>>>>SELECT array_shuffle(a) arr >>>>>FROM >>>>>(SELECT ARRAY(SELECT generate_series(1, 100)) a), >>>>>generate_series(1, 10); >>>>> >>>>> postgres=# SELECT (sort(arr))[1] FROM arrays_to_sort; -- original >>>>> Time: 990.199 ms >>>>> postgres=# SELECT (sort(arr))[1] FROM arrays_to_sort; -- patched >>>>> Time: 696.156 ms >>>>> >>>>> The benefit seems to be on the order of magnitude with 30% speedup. >>>>> >>>>> There's plenty of sorting by TransactionId, BlockNumber, OffsetNumber, >>>>> Oid etc. But this sorting routines never show up in perf top or something >>>>> like that. >>>>> >>>>> Seems like in most cases we do not spend much time in sorting. But >>>>> specialization do
Re: Patch bug: Fix jsonpath .* on Arrays
On Mon, Jul 8, 2024 at 11:09 PM David E. Wheeler wrote: > On Jun 27, 2024, at 04:17, Michael Paquier wrote: > > > The tests of jsonb_jsonpath.sql include a lot of patterns for @? and > > jsonb_path_query with the lax and strict modes, so shouldn't these > > additions be grouped closer to the existing tests rather than added at > > the end of the file? > > I’ve moved them closer to other tests for unwrapping behavior in the > attached updated and rebased v3 patch. > > Best, > > David > > > Hi! Looks good to me now! Please, register a patch in CF. Best regards, Stepan Neretin.
Supporting pg freeze in pg_dump, restore.
H, hackersi! Tell me, please Do I understand correctly that pg_dump/pg_restore has not been taught to do COPY FREEZE when filling data? The last mention of the plans was Bruce in 2013: https://postgrespro.com/list/thread-id/1815657 https://paquier.xyz/postgresql-2/postgres-9-3-feature-highlight-copy-freeze/ Best regards, Stepan Neretin.
Re: SPI_connect, SPI_connect_ext return type
> So if we tell extension authors they don't need to check the result, it's unlikely > that that will cause any new code they write to get used with PG > versions where it would be wrong. Yes, I concur. > This combines portions of Stepan's > two patches with some additional work (mostly, that he'd missed fixing > any of contrib/). Thank you for the feedback! I believe the patch looks satisfactory. Should we await a review? The changes seem straightforward to me.
Re: Sort functions with specialized comparators
Hi! I rebase, clean and some refactor my patches.
Best regards, Stepan Neretin.
From f88cbb80e478d5ac3f23945b4ba0ee881f0d9cd4 Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin"
Date: Sun, 8 Sep 2024 15:43:39 +0700
Subject: [PATCH v2 01/10] Use specialized sort facilities
---
contrib/intarray/_int.h | 12
contrib/intarray/_int_gist.c | 2 +-
contrib/intarray/_int_op.c | 19 +--
contrib/intarray/_int_tool.c | 12
src/include/port.h | 2 ++
src/port/qsort.c | 35 +++
6 files changed, 47 insertions(+), 35 deletions(-)
diff --git a/contrib/intarray/_int.h b/contrib/intarray/_int.h
index 0352cbd368..5225c9090a 100644
--- a/contrib/intarray/_int.h
+++ b/contrib/intarray/_int.h
@@ -176,16 +176,4 @@ bool execconsistent(QUERYTYPE *query, ArrayType *array, bool calcnot);
bool gin_bool_consistent(QUERYTYPE *query, bool *check);
bool query_has_required_values(QUERYTYPE *query);
-int compASC(const void *a, const void *b);
-int compDESC(const void *a, const void *b);
-
-/* sort, either ascending or descending */
-#define QSORT(a, direction) \
- do { \
- int _nelems_ = ARRNELEMS(a); \
- if (_nelems_ > 1) \
- qsort((void*) ARRPTR(a), _nelems_, sizeof(int32), \
- (direction) ? compASC : compDESC ); \
- } while(0)
-
#endif /* ___INT_H__ */
diff --git a/contrib/intarray/_int_gist.c b/contrib/intarray/_int_gist.c
index a09b7fa812..d39e40c66a 100644
--- a/contrib/intarray/_int_gist.c
+++ b/contrib/intarray/_int_gist.c
@@ -150,7 +150,7 @@ g_int_union(PG_FUNCTION_ARGS)
ptr += nel;
}
- QSORT(res, 1);
+ sort_int32_asc(ARRPTR(res), ARRNELEMS(res));
res = _int_unique(res);
*size = VARSIZE(res);
PG_RETURN_POINTER(res);
diff --git a/contrib/intarray/_int_op.c b/contrib/intarray/_int_op.c
index 5b164f6788..34d3aa183f 100644
--- a/contrib/intarray/_int_op.c
+++ b/contrib/intarray/_int_op.c
@@ -198,7 +198,6 @@ sort(PG_FUNCTION_ARGS)
text *dirstr = (fcinfo->nargs == 2) ? PG_GETARG_TEXT_PP(1) : NULL;
int32 dc = (dirstr) ? VARSIZE_ANY_EXHDR(dirstr) : 0;
char *d = (dirstr) ? VARDATA_ANY(dirstr) : NULL;
- int dir = -1;
CHECKARRVALID(a);
if (ARRNELEMS(a) < 2)
@@ -208,18 +207,18 @@ sort(PG_FUNCTION_ARGS)
&& (d[0] == 'A' || d[0] == 'a')
&& (d[1] == 'S' || d[1] == 's')
&& (d[2] == 'C' || d[2] == 'c')))
- dir = 1;
+ sort_int32_asc(ARRPTR(a), ARRNELEMS(a));
else if (dc == 4
&& (d[0] == 'D' || d[0] == 'd')
&& (d[1] == 'E' || d[1] == 'e')
&& (d[2] == 'S' || d[2] == 's')
&& (d[3] == 'C' || d[3] == 'c'))
- dir = 0;
- if (dir == -1)
+ sort_int32_desc(ARRPTR(a), ARRNELEMS(a));
+ else
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("second parameter must be \"ASC\" or \"DESC\"")));
- QSORT(a, dir);
+
PG_RETURN_POINTER(a);
}
@@ -229,7 +228,7 @@ sort_asc(PG_FUNCTION_ARGS)
ArrayType *a = PG_GETARG_ARRAYTYPE_P_COPY(0);
CHECKARRVALID(a);
- QSORT(a, 1);
+ sort_int32_asc(ARRPTR(a), ARRNELEMS(a));
PG_RETURN_POINTER(a);
}
@@ -239,7 +238,7 @@ sort_desc(PG_FUNCTION_ARGS)
ArrayType *a = PG_GETARG_ARRAYTYPE_P_COPY(0);
CHECKARRVALID(a);
- QSORT(a, 0);
+ sort_int32_desc(ARRPTR(a), ARRNELEMS(a));
PG_RETURN_POINTER(a);
}
@@ -381,7 +380,7 @@ intset_union_elem(PG_FUNCTION_ARGS)
result = intarray_add_elem(a, PG_GETARG_INT32(1));
PG_FREE_IF_COPY(a, 0);
- QSORT(result, 1);
+ sort_int32_asc(ARRPTR(result), ARRNELEMS(result));
PG_RETURN_POINTER(_int_unique(result));
}
@@ -403,10 +402,10 @@ intset_subtract(PG_FUNCTION_ARGS)
CHECKARRVALID(a);
CHECKARRVALID(b);
- QSORT(a, 1);
+ sort_int32_asc(ARRPTR(a), ARRNELEMS(a));
a = _int_unique(a);
ca = ARRNELEMS(a);
- QSORT(b, 1);
+ sort_int32_asc(ARRPTR(b), ARRNELEMS(b));
b = _int_unique(b);
cb = ARRNELEMS(b);
result = new_intArrayType(ca);
diff --git a/contrib/intarray/_int_tool.c b/contrib/intarray/_int_tool.c
index c85280c842..e83c6aadc6 100644
--- a/contrib/intarray/_int_tool.c
+++ b/contrib/intarray/_int_tool.c
@@ -393,15 +393,3 @@ int_to_intset(int32 elem)
aa[0] = elem;
return result;
}
-
-int
-compASC(const void *a, const void *b)
-{
- return pg_cmp_s32(*(const int32 *) a, *(const int32 *) b);
-}
-
-int
-compDESC(const void *a, const void *b)
-{
- return pg_cmp_s32(*(const int32 *) b, *(const int32 *) a);
-}
diff --git a/src/include/port.h b/src/include/port.h
index ba9ab0d34f..b82f969fc5 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -443,6 +443,8 @@ extern char *strsep(char **stringp, const char *delim);
extern void pg_qsort(void *base, size_t nel, size_t elsize,
int (*cmp) (const void *, const void *));
extern int pg_qsort_strcmp(const void *a, const void
Re: Sort functions with specialized comparators
Hi, why do you think that I rejected Andrey's offer? I included his patch first in my own. Yes, patch 2-0006 is the only patch to which I have not attached any statistics and it looks really dubious. But the rest seem useful. Above, I attached a speed graph for one of the patches and tps( pgbench) What do you think is the format in which to make benchmarks for my patches? Best regards, Stepan Neretin.
Re: Sort functions with specialized comparators
On Tue, Jul 16, 2024 at 1:47 AM Andrey M. Borodin wrote: > > > > On 15 Jul 2024, at 12:52, Stepan Neretin wrote: > > > > > > I run benchmark with my patches: > > ./pgbench -c 10 -j2 -t1000 -d postgres > > > > pgbench (18devel) > > starting vacuum...end. > > transaction type: > > scaling factor: 10 > > query mode: simple > > number of clients: 10 > > number of threads: 2 > > maximum number of tries: 1 > > number of transactions per client: 1000 > > number of transactions actually processed: 1/1 > > number of failed transactions: 0 (0.000%) > > latency average = 1.609 ms > > initial connection time = 24.080 ms > > tps = 6214.244789 (without initial connection time) > > > > and without: > > ./pgbench -c 10 -j2 -t1000 -d postgres > > > > pgbench (18devel) > > starting vacuum...end. > > transaction type: > > scaling factor: 10 > > query mode: simple > > number of clients: 10 > > number of threads: 2 > > maximum number of tries: 1 > > number of transactions per client: 1000 > > number of transactions actually processed: 1/1 > > number of failed transactions: 0 (0.000%) > > latency average = 1.731 ms > > initial connection time = 15.177 ms > > tps = 5776.173285 (without initial connection time) > > > > tps with my patches increase. What do you think? > > > Hi Stepan! > > Thank you for implementing specialized sorting and doing this benchmarks. > I believe it's a possible direction for good improvement. > However, I doubt in correctness of your benchmarks. > Increasing TPC-B performance from 5776 TPS to 6214 TPS seems too good to > be true. > > > Best regards, Andrey Borodin. Yes... I agree.. Very strange.. I restarted the tps measurement and see this: tps = 14291.893460 (without initial connection time) not patched tps = 14669.624075 (without initial connection time) patched What do you think about these measurements? Best regards, Stepan Neretin.
Re: SPI_connect, SPI_connect_ext return type
> >That would break a lot of code (much of it not under our control) to > little purpose; it would also foreclose the option to return to using > SPI_ERROR_CONNECT someday. > Agree, it makes sense. The only question left is, is there any logic in why in some places its return value of these functions is checked, and in some not? Can I add checks everywhere? Best Regards, Stepan Neretin.
Re: SPI_connect, SPI_connect_ext return type
> I'd give it decent odds since our example usage doesn't include the test. > https://www.postgresql.org/docs/current/spi-examples.html https://www.postgresql.org/docs/devel/trigger-example.html <https://www.postgresql.org/docs/current/spi-examples.html> > /* connect to SPI manager */ >if ((ret = SPI_connect()) < 0) >elog(ERROR, "trigf (fired %s): SPI_connect returned %d", when, ret); in this page check include in the example. I think we need to give examples of one kind. If there is no void in the code, then there should be checks everywhere (at least in the documentation). What do you think about the attached patch? Best Regards, Stepan Neretin. From 4c8d6d9ae6f8843bc6d1ad203d629df09657b039 Mon Sep 17 00:00:00 2001 From: Stepan Neretin Date: Sat, 10 Aug 2024 21:30:37 +0300 Subject: [PATCH v1] Fix SPI Documentation - Standardized the example in the execq function to ensure consistent error handling. Now, the error from SPI_connect() is checked and reported uniformly. - Removed the SPI_ERROR_CONNECT return entry, reflecting that there is now only one return value for SPI_connect_ext. --- doc/src/sgml/spi.sgml | 15 +-- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml index 7d154914b9..fdd31c3fdf 100644 --- a/doc/src/sgml/spi.sgml +++ b/doc/src/sgml/spi.sgml @@ -126,16 +126,10 @@ int SPI_connect_ext(int options) - - -SPI_ERROR_CONNECT - - - on error - - - + +All failure cases are reported via elog/ereport. + @@ -5281,7 +5275,8 @@ execq(PG_FUNCTION_ARGS) command = text_to_cstring(PG_GETARG_TEXT_PP(0)); cnt = PG_GETARG_INT32(1); -SPI_connect(); +if ((ret = SPI_connect()) < 0) +elog(ERROR, "SPI_connect returned %d"); ret = SPI_exec(command, cnt); -- 2.43.0
Re: Added anchor links generation function for refsect1,2,3
>
> >It adds docbook function, that generates anchor links ("#" symbol
> >related to any refsect1/2/3).
>
Hi there! It looks good to me that you've made some changes. Anchors are a
great addition for direct linking. I hope this update will make it easier
to write the documentation.
Best regards, Stepan Neretin.
Re: Recovery of .partial WAL segments
Hi there! It looks good to me! But I have a question. How is the partialxlogfname freed in case of an error? Best regards, Stepan Neretin.
Re: Forbid to DROP temp tables of other sessions
Hi, looks good for me, but please fix formatting in temp_tbl_fix.patch!
Re: [PATCH] Improve code coverage of network address functions
Hello Aleksander! I'm a beginner and I would like to see and try your patch. However, I have never worked with coverage in regression tests for PostgreSQL. Could you please tell me how it works and help me understand the process? Thanks! Best Regards, Stepan Neretin!
Re: Using read stream in autoprewarm
Dear Nazir, At first A quick look it looks good. I will take a closer look at it tomorrow. Could you please let me know about the performance tests and graphics? Best regards, Stepan Neretin!
Re: UUID v7
> > I think we typically avoid this kind of check failure by assigning > > uuidv7() and uuidv7(interval) different C functions that call the > > common function. That is, we have pg_proc entries like: > > > > Done. > > >>> > >>> It's odd to me that only uuid_extract_timestamp() supports UUID v6 in > >>> spite of not supporting UUID v6 generation. I think it makes more > >>> sense to support UUID v6 generation as well, if the need for it is > >>> high. > >> > >> RFC urges to use UUIDv7 instead of UUIDv6 when possible. I'm fine with > providing implementation, it's trivial. PFA patch with implementation. > >> > > > > My point is that we should either support full functionality for > > UUIDv6 (such as generation and extraction) or none of them. I'm not > > really sure we want UUIDv6 as well, but if we want it, it should be > > implemented in a separate patch. > > Make sense. I've removed all traces of v6. > Hi there, Firstly, I'd like to discuss the increased_clock_precision variable, which currently divides the timestamp into milliseconds and nanoseconds. However, this approach only approximates the extra bits for sub-millisecond precision, leading to imprecise timestamps in high-frequency UUID generation. To address this issue, we could consider using a more accurate method for calculating the timestamp. For instance, we could utilize a higher resolution clock or implement a more precise algorithm to ensure accurate timestamps. Additionally, it would be beneficial to add validation checks for the interval argument. These checks could verify that the input interval is within reasonable bounds and that the calculated timestamp is accurate. Examples of checks could include verifying if the interval is too small, too large, or exceeds the maximum possible number of milliseconds and nanoseconds in a timestamp. By implementing these changes, we can improve the accuracy and reliability of UUID generation, making it more suitable for high-frequency usage scenarios. What do you think about these suggestions? Let me know your thoughts! Best Regards, Stepan Neretin!
Re: Assertion failure when autovacuum drops orphan temp indexes.
> IIUC after an immediate shutdown all pgstat entries are wiped out so
> the server doesn't have any pgstat entries for databases at this
> point. And since we don't run autovacuum on databases that have no
> pg_stat entries, no autovacuum worker worked on the 'postgres'
> database. Please try executing any query (e.g. 'select 1') on the
> 'postgres' database after the restart, which creates a pgstat entry
> for the database.
>
> > sleep(5);
>
> While the test script sleeps for 5 seconds, the server restarts after
> a crash. So even if the assertion failure happens, the test would
> appear to be successful. I think you can set 'restart_after_crash =
> off' and execute another query using safe_psql() after the sleep. That
> way, the test ends up with safe_psql() failure because the database
> server is not running.
>
Hi, thank you for your suggestions! But they did not help me. Autovacuum
does not want to start :(
```
use strict;
use warnings;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->append_conf(
'postgresql.conf', qq[
autovacuum = on
autovacuum_naptime = 1s
autovacuum_max_workers = 1
restart_after_crash = off
]);
$node->start;
my $psql1 = $node->interactive_psql('postgres');
$psql1->query("create temp table test (a int primary key);");
$node->stop('immediate');
sleep(5);
$node->start;
sleep(3);
$node->restart;
my $psql2 = $node->interactive_psql('postgres');
$psql2->query('SELECT 1;');
$psql2->query('SELECT 1;');
my $regexp = qr/autovacuum/;
my $offset = 0;
$node->wait_for_log($regexp, $offset);
done_testing();
```
Best Regards, Stepan Neretin.
Re: Assertion failure when autovacuum drops orphan temp indexes.
>
> The assertion failure happens in an autovacuum worker. So if you are
> looking for a test that can be integrated in the tree, you could get
> some inspiration from 006_signal_autovacuum.pl and rely on an
> injection point wait with the existing autovacuum-worker-start. My
> 2c, as it looks easy enough to avoid the hardcoded waits.
> --
>
Greetings, I appreciate your proposal. I have attempted to implement your
suggestion, but unfortunately it did not yield the desired results.
```
use strict;
use warnings;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
if ($ENV{enable_injection_points} ne 'yes') {
plan skip_all => 'Injection points not supported by this build';
}
my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->append_conf(
'postgresql.conf', qq[
autovacuum = on
autovacuum_naptime = 1
autovacuum_max_workers = 1
]);
$node->start;
if (!$node->check_extension('injection_points')) {
plan skip_all => 'Extension injection_points not installed';
}
my $psql1 = $node->interactive_psql('postgres');
$psql1->query("create temp table test (a int primary key);");
$node->stop('immediate');
$node->start;
$node->restart;
my $psql2 = $node->interactive_psql('postgres');
$psql2->query('CREATE EXTENSION injection_points;');
$psql2->query("SELECT injection_points_attach('autovacuum-worker-start',
'wait');");
$node->wait_for_event('autovacuum worker', 'autovacuum-worker-start');
$psql2->query('select 1');
my $regexp = qr/autovacuum:/;
my $offset = 0;
ok( $node->log_contains(
$regexp,
$offset),
"autovacuum not started");
done_testing();
```
Best Regards, Stepan Neretin!
Re: Assertion failure when autovacuum drops orphan temp indexes.
> Thank you for your suggestion. IMHO I'm not sure we need to have a > regression test for this bug as it's just an oversight of recently > committed code. My suggestion was just to help Stepan reproduce this > failure using TAP tests. > > Yes, I have reproduced the issue manually before your changes, but I am unable to do so in the perl test. Best Regards, Stepan Neretin!
Re: Assertion failure when autovacuum drops orphan temp indexes.
> IMHO the best way to handle this is to just unconditionally push a
snapshot
> in this code path instead of making assumptions about what callers will
do.
Yes, I agree! I have found the same solution. I attempted to write Perl
tests to verify the patch fix, but the autovacuum process is not triggered
in my tests. Could you please assist me?
```
use strict;
use warnings;
use threads;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::RecursiveCopy;
use PostgreSQL::Test::Utils;
use Test::More;
use Data::Dumper;
my $node = PostgreSQL::Test::Cluster->new('main');
# Create a data directory with initdb
$node->init;
$node->append_conf(
'postgresql.conf', qq[
autovacuum = on
track_counts=on
autovacuum_naptime = 1s
autovacuum_max_workers = 1
autovacuum_vacuum_threshold = 1
]);
# Start the PostgreSQL server
$node->start;
my $psql1 = $node->interactive_psql('postgres');
$psql1->query("create temp table test (a int primary key);");
$node->stop('immediate');
$node->start();
sleep(5);
$node->restart();
ok(1);
done_testing();
```
Best Regards, Stepan Neretin!
Re: Making error message more user-friendly with spaces in a URI
> > I made a patch to make the error message more user-friendly when using a > URI to connect a database with psql. > Hi, Yushi! I could not find this line in the master branch and I couldn't apply this patch. I only see this error in the test (I think the test should also be changed), but the idea of fixing the error looks good to me. Best Regards, Stepan Neretin!
Multi delete wal IDEA
Hi there, hackers! How about trying out an idea to add an analog to save
memory in WAL files for deleting records, similar to multi-insert
optimization? This patch is trying to do just that.
Best Regards, Stepan Neretin!
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 1161520f76..fb2d4563fd 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -3816,13 +3816,14 @@ ExecModifyTable(PlanState *pstate)
CmdType operation = node->operation;
ResultRelInfo *resultRelInfo;
PlanState *subplanstate;
- TupleTableSlot *slot;
+ TupleTableSlot *slot = NULL;
TupleTableSlot *oldSlot;
ItemPointerData tuple_ctid;
HeapTupleData oldtupdata;
HeapTuple oldtuple;
ItemPointer tupleid;
bool tuplock;
+ List *items_to_delete = NULL;
CHECK_FOR_INTERRUPTS();
@@ -4179,8 +4180,15 @@ ExecModifyTable(PlanState *pstate)
break;
case CMD_DELETE:
-slot = ExecDelete(&context, resultRelInfo, tupleid, oldtuple,
- true, false, node->canSetTag, NULL, NULL, NULL);
+ItemPointer item_ptr = (ItemPointer) palloc0(sizeof(ItemPointerData));
+item_ptr->ip_blkid = tupleid->ip_blkid;
+item_ptr->ip_posid = tupleid->ip_posid;
+
+if (!items_to_delete)
+ items_to_delete = list_make1(item_ptr);
+else
+ items_to_delete = lappend(items_to_delete, item_ptr);
+
break;
case CMD_MERGE:
@@ -4197,10 +4205,28 @@ ExecModifyTable(PlanState *pstate)
* If we got a RETURNING result, return it to caller. We'll continue
* the work on next call.
*/
- if (slot)
+ if (slot && !(operation == CMD_DELETE))
return slot;
}
+ if (list_length(items_to_delete) > 0)
+ {
+ ListCell *cell;
+ ereport(WARNING, errmsg("NUM ITEMS TO DELETE = %d", list_length(items_to_delete)));
+ foreach(cell, items_to_delete)
+ {
+ ItemPointer item_ptr = (ItemPointer) lfirst(cell);
+ if (!slot)
+slot = ExecDelete(&context, resultRelInfo, item_ptr, NULL,
+ true, false, node->canSetTag, NULL, NULL, NULL);
+ else
+ExecDelete(&context, resultRelInfo, item_ptr, NULL,
+ true, false, node->canSetTag, NULL, NULL, NULL);
+ }
+
+ return slot;
+ }
+
/*
* Insert remaining tuples for batch insert.
*/
Optimize truncation logic to reduce AccessExclusive lock impact
Hi hackers, We've encountered an issue where autovacuum holds an AccessExclusive lock for an extended period when attempting to truncate heap tables. The root cause appears to be the way autovacuum handles block truncation, specifically when scanning shared buffers during smgrtruncate. This issue is particularly painful for large shared buffer configurations (e.g., 1.8 TiB with NUMA and multiple workers), and pg_statistic is among the most affected tables. When autovacuum detects freeable blocks at the tail of a relation, it attempts to truncate them. It acquires an AccessExclusive lock and scans all shared buffers to remove the corresponding blocks. With a large shared buffer configuration (e.g., 1.8 TiB, 200 million blocks, and multiple NUMA nodes), this process is slow and significantly impacts performance. We propose modifying the truncation condition in should_attempt_truncation to avoid unnecessary full buffer scans. The new formula ensures we only attempt truncation when we can free at least 3% of the relation size + 2 pages. This change prevents excessive truncation attempts on small tables while reducing the impact on larger relations. Previously, small tables could theoretically be truncated to a single page, but with this change, they may remain around 3 pages instead. We don't see this as a significant issue. The idea for this patch was proposed by Yuriy Sokolov. There is also an open discussion about optimizing the shared buffer traversal to minimize the impact of AccessExclusive locks. This patch is a step toward improving the situation. The patch is attached to this message. Best regards, Stepan Neretin. diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 159b822740ab32989d8e5d4da71a61c3450ec0f3..d8211470fc34cac00b3da2d8c1bd786ba9ed1e08 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -67,8 +67,8 @@ * REL_TRUNCATE_MINIMUM or (relsize / REL_TRUNCATE_FRACTION) (whichever * is less) potentially-freeable pages. */ -#define REL_TRUNCATE_MINIMUM 1000 -#define REL_TRUNCATE_FRACTION 16 +#define REL_TRUNCATE_MINIMUM 2 +#define REL_TRUNCATE_FRACTION 32 /* * Timing parameters for truncate locking heuristics. @@ -3089,9 +3089,7 @@ should_attempt_truncation(LVRelState *vacrel) return false; possibly_freeable = vacrel->rel_pages - vacrel->nonempty_pages; - if (possibly_freeable > 0 && - (possibly_freeable >= REL_TRUNCATE_MINIMUM || - possibly_freeable >= vacrel->rel_pages / REL_TRUNCATE_FRACTION)) + if (possibly_freeable >= vacrel->rel_pages / REL_TRUNCATE_FRACTION + REL_TRUNCATE_MINIMUM) return true; return false;
Re: Accessing an invalid pointer in BufferManagerRelation structure
Hello, Dmitriy Bondar and I decided to review this patch as part of the CommitFest Review. The first thing we both noticed is that the macro calls a function that won't be available without an additional header. This seems a bit inconvenient. I also have a question: is the logic correct that if the relation is valid, we should fetch it rather than the other way around? Additionally, is checking only the `rd_isvalid` flag sufficient, or should we also consider the flag below? ``` bool rd_isvalid; /* relcache entry is valid */ ``` Other than that, the tests pass, and there are no issues with code style. Thanks for the patch - it could indeed help prevent future issues. Best regards, Stepan Neretin вт, 11 мар. 2025 г., 17:32 Daniil Davydov <3daniss...@gmail.com>: > Hi, > I am posting the new v2 patch, which is now rebased on the `master` branch. > Waiting for your feedback :) > > -- > Best regards, > Daniil Davydov >
Re: FSM doesn't recover after zeroing damaged page.
On Fri, Feb 7, 2025 at 7:15 AM Anton A. Melnikov
wrote:
> Hi!
>
> At the current master i found that if not the last page of
> the FSM bottom layer was corrupted it is not restored after zeroing.
>
> Here is reproduction like that:
> 1) Create a table with FSM of 4 pages:
> create table t (int) as select * from generate_series(1, 1E6);
> delete from t where ctid in (select ctid from t tablesample bernoulli
> (20));
> SELECT pg_relation_filepath('t'); -- to know the filename with FSM
> vacuum t;
>
> 2) Do checkpoint and stop the server.
>
> 3) Corrupt a byte in the third page. For instance, the lower byte of the
> CRC:
> printf '\xAA' | dd of=/usr/local/pg12252-vanm/data/base/5/
> bs=1 seek=$((2*8192+8)) count=1 conv=notrunc
>
> 4) start server and execute: vacuum t; twice: to ensure that corrupted page
> is fixed in memory, zeroed and a new header was written on it.
>
> postgres=# vacuum t;
> WARNING: page verification failed, calculated checksum 13869 but expected
> 13994
> WARNING: invalid page in block 2 of relation base/5/16384_fsm; zeroing
> out page
> VACUUM
> postgres=# vacuum t; -- without warnings
> VACUUM
>
> 5) Do checkpoint and restart the server. After vacuum t; the warnings
> appeared again:
> postgres=# vacuum t;
> WARNING: page verification failed, calculated checksum 13869 but expected
> 13994
> WARNING: invalid page in block 2 of relation base/5/16384_fsm; zeroing
> out page
> VACUUM
>
> I noticed that the updated page is not written to disk because the
> buffer where it is located is not marked dirty. Moreover
> MarkBufferDirtyHint(),
> which is called for modified FSM pages, seems is not suitable here,
> since as i suppose the corrupted page must be rewritten certainly, not for
> hint.
> Therefore, maybe mark it dirty immediately after writing the new header?
> Here is a small patch that does it and eliminates multiple warnings.
> Would be glad if you take a look on it.
>
> With the best regards,
>
> --
> Anton A. Melnikov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
Hi, I reproduce the problem step by step. Patch fixes this problem. Looks
good to me.
Best Regards, Stepan Neretin.
Re: Proposal for enabling auto-vectorization for checksum calculations
On Sat, May 10, 2025 at 6:01 PM Stepan Neretin wrote:
>
>
> On Thu, May 8, 2025 at 6:57 AM Matthew Sterrett <
> matthewsterre...@gmail.com> wrote:
>
>> Hello! I'm still trying to figure out those CI failures, I just wanted
>> to update things.
>>
>> From my testing, with this patch repeatedly disabling/enabling
>> checksums is about 12.4% on an approximately 15 GB database.
>>
>> By the way, I'd love it if anyone could help me figure out how to
>> replicate a CI failure in the Cirrus CI.
>> I haven't been able to figure out how to test CI runs locally, does
>> anyone know a good method to do that?
>>
>>
>>
> Hi Matthew,
>
> Thanks for the patch!
>
> I ran some timing tests:
>
> (without avx2)
>
> Time: 4034.351 ms
> SELECT drive_pg_checksum(512);
>
> (with avx2)
>
>
> Time: 3559.076 ms
> SELECT drive_pg_checksum(512);
>
> Also attached two patches that should fix the CI issues.
>
> Best,
>
> Stepan Neretin
>
>
>
Oops, forgot to attach patches :)
Best,
Stepan Neretin
From a6288a4e73d97f3e76e4dabc1e6e6a86478ddbcd Mon Sep 17 00:00:00 2001
From: Matthew Sterrett
Date: Fri, 7 Mar 2025 11:33:45 -0800
Subject: [PATCH v2 1/4] Enable autovectorizing pg_checksum_block
---
config/c-compiler.m4| 31 ++
configure | 52 ++
configure.ac| 9 ++
meson.build | 28 ++
src/include/pg_config.h.in | 3 +
src/include/storage/checksum_impl.h | 150 +++-
6 files changed, 250 insertions(+), 23 deletions(-)
diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index 5f3e1d1faf9..4d5bceafe9e 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -710,6 +710,37 @@ fi
undefine([Ac_cachevar])dnl
])# PGAC_XSAVE_INTRINSICS
+# PGAC_AVX2_SUPPORT
+# -
+# Check if the compiler supports AVX2 in attribute((target))
+# and using AVX2 intrinsics in those functions
+#
+# If the intrinsics are supported, sets pgac_avx2_support.
+AC_DEFUN([PGAC_AVX2_SUPPORT],
+[define([Ac_cachevar], [AS_TR_SH([pgac_cv_avx2_support])])dnl
+AC_CACHE_CHECK([for AVX2 support], [Ac_cachevar],
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include
+#include
+#if defined(__has_attribute) && __has_attribute (target)
+__attribute__((target("avx2")))
+#endif
+static int avx2_test(void)
+{
+ const char buf@<:@sizeof(__m256i)@:>@;
+ __m256i accum = _mm256_loadu_si256((const __m256i *) buf);
+ accum = _mm256_add_epi32(accum, accum);
+ int result = _mm256_extract_epi32(accum, 0);
+ return (int) result;
+}],
+ [return avx2_test();])],
+ [Ac_cachevar=yes],
+ [Ac_cachevar=no])])
+if test x"$Ac_cachevar" = x"yes"; then
+ pgac_avx2_support=yes
+fi
+undefine([Ac_cachevar])dnl
+])# PGAC_AVX2_SUPPORT
+
# PGAC_AVX512_POPCNT_INTRINSICS
# -
# Check if the compiler supports the AVX-512 popcount instructions using the
diff --git a/configure b/configure
index 275c67ee67c..5ae2dc00d33 100755
--- a/configure
+++ b/configure
@@ -17711,6 +17711,58 @@ $as_echo "#define HAVE_XSAVE_INTRINSICS 1" >>confdefs.h
fi
+# Check for AVX2 target and intrinsic support
+#
+if test x"$host_cpu" = x"x86_64"; then
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for AVX2 support" >&5
+$as_echo_n "checking for AVX2 support... " >&6; }
+if ${pgac_cv_avx2_support+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include
+#include
+#if defined(__has_attribute) && __has_attribute (target)
+__attribute__((target("avx2")))
+#endif
+static int avx2_test(void)
+{
+ const char buf[sizeof(__m256i)];
+ __m256i accum = _mm256_loadu_si256((const __m256i *) buf);
+ accum = _mm256_add_epi32(accum, accum);
+ int result = _mm256_extract_epi32(accum, 0);
+ return (int) result;
+}
+int
+main ()
+{
+return avx2_test();
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+ pgac_cv_avx2_support=yes
+else
+ pgac_cv_avx2_support=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_avx2_support" >&5
+$as_echo "$pgac_cv_avx2_support" >&6; }
+if test x"$pgac_cv_avx2_support" = x"yes"; then
+ pgac_avx2_support=yes
+fi
+
+ if test x"$pgac_avx2_support" = x"yes"; then
+
+$as_echo "#define USE_AVX2_WITH_RUNTIME_CHECK 1" >>confdefs.h
+
+ fi
+fi
+
# Check for AVX-512 popcount intr
Re: Suggestion to add --continue-client-on-abort option to pgbench
On Sat, May 10, 2025 at 8:45 PM ikedarintarof wrote: > Hi hackers, > > I would like to suggest adding a new option to pgbench, which enables > the client to continue processing transactions even if some errors occur > during a transaction. > Currently, a client stops sending requests when its transaction is > aborted due to reasons other than serialization failures or deadlocks. I > think in some cases, especially when using custom scripts, the client > should be able to rollback the failed transaction and start a new one. > > For example, my custom script (insert_to_unique_column.sql) follows: > ``` > CREATE TABLE IF NOT EXISTS test (col1 serial, col2 int unique); > INSERT INTO test (col2) VALUES (random(0, 5)); > ``` > Assume we need to continuously apply load to the server using 5 clients > for a certain period of time. However, a client sometimes stops when its > transaction in my custom script is aborted due to a check constraint > violation. As a result, the load on the server is lower than expected, > which is the problem I want to address. > > The proposed new option solves this problem. When > --continue-client-on-abort is set to true, the client rolls back the > failed transaction and starts a new one. This allows all 5 clients to > continuously apply load to the server, even if some transactions fail. > > ``` > % bin/pgbench -d postgres -f ../insert_to_unique_column.sql -T 10 > --failures-detailed --continue-client-on-error > transaction type: ../custom_script_insert.sql > scaling factor: 1 > query mode: simple > number of clients: 1 > number of threads: 1 > maximum number of tries: 1 > duration: 10 s > number of transactions actually processed: 33552 > number of failed transactions: 21901 (39.495%) > number of serialization failures: 0 (0.000%) > number of deadlock failures: 0 (0.000%) > number of other failures: 21901 (39.495%) > latency average = 0.180 ms (including failures) > initial connection time = 2.857 ms > tps = 3356.092385 (without initial connection time) > ``` > > I have attached the patch. I would appreciate your feedback. > > Best regards, > > Rintaro Ikeda > NTT DATA Corporation Japan Hi Rintaro, Thanks for the patch and explanation. I understand your goal is to ensure that pgbench clients continue running even when some transactions fail due to application-level errors (e.g., constraint violations), especially when running custom scripts. However, I wonder if the intended behavior can't already be achieved using standard SQL constructs — specifically ON CONFLICT or careful transaction structure. For example, your sample script: CREATE TABLE IF NOT EXISTS test (col1 serial, col2 int unique); INSERT INTO test (col2) VALUES (random(0, 5)); can be rewritten as: \setrandom val 0 5 INSERT INTO test (col2) VALUES (:val) ON CONFLICT DO NOTHING; This avoids transaction aborts entirely in the presence of uniqueness violations and ensures the client continues to issue load without interruption. In many real-world benchmarking scenarios, this is the preferred and simplest approach. So from that angle, could you elaborate on specific cases where this SQL-level workaround wouldn't be sufficient? Are there error types you intend to handle that cannot be gracefully avoided or recovered from using SQL constructs like ON CONFLICT, or SAVEPOINT/ROLLBACK TO? Best regards, Stepan Neretin
Re: Proposal for enabling auto-vectorization for checksum calculations
On Thu, May 8, 2025 at 6:57 AM Matthew Sterrett wrote: > Hello! I'm still trying to figure out those CI failures, I just wanted > to update things. > > From my testing, with this patch repeatedly disabling/enabling > checksums is about 12.4% on an approximately 15 GB database. > > By the way, I'd love it if anyone could help me figure out how to > replicate a CI failure in the Cirrus CI. > I haven't been able to figure out how to test CI runs locally, does > anyone know a good method to do that? > > > Hi Matthew, Thanks for the patch! I ran some timing tests: (without avx2) Time: 4034.351 ms SELECT drive_pg_checksum(512); (with avx2) Time: 3559.076 ms SELECT drive_pg_checksum(512); Also attached two patches that should fix the CI issues. Best, Stepan Neretin
Re: Suggestion to add --continue-client-on-abort option to pgbench
On Sun, May 11, 2025 at 7:07 PM Matthias van de Meent < boekewurm+postg...@gmail.com> wrote: > > On Sat, May 10, 2025 at 8:45 PM ikedarintarof < > ikedarinta...@oss.nttdata.com> wrote: > >> > >> Hi hackers, > >> > >> I would like to suggest adding a new option to pgbench, which enables > >> the client to continue processing transactions even if some errors occur > >> during a transaction. > >> Currently, a client stops sending requests when its transaction is > >> aborted due to reasons other than serialization failures or deadlocks. I > >> think in some cases, especially when using custom scripts, the client > >> should be able to rollback the failed transaction and start a new one. > >> > >> For example, my custom script (insert_to_unique_column.sql) follows: > >> ``` > >> CREATE TABLE IF NOT EXISTS test (col1 serial, col2 int unique); > >> INSERT INTO test (col2) VALUES (random(0, 5)); > >> ``` > >> Assume we need to continuously apply load to the server using 5 clients > >> for a certain period of time. However, a client sometimes stops when its > >> transaction in my custom script is aborted due to a check constraint > >> violation. As a result, the load on the server is lower than expected, > >> which is the problem I want to address. > >> > >> The proposed new option solves this problem. When > >> --continue-client-on-abort is set to true, the client rolls back the > >> failed transaction and starts a new one. This allows all 5 clients to > >> continuously apply load to the server, even if some transactions fail. > > +1. I've had similar cases before too, where I'd wanted pgbench to > continue creating load on the server even if a transaction failed > server-side for any reason. Sometimes, I'd even want that type of > load. > > On Sat, 10 May 2025 at 17:02, Stepan Neretin wrote: > > INSERT INTO test (col2) VALUES (random(0, 5)); > > > > can be rewritten as: > > > > \setrandom val 0 5 > > INSERT INTO test (col2) VALUES (:val) ON CONFLICT DO NOTHING; > > That won't test the same execution paths, so an option to explicitly > rollback or ignore failed transactions (rather than stopping the > benchmark) would be a nice feature. > With e.g. ON CONFLICT DO NOTHING you'll have much higher workload if > there are many conflicting entries, as that triggers and catches > per-row errors, rather than per-statement. E.g. INSERT INTO ... SELECT > ...multiple rows could conflict on multiple rows, but will fail on the > first conflict. DO NOTHING would cause full execution of the SELECT > statement, which has an inherently different performance profile. > > > This avoids transaction aborts entirely in the presence of uniqueness > violations and ensures the client continues to issue load without > interruption. In many real-world benchmarking scenarios, this is the > preferred and simplest approach. > > > > So from that angle, could you elaborate on specific cases where this > SQL-level workaround wouldn't be sufficient? Are there error types you > intend to handle that cannot be gracefully avoided or recovered from using > SQL constructs like ON CONFLICT, or SAVEPOINT/ROLLBACK TO? > > The issue isn't necessarily whether you can construct SQL scripts that > don't raise such errors (indeed, it's possible to do so for nearly any > command; you can run pl/pgsql procedures or DO blocks which catch and > ignore errors), but rather whether we can make pgbench function in a > way that can keep load on the server even when it notices an error. > > Kind regards, > > Matthias van de Meent > Neon (https://neon.tech) > Hi Matthias, Thanks for your detailed explanation — it really helped clarify the usefulness of the patch. I agree that the feature is indeed valuable, and it's great to see it being pushed forward. Regarding the patch code, I noticed that there are duplicate case entries in the command-line option handling (specifically for case 18 or case ESTATUS_OTHER_SQL_ERROR, the continue-client-on-error option). These duplicated cases can be merged to simplify the logic and reduce redundancy. Best regards, Stepan Neretin
Re: [PATCH] avoid double scanning in function byteain
On Fri, May 9, 2025 at 5:24 PM Stepan Neretin wrote: > > > On Wed, Mar 26, 2025 at 9:39 PM Steven Niu wrote: > >> >> 在 2025/3/26 16:37, Kirill Reshke 写道: >> > On Wed, 26 Mar 2025 at 12:17, Steven Niu wrote: >> >> >> >> Hi, >> > >> > Hi! >> > >> >> This double scanning can be inefficient, especially for large inputs. >> >> So I optimized the function to eliminate the need for two scans, >> >> while preserving correctness and efficiency. >> > >> > While the argument that processing input once not twice is fast is >> > generally true, may we have some simple bench here just to have an >> > idea how valuable this patch is? >> > >> > >> > Patch: >> > >> > >> >> + /* Handle traditional escaped style in a single pass */ >> >> + input_len = strlen(inputText); >> >> + result = palloc(input_len + VARHDRSZ); /* Allocate max possible >> size */ >> >> rp = VARDATA(result); >> >> + tp = inputText; >> >> + >> >> while (*tp != '\0') >> > >> > >> > Isn't this `strlen` O(n) + `while` O(n)? Where is the speed up? >> > >> > >> > >> > [0] https://github.com/bminor/glibc/blob/master/string/strlen.c#L43-L45 >> > >> >> >> >> Hi, Kirill, >> >> Your deep insight suprised me! >> >> Yes, you are correct that strlen() actually performed a loop operation. >> So maybe the performance difference is not so obvious. >> >> However, there are some other reasons that drive me to make this change. >> >> 1. The author of original code left comment: "BUGS: The input is scanned >> twice." . >> You can find this line of code in my patch. This indicates a left work >> to be done. >> >> 2. If I were the author of this function, I would not be satisfied with >> myself that I used two loops to do something which actually can be done >> with one loop. >> I prefer to choose a way that would not add more burden to readers. >> >> 3. The while (*tp != '\0') loop has some unnecessary codes and I made >> some change. >> >> Thanks, >> Steven >> >> >> > > > Hi hackers, > > This is a revised version (v2) of the patch that optimizes the `byteain()` > function. > > The original implementation handled escaped input by scanning the string > twice — first to determine the output size and again to fill in the bytea. > This patch eliminates the double scan by using a single-pass approach with > `StringInfo`, simplifying the logic and improving maintainability. > > Changes since v1 (originally by Steven Niu): > - Use `StringInfo` instead of manual memory allocation. > - Remove redundant code and improve readability. > - Add regression tests for both hex and escaped formats. > > This version addresses performance and clarity while ensuring > compatibility with existing behavior. The patch also reflects discussion on > the original version, including feedback from Kirill Reshke. > > Looking forward to your review and comments. > > Best regards, > Stepan Neretin > Hi, I noticed that the previous version of the patch was authored with an incorrect email address due to a misconfigured git config. I've corrected the author information in this v2 and made sure it's consistent with my usual contributor identity. No other changes were introduced apart from that and the updates discussed earlier. Sorry for the confusion, and thanks for your understanding. Best regards, Stepan Neretin From 92c581fdd3081d8ac60ead2f2ffde823377efbbb Mon Sep 17 00:00:00 2001 From: Stepan Neretin Date: Fri, 9 May 2025 17:36:28 +0700 Subject: [PATCH v2 2/2] Refactor byteain() to avoid double scanning and simplify logic This patch reworks the escaped input handling in byteain() by replacing manual buffer management with a StringInfo-based single-pass parse. It combines ideas from a previous proposal by Steven Niu with additional improvements to structure and readability. Also adds regression tests covering edge cases for both hex and escaped formats. Includes input from discussion with Kirill Reshke on pgsql-hackers. --- contrib/btree_gin/expected/bytea.out | 92 contrib/btree_gin/sql/bytea.sql | 37 +++ src/backend/utils/adt/varlena.c | 63 --- 3 files changed, 155 insertions(+), 37 deletions(-) diff --git a/contrib/btree_gin/expected/bytea.out b/contrib/btree_gin/expected/bytea.out index b0ed7a53450..d4ad2878775 100644 --- a/contrib/btree_gin/expected/bytea.o
Re: [PATCH] Fix references in comments, and sync up heap_page_is_all_visible() with heap_page_prune_and_freeze()
On Tue, May 6, 2025 at 11:08 PM Gregory Burd wrote: > While working on [1] I found an outdated comment in > heap_page_is_all_visible() and two other small fixes. > > 0001: Updates that comment so future authors know that this "stripped down > function" should retain the logic in heap_page_prune_and_freeze(), not > lazy_scan_prune() as was the case before 6dbb490. > 0002: Mimics the same loop logic as in heap_page_is_all_visible() so as to > a) stay in sync and b) benefit from the mentioned CPU prefetching > optimization. > 0003: Moves the ItemSetPointer() just a bit further down in the function > again to a) stay in sync and b) to sometimes avoid that tiny overhead. > > best, > > -greg > > PS: per-community standards I've switched to my personal email address > rather than gregb...@amazon.com > > [1] > https://www.postgresql.org/message-id/flat/78574b24-be0a-42c5-8075-3fa9fa63b...@amazon.com Hi, looks good for me. Best regards, Stepan Neretin.
Re: [PATCH] avoid double scanning in function byteain
On Fri, May 9, 2025 at 7:43 PM Aleksander Alekseev wrote: > Hi Stepan, > > > Sorry for the noise — I'm resending the patch because I noticed a > compiler warning related to mixed declarations and code, which I’ve now > fixed. > > > > Apologies for the oversight in the previous submission. > > Thanks for the patch. > > As Kirill pointed out above, it would be nice if you could prove that > your implementation is actually faster. I think something like a > simple micro-benchmark will do. > > -- > Best regards, > Aleksander Alekseev > Hi, Thanks for the feedback. I’ve done a simple micro-benchmark using PL/pgSQL with a large escaped input string (\\123 repeated 100,000 times), converted to bytea in a loop: DO $$ DECLARE start_time TIMESTAMP; end_time TIMESTAMP; i INTEGER; dummy BYTEA; input TEXT := repeat(E'\\123', 10); elapsed_ms DOUBLE PRECISION; BEGIN start_time := clock_timestamp(); FOR i IN 1..1000 LOOP dummy := input::bytea; END LOOP; end_time := clock_timestamp(); elapsed_ms := EXTRACT(EPOCH FROM end_time - start_time) * 1000; RAISE NOTICE 'Average time per call: % ms', elapsed_ms / 1000; END $$; Here are the results from NOTICE output: *Without patch:* NOTICE: Average time per call: 0.491766004 ms NOTICE: Average time per call: 0.476589996 ms *With patch:* NOTICE: Average time per call: 0.468231 ms NOTICE: Average time per call: 0.463909 ms The gain is small but consistent. Let me know if a more rigorous benchmark would be useful. Best regards, Stepan Neretin
Re: Accessing an invalid pointer in BufferManagerRelation structure
On Mon, Apr 14, 2025 at 1:10 PM Daniil Davydov <3daniss...@gmail.com> wrote: > Hi, > > On Wed, Mar 26, 2025 at 2:14 PM Stepan Neretin wrote: > > > > The first thing we both noticed is that the macro calls a function that > won't be available without an additional header. This seems a bit > inconvenient. > > Well, I rebased the patch onto the latest `master` > (b51f86e49a7f119004c0ce5d0be89cdf98309141) and noticed that we don't > need to include `rel.h` in `localbuf.c` directly anymore, because > `#include lmgr.h` was added in memutils.h > I guess it solves this issue. Please, see v3 patch. > > > I also have a question: is the logic correct that if the relation is > valid, we should fetch it rather than the other way around? Additionally, > is checking only the `rd_isvalid` flag sufficient, or should we also > consider the flag below? > > > > ``` > > bool rd_isvalid; /* relcache entry is valid */ > > > > I don't think that we should check any Relation's flags here. We are > checking `RelationIsValid((bmr).rel) ?` to decide whether > BufferManagerRelation was created via BMR_REL or BMR_SMGR. > If the `rel` field is not NULL, we can definitely say that BMR_REL was > used, so we should call RelationGetSmgr in order to access smgr. > > -- > Best regards, > Daniil Davydov > Hi, now looks good for me. -- Best regards, Stepan Neretin
Re: [PATCH] avoid double scanning in function byteain
On Mon, Jul 28, 2025 at 1:41 PM David G. Johnston < david.g.johns...@gmail.com> wrote: > On Sunday, July 27, 2025, Stepan Neretin wrote: >> >> One small thing: it seems the commit or diff with the final adjustments >> and test additions wasn't attached or linked in the thread. Could you >> please point me to the commit hash or reference? I’d love to take a look at >> the final version. >> > > > https://www.postgresql.org/message-id/e1ucrum-006yyh...@gemulon.postgresql.org > > The pgsql-committers list is searchable. All commits get sent there. > > https://www.postgresql.org/search/?m=1&q=&l=16&d=31&s=d > > David J. > Hi David, Yes, I'm aware of the pgsql-committers archive and I did check there — thank you for the reminder! However, I couldn’t find the patch or final diff in either the pgsql-committers message you linked or as an attachment in the original thread. Best regards, Stepan
Re: [PATCH] avoid double scanning in function byteain
On Sat, Jul 19, 2025 at 3:48 AM Tom Lane wrote: > I wrote: > > I'm inclined to accept 0001, reject 0002, and move on. This doesn't > > seem like an area that's worth a huge amount of discussion. > > Done that way. I made a couple more cosmetic changes and added > test cases for the double-backslash code path (which hadn't been > covered in byteaout either, I see now). > > BTW, in my hands the microbenchmark that Stepan suggested shows the > committed version to be about 40% faster than before for long input. > So apparently the StringInfo-ification suggested in 0002 gave back > just about all the performance gain from 0001. > > regards, tom lane > Hi Tom, Thanks a lot for reviewing and committing the change — much appreciated! I agree with your rationale regarding patch 0001 vs 0002. It makes sense to avoid the overhead of StringInfo in this context, especially given the measurable performance benefit from the simpler approach. One small thing: it seems the commit or diff with the final adjustments and test additions wasn't attached or linked in the thread. Could you please point me to the commit hash or reference? I’d love to take a look at the final version. Best regards, *Stepan Neretin*
Re: Fix bug with accessing to temporary tables of other sessions
On Mon, Apr 14, 2025 at 12:36 PM Daniil Davydov <3daniss...@gmail.com>
wrote:
> Hi,
>
> During previous commitfest this topic already has been discussed
> within the "Forbid to DROP temp tables of other sessions" thread [1].
> Unfortunately its name doesn't reflect the real problem, so I decided
> to start a new thread, as David G. Johnston advised.
>
> Here are the summary results of the discussion [1] :
> The superuser is only allowed to DROP temporary relations of other
> sessions. Other commands (like SELECT, INSERT, UPDATE, DELETE ...)
> must be forbidden to him. Error message for this case will look like
> this : `could not access temporary relations of other sessions`.
> For now, superuser still can specify such operations because of a bug
> in the code that mistakenly recognizes other session's temp table as
> permanent (I've covered this topic in more detail in [2]). Attached
> patch fixes this bug (targeted on
> b51f86e49a7f119004c0ce5d0be89cdf98309141).
>
> Opened issue:
> Not everyone liked the idea that table's persistence can be assigned
> to table during makeRangeVarXXX calls (inside gram.y).
> My opinion - `As far as "pg_temp_" prefix is reserved by the postgres
> kernel, we can definitely say that we have encountered a temporary
> table when we see this prefix.`
>
> I will be glad to hear your opinion.
>
> --
> Best regards,
> Daniil Davydov
>
> [1]
> https://www.postgresql.org/message-id/CAJDiXgj72Axj0d4ojKdRWG_rnkfs4uWY414NL%3D15sCvh7-9rwg%40mail.gmail.com
> [2]
> https://www.postgresql.org/message-id/CAJDiXgj%2B5UKLWSUT5605rJhuw438NmEKecvhFAF2nnrMsgGK3w%40mail.gmail.com
Hi Daniil,
Your patch for securing cross-session temp table access is a great
improvement. The RVR_OTHER_TEMP_OK flag elegantly handles the DROP case
while keeping the main restriction in place.
For schema name validation, an exact strcmp for "pg_temp" and proper
numeric parsing for "pg_temp_X" would be more precise than the current
prefix check. This would avoid any accidental matches to similarly named
schemas.
The error message could be adjusted to emphasize permissions, like
"permission denied for cross-session temp table access". This would make
the security intent clearer to users.
I noticed the Assert assumes myTempNamespace is always valid. While
correct, a brief comment explaining why this is safe would help future
maintainers. The relpersistence logic could also be centralized in one
place for consistency.
I've added an isolation test to verify the behavior when trying to access
another backend's temp tables. It confirms the restrictions work as
intended while allowing permitted operations.
Thanks for working on this important security enhancement!
Best regards,
Stepan Neretin
From da27bc190faab3853f6a2cc0748f1f5476215001 Mon Sep 17 00:00:00 2001
From: Daniil Davidov
Date: Mon, 14 Apr 2025 11:01:56 +0700
Subject: [PATCH v6 1/2] Fix accessing other sessions temp tables
---
src/backend/catalog/namespace.c | 56
src/backend/commands/tablecmds.c | 3 +-
src/backend/nodes/makefuncs.c| 6 +++-
src/backend/parser/gram.y| 11 ++-
src/include/catalog/namespace.h | 2 ++
5 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index d97d632a7ef..f407efd9447 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -499,28 +499,44 @@ RangeVarGetRelidExtended(const RangeVar *relation, LOCKMODE lockmode,
*/
if (relation->relpersistence == RELPERSISTENCE_TEMP)
{
- if (!OidIsValid(myTempNamespace))
-relId = InvalidOid; /* this probably can't happen? */
- else
- {
-if (relation->schemaname)
-{
- Oid namespaceId;
+ Oid namespaceId;
- namespaceId = LookupExplicitNamespace(relation->schemaname, missing_ok);
+ if (relation->schemaname)
+ {
+namespaceId = LookupExplicitNamespace(relation->schemaname, missing_ok);
+/*
+ * If the user has specified an existing temporary schema
+ * owned by another user.
+ */
+if (OidIsValid(namespaceId) && namespaceId != myTempNamespace)
+{
/*
- * For missing_ok, allow a non-existent schema name to
- * return InvalidOid.
+ * We don't allow users to access temp tables of other
+ * sessions except for the case of dropping tables.
*/
- if (namespaceId != myTempNamespace)
+ if (!(flags & RVR_OTHER_TEMP_OK))
ereport(ERROR,
-(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("temporary tables cannot specify a schema name")));
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("could not access temporary relations of other sessions")
Making WAL archiving faster — multi-file support and async ideas
Hi hackers,
We’ve been thinking about how to make WAL archiving faster.
This topic was previously discussed in [1], and we’ve taken a first step by
implementing the attached patch, which adds support for archiving multiple
WAL files in one go.
The idea is straightforward: instead of invoking the archive command or
callback once per WAL file, we allow passing a batch of files. The patch
introduces support for new placeholders:
-
%F – list of WAL file names
-
%P – list of their full paths
-
%N – number of files
Since PostgreSQL already reads multiple files into memory and caches them
before archiving, this change avoids repeated fork() calls and reduces
overhead in high-throughput setups.
Of course, there are trade-offs. After discussing with Andrey Borodin, we
noted that if even one file in the batch fails to archive, we currently
have to retry the whole batch. While it’s technically possible to return a
list of successfully archived files, that would complicate the API and
introduce messy edge cases.
So we’re also exploring a more flexible idea: an asynchronous archiver mode.
The idea is to have PostgreSQL write WAL file names (marked .ready) into a
FIFO or pipe, and let an archive process or library asynchronously consume
and archive them. It would send back confirmations (or failures) through
another pipe, allowing PostgreSQL to retry failed files as needed. This
could decouple archiving from the archiver loop and open the door to more
efficient and parallel implementations.
We’d appreciate feedback on both directions:
-
Do you think the idea in the attached patch — batching WAL files for
archiving — is viable? Is it something worth pursuing?
-
What do you think about the async archiver concept? Would it fit
PostgreSQL’s architecture and operational expectations?
Thanks,
Stepan Neretin
[1]
https://www.postgresql.org/message-id/flat/BC335D75-105B-403F-9473-976C8BBC32E3%40yandex-team.ru#d45caa9d1075734567164f73371baf00
From 08104dcdd3295a48827e1d58c4c2382620267f5d Mon Sep 17 00:00:00 2001
From: Stepan Neretin
Date: Mon, 21 Jul 2025 14:13:51 +0700
Subject: [PATCH] Add support for multi-file archiving in archive modules
This patch introduces optional support for archiving multiple WAL files
at once via a new archive_files_cb callback in archive modules.
A new GUC archive_multi enables this behavior.
The shell and basic archive modules have been updated to support the new callback.
---
contrib/basic_archive/basic_archive.c| 22 ++
src/backend/archive/shell_archive.c | 159
src/backend/postmaster/pgarch.c | 300 +++
src/backend/utils/misc/guc_tables.c | 9 +
src/include/archive/archive_module.h | 18 ++
src/test/perl/PostgreSQL/Test/Cluster.pm | 47 ++--
src/test/recovery/t/002_archiving.pl | 1 +
7 files changed, 396 insertions(+), 160 deletions(-)
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 4a8b8c7ac29..0f009e510ec 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -46,6 +46,7 @@ static char *archive_directory = NULL;
static bool basic_archive_configured(ArchiveModuleState *state);
static bool basic_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static bool basic_archive_files(ArchiveModuleState *state, char **files, char **paths, int nfiles);
static bool check_archive_directory(char **newval, void **extra, GucSource source);
static bool compare_files(const char *file1, const char *file2);
@@ -53,6 +54,7 @@ static const ArchiveModuleCallbacks basic_archive_callbacks = {
.startup_cb = NULL,
.check_configured_cb = basic_archive_configured,
.archive_file_cb = basic_archive_file,
+ .archive_files_cb = basic_archive_files,
.shutdown_cb = NULL
};
@@ -229,6 +231,26 @@ basic_archive_file(ArchiveModuleState *state, const char *file, const char *path
return true;
}
+/*
++ * Archive multiple files.
++ */
+static bool
+basic_archive_files(ArchiveModuleState *state, char **files, char **paths, int nfiles)
+{
+ bool result = true;
+
+ for (int i = 0; i < nfiles; i++)
+ {
+ if (!basic_archive_file(state, files[i], paths[i]))
+ {
+ ereport(WARNING, (errmsg("failed to archive file \"%s\"", files[i])));
+ result = false;
+ }
+ }
+
+ return result;
+}
+
/*
* compare_files
*
diff --git a/src/backend/archive/shell_archive.c b/src/backend/archive/shell_archive.c
index 828723afe47..9e8bf95c6c4 100644
--- a/src/backend/archive/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -27,12 +27,17 @@ static bool shell_archive_configured(ArchiveModuleState *state);
static bool shell_archive_file(ArchiveModuleState *state,
const char *file,
const char *path);
+static bool shell_archive_files(ArchiveModuleState *state,
+ char **files,
+ char **paths,
+ in
Re: Assertion failure in pgbench
On Thu, Jul 31, 2025 at 9:03 AM Tatsuo Ishii wrote:
> >> Hi,
> >>
> >> I encountered the following assertion failure in pgbench on the current
> master:
> >>
> >> Assertion failed: (res == ((void*)0)), function discardUntilSync,
> >> file pgbench.c, line 3515.
> >> Abort trap: 6
> >>
> >>
> >> This can be reliably reproduced with the following steps:
> >>
> >>
> >> $ psql -c "ALTER SYSTEM SET default_transaction_isolation TO
> 'serializable'"
> >>
> >> $ psql -c "SELECT pg_reload_conf()"
> >>
> >> $ pgbench -i
> >>
> >> $ cat test.sql
> >> \set aid random(1, 10 * :scale)
> >> \set bid random(1, 1 * :scale)
> >> \set tid random(1, 10 * :scale)
> >> \set delta random(-5000, 5000)
> >> \startpipeline
> >> BEGIN;
> >> UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid =
> :aid;
> >> SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
> >> UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid =
> :tid;
> >> UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid =
> :bid;
> >> INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES
> >> (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
> >> END;
> >> \endpipeline
> >>
> >> $ pgbench -f test.sql -c 10 -j 10 -T 60 -M extended
> >>
> >>
> >> Even without a custom script, shutting down the server with
> >> immediate mode while running "pgbench -c 10 -j 10 -T 60" could
> >> trigger the same assertion, though not always reliably.
> >>
> >>
> >> /* receive PGRES_PIPELINE_SYNC and null following it */
> >> for (;;)
> >> {
> >> PGresult *res = PQgetResult(st->con);
> >>
> >> if (PQresultStatus(res) == PGRES_PIPELINE_SYNC)
> >> {
> >> PQclear(res);
> >> res = PQgetResult(st->con);
> >> Assert(res == NULL);
> >> break;
> >> }
> >> PQclear(res);
> >> }
> >>
> >> The failure occurs in this code. This code assumes that
> PGRES_PIPELINE_SYNC
> >> is always followed by a NULL. However, it seems that another
> >> PGRES_PIPELINE_SYNC can appear consecutively, which violates that
> assumption
> >> and causes the assertion to fail. Thought?
> >
> > Yes. When an error occurs and an error response message returned from
> > backend, pgbench will send one more sync message, then sends ROLLBACK
> > if necessary. I think the code above should be changed to call
> > PQgetResult repeatably until it returns NULL.
>
> Correction. That would not be a proper fix. Just removing inner
> PQgetResult and the Assert is enough?
>
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS K.K.
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp
>
>
Hi, Tatsuo.
Do you understand why there is an assertion error in the immediate shutdown
case?
Best Regards,
Stepan Neretin
Re: [PATCH] avoid double scanning in function byteain
On Fri, May 9, 2025 at 5:37 PM Stepan Neretin wrote: > > > On Fri, May 9, 2025 at 5:24 PM Stepan Neretin wrote: > >> >> >> On Wed, Mar 26, 2025 at 9:39 PM Steven Niu wrote: >> >>> >>> 在 2025/3/26 16:37, Kirill Reshke 写道: >>> > On Wed, 26 Mar 2025 at 12:17, Steven Niu wrote: >>> >> >>> >> Hi, >>> > >>> > Hi! >>> > >>> >> This double scanning can be inefficient, especially for large inputs. >>> >> So I optimized the function to eliminate the need for two scans, >>> >> while preserving correctness and efficiency. >>> > >>> > While the argument that processing input once not twice is fast is >>> > generally true, may we have some simple bench here just to have an >>> > idea how valuable this patch is? >>> > >>> > >>> > Patch: >>> > >>> > >>> >> + /* Handle traditional escaped style in a single pass */ >>> >> + input_len = strlen(inputText); >>> >> + result = palloc(input_len + VARHDRSZ); /* Allocate max possible >>> size */ >>> >> rp = VARDATA(result); >>> >> + tp = inputText; >>> >> + >>> >> while (*tp != '\0') >>> > >>> > >>> > Isn't this `strlen` O(n) + `while` O(n)? Where is the speed up? >>> > >>> > >>> > >>> > [0] >>> https://github.com/bminor/glibc/blob/master/string/strlen.c#L43-L45 >>> > >>> >>> >>> >>> Hi, Kirill, >>> >>> Your deep insight suprised me! >>> >>> Yes, you are correct that strlen() actually performed a loop operation. >>> So maybe the performance difference is not so obvious. >>> >>> However, there are some other reasons that drive me to make this change. >>> >>> 1. The author of original code left comment: "BUGS: The input is scanned >>> twice." . >>> You can find this line of code in my patch. This indicates a left work >>> to be done. >>> >>> 2. If I were the author of this function, I would not be satisfied with >>> myself that I used two loops to do something which actually can be done >>> with one loop. >>> I prefer to choose a way that would not add more burden to readers. >>> >>> 3. The while (*tp != '\0') loop has some unnecessary codes and I made >>> some change. >>> >>> Thanks, >>> Steven >>> >>> >>> >> >> >> Hi hackers, >> >> This is a revised version (v2) of the patch that optimizes the >> `byteain()` function. >> >> The original implementation handled escaped input by scanning the string >> twice — first to determine the output size and again to fill in the bytea. >> This patch eliminates the double scan by using a single-pass approach with >> `StringInfo`, simplifying the logic and improving maintainability. >> >> Changes since v1 (originally by Steven Niu): >> - Use `StringInfo` instead of manual memory allocation. >> - Remove redundant code and improve readability. >> - Add regression tests for both hex and escaped formats. >> >> This version addresses performance and clarity while ensuring >> compatibility with existing behavior. The patch also reflects discussion on >> the original version, including feedback from Kirill Reshke. >> >> Looking forward to your review and comments. >> >> Best regards, >> Stepan Neretin >> > > > Hi, > > I noticed that the previous version of the patch was authored with an > incorrect email address due to a misconfigured git config. > > I've corrected the author information in this v2 and made sure it's > consistent with my usual contributor identity. No other changes were > introduced apart from that and the updates discussed earlier. > > Sorry for the confusion, and thanks for your understanding. > > Best regards, > > Stepan Neretin > Hi, Sorry for the noise — I'm resending the patch because I noticed a compiler warning related to mixed declarations and code, which I’ve now fixed. Apologies for the oversight in the previous submission. Regards, Stepan Neretin From b589d728b54de071b8d4383a3a51de5f7c2e2293 Mon Sep 17 00:00:00 2001 From: Steven Niu Date: Wed, 26 Mar 2025 14:43:43 +0800 Subject: [PATCH v2 1/2] Optimize function byteain() to avoid double scanning Optimized the function to eliminate the need fo
Re: [PATCH] avoid double scanning in function byteain
On Wed, Mar 26, 2025 at 9:39 PM Steven Niu wrote:
>
> 在 2025/3/26 16:37, Kirill Reshke 写道:
> > On Wed, 26 Mar 2025 at 12:17, Steven Niu wrote:
> >>
> >> Hi,
> >
> > Hi!
> >
> >> This double scanning can be inefficient, especially for large inputs.
> >> So I optimized the function to eliminate the need for two scans,
> >> while preserving correctness and efficiency.
> >
> > While the argument that processing input once not twice is fast is
> > generally true, may we have some simple bench here just to have an
> > idea how valuable this patch is?
> >
> >
> > Patch:
> >
> >
> >> + /* Handle traditional escaped style in a single pass */
> >> + input_len = strlen(inputText);
> >> + result = palloc(input_len + VARHDRSZ); /* Allocate max possible size
> */
> >> rp = VARDATA(result);
> >> + tp = inputText;
> >> +
> >> while (*tp != '\0')
> >
> >
> > Isn't this `strlen` O(n) + `while` O(n)? Where is the speed up?
> >
> >
> >
> > [0] https://github.com/bminor/glibc/blob/master/string/strlen.c#L43-L45
> >
>
>
>
> Hi, Kirill,
>
> Your deep insight suprised me!
>
> Yes, you are correct that strlen() actually performed a loop operation.
> So maybe the performance difference is not so obvious.
>
> However, there are some other reasons that drive me to make this change.
>
> 1. The author of original code left comment: "BUGS: The input is scanned
> twice." .
> You can find this line of code in my patch. This indicates a left work
> to be done.
>
> 2. If I were the author of this function, I would not be satisfied with
> myself that I used two loops to do something which actually can be done
> with one loop.
> I prefer to choose a way that would not add more burden to readers.
>
> 3. The while (*tp != '\0') loop has some unnecessary codes and I made
> some change.
>
> Thanks,
> Steven
>
>
>
Hi hackers,
This is a revised version (v2) of the patch that optimizes the `byteain()`
function.
The original implementation handled escaped input by scanning the string
twice — first to determine the output size and again to fill in the bytea.
This patch eliminates the double scan by using a single-pass approach with
`StringInfo`, simplifying the logic and improving maintainability.
Changes since v1 (originally by Steven Niu):
- Use `StringInfo` instead of manual memory allocation.
- Remove redundant code and improve readability.
- Add regression tests for both hex and escaped formats.
This version addresses performance and clarity while ensuring compatibility
with existing behavior. The patch also reflects discussion on the original
version, including feedback from Kirill Reshke.
Looking forward to your review and comments.
Best regards,
Stepan Neretin
From b589d728b54de071b8d4383a3a51de5f7c2e2293 Mon Sep 17 00:00:00 2001
From: Steven Niu
Date: Wed, 26 Mar 2025 14:43:43 +0800
Subject: [PATCH v2 1/2] Optimize function byteain() to avoid double scanning
Optimized the function to eliminate the need for two scans,
while preserving correctness and efficiency.
Author: Steven Niu
---
src/backend/utils/adt/varlena.c | 66 +++--
1 file changed, 22 insertions(+), 44 deletions(-)
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 3e4d5568bde..f1f1efba053 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -291,7 +291,6 @@ text_to_cstring_buffer(const text *src, char *dst, size_t dst_len)
* ereport(ERROR, ...) if bad form.
*
* BUGS:
- *The input is scanned twice.
*The error checking of input is minimal.
*/
Datum
@@ -302,6 +301,7 @@ byteain(PG_FUNCTION_ARGS)
char *tp;
char *rp;
int bc;
+ size_t input_len;
bytea *result;
/* Recognize hex input */
@@ -318,45 +318,28 @@ byteain(PG_FUNCTION_ARGS)
PG_RETURN_BYTEA_P(result);
}
- /* Else, it's the traditional escaped style */
- for (bc = 0, tp = inputText; *tp != '\0'; bc++)
- {
- if (tp[0] != '\\')
- tp++;
- else if ((tp[0] == '\\') &&
- (tp[1] >= '0' && tp[1] <= '3') &&
- (tp[2] >= '0' && tp[2] <= '7') &&
- (tp[3] >= '0' && tp[3] <= '7'))
- tp += 4;
- else if ((tp[0] == '\\') &&
- (tp[1] == '\\'))
- tp += 2;
- else
- {
- /*
- * one backslash, not followed by another or ### valid octal
- */
- ereturn(escontext, (Datum) 0,
- (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
- errmsg("invalid input syntax for type %s", "bytea"))
Re: Fixes a trivial bug in dumped parse/query/plan trees
On Mon, Aug 25, 2025 at 7:55 AM Chao Li wrote: > Hi Hackers, > > This patch fixes a trivial bug where an extra whitespace was added > when dumping an array, for example: > > ``` > :sort.numCols 2 > :sort.sortColIdx ( 1 4) > :sort.sortOperators ( 97 1754) > :sort.collations ( 0 0) > :sort.nullsFirst ( false false) > ``` > > The unnecessary whitespace is now removed. > > Regard regards, > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > > > > Hi Chao, I was able to reproduce the problem using a small test: ```sql SET debug_print_parse = on; CREATE TABLE test_sort_example ( id SERIAL PRIMARY KEY, col1 INT, col2 TEXT, col3 TEXT ); INSERT INTO test_sort_example (col1, col2, col3) VALUES (1, 'apple', 'zebra'), (2, 'banana', 'yak'), (3, 'cherry', 'xylophone'), (4, 'date', 'wolf'); SELECT * FROM test_sort_example ORDER BY col2 ASC, col3 ASC; ``` The extra whitespace in the array dump (e.g., ``` :sort.numCols 2 :sort.sortColIdx ( 1 4) :sort.sortOperators ( 97 1754) :sort.collations ( 0 0) :sort.nullsFirst ( false false) ``` ) was indeed present before the patch. After applying your patch, the issue is fixed, and the array is now dumped correctly without extra spaces. The patch looks good to me. Regarding testing: for this case, a Perl test might actually be more suitable than SQL tests, because OIDs can change between runs and Perl tests allow for better control and normalization of such values. Best regards, Stepan Neretin
