Re: Table AM Interface Enhancements
Hi, Alexander and Andres! On Mon, 8 Apr 2024 at 03:25, Alexander Korotkov wrote: > Hi, > > On Mon, Apr 8, 2024 at 12:40 AM Andres Freund wrote: > > On 2024-03-30 23:33:04 +0200, Alexander Korotkov wrote: > > > I've pushed 0001, 0002 and 0006. > > > > I briefly looked at 27bc1772fc81 and I don't think the state post this > commit > > makes sense. Before this commit another block based AM could implement > analyze > > without much code duplication. Now a large portion of analyze.c has to be > > copied, because they can't stop acquire_sample_rows() from calling > > heapam_scan_analyze_next_block(). > > > > I'm quite certain this will break a few out-of-core AMs in a way that > can't > > easily be fixed. > > I was under the impression there are not so many out-of-core table > AMs, which have non-dummy analysis implementations. And even if there > are some, duplicating acquire_sample_rows() isn't a big deal. > > But given your feedback, I'd like to propose to keep both options > open. Turn back the block-level API for analyze, but let table-AM > implement its own analyze function. Then existing out-of-core AMs > wouldn't need to do anything (or probably just set the new API method > to NULL). > I think that providing both new and old interface functions for block-based and non-block based custom am is an excellent compromise. The patch v1-0001-Turn-back.. is mainly an undo of part of the 27bc1772fc81 that had turned off _analyze_next_tuple..analyze_next_block for external callers. If some extensions are already adapted to the old interface functions, they are free to still use it. > And even for non-block based AMs, the new interface basically requires > > reimplementing all of analyze.c. > . > Non-lock base AM needs to just provide an alternative implementation > for what acquire_sample_rows() does. This seems like reasonable > effort for me, and surely not reimplementing all of analyze.c. > I agree. Regards, Pavel Borisov Supabase
NLS doesn't work for pg_combinebackup
Hello.
I noticed that NLS doesn't work for pg_combinebackup. The cause is
that the tool forgets to call set_pglocale_pgservice().
This issue is fixed by the following chage.
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c
b/src/bin/pg_combinebackup/pg_combinebackup.c
index 1b07ca3fb6..2788c78fdd 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -154,6 +154,7 @@ main(int argc, char *argv[])
pg_logging_init(argv[0]);
progname = get_progname(argv[0]);
+ set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_combinebackup"));
handle_help_version_opts(argc, argv, progname, help);
memset(&opt, 0, sizeof(opt));
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: NLS doesn't work for pg_combinebackup
At Mon, 08 Apr 2024 16:27:02 +0900 (JST), Kyotaro Horiguchi
wrote in
> Hello.
>
> I noticed that NLS doesn't work for pg_combinebackup. The cause is
> that the tool forgets to call set_pglocale_pgservice().
>
> This issue is fixed by the following chage.
>
> diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c
> b/src/bin/pg_combinebackup/pg_combinebackup.c
> index 1b07ca3fb6..2788c78fdd 100644
> --- a/src/bin/pg_combinebackup/pg_combinebackup.c
> +++ b/src/bin/pg_combinebackup/pg_combinebackup.c
> @@ -154,6 +154,7 @@ main(int argc, char *argv[])
>
> pg_logging_init(argv[0]);
> progname = get_progname(argv[0]);
> + set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_combinebackup"));
> handle_help_version_opts(argc, argv, progname, help);
>
> memset(&opt, 0, sizeof(opt));
Forgot to mention, but pg_walsummary has the same issue.
diff --git a/src/bin/pg_walsummary/pg_walsummary.c
b/src/bin/pg_walsummary/pg_walsummary.c
index 5e41b376d7..daf6cd14ce 100644
--- a/src/bin/pg_walsummary/pg_walsummary.c
+++ b/src/bin/pg_walsummary/pg_walsummary.c
@@ -67,6 +67,7 @@ main(int argc, char *argv[])
pg_logging_init(argv[0]);
progname = get_progname(argv[0]);
+ set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_walsummary"));
handle_help_version_opts(argc, argv, progname, help);
/* process command-line options */
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Weird test mixup
On Mon, Apr 08, 2024 at 10:22:40AM +0900, Michael Paquier wrote:
> For now I have applied 997db123c054 to make the GIN tests with
> injection points repeatable as it was an independent issue, and
> f587338dec87 to add the local function pieces.
Bharath has reported me offlist that one of the new tests has a race
condition when doing the reconnection. When the backend creating the
local points is very slow to exit, the backend created after the
reconnection may detect that a local point previously created still
exists, causing a failure. The failure can be reproduced with a sleep
in the shmem exit callback, like:
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -163,6 +163,8 @@ injection_points_cleanup(int code, Datum arg)
if (!injection_point_local)
return;
+ pg_usleep(100 * 1L);
+
SpinLockAcquire(&inj_state->lock);
for (int i = 0; i < INJ_MAX_CONDITION; i++)
{
At first I was looking at a loop with a scan of pg_stat_activity, but
I've noticed that regress.so includes a wait_pid() that we can use to
make sure that a given process exits before moving on to the next
parts of a test, so I propose to just reuse that here. This requires
tweaks with --dlpath for meson and ./configure, nothing new. The CI
is clean. Patch attached.
Thoughts?
--
Michael
From 8f43807000c1f33a0238d7bbcc148a196e4134e4 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 8 Apr 2024 16:28:17 +0900
Subject: [PATCH] Stabilize injection point test
---
src/test/modules/injection_points/Makefile | 1 +
.../expected/injection_points.out| 16
src/test/modules/injection_points/meson.build| 1 +
.../injection_points/sql/injection_points.sql| 15 +++
4 files changed, 33 insertions(+)
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 1cb395c37a..31bd787994 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -7,6 +7,7 @@ DATA = injection_points--1.0.sql
PGFILEDESC = "injection_points - facility for injection points"
REGRESS = injection_points
+REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
# The injection points are cluster-wide, so disable installcheck
NO_INSTALLCHECK = 1
diff --git a/src/test/modules/injection_points/expected/injection_points.out b/src/test/modules/injection_points/expected/injection_points.out
index 3d94016ac9..d2a69b54e8 100644
--- a/src/test/modules/injection_points/expected/injection_points.out
+++ b/src/test/modules/injection_points/expected/injection_points.out
@@ -1,4 +1,11 @@
CREATE EXTENSION injection_points;
+\getenv libdir PG_LIBDIR
+\getenv dlsuffix PG_DLSUFFIX
+\set regresslib :libdir '/regress' :dlsuffix
+CREATE FUNCTION wait_pid(int)
+ RETURNS void
+ AS :'regresslib'
+ LANGUAGE C STRICT;
SELECT injection_points_attach('TestInjectionBooh', 'booh');
ERROR: incorrect action "booh" for injection point creation
SELECT injection_points_attach('TestInjectionError', 'error');
@@ -156,8 +163,17 @@ NOTICE: notice triggered for injection point TestConditionLocal2
(1 row)
+select pg_backend_pid() as oldpid \gset
-- reload, local injection points should be gone.
\c
+-- Wait for the previous backend process to exit, ensuring that its local
+-- injection points are cleaned up.
+SELECT wait_pid(:'oldpid');
+ wait_pid
+--
+
+(1 row)
+
SELECT injection_points_run('TestConditionLocal1'); -- nothing
injection_points_run
--
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index a29217f75f..8e1b5b4539 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -33,6 +33,7 @@ tests += {
'sql': [
'injection_points',
],
+'regress_args': ['--dlpath', meson.build_root() / 'src/test/regress'],
# The injection points are cluster-wide, so disable installcheck
'runningcheck': false,
},
diff --git a/src/test/modules/injection_points/sql/injection_points.sql b/src/test/modules/injection_points/sql/injection_points.sql
index 2aa76a542b..e18146eb8c 100644
--- a/src/test/modules/injection_points/sql/injection_points.sql
+++ b/src/test/modules/injection_points/sql/injection_points.sql
@@ -1,5 +1,14 @@
CREATE EXTENSION injection_points;
+\getenv libdir PG_LIBDIR
+\getenv dlsuffix PG_DLSUFFIX
+\set regresslib :libdir '/regress' :dlsuffix
+
+CREATE FUNCTION wait_pid(int)
+ RETURNS void
+ AS :'regresslib'
+ LANGUAGE C STRICT;
+
SELECT injection_points_attach('TestInjectionBooh', 'booh');
SELECT injection_points_attach('TestInjectionError', 'error');
SELECT injection_points_attach('TestInjectionLog', 'notice');
@@ -40,8 +49,14 @@ SELECT injection_points_attach('TestConditionLocal1', 'error');
SELECT injection_points_at
Re: NLS doesn't work for pg_combinebackup
On Mon, Apr 08, 2024 at 04:27:02PM +0900, Kyotaro Horiguchi wrote: > I noticed that NLS doesn't work for pg_combinebackup. The cause is > that the tool forgets to call set_pglocale_pgservice(). > > This issue is fixed by the following chage. Indeed. Good catch. -- Michael signature.asc Description: PGP signature
Re: Weird test mixup
> On 8 Apr 2024, at 10:33, Michael Paquier wrote: > > Thoughts? As an alternative we can make local injection points mutually exclusive. Best regards, Andrey Borodin.
Re: Speed up clean meson builds by ~25%
Hello Michael, 08.04.2024 08:23, Michael Paquier wrote: On Fri, Apr 05, 2024 at 06:19:20PM +0200, Jelte Fennema-Nio wrote: Agreed. While not a full solution, I think this patch is still good to address some of the pain: Waiting 10 seconds at the end of my build with only 1 of my 10 cores doing anything. So while this doesn't decrease CPU time spent it does decrease wall-clock time significantly in some cases, with afaict no downsides. Well, this is also painful with ./configure. So, even if we are going to move away from it at this point, we still need to support it for a couple of years. It looks to me that letting the clang folks know about the situation is the best way forward. As I wrote in [1], I didn't observe the issue with clang-18, so maybe it is fixed already. Perhaps it's worth rechecking... [1] https://www.postgresql.org/message-id/d2bf3727-bae4-3aee-65f6-caec2c4ebaa8%40gmail.com Best regards, Alexander
Re: Speed up clean meson builds by ~25%
On 05.04.24 18:19, Jelte Fennema-Nio wrote: On Fri, 5 Apr 2024 at 17:24, Andres Freund wrote: I recommend opening a bug report for clang, best with an already preprocessed input file. We're going to need to do something about this from our side as well, I suspect. The times aren't great with gcc either, even if not as bad as with clang. Agreed. While not a full solution, I think this patch is still good to address some of the pain: Waiting 10 seconds at the end of my build with only 1 of my 10 cores doing anything. So while this doesn't decrease CPU time spent it does decrease wall-clock time significantly in some cases, with afaict no downsides. I have tested this with various compilers, and I can confirm that this shaves off about 5 seconds from the build wall-clock time, which represents about 10%-20% of the total time. I think this is a good patch. Interestingly, if I apply the analogous changes to the makefiles, I don't get any significant improvements. (Depends on local circumstances, of course.) But I would still suggest to keep the makefiles aligned.
Re: Logging parallel worker draught
> On 29 Feb 2024, at 11:24, Benoit Lobréau wrote: > > Yes, thanks for the proposal, I'll work on it on report here. Hi Benoit! This is kind reminder that this thread is waiting for your response. CF entry [0] is in "Waiting on Author", I'll move it to July CF. Thanks! Best regards, Andrey Borodin. [0] https://commitfest.postgresql.org/47/4291/
Re: GenBKI emits useless open;close for catalogs without rows
> On 22 Sep 2023, at 18:50, Matthias van de Meent > wrote: Hi Matthias! This is kind reminder that this thread is waiting for your response. CF entry [0] is in "Waiting on Author", I'll move it to July CF. Thanks! Best regards, Andrey Borodin. [0] https://commitfest.postgresql.org/47/4544/
Re: LogwrtResult contended spinlock
On 2024-Apr-07, Jeff Davis wrote: > On Sun, 2024-04-07 at 14:19 +0200, Alvaro Herrera wrote: > > I pushed the "copy" pointer now, except that I renamed it to > > "insert", > > which is what we call the operation being tracked. I also added some > > comments. > > The "copy" pointer, as I called it, is not the same as the "insert" > pointer (as returned by GetXLogInsertRecPtr()). > > LSNs before the "insert" pointer are reserved for WAL inserters (and > may or may not have made it to WAL buffers yet). LSNs before the "copy" > pointer are written to WAL buffers with CopyXLogRecordToWAL() (and may > or may not have been evicted to the OS file yet). It seems a matter of interpretation. WAL insertion starts with reserving the space (in ReserveXLogInsertLocation) and moving the CurrBytePos point forward; the same WAL insertion completes when the actual data has been copied to the buffers. It is this process as a whole that we can an insertion. CurrBytePos (what GetXLogInsertRecPtr returns) is the point where the next insertions are going to occur; the new logInsertResult is the point where previous insertions are known to have completed. We could think about insertions as something that's happening to a range of bytes. CurrBytePos is the high end of that range, logInsertResult is its low end. (Although in reality we don't know the true low end, because the process writing the oldest WAL doesn't advertise as soon as it finishes its insertion, because it doesn't know that it is writing the oldest. We only know that X is this "oldest known" after we have waited for all those that were inserting earlier than X to have finished.) My trouble with the "copy" term is that we don't use that term anywhere in relation to WAL. It's a term we don't need. This "copy" is in reality just the insertion, after it's finished. The "Result" suffix is intended to convey that it's the point where the operation has successfully finished. Maybe we can add some commentary to make this clearer. Now, if you're set on renaming the variable back to Copy, I won't object. Lastly, I just noticed that I forgot credits and discussion link in that commit message. I would have attributed authorship to Bharath though, because I had not realized that this patch had actually been written by you initially[1]. My apologies. [1] https://postgr.es/m/727449f3151c6b9ab76ba706fa4d30bf7b03ad4f.ca...@j-davis.com -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Speed up clean meson builds by ~25%
On Mon, 8 Apr 2024 at 10:00, Alexander Lakhin wrote:
> As I wrote in [1], I didn't observe the issue with clang-18, so maybe it
> is fixed already.
> Perhaps it's worth rechecking...
Using the attached script I got these timings. Clang is significantly
slower in all of them. But especially with -Og the difference between
is huge.
gcc 11.4.0: 7.276s
clang 18.1.3: 17.216s
gcc 11.4.0 --debug: 7.441s
clang 18.1.3 --debug: 18.164s
gcc 11.4.0 --debug -Og: 2.418s
clang 18.1.3 --debug -Og: 14.864s
I reported this same issue to the LLVM project here:
https://github.com/llvm/llvm-project/issues/87973
#!/bin/bash
set -exo pipefail
compile() {
ninja -C build
src/interfaces/ecpg/preproc/ecpg.p/meson-generated_.._preproc.c.o
rm
build/src/interfaces/ecpg/preproc/ecpg.p/meson-generated_.._preproc.c.o
time ninja -C build
src/interfaces/ecpg/preproc/ecpg.p/meson-generated_.._preproc.c.o -v
}
CC=gcc CC_LD=lld meson setup --reconfigure build --wipe > /dev/null
compile
CC=clang-18 CC_LD=lld meson setup --reconfigure build --wipe > /dev/null
compile
CC=gcc CC_LD=lld meson setup --reconfigure --debug build --wipe > /dev/null
compile
CC=clang-18 CC_LD=lld meson setup --reconfigure --debug build --wipe > /dev/null
compile
CC=gcc CC_LD=lld meson setup --reconfigure --debug -Dc_args="-Og" build --wipe
> /dev/null
compile
CC=clang-18 CC_LD=lld meson setup --reconfigure --debug -Dc_args="-Og" build
--wipe > /dev/null
compile
Re: Experiments with Postgres and SSL
On 08/04/2024 04:25, Heikki Linnakangas wrote: One important open item now is that we need to register a proper ALPN protocol ID with IANA. I sent a request for that: https://mailarchive.ietf.org/arch/msg/tls-reg-review/9LWPzQfOpbc8dTT7vc9ahNeNaiw/ -- Heikki Linnakangas Neon (https://neon.tech)
Re: generic plans and "initial" pruning
Hi Andrey, On Sun, Mar 31, 2024 at 2:03 PM Andrey M. Borodin wrote: > > On 6 Dec 2023, at 23:52, Robert Haas wrote: > > > > I hope that it's at least somewhat useful. > > > On 5 Jan 2024, at 15:46, vignesh C wrote: > > > > There is a leak reported > > Hi Amit, > > this is a kind reminder that some feedback on your patch[0] is waiting for > your reply. > Thank you for your work! Thanks for moving this to the next CF. My apologies (especially to Robert) for not replying on this thread for a long time. I plan to start working on this soon. -- Thanks, Amit Langote
Re: Weird test mixup
On Mon, Apr 08, 2024 at 10:42:08AM +0300, Andrey M. Borodin wrote: > As an alternative we can make local injection points mutually exclusive. Sure. Now, the point of the test is to make sure that the local cleanup happens, so I'd rather keep it as-is and use the same names across reloads. -- Michael signature.asc Description: PGP signature
Re: Speed up clean meson builds by ~25%
On Mon, 8 Apr 2024 at 10:02, Peter Eisentraut wrote: > I have tested this with various compilers, and I can confirm that this > shaves off about 5 seconds from the build wall-clock time, which > represents about 10%-20% of the total time. I think this is a good patch. Great to hear. > Interestingly, if I apply the analogous changes to the makefiles, I > don't get any significant improvements. (Depends on local > circumstances, of course.) But I would still suggest to keep the > makefiles aligned. Attached is a patch that also updates the Makefiles, but indeed I don't get any perf gains there either. On Mon, 8 Apr 2024 at 07:23, Michael Paquier wrote: > Well, this is also painful with ./configure. So, even if we are going > to move away from it at this point, we still need to support it for a > couple of years. It looks to me that letting the clang folks know > about the situation is the best way forward. I reported the issue to the clang folks: https://github.com/llvm/llvm-project/issues/87973 But even if my patch doesn't help for ./configure, it still seems like a good idea to me to still reduce compile times when using meson while we wait for clang folks to address the issue. v2-0001-Speed-up-clean-parallel-meson-builds-a-lot.patch Description: Binary data
Re: Speed up clean meson builds by ~25%
Hello Jelte, 08.04.2024 11:36, Jelte Fennema-Nio wrote: On Mon, 8 Apr 2024 at 10:00, Alexander Lakhin wrote: As I wrote in [1], I didn't observe the issue with clang-18, so maybe it is fixed already. Perhaps it's worth rechecking... Using the attached script I got these timings. Clang is significantly slower in all of them. But especially with -Og the difference between is huge. gcc 11.4.0: 7.276s clang 18.1.3: 17.216s gcc 11.4.0 --debug: 7.441s clang 18.1.3 --debug: 18.164s gcc 11.4.0 --debug -Og: 2.418s clang 18.1.3 --debug -Og: 14.864s I reported this same issue to the LLVM project here: https://github.com/llvm/llvm-project/issues/87973 Maybe we're talking about different problems. At [1] Thomas (and then I) was unhappy with more than 200 seconds duration... https://www.postgresql.org/message-id/CA%2BhUKGLvJ7-%3DfS-J9kN%3DaZWrpyqykwqCBbxXLEhUa9831dPFcg%40mail.gmail.com Best regards, Alexander
Re: remaining sql/json patches
On Mon, Apr 8, 2024 at 2:02 PM jian he wrote: > On Mon, Apr 8, 2024 at 11:21 AM jian he wrote: > > > > On Mon, Apr 8, 2024 at 12:34 AM jian he wrote: > > > > > > On Sun, Apr 7, 2024 at 9:36 PM Amit Langote > > > wrote: > > > > 0002 needs an expanded commit message but I've run out of energy today. > > > > > > > > other than that, it looks good to me. > > one more tiny issue. > +EXPLAIN (COSTS OFF, VERBOSE) SELECT * FROM jsonb_table_view1; > +ERROR: relation "jsonb_table_view1" does not exist > +LINE 1: EXPLAIN (COSTS OFF, VERBOSE) SELECT * FROM jsonb_table_view1... > + ^ > maybe you want > EXPLAIN (COSTS OFF, VERBOSE) SELECT * FROM jsonb_table_view7; > at the end of the sqljson_jsontable.sql. > I guess it will be fine, but the format json explain's out is quite big. > > you also need to `drop table s cascade;` at the end of the test? Pushed after fixing this and other issues. Thanks a lot for your careful reviews. I've marked the CF entry for this as committed now: https://commitfest.postgresql.org/47/4377/ Let's work on the remaining PLAN clause with a new entry in the next CF, possibly in a new email thread. -- Thanks, Amit Langote
Re: Speed up clean meson builds by ~25%
Hi, On Mon, 8 Apr 2024 at 11:00, Alexander Lakhin wrote: > > Hello Michael, > > 08.04.2024 08:23, Michael Paquier wrote: > > On Fri, Apr 05, 2024 at 06:19:20PM +0200, Jelte Fennema-Nio wrote: > >> Agreed. While not a full solution, I think this patch is still good to > >> address some of the pain: Waiting 10 seconds at the end of my build > >> with only 1 of my 10 cores doing anything. > >> > >> So while this doesn't decrease CPU time spent it does decrease > >> wall-clock time significantly in some cases, with afaict no downsides. > > Well, this is also painful with ./configure. So, even if we are going > > to move away from it at this point, we still need to support it for a > > couple of years. It looks to me that letting the clang folks know > > about the situation is the best way forward. > > > > As I wrote in [1], I didn't observe the issue with clang-18, so maybe it > is fixed already. > Perhaps it's worth rechecking... > > [1] > https://www.postgresql.org/message-id/d2bf3727-bae4-3aee-65f6-caec2c4ebaa8%40gmail.com I had this problem on my local computer. My build times are: gcc: 20s clang-15: 24s clang-16: 105s clang-17: 111s clang-18: 25s -- Regards, Nazir Bilal Yavuz Microsoft
Re: Weird test mixup
> On 8 Apr 2024, at 11:55, Michael Paquier wrote: > > the point of the test is to make sure that the local > cleanup happens Uh, I did not understand this. Because commit message was about stabiilzizing tests, not extending coverage. Also, should we drop function wait_pid() at the end of a test? Given that tweaks with are nothing new, I think patch looks good. Best regards, Andrey Borodin.
Re: Table AM Interface Enhancements
On Mon, Apr 8, 2024 at 10:18 AM Pavel Borisov wrote: > On Mon, 8 Apr 2024 at 03:25, Alexander Korotkov wrote: >> >> Hi, >> >> On Mon, Apr 8, 2024 at 12:40 AM Andres Freund wrote: >> > On 2024-03-30 23:33:04 +0200, Alexander Korotkov wrote: >> > > I've pushed 0001, 0002 and 0006. >> > >> > I briefly looked at 27bc1772fc81 and I don't think the state post this >> > commit >> > makes sense. Before this commit another block based AM could implement >> > analyze >> > without much code duplication. Now a large portion of analyze.c has to be >> > copied, because they can't stop acquire_sample_rows() from calling >> > heapam_scan_analyze_next_block(). >> > >> > I'm quite certain this will break a few out-of-core AMs in a way that can't >> > easily be fixed. >> >> I was under the impression there are not so many out-of-core table >> AMs, which have non-dummy analysis implementations. And even if there >> are some, duplicating acquire_sample_rows() isn't a big deal. >> >> But given your feedback, I'd like to propose to keep both options >> open. Turn back the block-level API for analyze, but let table-AM >> implement its own analyze function. Then existing out-of-core AMs >> wouldn't need to do anything (or probably just set the new API method >> to NULL). > > I think that providing both new and old interface functions for block-based > and non-block based custom am is an excellent compromise. > > The patch v1-0001-Turn-back.. is mainly an undo of part of the 27bc1772fc81 > that had turned off _analyze_next_tuple..analyze_next_block for external > callers. If some extensions are already adapted to the old interface > functions, they are free to still use it. Please, check this. Instead of keeping two APIs, it generalizes acquire_sample_rows(). The downside is change of AcquireSampleRowsFunc signature, which would need some changes in FDWs too. -- Regards, Alexander Korotkov v2-0001-Generalize-acquire_sample_rows.patch Description: Binary data
Re: Should consider materializing the cheapest inner path in consider_parallel_nestloop()
> On 27 Sep 2023, at 16:06, tender wang wrote: > >Do you have any comments or suggestions on this issue? Thanks. Hi Tender, there are some review comments in the thread, that you might be interested in. I'll mark this [0] entry "Waiting on Author" and move to next CF. Thanks! Best regards, Andrey Borodin. [0]https://commitfest.postgresql.org/47/4549/
Re: Catalog domain not-null constraints
On 21.03.24 12:23, Peter Eisentraut wrote:
All the examples in the tests append "value" to this, presumably by
analogy with CHECK constraints, but it looks as though anything works,
and is simply ignored:
ALTER DOMAIN d ADD CONSTRAINT nn NOT NULL xxx; -- works
That doesn't seem particularly satisfactory. I think it should not
require (and reject) a column name after "NOT NULL".
Hmm. CREATE DOMAIN uses column constraint syntax, but ALTER DOMAIN uses
table constraint syntax. As long as you are only dealing with CHECK
constraints, there is no difference, but it shows up when using NOT NULL
constraint syntax. I agree that this is unsatisfactory. Attached is a
patch to try to sort this out.
After studying this a bit more, I think moving forward in this direction
is the best way. Attached is a new patch version, mainly with a more
elaborate commit message. This patch makes the not-null constraint
syntax consistent between CREATE DOMAIN and ALTER DOMAIN, and also makes
the respective documentation correct.
(Note that, as I show in the commit message, commit e5da0fe3c22 had in
passing fixed a couple of bugs in CREATE and ALTER DOMAIN, so just
reverting that commit wouldn't be a complete solution.)
From 4e4de402dda81798bb0286a09d39c6ed2f087228 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 8 Apr 2024 11:39:48 +0200
Subject: [PATCH v2] Fix ALTER DOMAIN NOT NULL syntax
In CREATE DOMAIN, a NOT NULL constraint looks like
CREATE DOMAIN d1 AS int [ CONSTRAINT conname ] NOT NULL
(Before e5da0fe3c22, the constraint name was accepted but ignored.)
But in ALTER DOMAIN, a NOT NULL constraint looks like
ALTER DOMAIN d1 ADD [ CONSTRAINT conname ] NOT NULL VALUE
where VALUE is where for a table constraint the column name would be.
(This works as of e5da0fe3c22. Before e5da0fe3c22, this syntax
resulted in an internal error.)
But for domains, this latter syntax is confusing and needlessly
inconsistent between CREATE and ALTER. So this changes it to just
ALTER DOMAIN d1 ADD [ CONSTRAINT conname ] NOT NULL
(None of these syntaxes are per SQL standard; we are just living with
the bits of inconsistency that have built up over time.)
Discussion:
https://www.postgresql.org/message-id/flat/9ec24d7b-633d-463a-84c6-7acff769c9e8%40eisentraut.org
---
doc/src/sgml/ref/create_domain.sgml | 17 ++--
src/backend/parser/gram.y| 60 ++--
src/test/regress/expected/domain.out | 8 ++--
src/test/regress/sql/domain.sql | 8 ++--
4 files changed, 79 insertions(+), 14 deletions(-)
diff --git a/doc/src/sgml/ref/create_domain.sgml
b/doc/src/sgml/ref/create_domain.sgml
index 73f9f28d6cf..078bea8410d 100644
--- a/doc/src/sgml/ref/create_domain.sgml
+++ b/doc/src/sgml/ref/create_domain.sgml
@@ -24,9 +24,9 @@
CREATE DOMAIN name [ AS ]
data_type
[ COLLATE collation ]
[ DEFAULT expression ]
-[ constraint [ ... ] ]
+[ domain_constraint [ ... ] ]
-where constraint
is:
+where domain_constraint
is:
[ CONSTRAINT constraint_name ]
{ NOT NULL | NULL | CHECK (expression) }
@@ -190,7 +190,7 @@ Parameters
-
+
Notes
@@ -279,6 +279,17 @@ Compatibility
The command CREATE DOMAIN conforms to the SQL
standard.
+
+
+ The syntax NOT NULL in this command is a
+ PostgreSQL extension. (A standard-conforming
+ way to write the same would be CHECK (VALUE IS NOT
+ NULL). However, per ,
+ such constraints a best avoided in practice anyway.) The
+ NULL constraint is a
+ PostgreSQL extension (see also ).
+
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 0523f7e891e..e8b619926ef 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -524,7 +524,7 @@ static Node *makeRecursiveViewSelect(char *relname, List
*aliases, Node *query);
%type generic_set set_rest set_rest_more generic_reset reset_rest
SetResetClause FunctionSetResetClause
-%typeTableElement TypedTableElement ConstraintElem TableFuncElement
+%typeTableElement TypedTableElement ConstraintElem
DomainConstraintElem TableFuncElement
%typecolumnDef columnOptions optionalPeriodName
%type def_elem reloption_elem old_aggr_elem operator_def_elem
%typedef_arg columnElem where_clause where_or_current_clause
@@ -596,7 +596,7 @@ static Node *makeRecursiveViewSelect(char *relname, List
*aliases, Node *query);
%type col_name_keyword reserved_keyword
%type bare_label_keyword
-%typeTableConstraint TableLikeClause
+%typeDomainConstraint TableConstraint TableLikeClause
%typeTableLikeOptionList TableLikeOption
%type column_compression opt_column_compression
column_storage opt_column_storage
%typeColQualList
@@ -4334,6 +4334,60 @@ ConstraintElem:
}
;
+/*
+ * DomainConstraint is separate from TableConstraint because the syntax for
+ * NOT NULL constraints is diffe
Re: Table AM Interface Enhancements
On Mon, 8 Apr 2024 at 13:34, Alexander Korotkov wrote: > On Mon, Apr 8, 2024 at 10:18 AM Pavel Borisov > wrote: > > On Mon, 8 Apr 2024 at 03:25, Alexander Korotkov > wrote: > >> > >> Hi, > >> > >> On Mon, Apr 8, 2024 at 12:40 AM Andres Freund > wrote: > >> > On 2024-03-30 23:33:04 +0200, Alexander Korotkov wrote: > >> > > I've pushed 0001, 0002 and 0006. > >> > > >> > I briefly looked at 27bc1772fc81 and I don't think the state post > this commit > >> > makes sense. Before this commit another block based AM could > implement analyze > >> > without much code duplication. Now a large portion of analyze.c has > to be > >> > copied, because they can't stop acquire_sample_rows() from calling > >> > heapam_scan_analyze_next_block(). > >> > > >> > I'm quite certain this will break a few out-of-core AMs in a way that > can't > >> > easily be fixed. > >> > >> I was under the impression there are not so many out-of-core table > >> AMs, which have non-dummy analysis implementations. And even if there > >> are some, duplicating acquire_sample_rows() isn't a big deal. > >> > >> But given your feedback, I'd like to propose to keep both options > >> open. Turn back the block-level API for analyze, but let table-AM > >> implement its own analyze function. Then existing out-of-core AMs > >> wouldn't need to do anything (or probably just set the new API method > >> to NULL). > > > > I think that providing both new and old interface functions for > block-based and non-block based custom am is an excellent compromise. > > > > The patch v1-0001-Turn-back.. is mainly an undo of part of the > 27bc1772fc81 that had turned off _analyze_next_tuple..analyze_next_block > for external callers. If some extensions are already adapted to the old > interface functions, they are free to still use it. > > Please, check this. Instead of keeping two APIs, it generalizes > acquire_sample_rows(). The downside is change of > AcquireSampleRowsFunc signature, which would need some changes in FDWs > too. > To me, both approaches v1-0001-Turn-back... and v2-0001-Generalize... and patch v2 look good. Pavel.
Re: Security lessons from liblzma - libsystemd
Hi, > There are many more interesting and scary libraries in the dependency > tree of "postgres", so just picking off one right now doesn't really > accomplish anything. The next release of libsystemd will drop all > the compression libraries as hard dependencies, so the issue in that > sense is gone anyway. Also, fun fact: liblzma is also a dependency > via libxml2. Having an audit of all libraries linked to postgres and their level of trust should help to point the next weak point. I'm pretty sure we have several of these tiny libraries maintained by a lone out of time hacker linked somewhere. What is the next xz ? Regards, Étienne -- DALIBO
Re: Synchronizing slots from primary to standby
On Mon, Apr 8, 2024 at 12:19 PM Zhijie Hou (Fujitsu) wrote: > > On Saturday, April 6, 2024 12:43 PM Amit Kapila > wrote: > > On Fri, Apr 5, 2024 at 8:05 PM Bertrand Drouvot > > wrote: > > > > Yeah, that could be the first step. We can probably add an injection point > > to > > control the bgwrite behavior and then add tests involving walsender > > performing the decoding. But I think it is important to have sufficient > > tests in > > this area as I see they are quite helpful in uncovering the issues. > > Here is the patch to drop the subscription in the beginning so that the > restart_lsn of the lsub1_slot won't be advanced due to concurrent > xl_running_xacts from bgwriter. The subscription will be re-created after all > the slots are sync-ready. I think maybe we can use this to stabilize the test > as a first step and then think about how to make use of injection point to add > more tests if it's worth it. > Pushed. -- With Regards, Amit Kapila.
Re: Flushing large data immediately in pqcomm
On Sun, 7 Apr 2024 at 14:41, David Rowley wrote: > Looking at the code in socket_putmessage_noblock(), I don't understand > why it's ok for PqSendBufferSize to be int but "required" must be > size_t. There's a line that does "PqSendBufferSize = required;". It > kinda looks like they both should be size_t. Am I missing something > that you've thought about? You and Ranier are totally right (I missed this assignment). Attached is v8. v8-0001-Make-a-few-variables-size_t-in-pqcomm.c.patch Description: Binary data
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi all, I went through the MERGE/SPLIT partition codes today, thanks for the works. I found some grammar errors: i. in error messages(Users can see this grammar errors, not friendly). ii. in codes comments Alexander Korotkov 于2024年4月7日周日 06:23写道： > Hi, Dmitry! > > On Fri, Apr 5, 2024 at 4:00 PM Dmitry Koval > wrote: > > > I've revised the patchset. > > > > Thanks for the corrections (especially ddl.sgml). > > Could you also look at a small optimization for the MERGE PARTITIONS > > command (in a separate file > > v31-0003-Additional-patch-for-ALTER-TABLE-.-MERGE-PARTITI.patch, I wrote > > about it in an email 2024-03-31 00:56:50)? > > > > Files v31-0001-*.patch, v31-0002-*.patch are the same as > > v30-0001-*.patch, v30-0002-*.patch (after rebasing because patch stopped > > applying due to changes in upstream). > > I've pushed 0001 and 0002. I didn't push 0003 for the following reasons. > 1) This doesn't keep functionality equivalent to 0001. With 0003, the > merged partition will inherit indexes, constraints, and so on from the > one of merging partitions. > 2) This is not necessarily an optimization. Without 0003 indexes on > the merged partition are created after moving the rows in > attachPartitionTable(). With 0003 we merge data into the existing > partition which saves its indexes. That might cause a significant > performance loss because mass inserts into indexes may be much slower > than building indexes from scratch. > I think both aspects need to be carefully considered. Even if we > accept them, this needs to be documented. I think now it's too late > for both of these. So, this should wait for v18. > > -- > Regards, > Alexander Korotkov > > > -- Tender Wang OpenPie: https://en.openpie.com/ 0001-Fix-some-grammer-errors-from-error-messages-and-code.patch Description: Binary data
Re: Should consider materializing the cheapest inner path in consider_parallel_nestloop()
Andrey M. Borodin 于2024年4月8日周一 17:40写道： > > > > On 27 Sep 2023, at 16:06, tender wang wrote: > > > >Do you have any comments or suggestions on this issue? Thanks. > Hi Tender, > > there are some review comments in the thread, that you might be interested > in. > I'll mark this [0] entry "Waiting on Author" and move to next CF. > Thank you for the reminder. I will update the patch later. I also deeply hope to get more advice about this patch. (even the advice that not worth continuint to work on this patch). Thanks. Thanks! > > > Best regards, Andrey Borodin. > > [0]https://commitfest.postgresql.org/47/4549/ -- Tender Wang OpenPie: https://en.openpie.com/
Re: Table AM Interface Enhancements
Hi, Alexander On Mon, 8 Apr 2024 at 13:59, Pavel Borisov wrote: > > > On Mon, 8 Apr 2024 at 13:34, Alexander Korotkov > wrote: > >> On Mon, Apr 8, 2024 at 10:18 AM Pavel Borisov >> wrote: >> > On Mon, 8 Apr 2024 at 03:25, Alexander Korotkov >> wrote: >> >> >> >> Hi, >> >> >> >> On Mon, Apr 8, 2024 at 12:40 AM Andres Freund >> wrote: >> >> > On 2024-03-30 23:33:04 +0200, Alexander Korotkov wrote: >> >> > > I've pushed 0001, 0002 and 0006. >> >> > >> >> > I briefly looked at 27bc1772fc81 and I don't think the state post >> this commit >> >> > makes sense. Before this commit another block based AM could >> implement analyze >> >> > without much code duplication. Now a large portion of analyze.c has >> to be >> >> > copied, because they can't stop acquire_sample_rows() from calling >> >> > heapam_scan_analyze_next_block(). >> >> > >> >> > I'm quite certain this will break a few out-of-core AMs in a way >> that can't >> >> > easily be fixed. >> >> >> >> I was under the impression there are not so many out-of-core table >> >> AMs, which have non-dummy analysis implementations. And even if there >> >> are some, duplicating acquire_sample_rows() isn't a big deal. >> >> >> >> But given your feedback, I'd like to propose to keep both options >> >> open. Turn back the block-level API for analyze, but let table-AM >> >> implement its own analyze function. Then existing out-of-core AMs >> >> wouldn't need to do anything (or probably just set the new API method >> >> to NULL). >> > >> > I think that providing both new and old interface functions for >> block-based and non-block based custom am is an excellent compromise. >> > >> > The patch v1-0001-Turn-back.. is mainly an undo of part of the >> 27bc1772fc81 that had turned off _analyze_next_tuple..analyze_next_block >> for external callers. If some extensions are already adapted to the old >> interface functions, they are free to still use it. >> >> Please, check this. Instead of keeping two APIs, it generalizes >> acquire_sample_rows(). The downside is change of >> AcquireSampleRowsFunc signature, which would need some changes in FDWs >> too. >> > To me, both approaches v1-0001-Turn-back... and v2-0001-Generalize... and > patch v2 look good. > > Pavel. > I added some changes in comments to better reflect changes in patch v2. See a patch v3 (code unchanged from v2) Regards, Pavel v3-0001-Generalize-acquire_sample_rows.patch Description: Binary data
Re: Why is parula failing?
On Tue, 2 Apr 2024 at 15:01, Tom Lane wrote: > "Tharakan, Robins" writes: > > So although HEAD ran fine, but I saw multiple failures (v12, v13, v16) all of which passed on subsequent-tries, > > of which some were even"signal 6: Aborted". > > Ugh... parula didn't send any reports to buildfarm for the past 44 hours. Logged in to see that postgres was stuck on pg_sleep(), which was quite odd! I captured the backtrace and triggered another run on HEAD, which came out okay. I'll keep an eye on this instance more often for the next few days. (Let me know if I could capture more if a run gets stuck again) (gdb) bt #0 0x952ae954 in epoll_pwait () from /lib64/libc.so.6 #1 0x0083e9c8 in WaitEventSetWaitBlock (nevents=1, occurred_events=, cur_timeout=297992, set=0x2816dac0) at latch.c:1570 #2 WaitEventSetWait (set=0x2816dac0, timeout=timeout@entry=60, occurred_events=occurred_events@entry=0xc395ed28, nevents=nevents@entry=1, wait_event_info=wait_event_info@entry=150994946) at latch.c:1516 #3 0x0083ed84 in WaitLatch (latch=, wakeEvents=wakeEvents@entry=41, timeout=60, wait_event_info=wait_event_info@entry=150994946) at latch.c:538 #4 0x00907404 in pg_sleep (fcinfo=) at misc.c:406 #5 0x00696b10 in ExecInterpExpr (state=0x28384040, econtext=0x28383e38, isnull=) at execExprInterp.c:764 #6 0x006ceef8 in ExecEvalExprSwitchContext (isNull=0xc395ee9f, econtext=0x28383e38, state=) at ../../../src/include/executor/executor.h:356 #7 ExecProject (projInfo=) at ../../../src/include/executor/executor.h:390 #8 ExecResult (pstate=) at nodeResult.c:135 #9 0x006b7aec in ExecProcNode (node=0x28383d28) at ../../../src/include/executor/executor.h:274 #10 gather_getnext (gatherstate=0x28383b38) at nodeGather.c:287 #11 ExecGather (pstate=0x28383b38) at nodeGather.c:222 #12 0x0069aa4c in ExecProcNode (node=0x28383b38) at ../../../src/include/executor/executor.h:274 #13 ExecutePlan (execute_once=, dest=0x2831ffb0, direction=, numberTuples=0, sendTuples=, operation=CMD_SELECT, use_parallel_mode=, planstate=0x28383b38, estate=0x28383910) at execMain.c:1646 #14 standard_ExecutorRun (queryDesc=0x283239c0, direction=, count=0, execute_once=) at execMain.c:363 #15 0x0086d454 in PortalRunSelect (portal=portal@entry=0x281f0fb0, forward=forward@entry=true, count=0, count@entry=9223372036854775807, dest=dest@entry=0x2831ffb0) at pquery.c:924 #16 0x0086ec70 in PortalRun (portal=portal@entry=0x281f0fb0, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true, dest=dest@entry=0x2831ffb0, altdest=altdest@entry=0x2831ffb0, qc=qc@entry=0xc395f250) at pquery.c:768 #17 0x0086a944 in exec_simple_query (query_string=query_string@entry=0x28171c90 "SELECT pg_sleep(0.1);") at postgres.c:1274 #18 0x0086b480 in PostgresMain (dbname=, username=) at postgres.c:4680 #19 0x00866a0c in BackendMain (startup_data=, startup_data_len=) at backend_startup.c:101 #20 0x007c1738 in postmaster_child_launch (child_type=child_type@entry=B_BACKEND, startup_data=startup_data@entry=0xc395f718 "", startup_data_len=startup_data_len@entry=4, client_sock=client_sock@entry=0xc395f720) at launch_backend.c:265 #21 0x007c5120 in BackendStartup (client_sock=0xc395f720) at postmaster.c:3593 #22 ServerLoop () at postmaster.c:1674 #23 0x007c6dc8 in PostmasterMain (argc=argc@entry=8, argv=argv@entry=0x2816d320) at postmaster.c:1372 #24 0x00496bb8 in main (argc=8, argv=0x2816d320) at main.c:197 > > The update_personality.pl script in the buildfarm client distro > is what to use to adjust OS version or compiler version data. > Thanks. Fixed that. - robins
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi Tender Wang, 08.04.2024 13:43, Tender Wang wrote: Hi all, I went through the MERGE/SPLIT partition codes today, thanks for the works. I found some grammar errors: i. in error messages(Users can see this grammar errors, not friendly). ii. in codes comments On a quick glance, I saw also: NULL-value partitionde splited temparary And a trailing whitespace at: the quarter partition back to monthly partitions: warning: 1 line adds whitespace errors. I'm also confused by "administrators" here: https://www.postgresql.org/docs/devel/ddl-partitioning.html (We can find on the same page, for instance: ... whereas table inheritance allows data to be divided in a manner of the user's choosing. It seems to me, that "users" should work for merging partitions as well.) Though the documentation addition requires more than just a quick glance, of course. Best regards, Alexander
Re: Flushing large data immediately in pqcomm
Em seg., 8 de abr. de 2024 às 07:42, Jelte Fennema-Nio escreveu: > On Sun, 7 Apr 2024 at 14:41, David Rowley wrote: > > Looking at the code in socket_putmessage_noblock(), I don't understand > > why it's ok for PqSendBufferSize to be int but "required" must be > > size_t. There's a line that does "PqSendBufferSize = required;". It > > kinda looks like they both should be size_t. Am I missing something > > that you've thought about? > > > You and Ranier are totally right (I missed this assignment). Attached is > v8. > +1 LGTM. best regards, Ranier Vilela
Re: Add last_commit_lsn to pg_stat_database
Hi James, There are some review in the thread that need to be addressed. it seems that we need to mark this entry "Waiting on Author" and move to the next CF [0]. Thanks [0] https://commitfest.postgresql.org/47/4355/ On Sat, 10 Jun 2023 at 05:27, James Coleman wrote: > I've previously noted in "Add last commit LSN to > pg_last_committed_xact()" [1] that it's not possible to monitor how > many bytes of WAL behind a logical replication slot is (computing such > is obviously trivial for physical slots) because the slot doesn't need > to replicate beyond the last commit. In some cases it's possible for > the current WAL location to be far beyond the last commit. A few > examples: > > - An idle server with checkout_timeout at a lower value (so lots of > empty WAL advance). > - Very large transactions: particularly insidious because committing a > 1 GB transaction after a small transaction may show almost zero time > lag even though quite a bit of data needs to be processed and sent > over the wire (so time to replay is significantly different from > current lag). > - A cluster with multiple databases complicates matters further, > because while physical replication is cluster-wide, the LSNs that > matter for logical replication are database specific. > > Since we don't expose the most recent commit's LSN there's no way to > say "the WAL is currently 1250, the last commit happened at 1000, the > slot has flushed up to 800, therefore there are at most 200 bytes > replication needs to read through to catch up. > > In the aforementioned thread [1] I'd proposed a patch that added a SQL > function pg_last_commit_lsn() to expose the most recent commit's LSN. > Robert Haas didn't think the initial version's modifications to > commit_ts made sense, and a subsequent approach adding the value to > PGPROC didn't have strong objections, from what I can see, but it also > didn't generate any enthusiasm. > > As I was thinking about how to improve things, I realized that this > information (since it's for monitoring anyway) fits more naturally > into the stats system. I'd originally thought of exposing it in > pg_stat_wal, but that's per-cluster rather than per-database (indeed, > this is a flaw I hadn't considered in the original patch), so I think > pg_stat_database is the correct location. > > I've attached a patch to track the latest commit's LSN in pg_stat_database. > > Regards, > James Coleman > > 1: > https://www.postgresql.org/message-id/flat/caaaqye9qbiau+j8rbun_jkbre-3hekluhfvvsyfsxqg0vql...@mail.gmail.com >
Re: [PoC] Improve dead tuple storage for lazy vacuum
Hi, John! On Mon, 8 Apr 2024 at 03:13, John Naylor wrote: > On Mon, Apr 8, 2024 at 2:07 AM Andres Freund wrote: > > > > Looking at the code, the failure isn't suprising anymore: > > chardata[MaxBlocktableEntrySize]; > > BlocktableEntry *page = (BlocktableEntry *) data; > > > > 'char' doesn't enforce any alignment, but you're storing a > BlocktableEntry in > > a char[]. You can't just do that. Look at how we do that for > > e.g. PGAlignedblock. > > > > > > With the attached minimal fix, the tests pass again. > > Thanks, will push this shortly! > Buildfarm animal mylodon looks unhappy with this: FAILED: src/backend/postgres_lib.a.p/access_common_tidstore.c.o ccache clang-14 -Isrc/backend/postgres_lib.a.p -Isrc/include -I../pgsql/src/include -I/usr/include/libxml2 -I/usr/include/security -fdiagnostics-color=never -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O2 -g -fno-strict-aliasing -fwrapv -D_GNU_SOURCE -Wmissing-prototypes -Wpointer-arith -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security -Wdeclaration-after-statement -Wno-unused-command-line-argument -Wno-compound-token-split-by-macro -O1 -ggdb -g3 -fno-omit-frame-pointer -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wno-missing-field-initializers -Wno-array-bounds -std=c99 -Wc11-extensions -Werror=c11-extensions -fPIC -isystem /usr/include/mit-krb5 -pthread -DBUILDING_DLL -MD -MQ src/backend/postgres_lib.a.p/access_common_tidstore.c.o -MF src/backend/postgres_lib.a.p/access_common_tidstore.c.o.d -o src/backend/postgres_lib.a.p/access_common_tidstore.c.o -c ../pgsql/src/backend/access/common/tidstore.c ../pgsql/src/backend/access/common/tidstore.c:48:3: error: anonymous structs are a C11 extension [-Werror,-Wc11-extensions] struct ^ 1 error generated. Regards, Pavel Borisov Supabase
Re: [PoC] Improve dead tuple storage for lazy vacuum
On Sun, Apr 7, 2024 at 9:08 AM John Naylor wrote:
>
> I've attached a mostly-polished update on runtime embeddable values,
> storing up to 3 offsets in the child pointer (1 on 32-bit platforms).
> As discussed, this includes a macro to cap max possible offset that
> can be stored in the bitmap, which I believe only reduces the valid
> offset range for 32kB pages on 32-bit platforms. Even there, it allows
> for more line pointers than can possibly be useful. It also splits
> into two parts for readability. It would be committed in two pieces as
> well, since they are independently useful.
I pushed both of these and see that mylodon complains that anonymous
unions are a C11 feature. I'm not actually sure that the union with
uintptr_t is actually needed, though, since that's not accessed as
such here. The simplest thing seems to get rid if the union and name
the inner struct "header", as in the attached.
diff --git a/src/backend/access/common/tidstore.c b/src/backend/access/common/tidstore.c
index cddbaf013b..78730797d6 100644
--- a/src/backend/access/common/tidstore.c
+++ b/src/backend/access/common/tidstore.c
@@ -43,35 +43,30 @@
*/
typedef struct BlocktableEntry
{
- union
+ struct
{
- struct
- {
#ifndef WORDS_BIGENDIAN
- /*
- * We need to position this member so that the backing radix tree
- * can use the lowest bit for a pointer tag. In particular, it
- * must be placed within 'header' so that it corresponds to the
- * lowest byte in 'ptr'. We position 'nwords' along with it to
- * avoid struct padding.
- */
- uint8 flags;
-
- int8 nwords;
+ /*
+ * We need to position this member so that the backing radix tree can
+ * use the lowest bit for a pointer tag. In particular, it must be
+ * placed within 'header' so that it corresponds to the lowest byte in
+ * 'ptr'. We position 'nwords' along with it to avoid struct padding.
+ */
+ uint8 flags;
+
+ int8 nwords;
#endif
- /*
- * We can store a small number of offsets here to avoid wasting
- * space with a sparse bitmap.
- */
- OffsetNumber full_offsets[NUM_FULL_OFFSETS];
+ /*
+ * We can store a small number of offsets here to avoid wasting space
+ * with a sparse bitmap.
+ */
+ OffsetNumber full_offsets[NUM_FULL_OFFSETS];
#ifdef WORDS_BIGENDIAN
- int8 nwords;
- uint8 flags;
+ int8 nwords;
+ uint8 flags;
#endif
- };
- uintptr_t ptr;
} header;
bitmapword words[FLEXIBLE_ARRAY_MEMBER];
Re: Flushing large data immediately in pqcomm
On Sun, 7 Apr 2024 at 11:34, David Rowley wrote: > That seems to require modifying the following function signatures: > secure_write(), be_tls_write(), be_gssapi_write(). That's not an area > I'm familiar with, however. Attached is a new patchset where 0003 does exactly that. The only place where we need to cast to non-const is for GSS, but that seems fine (commit message has more details). I also added patch 0002, which is a small addition to the function comment of internal_flush_buffer that seemed useful to me to differentiate it from internal_flush (feel free to ignore/rewrite). v9-0001-Make-a-few-variables-size_t-in-pqcomm.c.patch Description: Binary data v9-0002-Expand-comment-of-internal_flush_buffer.patch Description: Binary data v9-0003-Make-backend-libpq-write-functions-take-const-poi.patch Description: Binary data
Re: Improve eviction algorithm in ReorderBuffer
On Sat, Apr 6, 2024 at 5:44 AM Jeff Davis wrote:
>
> >
> > It sounds like a data structure that mixes the hash table and the
> > binary heap and we use it as the main storage (e.g. for
> > ReorderBufferTXN) instead of using the binary heap as the secondary
> > data structure. IIUC with your idea, the indexed binary heap has a
> > hash table to store elements each of which has its index within the
> > heap node array. I guess it's better to create it as a new data
> > structure rather than extending the existing binaryheap, since APIs
> > could be very different. I might be missing something, though.
>
> You are right that this approach starts to feel like a new data
> structure and is not v17 material.
>
> I am interested in this for v18 though -- we could make the API more
> like simplehash to be more flexible when using values (rather than
> pointers) and to be able to inline the comparator.
Interesting project. It would be great if we could support increasing
and decreasing the key as APIs. The current
binaryheap_update_{up|down} APIs are not very user-friendly.
>
> > > * remove the hash table from binaryheap.c
> > >
> > > * supply a new callback to the binary heap with type like:
> > >
> > > typedef void (*binaryheap_update_index)(
> > > bh_node_type node,
> > > int new_element_index);
> > >
> > > * make the remove, update_up, and update_down methods take the
> > > element
> > > index rather than the pointer
>
> ...
>
> > This shows that the current indexed binaryheap is much slower than
> > the
> > other two implementations, and the xx_binaryheap has a good number in
> > spite of also being indexed.
>
> xx_binaryheap isn't indexed though, right?
Well, yes. To be xact, xx_binaryheap isn't indexed but the element
indexes are stored in the element itself (see test_elem struct) so the
caller still can update the key using xx_binaryheap_update_{up|down}.
>
> > When it comes to
> > implementing the above idea (i.e. changing binaryheap to
> > xx_binaryheap), it was simple since we just replace the code where we
> > update the hash table with the code where we call the callback, if we
> > get the consensus on API change.
>
> That seems reasonable to me.
>
> > The fact that we use simplehash for the internal hash table might
> > make
> > this idea complex. If I understand your suggestion correctly, the
> > caller needs to tell the hash table the hash function when creating a
> > binaryheap but the hash function needs to be specified at a compile
> > time. We can use a dynahash instead but it would make the binaryheap
> > slow further.
>
> simplehash.h supports private_data, which makes it easier to track a
> callback.
>
> In binaryheap.c, that would look something like:
>
> static inline uint32
> binaryheap_hash(bh_nodeidx_hash *tab, uint32 key)
> {
> binaryheap_hashfunc hashfunc = tab->private_data;
> return hashfunc(key);
> }
>
> ...
> #define SH_HASH_KEY(tb, key) binaryheap_hash(tb, key)
> ...
>
> binaryheap_allocate(int num_nodes, binaryheap_comparator compare,
> void *arg, binaryheap_hashfunc hashfunc)
> {
> ...
> if (hashfunc != NULL)
> {
>/* could have a new structure, but we only need to
> * store one pointer, so don't bother with palloc/pfree */
>void *private_data = (void *)hashfunc;
>heap->bh_nodeidx = bh_nodeidx_create(..., private_data);
>...
>
>
> And in reorderbuffer.c, define the callback like:
>
> static uint32
> reorderbuffer_xid_hash(TransactionId xid)
> {
> /* fasthash32 is 'static inline' so may
> * be faster than hash_bytes()? */
> return fasthash32(&xid, sizeof(TransactionId), 0);
> }
>
Thanks, that's a good idea.
>
>
> In summary, there are two viable approaches for addressing the concerns
> in v17:
>
> 1. Provide callback to update ReorderBufferTXN->heap_element_index, and
> use that index (rather than the pointer) for updating the heap key
> (transaction size) or removing elements from the heap.
>
> 2. Provide callback for hashing, so that binaryheap.c can hash the xid
> value rather than the pointer.
>
> I don't have a strong opinion about which one to use. I prefer
> something closer to #1 for v18, but for v17 I suggest whichever one
> comes out simpler.
I've implemented prototypes of both ideas, and attached the draft patches.
I agree with you that something closer to #1 is for v18. Probably we
can implement the #1 idea while making binaryheap codes template like
simplehash.h. For v17, changes for #2 are smaller, but I'm concerned
that the new API that requires a hash function to be able to use
binaryheap_update_{up|down} might not be user friendly. In terms of
APIs, I prefer #1 idea. And changes for #1 can make the binaryheap
code simple, although it requires adding a variable in
ReorderBufferTXN instead. But overall, it can remove the hash table
and some functions so it looks better to me.
When it comes to performance overhead, I mentioned
Re: [PoC] Improve dead tuple storage for lazy vacuum
On Mon, 8 Apr 2024 at 16:27, John Naylor wrote: > On Sun, Apr 7, 2024 at 9:08 AM John Naylor > wrote: > > > > I've attached a mostly-polished update on runtime embeddable values, > > storing up to 3 offsets in the child pointer (1 on 32-bit platforms). > > As discussed, this includes a macro to cap max possible offset that > > can be stored in the bitmap, which I believe only reduces the valid > > offset range for 32kB pages on 32-bit platforms. Even there, it allows > > for more line pointers than can possibly be useful. It also splits > > into two parts for readability. It would be committed in two pieces as > > well, since they are independently useful. > > I pushed both of these and see that mylodon complains that anonymous > unions are a C11 feature. I'm not actually sure that the union with > uintptr_t is actually needed, though, since that's not accessed as > such here. The simplest thing seems to get rid if the union and name > the inner struct "header", as in the attached. > Provided uintptr_t is not accessed it might be good to get rid of it. Maybe this patch also need correction in this: +#define NUM_FULL_OFFSETS ((sizeof(uintptr_t) - sizeof(uint8) - sizeof(int8)) / sizeof(OffsetNumber)) Regards, Pavel
Re: [PoC] Improve dead tuple storage for lazy vacuum
On Mon, Apr 8, 2024 at 7:42 PM Pavel Borisov wrote: > >> I pushed both of these and see that mylodon complains that anonymous >> unions are a C11 feature. I'm not actually sure that the union with >> uintptr_t is actually needed, though, since that's not accessed as >> such here. The simplest thing seems to get rid if the union and name >> the inner struct "header", as in the attached. > > > Provided uintptr_t is not accessed it might be good to get rid of it. > > Maybe this patch also need correction in this: > +#define NUM_FULL_OFFSETS ((sizeof(uintptr_t) - sizeof(uint8) - sizeof(int8)) > / sizeof(OffsetNumber)) For full context the diff was -#define NUM_FULL_OFFSETS ((sizeof(bitmapword) - sizeof(uint16)) / sizeof(OffsetNumber)) +#define NUM_FULL_OFFSETS ((sizeof(uintptr_t) - sizeof(uint8) - sizeof(int8)) / sizeof(OffsetNumber)) I wanted the former, from f35bd9bf35 , to be independently useful (in case the commit in question had some unresolvable issue), and its intent is to fill struct padding when the array of bitmapword happens to have length zero. Changing to uintptr_t for the size calculation reflects the intent to fit in a (local) pointer, regardless of the size of a bitmapword. (If a DSA pointer happens to be a different size for some odd platform, it should still work, BTW.) My thinking with the union was, for big-endian, to force the 'flags' member to where it can be set, but thinking again, it should still work if by happenstance the header was smaller than the child pointer: A different bit would get tagged, but I believe that's irrelevant. The 'flags' member makes sure a byte is reserved for the tag, but it may not be where the tag is actually located, if that makes sense.
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Sun, Apr 7, 2024 at 6:50 PM Michael Paquier wrote: > And, as of the moment of typing this email, I get: > =# select '2024-04-08 00:00-12:00' - now() as time_remaining; > time_remaining > - > 13:10:35.688134 > (1 row) > > So there is just a bit more than half a day remaining before the > feature freeze is in effect. OK, so feature freeze is now in effect, then. In the future, let's use GMT for these deadlines. There have got to be a lot more people who can easily understand when a certain GMT timestamp falls in their local timezone than there are people who can easily understand when a certain AOE timestamp falls in their local timezone. And maybe we need to think of a way to further mitigate this crush of last minute commits. e.g. In the last week, you can't have more feature commits, or more lines of insertions in your commits, than you did in the prior 3 weeks combined. I don't know. I think this mad rush of last-minute commits is bad for the project. -- Robert Haas EDB: http://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-04-07 Su 20:58, Tom Lane wrote: Coverity complained that this patch leaks memory: /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_combinebackup/load_manifest.c: 212 in load_backup_manifest() 206 bytes_left -= rc; 207 json_parse_manifest_incremental_chunk( 208 inc_state, buffer, rc, bytes_left == 0); 209 } 210 211 close(fd); CID 1596259:(RESOURCE_LEAK) Variable "inc_state" going out of scope leaks the storage it points to. 212 } 213 214 /* All done. */ 215 pfree(buffer); 216 return result; 217 } /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c: 488 in parse_manifest_file() 482 bytes_left -= rc; 483 json_parse_manifest_incremental_chunk( 484 inc_state, buffer, rc, bytes_left == 0); 485 } 486 487 close(fd); CID 1596257:(RESOURCE_LEAK) Variable "inc_state" going out of scope leaks the storage it points to. 488 } 489 490 /* Done with the buffer. */ 491 pfree(buffer); 492 493 return result; It's right about that AFAICS, and not only is the "inc_state" itself leaked but so is its assorted infrastructure. Perhaps we don't care too much about that in the existing applications, but ISTM that isn't going to be a tenable assumption across the board. Shouldn't there be a "json_parse_manifest_incremental_shutdown()" or the like to deallocate all the storage allocated by the parser? yeah, probably. Will work on it. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
RE: Synchronizing slots from primary to standby
On Monday, April 8, 2024 6:32 PM Amit Kapila wrote:
>
> On Mon, Apr 8, 2024 at 12:19 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Saturday, April 6, 2024 12:43 PM Amit Kapila
> wrote:
> > > On Fri, Apr 5, 2024 at 8:05 PM Bertrand Drouvot
> > > wrote:
> > >
> > > Yeah, that could be the first step. We can probably add an injection
> > > point to control the bgwrite behavior and then add tests involving
> > > walsender performing the decoding. But I think it is important to
> > > have sufficient tests in this area as I see they are quite helpful in
> > > uncovering
> the issues.
> >
> > Here is the patch to drop the subscription in the beginning so that
> > the restart_lsn of the lsub1_slot won't be advanced due to concurrent
> > xl_running_xacts from bgwriter. The subscription will be re-created
> > after all the slots are sync-ready. I think maybe we can use this to
> > stabilize the test as a first step and then think about how to make
> > use of injection point to add more tests if it's worth it.
> >
>
> Pushed.
Thanks for pushing.
I checked the BF status, and noticed one BF failure, which I think is related to
a miss in the test code.
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=adder&dt=2024-04-08%2012%3A04%3A27
From the following log, I can see the sync failed because the standby is
lagging behind of the failover slot.
-
# No postmaster PID for node "cascading_standby"
error running SQL: 'psql::1: ERROR: skipping slot synchronization as
the received slot sync LSN 0/4000148 for slot "snap_test_slot" is ahead of the
standby position 0/4000114'
while running 'psql -XAtq -d port=50074 host=/tmp/t4HQFlrDmI dbname='postgres'
-f - -v ON_ERROR_STOP=1' with sql 'SELECT pg_sync_replication_slots();' at
/home/bf/bf-build/adder/HEAD/pgsql/src/test/perl/PostgreSQL/Test/Cluster.pm
line 2042.
# Postmaster PID for node "publisher" is 3715298
-
I think it's because we missed to call wait_for_replay_catchup before syncing
slots.
-
$primary->safe_psql('postgres',
"SELECT pg_create_logical_replication_slot('snap_test_slot',
'test_decoding', false, false, true);"
);
# ? missed to wait here
$standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
-
While testing, I noticed another place where we were calling
wait_for_replay_catchup before doing pg_replication_slot_advance, which also has
a small possibility to cause the failover slot to be ahead of the standby if
some logs are written in between these two steps. So, I adjusted them together.
Here is a small patch to improve the test.
Best Regards,
Hou zj
0001-Ensure-the-standby-is-not-lagging-behind-the-failove.patch
Description: 0001-Ensure-the-standby-is-not-lagging-behind-the-failove.patch
Re: PostgreSQL 17 Release Management Team & Feature Freeze
Robert Haas writes: > And maybe we need to think of a way to further mitigate this crush of > last minute commits. e.g. In the last week, you can't have more > feature commits, or more lines of insertions in your commits, than you > did in the prior 3 weeks combined. I don't know. I think this mad rush > of last-minute commits is bad for the project. I was just about to pen an angry screed along the same lines. The commit flux over the past couple days, and even the last twelve hours, was flat-out ridiculous. These patches weren't ready a week ago, and I doubt they were ready now. The RMT should feel free to exercise their right to require revert "early and often", or we are going to be shipping a very buggy release. regards, tom lane
Re: documentation structure
On 05.04.24 17:11, Robert Haas wrote: 4. Consolidate the "Generic WAL Records" and "Custom WAL Resource Managers" chapters, which cover related topics, into a single one. I didn't see anyone object to this, but David Johnston pointed out that the patch I posted was a few bricks short of a load, because it really needed to put some introductory text into the new chapter. I'll study this a bit more and propose a new patch that does the same thing a bit more carefully than my previous version did. Here is a new version of this patch. I think this is v18 material at this point, absent an outcry to the contrary. Sometimes we're flexible about doc patches. Looks good to me. I think this could go into PG17.
Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs
Tom Lane wrote: > I've reconsidered after realizing that implementing FETCH_COUNT > atop traditional single-row mode would require either merging > single-row results into a bigger PGresult or persuading psql's > results-printing code to accept an array of PGresults not just > one. Either of those would be expensive and ugly, not to mention > needing chunks of code we don't have today. Yes, we must accumulate results because the aligned format needs to know the columns widths for a entire "page", and the row-by-row logic does not fit that well in that case. One of the posted patches implemented this with an array of PGresult in single-row mode [1] but I'm confident that the newer version you pushed with the libpq changes is a better approach. > So I whacked the patch around till I liked it better, and pushed it. Thanks for taking care of this! [1] https://www.postgresql.org/message-id/092583fb-97c5-428f-8d99-fd31be4a5...@manitou-mail.org Best regards, -- Daniel Vérité https://postgresql.verite.pro/ Twitter: @DanielVerite
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Mon, Apr 8, 2024 at 9:26 AM Robert Haas wrote: > > On Sun, Apr 7, 2024 at 6:50 PM Michael Paquier wrote: > > And, as of the moment of typing this email, I get: > > =# select '2024-04-08 00:00-12:00' - now() as time_remaining; > > time_remaining > > - > > 13:10:35.688134 > > (1 row) > > > > So there is just a bit more than half a day remaining before the > > feature freeze is in effect. > > OK, so feature freeze is now in effect, then. > > In the future, let's use GMT for these deadlines. There have got to be > a lot more people who can easily understand when a certain GMT > timestamp falls in their local timezone than there are people who can > easily understand when a certain AOE timestamp falls in their local > timezone. > > And maybe we need to think of a way to further mitigate this crush of > last minute commits. e.g. In the last week, you can't have more > feature commits, or more lines of insertions in your commits, than you > did in the prior 3 weeks combined. I don't know. I think this mad rush > of last-minute commits is bad for the project. What if we pick the actual feature freeze time randomly? That is, starting on March 15th (or whenever but more than a week before), each night someone from RMT generates a random number between $current_day and April 8th. If the number matches $current_day, that day at midnight is the feature freeze. - Melanie
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Mon, Apr 8, 2024 at 10:27 AM Melanie Plageman wrote: > On Mon, Apr 8, 2024 at 9:26 AM Robert Haas wrote: > > On Sun, Apr 7, 2024 at 6:50 PM Michael Paquier wrote: > > > And, as of the moment of typing this email, I get: > > > =# select '2024-04-08 00:00-12:00' - now() as time_remaining; > > > time_remaining > > > - > > > 13:10:35.688134 > > > (1 row) > > > > > > So there is just a bit more than half a day remaining before the > > > feature freeze is in effect. > > > > OK, so feature freeze is now in effect, then. > > > > In the future, let's use GMT for these deadlines. There have got to be > > a lot more people who can easily understand when a certain GMT > > timestamp falls in their local timezone than there are people who can > > easily understand when a certain AOE timestamp falls in their local > > timezone. > > > > And maybe we need to think of a way to further mitigate this crush of > > last minute commits. e.g. In the last week, you can't have more > > feature commits, or more lines of insertions in your commits, than you > > did in the prior 3 weeks combined. I don't know. I think this mad rush > > of last-minute commits is bad for the project. > > What if we pick the actual feature freeze time randomly? That is, > starting on March 15th (or whenever but more than a week before), each > night someone from RMT generates a random number between $current_day > and April 8th. If the number matches $current_day, that day at > midnight is the feature freeze. > Unfortunately many humans are hardwired towards procrastination and last minute heroics (it's one reason why deadline driven development works even though in the long run it tends to be bad practice), and historically was one of the driving factors in why we started doing commitfests in the first place (you should have seen the mad dash of commits before we had that process), so ISTM that it's not completely avoidable... That said, are you suggesting that the feature freeze deadline be random, and also held in secret by the RMT, only to be announced after the freeze time has passed? This feels weird, but might apply enough deadline related pressure while avoiding last minute shenanigans. Robert Treat https://xzilla.net
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Mon, 2024-04-08 at 09:26 -0400, Robert Haas wrote: > And maybe we need to think of a way to further mitigate this crush of > last minute commits. e.g. In the last week, you can't have more > feature commits, or more lines of insertions in your commits, than you > did in the prior 3 weeks combined. I don't know. I think this mad rush > of last-minute commits is bad for the project. I found that there are a lot of people who can only get going with a pressing deadline. But that's just an observation, not an excuse. I don't know if additional rules will achieve anything here. This can only improve with buy-in from the committers, and that cannot be forced. Yours, Laurenz Albe
Re: PostgreSQL 17 Release Management Team & Feature Freeze
> On 8 Apr 2024, at 17:26, Melanie Plageman wrote: > > What if we pick the actual feature freeze time randomly? That is, > starting on March 15th (or whenever but more than a week before), each > night someone from RMT generates a random number between $current_day > and April 8th. If the number matches $current_day, that day at > midnight is the feature freeze. But this implies that actual date is not publicly known before feature freeze is in effect. Do I understand idea correctly? Best regards, Andrey Borodin.
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On 08/04/2024 16:43, Tom Lane wrote: Robert Haas writes: And maybe we need to think of a way to further mitigate this crush of last minute commits. e.g. In the last week, you can't have more feature commits, or more lines of insertions in your commits, than you did in the prior 3 weeks combined. I don't know. I think this mad rush of last-minute commits is bad for the project. I was just about to pen an angry screed along the same lines. The commit flux over the past couple days, and even the last twelve hours, was flat-out ridiculous. These patches weren't ready a week ago, and I doubt they were ready now. The RMT should feel free to exercise their right to require revert "early and often", or we are going to be shipping a very buggy release. Can you elaborate, which patches you think were not ready? Let's make sure to capture any concrete concerns in the Open Items list. I agree the last-minute crunch felt more intense than in previous years. I'm guilty myself; I crunched the "direct SSL" patches in. My rationale for that one: It's been in a pretty settled state for a long time. There hasn't been any particular concerns about the design or the implementation. I haven't commit tit sooner because I was not comfortable with the lack of tests, especially the libpq parts. So I made a last minute dash to write the tests so that I'm comfortable with it, and I restructured the commits so that the tests and refactorings come first. The resulting code changes look the same they have for a long time, and I didn't uncover any significant new issues while doing that. I would not have felt comfortable committing it otherwise. Yeah, I should have done that sooner, but realistically, there's nothing like a looming deadline as a motivator. One idea to avoid the mad rush in the future would be to make the feature freeze deadline more progressive. For example: April 1: If you are still working on a feature that you still want to commit, you must explicitly flag it in the commitfest as "I plan to commit this very soon". April 4: You must give a short status update about anything that you haven't committed yet, with an explanation of how you plan to proceed with it. April 5-8: Mandatory daily status updates, explicit approval by the commitfest manager needed each day to continue working on it. April 8: Hard feature freeze deadline This would also give everyone more visibility, so that we're not all surprised by the last minute flood of commits. -- Heikki Linnakangas Neon (https://neon.tech)
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Mon, Apr 8, 2024 at 10:41 AM Robert Treat wrote: > > On Mon, Apr 8, 2024 at 10:27 AM Melanie Plageman > wrote: > > On Mon, Apr 8, 2024 at 9:26 AM Robert Haas wrote: > > > On Sun, Apr 7, 2024 at 6:50 PM Michael Paquier > > > wrote: > > > > And, as of the moment of typing this email, I get: > > > > =# select '2024-04-08 00:00-12:00' - now() as time_remaining; > > > > time_remaining > > > > - > > > > 13:10:35.688134 > > > > (1 row) > > > > > > > > So there is just a bit more than half a day remaining before the > > > > feature freeze is in effect. > > > > > > OK, so feature freeze is now in effect, then. > > > > > > In the future, let's use GMT for these deadlines. There have got to be > > > a lot more people who can easily understand when a certain GMT > > > timestamp falls in their local timezone than there are people who can > > > easily understand when a certain AOE timestamp falls in their local > > > timezone. > > > > > > And maybe we need to think of a way to further mitigate this crush of > > > last minute commits. e.g. In the last week, you can't have more > > > feature commits, or more lines of insertions in your commits, than you > > > did in the prior 3 weeks combined. I don't know. I think this mad rush > > > of last-minute commits is bad for the project. > > > > What if we pick the actual feature freeze time randomly? That is, > > starting on March 15th (or whenever but more than a week before), each > > night someone from RMT generates a random number between $current_day > > and April 8th. If the number matches $current_day, that day at > > midnight is the feature freeze. > > > > Unfortunately many humans are hardwired towards procrastination and > last minute heroics (it's one reason why deadline driven development > works even though in the long run it tends to be bad practice), and > historically was one of the driving factors in why we started doing > commitfests in the first place (you should have seen the mad dash of > commits before we had that process), so ISTM that it's not completely > avoidable... > > That said, are you suggesting that the feature freeze deadline be > random, and also held in secret by the RMT, only to be announced after > the freeze time has passed? This feels weird, but might apply enough > deadline related pressure while avoiding last minute shenanigans. Basically, yes. The RMT would find out each day whether or not that day is the feature freeze but not tell anyone. Then they would push some kind of feature freeze tag (do we already do a feature freeze tag? I didn't think so) at 11:59 PM (in some timezone) that evening and all commits that are feature commits after that are reverted. I basically thought it would be a way for people to know that they need to have their work done before April but keep them from waiting until the actual last minute. The rationale for doing it this way is it requires way less human involvement than some of the other methods proposed. Figuring out how many commits each committer is allowed to do based on past number of LOC,etc like Robert's suggestion sounds like a lot of work. I was trying to think of a simple way to beat the natural propensity people have toward procrastination. But, an approach like Heikki suggested [1] with check-ins and staggered deadlines is certainly much more principled. It just sounds like it will require a lot of enforcement and oversight. And it might be hard to ensure it doesn't end up being enforced only for some people and not others. However, I suppose everyone is saying a mindset shift is needed. And you can't usually shift a mindset with a technical solution like I proposed (despite what Libertarians might tell you about carbon offsets). - Melanie [1] https://www.postgresql.org/message-id/a5485b74-059a-4ea0-b445-7c393d6fe0de%40iki.fi
Re: CI and test improvements
> On 19 Feb 2024, at 11:33, Peter Eisentraut wrote: > > Ok, I didn't see that my feedback had been addressed. I have committed this > patch. Justin, Peter, I can't determine actual status of the CF entry [0]. May I ask someone of you to move patch to next CF or close as committed? Thanks! Best regards, Andrey Borodin. [0] https://commitfest.postgresql.org/47/3709/
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Mon, 8 Apr 2024 at 18:42, Heikki Linnakangas wrote: > On 08/04/2024 16:43, Tom Lane wrote: > > Robert Haas writes: > >> And maybe we need to think of a way to further mitigate this crush of > >> last minute commits. e.g. In the last week, you can't have more > >> feature commits, or more lines of insertions in your commits, than you > >> did in the prior 3 weeks combined. I don't know. I think this mad rush > >> of last-minute commits is bad for the project. > > > > I was just about to pen an angry screed along the same lines. > > The commit flux over the past couple days, and even the last > > twelve hours, was flat-out ridiculous. These patches weren't > > ready a week ago, and I doubt they were ready now. > > > > The RMT should feel free to exercise their right to require > > revert "early and often", or we are going to be shipping a > > very buggy release. > > > Can you elaborate, which patches you think were not ready? Let's make > sure to capture any concrete concerns in the Open Items list. > > I agree the last-minute crunch felt more intense than in previous years. > I'm guilty myself; I crunched the "direct SSL" patches in. My rationale > for that one: It's been in a pretty settled state for a long time. There > hasn't been any particular concerns about the design or the > implementation. I haven't commit tit sooner because I was not > comfortable with the lack of tests, especially the libpq parts. So I > made a last minute dash to write the tests so that I'm comfortable with > it, and I restructured the commits so that the tests and refactorings > come first. The resulting code changes look the same they have for a > long time, and I didn't uncover any significant new issues while doing > that. I would not have felt comfortable committing it otherwise. > > Yeah, I should have done that sooner, but realistically, there's nothing > like a looming deadline as a motivator. One idea to avoid the mad rush > in the future would be to make the feature freeze deadline more > progressive. For example: > > April 1: If you are still working on a feature that you still want to > commit, you must explicitly flag it in the commitfest as "I plan to > commit this very soon". > > April 4: You must give a short status update about anything that you > haven't committed yet, with an explanation of how you plan to proceed > with it. > > April 5-8: Mandatory daily status updates, explicit approval by the > commitfest manager needed each day to continue working on it. > > April 8: Hard feature freeze deadline > > This would also give everyone more visibility, so that we're not all > surprised by the last minute flood of commits. > IMO the fact that people struggle to work on patches, and make them better, etc. is an immense blessing for the Postgres community. Is the peak of commits really a big problem provided we have 6 months before actual release? I doubt March patches tend to be worse than the November ones. People are different, so are the ways they feel motivation and inspiration. This could be easily broken with bureaucratic decisions some of them, like proposed counting of lines of code vs period of time look even little bit repressive. Let's remain an open community, support inspiration in each other, and don't build an over-regulated corporation. I feel that Postgres will win if people feel less limited by formal rules. Personally, I believe RMT has enough experience and authority to stabilize and interact with authors if questions arise. Regards, Pavel Borisov
Re: PostgreSQL 17 Release Management Team & Feature Freeze
Heikki Linnakangas writes: > On 08/04/2024 16:43, Tom Lane wrote: >> I was just about to pen an angry screed along the same lines. >> The commit flux over the past couple days, and even the last >> twelve hours, was flat-out ridiculous. These patches weren't >> ready a week ago, and I doubt they were ready now. > Can you elaborate, which patches you think were not ready? Let's make > sure to capture any concrete concerns in the Open Items list. [ shrug... ] There were fifty-some commits on the last day, some of them quite large, and you want me to finger particular ones? I can't even have read them all yet. > Yeah, I should have done that sooner, but realistically, there's nothing > like a looming deadline as a motivator. One idea to avoid the mad rush > in the future would be to make the feature freeze deadline more > progressive. For example: > April 1: If you are still working on a feature that you still want to > commit, you must explicitly flag it in the commitfest as "I plan to > commit this very soon". > April 4: You must give a short status update about anything that you > haven't committed yet, with an explanation of how you plan to proceed > with it. > April 5-8: Mandatory daily status updates, explicit approval by the > commitfest manager needed each day to continue working on it. > April 8: Hard feature freeze deadline > This would also give everyone more visibility, so that we're not all > surprised by the last minute flood of commits. Perhaps something like that could help, but it seems like a lot of mechanism. I think the real problem is just that committers need to re-orient their thinking a little. We must get *less* willing to commit marginal patches, not more so, as the deadline approaches. regards, tom lane
Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs
Hello Daniel and Tom, 08.04.2024 17:25, Daniel Verite wrote: So I whacked the patch around till I liked it better, and pushed it. Thanks for taking care of this! Now that ExecQueryUsingCursor() is gone, it's not clear, what does the following comment mean:? * We must turn off gexec_flag to avoid infinite recursion. Note that * this allows ExecQueryUsingCursor to be applied to the individual query * results. Shouldn't it be removed? Best regards, Alexander
Re: PostgreSQL 17 Release Management Team & Feature Freeze
Pavel Borisov writes: > IMO the fact that people struggle to work on patches, and make them better, > etc. is an immense blessing for the Postgres community. Is the peak of > commits really a big problem provided we have 6 months before actual > release? I doubt March patches tend to be worse than the November ones. Yes, it's a problem, and yes the average quality of last-minute patches is visibly worse than that of patches committed in a less hasty fashion. We have been through this every year for the last couple decades, seems like, and we keep re-learning that lesson the hard way. I'm just distressed at our utter failure to learn from experience. regards, tom lane
Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs
Alexander Lakhin writes: > Now that ExecQueryUsingCursor() is gone, it's not clear, what does > the following comment mean:? > * We must turn off gexec_flag to avoid infinite recursion. Note that > * this allows ExecQueryUsingCursor to be applied to the individual query > * results. Hmm, the point about recursion is still valid isn't it? I agree the reference to ExecQueryUsingCursor is obsolete, but I think we need to reconstruct what this comment is actually talking about. It's certainly pretty obscure ... regards, tom lane
Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs
08.04.2024 18:08, Tom Lane wrote: Alexander Lakhin writes: Now that ExecQueryUsingCursor() is gone, it's not clear, what does the following comment mean:? * We must turn off gexec_flag to avoid infinite recursion. Note that * this allows ExecQueryUsingCursor to be applied to the individual query * results. Hmm, the point about recursion is still valid isn't it? I agree the reference to ExecQueryUsingCursor is obsolete, but I think we need to reconstruct what this comment is actually talking about. It's certainly pretty obscure ... Sorry, I wasn't clear enough, I meant to remove only that reference, not the quoted comment altogether. Best regards, Alexander
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On 4/8/24 16:59, Tom Lane wrote: > Heikki Linnakangas writes: >> On 08/04/2024 16:43, Tom Lane wrote: >>> I was just about to pen an angry screed along the same lines. >>> The commit flux over the past couple days, and even the last >>> twelve hours, was flat-out ridiculous. These patches weren't >>> ready a week ago, and I doubt they were ready now. > >> Can you elaborate, which patches you think were not ready? Let's make >> sure to capture any concrete concerns in the Open Items list. > > [ shrug... ] There were fifty-some commits on the last day, > some of them quite large, and you want me to finger particular ones? > I can't even have read them all yet. > >> Yeah, I should have done that sooner, but realistically, there's nothing >> like a looming deadline as a motivator. One idea to avoid the mad rush >> in the future would be to make the feature freeze deadline more >> progressive. For example: >> April 1: If you are still working on a feature that you still want to >> commit, you must explicitly flag it in the commitfest as "I plan to >> commit this very soon". >> April 4: You must give a short status update about anything that you >> haven't committed yet, with an explanation of how you plan to proceed >> with it. >> April 5-8: Mandatory daily status updates, explicit approval by the >> commitfest manager needed each day to continue working on it. >> April 8: Hard feature freeze deadline > >> This would also give everyone more visibility, so that we're not all >> surprised by the last minute flood of commits. > > Perhaps something like that could help, but it seems like a lot > of mechanism. I think the real problem is just that committers > need to re-orient their thinking a little. We must get *less* > willing to commit marginal patches, not more so, as the deadline > approaches. > For me the main problem with the pre-freeze crush is that it leaves pretty much no practical chance to do meaningful review/testing, and some of the patches likely went through significant changes (at least judging by the number of messages and patch versions in the associated threads). That has to have a cost later ... That being said, I'm not sure the "progressive deadline" proposed by Heikki would improve that materially, and it seems like a lot of effort to maintain etc. And even if someone updates the CF app with all the details, does it even give others sufficient opportunity to review the new patch versions? I don't think so. (It anything, it does not seem fair to expect others to do last-minute reviews under pressure.) Maybe it'd be better to start by expanding the existing rule about not committing patches introduced for the first time in the last CF. What if we said that patches in the last CF must not go through significant changes, and if they do it'd mean the patch is moved to the next CF? Perhaps with exceptions to be granted by the RMT when appropriate? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On 4/8/24 11:05, Tom Lane wrote: Pavel Borisov writes: IMO the fact that people struggle to work on patches, and make them better, etc. is an immense blessing for the Postgres community. Is the peak of commits really a big problem provided we have 6 months before actual release? I doubt March patches tend to be worse than the November ones. Yes, it's a problem, and yes the average quality of last-minute patches is visibly worse than that of patches committed in a less hasty fashion. We have been through this every year for the last couple decades, seems like, and we keep re-learning that lesson the hard way. I'm just distressed at our utter failure to learn from experience. I don't dispute that we could do better, and this is just a simplistic look based on "number of commits per day", but the attached does put it in perspective to some extent. -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: PostgreSQL 17 Release Management Team & Feature Freeze
Hi, On 2024-04-08 09:26:09 -0400, Robert Haas wrote: > On Sun, Apr 7, 2024 at 6:50 PM Michael Paquier wrote: > And maybe we need to think of a way to further mitigate this crush of > last minute commits. e.g. In the last week, you can't have more > feature commits, or more lines of insertions in your commits, than you > did in the prior 3 weeks combined. I don't know. I think this mad rush > of last-minute commits is bad for the project. I don't think it's very useful to paint a very broad brush here, unfortunately. Some will just polish commits until the last minute, until the the dot's on the i's really shine, others will continue picking up more CF entries until the freeze is reached, others will push half baked stuff. Of course there will be an increased commit rate, but it does looks like there was some stuff that looked somewhat rickety. Greetings, Andres
Re: Table AM Interface Enhancements
Hi, On 2024-04-08 11:17:51 +0400, Pavel Borisov wrote: > On Mon, 8 Apr 2024 at 03:25, Alexander Korotkov > > I was under the impression there are not so many out-of-core table > > AMs, which have non-dummy analysis implementations. And even if there > > are some, duplicating acquire_sample_rows() isn't a big deal. > > > > But given your feedback, I'd like to propose to keep both options > > open. Turn back the block-level API for analyze, but let table-AM > > implement its own analyze function. Then existing out-of-core AMs > > wouldn't need to do anything (or probably just set the new API method > > to NULL). > > > I think that providing both new and old interface functions for block-based > and non-block based custom am is an excellent compromise. I don't agree, that way lies an unmanageable API. To me the new API doesn't look well polished either, so it's not a question of a smoother transition or something like that. I don't think redesigning extension APIs at this stage of the release cycle makes sense. Greetings, Andres Freund
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Mon, 8 Apr 2024 at 17:21, Tomas Vondra wrote: > > > > On 4/8/24 16:59, Tom Lane wrote: > > Heikki Linnakangas writes: > >> On 08/04/2024 16:43, Tom Lane wrote: > >>> I was just about to pen an angry screed along the same lines. > >>> The commit flux over the past couple days, and even the last > >>> twelve hours, was flat-out ridiculous. These patches weren't > >>> ready a week ago, and I doubt they were ready now. > > > >> Can you elaborate, which patches you think were not ready? Let's make > >> sure to capture any concrete concerns in the Open Items list. > > > > [ shrug... ] There were fifty-some commits on the last day, > > some of them quite large, and you want me to finger particular ones? > > I can't even have read them all yet. > > > >> Yeah, I should have done that sooner, but realistically, there's nothing > >> like a looming deadline as a motivator. One idea to avoid the mad rush > >> in the future would be to make the feature freeze deadline more > >> progressive. For example: > >> April 1: If you are still working on a feature that you still want to > >> commit, you must explicitly flag it in the commitfest as "I plan to > >> commit this very soon". > >> April 4: You must give a short status update about anything that you > >> haven't committed yet, with an explanation of how you plan to proceed > >> with it. > >> April 5-8: Mandatory daily status updates, explicit approval by the > >> commitfest manager needed each day to continue working on it. > >> April 8: Hard feature freeze deadline > > > >> This would also give everyone more visibility, so that we're not all > >> surprised by the last minute flood of commits. > > > > Perhaps something like that could help, but it seems like a lot > > of mechanism. I think the real problem is just that committers > > need to re-orient their thinking a little. We must get *less* > > willing to commit marginal patches, not more so, as the deadline > > approaches. > > > > For me the main problem with the pre-freeze crush is that it leaves > pretty much no practical chance to do meaningful review/testing, and > some of the patches likely went through significant changes (at least > judging by the number of messages and patch versions in the associated > threads). That has to have a cost later ... > > That being said, I'm not sure the "progressive deadline" proposed by > Heikki would improve that materially, and it seems like a lot of effort > to maintain etc. And even if someone updates the CF app with all the > details, does it even give others sufficient opportunity to review the > new patch versions? I don't think so. (It anything, it does not seem > fair to expect others to do last-minute reviews under pressure.) > > Maybe it'd be better to start by expanding the existing rule about not > committing patches introduced for the first time in the last CF. I don't think adding more hurdles about what to include into the next release is a good solution. Why the March CF, and not earlier? Or later? How about unregistered patches? Changes to the docs? Bug fixes? The March CF already has a submission deadline of "before march", so that already puts a soft limit on the patches considered for the april deadline. > What if > we said that patches in the last CF must not go through significant > changes, and if they do it'd mean the patch is moved to the next CF? I also think there is already a big issue with a lack of interest in getting existing patches from non-committers committed, reducing the set of patches that could be considered is just cheating the numbers and discouraging people from contributing. For one, I know I have motivation issues keeping up with reviewing other people's patches when none (well, few, as of this CF) of my patches get reviewed materially and committed. I don't see how shrinking the window of opportunity for significant review from 9 to 7 months is going to help there. So, I think that'd be counter-productive, as this would get the perverse incentive to band-aid over (architectural) issues to limit churn inside the patch, rather than fix those issues in a way that's appropriate for the project as a whole. -Matthias
Re: pgsql: Fix the intermittent buildfarm failures in 040_standby_failover_
On Mon, Apr 8, 2024 at 4:04 AM Amit Kapila wrote: > Fix the intermittent buildfarm failures in 040_standby_failover_slots_sync. > > It is possible that even if the primary waits for the subscriber to catch > up and then disables the subscription, the XLOG_RUNNING_XACTS record gets > inserted between the two steps by bgwriter and walsender processes it. > This can move the restart_lsn of the corresponding slot in an > unpredictable way which further leads to slot sync failure. > > To ensure predictable behaviour, we drop the subscription and manually > create the slot before the test. The other idea we discussed to write a > predictable test is to use injection points to control the bgwriter > logging XLOG_RUNNING_XACTS but that needs more analysis. We can add a > separate test using injection points. Hi, I'm concerned that the failover slots feature may not be in sufficiently good shape for us to ship it. Since this test file was introduced at the end of January, it's been touched by a total of 16 commits, most of which seem to be trying to get it to pass reliably: 6f3d8d5e7c Fix the intermittent buildfarm failures in 040_standby_failover_slots_sync. 6f132ed693 Allow synced slots to have their inactive_since. 2ec005b4e2 Ensure that the sync slots reach a consistent state after promotion without losing data. 6ae701b437 Track invalidation_reason in pg_replication_slots. bf279ddd1c Introduce a new GUC 'standby_slot_names'. def0ce3370 Fix BF failure introduced by commit b3f6b14cf4. b3f6b14cf4 Fixups for commit 93db6cbda0. d13ff82319 Fix BF failure in commit 93db6cbda0. 93db6cbda0 Add a new slot sync worker to synchronize logical slots. 801792e528 Improve ERROR/LOG messages added by commits ddd5f4f54a and 7a424ece48. b7bdade6a4 Disable autovacuum on primary in 040_standby_failover_slots_sync test. d9e225f275 Change the LOG level in 040_standby_failover_slots_sync.pl to DEBUG2. 9bc1eee988 Another try to fix BF failure introduced in commit ddd5f4f54a. bd8fc1677b Fix BF introduced in commit ddd5f4f54a. ddd5f4f54a Add a slot synchronization function. 776621a5e4 Add a failover option to subscriptions. It's not really the test failures themselves that concern me here, so much as the possibility of users who try to make use of this feature having a similar amount of difficulty getting it to work reliably. The test case seems to be taking more and more elaborate precautions to prevent incidental things from breaking the feature. But users won't like this feature very much if they have to take elaborate precautions to get it to work in the real world. Is there a reason to believe that all of this stabilization work is addressing artificial problems that won't inconvenience real users, or should we be concerned that the feature itself is going to be difficult to use effectively? -- Robert Haas EDB: http://www.enterprisedb.com
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Mon, 8 Apr 2024 at 19:48, Matthias van de Meent < boekewurm+postg...@gmail.com> wrote: > On Mon, 8 Apr 2024 at 17:21, Tomas Vondra > wrote: > > > > > > > > On 4/8/24 16:59, Tom Lane wrote: > > > Heikki Linnakangas writes: > > >> On 08/04/2024 16:43, Tom Lane wrote: > > >>> I was just about to pen an angry screed along the same lines. > > >>> The commit flux over the past couple days, and even the last > > >>> twelve hours, was flat-out ridiculous. These patches weren't > > >>> ready a week ago, and I doubt they were ready now. > > > > > >> Can you elaborate, which patches you think were not ready? Let's make > > >> sure to capture any concrete concerns in the Open Items list. > > > > > > [ shrug... ] There were fifty-some commits on the last day, > > > some of them quite large, and you want me to finger particular ones? > > > I can't even have read them all yet. > > > > > >> Yeah, I should have done that sooner, but realistically, there's > nothing > > >> like a looming deadline as a motivator. One idea to avoid the mad rush > > >> in the future would be to make the feature freeze deadline more > > >> progressive. For example: > > >> April 1: If you are still working on a feature that you still want to > > >> commit, you must explicitly flag it in the commitfest as "I plan to > > >> commit this very soon". > > >> April 4: You must give a short status update about anything that you > > >> haven't committed yet, with an explanation of how you plan to proceed > > >> with it. > > >> April 5-8: Mandatory daily status updates, explicit approval by the > > >> commitfest manager needed each day to continue working on it. > > >> April 8: Hard feature freeze deadline > > > > > >> This would also give everyone more visibility, so that we're not all > > >> surprised by the last minute flood of commits. > > > > > > Perhaps something like that could help, but it seems like a lot > > > of mechanism. I think the real problem is just that committers > > > need to re-orient their thinking a little. We must get *less* > > > willing to commit marginal patches, not more so, as the deadline > > > approaches. > > > > > > > For me the main problem with the pre-freeze crush is that it leaves > > pretty much no practical chance to do meaningful review/testing, and > > some of the patches likely went through significant changes (at least > > judging by the number of messages and patch versions in the associated > > threads). That has to have a cost later ... > > > > That being said, I'm not sure the "progressive deadline" proposed by > > Heikki would improve that materially, and it seems like a lot of effort > > to maintain etc. And even if someone updates the CF app with all the > > details, does it even give others sufficient opportunity to review the > > new patch versions? I don't think so. (It anything, it does not seem > > fair to expect others to do last-minute reviews under pressure.) > > > > Maybe it'd be better to start by expanding the existing rule about not > > committing patches introduced for the first time in the last CF. > > I don't think adding more hurdles about what to include into the next > release is a good solution. Why the March CF, and not earlier? Or > later? How about unregistered patches? Changes to the docs? Bug fixes? > The March CF already has a submission deadline of "before march", so > that already puts a soft limit on the patches considered for the april > deadline. > > > What if > > we said that patches in the last CF must not go through significant > > changes, and if they do it'd mean the patch is moved to the next CF? > > I also think there is already a big issue with a lack of interest in > getting existing patches from non-committers committed, reducing the > set of patches that could be considered is just cheating the numbers > and discouraging people from contributing. For one, I know I have > motivation issues keeping up with reviewing other people's patches > when none (well, few, as of this CF) of my patches get reviewed > materially and committed. I don't see how shrinking the window of > opportunity for significant review from 9 to 7 months is going to help > there. > > So, I think that'd be counter-productive, as this would get the > perverse incentive to band-aid over (architectural) issues to limit > churn inside the patch, rather than fix those issues in a way that's > appropriate for the project as a whole. > I second your opinion, Mattias! I also feel that the management of the review of other contibutor's patches participation is much more important for a community as a whole. And this could make process of patches proposals and improvement running, while motivating participation (in all three roles of contributors, reviewers and committers), not vice versa. Regards, Pavel.
Re: Table AM Interface Enhancements
On 2024-04-08 08:37:44 -0700, Andres Freund wrote: > On 2024-04-08 11:17:51 +0400, Pavel Borisov wrote: > > On Mon, 8 Apr 2024 at 03:25, Alexander Korotkov > > > I was under the impression there are not so many out-of-core table > > > AMs, which have non-dummy analysis implementations. And even if there > > > are some, duplicating acquire_sample_rows() isn't a big deal. > > > > > > But given your feedback, I'd like to propose to keep both options > > > open. Turn back the block-level API for analyze, but let table-AM > > > implement its own analyze function. Then existing out-of-core AMs > > > wouldn't need to do anything (or probably just set the new API method > > > to NULL). > > > > > I think that providing both new and old interface functions for block-based > > and non-block based custom am is an excellent compromise. > > I don't agree, that way lies an unmanageable API. To me the new API doesn't > look well polished either, so it's not a question of a smoother transition or > something like that. > > I don't think redesigning extension APIs at this stage of the release cycle > makes sense. Wait, you already pushed an API redesign? With a design that hasn't even seen the list from what I can tell? Without even mentioning that on the list? You got to be kidding me.
Re: Logging parallel worker draught
On 4/8/24 10:05, Andrey M. Borodin wrote: Hi Benoit! This is kind reminder that this thread is waiting for your response. CF entry [0] is in "Waiting on Author", I'll move it to July CF. Hi thanks for the reminder, The past month as been hectic for me. It should calm down by next week at wich point I'll have time to go back to this. sorry for the delay. -- Benoit Lobréau Consultant http://dalibo.com
Re: LogwrtResult contended spinlock
On Mon, 2024-04-08 at 10:24 +0200, Alvaro Herrera wrote: > My trouble with the "copy" term is that we don't use that term > anywhere > in relation to WAL. I got the term from CopyXLogRecordToWAL(). > This "copy" is in > reality just the insertion, after it's finished. The "Result" suffix > is intended to convey that it's the point where the operation has > successfully finished. That's a convincing point. > Maybe we can add some commentary to make this clearer. For now, I'd just suggest a comment on GetXLogInsertRecPtr() explaining what it's returning and how that's different from logInsertResult. In the future, it would be nice to clarify where variable names like reservedUpto and CurrBytePos fit in. Also the external documentation for pg_current_wal_insert_lsn() is a bit ambiguous. > Lastly, I just noticed that I forgot credits and discussion link in > that > commit message. I would have attributed authorship to Bharath > though, > because I had not realized that this patch had actually been written > by > you initially[1]. My apologies. No worries. Thank you for reviewing and committing it. Regards, Jeff Davis
Re: Synchronizing slots from primary to standby
Hi, On 2024-04-08 16:01:41 +0530, Amit Kapila wrote: > Pushed. https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=adder&dt=2024-04-08%2012%3A04%3A27 This unfortunately is a commit after commit 6f3d8d5e7cc Author: Amit Kapila Date: 2024-04-08 13:21:55 +0530 Fix the intermittent buildfarm failures in 040_standby_failover_slots_sync. Greetings, Andres
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On 4/8/24 8:29 AM, Andres Freund wrote: Hi, On 2024-04-08 09:26:09 -0400, Robert Haas wrote: On Sun, Apr 7, 2024 at 6:50 PM Michael Paquier wrote: And maybe we need to think of a way to further mitigate this crush of last minute commits. e.g. In the last week, you can't have more feature commits, or more lines of insertions in your commits, than you did in the prior 3 weeks combined. I don't know. I think this mad rush of last-minute commits is bad for the project. I don't think it's very useful to paint a very broad brush here, unfortunately. Some will just polish commits until the last minute, until the the dot's on the i's really shine, others will continue picking up more CF entries until the freeze is reached, others will push half baked stuff. Of course there will be an increased commit rate, but it does looks like there was some stuff that looked somewhat rickety. I agree with Andres here (though decline to comment on the rickety-ness of the patches). I think overcoming human nature to be more proactive at a deadline is at least a NP-hard problem. This won't change if we adjust deadlines. I think it's better to ensure we're enforcing best practices for commits, and maybe that's a separate review to have. As mentioned in different contexts, we do have several safeguards for a release: * We have a (fairly long) beta period; this allows us to remove patches prior to GA and get in further testing. * We have a RMT that (as Tom mentioned) can use its powers early and often to remove patches that are not up to our quality levels. * We can evaluate the quality of the commits coming in and coach folks on what to do better. I understand that a revert is costly, particularly the longer a commit stays in, and I do 100% agree we should maintain the high commit bar we have and not rush things in just so "they're in for feature freeze and we'll clean them up for beta." That's where best practices come in. I tend to judge the release by the outcome: once we go GA, how buggy is the release? Did something during the release cycle (e.g. a sloppy commit during feature freeze, lack of testing) lead to a bug that warranted an out-of-cycle release? And yes, how we commit things at feature freeze / through the beta impact that - we should ensure we're still committing things that we would have committed at a least hectic time, but understand that the deadline is still a strong motivator co complete the work. Thanks, Jonathan OpenPGP_signature.asc Description: OpenPGP digital signature
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Mon, Apr 8, 2024 at 11:48 AM Matthias van de Meent wrote: > I also think there is already a big issue with a lack of interest in > getting existing patches from non-committers committed, reducing the > set of patches that could be considered is just cheating the numbers > and discouraging people from contributing. For one, I know I have > motivation issues keeping up with reviewing other people's patches > when none (well, few, as of this CF) of my patches get reviewed > materially and committed. I don't see how shrinking the window of > opportunity for significant review from 9 to 7 months is going to help > there. > > So, I think that'd be counter-productive, as this would get the > perverse incentive to band-aid over (architectural) issues to limit > churn inside the patch, rather than fix those issues in a way that's > appropriate for the project as a whole. I don't think you're wrong about any of this, but I don't think Tom and I are wrong to be upset about the volume of last-minute commits, either. There's a lot of this stuff that could have been committed a month ago, or two months ago, or six months ago, and it just wasn't. A certain amount of that is, as Heikki says, understandable and expected. People procrastinate. But, if too many people procrastinate too much, then it becomes a problem, and if you don't do anything about that problem then, well, you still have one. I don't want more barriers to getting stuff committed here either, but I also don't want somebody whose 100-line patch is basically unchanged since last July to commit it 19 hours before the feature freeze deadline[1]. That's just making everyone's life more difficult. If that patch happens to have been submitted by a non-committer, that non-committer waited an extra 9 months for the commit, not knowing whether it would actually happen, which like you say is demotivating. And if it was the committer's own patch then it was probably going in sooner or later, barring objections, so basically, they just deprived the project of 9 months of in-tree testing that the patch could have had basically for free. There's simply no world in which this kind of behavior is actually helpful to committers, non-committers, reviews, or the project in general. -- Robert Haas EDB: http://www.enterprisedb.com [1] This is a fictional example, I made up these numbers without checking anything, but I think it's probably not far off some of what actually happened.
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On 2024-Apr-08, Robert Haas wrote: > And maybe we need to think of a way to further mitigate this crush of > last minute commits. e.g. In the last week, you can't have more > feature commits, or more lines of insertions in your commits, than you > did in the prior 3 weeks combined. I don't know. I think this mad rush > of last-minute commits is bad for the project. Another idea is to run a patch triage around mid March 15th, with the intention of punting patches to the next cycle early enough. But rather than considering each patch in its own merits, consider the responsible _committers_ and the load that they are reasonably expected to handle: determine which patches each committer deems his or her responsibility for the rest of that March commitfest, and punt all the rest. That way we have a reasonably vetted amount of effort that each committer is allowed to spend for the remainder of that commitfest. Excesses should be obvious enough and discouraged. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Table AM Interface Enhancements
On Mon, Apr 8, 2024, 19:08 Andres Freund wrote: > On 2024-04-08 08:37:44 -0700, Andres Freund wrote: > > On 2024-04-08 11:17:51 +0400, Pavel Borisov wrote: > > > On Mon, 8 Apr 2024 at 03:25, Alexander Korotkov > > > > I was under the impression there are not so many out-of-core table > > > > AMs, which have non-dummy analysis implementations. And even if > there > > > > are some, duplicating acquire_sample_rows() isn't a big deal. > > > > > > > > But given your feedback, I'd like to propose to keep both options > > > > open. Turn back the block-level API for analyze, but let table-AM > > > > implement its own analyze function. Then existing out-of-core AMs > > > > wouldn't need to do anything (or probably just set the new API method > > > > to NULL). > > > > > > > I think that providing both new and old interface functions for > block-based > > > and non-block based custom am is an excellent compromise. > > > > I don't agree, that way lies an unmanageable API. To me the new API > doesn't > > look well polished either, so it's not a question of a smoother > transition or > > something like that. > > > > I don't think redesigning extension APIs at this stage of the release > cycle > > makes sense. > > Wait, you already pushed an API redesign? With a design that hasn't even > seen > the list from what I can tell? Without even mentioning that on the list? > You > got to be kidding me. > Yes, it was my mistake. I got rushing trying to fit this to FF, even doing significant changes just before commit. I'll revert this later today. -- Regards, Alexander Korotkov
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Tue, Apr 9, 2024 at 12:30 AM Andres Freund wrote: > > Hi, > > On 2024-04-08 09:26:09 -0400, Robert Haas wrote: > > On Sun, Apr 7, 2024 at 6:50 PM Michael Paquier wrote: > > And maybe we need to think of a way to further mitigate this crush of > > last minute commits. e.g. In the last week, you can't have more > > feature commits, or more lines of insertions in your commits, than you > > did in the prior 3 weeks combined. I don't know. I think this mad rush > > of last-minute commits is bad for the project. > > Some will just polish commits until the last minute, until the > the dot's on the i's really shine, others will continue picking up more CF > entries until the freeze is reached, others will push half baked stuff. I agree with this part. Aside from considering how to institute some rules for mitigating the last-minute rush, it might also be a good idea to consider how to improve testing the new commits during beta. FWIW in each year, after feature freeze I personally pick some new features that I didn't get involved with during the development and do intensive reviews in April. It might be good if more people did things like that. That might help finding half baked features earlier and improve the quality in general. So for example, we list features that could require more reviews (e.g. because of its volume, complexity, and a span of influence etc.) and we do intensive reviews for these items. Each item should be reviewed by other than the author and the committer. We may want to set aside a specific period for intensive testing. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Allow non-superuser to cancel superuser tasks.
>>> There is pg_read_all_stats as well, so I don't see a big issue in
>>> requiring to be a member of this role as well for the sake of what's
>>> proposing here.
>>
>> Well, that tells you quite a bit more than just which PIDs correspond to
>> autovacuum workers, but maybe that's good enough for now.
>
> That may be a good initial compromise, for now.
Sounds good to me. I will update the documentation.
> And we should try to reshape things so as we get an ERROR like
> "permission denied to terminate process" or "permission denied to
> cancel query" for all the error paths, including autovacuum workers
> and backends, so as we never leak any information about the backend
> types involved when a role has no permission to issue the signal.
> Perhaps that's the most intuitive thing as well, because autovacuum
> workers are backends. One thing that we could do is to mention both
> pg_signal_backend and pg_signal_autovacuum in the errdetail, and have
> both cases be handled by SIGNAL_BACKEND_NOPERMISSION on failure.
I understand your concern that we should avoid exposing the fact that the
backend which the user is attempting to terminate is an AV worker unless the
user has pg_signal_backend privileges and pg_signal_autovacuum privileges.
But Im not following how we can re-use SIGNAL_BACKEND_NOPERMISSION for this. If
we return SIGNAL_BACKEND_NOPERMISSION here as the following, it'll stay return
the "permission denied to terminate / cancel query" errmsg and errdetail in
pg_cancel/terminate_backend.
/*
* If the backend is autovacuum worker, allow user with the privileges
of
* pg_signal_autovacuum role to signal the backend.
*/
if (pgstat_get_backend_type(GetNumberFromPGProc(proc)) ==
B_AUTOVAC_WORKER)
{
if (!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM))
return SIGNAL_BACKEND_NOPERMISSION;
}
Are you suggesting that we check if the backend is B_AUTOVAC in pg_cancel/
terminate_backend? That seems a bit unclean to me since pg_cancel_backend &
pg_cancel_backend does not access to the procNumber to check the type of the
backend.
IMHO, we can keep SIGNAL_BACKEND_NOAUTOVACUUM but just improve the errmsg /
errdetail to not expose that the backend is an AV worker. It'll also be helpful
if you can suggest what errdetail we should use here.
Thanks
--
Anthony Leung
Amazon Web Services: https://aws.amazon.com
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On 4/8/24 17:48, Matthias van de Meent wrote: > On Mon, 8 Apr 2024 at 17:21, Tomas Vondra > wrote: >> >> ... >> >> For me the main problem with the pre-freeze crush is that it leaves >> pretty much no practical chance to do meaningful review/testing, and >> some of the patches likely went through significant changes (at least >> judging by the number of messages and patch versions in the associated >> threads). That has to have a cost later ... >> >> That being said, I'm not sure the "progressive deadline" proposed by >> Heikki would improve that materially, and it seems like a lot of effort >> to maintain etc. And even if someone updates the CF app with all the >> details, does it even give others sufficient opportunity to review the >> new patch versions? I don't think so. (It anything, it does not seem >> fair to expect others to do last-minute reviews under pressure.) >> >> Maybe it'd be better to start by expanding the existing rule about not >> committing patches introduced for the first time in the last CF. > > I don't think adding more hurdles about what to include into the next > release is a good solution. Why the March CF, and not earlier? Or > later? How about unregistered patches? Changes to the docs? Bug fixes? > The March CF already has a submission deadline of "before march", so > that already puts a soft limit on the patches considered for the april > deadline. > >> What if >> we said that patches in the last CF must not go through significant >> changes, and if they do it'd mean the patch is moved to the next CF? > > I also think there is already a big issue with a lack of interest in > getting existing patches from non-committers committed, reducing the > set of patches that could be considered is just cheating the numbers > and discouraging people from contributing. For one, I know I have > motivation issues keeping up with reviewing other people's patches > when none (well, few, as of this CF) of my patches get reviewed > materially and committed. I don't see how shrinking the window of > opportunity for significant review from 9 to 7 months is going to help > there. > I 100% understand how frustrating the lack of progress can be, and I agree we need to do better. I tried to move a number of stuck patches this CF, and I hope (and plan) to do more of this in the future. But I don't quite see how would this rule modification change the situation for non-committers. AFAIK we already have the rule that (complex) patches should not appear in the last CF for the first time, and I'd argue that a substantial rework of a complex patch is not that far from a completely new patch. Sure, the reworks may be based on a thorough review, so there's a lot of nuance. But still, is it possible to properly review if it gets reworked at the very end of the CF? > So, I think that'd be counter-productive, as this would get the > perverse incentive to band-aid over (architectural) issues to limit > churn inside the patch, rather than fix those issues in a way that's > appropriate for the project as a whole. > Surely those architectural shortcomings should be identified in a review - which however requires time to do properly, and thus is an argument for ensuring there's enough time for such review (which is in direct conflict with the last-minute crush, IMO). Once again, I 100% agree we need to do better in handling patches from non-committers, absolutely no argument there. But does it require this last-minute crush? I don't think so, it can't be at the expense of proper review and getting it right. A complex patch needs to be submitted early in the cycle, not in the last CF. If it's submitted early, but does not receive enough interest/reviews, I think we need to fix & improve that - not to rework/push it at the very end. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Security lessons from liblzma
On Fri, Apr 5, 2024 at 5:14 PM Michael Paquier wrote: > Saying that, my spidey sense tingles at the recent commit > 3311ea86edc7, that had the idea to introduce a 20k line output file > based on a 378 line input file full of random URLs. In my experience, > tests don't require to be that large to be useful, and the input data > is very hard to parse. That's a good point. I've proposed a patch over at [1] to shrink it substantially. --Jacob [1] https://www.postgresql.org/message-id/CAOYmi%2B%3Dkx14ui_A__4L_XcFePSuUuR1kwJfUKxphuZU_i6%3DWpA%40mail.gmail.com
Re: Table AM Interface Enhancements
On Mon, Apr 8, 2024 at 12:33 PM Alexander Korotkov wrote: > Yes, it was my mistake. I got rushing trying to fit this to FF, even doing > significant changes just before commit. > I'll revert this later today. Alexander, Exactly how much is getting reverted here? I see these, all since March 23rd: dd1f6b0c17 Provide a way block-level table AMs could re-use acquire_sample_rows() 9bd99f4c26 Custom reloptions for table AM 97ce821e3e Fix the parameters order for TableAmRoutine.relation_copy_for_cluster() 867cc7b6dd Revert "Custom reloptions for table AM" b1484a3f19 Let table AM insertion methods control index insertion c95c25f9af Custom reloptions for table AM 27bc1772fc Generalize relation analyze in table AM interface 87985cc925 Allow locking updated tuples in tuple_update() and tuple_delete() c35a3fb5e0 Allow table AM tuple_insert() method to return the different slot 02eb07ea89 Allow table AM to store complex data structures in rd_amcache I'm not really feeling very good about all of this, because: - 87985cc925 was previously committed as 11470f544e on March 23, 2023, and almost immediately reverted. Now you tried again on March 26, 2024. I know there was a bunch of rework in the middle, but there are times in the year that things can be committed other than right before the feature freeze. Like, don't wait a whole year for the next attempt and then again do it right before the cutoff. - The Discussion links in the commit messages do not seem to stand for the proposition that these particular patches ought to be committed in this form. Some of them are just links to the messages where the patch was originally posted, which is probably not against policy or anything, but it'd be nicer to see links to versions of the patch with which people are, in nearby emails, agreeing. Even worse, some of these are links to emails where somebody said, "hey, some earlier commit does not look good." In particular, dd1f6b0c172a643a73d6b71259fa2d10378b39eb has a discussion link where Andres complains about 27bc1772fc814946918a5ac8ccb9b5c5ad0380aa, but it's not clear how that justifies the new commit. - The commit message for 867cc7b6dd says "This reverts commit c95c25f9af4bc77f2f66a587735c50da08c12b37 due to multiple design issues spotted after commit." That's not a very good justification for then trying again 6 days later with 9bd99f4c26, and it's *definitely* not a good justification for there being no meaningful discussion links in the commit message for 9bd99f4c26. They're just the same links you had in the previous attempt, so it's pretty hard for anybody to understand what got fixed and whether all of the concerns were really addressed. Just looking over the commit, it's pretty hard to understand what is being changed and why: there's not a lot of comment updates, there's no documentation changes, and there's not a lot of explanation in the commit message either. Even if this feature is great and all the code is perfect now, it's going to be hard for anyone to figure out how to use it. 97ce821e3e looks like a clear bug fix to me, but I wonder if the rest of this should all just be reverted, with a ban on ever trying it again after March 1 of any year. I'd like to believe that there are only bookkeeping problems here, and that there was in fact clear agreement that all of these changes should be made in this form, and that the commit messages simply failed to reference the most relevant emails. But what I fear, especially in view of Andres's remarks, is that these commits were done in haste without adequate consensus, and I think that's a serious problem. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Improve eviction algorithm in ReorderBuffer
On Mon, 2024-04-08 at 21:29 +0900, Masahiko Sawada wrote:
> For v17, changes for #2 are smaller, but I'm concerned
> that the new API that requires a hash function to be able to use
> binaryheap_update_{up|down} might not be user friendly.
The only API change in 02 is accepting a hash callback rather than a
boolean in binaryheap_allocate(), so I don't see that as worse than
what's there now. It also directly fixes my complaint (hashing the
pointer) and does nothing more, so I think it's the right fix for now.
I do think that the API can be better (templated like simplehash), but
I don't think right now is a great time to change it.
> When it comes to performance overhead, I mentioned that there is some
> regression in the current binaryheap even without indexing.
As far as I can tell, you are just adding a single branch in that path,
and I would expect it to be a predictable branch, right?
Thank you for testing, but small differences in a microbenchmark aren't
terribly worrying for me. If other call sites are that sensitive to
binaryheap performance, the right answer is to have a templated version
that would not only avoid this unnecessary branch, but also inline the
comparator (which probably matters more).
Regards,
Jeff Davis
Re: PostgreSQL 17 Release Management Team & Feature Freeze
Hi, On 4/8/24 14:15, Tomas Vondra wrote: I think we need to fix & improve that - not to rework/push it at the very end. This is going to be very extreme... Either a patch is ready for merge or it isn't - when 2 or more Committers agree on it then it can be merged - the policy have to be discussed of course. However, development happens all through out the year, so having to wait for potential feedback during CommitFests windows can stop development during the other months - I'm talking about non-Committers here... Having patches on -hackers@ is best, but maybe there is a hybrid model where they exists in pull requests, tested through CfBot, and merged when ready - no matter what month it is. Pull requests could still have labels that ties them to a "CommitFest" to "high-light" them, but it would make it easier to have a much clearer cut-off date for feature freeze. And, pull request labels are easily changed. March is seeing a lot of last-minute changes... Just something to think about. Best regards, Jesper
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Mon, 8 Apr 2024 at 20:15, Tomas Vondra wrote: > I 100% understand how frustrating the lack of progress can be, and I > agree we need to do better. I tried to move a number of stuck patches > this CF, and I hope (and plan) to do more of this in the future. > > But I don't quite see how would this rule modification change the > situation for non-committers. The problem that I feel I'm seeing is that committers mostly seem to materially review large patchsets in the last two commit fests. This might be not based in reality, but it is definitely how it feels to me. If someone has stats on this, feel free to share. I'll sketch a situation: There's a big patch that some non-committer submitted that has been sitting on the mailinglist for 6 months or more, only being reviewed by other non-committers, which the submitter quickly addresses. Then in the final commit fest it is finally reviewed by a committer, and they say it requires significant changes. Right now, the submitter can decide to quickly address those changes, and hope to keep the attention of this committer, to hopefully get it merged before the deadline after probably a few more back and forths. But this new rule would mean that those significant changes would be a reason not to put it in the upcoming release. Which I expect would first of all really feel like a slap in the face to the submitter, because it's not their fault that those changes were only requested in the last commit fest. This would then likely result in the submitter not following up quickly (why make time right at this moment, if you're suddenly going to have another full year), which would then cause the committer to lose context of the patch and thus interest in the patch. And then finally getting into the exact same situation next year in the final commit fest, when some other committer didn't agree with the redesign of the first one and request a new one pushing it another year. So yeah, I definitely agree with Matthias. I definitely feel like his rule would seriously impact contributions made by non-committers. Maybe a better solution to this problem would be to spread impactful reviews by committers more evenly throughout the year. Then there wouldn't be such a rush to address them in the last commit fest.
post-freeze damage control
On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas wrote:
> Can you elaborate, which patches you think were not ready? Let's make
> sure to capture any concrete concerns in the Open Items list.
Hi,
I'm moving this topic to a new thread for better visibility and less
admixture of concerns. I'd like to invite everyone here present to
opine on which patches we ought to be worried about. Here are a few
picks from me to start things off. My intention here is to convey "I
find these scary" rather than "these commits were irresponsible," so I
respectfully ask that you don't take the choice to list your patch
here as an attack, or the choice not to list your patch here as an
endorsement. I'm very happy if I'm wrong and these patches are not
actually scary. And likewise let's view any allegations of scariness
by others as a genuine attempt to figure out what might be broken,
rather than as an attempt to get anyone into trouble or whatever.
- As I wrote about separately, I'm concerned that the failover slots
stuff may not be in as good a shape as it needs to be for people to
get good use out of it.
http://postgr.es/m/CA+TgmoaA4oufUBR5B-4o83rnwGZ3zAA5UvwxDX=njcm1tvg...@mail.gmail.com
- I also wrote separately about the flurry of recent table AM changes.
http://postgr.es/m/ca+tgmozpwb50gnjzyf1x8pentqxdtfbh_euu6ybgfzey34o...@mail.gmail.com
- The streaming read API stuff was all committed very last minute. I
think this should have been committed much sooner. It's probably not
going to break the world; it's more likely to have performance
consequences. But if it had gone in sooner, we'd have had more time to
figure that out.
- The heap pruning refactoring work, on the other hand, seems to be at
very significant risk of breaking the world. I don't doubt the
authors, but this is some very critical stuff and a lot of it happened
very quickly right at the end.
- Incremental backup could have all sorts of terrible data-corrupting
bugs. 55a5ee30cd65886ff0a2e7ffef4ec2816fbec273 was a full brown-paper
bag level fix, so the chances of there being further problems are
probably high.
- I'm slightly worried about the TID store work (ee1b30f12, 30e144287,
667e65aac35), perhaps for no reason. Actually, the results seem really
impressive, and a very quick look at some of the commits seemed kind
of reassuring. But, like the changes to pruning and freezing, this is
making some really fundamental changes to critical code. In this case,
it's code that has evolved very little over the years and seems to
have now evolved quite a lot.
- I couldn't understand why the "Operate
XLogCtl->log{Write,Flush}Result with atomics" code was correct when I
read it. That's not to say I know it to be incorrect. But, a spinlock
protecting two variables together guarantees more than atomic access
to each of those variables separately.
There's probably a lot more to worry about; there have been so
incredibly many changes recently. But this is as far as my speculation
extends, as of now. Comments welcome. Additional concerns also
welcome, as noted above. And again, no offense is intended to anyone.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On Mon, Apr 8, 2024 at 3:32 PM Jelte Fennema-Nio wrote: > Maybe a better solution to this problem would be to spread impactful > reviews by committers more evenly throughout the year. Then there > wouldn't be such a rush to address them in the last commit fest. Spreading activity of all sorts more evenly through the year should definitely be the goal, I think. It's just not exactly clear how we can do that. -- Robert Haas EDB: http://www.enterprisedb.com
Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs
Alexander Lakhin wrote: > >> Now that ExecQueryUsingCursor() is gone, it's not clear, what does > >> the following comment mean:? > >> * We must turn off gexec_flag to avoid infinite recursion. Note that > >> * this allows ExecQueryUsingCursor to be applied to the individual > >> query > >> * results. > > Hmm, the point about recursion is still valid isn't it? I agree the > > reference to ExecQueryUsingCursor is obsolete, but I think we need to > > reconstruct what this comment is actually talking about. It's > > certainly pretty obscure ... > > Sorry, I wasn't clear enough, I meant to remove only that reference, not > the quoted comment altogether. The comment might want to stress the fact that psql honors FETCH_COUNT "on top of" \gset, so if the user issues for instance: select 'select ' || i from generate_series(1,) as i \gexec what's going to be sent to the server is a series of: BEGIN DECLARE _psql_cursor NO SCROLL CURSOR FOR select FETCH FORWARD FROM _psql_cursor (possibly repeated) CLOSE _psql_cursor COMMIT Another choice would be to ignore FETCH_COUNT and send exactly the queries that \gset produces, with the assumption that it better matches the user's expectation. Maybe that alternative was considered and the comment reflects the decision. Since the new implementation doesn't rewrite the user-supplied queries, the point is moot, and this part should be removed: "Note that this allows ExecQueryUsingCursor to be applied to the individual query results" I'll wait a bit for other comments and submit a patch. Best regards, -- Daniel Vérité https://postgresql.verite.pro/ Twitter: @DanielVerite
Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs
Alexander Lakhin writes: > 08.04.2024 18:08, Tom Lane wrote: >> Hmm, the point about recursion is still valid isn't it? I agree the >> reference to ExecQueryUsingCursor is obsolete, but I think we need to >> reconstruct what this comment is actually talking about. It's >> certainly pretty obscure ... > Sorry, I wasn't clear enough, I meant to remove only that reference, not > the quoted comment altogether. After looking at it, I realized that the comment's last sentence was also out of date, since SendQuery() isn't where the check of gexec_flag happens any more. I concluded that documenting the behavior of other functions here isn't such a hot idea, and removed both sentences in favor of expanding the relevant comments in ExecQueryAndProcessResults. While doing that, I compared the normal and chunked-fetch code paths in ExecQueryAndProcessResults more carefully, and realized that the patch was a few other bricks shy of a load: * it didn't honor pset.queryFout; * it ignored the passed-in "printQueryOpt *opt" (maybe that's always NULL, but doesn't seem like a great assumption); * it failed to call PrintQueryStatus, so that INSERT RETURNING and the like would print a status line only in non-FETCH_COUNT mode. I cleaned all that up at c21d4c416. BTW, I had to reverse-engineer the exact reasoning for the cases where we don't honor FETCH_COUNT. Most of them are clear enough, but I'm not totally sure about \watch. I wrote: + * * We're doing \watch: users probably don't want us to force use of the + * pager for that, plus chunking could break the min_rows check. It would not be terribly hard to make the chunked-fetch code path handle min_rows correctly, and AFAICS the only other thing that is_watch does differently is to not do SetResultVariables, which we could match easily enough. So this is really down to whether forcing pager mode is okay for a \watch'd query. I wonder if that was actually Daniel's reasoning for excluding \watch, and how strong that argument really is. regards, tom lane
Re: Converting README documentation to Markdown
On 2024-04-08 21:29 +0200, Daniel Gustafsson wrote: > Over in [0] I asked whether it would be worthwhile converting all our README > files to Markdown, and since it wasn't met with pitchforks I figured it would > be an interesting excercise to see what it would take (my honest gut feeling > was that it would be way too intrusive). Markdown does brings a few key > features however so IMHO it's worth attempting to see: > > * New developers are very used to reading/writing it > * Using a defined format ensures some level of consistency > * Many users and contributors new *as well as* old like reading documentation > nicely formatted in a browser > * The documentation now prints really well > * pandoc et.al can be used to render nice looking PDF's > * All the same benefits as discussed in [0] > > The plan was to follow Grubers original motivation for Markdown closely: > > "The idea is that a Markdown-formatted document should be publishable > as-is, as plain text, without looking like it’s been marked up with > tags or formatting instructions." +1 for keeping the plaintext readable. > This translates to making the least amount of changes to achieve a) retained > plain text readability at todays level, b) proper Markdown rendering, not > looking like text files in a HTML window, and c) absolutly no reflows and > minimal impact on git blame. > > Turns out we've been writing Markdown for quite some time, so it really didn't > take much at all. I renamed all the files .md and with almost just changing > whitespace achieved what I think is pretty decent results. The rendered > versions can be seen by browsing the tree below: > > https://github.com/danielgustafsson/postgres/tree/markdown > > The whitespace changes are mostly making sure that code (anything which is to > be rendered without styling really) is indented from column 0 with tab or 4 > spaces (depending on what was already used in the file) and has a blank line > before and after. This is the bulk of the changes. I've only peeked at a couple of those READMEs, but they look alright so far (at least on GitHub). Should we settle on a specific Markdown flavor[1]? Because I'm never sure if some markups only work on specific code-hosting sites. Maybe also a guide on writing Markdown that renders properly, especially with regard to escaping that may be necessary (see below). > The non-whitespace changes introduced are: > > [...] > > * In the regex README there are two file references using * as a wildcard, but > the combination of the two makes Markdown render the text between them in > italics. Wrapping these in backticks solves it, but I'm not a fan since we > don't do that elsewhere. A solution which avoids backticks would ne nice. Escaping does the trick: regc_\*.c > [...] > > * Anything inside <> is rendered as a link if it matches, so in cases where > > is used to indicatee "replace with X" I added whitespace like "< X >" which > might be a bit ugly, but works. When referencing header files with > the <> are removed to just say the header name, which seemed like the least > bad > option there. Can be escaped as well: \ [1] https://markdownguide.offshoot.io/extended-syntax/#lightweight-markup-languages -- Erik
Re: Converting README documentation to Markdown
> On 8 Apr 2024, at 22:30, Erik Wienhold wrote: > On 2024-04-08 21:29 +0200, Daniel Gustafsson wrote: > I've only peeked at a couple of those READMEs, but they look alright so > far (at least on GitHub). Should we settle on a specific Markdown > flavor[1]? Because I'm never sure if some markups only work on > specific code-hosting sites. Probably, but if we strive for maintained textual readability with avoiding most of the creative markup then we're probably close to the original version. But I agree, it should be evaluated. > Maybe also a guide on writing Markdown > that renders properly, especially with regard to escaping that may be > necessary (see below). That's a good point, if we opt for an actual format there should be some form of documentation about that format, especially if we settle for using a fraction of the capabilities of the format. >> * In the regex README there are two file references using * as a wildcard, >> but >> the combination of the two makes Markdown render the text between them in >> italics. Wrapping these in backticks solves it, but I'm not a fan since we >> don't do that elsewhere. A solution which avoids backticks would ne nice. > > Escaping does the trick: regc_\*.c Right, but that makes the plaintext version less readable than the backticks I think. > Can be escaped as well: \ ..and same with this one. It's all very subjective though. -- Daniel Gustafsson
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hi!
Attached fix for the problems found by Alexander Lakhin.
About grammar errors.
Unfortunately, I don't know English well.
Therefore, I plan (in the coming days) to show the text to specialists
who perform technical translation of documentation.
--
With best regards,
Dmitry Koval
Postgres Professional: http://postgrespro.comFrom 578b3fae50baffa3626570447d55ce4177fc6e7d Mon Sep 17 00:00:00 2001
From: Koval Dmitry
Date: Mon, 8 Apr 2024 23:10:52 +0300
Subject: [PATCH v1] Fixes for ALTER TABLE ... SPLIT/MERGE PARTITIONS ...
commands
---
doc/src/sgml/ddl.sgml | 2 +-
src/backend/commands/tablecmds.c | 12 --
src/backend/parser/parse_utilcmd.c| 38 +++
src/test/regress/expected/partition_merge.out | 29 +++---
src/test/regress/expected/partition_split.out | 13 +++
src/test/regress/sql/partition_merge.sql | 24 ++--
src/test/regress/sql/partition_split.sql | 15
7 files changed, 111 insertions(+), 22 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 8ff9a520ca..cd8304ef75 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -4410,7 +4410,7 @@ ALTER TABLE measurement
this operation is not supported for hash-partitioned tables and acquires
an ACCESS EXCLUSIVE lock, which could impact high-load
systems due to the lock's restrictive nature. For example, we can split
- the quarter partition back to monthly partitions:
+ the quarter partition back to monthly partitions:
ALTER TABLE measurement SPLIT PARTITION measurement_y2006q1 INTO
(PARTITION measurement_y2006m01 FOR VALUES FROM ('2006-01-01') TO
('2006-02-01'),
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 865c6331c1..8a98a0af48 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -21223,12 +21223,6 @@ ATExecSplitPartition(List **wqueue, AlteredTableInfo
*tab, Relation rel,
*/
splitRel = table_openrv(cmd->name, AccessExclusiveLock);
- if (splitRel->rd_rel->relkind != RELKIND_RELATION)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-errmsg("cannot split non-table partition
\"%s\"",
-
RelationGetRelationName(splitRel;
-
splitRelOid = RelationGetRelid(splitRel);
/* Check descriptions of new partitions. */
@@ -21463,12 +21457,6 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo
*tab, Relation rel,
*/
mergingPartition = table_openrv(name, AccessExclusiveLock);
- if (mergingPartition->rd_rel->relkind != RELKIND_RELATION)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-errmsg("cannot merge non-table
partition \"%s\"",
-
RelationGetRelationName(mergingPartition;
-
/*
* Checking that two partitions have the same name was before,
in
* function transformPartitionCmdForMerge().
diff --git a/src/backend/parser/parse_utilcmd.c
b/src/backend/parser/parse_utilcmd.c
index 9e3e14087f..0d5ed0079c 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -32,6 +32,7 @@
#include "catalog/heap.h"
#include "catalog/index.h"
#include "catalog/namespace.h"
+#include "catalog/partition.h"
#include "catalog/pg_am.h"
#include "catalog/pg_collation.h"
#include "catalog/pg_constraint.h"
@@ -3415,6 +3416,38 @@ transformRuleStmt(RuleStmt *stmt, const char
*queryString,
}
+/*
+ * checkPartition: check that partRelOid is partition of rel
+ */
+static void
+checkPartition(Relation rel, Oid partRelOid)
+{
+ RelationpartRel;
+
+ partRel = relation_open(partRelOid, AccessShareLock);
+
+ if (partRel->rd_rel->relkind != RELKIND_RELATION)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("\"%s\" is not a table",
+
RelationGetRelationName(partRel;
+
+ if (!partRel->rd_rel->relispartition)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("\"%s\" is not a partition",
+
RelationGetRelationName(partRel;
+
+ if (get_partition_parent(partRelOid, false) != RelationGetRelid(rel))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+errmsg("relation \"%s\" is not a partition of
relation \"%s\"",
+
RelationGetRel
Re: Table AM Interface Enhancements
On Mon, Apr 8, 2024 at 9:54 PM Robert Haas wrote: > On Mon, Apr 8, 2024 at 12:33 PM Alexander Korotkov > wrote: > > Yes, it was my mistake. I got rushing trying to fit this to FF, even doing > > significant changes just before commit. > > I'll revert this later today. It appears to be a non-trivial revert, because 041b96802e already revised the relation analyze after 27bc1772fc. That is, I would need to "backport" 041b96802e. Sorry, I'm too tired to do this today. I'll come back to this tomorrow. > Alexander, > > Exactly how much is getting reverted here? I see these, all since March 23rd: > > dd1f6b0c17 Provide a way block-level table AMs could re-use > acquire_sample_rows() > 9bd99f4c26 Custom reloptions for table AM > 97ce821e3e Fix the parameters order for > TableAmRoutine.relation_copy_for_cluster() > 867cc7b6dd Revert "Custom reloptions for table AM" > b1484a3f19 Let table AM insertion methods control index insertion > c95c25f9af Custom reloptions for table AM > 27bc1772fc Generalize relation analyze in table AM interface > 87985cc925 Allow locking updated tuples in tuple_update() and tuple_delete() > c35a3fb5e0 Allow table AM tuple_insert() method to return the different slot > 02eb07ea89 Allow table AM to store complex data structures in rd_amcache It would be discouraging to revert all of this. Some items are very simple, some items get a lot of work. I'll come back tomorrow and answer all your points. -- Regards, Alexander Korotkov
Re: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE
Hi! Thank you for your work on this issue!
Your patch required a little revision. I did this and attached the patch.
Also, I think you should add some clarification to the comments about
printing 'exact' and 'loosy' pages in show_hashagg_info function, which
you get from planstate->stats, whereas previously it was output only
from planstate. Perhaps it is enough to mention this in the comment to
the commit.
I mean this place:
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 926d70afaf..02251994c6 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -3467,26 +3467,57 @@ show_hashagg_info(AggState *aggstate,
ExplainState *es)
static void
show_tidbitmap_info(BitmapHeapScanState *planstate, ExplainState *es)
{
+ Assert(es->analyze);
+
if (es->format != EXPLAIN_FORMAT_TEXT)
{
ExplainPropertyInteger("Exact Heap Blocks", NULL,
- planstate->exact_pages, es);
+ planstate->stats.exact_pages, es);
ExplainPropertyInteger("Lossy Heap Blocks", NULL,
- planstate->lossy_pages, es);
+ planstate->stats.lossy_pages, es);
}
else
{
- if (planstate->exact_pages > 0 || planstate->lossy_pages > 0)
+ if (planstate->stats.exact_pages > 0 ||
planstate->stats.lossy_pages > 0)
{
ExplainIndentText(es);
appendStringInfoString(es->str, "Heap Blocks:");
- if (planstate->exact_pages > 0)
- appendStringInfo(es->str, " exact=%ld",
planstate->exact_pages);
- if (planstate->lossy_pages > 0)
- appendStringInfo(es->str, " lossy=%ld",
planstate->lossy_pages);
+ if (planstate->stats.exact_pages > 0)
+ appendStringInfo(es->str, " exact=%ld",
planstate->stats.exact_pages);
+ if (planstate->stats.lossy_pages > 0)
+ appendStringInfo(es->str, " lossy=%ld",
planstate->stats.lossy_pages);
appendStringInfoChar(es->str, '\n');
}
}
+
+ if (planstate->pstate != NULL)
+ {
+ for (int n = 0; n < planstate->sinstrument->num_workers; n++)
+ {
+ BitmapHeapScanInstrumentation *si =
&planstate->sinstrument->sinstrument[n];
+
+ if (si->exact_pages == 0 && si->lossy_pages == 0)
+ continue;
+
+ if (es->workers_state)
+ ExplainOpenWorker(n, es);
+
+ if (es->format == EXPLAIN_FORMAT_TEXT)
+ {
+ ExplainIndentText(es);
+ appendStringInfo(es->str, "Heap Blocks: exact=%ld
lossy=%ld\n",
+ si->exact_pages, si->lossy_pages);
+ }
+ else
+ {
+ ExplainPropertyInteger("Exact Heap Blocks", NULL,
si->exact_pages, es);
+ ExplainPropertyInteger("Lossy Heap Blocks", NULL,
si->lossy_pages, es);
+ }
+
+ if (es->workers_state)
+ ExplainCloseWorker(n, es);
+ }
+ }
}
I suggest some code refactoring (diff.diff.no-cfbot file) that allows
you to improve your code.
--
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/src/backend/executor/nodeBitmapHeapscan.c
b/src/backend/executor/nodeBitmapHeapscan.c
index 973bf56022d..ca8d94ba09a 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -634,8 +634,9 @@ ExecEndBitmapHeapScan(BitmapHeapScanState *node)
*/
if (node->sinstrument != NULL && IsParallelWorker())
{
+ BitmapHeapScanInstrumentation *si;
Assert(ParallelWorkerNumber <= node->sinstrument->num_workers);
- BitmapHeapScanInstrumentation *si =
&node->sinstrument->sinstrument[ParallelWorkerNumber];
+ si = &node->sinstrument->sinstrument[ParallelWorkerNumber];
si->exact_pages += node->stats.exact_pages;
si->lossy_pages += node->stats.lossy_pages;
}
@@ -864,7 +865,7 @@ ExecBitmapHeapInitializeDSM(BitmapHeapScanState *node,
ptr = shm_toc_allocate(pcxt->toc, size);
pstate = (ParallelBitmapHeapState *) ptr;
- ptr += MAXALIGN(sizeof(ParallelBitmapHeapState));
+ ptr += size;
if (node->ss.ps.instrument && pcxt->nworkers > 0)
sinstrument = (SharedBitmapHeapInstrumentation *) ptr;
From 8dc939749a3f0cea9ba337c020b9ccd474e7c8e3 Mon Sep 17 00:00:00 2001
From: Alena Rybakina
Date: Mon, 8 Apr 2024 23:41:41 +0300
Subject: [PATCH] Parallel Bitmap Heap Scan reports per-worker stats.
Similarly to other nodes (e.g. hash join, sort, memoize),
Bitmap Heap Scan now reports per-worker stats in the EXPLAIN
ANALYZE output. Previously only the heap blocks stats for the
leader were reported which was incomplete in parallel sca
Re: post-freeze damage control
On Tue, Apr 9, 2024 at 7:47 AM Robert Haas wrote: > - The streaming read API stuff was all committed very last minute. I > think this should have been committed much sooner. It's probably not > going to break the world; it's more likely to have performance > consequences. But if it had gone in sooner, we'd have had more time to > figure that out. OK, let me give an update on this work stream (pun intended). One reason for the delay in committing was precisely that we were fretting about regressions risks. We tried pretty hard to identify and grind down every regression we could find, and cases with outstanding not-fully-understood or examined problems in that area have been booted into the next cycle for more work: streaming bitmap heapscan, several streaming vacuum patches, and more, basically things that seem to have more complex interactions with other machinery. The only three places using streaming I/O that went in were: 041b9680: Use streaming I/O in ANALYZE. b7b0f3f2: Use streaming I/O in sequential scans. 3a352df0: Use streaming I/O in pg_prewarm. The first is a good first exercise in streaming random blocks; hopefully no one would be too upset about an unexpected small regression in ANALYZE, but as it happens it goes faster hot and cold according to all reports. The second is a good first exercise in streaming sequential blocks, and it ranges from faster to no regression, according to testing and reports. The third is less important, but it also goes faster. Of those, streaming seq scan is clearly the most relevant to real workloads that someone might be upset about, and I made a couple of choices that you might say had damage control in mind: * A conservative choice not to get into the business of the issuing new hints to the kernel for random jumps in cold scans, even though we think we probably should for better performance: more research needed precisely to avoid unexpected interactions (cf the booted bitmap heapscan where that sort of thing seems to be afoot). * A GUC to turn off I/O combining if it somehow upsets your storage in ways we didn't foresee (io_combine_limit=1). For fully cached hot scans, it does seem to be quite sensitive to tiny changes in a hot code path that I and others spent a lot of time optimising and testing during the CF. Perhaps it is possible that someone else's microarchitecture or compiler could show a regression that I don't see, and I will certainly look into it with vim and vigour if so. In that case we could consider a tiny micro-optimisation that I've shared already (it seemed a little novel so I'd rather propose it in the new cycle if I can), or, if it comes to it based on evidence and inability to address a problem quickly, reverting just b7b0f3f2 which itself is a very small patch. An aspect you didn't mention is correctness. I don't actually know how to prove that buffer manager protocols are correct beyond thinking and torture testing, ie what kind of new test harness machinery could be used to cross-check more things about buffer pool state explicitly, and that is a weakness I'm planning to look into. I realise that "these are the good ones, you should see all the stuff we decided not to commit!" is not an argument, I'm just laying out how I see the patches that went in and why I thought they were good. It's almost an architectural change, but done in tiny footsteps. I appreciate that people would have liked to see those particular tiny footsteps in some of the other fine months available for patching the tree, and some of the earlier underpinning patches that were part of the same patch series did go in around New Year, but clearly my "commit spreading" didn't go as well as planned after that (not helped by Jan/Feb summer vacation season down here). Mr Paquier this year announced his personal code freeze a few weeks back on social media, which seemed like an interesting idea I might adopt. Perhaps that is what some other people are doing without saying so, and perhaps the time they are using for that is the end of the calendar year. I might still be naturally inclined to crunch-like behaviour, but it wouldn't be at the same time as everyone else, except all the people who follow the same advice.
Re: WIP Incremental JSON Parser
On 2024-04-08 Mo 14:24, Jacob Champion wrote: Michael pointed out over at [1] that the new tiny.json is pretty inscrutable given its size, and I have to agree. Attached is a patch to pare it down 98% or so. I think people wanting to run the performance comparisons will need to come up with their own gigantic files. Let's see if we can do a bit better than that. Maybe a script to construct a larger input for the speed test from the smaller file. Should be pretty simple. Michael, with your "Jacob might be a nefarious cabal of state-sponsored hackers" hat on, is this observable enough, or do we need to get it smaller? I was thinking we may want to replace the URLs with stuff that doesn't link randomly around the Internet. Delicious in its original form is long gone. Arguably the fact that it points nowhere is a good thing. But feel free to replace it with something else. It doesn't have to be URLs at all. That happened simply because it was easy to extract from a very large piece of JSON I had lying around, probably from the last time I wrote a JSON parser :-) cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
enhance the efficiency of migrating particularly large tables
Hi Postgres hackers,
I'm reaching out to gather some comments on enhancing the efficiency of
migrating particularly large tables with significant data volumes in
PostgreSQL.
When migrating a particularly large table with a significant amount of
data, users sometimes tend to split the table into multiple segments and
utilize multiple sessions to process data from different segments in
parallel, aiming to enhance efficiency. When segmenting a large table,
it's challenging if the table lacks fields suitable for segmentation or
if the data distribution is uneven. I believe that the data volume in
each block should be relatively balanced when vacuum is enabled.
Therefore, the ctid can be used to segment a large table, and I am
thinking the entire process can be outlined as follows:
1) determine the minimum and maximum ctid.
2) calculate the number of data blocks based on the maximum and minimum
ctid.
3) generate multiple SQL queries, such as SELECT * FROM tbl WHERE ctid
>= '(xx,1)' AND ctid < '(xxx,1)'.
However, when executing SELECT min(ctid) and max(ctid), it performs a
Seq Scan, which can be slow for a large table. Is there a way to
retrieve the minimum and maximum ctid other than using the system
functions min() and max()?
Since the minimum and maximum ctid are in order, theoretically, it
should start searching from the first block and can stop as soon as it
finds the first available one when retrieving the minimum ctid.
Similarly, it should start searching in reverse order from the last
block and stop upon finding the first occurrence when retrieving the
maximum ctid. Here's a piece of code snippet:
/* scan the relation for minimum or maximum ctid */
if (find_max_ctid)
dir = BackwardScanDirection;
else
dir = ForwardScanDirection;
while ((tuple = heap_getnext(scan, dir)) != NULL)
...
The attached is a simple POC by referring to the extension pgstattuple.
Any feedback, suggestions, or alternative solutions from the community
would be greatly appreciated.
Thank you,
David
#include "postgres.h"
#include "fmgr.h"
#include "funcapi.h"
#include "miscadmin.h"
#include "nodes/primnodes.h"
#include "utils/relcache.h"
#include "catalog/namespace.h"
#include "catalog/pg_am_d.h"
#include "utils/varlena.h"
#include "storage/bufmgr.h"
#include "utils/snapmgr.h"
#include "access/heapam.h"
#include "access/relscan.h"
#include "access/tableam.h"
PG_MODULE_MAGIC;
PG_FUNCTION_INFO_V1(get_ctid);
Datum
get_ctid(PG_FUNCTION_ARGS)
{
boolfind_max_ctid = 0;
text*relname;
RangeVar*relrv;
Relationrel;
charctid[32] = {0};
if (PG_ARGISNULL(0) || PG_ARGISNULL(1))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("two parameters are required")));
relname = PG_GETARG_TEXT_PP(0);
find_max_ctid = PG_GETARG_UINT32(1);
/* open relation */
relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
rel = relation_openrv(relrv, AccessShareLock);
if (RELATION_IS_OTHER_TEMP(rel))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary tables of
other sessions")));
if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind) ||
rel->rd_rel->relkind == RELKIND_SEQUENCE)
{
TableScanDesc scan;
HeapScanDesc hscan;
HeapTuple tuple;
SnapshotData SnapshotDirty;
BlockNumber blockNumber;
OffsetNumber offsetNumber;
ScanDirection dir;
if (rel->rd_rel->relam != HEAP_TABLE_AM_OID)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("only heap AM is supported")));
/* Disable syncscan because we assume we scan from block zero
upwards */
scan = table_beginscan_strat(rel, SnapshotAny, 0, NULL, true,
false);
hscan = (HeapScanDesc) scan;
InitDirtySnapshot(SnapshotDirty);
/* scan the relation for minimum or maximum ctid */
if (find_max_ctid)
dir = BackwardScanDirection;
else
dir = ForwardScanDirection;
while ((tuple = heap_getnext(scan, dir)) != NULL)
{
CHECK_FOR_INTERRUPTS();
/* must hold a buffer lock to call
HeapTupleSatisfiesVisibility */
LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_SHARE);
if (HeapTupleSatisfiesVisibility(tuple, &SnapshotDirty,
hscan->
Re: WIP Incremental JSON Parser
On 2024-04-08 Mo 09:29, Andrew Dunstan wrote:
On 2024-04-07 Su 20:58, Tom Lane wrote:
Coverity complained that this patch leaks memory:
/srv/coverity/git/pgsql-git/postgresql/src/bin/pg_combinebackup/load_manifest.c:
212 in load_backup_manifest()
206 bytes_left -= rc;
207 json_parse_manifest_incremental_chunk(
208 inc_state, buffer, rc, bytes_left == 0);
209 }
210
211 close(fd);
CID 1596259: (RESOURCE_LEAK)
Variable "inc_state" going out of scope leaks the storage it
points to.
212 }
213
214 /* All done. */
215 pfree(buffer);
216 return result;
217 }
/srv/coverity/git/pgsql-git/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c:
488 in parse_manifest_file()
482 bytes_left -= rc;
483 json_parse_manifest_incremental_chunk(
484 inc_state, buffer, rc, bytes_left == 0);
485 }
486
487 close(fd);
CID 1596257: (RESOURCE_LEAK)
Variable "inc_state" going out of scope leaks the storage it
points to.
488 }
489
490 /* Done with the buffer. */
491 pfree(buffer);
492
493 return result;
It's right about that AFAICS, and not only is the "inc_state" itself
leaked but so is its assorted infrastructure. Perhaps we don't care
too much about that in the existing applications, but ISTM that
isn't going to be a tenable assumption across the board. Shouldn't
there be a "json_parse_manifest_incremental_shutdown()" or the like
to deallocate all the storage allocated by the parser?
yeah, probably. Will work on it.
Here's a patch. In addition to the leaks Coverity found, there was
another site in the backend code that should call the shutdown function,
and a probably memory leak from a logic bug in the incremental json
parser code. All these are fixed.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 131598bade..8ae4a62ccc 100644
--- a/src/backend/backup/basebackup_incremental.c
+++ b/src/backend/backup/basebackup_incremental.c
@@ -241,6 +241,9 @@ FinalizeIncrementalManifest(IncrementalBackupInfo *ib)
pfree(ib->buf.data);
ib->buf.data = NULL;
+ /* Done with inc_state, so release that memory too */
+ json_parse_manifest_incremental_shutdown(ib->inc_state);
+
/* Switch back to previous memory context. */
MemoryContextSwitchTo(oldcontext);
}
diff --git a/src/bin/pg_combinebackup/load_manifest.c b/src/bin/pg_combinebackup/load_manifest.c
index 9c9332cdd5..d857ea0006 100644
--- a/src/bin/pg_combinebackup/load_manifest.c
+++ b/src/bin/pg_combinebackup/load_manifest.c
@@ -208,6 +208,9 @@ load_backup_manifest(char *backup_directory)
inc_state, buffer, rc, bytes_left == 0);
}
+ /* Release the incremental state memory */
+ json_parse_manifest_incremental_shutdown(inc_state);
+
close(fd);
}
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index 90ef4b2037..9594c615c7 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -484,6 +484,9 @@ parse_manifest_file(char *manifest_path)
inc_state, buffer, rc, bytes_left == 0);
}
+ /* Release the incremental state memory */
+ json_parse_manifest_incremental_shutdown(inc_state);
+
close(fd);
}
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 44dbb7f7f9..9dfbc397c0 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -488,19 +488,18 @@ freeJsonLexContext(JsonLexContext *lex)
if (lex->errormsg)
destroyStringInfo(lex->errormsg);
+ if (lex->incremental)
+ {
+ pfree(lex->inc_state->partial_token.data);
+ pfree(lex->inc_state);
+ pfree(lex->pstack->prediction);
+ pfree(lex->pstack->fnames);
+ pfree(lex->pstack->fnull);
+ pfree(lex->pstack);
+ }
+
if (lex->flags & JSONLEX_FREE_STRUCT)
- {
- if (lex->incremental)
- {
- pfree(lex->inc_state->partial_token.data);
- pfree(lex->inc_state);
- pfree(lex->pstack->prediction);
- pfree(lex->pstack->fnames);
- pfree(lex->pstack->fnull);
- pfree(lex->pstack);
- }
pfree(lex);
- }
}
/*
diff --git a/src/common/parse_manifest.c b/src/common/parse_manifest.c
index 970a756ce8..a94e3d6b15 100644
--- a/src/common/parse_manifest.c
+++ b/src/common/parse_manifest.c
@@ -123,7 +123,6 @@ static bool parse_xlogrecptr(XLogRecPtr *result, char *input);
/*
* Set up for incremental parsing of the manifest.
- *
*/
JsonManifestParseIncrementalState *
@@ -163,6 +162,18 @@ json_parse_manifest_incremental_init(JsonManifestParseContext *context)
return incstate;
}
+/*
+ * Free an incremental state object and its contents.
+ */
+void
+json_parse_manifest_incremental_shutdown(JsonManifestParseIncrementalState *incstate)
+{
+ pfree(incstate->sem.semstate);
+ freeJsonLexContext(&(incstate->lex));
Re: enhance the efficiency of migrating particularly large tables
On Tue, 9 Apr 2024 at 09:52, David Zhang wrote:
> However, when executing SELECT min(ctid) and max(ctid), it performs a
> Seq Scan, which can be slow for a large table. Is there a way to
> retrieve the minimum and maximum ctid other than using the system
> functions min() and max()?
Finding the exact ctid seems overkill for what you need. Why you
could just find the maximum block with:
N = pg_relation_size('name_of_your_table'::regclass) /
current_Setting('block_size')::int;
and do WHERE ctid < '(N,1)';
If we wanted to optimise this in PostgreSQL, the way to do it would
be, around set_plain_rel_pathlist(), check if the relation's ctid is a
required PathKey by the same means as create_index_paths() does, then
if found, create another seqscan path without synchronize_seqscans *
and tag that with the ctid PathKey sending the scan direction
according to the PathKey direction. nulls_first does not matter since
ctid cannot be NULL.
Min(ctid) query should be able to make use of this as the planner
should rewrite those to subqueries with a ORDER BY ctid LIMIT 1.
* We'd need to invent an actual Path type for SeqScanPath as I see
create_seqscan_path() just uses the base struct Path.
synchronize_seqscans would have to become a property of that new Path
type and it would need to be carried forward into the plan and looked
at in the executor so that we always start a scan at the first or last
block.
Unsure if such a feature is worthwhile. I think maybe not for just
min(ctid)/max(ctid). However, there could be other reasons, such as
the transform OR to UNION stuff that Tom worked on a few years ago.
That needed to eliminate duplicate rows that matched both OR branches
and that was done using ctid.
David
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On 4/8/24 21:32, Jelte Fennema-Nio wrote: > On Mon, 8 Apr 2024 at 20:15, Tomas Vondra > wrote: >> I 100% understand how frustrating the lack of progress can be, and I >> agree we need to do better. I tried to move a number of stuck patches >> this CF, and I hope (and plan) to do more of this in the future. >> >> But I don't quite see how would this rule modification change the >> situation for non-committers. > > The problem that I feel I'm seeing is that committers mostly seem to > materially review large patchsets in the last two commit fests. This > might be not based in reality, but it is definitely how it feels to > me. If someone has stats on this, feel free to share. > > I'll sketch a situation: There's a big patch that some non-committer > submitted that has been sitting on the mailinglist for 6 months or > more, only being reviewed by other non-committers, which the submitter > quickly addresses. Then in the final commit fest it is finally > reviewed by a committer, and they say it requires significant changes. > Right now, the submitter can decide to quickly address those changes, > and hope to keep the attention of this committer, to hopefully get it > merged before the deadline after probably a few more back and forths. > But this new rule would mean that those significant changes would be a > reason not to put it in the upcoming release. Which I expect would > first of all really feel like a slap in the face to the submitter, > because it's not their fault that those changes were only requested in > the last commit fest. This would then likely result in the submitter > not following up quickly (why make time right at this moment, if > you're suddenly going to have another full year), which would then > cause the committer to lose context of the patch and thus interest in > the patch. And then finally getting into the exact same situation next > year in the final commit fest, when some other committer didn't agree > with the redesign of the first one and request a new one pushing it > another year. > FWIW I have no doubt this problem is very real. It has never been easy to get stuff reviewed/committed, and I doubt it improved in last couple years, considering how the traffic on pgsql-hackers exploded :-( > So yeah, I definitely agree with Matthias. I definitely feel like his > rule would seriously impact contributions made by non-committers. > > Maybe a better solution to this problem would be to spread impactful > reviews by committers more evenly throughout the year. Then there > wouldn't be such a rush to address them in the last commit fest. Right. I think that's mostly what I was aiming for, although I haven't made it very clear/explicit. But yeah, if the consequence of the "rule" was that some of the patches are neglected entirely, that'd be pretty terrible - both for the project and for the contributors. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On 2024-04-08 Mo 12:07, Alvaro Herrera wrote: On 2024-Apr-08, Robert Haas wrote: And maybe we need to think of a way to further mitigate this crush of last minute commits. e.g. In the last week, you can't have more feature commits, or more lines of insertions in your commits, than you did in the prior 3 weeks combined. I don't know. I think this mad rush of last-minute commits is bad for the project. Another idea is to run a patch triage around mid March 15th, with the intention of punting patches to the next cycle early enough. But rather than considering each patch in its own merits, consider the responsible _committers_ and the load that they are reasonably expected to handle: determine which patches each committer deems his or her responsibility for the rest of that March commitfest, and punt all the rest. That way we have a reasonably vetted amount of effort that each committer is allowed to spend for the remainder of that commitfest. Excesses should be obvious enough and discouraged. I quite like the triage idea. But I think there's also a case for being more a bit more flexible with those patches we don't throw out. A case close to my heart: I'd have been very sad if the NESTED piece of JSON_TABLE hadn't made the cut, which it did with a few hours to spare, and I would not have been alone, far from it. I'd have been happy to give Amit a few more days or a week if he needed it, for a significant headline feature. I know there will be those who say it's the thin end of the wedge and rulez is rulez, but this is my view. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: CASE control block broken by a single line comment
Erik Wienhold writes:
> On 2024-04-07 06:33 +0200, Tom Lane wrote:
>> I suspect it'd be much more robust if we could remove the comment from
>> the expr->query string. No idea how hard that is.
> I slept on it and I think this can be fixed by tracking the end of the
> last token before THEN and use that instead of yylloc in the call to
> plpgsql_append_source_text. We already already track the token length
> in plpgsql_yyleng but don't make it available outside pl_scanner.c yet.
> Attached v2 tries to do that. But it breaks other test cases, probably
> because the calculation of endlocation is off. I'm missing something
> here.
I poked at this and found that the failures occur when the patched
code decides to trim an expression like "_r.v" to just "_r", naturally
breaking the semantics completely. That happens because when
plpgsql_yylex recognizes a compound token, it doesn't bother to
adjust the token length to include the additional word(s). I vaguely
remember having thought about that when writing the lookahead logic,
and deciding that it wasn't worth the trouble -- but now it is.
Up to now, the only thing we did with plpgsql_yyleng was to set the
cutoff point for text reported by plpgsql_yyerror. Extending the
token length changes reports like this:
regression=# do $$ declare r record; r.x$$;
ERROR: syntax error at or near "r"
LINE 1: do $$ declare r record; r.x$$;
^
to this:
regression=# do $$ declare r record; r.x$$;
ERROR: syntax error at or near "r.x"
LINE 1: do $$ declare r record; r.x$$;
^
which seems like strictly an improvement to me (the syntax error is
premature EOF, which is after the "x"); but in any case it's minor
enough to not be worth worrying about.
Looking around, I noticed that we *have* had a similar case in the
past, which 4adead1d2 noticed and worked around by suppressing the
whitespace-trimming action in read_sql_construct. We could probably
reach a near-one-line fix for the current problem by passing
trim=false in the CASE calls, but TBH that discovery just reinforces
my feeling that we need a cleaner fix. The attached v3 reverts
the make-trim-optional hack that 4adead1d2 added, since we don't
need or want the manual trimming anymore.
With this in mind, I find the other manual whitespace trimming logic,
in make_execsql_stmt(), quite scary; but it looks somewhat nontrivial
to get rid of it. (The problem is that parsing of an INTO clause
will leave us with a pushed-back token as next, and then we don't
know where the end of the token before that is.) Since we don't
currently do anything as crazy as combining execsql statements,
I think the problem is only latent, but still...
Anyway, the attached works for me.
regards, tom lane
diff --git a/src/pl/plpgsql/src/expected/plpgsql_control.out b/src/pl/plpgsql/src/expected/plpgsql_control.out
index 328bd48586..ccd4f54704 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_control.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_control.out
@@ -681,3 +681,20 @@ select case_test(13);
other
(1 row)
+-- test line comment between WHEN and THEN
+create or replace function case_comment(int) returns text as $$
+begin
+ case $1
+when 1 -- comment before THEN
+ then return 'one';
+else
+ return 'other';
+ end case;
+end;
+$$ language plpgsql immutable;
+select case_comment(1);
+ case_comment
+--
+ one
+(1 row)
+
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index bef33d58a2..a29d2dfacd 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -66,7 +66,6 @@ static PLpgSQL_expr *read_sql_construct(int until,
RawParseMode parsemode,
bool isexpression,
bool valid_sql,
- bool trim,
int *startloc,
int *endtoken);
static PLpgSQL_expr *read_sql_expression(int until,
@@ -895,7 +894,7 @@ stmt_perform : K_PERFORM
*/
new->expr = read_sql_construct(';', 0, 0, ";",
RAW_PARSE_DEFAULT,
- false, false, true,
+ false, false,
&startloc, NULL);
/* overwrite "perform" ... */
memcpy(new->expr->query, " SELECT", 7);
@@ -981,7 +980,7 @@ stmt_assign : T_DATUM
plpgsql_push_back_token(T_DATUM);
new->expr = read_sql_construct(';', 0, 0, ";",
pmode,
- false, true, true,
+ false, true,
NULL, NULL);
$$ = (PLpgSQL_stmt *) new;
@@ -1474,7 +1473,6 @@ for_control : for_variable K_IN
RAW_PARSE_DEFAULT,
true,
false,
- true,
&expr1loc,
&tok);
@@ -1879,7 +1877,7 @@ stmt_raise : K_RAISE
expr = read_sql_construct(',', ';', K_USING,
", or ; or USING",
RAW_PARSE_PLPGSQL_EXPR,
- true, tru
Re: post-freeze damage control
On Tue, Apr 09, 2024 at 09:35:00AM +1200, Thomas Munro wrote: > Mr Paquier this year announced his personal code freeze a few weeks > back on social media, which seemed like an interesting idea I might > adopt. Perhaps that is what some other people are doing without > saying so, and perhaps the time they are using for that is the end of > the calendar year. I might still be naturally inclined to crunch-like > behaviour, but it wouldn't be at the same time as everyone else, > except all the people who follow the same advice. That's more linked to the fact that I was going silent without a laptop for a few weeks before the end of the release cycle, and a way to say to not count on me, while I was trying to keep my room clean to avoid noise for others who would rush patches. It is a vacation period for schools in Japan as the fiscal year finishes at the end of March, while the rest of the world still studies/works, so that makes trips much easier with areas being less busy when going abroad. If you want to limit commit activity during this period, the answer is simple then: require that all the committers live in Japan. Jokes apart, I really try to split commit effort across the year and not rush things at the last minute. If something's not agreed upon and commit-ready by the 15th of March, the chances that I would apply it within the release cycle are really slim. That's a kind of personal policy I have in place for a few years now. -- Michael signature.asc Description: PGP signature
