Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN
>> > or the case when the last usage fit in memory but an earlier >> > usage spilled to disk. >> >> In my understanding once tuplestore changes the storage type to disk, >> it never returns to the memory storage type in terms of >> tuplestore_get_stats. i.e. once state->usedDisk is set to true, it >> never goes back to false. So the test case is not necessary. >> David, am I correct? > > I understand that. I am requesting a testcase to test that same logic. Maybe something like this? In the example below there are 2 partitions. the first one has 1998 rows and the second one has 2 rows. Assuming that work_mem is 64kB, the first one does not fit the memory and spills to disk. The second partition fits memory. However as state->usedDisk remains true, explain shows "Storage: Disk". test=# explain (analyze,costs off) select sum(n) over(partition by m) from (SELECT n < 3 as m, n from generate_series(1,2000) a(n)); n QUERY PLAN - WindowAgg (actual time=1.958..473328.589 rows=2000 loops=1) Storage: Disk Maximum Storage: 65kB -> Sort (actual time=1.008..1.277 rows=2000 loops=1) Sort Key: ((a.n < 3)) Sort Method: external merge Disk: 48kB -> Function Scan on generate_series a (actual time=0.300..0.633 rows=2 000 loops=1) Planning Time: 0.069 ms Execution Time: 474515.476 ms (8 rows) Best reagards, -- Tatsuo Ishii SRA OSS K.K. English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Mutable foreign key constraints
On 2024-09-12 Th 5:33 PM, Tom Lane wrote: I happened to notice that Postgres will let you do regression=# create table foo (id timestamp primary key); CREATE TABLE regression=# create table bar (ts timestamptz references foo); CREATE TABLE This strikes me as a pretty bad idea, because whether a particular timestamp is equal to a particular timestamptz depends on your timezone setting. Thus the constraint could appear to be violated after a timezone change. I'm inclined to propose rejecting FK constraints if the comparison operator is not immutable. Among the built-in opclasses, the only instances of non-immutable btree equality operators are regression=# select amopopr::regoperator from pg_amop join pg_operator o on o.oid = amopopr join pg_proc p on p.oid = oprcode where amopmethod=403 and amopstrategy=3 and provolatile != 'i'; amopopr - =(date,timestamp with time zone) =(timestamp without time zone,timestamp with time zone) =(timestamp with time zone,date) =(timestamp with time zone,timestamp without time zone) (4 rows) A possible objection is that if anybody has such a setup and hasn't noticed a problem because they never change their timezone setting, they might not appreciate us breaking it. So I certainly wouldn't propose back-patching this. But maybe we should add it as a foot-gun defense going forward. Thoughts? Isn't there an upgrade hazard here? People won't thank us if they can't now upgrade their clusters. If we can get around that then +1. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Use streaming read API in ANALYZE
On Fri, Sep 13, 2024 at 10:10 AM Mats Kindahl wrote:
> On Tue, Sep 10, 2024 at 6:04 AM Thomas Munro
> wrote:
>
>> On Tue, Sep 10, 2024 at 10:27 AM Thomas Munro
>> wrote:
>> > Mats, what do you think about
>> > this? (I haven't tried to preserve the prefetching behaviour, which
>> > probably didn't actually too work for you in v16 anyway at a guess,
>> > I'm just looking for the absolute simplest thing we can do to resolve
>> > this API mismatch.) TimeScale could then continue to use its v16
>> > coding to handle the two-relations-in-a-trenchcoat problem, and we
>> > could continue discussing how to make v18 better.
>>
>> . o O { Spitballing here: if we add that tiny function I showed to get
>> you unstuck for v17, then later in v18, if we add a multi-relation
>> ReadStream constructor/callback (I have a patch somewhere, I want to
>> propose that as it is needed for streaming recovery), you could
>> construct a new ReadSteam of your own that is daisy-chained from that
>> one. You could keep using your N + M block numbering scheme if you
>> want to, and the callback of the new stream could decode the block
>> numbers and redirect to the appropriate relation + real block number.
>>
>
> I think it is good to make as small changes as possible to the RC, so
> agree with this approach. Looking at the patch. I think it will work, but
> I'll do some experimentation with the patch.
>
> Just asking, is there any particular reason why you do not want to *add*
> new functions for opaque objects inside a major release? After all, that
> was the reason they were opaque from the beginning and extending with new
> functions would not break any existing code, not even from the ABI
> perspective.
>
>
>> That way you'd get I/O concurrency for both relations (for now just
>> read-ahead advice, but see Andres's AIO v2 thread). That'd
>> essentially be a more supported version of the 'access the struct
>> internals' idea (or at least my understanding of what you had in
>> mind), through daisy-chained streams. A little weird maybe, and maybe
>> the redesign work will result in something completely
>> different/better... just a thought... }
>>
>
> I'll take a look at the thread. I really think the ReadStream abstraction
> is a good step in the right direction.
> --
> Best wishes,
> Mats Kindahl, Timescale
>
Hi Thomas,
I used the combination of your patch and making the computation of
vacattrstats for a relation available through the API and managed to
implement something that I think does the right thing. (I just sampled a
few different statistics to check if they seem reasonable, like most common
vals and most common freqs.) See attached patch.
I need the vacattrstats to set up the two streams for the internal
relations. I can just re-implement them in the same way as is already done,
but this seems like a small change that avoids unnecessary code
duplication.
--
Best wishes,
Mats Kindahl, Timescale
From 8acb707cc859f1570046919295817e27f0d8b4f6 Mon Sep 17 00:00:00 2001
From: Mats Kindahl
Date: Sat, 14 Sep 2024 14:03:35 +0200
Subject: [PATCH] Support extensions wanting to do more advanced analyze
To support extensions that want to do more advanced analyzis
implementations, this commit adds two function:
`analuze_compute_vacattrstats` to compute vacuum attribute stats that
can be used to compute the number of target rows to analyze.
`read_stream_next_block` which allow an extension to just use the
provided analyze stream to sample blocks.
---
src/backend/commands/analyze.c| 120 ++
src/backend/storage/aio/read_stream.c | 14 +++
src/include/commands/vacuum.h | 2 +
src/include/storage/read_stream.h | 2 +
4 files changed, 85 insertions(+), 53 deletions(-)
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index c590a2adc35..8a402ad15c6 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -269,6 +269,72 @@ analyze_rel(Oid relid, RangeVar *relation,
pgstat_progress_end_command();
}
+/*
+ * Determine which columns to analyze
+ *
+ * Note that system attributes are never analyzed, so we just reject them
+ * at the lookup stage. We also reject duplicate column mentions. (We
+ * could alternatively ignore duplicates, but analyzing a column twice
+ * won't work; we'd end up making a conflicting update in pg_statistic.)
+ */
+int
+analyze_compute_vacattrstats(Relation onerel, List *va_cols, VacAttrStats ***vacattrstats_out)
+{
+ int tcnt,
+i,
+attr_cnt;
+ VacAttrStats **vacattrstats;
+
+ if (va_cols != NIL)
+ {
+ Bitmapset *unique_cols = NULL;
+ ListCell *le;
+
+ vacattrstats = (VacAttrStats **) palloc(list_length(va_cols) *
+sizeof(VacAttrStats *));
+ tcnt = 0;
+ foreach(le, va_cols)
+ {
+ char *col = strVal(lfirst(le));
+
+ i = attnameAttNum(onerel, col, false);
+ if (i == InvalidAttrNumber)
+ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg(
Re: Adding skip scan (including MDAM style range skip scan) to nbtree
On 9/12/24 16:49, Matthias van de Meent wrote: > On Mon, 9 Sept 2024 at 21:55, Peter Geoghegan wrote: >> > ... > > The fix in 0001 is relatively simple: we stop backends from waiting > for a concurrent backend to resolve the NEED_PRIMSCAN condition, and > instead move our local state machine so that we'll hit _bt_first > ourselves, so that we may be able to start the next primitive scan. > Also attached is 0002, which adds tracking of responsible backends to > parallel btree scans, thus allowing us to assert we're never waiting > for our own process to move the state forward. I found this patch > helpful while working on solving this issue, even if it wouldn't have > found the bug as reported. > No opinion on the analysis / coding, but per my testing the fix indeed addresses the issue. The script reliably got stuck within a minute, now it's running for ~1h just fine. It also checks results and that seems fine too, so that seems fine too. regards -- Tomas Vondra
Re: Psql meta-command conninfo+
Hi Jim, On Fri, Sep 13, 2024 at 4:27 PM Jim Jones wrote: > I just noticed that messages' order has been slightly changed. The > message "You are connected to database "postgres" as user "hunaid" via > socket in "/tmp" at port "5430" used to be printed after the table, and > now it is printed before. > > $ /usr/local/postgres-dev/bin/psql -x "\ > hostaddr=0 > user=jim dbname=postgres > port=54322" -c "\conninfo+" > > You are connected to database "postgres" as user "jim" on host "0" > (address "0.0.0.0") at port "54322". > Connection Information > -[ RECORD 1 ]+ > Protocol Version | 3 > SSL Connection | no > GSSAPI Authenticated | no > Client Encoding | UTF8 > Server Encoding | UTF8 > Session User | jim > Backend PID | 2419033 > > It is IMHO a little strange because the "SSL connection" info keeps > being printed in the end. I would personally prefer if they're printed > together --- preferably after the table. But I'm not sure if there's any > convention for that. > I agree that both messages should be printed together. IMO the message "You are connected to database..." should be printed at the top, no? Because it shows important info that the user may be interested to see first. Then we can combine the ssl message. postgres=# \x Expanded display is on. postgres=# \conninfo+ You are connected to database "postgres" as user "hunaid" on host "localhost" (address "127.0.0.1") at port "5430". SSL connection (protocol: TLSv1.3, cipher: TLS_AES_256_GCM_SHA384, compression: off, ALPN: postgresql) Connection Information -[ RECORD 1 ]+--- Protocol Version | 3 SSL Connection | yes GSSAPI Authenticated | no Client Encoding | UTF8 Server Encoding | UTF8 Session User | hunaid Backend PID | 109092 > Also, there are a few compilation warnings regarding const qualifiers: > Noted. I will fix them in the next patch. Regards, Hunaid Sohail
Regression tests fail with tzdata 2024b
Building --with-system-tzdata and the latest tzdata 2024b fails the
regression tests for me (see attached .diffs). This seems to be because
of [1], which changed the way "PST8PDT" is handled. This is the timezone
that the regression tests are run with.
From 2024b on, "PST8PDT" is the same as "America/Los_Angeles", so by
changing the regression tests to use the latter as the default, we're
getting consistent output on at least 2024a and 2024b.
Patch attached.
Best,
Wolfgang
[1]:
https://github.com/eggert/tz/commit/a0b09c0230089252acf2eb0f1ba922e99f7f4a03diff -U3 /build/source/src/test/regress/expected/date.out
/build/source/build/testrun/regress/regress/results/date.out
--- /build/source/src/test/regress/expected/date.out1970-01-01
00:00:01.0 +
+++ /build/source/build/testrun/regress/regress/results/date.out
2024-09-13 17:58:09.142429792 +
@@ -1295,7 +1295,7 @@
SELECT DATE_TRUNC('MILLENNIUM', DATE '1970-03-20'); -- 1001-01-01
date_trunc
--
- Thu Jan 01 00:00:00 1001 PST
+ Thu Jan 01 00:00:00 1001 LMT
(1 row)
SELECT DATE_TRUNC('CENTURY', TIMESTAMP '1970-03-20 04:30:00.0'); -- 1901
@@ -1319,13 +1319,13 @@
SELECT DATE_TRUNC('CENTURY', DATE '0002-02-04'); -- 0001-01-01
date_trunc
--
- Mon Jan 01 00:00:00 0001 PST
+ Mon Jan 01 00:00:00 0001 LMT
(1 row)
SELECT DATE_TRUNC('CENTURY', DATE '0055-08-10 BC'); -- 0100-01-01 BC
date_trunc
-
- Tue Jan 01 00:00:00 0100 PST BC
+ Tue Jan 01 00:00:00 0100 LMT BC
(1 row)
SELECT DATE_TRUNC('DECADE', DATE '1993-12-25'); -- 1990-01-01
@@ -1337,13 +1337,13 @@
SELECT DATE_TRUNC('DECADE', DATE '0004-12-25'); -- 0001-01-01 BC
date_trunc
-
- Sat Jan 01 00:00:00 0001 PST BC
+ Sat Jan 01 00:00:00 0001 LMT BC
(1 row)
SELECT DATE_TRUNC('DECADE', DATE '0002-12-31 BC'); -- 0011-01-01 BC
date_trunc
-
- Mon Jan 01 00:00:00 0011 PST BC
+ Mon Jan 01 00:00:00 0011 LMT BC
(1 row)
--
diff -U3 /build/source/src/test/regress/expected/timestamptz.out
/build/source/build/testrun/regress/regress/results/timestamptz.out
--- /build/source/src/test/regress/expected/timestamptz.out 1970-01-01
00:00:01.0 +
+++ /build/source/build/testrun/regress/regress/results/timestamptz.out
2024-09-13 17:58:09.479104285 +
@@ -330,12 +330,12 @@
Fri Feb 14 17:32:01 1997 PST
Sat Feb 15 17:32:01 1997 PST
Sun Feb 16 17:32:01 1997 PST
- Tue Feb 16 17:32:01 0097 PST BC
- Sat Feb 16 17:32:01 0097 PST
- Thu Feb 16 17:32:01 0597 PST
- Tue Feb 16 17:32:01 1097 PST
- Sat Feb 16 17:32:01 1697 PST
- Thu Feb 16 17:32:01 1797 PST
+ Tue Feb 16 17:32:01 0097 LMT BC
+ Sat Feb 16 17:32:01 0097 LMT
+ Thu Feb 16 17:32:01 0597 LMT
+ Tue Feb 16 17:32:01 1097 LMT
+ Sat Feb 16 17:32:01 1697 LMT
+ Thu Feb 16 17:32:01 1797 LMT
Tue Feb 16 17:32:01 1897 PST
Sun Feb 16 17:32:01 1997 PST
Sat Feb 16 17:32:01 2097 PST
@@ -359,19 +359,19 @@
SELECT '4714-11-24 00:00:00+00 BC'::timestamptz;
timestamptz
-
- Sun Nov 23 16:00:00 4714 PST BC
+ Sun Nov 23 16:07:02 4714 LMT BC
(1 row)
SELECT '4714-11-23 16:00:00-08 BC'::timestamptz;
timestamptz
-
- Sun Nov 23 16:00:00 4714 PST BC
+ Sun Nov 23 16:07:02 4714 LMT BC
(1 row)
SELECT 'Sun Nov 23 16:00:00 4714 PST BC'::timestamptz;
timestamptz
-
- Sun Nov 23 16:00:00 4714 PST BC
+ Sun Nov 23 16:07:02 4714 LMT BC
(1 row)
SELECT '4714-11-23 23:59:59+00 BC'::timestamptz; -- out of range
@@ -461,12 +461,12 @@
-
-infinity
Wed Dec 31 16:00:00 1969 PST
- Tue Feb 16 17:32:01 0097 PST BC
- Sat Feb 16 17:32:01 0097 PST
- Thu Feb 16 17:32:01 0597 PST
- Tue Feb 16 17:32:01 1097 PST
- Sat Feb 16 17:32:01 1697 PST
- Thu Feb 16 17:32:01 1797 PST
+ Tue Feb 16 17:32:01 0097 LMT BC
+ Sat Feb 16 17:32:01 0097 LMT
+ Thu Feb 16 17:32:01 0597 LMT
+ Tue Feb 16 17:32:01 1097 LMT
+ Sat Feb 16 17:32:01 1697 LMT
+ Thu Feb 16 17:32:01 1797 LMT
Tue Feb 16 17:32:01 1897 PST
Wed Feb 28 17:32:01 1996 PST
Thu Feb 29 17:32:01 1996 PST
@@ -529,12 +529,12 @@
Fri Feb 14 17:32:01 1997 PST
Sat Feb 15 17:32:01 1997 PST
Sun Feb 16 17:32:01 1997 PST
- Tue Feb 16 17:32:01 0097 PST BC
- Sat Feb 16 17:32:01 0097 PST
- Thu Feb 16 17:32:01 0597 PST
- Tue Feb 16 17:32:01 1097 PST
- Sat Feb 16 17:32:01 1697 PST
- Thu Feb 16 17:32:01 1797 PST
+ Tue Feb 16 17:32:01 0097 LMT BC
+ Sat Feb 16 17:32:01 0097 LMT
+ Thu Feb 16 17:32:01 0597 LMT
+ Tue Feb 16 17:32:01 1097 LMT
+ Sat Feb 16 17:32:01 1697 LMT
+ Thu Feb 16 17:32:01 1797 LMT
Tue Feb 16 17:32:01 1897 PST
Sun Feb 16 17:32:01 1997 PST
Sat Feb 16 17:32:01 2097 PST
@@ -561,12 +561,12 @@
-infinity
Wed Dec 31 16:00:0
Re: First draft of PG 17 release notes
On Wed, Sep 11, 2024 at 07:50:40PM +0200, Álvaro Herrera wrote: > There's also a bunch of items on EXPLAIN, which could perhaps be grouped > in a single item with sub-paras for each individual change; I'd also > move it to the bottom of E.1.3.2. Oh, I hadn't noticed I have five EXPLAIN items --- that is enough to make a new section, done at: https://momjian.us/pgsql_docs/release-17.html#RELEASE-17-EXPLAIN I don't think I can combine the EXPLAIN items without making them too complex. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com When a patient asks the doctor, "Am I going to die?", he means "Am I going to die soon?"
Re: Detailed release notes
On Fri, Sep 13, 2024 at 05:54:31PM -0400, Bruce Momjian wrote: > On Fri, Sep 13, 2024 at 06:41:30PM -0300, Marcos Pegoraro wrote: > > Em sex., 13 de set. de 2024 às 13:39, Bruce Momjian > > escreveu: > > > > I changed the patch to use the section symbol "§" instead of showing > > the hashes. The hashes seemed too detailed. Does anyone see a better > > symbol to use from here? > > > > > > Robert and others liked commit hashes because you can do "git show > > $COMMITID" > > We have to consider the small percentage of people who will want to do > that vs the number of people reading the release notes. I think it is > reasonable to require someone to click on the link to see the hash. Thinking some more, I think that displaying the hashes would give the average reader the impression that the release notes are too complex for them to understand. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com When a patient asks the doctor, "Am I going to die?", he means "Am I going to die soon?"
Re: Robocopy might be not robust enough for never-ending testing on Windows
On 2024-09-14 Sa 9:00 AM, Alexander Lakhin wrote:
Hello hackers,
While trying to reproduce inexplicable drongo failures (e. g., [1])
seemingly related to BackgroundPsql, I stumbled upon close, but not
the same issue. After many (6-8 thousands) iterations of the
015_stream.pl TAP test, psql failed to start with a
STATUS_DLL_INIT_FAILED
error, and a corresponding Windows popup preventing a test exit (see ss-1
in the archive attached).
Upon reaching that state of the system, following test runs fail with one
or another error related to memory (not only with psql, but also with the
server processes):
testrun/subscription_13/015_stream/log/015_stream_publisher.log:2024-09-11
20:01:51.777 PDT [8812] LOG: server process (PID 11532) was
terminated by exception 0xC0FD
testrun/subscription_14/015_stream/log/015_stream_subscriber.log:2024-09-11
20:01:19.806 PDT [2036] LOG: server process (PID 10548) was
terminated by exception 0xC0FD
testrun/subscription_16/015_stream/log/015_stream_publisher.log:2024-09-11
19:59:41.513 PDT [9128] LOG: server process (PID 14476) was
terminated by exception 0xC409
testrun/subscription_19/015_stream/log/015_stream_publisher.log:2024-09-11
20:03:27.801 PDT [10156] LOG: server process (PID 2236) was
terminated by exception 0xC409
testrun/subscription_20/015_stream/log/015_stream_publisher.log:2024-09-11
19:59:41.359 PDT [10656] LOG: server process (PID 14712) was
terminated by exception 0xC12D
testrun/subscription_3/015_stream/log/015_stream_publisher.log:2024-09-11
20:02:23.815 PDT [13704] LOG: server process (PID 13992) was
terminated by exception 0xC0FD
testrun/subscription_9/015_stream/log/015_stream_publisher.log:2024-09-11
19:59:41.360 PDT [9760] LOG: server process (PID 11608) was
terminated by exception 0xC142
...
When tests fail, I see Commit Charge reaching 100% (see ss-2 in the
attachment), while Physical Memory isn't all used up. To get OS to a
working state, I had to reboot it — killing processes, logoff/logon
didn't
help.
I reproduced this issue again, investigated it and found out that it is
caused by robocopy (used by PostgreSQL::Test::Cluster->init), which is
leaking kernel objects, namely "Event objects" within Non-Paged pool on
each run.
This can be seen with Kernel Pool Monitor, or just with this simple PS
script:
for ($i = 1; $i -le 100; $i++)
{
echo "iteration $i"
rm -r c:\temp\target
robocopy.exe /E /NJH /NFL /NDL /NP c:\temp\initdb-template c:\temp\target
Get-WmiObject -Class Win32_PerfRawData_PerfOS_Memory | %
PoolNonpagedBytes
}
It shows to me:
iteration 1
Total Copied Skipped Mismatch FAILED Extras
Dirs : 27 27 0 0 0 0
Files : 968 968 0 0 0 0
Bytes : 38.29 m 38.29 m 0 0 0 0
Times : 0:00:00 0:00:00 0:00:00 0:00:00
...
1226063872
...
iteration 100
Total Copied Skipped Mismatch FAILED Extras
Dirs : 27 27 0 0 0 0
Files : 968 968 0 0 0 0
Bytes : 38.29 m 38.29 m 0 0 0 0
Times : 0:00:00 0:00:00 0:00:00 0:00:00
...
1245220864
(That is, 0.1-0.2 MB leaks per one robocopy run.)
I observed this on Windows 10 (Version 10.0.19045.4780), with all updates
installed, but not on Windows Server 2016 (10.0.14393.0). Moreover, using
robocopy v14393 on Windows 10 doesn't affect the issue.
Perhaps this information can be helpful for someone who is running
buildfarm/CI tests on Windows animals...
[1]
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2024-09-11%2007%3A24%3A53
Interesting, good detective work. Still, wouldn't this mean drongo would
fail consistently?
I wonder why we're using robocopy instead of our own RecursiveCopy module?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: meson vs windows perl
On 2024-07-30 Tu 3:47 PM, Andrew Dunstan wrote: On 2024-07-20 Sa 9:41 AM, Andrew Dunstan wrote: On 2024-05-28 Tu 6:13 PM, Andres Freund wrote: Hi, On 2024-04-05 16:12:12 -0400, Andrew Dunstan wrote: OK, this has been fixed and checked. The attached is what I propose. The perl command is pretty hard to read. What about using python's shlex module instead? Rough draft attached. Still not very pretty, but seems easier to read? It'd be even better if we could just get perl to print out the flags in an easier to parse way, but I couldn't immediately see a way. Thanks for looking. The attached should be easier to read. The perl package similar to shlex is Text::ParseWords, which is already a requirement. It turns out that shellwords eats backslashes, so we would need something like this version, which I have tested on Windows. I will probably commit this in the next few days unless there's an objection. pushed cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Mutable foreign key constraints
Andrew Dunstan writes: > On 2024-09-12 Th 5:33 PM, Tom Lane wrote: >> I'm inclined to propose rejecting FK constraints if the comparison >> operator is not immutable. > Isn't there an upgrade hazard here? People won't thank us if they can't > now upgrade their clusters. If we can get around that then +1. Yeah, they would have to fix the bad DDL before upgrading. It'd be polite of us to add a pg_upgrade precheck for such cases, perhaps. regards, tom lane
Re: why there is not VACUUM FULL CONCURRENTLY?
On Wed, Sep 4, 2024 at 7:41 PM Antonin Houska wrote:
>
> Junwang Zhao wrote:
>
> > Thanks for working on this, I think this is a very useful feature.
> >
> > The patch doesn't compile in the debug build with errors:
> >
> > ../postgres/src/backend/commands/cluster.c: In function ‘get_catalog_state’:
> > ../postgres/src/backend/commands/cluster.c:2771:33: error: declaration
> > of ‘td_src’ shadows a previous local [-Werror=shadow=compatible-local]
> > 2771 | TupleDesc td_src, td_dst;
> > | ^~
> > ../postgres/src/backend/commands/cluster.c:2741:25: note: shadowed
> > declaration is here
> > 2741 | TupleDesc td_src = RelationGetDescr(rel);
>
> ok, gcc14 complains here, the compiler I used before did not. Fixed.
>
> > you forgot the meson build for pgoutput_cluster
> >
> > diff --git a/src/backend/meson.build b/src/backend/meson.build
> > index 78c5726814..0f9141a4ac 100644
> > --- a/src/backend/meson.build
> > +++ b/src/backend/meson.build
> > @@ -194,5 +194,6 @@ pg_test_mod_args = pg_mod_args + {
> > subdir('jit/llvm')
> > subdir('replication/libpqwalreceiver')
> > subdir('replication/pgoutput')
> > +subdir('replication/pgoutput_cluster')
>
> Fixed, thanks. That might be the reason for the cfbot to fail when using
> meson.
>
> > I noticed that you use lmode/lock_mode/lockmode, there are lmode and
> > lockmode
> > in the codebase, but I remember someone proposed all changes to lockmode,
> > how
> > about sticking to lockmode in your patch?
>
> Fixed.
>
> > 0004:
> >
> > + sure that the old files do not change during the processing because
> > the
> > + chnages would get lost due to the swap.
> > typo
>
> Fixed.
>
> >
> > + files. The data changes that took place during the creation of the
> > new
> > + table and index files are captured using logical decoding
> > + () and applied before
> > + the ACCESS EXCLUSIVE lock is requested. Thus the
> > lock
> > + is typically held only for the time needed to swap the files, which
> > + should be pretty short.
> >
> > I remember pg_squeeze also did some logical decoding after getting the
> > exclusive
> > lock, if that is still true, I guess the doc above is not precise.
>
> The decoding takes place before requesting the lock, as well as after
> that. I've adjusted the paragraph, see 0007.
>
> > + Note that CLUSTER with the
> > + the CONCURRENTLY option does not try to order the
> > + rows inserted into the table after the clustering started.
> >
> > Do you mean after the *logical decoding* started here? If CLUSTER
> > CONCURRENTLY
> > does not order rows at all, why bother implementing it?
>
> The rows inserted before CLUSTER (CONCURRENTLY) started do get ordered, the
> rows inserted after that do not. (Actually what matters is when the snapshot
> for the initial load is created, but that happens in very early stage of the
> processing. Not sure if user is interested in such implementation details.)
>
>
> > + errhint("CLUSTER CONCURRENTLY is only allowed for permanent relations")));
> >
> > errhint messages should end with a dot. Why hardcoded to "CLUSTER
> > CONCURRENTLY"
> > instead of parameter *stmt*.
>
> Fixed.
>
> > + ResourceOwner oldowner = CurrentResourceOwner;
> > +
> > + /*
> > + * In the CONCURRENT case, do the planning in a subtrensaction so that
> > typo
>
> Fixed.
>
> > I did not see VacuumStmt changes in gram.y, how do we suppose to
> > use the vacuum full concurrently? I tried the following but no success.
>
> With the "parethesized syntax", new options can be added w/o changing
> gram.y. (While the "unparenthesized syntax" is deprecated.)
>
> > [local] postgres@demo:5432-36097=# vacuum (concurrently) aircrafts_data;
> > ERROR: CONCURRENTLY can only be specified with VACUUM FULL
>
> The "lazy" VACUUM works concurrently as such.
>
> > [local] postgres@demo:5432-36097=# vacuum full (concurrently) full
> > aircrafts_data;
> > ERROR: syntax error at or near "("
> > LINE 1: vacuum full (concurrently) full aircrafts_data;
>
> This is not specific to the CONCURRENTLY option. For example:
>
> postgres=3D# vacuum full (analyze) full aircrafts_data;
> ERROR: syntax error at or near "("
> LINE 1: vacuum full (analyze) full aircrafts_data;
>
> (You seem to combine the parenthesized syntax with the unparenthesized.)
Yeah, my mistake, *vacuum (full, concurrently)* works.
+ if (TransactionIdIsNormal(HeapTupleHeaderGetRawXmax(tuple->t_data)) &&
+ HeapTupleMVCCNotDeleted(tuple, snapshot, buffer))
+ {
+ /* TODO More work needed here?*/
+ tuple->t_data->t_infomask |= HEAP_XMAX_INVALID;
+ HeapTupleHeaderSetXmax(tuple->t_data, 0);
+ }
I don't quite understand the above code, IIUC xmax and xmax invalid
are set directly on the buffer page. What if the command failed? Will
this break the visibility rules?
btw, v4-0006 failed to apply.
>
> --
> Antonin Houska
> Web: https://www.cybertec-postgresql.com
>
--
Regards
Junwang Zhao
Re: Robocopy might be not robust enough for never-ending testing on Windows
Hello Andrew, 14.09.2024 17:22, Andrew Dunstan wrote: On 2024-09-14 Sa 9:00 AM, Alexander Lakhin wrote: While trying to reproduce inexplicable drongo failures (e. g., [1]) seemingly related to BackgroundPsql, I stumbled upon close, but not the same issue. ... Interesting, good detective work. Still, wouldn't this mean drongo would fail consistently? Yes, I think drongo is suffering from another disease, we're yet to find which. I wonder why we're using robocopy instead of our own RecursiveCopy module? AFAICS, robocopy is also used by regress.exe, so switching to the perl module would require perl even for regress tests. I know that perl was a requirement for MSVC builds, but maybe that's not so with meson... Best regards, Alexander
Re: Obsolete comment in pg_stat_statements
Julien Rouhaud writes: > On Sat, 14 Sept 2024, 12:39 Tom Lane, wrote: >> Hmm ... I agree that para is out of date, but is there anything to >> salvage rather than just delete it? > I thought about it but I think that now that knowledge is in the else > branch, with the mention that we still have to bump the nesting level even > if it's not locally handled. After sleeping on it I looked again, and I think you're right, there's no useful knowledge remaining in this para. Pushed. regards, tom lane
Re: Psql meta-command conninfo+
On 2024-Sep-14, Hunaid Sohail wrote:
> I agree that both messages should be printed together. IMO the message
> "You are connected to database..." should be printed at the top, no?
> Because it shows important info that the user may be interested to see
> first. Then we can combine the ssl message.
>
> postgres=# \x
> Expanded display is on.
> postgres=# \conninfo+
> You are connected to database "postgres" as user "hunaid" on host
> "localhost" (address "127.0.0.1") at port "5430".
> SSL connection (protocol: TLSv1.3, cipher: TLS_AES_256_GCM_SHA384,
> compression: off, ALPN: postgresql)
> Connection Information
> -[ RECORD 1 ]+---
> Protocol Version | 3
> SSL Connection | yes
> GSSAPI Authenticated | no
> Client Encoding | UTF8
> Server Encoding | UTF8
> Session User | hunaid
> Backend PID | 109092
I don't understand why this is is printing half the information in
free-form plain text and the other half in tabular format. All these
items that you have in the free-form text lines should be part of the
table, I think.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
#error "Operator lives in the wrong universe"
("Use of cookies in real-time system development", M. Gleixner, M. Mc Guire)
Re: Psql meta-command conninfo+
Alvaro Herrera writes: > I don't understand why this is is printing half the information in > free-form plain text and the other half in tabular format. All these > items that you have in the free-form text lines should be part of the > table, I think. +1, that was my reaction as well. I can see the point of showing those items the same way as plain \conninfo does, but I think we're better off just making \conninfo+ produce a table and nothing else. regards, tom lane
Re: access numeric data in module
Good afternoon- Was there a resolution of this? I'm wondering if it is worth it for me to submit a PR for the next commitfest. -Ed On Mon, Sep 9, 2024 at 8:40 PM Ed Behn wrote: > Sorry for taking so long to respond. I was at my day-job. > > As I mentioned, I am the maintainer of the PL/Haskell language extension. > This extension allows users to write code in the Haskell language. In order > to use numeric types, I will need to create a Haskell type equivalent. > Something like > > data Numeric = PosInfinity | NegInfinity | NaN | Number Integer Int16 > > where the Number constructor represents a numeric's mantissa and weight. > > In order to get or return data, I would need to be able to access those > fields of the numeric type. > > I'm not proposing giving access to the actual numeric structure. Rather, > the data should be accessed by function call or macro. This would allow > future changes to the inner workings without breaking compatibility as long > as the interface is maintained. It looks to me like all of the code to > access data exists, it should simply be made accessible. An additional > function should exist that allows an extension to create a numeric > structure by passing the needed data. > > -Ed > > > On Mon, Sep 9, 2024 at 2:45 PM Robert Haas wrote: > >> On Mon, Sep 9, 2024 at 2:02 PM Tom Lane wrote: >> > By that argument, we should move every declaration in every .c file >> > into c.h and be done. I'd personally be happier if we had *not* >> > exposed the other data structure details you mention, but that >> > ship has sailed. >> >> Not every declaration in every .c file is of general interest, but the >> ones that are probably should be moved into .h files. The on-disk >> representation of a commonly-used data type certainly qualifies. >> >> You can see from Chapman's reply that I'm not making this up: when we >> don't expose things, it doesn't keep people from depending on them, it >> just makes them copy our code into their own repository. That's not a >> win. It makes those extensions more fragile, not less, and it makes >> the PostgreSQL extension ecosystem worse. pg_hint_plan is another, >> recently-discussed example of this phenomenon: refuse to give people >> the keys, and they start hot-wiring stuff. >> >> > If we do do what you're advocating, I'd at least insist that the >> > declarations go into a new file numeric_internal.h, so that it's >> > clear to all concerned that they're playing with fire if they >> > depend on that stuff. >> >> I think that's a bit pointless considering that we don't do it in any >> of the other cases. I'd rather be consistent with our usual practice. >> But if it ends up in a separate header file that's still better than >> the status quo. >> >> -- >> Robert Haas >> EDB: http://www.enterprisedb.com >> >
Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
Hi, Quick question, are there any more revisions left to be done on this patch from the previous feedback? Or should I continue with reviewing the current patch? Regards, Akshat Jaimini
Cleaning up ERRCODE usage in our XML code
I noticed while working on bug #18617 [1] that we are fairly slipshod
about which SQLSTATE to report when libxml2 returns an error. There
are places using ERRCODE_INTERNAL_ERROR for easily-triggered errors;
there are different places that use different ERRCODEs for exactly
the same condition; and there are places that use different ERRCODEs
for failures from xmlXPathCompile and xmlXPathCompiledEval. I found
that this last is problematic because some errors you might expect
to be reported during XPath compilation are not detected till
execution, notably namespace-related errors. That seems more like
a libxml2 implementation artifact than something we should expect to
be stable behavior, so I think we should avoid using different
ERRCODEs.
A lot of this can be blamed on there not being any especially on-point
SQLSTATE values back when this code was first written. I learned that
recent revisions of SQL have a whole new SQLSTATE class, class 10 =
"XQuery Error", so we have an opportunity to sync up with that as well
as be more self-consistent. The spec's subclass codes in this class
seem quite fine-grained. It might be an interesting exercise to try
to teach xml_errorHandler() to translate libxml2's error->code values
into fine-grained SQLSTATEs, but I've not attempted that; I'm not
sure whether there is a close mapping between what libxml2 reports
and the set of codes the SQL committee chose. What I've done in the
attached first-draft patch is just to select one relatively generic
code in class 10, 10608 = invalid_argument_for_xquery, and use that
where it seemed apropos.
This is pretty low-priority, so I'll stash it in the next CF.
regards, tom lane
[1] https://www.postgresql.org/message-id/356363.1726333674%40sss.pgh.pa.us
diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index f94b622d92..cc40fa5624 100644
--- a/contrib/xml2/xpath.c
+++ b/contrib/xml2/xpath.c
@@ -388,7 +388,7 @@ pgxml_xpath(text *document, xmlChar *xpath, xpath_workspace *workspace)
/* compile the path */
comppath = xmlXPathCompile(xpath);
if (comppath == NULL)
-xml_ereport(xmlerrcxt, ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
+xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
"XPath Syntax Error");
/* Now evaluate the path expression. */
@@ -652,7 +652,7 @@ xpath_table(PG_FUNCTION_ARGS)
comppath = xmlXPathCompile(xpaths[j]);
if (comppath == NULL)
xml_ereport(xmlerrcxt, ERROR,
- ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
+ ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
"XPath Syntax Error");
/* Now evaluate the path expression. */
diff --git a/contrib/xml2/xslt_proc.c b/contrib/xml2/xslt_proc.c
index f30a3a42c0..e761ca5cb5 100644
--- a/contrib/xml2/xslt_proc.c
+++ b/contrib/xml2/xslt_proc.c
@@ -90,7 +90,7 @@ xslt_process(PG_FUNCTION_ARGS)
XML_PARSE_NOENT);
if (doctree == NULL)
- xml_ereport(xmlerrcxt, ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_XML_DOCUMENT,
"error parsing XML document");
/* Same for stylesheet */
@@ -99,14 +99,14 @@ xslt_process(PG_FUNCTION_ARGS)
XML_PARSE_NOENT);
if (ssdoc == NULL)
- xml_ereport(xmlerrcxt, ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_XML_DOCUMENT,
"error parsing stylesheet as XML document");
/* After this call we need not free ssdoc separately */
stylesheet = xsltParseStylesheetDoc(ssdoc);
if (stylesheet == NULL)
- xml_ereport(xmlerrcxt, ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
"failed to parse stylesheet");
xslt_ctxt = xsltNewTransformContext(stylesheet, doctree);
@@ -141,7 +141,7 @@ xslt_process(PG_FUNCTION_ARGS)
NULL, NULL, xslt_ctxt);
if (restree == NULL)
- xml_ereport(xmlerrcxt, ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
"failed to apply stylesheet");
resstat = xsltSaveResultToString(&resstr, &reslen, restree, stylesheet);
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 1a07876cd5..3eaf9f78a8 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -742,7 +742,7 @@ xmltotext_with_options(xmltype *data, XmlOptionType xmloption_arg, bool indent)
{
/* If it's a document, saving is easy. */
if (xmlSaveDoc(ctxt, doc) == -1 || xmlerrcxt->err_occurred)
-xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
+xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
"could not save document to xmlBuffer");
}
else if (content_nodes != NULL)
@@ -785,7 +785,7 @@ xmltotext_with_options(xmltype *data, XmlOptionType xmloption_arg, bool indent)
if (xmlSaveTree(ctxt, newline) == -1 || xmlerrcxt->err_occurred)
{
xmlFreeNode(newline);
- x
Re: Robocopy might be not robust enough for never-ending testing on Windows
On Sun, Sep 15, 2024 at 1:00 AM Alexander Lakhin wrote: > (That is, 0.1-0.2 MB leaks per one robocopy run.) > > I observed this on Windows 10 (Version 10.0.19045.4780), with all updates > installed, but not on Windows Server 2016 (10.0.14393.0). Moreover, using > robocopy v14393 on Windows 10 doesn't affect the issue. I don't understand Windows but that seems pretty weird to me, as it seems to imply that a driver or something fairly low level inside the kernel is leaking objects (at least by simple minded analogies to operating systems I understand better). Either that or robocop.exe has userspace stuff involving at least one thread still running somewhere after it's exited, but that seems unlikely as I guess you'd have noticed that... Just a thought: I was surveying the block cloning landscape across OSes and filesystems while looking into clone-based CREATE DATABASE (CF #4886) and also while thinking about the new TAP test initdb template copy trick, is that robocopy.exe tries to use Windows' block cloning magic, just like cp on recent Linux and FreeBSD systems (at one point I was wondering if that was causing some funky extra flush stalls on some systems, I need to come back to that...). It probably doesn't actually work unless you have Windows 11 kernel with DevDrive enabled (from reading, no Windows here), but I guess it still probably uses the new system interfaces, probably something like CopyFileEx(). Does it still leak if you use /nooffload or /noclone?
Re: Regression tests fail with tzdata 2024b
Wolfgang Walther writes: > Building --with-system-tzdata and the latest tzdata 2024b fails the > regression tests for me (see attached .diffs). This seems to be because > of [1], which changed the way "PST8PDT" is handled. This is the timezone > that the regression tests are run with. That's quite annoying, especially since it was not mentioned in the 2024b release notes. (I had read the notes and concluded that 2024b didn't require any immediate attention on our part.) > From 2024b on, "PST8PDT" is the same as "America/Los_Angeles", so by > changing the regression tests to use the latter as the default, we're > getting consistent output on at least 2024a and 2024b. I'm fairly un-thrilled with this answer, not least because it exposes that zone's idiosyncratic "LMT" offset of -7:52:58 for years before 1883. (I'm surprised that that seems to affect only one or two regression results.) Also, as a real place to a greater extent than "PST8PDT" is, it's more subject to historical revisionism when somebody turns up evidence of local law having been different than TZDB currently thinks. We may not have a lot of choice though. I experimented with using full POSIX notation, that is "PST8PDT,M3.2.0,M11.1.0", but that is actually worse in terms of the number of test diffs, since it doesn't match the DST transition dates that the tests expect for years before 2007. Another objection is that the tests would then not exercise any of the mainstream tzdb-file-reading code paths within the timezone code itself. Grumble. regards, tom lane
Re: Robocopy might be not robust enough for never-ending testing on Windows
On Sun, Sep 15, 2024 at 8:32 AM Thomas Munro wrote: > template copy trick, is that robocopy.exe tries to use Windows' block Erm, sorry for my fumbled early morning message editing and garbled English... I meant to write "one thing I noticed is that..".
Re: may be a mismatch between the construct_array and construct_md_array comments
On 12.09.2024 20:44, Tom Lane wrote: Alena Rybakina writes: I noticed that there is a comment that values with NULL are not processed there, but in fact this function calls the construct_md_array function, which contains a comment that it can handle NULL values. Right. construct_md_array has a "bool *nulls" argument, but construct_array doesn't --- it passes NULL for that to construct_md_array, which will therefore assume there are no null array elements. Understood. At first I thought this comment was related to the value of a NULL element that might be in the Array, but now I realized that this is not the case. Thanks for the explanation, it helped a lot!
Re: Why don't we consider explicit Incremental Sort?
On 9/14/24 05:50, Richard Guo wrote: > On Fri, Sep 13, 2024 at 7:51 PM Tomas Vondra wrote: >> Sure, an incremental sort can improve things if things go well, no doubt >> about that. But how significant can the improvement be, especially if we >> need to sort everything? In your example it's ~15%, which is nice, and >> maybe the benefits can be much larger if the incremental sort can do >> everything in memory, without flushing to disk. >> >> But what about the risks? What will happen if the groups are not this >> uniformly and independently sized, and stuff like that? Maybe it'll be >> costed well enough, I haven't tried. > > I understand the concern and agree that there is a risk of regression > if there is a skew in the number of rows per pre-sorted group. > > Actually there were discussions about this during the work on commit > 4a29eabd1. Please see [1]. I will repeat David's demonstration and > rerun his query on the current master branch to see what happens. > > create table ab (a int not null, b int not null); > insert into ab select 0,x from generate_Series(1,999000)x union all > select x%1000+1,0 from generate_Series(999001,100)x; > > create index on ab (a); > > analyze ab; > > So this is a table with a very large skew: the 0 group has 999000 > rows, and the remaining groups 1-1000 have just 1 row each. > > -- on master > explain (analyze, timing off) select * from ab order by a,b; >QUERY PLAN > - > Incremental Sort (cost=2767.38..109344.55 rows=100 width=8) > (actual rows=100 loops=1) >Sort Key: a, b >Presorted Key: a >Full-sort Groups: 33 Sort Method: quicksort Average Memory: 26kB > Peak Memory: 26kB >Pre-sorted Groups: 1 Sort Method: external merge Average Disk: > 17680kB Peak Disk: 17680kB >-> Index Scan using ab_a_idx on ab (cost=0.42..22832.42 > rows=100 width=8) (actual rows=100 loops=1) > Planning Time: 0.829 ms > Execution Time: 1028.892 ms > (8 rows) > > -- manually disable incremental sort > set enable_incremental_sort to off; > explain (analyze, timing off) select * from ab order by a,b; >QUERY PLAN > > Sort (cost=127757.34..130257.34 rows=100 width=8) (actual > rows=100 loops=1) >Sort Key: a, b >Sort Method: external merge Disk: 17704kB >-> Seq Scan on ab (cost=0.00..14425.00 rows=100 width=8) > (actual rows=100 loops=1) > Planning Time: 0.814 ms > Execution Time: 765.198 ms > (6 rows) > > Look, regression happens on current master! > > This is a question I’ve often pondered: each time we introduce a new > optimization, there are always potential cases where it could lead to > regressions. What should we do about this? I kind of agree on > David's option that, as in the commit message of 4a29eabd1: > Right, or as Goetz Graefe said "choice is confusion." The funny thing is it also matters when the alternative plans are introduced. Had it been at the very beginning, we'd consider the behavior (including choosing sub-optimal plans) the baseline, and it'd be okay-ish. But when we're introducing those alternative paths later, it's more likely to be seen as a "regression" ... > " > That, of course, as with teaching the planner any new tricks, > means an increased likelihood that the planner will perform an incremental > sort when it's not the best method. Our standard escape hatch for these > cases is an enable_* GUC. > " > > I know ideally we should not rely on these enable_* GUCs, but in > reality it seems that sometimes we do not have a better solution. > Right, the basic truth is there's no way to teach the optimizer to do new stuff without a risk of regression. We simply can't do perfect decisions based on incomplete information (which is what the statistics are, intentionally). It is up to us to reason about the risks, and ideally deal with that later at execution time. I still don't think we should rely on GUCs too much for this. regards -- Tomas Vondra
Re: Why don't we consider explicit Incremental Sort?
On 9/14/24 05:37, Richard Guo wrote: > On Fri, Sep 13, 2024 at 7:35 PM Tomas Vondra wrote: >> On 9/13/24 06:04, Richard Guo wrote: >>> It seems to me that some parts of our code assume that, for a given >>> input path that is partially ordered, an incremental sort is always >>> preferable to a full sort (see commit 4a29eabd1). Am I getting this >>> correctly? >> >> I don't think we're making such assumption. I don't know which of the >> many places modified by 4a29eabd1 you have in mind, but IIRC we always >> consider both full and incremental sort. > > Hmm, I don't think it's true that we always consider both full and > incremental sort on the same path. It was true initially, but that’s > no longer the case. > > I checked the 9 callers of create_incremental_sort_path, and they all > follow the logic that if there are presorted keys, only incremental > sort is considered. To quote a comment from one of the callers: > > * ... We've no need to consider both > * sort and incremental sort on the same path. We assume that > * incremental sort is always faster when there are presorted > * keys. > > I think this is also explained in the commit message of 4a29eabd1, > quoted here: > > " > Previously we would generally create a sort path on the cheapest input > path (if that wasn't sorted already) and incremental sort paths on any > path which had presorted keys. This meant that if the cheapest input path > wasn't completely sorted but happened to have presorted keys, we would > create a full sort path *and* an incremental sort path on that input path. > Here we change this logic so that if there are presorted keys, we only > create an incremental sort path, and create sort paths only when a full > sort is required. > " > Hmmm ... I wasn't involved in that discussion and I haven't studied the thread now, but this seems a bit weird to me. If the presorted keys have low cardinality, can't the full sort be faster? Maybe it's not possible with the costing changes (removing the penalization), in which case it would be fine to not consider the full sort, because we'd just throw it away immediately. But if the full sort could be costed as cheaper, I'd say that might be wrong. regards -- Tomas Vondra
Re: Detailed release notes
On Fri, Sep 13, 2024 at 12:39:28PM -0400, Bruce Momjian wrote:
> I applied this patch to PG 17. You can see the results at:
>
> https://momjian.us/pgsql_docs/release-17.html
>
> The community doc build only shows the master branch, which is PG 18,
> and the PG 17 docs are only built before the release.
>
> I changed the patch to use the section symbol "§" instead of showing
> the hashes. The hashes seemed too detailed. Does anyone see a better
> symbol to use from here?
>
> http://www.zipcon.net/~swhite/docs/computers/browsers/entities_page.html
>
> I think we are limited to the HTML entities listed on that page. I also
> removed the brackets and the period you added at the end of the text.
I wrote the attached Perl script that automatically adds commit links.
I tested it against PG 12-17 and the results were good. I plan to add
this script to all branches and run it on all branch release notes in a
few days.
--
Bruce Momjian https://momjian.us
EDB https://enterprisedb.com
When a patient asks the doctor, "Am I going to die?", he means
"Am I going to die soon?"
#! /usr/bin/perl
#
# add_commit_links.pl -- add commit links to the release notes
#
# Copyright (c) 2024, PostgreSQL Global Development Group
#
# src/tools/add_commit_links.pl
#
#
# This script adds commit links to the release notes.
#
# Usage: cd to top of source tree and issue
# src/tools/add_commit_links.pl release_notes_file
#
# The script can add links for release note items that lack them, and update
# those that have them. The script is sensitive to the release note file being
# in a specific format:
#
# * File name contains the major version number preceded by a dash
#and followed by a period
# * Commit text is generated by src/tools/git_changelog
# * SGML comments around commit text start in the first column
# * The commit item title ends with an attribution that ends with
#a closing parentheses
# * previously added URL link text is unmodified
# * a "" follows the commit item title
#
# The major version number is used to select the commit hash for minor
# releases. An error will be generated if valid commits are found but
# no proper location for the commit links is found.
use strict;
use warnings FATAL => 'all';
if (@ARGV != 1)
{
printf(STDERR "Usage: %s release_notes_file\n", $0);
exit(1);
}
my $in_comment = 0;
my $prev_line_ended_with_paren = 0;
my $prev_leading_space = '';
my $lineno = 0;
my @hashes = ();
my $file = shift @ARGV;
my $tmpfile = $file . ".tmp";
# Get major version number from the file name.
$file =~ m/-([0-9]+)\./;
my $major_version = $1;
open(my $fh, '<', $file) || die "could not open file %s: $!\n", $file;
open(my $tfh, '>', $tmpfile) || die "could not open file %s: $!\n", $tmpfile;
while (<$fh>)
{
$lineno++;
$in_comment = 1 if (m/^/);
}
close($fh);
close($tfh);
rename($tmpfile, $file) || die "could not rename %s to %s: $!\n", $tmpfile,
$file;
[PATCH] WIP: replace method for jsonpath
Hi,When working with jsonb/jsonpath, I’ve always wanted more fluent string operations for data cleaning.Ideally, chaining text processing methods together.Here’s a draft patch that adds a string replace method.It works like thisselect jsonb_path_query('"hello world"', '$.replace("hello","bye")'); jsonb_path_query -- "bye world"(1 row)And looks like plays nicely with existing facilities select jsonb_path_query('"hello world"', '$.replace("hello","bye") starts with "bye"'); jsonb_path_query -- true(1 row)I’d like some initial feedback on whether this is of interested before I proceed with the following:- I’ve tried respecting the surrounding code, but a more experienced eye can spot some inconsistencies. I’ll revisit those- Add more test cases (I’ve added the obvious ones, but ideas on more cases are welcome).- pg upgrades currently fail on CI (see)- better error handling/reporting: I’ve kept the wording of error messages, but we’ll need something akin to ERRCODE_INVALID_ARGUMENT_FOR_SQL_JSON_DATETIME_FUNCTION.- documentation.
v1-0001-jsonpath-replace-method.patch
Description: Binary data
Re: Obsolete comment in pg_stat_statements
On Sat, 14 Sept 2024, 23:44 Tom Lane, wrote: > Julien Rouhaud writes: > > On Sat, 14 Sept 2024, 12:39 Tom Lane, wrote: > >> Hmm ... I agree that para is out of date, but is there anything to > >> salvage rather than just delete it? > > > I thought about it but I think that now that knowledge is in the else > > branch, with the mention that we still have to bump the nesting level > even > > if it's not locally handled. > > After sleeping on it I looked again, and I think you're right, > there's no useful knowledge remaining in this para. Pushed. thanks!
