Re: Broken make dependency somewhere near win32ver.o?
On Tue, Mar 22, 2022 at 4:14 PM Andres Freund wrote: > The problem looks to be that pg_dumpall doesn't have a dependency on OBJs, > which in turn is what contains the dependency on WIN32RES, which in turn > contains win32ver.o. So the build succeeds if pg_dump/restores's dependencies > are built first, but not if pg_dumpall starts to be built before that... > > Seems we just need to add $(WIN32RES) to pg_dumpall: ? Ah, yeah, that looks right. I don't currently have a mingw setup to test, but clearly $(WIN32RES) is passed to $(CC) despite not being listed as a dependency.
Re: Add sub-transaction overflow status in pg_stat_activity
On Tue, Mar 22, 2022 at 5:15 AM Andres Freund wrote: > There seems to be some agreement on this (I certainly do agree). Thus it seems > we should mark the CF entry as rejected? > > It's been failing on cfbot for weeks... > https://cirrus-ci.com/task/5289336424890368?logs=docs_build#L347 I have marked it rejected. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: New Object Access Type hooks
On Fri, Mar 18, 2022 at 4:22 PM Mark Dilger wrote: > (FYI, I got a test failure from src/test/recovery/t/013_crash_restart.pl when > testing v1-0001. I'm not sure yet what that is about.) Doesn't look like 0001 has anything to do with that... Are you on a Mac? Did it look like this recent failure from CI? https://cirrus-ci.com/task/4686108033286144 https://api.cirrus-ci.com/v1/artifact/task/4686108033286144/log/src/test/recovery/tmp_check/log/regress_log_013_crash_restart https://api.cirrus-ci.com/v1/artifact/task/4686108033286144/log/src/test/recovery/tmp_check/log/013_crash_restart_primary.log I have no idea what is going on there, but searching for discussion brought me here...
Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints
On Mon, Mar 21, 2022 at 11:53 PM Robert Haas wrote: > > On Mon, Mar 21, 2022 at 11:21 AM Dilip Kumar wrote: > > I tried to debug the case but I realized that somehow > > CHECK_FOR_INTERRUPTS() is not calling the > > AcceptInvalidationMessages() and I could not find the same while > > looking into the code as well. While debugging I noticed that > > AcceptInvalidationMessages() is called multiple times but that is only > > through LockRelationId() but while locking the relation we had already > > closed the previous smgr because at a time we keep only one smgr open. > > And that's the reason it is not hitting the issue which we think it > > could. Is there any condition under which it will call > > AcceptInvalidationMessages() through CHECK_FOR_INTERRUPTS() ? because > > I could not see while debugging as well as in code. > > Yeah, I think the reason you can't find it is that it's not there. I > was confused in what I wrote earlier. I think we only process sinval > catchups when we're idle, not at every CHECK_FOR_INTERRUPTS(). And I > think the reason for that is precisely that it would be hard to write > correct code otherwise, since invalidations might then get processed > in a lot more places. So ... I guess all we really need to do here is > avoid assuming that the results of smgropen() are valid across any > code that might acquire relation locks. Which I think the code already > does. > > But on a related note, why doesn't CreateDatabaseUsingWalLog() acquire > locks on both the source and destination relations? It looks like > you're only taking locks for the source template database ... but I > thought the intention here was to make sure that we didn't pull pages > into shared_buffers without holding a lock on the relation and/or the > database? I suppose the point is that while the template database > might be concurrently dropped, nobody can be doing anything > concurrently to the target database because nobody knows that it > exists yet. Still, I think that this would be the only case where we > let pages into shared_buffers without a relation or database lock, > though maybe I'm confused about this point, too. If not, perhaps we > should consider locking the target database OID and each relation OID > as we are copying it? > > I guess I'm imagining that there might be more code pathways in the > future that want to ensure that there are no remaining buffers for > some particular database or relation OID. It seems natural to want to > be able to take some lock that prevents buffers from being added, and > then go and get rid of all the ones that are there already. But I > admit I can't quite think of a concrete case where we'd want to do > something like this where the patch as coded would be a problem. I'm > just thinking perhaps taking locks is fairly harmless and might avoid > some hypothetical problem later. > > Thoughts? I think this make sense. I haven't changed the original patch as you told you were improving on some comments, so in order to avoid conflict I have created this add on patch. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index 9636688..5d0750f 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -460,12 +460,6 @@ CreateDatabaseUsingWalLog(Oid src_dboid, Oid dst_dboid, Oid src_tsid, Oid dst_ts Assert(rnodelist != NIL); /* - * Database id is common for all the relation so set it before entering to - * the loop. - */ - relid.dbId = src_dboid; - - /* * Iterate over each relfilenode and copy the relation data block by block * from source database to the destination database. */ @@ -488,7 +482,15 @@ CreateDatabaseUsingWalLog(Oid src_dboid, Oid dst_dboid, Oid src_tsid, Oid dst_ts dstrnode.dbNode = dst_dboid; dstrnode.relNode = srcrnode.relNode; - /* Acquire the lock on relation before start copying. */ + /* + * Acquire relation lock on the source and the destination relation id + * before start copying. + */ + relid.dbId = src_dboid; + relid.relId = relinfo->reloid; + LockRelationId(, AccessShareLock); + + relid.dbId = dst_dboid; relid.relId = relinfo->reloid; LockRelationId(, AccessShareLock); @@ -1218,6 +1220,17 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt) InvokeObjectPostCreateHook(DatabaseRelationId, dboid, 0); /* + * Acquire a lock on the target database, although this is a new database + * and no one else should be able to see it. But if we are using wal log + * strategy then we are going to access the relation pages using shared + * buffers. Therefore, it would be better to take the database lock. And, + * later we would acquire the relation lock as and when we would access the + * individual relations' shared buffers. + */ + if (dbstrategy == CREATEDB_WAL_LOG) + LockSharedObject(DatabaseRelationId, dboid, 0, ExclusiveLock); + + /* * Once we start
Re: [PATCH] Accept IP addresses in server certificate SANs
At Fri, 18 Mar 2022 16:38:57 +0900 (JST), Kyotaro Horiguchi wrote in > At Thu, 17 Mar 2022 21:55:07 +, Jacob Champion > wrot> Thanks! .. and some nitpicks..(Sorry) > > fe-secure-common.c doesn't need netinet/in.h. > > > +++ b/src/include/utils/inet.h > .. > +#include "common/inet-common.h" > > I'm not sure about the project policy on #include practice, but I > think it is the common practice not to include headers that are not > required by the file itself. In this case, fe-secure-common.h itself > doesn't need the include. Instead, fe-secure-openssl.c and > fe-secure-common.c needs the include. I noticed that this doesn't contain doc changes. https://www.postgresql.org/docs/current/libpq-ssl.html > In verify-full mode, the host name is matched against the > certificate's Subject Alternative Name attribute(s), or against the > Common Name attribute if no Subject Alternative Name of type dNSName > is present. If the certificate's name attribute starts with an > asterisk (*), the asterisk will be treated as a wildcard, which will > match all characters except a dot (.). This means the certificate will > not match subdomains. If the connection is made using an IP address > instead of a host name, the IP address will be matched (without doing > any DNS lookups). This refers to dNSName, so we should revise this so that it describes the new behavior. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Out-of-tree certificate interferes ssltest
On Fri, Mar 18, 2022 at 06:15:28PM -0400, Andrew Dunstan wrote: > On 3/17/22 21:02, Michael Paquier wrote: >> Another thing that Horiguchi-san has pointed out upthread (?) is 003, >> where it is also possible to trigger failures once the environment is >> hijacked. The attached allows the full test suite to pass without >> issues on my side. > > LGTM Thanks for looking at it. I have been able to check this stuff across all the supported branches, and failures happen down to 10. That's easy enough to see once you know how to break the tests. There were a couple of conflicts, but nothing impossible to fix, so applied down to v10. REL_11_STABLE had one extra failure in 002_scram.pl that was already fixed in v12~. -- Michael signature.asc Description: PGP signature
Re: Commitfest manager for 2022-03
On Mon, 21 Mar 2022 at 21:48, Andres Freund wrote: > > Hi, > > On 2022-02-26 16:12:27 -0500, Greg Stark wrote: > > I do have the time available. What I don't necessarily have is insight > > into everything that needs to be done, especially behind the scenes. > > One thing that desperately needs doing, particularly during the last > commitfest, is looking through CF entries and pruning stuff that shouldn't be > there anymore or that are in the wrong state. Thanks > One can't just automatically mark all failing runs as "waiting on author" > because sometimes there are issues with somebody else posting an incremental > diff confusing cfbot or spurious test failures... What I'm seeing is patches that are failing with either the 027_stream_regress.pl failure that I see is being actively worked on in another thread or simply a timeout which I'm not sure but may be the same issue? But I'll do a pass and then do another pass later in the week when those problems may have been ironed out. -- greg
Re: simplifying foreign key/RI checks
On Mon, Mar 14, 2022 at 6:28 PM Zhihong Yu wrote: > On Mon, Mar 14, 2022 at 1:33 AM Amit Langote wrote: >> On Tue, Jan 18, 2022 at 3:30 PM Amit Langote wrote: >> > v13 is attached. >> >> I noticed that the recent 641f3dffcdf's changes to >> get_constraint_index() made it basically unusable for this patch's >> purposes. >> >> Reading in the thread that led to 641f3dffcdf why >> get_constraint_index() was changed the way it was, I invented in the >> attached updated patch a get_fkey_constraint_index() that is local to >> ri_triggers.c for use by the new ri_ReferencedKeyExists(), replacing >> get_constraint_index() that no longer gives it the index it's looking >> for. >> > > Hi, > + partkey_isnull[j] = (key_nulls[k] == 'n' ? true : false); > > The above can be shortened as: > > partkey_isnull[j] = key_nulls[k] == 'n'; > > +* May neeed to cast each of the individual values of the foreign key > > neeed -> need Both fixed, thanks. -- Amit Langote EDB: http://www.enterprisedb.com v15-0001-Add-isolation-tests-for-snapshot-behavior-in-ri_.patch Description: Binary data v15-0002-Avoid-using-SPI-for-some-RI-checks.patch Description: Binary data
Re: A test for replay of regression tests
On Tue, Mar 22, 2022 at 4:31 PM Masahiko Sawada wrote:
> SELECT pg_relation_size('vac_truncate_test') = 0;
> ?column?
> --
> - t
> + f
Thanks. Ahh, déjà vu... this probably needs the same treatment as
b700f96c and 3414099c provided for the reloptions test. Well, at
least the first one. Here's a patch like that.
From ba05f03c202bf66c7692787ec24ece13b193a897 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Tue, 22 Mar 2022 16:54:42 +1300
Subject: [PATCH] Try to stabilize vacuum test.
As commits b700f96c and 3414099c did for the reloptions test, take
measures to make sure that vacuum always truncates the table.
Discussion: https://postgr.es/m/CAD21AoCNoWjYkdEtr%2BVDoF9v__V905AedKZ9iF%3DArgCtrbxZqw%40mail.gmail.com
---
src/test/regress/expected/vacuum.out | 6 +++---
src/test/regress/sql/vacuum.sql | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 3e70e4c788..c63a157e5f 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -165,19 +165,19 @@ VACUUM (INDEX_CLEANUP FALSE) vaccluster;
VACUUM (INDEX_CLEANUP AUTO) vactst; -- index cleanup option is ignored if no indexes
VACUUM (INDEX_CLEANUP FALSE, FREEZE TRUE) vaccluster;
-- TRUNCATE option
-CREATE TABLE vac_truncate_test(i INT NOT NULL, j text)
+CREATE TEMP TABLE vac_truncate_test(i INT NOT NULL, j text)
WITH (vacuum_truncate=true, autovacuum_enabled=false);
INSERT INTO vac_truncate_test VALUES (1, NULL), (NULL, NULL);
ERROR: null value in column "i" of relation "vac_truncate_test" violates not-null constraint
DETAIL: Failing row contains (null, null).
-VACUUM (TRUNCATE FALSE) vac_truncate_test;
+VACUUM (TRUNCATE FALSE, DISABLE_PAGE_SKIPPING) vac_truncate_test;
SELECT pg_relation_size('vac_truncate_test') > 0;
?column?
--
t
(1 row)
-VACUUM vac_truncate_test;
+VACUUM (DISABLE_PAGE_SKIPPING) vac_truncate_test;
SELECT pg_relation_size('vac_truncate_test') = 0;
?column?
--
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 18cb7fd08a..9faa8a34a6 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -147,12 +147,12 @@ VACUUM (INDEX_CLEANUP AUTO) vactst; -- index cleanup option is ignored if no ind
VACUUM (INDEX_CLEANUP FALSE, FREEZE TRUE) vaccluster;
-- TRUNCATE option
-CREATE TABLE vac_truncate_test(i INT NOT NULL, j text)
+CREATE TEMP TABLE vac_truncate_test(i INT NOT NULL, j text)
WITH (vacuum_truncate=true, autovacuum_enabled=false);
INSERT INTO vac_truncate_test VALUES (1, NULL), (NULL, NULL);
-VACUUM (TRUNCATE FALSE) vac_truncate_test;
+VACUUM (TRUNCATE FALSE, DISABLE_PAGE_SKIPPING) vac_truncate_test;
SELECT pg_relation_size('vac_truncate_test') > 0;
-VACUUM vac_truncate_test;
+VACUUM (DISABLE_PAGE_SKIPPING) vac_truncate_test;
SELECT pg_relation_size('vac_truncate_test') = 0;
VACUUM (TRUNCATE FALSE, FULL TRUE) vac_truncate_test;
DROP TABLE vac_truncate_test;
--
2.30.2
Re: Per-table storage parameters for TableAM/IndexAM extensions
On Tue, Mar 22, 2022 at 7:24 AM Andres Freund wrote: > > Hi, > > On 2022-02-24 12:26:08 +0530, Sadhuprasad Patro wrote: > > I have added a dummy test module for table AM and did the document > > change in the latest patch attached... > > The test module doesn't build on windows, unfortunately... Looks like you need > to add PGDLLIMPORT to a few variables: > [01:26:18.539] > c:\cirrus\src\test\modules\dummy_table_am\dummy_table_am.c(488): warning > C4700: uninitialized local variable 'rel' used > [c:\cirrus\dummy_table_am.vcxproj] > [01:26:18.539] dummy_table_am.obj : error LNK2001: unresolved external symbol > synchronize_seqscans [c:\cirrus\dummy_table_am.vcxproj] > [01:26:18.539] .\Debug\dummy_table_am\dummy_table_am.dll : fatal error > LNK1120: 1 unresolved externals [c:\cirrus\dummy_table_am.vcxproj] > [01:26:18.539] 1 Warning(s) > [01:26:18.539] 2 Error(s) > > https://cirrus-ci.com/task/5067519584108544?logs=build#L2085 > > Marked the CF entry as waiting-on-author. HI, Thank you for the feedback Andres. I will take care of the same. As of now attached is a new patch on this to support the addition of new option parameters or drop the old parameters through ALTER TABLE command. Need some more testing on this, which is currently in progress. Providing the patch to get early feedback in case of any major comments... New Command: ALTER TABLE name SET ACCESS METHOD amname [ OPTIONS ( ADD | DROP option 'value' [, ... ] ) ]; Thanks & Regards SadhuPrasad http://www.EnterpriseDB.com v4-0001-PATCH-V4-Per-table-storage-parameters-for-TableAM.patch Description: Binary data
Re: A test for replay of regression tests
i,
On Mon, Mar 21, 2022 at 5:45 AM Thomas Munro wrote:
>
> On Mon, Mar 21, 2022 at 2:34 AM Andrew Dunstan wrote:
> > On 3/20/22 05:36, Thomas Munro wrote:
> > > On Sun, Mar 20, 2022 at 5:20 PM Thomas Munro
> > > wrote:
> > >> I'll try to come up with the perl needed to see the regression.diffs
> > >> next time...
> > > Here's my proposed change to achieve that.
> >
> > I think that's OK.
>
> Thanks for looking! Pushed.
FYI idiacanthus failed 027_stream_regress.pl:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=idiacanthus=2022-03-22%2001%3A58%3A04
The log shows:
=== dumping
/home/bf/build/buildfarm-idiacanthus/HEAD/pgsql.build/src/test/recovery/tmp_check/regression.diffs
===
diff -U3
/home/bf/build/buildfarm-idiacanthus/HEAD/pgsql/src/test/regress/expected/vacuum.out
/home/bf/build/buildfarm-idiacanthus/HEAD/pgsql.build/src/test/recovery/tmp_check/results/vacuum.out
---
/home/bf/build/buildfarm-idiacanthus/HEAD/pgsql/src/test/regress/expected/vacuum.out
2021-07-01 19:00:01.936659446 +0200
+++
/home/bf/build/buildfarm-idiacanthus/HEAD/pgsql.build/src/test/recovery/tmp_check/results/vacuum.out
2022-03-22 03:28:09.813377179 +0100
@@ -181,7 +181,7 @@
SELECT pg_relation_size('vac_truncate_test') = 0;
?column?
--
- t
+ f
(1 row)
VACUUM (TRUNCATE FALSE, FULL TRUE) vac_truncate_test;
=== EOF ===
not ok 2 - regression tests pass
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Re: Add index scan progress to pg_stat_progress_vacuum
Hi,
On 2022-03-16 21:47:49 +, Imseih (AWS), Sami wrote:
> From 85c47dfb3bb72f764b9052e74a7282c19ebd9898 Mon Sep 17 00:00:00 2001
> From: "Sami Imseih (AWS)"
> Date: Wed, 16 Mar 2022 20:39:52 +
> Subject: [PATCH 1/1] Add infrastructure for parallel progress reporting
>
> Infrastructure to allow a parallel worker to report
> progress. In a PARALLEL command, the workers and
> leader can report progress using a new pgstat_progress
> API.
What happens if we run out of memory for hashtable entries?
> +void
> +pgstat_progress_update_param_parallel(int leader_pid, int index, int64 val)
> +{
> + ProgressParallelEntry *entry;
> + bool found;
> +
> + LWLockAcquire(ProgressParallelLock, LW_EXCLUSIVE);
> +
> + entry = (ProgressParallelEntry *) hash_search(ProgressParallelHash,
> _pid, HASH_ENTER, );
> +
> + /*
> + * If the entry is not found, set the value for the index'th member,
> + * else increment the current value of the index'th member.
> + */
> + if (!found)
> + entry->st_progress_param[index] = val;
> + else
> + entry->st_progress_param[index] += val;
> +
> + LWLockRelease(ProgressParallelLock);
> +}
I think that's an absolute no-go. Adding locking to progress reporting,
particularly a single central lwlock, is going to *vastly* increase the
overhead incurred by progress reporting.
Greetings,
Andres Freund
Re: Why is src/test/modules/committs/t/002_standby.pl flaky?
On Tue, Mar 22, 2022 at 4:13 PM Tom Lane wrote: > Thomas Munro writes: > > I have a new socket abstraction patch that should address the known > > Windows socket/event bugs, but it's a little bigger than I thought it > > would be, not quite ready, and now too late to expect people to review > > for 15, so I think it should go into the next cycle. I've bounced > > https://commitfest.postgresql.org/37/3523/ into the next CF. We'll > > need to do something like 75674c7e for master. > > OK. You want me to push 75674c7e to HEAD? Thanks, yes, please do.
Re: Add index scan progress to pg_stat_progress_vacuum
Sorry for the late reply. On Tue, Mar 15, 2022 at 1:20 AM Imseih (AWS), Sami wrote: > > >I'm still unsure the current design of 0001 patch is better than other > >approaches we’ve discussed. Even users who don't use parallel vacuum > >are forced to allocate shared memory for index vacuum progress, with > >GetMaxBackends() entries from the beginning. Also, it’s likely to > >extend the progress tracking feature for other parallel operations in > >the future but I think the current design is not extensible. If we > >want to do that, we will end up creating similar things for each of > >them or re-creating index vacuum progress tracking feature while > >creating a common infra. It might not be a problem as of now but I'm > >concerned that introducing a feature that is not extensible and forces > >users to allocate additional shmem might be a blocker in the future. > >Looking at the precedent example, When we introduce the progress > >tracking feature, we implemented it in an extensible way. On the other > >hand, others in this thread seem to agree with this approach, so I'd > >like to leave it to committers. > > Thanks for the review! > > I think you make strong arguments as to why we need to take a different > approach now than later. > > Flaws with current patch set: > > 1. GetMaxBackends() is a really heavy-handed overallocation of a shared > memory serving a very specific purpose. > 2. Going with the approach of a vacuum specific hash breaks the design of > progress which is meant to be extensible. > 3. Even if we go with this current approach as an interim solution, it will > be a real pain in the future. > > With that said, v7 introduces the new infrastructure. 0001 includes the new > infrastructure and 0002 takes advantage of this. > > This approach is the following: > > 1. Introduces a new API called pgstat_progress_update_param_parallel along > with some others support functions. This new infrastructure is in > backend_progress.c > > 2. There is still a shared memory involved, but the size is capped to " > max_worker_processes" which is the max to how many parallel workers can be > doing work at any given time. The shared memory hash includes a > st_progress_param array just like the Backend Status array. I think that there is a corner case where a parallel operation could not perform due to the lack of a free shared hash entry, because there is a window between a parallel worker exiting and the leader deallocating the hash table entry. BTW have we discussed another idea I mentioned before that we have the leader process periodically check the number of completed indexes and advertise it in its progress information? I'm not sure which one is better but this idea would require only changes of vacuum code and probably simpler than the current idea. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Broken make dependency somewhere near win32ver.o?
Hi,
On 2022-03-22 15:47:13 +1300, Thomas Munro wrote:
> Here's a strange one-off failure seen on CI[1], in the
> CompilerWarnings task where we check that mingw cross-compile works:
>
> [10:48:29.045] time make -s -j${BUILD_JOBS} world-bin
> [10:48:38.705] x86_64-w64-mingw32-gcc: error: win32ver.o: No such file
> or directory
> [10:48:38.705] make[3]: *** [Makefile:44: pg_dumpall] Error 1
> [10:48:38.705] make[3]: *** Waiting for unfinished jobs
> [10:48:38.709] make[2]: *** [Makefile:43: all-pg_dump-recurse] Error 2
> [10:48:38.709] make[2]: *** Waiting for unfinished jobs
> [10:48:38.918] make[1]: *** [Makefile:42: all-bin-recurse] Error 2
> [10:48:38.918] make: *** [GNUmakefile:21: world-bin-src-recurse] Error 2
>
> I guess this implies a dependency problem somewhere around
> src/makefiles/Makefile.win32 but I'm not familiar with how that .rc
> stuff is supposed to work and I figured I'd mention it here in case
> it's obvious to someone else...
Oh. I think I figured out how to reproduce it reliably:
make -s clean
make -j pg_dumpall -C src/bin/pg_dump/
...
x86_64-w64-mingw32-gcc: error: win32ver.o: No such file or directory
The problem looks to be that pg_dumpall doesn't have a dependency on OBJs,
which in turn is what contains the dependency on WIN32RES, which in turn
contains win32ver.o. So the build succeeds if pg_dump/restores's dependencies
are built first, but not if pg_dumpall starts to be built before that...
Seems we just need to add $(WIN32RES) to pg_dumpall: ?
Greetings,
Andres Freund
Re: Why is src/test/modules/committs/t/002_standby.pl flaky?
Thomas Munro writes: > I have a new socket abstraction patch that should address the known > Windows socket/event bugs, but it's a little bigger than I thought it > would be, not quite ready, and now too late to expect people to review > for 15, so I think it should go into the next cycle. I've bounced > https://commitfest.postgresql.org/37/3523/ into the next CF. We'll > need to do something like 75674c7e for master. OK. You want me to push 75674c7e to HEAD? regards, tom lane
Re: shared-memory based stats collector - v67
At Mon, 21 Mar 2022 14:30:17 -0700, Andres Freund wrote in > Hi, > > Attached is v67 of the patch. Changes: Thanks for the lot of work on this. > > This is also painful to maintain. Mostly kept separate from 0007 for easier > > reviewing: > > - 0009-pgstat-reorder-file-pgstat.c-pgstat.h-contents.patch > > Planning to commit this soon (it's now 0001). Doing a last few passes of > readthrough / polishing. This looks like committed. > > I don't yet know what we should do with other users of > > PG_STAT_TMP_DIR. There's no need for it for pgstat.c et al anymore. Not sure > > that pg_stat_statement is enough of a reason to keep the > > stats_temp_directory > > GUC around? > > - 0019-pgstat-wip-remove-stats_temp_directory.patch > > Still unclear. Might raise this separately for higher visibility. > > > > Right now we reset stats for replicas, even if we start from a shutdown > > checkpoint. That seems pretty unnecessary with this patch: > > - 0021-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch > > Might raise this in another thread for higher visibility. > > > > The biggest todos are: > > - Address all the remaining AFIXMEs and XXXs > > - add longer explanation of architecture to pgstat.c (or a README) > > - make naming not "a pain in the neck": [1] > > - lots of polishing > > - run benchmarks - I've done so in the past, but not recently > > Still TBD > > - revise docs > > Kyotaro-san, maybe you could do a first pass? Docs.. Yeah I'll try it. > > - Further improve our stats test coverage - there's a crapton not covered, > > despite 0017: > > - test WAL replay with stats (stats for dropped tables are removed etc) > > - test crash recovery and "invalid stats file" paths > > - lot of the pg_stat_ views like bgwriter, pg_stat_database have zero > > coverage today > > That's gotten a lot better with Melanie's tests, still a bit further to go. I > think she's found at least one more small bug that's not yet fixed here. > > > > - perhaps 0014 can be further broken down - it's still uncomfortably large > > Things that I think can be split out: > - Encapsulate "if (pgStatSock == PGINVALID_SOCKET || !pgstat_track_counts)" > style tests in a helper function. Then just the body needs to be changed, > rather than a lot of places needing such checks. > > Yep, that's it. I don't really see anything else that wouldn't be too > awkward. Would welcome suggestions!. I'm overwhelmed by the amout, but I'm going to look into them. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: On login trigger: take three
Hi, On 2022-03-18 17:03:39 +0100, Daniel Gustafsson wrote: > > On 18 Mar 2022, at 08:24, a.soko...@postgrespro.ru wrote: > > > - auth_extra => [ '--create-role', 'repl_role' ]); > > + auth_extra => [ '--create-role', 'repl_role,regress_user' ]); > > Looking at the test in question it's not entirely clear to me what the > original > author really intended here, so I've changed it up a bit to actually test > something and removed the need for the regress_user role. I've also fixed the > silly mistake highlighted in the postgresql.conf.sample test. docs fail to build: https://cirrus-ci.com/task/5556234047717376?logs=docs_build#L349 Greetings, Andres Freund
Broken make dependency somewhere near win32ver.o?
Hi,
Here's a strange one-off failure seen on CI[1], in the
CompilerWarnings task where we check that mingw cross-compile works:
[10:48:29.045] time make -s -j${BUILD_JOBS} world-bin
[10:48:38.705] x86_64-w64-mingw32-gcc: error: win32ver.o: No such file
or directory
[10:48:38.705] make[3]: *** [Makefile:44: pg_dumpall] Error 1
[10:48:38.705] make[3]: *** Waiting for unfinished jobs
[10:48:38.709] make[2]: *** [Makefile:43: all-pg_dump-recurse] Error 2
[10:48:38.709] make[2]: *** Waiting for unfinished jobs
[10:48:38.918] make[1]: *** [Makefile:42: all-bin-recurse] Error 2
[10:48:38.918] make: *** [GNUmakefile:21: world-bin-src-recurse] Error 2
I guess this implies a dependency problem somewhere around
src/makefiles/Makefile.win32 but I'm not familiar with how that .rc
stuff is supposed to work and I figured I'd mention it here in case
it's obvious to someone else...
[1] https://cirrus-ci.com/task/5546921619095552
Re: Fast COPY FROM based on batch insert
On Tue, Mar 22, 2022 at 8:58 AM Andres Freund wrote: > > On 2021-06-07 16:16:58 +0500, Andrey Lepikhov wrote: > > Second version of the patch fixes problems detected by the FDW regression > > tests and shows differences of error reports in tuple-by-tuple and batched > > COPY approaches. > > Patch doesn't apply and likely hasn't for a while... Actually, it has bit-rotted due to the recent fix for cross-partition updates (i.e., commit ba9a7e392). Thanks! Best regards, Etsuro Fujita
Re: Make mesage at end-of-recovery less scary.
At Mon, 21 Mar 2022 17:01:19 -0700, Andres Freund wrote in
> Patch currently fails to apply, needs a rebase:
> http://cfbot.cputube.org/patch_37_2490.log
Thanks for noticing me of that.
Rebased to the current HEAD.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From a7c9f36e631eaba5078398598dae5d459e79add9 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Fri, 28 Feb 2020 15:52:58 +0900
Subject: [PATCH v17] Make End-Of-Recovery error less scary
When recovery in any type ends, we see a bit scary error message like
"invalid record length" that suggests something serious is
happening. Actually if recovery meets a record with length = 0, that
usually means it finished applying all available WAL records.
Make this message less scary as "reached end of WAL". Instead, raise
the error level for other kind of WAL failure to WARNING.
---
src/backend/access/transam/xlogreader.c | 145 +-
src/backend/access/transam/xlogrecovery.c | 92 ++
src/backend/replication/walreceiver.c | 7 +-
src/bin/pg_waldump/pg_waldump.c | 13 +-
src/include/access/xlogreader.h | 1 +
src/test/recovery/t/011_crash_recovery.pl | 106
6 files changed, 306 insertions(+), 58 deletions(-)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index e437c42992..0942265408 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -46,6 +46,8 @@ static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
int reqLen);
static void XLogReaderInvalReadState(XLogReaderState *state);
static XLogPageReadResult XLogDecodeNextRecord(XLogReaderState *state, bool non_blocking);
+static bool ValidXLogRecordLength(XLogReaderState *state, XLogRecPtr RecPtr,
+ XLogRecord *record);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
static bool ValidXLogRecord(XLogReaderState *state, XLogRecord *record,
@@ -147,6 +149,7 @@ XLogReaderAllocate(int wal_segment_size, const char *waldir,
pfree(state);
return NULL;
}
+ state->EndOfWAL = false;
state->errormsg_buf[0] = '\0';
/*
@@ -552,6 +555,7 @@ XLogDecodeNextRecord(XLogReaderState *state, bool nonblocking)
/* reset error state */
state->errormsg_buf[0] = '\0';
decoded = NULL;
+ state->EndOfWAL = false;
state->abortedRecPtr = InvalidXLogRecPtr;
state->missingContrecPtr = InvalidXLogRecPtr;
@@ -633,25 +637,21 @@ restart:
Assert(pageHeaderSize <= readOff);
/*
- * Read the record length.
+ * Validate the record header.
*
- * NB: Even though we use an XLogRecord pointer here, the whole record
- * header might not fit on this page. xl_tot_len is the first field of the
- * struct, so it must be on this page (the records are MAXALIGNed), but we
- * cannot access any other fields until we've verified that we got the
- * whole header.
- */
- record = (XLogRecord *) (state->readBuf + RecPtr % XLOG_BLCKSZ);
- total_len = record->xl_tot_len;
-
- /*
- * If the whole record header is on this page, validate it immediately.
- * Otherwise do just a basic sanity check on xl_tot_len, and validate the
- * rest of the header after reading it from the next page. The xl_tot_len
+ * Even though we use an XLogRecord pointer here, the whole record header
+ * might not fit on this page. If the whole record header is on this page,
+ * validate it immediately. Even otherwise xl_tot_len must be on this page
+ * (it is the first field of MAXALIGNed records), but we still cannot
+ * access any further fields until we've verified that we got the whole
+ * header, so do just a basic sanity check on record length, and validate
+ * the rest of the header after reading it from the next page. The length
* check is necessary here to ensure that we enter the "Need to reassemble
* record" code path below; otherwise we might fail to apply
* ValidXLogRecordHeader at all.
*/
+ record = (XLogRecord *) (state->readBuf + RecPtr % XLOG_BLCKSZ);
+
if (targetRecOff <= XLOG_BLCKSZ - SizeOfXLogRecord)
{
if (!ValidXLogRecordHeader(state, RecPtr, state->DecodeRecPtr, record,
@@ -661,18 +661,14 @@ restart:
}
else
{
- /* XXX: more validation should be done here */
- if (total_len < SizeOfXLogRecord)
- {
- report_invalid_record(state,
- "invalid record length at %X/%X: wanted %u, got %u",
- LSN_FORMAT_ARGS(RecPtr),
- (uint32) SizeOfXLogRecord, total_len);
+ if (!ValidXLogRecordLength(state, RecPtr, record))
goto err;
- }
+
gotheader = false;
}
+ total_len = record->xl_tot_len;
+
/*
* Find space to decode this record. Don't allow oversized allocation if
* the caller requested nonblocking. Otherwise, we *have* to try to
@@ -904,6 +900,15 @@ err:
*/
state->abortedRecPtr = RecPtr;
state->missingContrecPtr =
Re: [RFC] building postgres with meson -v8
Hi, Attached is v8. It's just a rebase to resolve conflicts with recent changes. - Andres v8-0001-meson-prereq-output-and-depencency-tracking-work.patch.gz Description: application/patch-gzip binfm5f_PMrcn.bin Description: application/patch-gzip binkuFB75lqSa.bin Description: application/patch-gzip binoDrSGmWV5b.bin Description: application/patch-gzip binWxMQiHSu4a.bin Description: application/patch-gzip binslUpvb_FiA.bin Description: application/patch-gzip binoppP0RAfXA.bin Description: application/patch-gzip binTg63q57FoM.bin Description: application/patch-gzip v8-0009-prereq-make-unicode-targets-work-in-vpath-builds.patch.gz Description: application/patch-gzip v8-0010-wip-split-TESTDIR-into-two.patch.gz Description: application/patch-gzip v8-0011-meson-Add-meson-based-buildsystem.patch.gz Description: application/patch-gzip v8-0012-meson-ci-Build-both-with-meson-and-as-before.patch.gz Description: application/patch-gzip
Re: Why is src/test/modules/committs/t/002_standby.pl flaky?
I have a new socket abstraction patch that should address the known Windows socket/event bugs, but it's a little bigger than I thought it would be, not quite ready, and now too late to expect people to review for 15, so I think it should go into the next cycle. I've bounced https://commitfest.postgresql.org/37/3523/ into the next CF. We'll need to do something like 75674c7e for master.
Re: Frontend error logging style
Hi, On 2022-03-21 22:00:37 -0400, Tom Lane wrote: > Andres Freund writes: > > This unfortunately has had some bitrot: > > http://cfbot.cputube.org/patch_37_3574.log > > Yeah, I know. That patch touches enough places that it's sure to > break every few days during a commitfest. I'm not real excited about > maintaining it reactively. Makes sense. I'd leave it on waiting-on-author then, so that reviewers looking through the CF don't need to look at it? > Maybe the plan could be to push it at the end of the commitfest? Would make sense to me. Arguably parts of it could be committed sooner than that, e.g. exit(-1). But that'd not make it meaningfully easier to maintain, so ... > But what I'd really like at the moment is buy-in or rejection of the whole > concept, so I know whether it's worth spending more time on. I'm +1. I've not carefully looked through every single changed callsite, but IMO it looks like a clear improvement. Greetings, Andres Freund
Re: Frontend error logging style
Andres Freund writes: > On 2022-02-25 12:15:25 -0500, Tom Lane wrote: >> The attached revision does that, standardizes on pg_fatal() as the >> abbreviation for pg_log_error() + exit(1), and invents detail/hint >> features as per previous discussion. > This unfortunately has had some bitrot: > http://cfbot.cputube.org/patch_37_3574.log Yeah, I know. That patch touches enough places that it's sure to break every few days during a commitfest. I'm not real excited about maintaining it reactively. The flip side of the coin is that pushing it will doubtless break a lot of other uncommitted patches. So I'm not sure how to proceed. Maybe the plan could be to push it at the end of the commitfest? But what I'd really like at the moment is buy-in or rejection of the whole concept, so I know whether it's worth spending more time on. regards, tom lane
RE: logical replication empty transactions
> On Monday, March 21, 2022 6:01 PM Amit Kapila
> wrote:
> >
> > On Sat, Mar 19, 2022 at 9:10 AM Ajin Cherian wrote:
> > >
> > > On Thu, Mar 17, 2022 at 10:43 PM Amit Kapila
> > >
> > wrote:
> > >
> > > > 3. Can we add a simple test for it in one of the existing test
> > > > files(say in 001_rep_changes.pl)?
> > >
> > > added a simple test.
> > >
> >
> > This doesn't verify if the transaction is skipped. I think we should
> > extend this test to check for a DEBUG message in the Logs (you need to
> > probably set log_min_messages to DEBUG1 for this test). As an example,
> > you can check the patch [1]. Also, it seems by mistake you have added
> > wait_for_catchup() twice.
>
> I added a testcase to check the DEBUG message.
>
> > Few other comments:
> > =
> > 1. Let's keep the parameter name as skipped_empty_xact in
> > OutputPluginUpdateProgress so as to not confuse with the other patch's
> > [2] keep_alive parameter. I think in this case we must send the
> > keep_alive message so as to not make the syncrep wait whereas in the
> > other patch we only need to send it periodically based on
> > wal_sender_timeout parameter.
> > 2. The new function SyncRepEnabled() seems confusing to me as the
> > comments in SyncRepWaitForLSN() clearly state why we need to first
> > read the parameter 'sync_standbys_defined' without any lock then read
> > it again with a lock if the parameter is true. So, I just put that
> > check back and also added a similar check in WalSndUpdateProgress.
> > 3.
> > @@ -1392,11 +1481,21 @@ pgoutput_truncate(LogicalDecodingContext *ctx,
> > ReorderBufferTXN *txn,
> > continue;
> >
> > relids[nrelids++] = relid;
> > +
> > + /* Send BEGIN if we haven't yet */
> > + if (txndata && !txndata->sent_begin_txn) pgoutput_send_begin(ctx,
> > + txn);
> > maybe_send_schema(ctx, change, relation, relentry);
> > }
> >
> > if (nrelids > 0)
> > {
> > + txndata = (PGOutputTxnData *) txn->output_plugin_private;
> > +
> > + /* Send BEGIN if we haven't yet */
> > + if (txndata && !txndata->sent_begin_txn) pgoutput_send_begin(ctx,
> > + txn);
> > +
> >
> > Why do we need to try sending the begin in the second check? I think
> > it should be sufficient to do it in the above loop.
> >
> > I have made these and a number of other changes in the attached patch.
> > Do let me know what you think of the attached?
>
> The changes look good to me.
> And I did some basic tests for the patch and didn’t find some other problems.
>
> Attach the new version patch.
Oh, sorry, I posted the wrong patch, here is the correct one.
Best regards,
Hou zj
v28-0001-Skip-empty-transactions-for-logical-replication.patch
Description: v28-0001-Skip-empty-transactions-for-logical-replication.patch
RE: Logical replication timeout problem
On Mon, Mar 21, 2022 at 1:31 PM Amit Kapila wrote:
>
Thanks for your comments.
> On Fri, Mar 18, 2022 at 4:20 PM Amit Kapila wrote:
> >
> > On Fri, Mar 18, 2022 at 10:43 AM wangw.f...@fujitsu.com
> > wrote:
> > >
> > > On Thu, Mar 17, 2022 at 7:52 PM Masahiko Sawada
> wrote:
> > > >
> > >
> > > Attach the new patch.
> > >
> >
> > *
> > case REORDER_BUFFER_CHANGE_INVALIDATION:
> > - /* Execute the invalidation messages locally */
> > - ReorderBufferExecuteInvalidations(
> > - change->data.inval.ninvalidations,
> > - change->data.inval.invalidations);
> > - break;
> > + {
> > + LogicalDecodingContext *ctx = rb->private_data;
> > +
> > + /* Try to send a keepalive message. */
> > + UpdateProgress(ctx, true);
> >
> > Calling UpdateProgress() here appears adhoc to me especially because
> > it calls OutputPluginUpdateProgress which appears to be called only
> > from plugin API. Am, I missing something? Also why the same handling
> > is missed in other similar messages like
> > REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID where we don't call
> any
> > plug-in API?
Yes, you are right.
And I invoke in case REORDER_BUFFER_CHANGE_INVALIDATION because I think every
DDL will modify the catalog then get into this case. So I only invoke function
UpdateProgress here to handle DDL.
> > I am not sure what is a good way to achieve this but one idea that
> > occurred to me was shall we invent a new callback
> > ReorderBufferSkipChangeCB similar to ReorderBufferApplyChangeCB and
> > then pgoutput can register its API where we can have the logic similar
> > to what you have in UpdateProgress()? If we do so, then all the
> > cuurent callers of UpdateProgress in pgoutput can also call that API.
> > What do you think?
> >
> Another idea could be that we leave the DDL case for now as anyway
> there is very less chance of timeout for skipping DDLs and we may
> later need to even backpatch this bug-fix which would be another
> reason to not make such invasive changes. We can handle the DDL case
> if required separately.
Yes, I think a new callback function would be nice.
Yes, as you said, maybe we could fix the usecase that found the problem in the
first place. Then make further modifications on the master branch.
Modify the patch. Currently only DML related code remains.
> > * Why don't you have a quick exit like below code in WalSndWriteData?
> > /* Try taking fast path unless we get too close to walsender timeout. */ if
> > (now
> > < TimestampTzPlusMilliseconds(last_reply_timestamp,
> > wal_sender_timeout / 2) &&
> > !pq_is_send_pending())
> > {
> > return;
> > }
Fixed. I missed this so adding it in the new patch.
> > * Can we rename variable 'is_send' to 'change_sent'?
Improve the the name of this variable.(From 'is_send' to 'change_sent')
Attach the new patch. [suggestion by Amit-San.]
1. Remove DDL related code. Handle the DDL case later separately if need.
2. Fix a missing.(In function WalSndUpdateProgress)
3. Improve variable names. (From 'is_send' to 'change_sent')
4. Fix some comments.(Above and inside the function WalSndUpdateProgress.)
Regards,
Wang wei
v4-0001-Fix-the-timeout-of-subscriber-in-long-transaction.patch
Description: v4-0001-Fix-the-timeout-of-subscriber-in-long-transaction.patch
Re: Per-table storage parameters for TableAM/IndexAM extensions
Hi, On 2022-02-24 12:26:08 +0530, Sadhuprasad Patro wrote: > I have added a dummy test module for table AM and did the document > change in the latest patch attached... The test module doesn't build on windows, unfortunately... Looks like you need to add PGDLLIMPORT to a few variables: [01:26:18.539] c:\cirrus\src\test\modules\dummy_table_am\dummy_table_am.c(488): warning C4700: uninitialized local variable 'rel' used [c:\cirrus\dummy_table_am.vcxproj] [01:26:18.539] dummy_table_am.obj : error LNK2001: unresolved external symbol synchronize_seqscans [c:\cirrus\dummy_table_am.vcxproj] [01:26:18.539] .\Debug\dummy_table_am\dummy_table_am.dll : fatal error LNK1120: 1 unresolved externals [c:\cirrus\dummy_table_am.vcxproj] [01:26:18.539] 1 Warning(s) [01:26:18.539] 2 Error(s) https://cirrus-ci.com/task/5067519584108544?logs=build#L2085 Marked the CF entry as waiting-on-author. Greetings, Andres
Re: Fast COPY FROM based on batch insert
On Fri, Jun 4, 2021 at 5:26 PM Andrey Lepikhov
wrote:
> We still have slow 'COPY FROM' operation for foreign tables in current
> master.
> Now we have a foreign batch insert operation And I tried to rewrite the
> patch [1] with this machinery.
I’d been reviewing the previous version of the patch without noticing
this. (Gmail grouped it in a new thread due to the subject change,
but I overlooked the whole thread.)
I agree with you that the first step for fast copy into foreign
tables/partitions is to use the foreign-batch-insert API. (Actually,
I was also thinking the same while reviewing the previous version.)
Thanks for the new version of the patch!
The patch has been rewritten to something essentially different, but
no one reviewed it. (Tsunakawa-san gave some comments without looking
at it, though.) So the right status of the patch is “Needs review”,
rather than “Ready for Committer”? Anyway, here are a few review
comments from me:
* I don’t think this assumption is correct:
@@ -359,6 +386,12 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,
(resultRelInfo->ri_TrigDesc->trig_insert_after_row ||
resultRelInfo->ri_TrigDesc->trig_insert_new_table))
{
+ /*
+* AFTER ROW triggers aren't allowed with the foreign bulk insert
+* method.
+*/
+ Assert(resultRelInfo->ri_RelationDesc->rd_rel->relkind !=
RELKIND_FOREIGN_TABLE);
+
In postgres_fdw we disable foreign batch insert when the target table
has AFTER ROW triggers, but the core allows it even in that case. No?
* To allow foreign multi insert, the patch made an invasive change to
the existing logic to determine whether to use multi insert for the
target relation, adding a new member ri_usesMultiInsert to the
ResultRelInfo struct, as well as introducing a new function
ExecMultiInsertAllowed(). But I’m not sure we really need such a
change. Isn’t it reasonable to *adjust* the existing logic to allow
foreign multi insert when possible?
I didn’t finish my review, but I’ll mark this as “Waiting on Author”.
My apologies for the long long delay.
Best regards,
Etsuro Fujita
Re: Frontend error logging style
Hi, On 2022-02-25 12:15:25 -0500, Tom Lane wrote: > The attached revision does that, standardizes on pg_fatal() as the > abbreviation for pg_log_error() + exit(1), and invents detail/hint > features as per previous discussion. This unfortunately has had some bitrot: http://cfbot.cputube.org/patch_37_3574.log Marked as waiting-on-author. Greetings, Andres
Re: Commitfest manager for 2022-03
Hi, On 2022-02-26 16:12:27 -0500, Greg Stark wrote: > I do have the time available. What I don't necessarily have is insight > into everything that needs to be done, especially behind the scenes. One thing that desperately needs doing, particularly during the last commitfest, is looking through CF entries and pruning stuff that shouldn't be there anymore or that are in the wrong state. I just went through the list of patches that are failing on http://cfbot.cputube.org/index.html There were several CF entries that haven't made progress in months marked as "needs review", despite the last things on the thread being asks of the author(s). One can't just automatically mark all failing runs as "waiting on author" because sometimes there are issues with somebody else posting an incremental diff confusing cfbot or spurious test failures... If a patch fails to apply and it looks to be a "real patch" clearly some action has to be taken by the author, so marking entries as "waiting on author" is good. Greetings, Andres Freund
Re: speed up a logical replica setup
On Fri, 18 Mar 2022 at 19:34 Andrew Dunstan wrote: > > On 3/15/22 09:51, Peter Eisentraut wrote: > > On 21.02.22 13:09, Euler Taveira wrote: > >> A new tool called pg_subscriber does this conversion and is tightly > >> integrated > >> with Postgres. > > > > Are we comfortable with the name pg_subscriber? It seems too general. > > Are we planning other subscriber-related operations in the future? If > > so, we should at least make this one use a --create option or > > something like that. > > > Not really sold on the name (and I didn't much like the name > pglogical_create_subscriber either, although it's a cool facility and > I'm happy to see us adopting something like it). > > ISTM we should have a name that conveys that we are *converting* a > replica or equivalent to a subscriber. > > Some time ago I did a POC on it [1] and I used the name pg_create_subscriber [1] https://github.com/fabriziomello/pg_create_subscriber -- Fabrízio de Royes Mello
Re: Condition pushdown: why (=) is pushed down into join, but BETWEEN or >= is not?
Hi, On 2022-03-08 21:44:37 +0800, Andy Fan wrote: > I have finished the PoC for planning timing improvement and joinrel rows > estimation. This currently crashes on cfbot: https://api.cirrus-ci.com/v1/task/6158455839916032/logs/cores.log https://cirrus-ci.com/task/6158455839916032 As this is clearly not 15 material, I've set the target version as 16. But it might be good to just move the whole entry to the next CF... Greetings, Andres Freund
Re: MDAM techniques and Index Skip Scan patch
Hi, On 2022-01-22 22:37:19 +0100, Dmitry Dolgov wrote: > > On Fri, Jan 14, 2022 at 04:03:41PM +0800, Julien Rouhaud wrote: > > Hi, > > > > On Fri, Jan 14, 2022 at 08:55:26AM +0100, Dmitry Dolgov wrote: > > > > > > FYI, I've attached this thread to the CF item as an informational one, > > > but as there are some patches posted here, folks may get confused. For > > > those who have landed here with no context, I feel obliged to mention > > > that now there are two alternative patch series posted under the same > > > CF item: > > > > > > * the original one lives in [1], waiting for reviews since the last May > > > * an alternative one posted here from Floris > > > > Ah, I indeed wasn't sure of which patchset(s) should actually be reviewed. > > It's nice to have the alternative approach threads linkied in the commit > > fest, > > but it seems that the cfbot will use the most recent attachments as the only > > patchset, thus leaving the "original" one untested. > > > > I'm not sure of what's the best approach in such situation. Maybe creating > > a > > different CF entry for each alternative, and link the other cf entry on the > > cf > > app using the "Add annotations" or "Links" feature rather than attaching > > threads? > > I don't mind having all of the alternatives under the same CF item, only > one being tested seems to be only a small limitation of cfbot. IMO it's pretty clear that having "duelling" patches below one CF entry is a bad idea. I think they should be split, with inactive approaches marked as returned with feeback or whatnot. Either way, currently this patch fails on cfbot due to a new GUC: https://api.cirrus-ci.com/v1/artifact/task/5134905372835840/log/src/test/recovery/tmp_check/regression.diffs https://cirrus-ci.com/task/5134905372835840 Greetings, Andres Freund
Re: pg14 psql broke \d datname.nspname.relname
> On Mar 21, 2022, at 6:12 PM, Andres Freund wrote: > > Needs another one: http://cfbot.cputube.org/patch_37_3367.log > > Marked as waiting-on-author. v6-0001-Reject-patterns-with-too-many-parts-or-wrong-db.patch Description: Binary data — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
Hi, On 2022-03-09 10:50:45 -0600, Justin Pryzby wrote: > Rebased over 9e9858389 (Michael may want to look at the tuplestore part?). Doesn't apply cleanly anymore: http://cfbot.cputube.org/patch_37_2377.log Marked as waiting-on-author. Greetings, Andres Freund
Re: Temporary tables versus wraparound... again
No problem, I can update the patch and check on the fuzz. But the actual conflict is just in the test and I'm not sure it's really worth having a test at all. It's testing a pretty low level detail. So I'm leaning toward fixing the conflict by just ripping the test out. Nathan also pointed out there was a simpler way to get the pid. I don't think the way I was doing it was wrong but I'll double check that.
Re: speed up a logical replica setup
Hi, On 2022-02-21 09:09:12 -0300, Euler Taveira wrote: > A new tool called pg_subscriber does this conversion and is tightly integrated > with Postgres. Given that this has been submitted just before the last CF and is a patch of nontrivial size, has't made significant progress ISTM it should be moved to the next CF? It currently fails in cfbot, but that's likely just due to Peter's incremental patch. Perhaps you could make sure the patch still applies and repost? Greetings, Andres Freund
Re: [PATCH] sort leaf pages by ctid for gist indexes built using sorted method
Hi, On 2021-12-16 14:49:25 +0500, Andrey Borodin wrote: > > > With the current implementation, for GiST indexes created by doing multiple > > inserts, index tuples match heap tuples order, but it doesn't work that way > > for sorted method where index tuples on all levels are ordered using > > comparator provided in sortsupport (z-order for geometry type, for > > example). This means two tuples that are on the same heap page can be far > > apart from one another on an index page, and the heap page may be read > > twice and prefetch performance will degrade. > > > > I've created a patch intended to improve that by sorting index tuples by > > heap tuples TID order on leaf pages. > > Thanks you for the patch. The code looks nice and clean. The patch fails currently doesn't apply: http://cfbot.cputube.org/patch_37_3454.log > But can we have some benchmarks showing that this optimization really helps? As there hasn't been a response to this even in the last CF, I'm going to mark this entry as returned with feedback (IMO shouldn't even have been moved to this CF). Greetings, Andres Freund
Re: a misbehavior of partition row movement (?)
Hi Alvaro, On Mon, Mar 21, 2022 at 2:58 AM Alvaro Herrera wrote: > On 2022-Mar-20, Amit Langote wrote: > > On Sun, Mar 20, 2022 at 5:13 AM Alvaro Herrera > > wrote: > > > On 2022-Mar-18, Zhihong Yu wrote: > > > > > + if (!partRel->rd_rel->relispartition) > > > > + elog(ERROR, "cannot find ancestors of a non-partition result > > > > relation"); > > > > > > > > It would be better to include the relation name in the error message. > > > > > > I don't think it matters. We don't really expect to hit this. > > > > I tend to think maybe showing at least the OID in the error message > > doesn't hurt, but maybe we don't need to. > > Since we don't even know of a situation in which this error message > would be raised, I'm hardly bothered by failing to print the OID. If > any users complain, we can add more detail. Sure. > I lament the fact that this fix is not going to hit Postgres 12-14, but > ratio of effort to reward seems a bit too high. I think we could > backpatch the two involved commits if someone is motivated enough to > verify everything and come up with solutions for the necessary ABI > changes. > > Thank you, Amit, for your perseverance in getting this bug fixed! Thanks a lot for taking the time to review and commit. -- Amit Langote EDB: http://www.enterprisedb.com
Re: postgres_fdw: using TABLESAMPLE to collect remote sample
Hi, On 2022-02-23 00:51:24 +0100, Tomas Vondra wrote: > And here's the slightly simplified patch, without the part adding the > unnecessary GetServerVersion() function. Doesn't apply anymore: http://cfbot.cputube.org/patch_37_3535.log Marked as waiting-on-author. Greetings, Andres Freund
Re: Allow batched insert during cross-partition updates
On Tue, Mar 22, 2022 at 9:30 AM Andres Freund wrote: > On 2021-08-24 12:03:59 +0900, Amit Langote wrote: > > Tomas committed the bug-fix, so attaching a rebased version of the > > patch for $subject. > > This patch is in the current CF, but doesn't apply: > http://cfbot.cputube.org/patch_37_2992.log > marked the entry as waiting-on-author. Thanks for the heads up. Rebased to fix a minor conflict with some recently committed nodeModifyTable.c changes. -- Amit Langote EDB: http://www.enterprisedb.com v10-0001-Allow-batching-of-inserts-during-cross-partition.patch Description: Binary data
Re: [PATCH] pg_stat_toast v8
Hi, On 2022-03-08 19:32:03 +0100, Gunnar "Nick" Bluth wrote: > v8 (applies cleanly to today's HEAD/master) attached. This doesn't apply anymore, likely due to my recent pgstat changes - which you'd need to adapt to... http://cfbot.cputube.org/patch_37_3457.log Marked as waiting on author. Greetings, Andres Freund
Re: Separate the result of \watch for each query execution (psql)
Hi, On 2022-02-25 13:23:31 +0900, Noboru Saito wrote: > I have created a patch that allows you to turn it on and off in \pset. The patch unfortunately causes tests to fail: https://cirrus-ci.com/task/5932406812180480 diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/psql.out /tmp/cirrus-ci-build/src/test/regress/results/psql.out --- /tmp/cirrus-ci-build/src/test/regress/expected/psql.out 2022-03-21 09:55:55.875784000 + +++ /tmp/cirrus-ci-build/src/test/regress/results/psql.out 2022-03-21 09:59:51.950512000 + @@ -304,6 +304,7 @@ fieldsep_zerooff footer on format aligned +formfeed off linestyleascii null '' numericlocaleoff Greetings, Andres Freund
Re: pg14 psql broke \d datname.nspname.relname
On 2022-01-26 09:04:15 -0800, Mark Dilger wrote: > Also, rebased as necessary: Needs another one: http://cfbot.cputube.org/patch_37_3367.log Marked as waiting-on-author. Greetings, Andres Freund
Re: LogwrtResult contended spinlock
Hi, On 2021-11-22 18:56:43 -0300, Alvaro Herrera wrote: > There was an earlier comment by Andres that asyncXactLSN should also be > atomic, to avoid an ugly spinlock interaction with the new atomic-based > logwrtResult. The 0002 here is an attempt at doing that; I found that > it also needed to change WalWriterSleeping to use atomics, to avoid > XLogSetAsyncXactLSN having to grab the spinlock for that. This patch currently doesn't apply: http://cfbot.cputube.org/patch_37_2716.log Are you aiming this for v15? Otherwise I'd like to move the entry to the next CF. Marked as waiting-on-author. Greetings, Andres Freund
Re: logical replication restrictions
On Mon, Mar 21, 2022, at 10:04 PM, Andres Freund wrote: > On 2022-03-20 21:40:40 -0300, Euler Taveira wrote: > > On Mon, Feb 28, 2022, at 9:18 PM, Euler Taveira wrote: > > > Long time, no patch. Here it is. I will provide documentation in the next > > > version. I would appreciate some feedback. > > This patch is broken since commit 705e20f8550c0e8e47c0b6b20b5f5ffd6ffd9e33. > > I > > rebased it. > > This fails tests, specifically it seems psql crashes: > https://cirrus-ci.com/task/6592281292570624?logs=cores#L46 Yeah. I forgot to test this patch with cassert before sending it. :( I didn't send a new patch because there is another issue (with int128) that I'm currently reworking. I'll send another patch soon. -- Euler Taveira EDB https://www.enterprisedb.com/
Re: WaitLatchOrSocket seems to not count to 4 right...
Hi, On 2022-02-11 10:47:45 +1300, Thomas Munro wrote: > I'd originally sketched this out for another project, but I don't > think I need it for that anymore. After this exchange I couldn't > resist fleshing it out for a commitfest, just on useability grounds. > Thoughts? This currently doesn't apply: http://cfbot.cputube.org/patch_37_3533.log Marked as waiting-on-author for now. Are you aiming this for 15? Greetings, Andres Freund
Re: Window Function "Run Conditions"
Hi, On 2021-08-19 18:35:27 +1200, David Rowley wrote: > Anyway, I'll take a few more days to think about it before posting a fix. The patch in the CF entry doesn't apply: http://cfbot.cputube.org/patch_37_3234.log The quoted message was ~6 months ago. I think this CF entry should be marked as returned-with-feedback? - Andres
Re: Temporary tables versus wraparound... again
Hi, On 2021-10-12 18:04:35 -0400, Greg Stark wrote: > Here's an updated patch. Unfortunately it doesn't apply anymore these days: http://cfbot.cputube.org/patch_37_3358.log Marked as waiting-on-author. Greetings, Andres Freund
Re: logical replication restrictions
On 2022-03-20 21:40:40 -0300, Euler Taveira wrote: > On Mon, Feb 28, 2022, at 9:18 PM, Euler Taveira wrote: > > Long time, no patch. Here it is. I will provide documentation in the next > > version. I would appreciate some feedback. > This patch is broken since commit 705e20f8550c0e8e47c0b6b20b5f5ffd6ffd9e33. I > rebased it. This fails tests, specifically it seems psql crashes: https://cirrus-ci.com/task/6592281292570624?logs=cores#L46 Marked as waiting-on-author. Greetings, Andres Freund
Re: WIN32 pg_import_system_collations
Hi, On 2022-01-25 15:49:01 +0100, Juan José Santamaría Flecha wrote: > So, I'm doing that in the attached version, just changing the object name. Currently fails to apply, please rebase: http://cfbot.cputube.org/patch_37_3450.log Marked as waiting-on-author. - Andres
Re: Removing unneeded self joins
Hi, On 2022-03-04 15:47:47 +0500, Andrey Lepikhov wrote: > Also, in new version of the patch I fixed one stupid bug: checking a > self-join candidate expression operator - we can remove only expressions > like F(arg1) = G(arg2). This CF entry currently fails tests: https://cirrus-ci.com/task/4632127944785920?logs=test_world#L1938 Looks like you're missing an adjustment of postgresql.conf.sample Marked as waiting-on-author. Greetings, Andres Freund
Re: Reducing power consumption on idle servers
On 2022-02-21 21:04:19 +, Simon Riggs wrote: > On Mon, 21 Feb 2022 at 16:49, Chapman Flack wrote: > > > Shouldn't the comment be "with work_done=false" ? > > Good catch, thanks. > > I've also added docs to say that "promote_trigger_file" is now > deprecated. There were no tests for that functionality, so just as > well it is being removed. Doesn't pass tests, and hasn't for weeks from what I can see: https://cirrus-ci.com/task/5925633648754688?logs=test_world#L1153 Marked as waiting-on-author. - Andres
Re: [PATCH] Proof of concept for GUC improvements
Andres Freund writes: > My impression is that there's not a lot of enthusiasm for the concept? If > that's true we maybe ought to mark the CF entry as rejected? Yeah, I'm kind of leaning that way too. I don't see how we can incorporate the symbolic values into any existing display paths without breaking applications that expect the old output. That being the case, it seems like we'd have "two ways to do it" indefinitely, which would add enough confusion that I'm not sure there's a net gain. In particular, I foresee novice questions along the lines of "I set foo to disabled, why is it showing as zero?". If we'd done it like this from the beginning, it'd have been great, but retrofitting it now is a lot less appealing. regards, tom lane
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On 2022-01-24 14:44:10 -0500, Robert Haas wrote: > On Sat, Jan 22, 2022 at 2:20 AM Shruthi Gowda wrote: > > Agree. In the latest patch, the template0 and postgres OIDs are fixed > > to unused manually assigned OIDs 4 and 5 respectively. These OIDs are > > no more listed as unused OIDs. > > Thanks. Committed with a few more cosmetic changes. I noticed this still has an open CF entry: https://commitfest.postgresql.org/37/3296/ I assume it can be marked as committed? - Andres
Re: Pluggable toaster
Hi, On 2022-03-22 02:31:21 +0300, Nikita Malakhov wrote: > Hi Hackers, > Because of 3 months have passed since Pluggable Toaster presentation and a > lot of > commits were pushed into v15 master - we would like to re-introduce > this patch > rebased onto actual master. Last commit being used - > commit 641f3dffcdf1c7378cfb94c98b6642793181d6db (origin/master) > Author: Tom Lane > Date: Fri Mar 11 13:47:26 2022 -0500 It currently fails to apply: http://cfbot.cputube.org/patch_37_3490.log Given the size of the patch, and the degree of review it has gotten so far, it seems not realistically a fit for 15, but is marked as such. Think it should be moved to the next CF. Marked as waiting-on-author for now. Greetings, Andres Freund
Re: Partial aggregates pushdown
On 2022-01-17 15:27:53 +0300, Alexander Pyhalov wrote: > Alexander Pyhalov писал 2022-01-17 15:26: > > Updated patch. > > Sorry, missed attachment. Needs another update: http://cfbot.cputube.org/patch_37_3369.log Marked as waiting on author. - Andres
RE: logical replication empty transactions
On Monday, March 21, 2022 6:01 PM Amit Kapila wrote:
>
> On Sat, Mar 19, 2022 at 9:10 AM Ajin Cherian wrote:
> >
> > On Thu, Mar 17, 2022 at 10:43 PM Amit Kapila
> wrote:
> >
> > > 3. Can we add a simple test for it in one of the existing test
> > > files(say in 001_rep_changes.pl)?
> >
> > added a simple test.
> >
>
> This doesn't verify if the transaction is skipped. I think we should
> extend this test to check for a DEBUG message in the Logs (you need to
> probably set log_min_messages to DEBUG1 for this test). As an example,
> you can check the patch [1]. Also, it seems by mistake you have added
> wait_for_catchup() twice.
I added a testcase to check the DEBUG message.
> Few other comments:
> =
> 1. Let's keep the parameter name as skipped_empty_xact in
> OutputPluginUpdateProgress so as to not confuse with the other patch's
> [2] keep_alive parameter. I think in this case we must send the
> keep_alive message so as to not make the syncrep wait whereas in the
> other patch we only need to send it periodically based on
> wal_sender_timeout parameter.
> 2. The new function SyncRepEnabled() seems confusing to me as the
> comments in SyncRepWaitForLSN() clearly state why we need to first
> read the parameter 'sync_standbys_defined' without any lock then read
> it again with a lock if the parameter is true. So, I just put that
> check back and also added a similar check in WalSndUpdateProgress.
> 3.
> @@ -1392,11 +1481,21 @@ pgoutput_truncate(LogicalDecodingContext *ctx,
> ReorderBufferTXN *txn,
> continue;
>
> relids[nrelids++] = relid;
> +
> + /* Send BEGIN if we haven't yet */
> + if (txndata && !txndata->sent_begin_txn)
> + pgoutput_send_begin(ctx, txn);
> maybe_send_schema(ctx, change, relation, relentry);
> }
>
> if (nrelids > 0)
> {
> + txndata = (PGOutputTxnData *) txn->output_plugin_private;
> +
> + /* Send BEGIN if we haven't yet */
> + if (txndata && !txndata->sent_begin_txn)
> + pgoutput_send_begin(ctx, txn);
> +
>
> Why do we need to try sending the begin in the second check? I think
> it should be sufficient to do it in the above loop.
>
> I have made these and a number of other changes in the attached patch.
> Do let me know what you think of the attached?
The changes look good to me.
And I did some basic tests for the patch and didn’t find some other problems.
Attach the new version patch.
Best regards,
Hou zj
v28-0001-Skip-empty-transactions-for-logical-replication.patch
Description: v28-0001-Skip-empty-transactions-for-logical-replication.patch
Re: Parallelize correlated subqueries that execute within each worker
Hi, On 2022-01-22 20:25:19 -0500, James Coleman wrote: > On the other hand this is a dramatically simpler patch series. > Assuming the approach is sound, it should much easier to maintain than > the previous version. > > The final patch in the series is a set of additional checks I could > imagine to try to be more explicit, but at least in the current test > suite there isn't anything at all they affect. > > Does this look at least somewhat more like what you'd envisionsed > (granting the need to squint hard given the relids checks instead of > directly checking params)? This fails on freebsd (so likely a timing issue): https://cirrus-ci.com/task/4758411492458496?logs=test_world#L2225 Marked as waiting on author. Greetings, Andres Freund
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
> On Sun, 20 Mar 2022 16:11:43 +0900 (JST) > Tatsuo Ishii wrote: > >> > Hi Yugo, >> > >> > I tested with serialization error scenario by setting: >> > default_transaction_isolation = 'repeatable read' >> > The result was: >> > >> > $ pgbench -t 10 -c 10 --max-tries=10 test >> > transaction type: >> > scaling factor: 10 >> > query mode: simple >> > number of clients: 10 >> > number of threads: 1 >> > maximum number of tries: 10 >> > number of transactions per client: 10 >> > number of transactions actually processed: 100/100 >> > number of failed transactions: 0 (0.000%) >> > number of transactions retried: 35 (35.000%) >> > total number of retries: 74 >> > latency average = 5.306 ms >> > initial connection time = 15.575 ms >> > tps = 1884.516810 (without initial connection time) >> > >> > I had hard time to understand what those numbers mean: >> > number of transactions retried: 35 (35.000%) >> > total number of retries: 74 >> > >> > It seems "total number of retries" matches with the number of ERRORs >> > reported in PostgreSQL. Good. What I am not sure is "number of >> > transactions retried". What does this mean? >> >> Oh, ok. I see it now. It turned out that "number of transactions >> retried" does not actually means the number of transactions >> rtried. Suppose pgbench exectutes following in a session: >> >> BEGIN; -- transaction A starts >> : >> (ERROR) >> ROLLBACK; -- transaction A aborts >> >> (retry) >> >> BEGIN; -- transaction B starts >> : >> (ERROR) >> ROLLBACK; -- transaction B aborts >> >> (retry) >> >> BEGIN; -- transaction C starts >> : >> END; -- finally succeeds >> >> In this case "total number of retries:" = 2 and "number of >> transactions retried:" = 1. In this patch transactions A, B and C are >> regarded as "same" transaction, so the retried transaction count >> becomes 1. But it's confusing to use the language "transaction" here >> because A, B and C are different transactions. I would think it's >> better to use different language instead of "transaction", something >> like "cycle"? i.e. >> >> number of cycles retried: 35 (35.000%) I realized that the same argument can be applied even to "number of transactions actually processed" because with the retry feature, "transaction" could comprise multiple transactions. But if we go forward and replace those "transactions" with "cycles" (or whatever) altogether, probably it could bring enough confusion to users who have been using pgbench. Probably we should give up the language changing and redefine "transaction" when the retry feature is enabled instead like "when retry feature is enabled, each transaction can be consisted of multiple transactions retried." > In the original patch by Marina Polyakova it was "number of retried", > but I changed it to "number of transactions retried" is because I felt > it was confusing with "number of retries". I chose the word "transaction" > because a transaction ends in any one of successful commit , skipped, or > failure, after possible retries. Ok. > Well, I agree with that it is somewhat confusing wording. If we can find > nice word to resolve the confusion, I don't mind if we change the word. > Maybe, we can use "executions" as well as "cycles". However, I am not sure > that the situation is improved by using such word because what such word > exactly means seems to be still unclear for users. > > Another idea is instead reporting only "the number of successfully > retried transactions" that does not include "failed transactions", > that is, transactions failed after retries, like this; > > number of transactions actually processed: 100/100 > number of failed transactions: 0 (0.000%) > number of successfully retried transactions: 35 (35.000%) > total number of retries: 74 > > The meaning is clear and there seems to be no confusion. Thank you for the suggestion. But I think it would better to leave it as it is because of the reason I mentioned above. Best reagards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [PATCH] New [relation] option engine
Hi, On 2022-02-14 00:43:36 +0300, Nikolay Shaplov wrote: > I'd like to introduce a patch that reworks options processing. This doesn't apply anymore: http://cfbot.cputube.org/patch_37_3536.log Given that this patch has been submitted just to the last CF and that there's been no action on it, I don't see this going into 15. Therefore I'd like to move it to the next CF? Greetings, Andres Freund
Re: Map WAL segment files on PMEM as WAL buffers
Hi, On 2022-01-20 14:55:13 +0900, Takashi Menjo wrote: > Here is patchset v8. It will have "make check-world" and Cirrus to > pass. This unfortunately does not apply anymore: http://cfbot.cputube.org/patch_37_3181.log Could you rebase? - Andres
Re: Logical insert/update/delete WAL records for custom table AMs
Hi, On 2022-01-17 01:05:14 -0800, Jeff Davis wrote: > Great, I attached a rebased version. Currently this doesn't apply: http://cfbot.cputube.org/patch_37_3394.log - Andres
Re: Proposal: allow database-specific role memberships
Hi, On 2022-01-22 22:56:44 +0800, Julien Rouhaud wrote: > On Sat, Jan 22, 2022 at 05:28:05AM -0800, Kenaniah Cerny wrote: > > Thank you so much for the backtrace! > > > > This latest patch should address by moving the dumpRoleMembership call to > > before the pointer is closed. > > Thanks! The cfbot turned green since: > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/36/3374 red again: https://cirrus-ci.com/task/5516269981007872?logs=test_world#L1480 Marked as waiting-on-author. - Andres
Re: [PATCH] Proof of concept for GUC improvements
Hi, On 2022-01-12 12:57:02 -0600, David Christensen wrote: > > Hi, > > > > According to the cfbot, the patch doesn't apply anymore and needs a > > rebase: http://cfbot.cputube.org/patch_36_3290.log > > V4 rebased attached. Doesn't apply anymore, again: http://cfbot.cputube.org/patch_37_3290.log My impression is that there's not a lot of enthusiasm for the concept? If that's true we maybe ought to mark the CF entry as rejected? Greetings, Andres Freund
Re: [WIP] Allow pg_upgrade to copy segments of the same relfilenode in parallel
Hi,
On 2022-02-01 21:57:00 -0500, Jaime Casanova wrote:
> This patch adds a new option (-J num, --jobs-per-disk=num) in
> pg_upgrade to speed up copy mode. This generates upto ${num}
> processes per tablespace to copy segments of the same relfilenode
> in parallel.
>
> This can help when you have many multi gigabyte tables (each segment
> is 1GB by default) in different tablespaces (each tablespace in a
> different disk) and multiple processors.
>
> In a customer's database (~20Tb) it went down from 6h to 4h 45min.
>
> It lacks documentation and I need help with WIN32 part of it, I created
> this new mail to put the patch on the next commitfest.
The patch currently fails on cfbot due to warnings, likely related due to the
win32 issue:
https://cirrus-ci.com/task/4566046517493760?logs=mingw_cross_warning#L388
As it's a new patch submitted to the last CF, hasn't gotten any review yet and
misses some platform support, it seems like there's no chance it can make it
into 15?
Greetings,
Andres Freund
Re: PATCH: generate fractional cheapest paths in generate_orderedappend_path
On 3/22/22 01:18, Andres Freund wrote: > Hi, > > On 2022-01-14 01:39:30 +0100, Tomas Vondra wrote: >> Are you interested / willing to do some of this work? > > This patch hasn't moved in the last two months. I think it may be time to > mark it as returned with feedback for now? > > It's also failing tests, and has done so for months: > > https://cirrus-ci.com/task/5308087077699584 > https://api.cirrus-ci.com/v1/artifact/task/5308087077699584/log/src/test/regress/regression.diffs > > Greetings, > Yeah. I think it's a useful improvement, but it needs much more work than is doable by the end of this CF. RwF seems about right. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Supply restore_command to pg_rewind via CLI argument
Hi, On 2021-11-05 15:10:29 +0500, Andrey Borodin wrote: > > 4 нояб. 2021 г., в 17:55, Daniel Gustafsson написал(а): > > > > The patch no longer applies, can you submit a rebased version please? > > Thanks, Daniel! PFA rebase. Doesn't apply once more: http://cfbot.cputube.org/patch_37_3213.log Greetings, Andres Freund
Re: Fix overflow in DecodeInterval
Joseph Koshakow writes:
> [ v8-0001-Check-for-overflow-when-decoding-an-interval.patch ]
This isn't applying per the cfbot; looks like it got sideswiped
by 9e9858389. Here's a quick rebase. I've not reviewed it, but
I did notice (because git was in my face about this) that it's
got whitespace issues. Please try to avoid unnecessary whitespace
changes ... pgindent will clean those up, but it makes reviewing
harder.
regards, tom lane
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index ba0ec35ac5..014ec88e0d 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -21,6 +21,7 @@
#include "access/htup_details.h"
#include "access/xact.h"
#include "catalog/pg_type.h"
+#include "common/int.h"
#include "common/string.h"
#include "funcapi.h"
#include "miscadmin.h"
@@ -38,16 +39,20 @@ static int DecodeNumberField(int len, char *str,
int fmask, int *tmask,
struct pg_tm *tm, fsec_t *fsec, bool *is2digits);
static int DecodeTime(char *str, int fmask, int range,
- int *tmask, struct pg_tm *tm, fsec_t *fsec);
+ int *tmask, struct pg_tm *tm, int64 *hour, fsec_t *fsec);
+static int DecodeTimeForInterval(char *str, int fmask, int range,
+ int *tmask, struct pg_itm_in *itm_in);
static const datetkn *datebsearch(const char *key, const datetkn *base, int nel);
static int DecodeDate(char *str, int fmask, int *tmask, bool *is2digits,
struct pg_tm *tm);
-static char *AppendSeconds(char *cp, int sec, fsec_t fsec,
+static char *AppendSeconds(char *cp, int sec, int64 fsec,
int precision, bool fillzeros);
-static void AdjustFractSeconds(double frac, struct pg_tm *tm, fsec_t *fsec,
- int scale);
-static void AdjustFractDays(double frac, struct pg_tm *tm, fsec_t *fsec,
- int scale);
+static bool AdjustFractMicroseconds(long double frac, struct pg_itm_in *itm_in, int64 scale);
+static bool AdjustFractDays(double frac, struct pg_itm_in *pg_itm_in, int scale);
+static bool AdjustFractMonths(double frac, struct pg_itm_in *itm_in, int scale);
+static bool AdjustMicroseconds(int64 val, struct pg_itm_in *itm_in, int64 multiplier, double fval);
+static bool AdjustDays(int val, struct pg_itm_in *itm_in, int multiplier);
+static bool AdjustYears(int val, struct pg_itm_in *itm_in, int multiplier);
static int DetermineTimeZoneOffsetInternal(struct pg_tm *tm, pg_tz *tzp,
pg_time_t *tp);
static bool DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t,
@@ -428,7 +433,7 @@ GetCurrentTimeUsec(struct pg_tm *tm, fsec_t *fsec, int *tzp)
* Note that any sign is stripped from the input seconds values.
*/
static char *
-AppendSeconds(char *cp, int sec, fsec_t fsec, int precision, bool fillzeros)
+AppendSeconds(char *cp, int sec, int64 fsec, int precision, bool fillzeros)
{
Assert(precision >= 0);
@@ -437,10 +442,9 @@ AppendSeconds(char *cp, int sec, fsec_t fsec, int precision, bool fillzeros)
else
cp = pg_ultostr(cp, Abs(sec));
- /* fsec_t is just an int32 */
if (fsec != 0)
{
- int32 value = Abs(fsec);
+ int64 value = Abs(fsec);
char *end = [precision + 1];
bool gotnonzero = false;
@@ -453,8 +457,8 @@ AppendSeconds(char *cp, int sec, fsec_t fsec, int precision, bool fillzeros)
*/
while (precision--)
{
- int32 oldval = value;
- int32 remainder;
+ int64 oldval = value;
+ int64 remainder;
value /= 10;
remainder = oldval - value * 10;
@@ -475,7 +479,7 @@ AppendSeconds(char *cp, int sec, fsec_t fsec, int precision, bool fillzeros)
* which will generate a correct answer in the minimum valid width.
*/
if (value)
- return pg_ultostr(cp, Abs(fsec));
+ return pg_ulltostr(cp, Abs(fsec));
return end;
}
@@ -497,36 +501,96 @@ AppendTimestampSeconds(char *cp, struct pg_tm *tm, fsec_t fsec)
}
/*
- * Multiply frac by scale (to produce seconds) and add to *tm & *fsec.
- * We assume the input frac is less than 1 so overflow is not an issue.
+ * Multiply frac by scale (to produce microseconds) and add to *itm.
+ * We assume the input frac is less than 1 so overflow of frac is not an issue.
*/
-static void
-AdjustFractSeconds(double frac, struct pg_tm *tm, fsec_t *fsec, int scale)
+static bool
+AdjustFractMicroseconds(long double frac, struct pg_itm_in *itm_in, int64 scale)
{
- int sec;
+ int64 usec;
+ int64 round = 0;
if (frac == 0)
- return;
+ return true;
frac *= scale;
- sec = (int) frac;
- tm->tm_sec += sec;
- frac -= sec;
- *fsec += rint(frac * 100);
+ usec = (int64) frac;
+ if (pg_add_s64_overflow(itm_in->tm_usec, usec, _in->tm_usec))
+ return false;
+
+ frac = frac - usec;
+ if (frac > 0.5)
+ round = 1;
+ else if (frac < -0.5)
+ round = -1;
+
+ return !pg_add_s64_overflow(itm_in->tm_usec, round, _in->tm_usec);
}
/* As above, but initial scale produces days */
-static void
-AdjustFractDays(double frac, struct pg_tm *tm, fsec_t *fsec, int scale)
+static bool
Re: Allow batched insert during cross-partition updates
Hi, On 2021-08-24 12:03:59 +0900, Amit Langote wrote: > Tomas committed the bug-fix, so attaching a rebased version of the > patch for $subject. This patch is in the current CF, but doesn't apply: http://cfbot.cputube.org/patch_37_2992.log marked the entry as waiting-on-author. Greetings, Andres Freund
Re: Extensible storage manager API - smgr hooks
Hi,
On 2021-06-30 05:36:11 +0300, Yura Sokolov wrote:
> Anastasia Lubennikova писал 2021-06-30 00:49:
> > Hi, hackers!
> >
> > Many recently discussed features can make use of an extensible storage
> > manager API. Namely, storage level compression and encryption [1],
> > [2], [3], disk quota feature [4], SLRU storage changes [5], and any
> > other features that may want to substitute PostgreSQL storage layer
> > with their implementation (i.e. lazy_restore [6]).
> >
> > Attached is a proposal to change smgr API to make it extensible. The
> > idea is to add a hook for plugins to get control in smgr and define
> > custom storage managers. The patch replaces smgrsw[] array and smgr_sw
> > selector with smgr() function that loads f_smgr implementation.
> >
> > As before it has only one implementation - smgr_md, which is wrapped
> > into smgr_standard().
> >
> > To create custom implementation, a developer needs to implement smgr
> > API functions
> > static const struct f_smgr smgr_custom =
> > {
> > .smgr_init = custominit,
> > ...
> > }
> >
> > create a hook function
> >
> >const f_smgr * smgr_custom(BackendId backend, RelFileNode rnode)
> > {
> > //Here we can also add some logic and chose which smgr to use
> > based on rnode and backend
> > return _custom;
> > }
> >
> > and finally set the hook:
> > smgr_hook = smgr_custom;
> >
> > [1]
> > https://www.postgresql.org/message-id/flat/11996861554042...@iva4-dd95b404a60b.qloud-c.yandex.net
> > [2]
> > https://www.postgresql.org/message-id/flat/272dd2d9.e52a.17235f2c050.Coremail.chjischj%40163.com
> > [3] https://postgrespro.com/docs/enterprise/9.6/cfs
> > [4]
> > https://www.postgresql.org/message-id/flat/CAB0yre%3DRP_ho6Bq4cV23ELKxRcfhV2Yqrb1zHp0RfUPEWCnBRw%40mail.gmail.com
> > [5]
> > https://www.postgresql.org/message-id/flat/20180814213500.GA74618%4060f81dc409fc.ant.amazon.com
> > [6]
> > https://wiki.postgresql.org/wiki/PGCon_2021_Fun_With_WAL#Lazy_Restore
> >
> > --
> >
> > Best regards,
> > Lubennikova Anastasia
>
> Good day, Anastasia.
>
> I also think smgr should be extended with different implementations aside of
> md.
> But which way concrete implementation will be chosen for particular
> relation?
> I believe it should be (immutable!) property of tablespace, and should be
> passed
> to smgropen. Patch in current state doesn't show clear way to distinct
> different
> implementations per relation.
>
> I don't think patch should be that invasive. smgrsw could pointer to
> array instead of static array as it is of now, and then reln->smgr_which
> will remain with same meaning. Yep it then will need a way to select
> specific
> implementation, but something like `char smgr_name[NAMEDATALEN]` field with
> linear search in (i believe) small smgrsw array should be enough.
>
> Maybe I'm missing something?
There has been no activity on this thread for > 6 months. Therefore I'm
marking it as returned with feedback. Anastasia, if you want to work on this,
please do, but there's obviously no way it can be merged into 15...
Greetings,
Andres
Re: PATCH: generate fractional cheapest paths in generate_orderedappend_path
Hi, On 2022-01-14 01:39:30 +0100, Tomas Vondra wrote: > Are you interested / willing to do some of this work? This patch hasn't moved in the last two months. I think it may be time to mark it as returned with feedback for now? It's also failing tests, and has done so for months: https://cirrus-ci.com/task/5308087077699584 https://api.cirrus-ci.com/v1/artifact/task/5308087077699584/log/src/test/regress/regression.diffs Greetings, Andres Freund
Re: PoC: using sampling to estimate joins / complex conditions
On 3/22/22 00:35, Andres Freund wrote: > Hi, > > On 2022-01-21 01:06:37 +0100, Tomas Vondra wrote: >> Yeah, I haven't updated some of the test output because some of those >> changes are a bit wrong (and I think that's fine for a PoC patch). I >> should have mentioned that in the message, though. Sorry about that. > > Given that the patch hasn't been updated since January and that it's a PoC in > the final CF, it seems like it should at least be moved to the next CF? Or > perhaps returned? > > I've just marked it as waiting-on-author for now - iirc that leads to fewer > reruns by cfbot once it's failing... > Either option works for me. > >> 2) The correlated samples are currently built using a query, executed >> through SPI in a loop. So given a "driving" sample of 30k rows, we do >> 30k lookups - that'll take time, even if we do that just once and cache >> the results. > > Ugh, yea, that's going to increase overhead by at least a few factors. > > >> I'm sure there there's room for some improvement, though - for example >> we don't need to fetch all columns included in the statistics object, >> but just stuff referenced by the clauses we're estimating. That could >> improve chance of using IOS etc. > > Yea. Even just avoid avoiding SPI / planner + executor seems likely to be a > big win. > > > It seems one more of the cases where we really need logic to recognize "cheap" > vs "expensive" plans, so that we only do sampling when useful. I don't think > that's solved just by having a declarative syntax. > Right. I was thinking about walking the first table, collecting all the values, and then doing a single IN () query for the second table - a bit like a custom join (which seems a bit terrifying, TBH). But even if we manage to make this much cheaper, there will still be simple queries where it's going to be prohibitively expensive. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Mingw task for Cirrus CI
Hi, On 2022-02-25 19:44:27 +0300, Melih Mutlu wrote: > I've been working on adding Windows+MinGW environment into cirrus-ci tasks > (discussion about ci is here [1]). This doesn't apply anymore: http://cfbot.cputube.org/patch_37_3575.log Could you rebase? Greetings, Andres Freund
Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
Hi, On 2022-02-19 11:06:18 -0500, Melanie Plageman wrote: > v21 rebased with compile errors fixed is attached. This currently doesn't apply (mea culpa likely): http://cfbot.cputube.org/patch_37_3272.log Could you rebase? Marked as waiting-on-author for now. - Andres
Re: Add connection active, idle time to pg_stat_activity
Hi, On 2022-02-04 10:58:24 +0100, Sergey Dudoladov wrote: > Thank you for the contribution. I included both of your diffs with > minor changes. This currently doesn't apply: http://cfbot.cputube.org/patch_37_3405.log Could you rebase? Marking as waiting on author for now. - Andres
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
On Sun, 20 Mar 2022 16:11:43 +0900 (JST) Tatsuo Ishii wrote: > > Hi Yugo, > > > > I tested with serialization error scenario by setting: > > default_transaction_isolation = 'repeatable read' > > The result was: > > > > $ pgbench -t 10 -c 10 --max-tries=10 test > > transaction type: > > scaling factor: 10 > > query mode: simple > > number of clients: 10 > > number of threads: 1 > > maximum number of tries: 10 > > number of transactions per client: 10 > > number of transactions actually processed: 100/100 > > number of failed transactions: 0 (0.000%) > > number of transactions retried: 35 (35.000%) > > total number of retries: 74 > > latency average = 5.306 ms > > initial connection time = 15.575 ms > > tps = 1884.516810 (without initial connection time) > > > > I had hard time to understand what those numbers mean: > > number of transactions retried: 35 (35.000%) > > total number of retries: 74 > > > > It seems "total number of retries" matches with the number of ERRORs > > reported in PostgreSQL. Good. What I am not sure is "number of > > transactions retried". What does this mean? > > Oh, ok. I see it now. It turned out that "number of transactions > retried" does not actually means the number of transactions > rtried. Suppose pgbench exectutes following in a session: > > BEGIN;-- transaction A starts > : > (ERROR) > ROLLBACK; -- transaction A aborts > > (retry) > > BEGIN;-- transaction B starts > : > (ERROR) > ROLLBACK; -- transaction B aborts > > (retry) > > BEGIN;-- transaction C starts > : > END; -- finally succeeds > > In this case "total number of retries:" = 2 and "number of > transactions retried:" = 1. In this patch transactions A, B and C are > regarded as "same" transaction, so the retried transaction count > becomes 1. But it's confusing to use the language "transaction" here > because A, B and C are different transactions. I would think it's > better to use different language instead of "transaction", something > like "cycle"? i.e. > > number of cycles retried: 35 (35.000%) In the original patch by Marina Polyakova it was "number of retried", but I changed it to "number of transactions retried" is because I felt it was confusing with "number of retries". I chose the word "transaction" because a transaction ends in any one of successful commit , skipped, or failure, after possible retries. Well, I agree with that it is somewhat confusing wording. If we can find nice word to resolve the confusion, I don't mind if we change the word. Maybe, we can use "executions" as well as "cycles". However, I am not sure that the situation is improved by using such word because what such word exactly means seems to be still unclear for users. Another idea is instead reporting only "the number of successfully retried transactions" that does not include "failed transactions", that is, transactions failed after retries, like this; number of transactions actually processed: 100/100 number of failed transactions: 0 (0.000%) number of successfully retried transactions: 35 (35.000%) total number of retries: 74 The meaning is clear and there seems to be no confusion. Regards, Yugo Nagata -- Yugo NAGATA
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
Hi Ishii-san, On Sun, 20 Mar 2022 09:52:06 +0900 (JST) Tatsuo Ishii wrote: > Hi Yugo, > > I have looked into the patch and I noticed that linkend=... endterm=...> is used in pgbench.sgml. e.g. > > > > AFAIK this is the only place where "endterm" is used. In other places > "link" tag is used instead: Thank you for pointing out it. I've checked other places using referring to , and found that "xreflabel"s are used in such tags. So, I'll fix it in this style. Regards, Yugo Nagata -- Yugo NAGATA
Re: [PATCH] Add TOAST support for several system tables
On 2022-02-28 18:08:48 -0500, Tom Lane wrote: > =?UTF-8?B?U29maWEgS29waWtvdmE=?= writes: > > ACL lists in tables may potentially be large in size. I suggest adding > > TOAST support for system tables, namely pg_class, pg_attribute and > > pg_largeobject_metadata, for they include ACL columns. > > TBH, the idea of adding a toast table to pg_class scares the daylights > out of me. I do not for one minute believe that you've fixed every > problem that will cause, nor do I think "allow wider ACLs" is a > compelling enough reason to take the risk. > > I wonder if it'd be practical to move the ACLs for relations > and attributes into some new table, indexed like pg_depend or > pg_description, so that we could dodge that whole problem. > pg_attrdef is a prototype for how this could work. > > (Obviously, once we had such a table the ACLs for other things > could be put in it, but I'm not sure that the ensuing breakage > would be justified for other sorts of objects.) As there has been no progress since this email, I'm marking this CF entry as returned with feedback for now. - Andres
Re: [PATCH] Full support for index LP_DEAD hint bits on standby
Hi, On 2022-01-25 19:21:01 +0800, Julien Rouhaud wrote: > I rebased the pathset in attached v9. Please double check that I didn't miss > anything in the rebase. Fails to apply at the moment: http://cfbot.cputube.org/patch_37_2947.log Marked as waiting for author. - Andres
Re: Parameter for planner estimate of recursive queries
Hi, On 2022-03-10 17:42:14 +, Simon Riggs wrote: > Shall I set this as Ready For Committer? Currently this CF entry fails on cfbot: https://cirrus-ci.com/task/4531771134967808?logs=test_world#L1158 [16:27:35.772] # Failed test 'no parameters missing from postgresql.conf.sample' [16:27:35.772] # at t/003_check_guc.pl line 82. [16:27:35.772] # got: '1' [16:27:35.772] # expected: '0' [16:27:35.772] # Looks like you failed 1 test of 3. [16:27:35.772] [16:27:35] t/003_check_guc.pl .. Marked as waiting on author. - Andres
Re: Make mesage at end-of-recovery less scary.
Hi, On 2022-03-04 09:43:59 +0900, Kyotaro Horiguchi wrote: > On second thought the two seems repeating the same things. Thus I > merged the two comments together. In this verion 16 it looks like > this. Patch currently fails to apply, needs a rebase: http://cfbot.cputube.org/patch_37_2490.log Greetings, Andres Freund
Re: Identify missing publications from publisher while create/alter subscription.
On 2022-02-13 19:34:05 +0530, vignesh C wrote: > Thanks for the comments, the attached v14 patch has the changes for the same. The patch needs a rebase, it currently fails to apply: http://cfbot.cputube.org/patch_37_2957.log
Re: Fast COPY FROM based on batch insert
On 2021-06-07 16:16:58 +0500, Andrey Lepikhov wrote: > Second version of the patch fixes problems detected by the FDW regression > tests and shows differences of error reports in tuple-by-tuple and batched > COPY approaches. Patch doesn't apply and likely hasn't for a while...
Re: pgcrypto: Remove internal padding implementation
On Wed, Mar 16, 2022 at 11:12:06AM +0100, Peter Eisentraut wrote: > Right, the previous behaviors were clearly faulty. I have updated the > commit message to call out the behavior change more clearly. > > This patch is now complete from my perspective. I took a look at this patch and found nothing of concern. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Add parameter jit_warn_above_fraction
Hi, On 2022-03-07 13:10:32 +0100, Magnus Hagander wrote: > Meanwhile here is an updated based on your other comments above, as > well as those from Julien. This fails on cfbot, due to compiler warnings: https://cirrus-ci.com/task/5127667648299008?logs=mingw_cross_warning#L390 Greetings, Andres Freund
Re: TAP output format in pg_regress
Hi, On 2022-02-22 15:10:11 +0100, Daniel Gustafsson wrote: > > On 22 Feb 2022, at 00:08, Daniel Gustafsson wrote: > > > I'll fix that. > > The attached v3 fixes that thinko, and cleans up a lot of the output which > isn't diagnostic per the TAP spec to make it less noisy. It also fixes tag > support used in the ECPG tests and a few small cleanups. There is a blank > line > printed which needs to be fixed, but I'm running out of time and wanted to get > a non-broken version on the list before putting it aside for today. > > The errorpaths that exit(2) the testrun should be converted to "bail out" > lines > when running with TAP output, but apart from that I think it's fairly spec > compliant. This is failing with segmentation faults on cfbot: https://cirrus-ci.com/task/4879227009892352?logs=test_world#L21 Looks like something around isolationtester is broken? Unfortunately there's no useful backtraces for isolationtester. Not sure what's up there. Seems like we might not have energy to push this forward in the next two weeks, so maybe the CF entry should be marked returned or moved for now? Greetings, Andres Freund
Re: Add sub-transaction overflow status in pg_stat_activity
On 2022-01-14 11:25:45 -0500, Tom Lane wrote: > Julien Rouhaud writes: > > Like many I previously had to investigate a slowdown due to sub-transaction > > overflow, and even with the information available in a monitoring view (I > > had > > to rely on a quick hackish extension as I couldn't patch postgres) it's not > > terribly fun to do this way. On top of that log analyzers like pgBadger > > could > > help to highlight such a problem. > > It feels to me like far too much effort is being invested in fundamentally > the wrong direction here. If the subxact overflow business is causing > real-world performance problems, let's find a way to fix that, not put > effort into monitoring tools that do little to actually alleviate anyone's > pain. There seems to be some agreement on this (I certainly do agree). Thus it seems we should mark the CF entry as rejected? It's been failing on cfbot for weeks... https://cirrus-ci.com/task/5289336424890368?logs=docs_build#L347
Re: PoC: using sampling to estimate joins / complex conditions
Hi, On 2022-01-21 01:06:37 +0100, Tomas Vondra wrote: > Yeah, I haven't updated some of the test output because some of those > changes are a bit wrong (and I think that's fine for a PoC patch). I > should have mentioned that in the message, though. Sorry about that. Given that the patch hasn't been updated since January and that it's a PoC in the final CF, it seems like it should at least be moved to the next CF? Or perhaps returned? I've just marked it as waiting-on-author for now - iirc that leads to fewer reruns by cfbot once it's failing... > 2) The correlated samples are currently built using a query, executed > through SPI in a loop. So given a "driving" sample of 30k rows, we do > 30k lookups - that'll take time, even if we do that just once and cache > the results. Ugh, yea, that's going to increase overhead by at least a few factors. > I'm sure there there's room for some improvement, though - for example > we don't need to fetch all columns included in the statistics object, > but just stuff referenced by the clauses we're estimating. That could > improve chance of using IOS etc. Yea. Even just avoid avoiding SPI / planner + executor seems likely to be a big win. It seems one more of the cases where we really need logic to recognize "cheap" vs "expensive" plans, so that we only do sampling when useful. I don't think that's solved just by having a declarative syntax. Greetings, Andres Freund
Re: New Object Access Type hooks
> On Mar 21, 2022, at 1:30 PM, Andrew Dunstan wrote: > > To the best of my knowledge .control files are only used by extensions, > not by other modules. They are only referenced in > src/backend/commands/extension.c in the backend code. For example, > auto_explain which is a loadable module but not en extension does not > have one, and I bet if you remove it you'll find this will work just fine. Fixed, also with adjustments to Joshua's function comments. v3-0001-Add-String-object-access-hooks.patch Description: Binary data v3-0002-Add-regression-tests-of-Object-Access-Type-hooks.patch Description: Binary data — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH] add relation and block-level filtering to pg_waldump
On Mon, Mar 21, 2022 at 4:39 PM Thomas Munro wrote: [snip] I guess you did this because init fork references aren't really > expected in the WAL, but I think it's more consistent to allow up to > MAX_FORKNUM, not least because your documentation mentions 3 as a > valid value. So I adjust this to allow MAX_FORKNUM. Make sense? > Makes sense, but I think I'd actually thought it was +1 of the max forks, so you give me more credit than I deserve in this case... :-) > Here are some more details I noticed, as a likely future user of this > very handy feature, which I haven't changed, because they seem more > debatable and you might disagree... > > 1. I think it'd be less surprising if the default value for --fork > wasn't 0... why not show all forks? > Agreed; made it default to all, with the ability to filter down if desired. > 2. I think it'd be less surprising if --fork without --relation > either raised an error (like --block without --relation), or were > allowed, with the meaning "show me this fork of all relations". > Agreed; reworked to support the use case of only showing target forks. > 3. It seems funny to have no short switch for --fork when everything > else has one... what about -F? > Good idea; I'd hadn't seen capitals in the getopt list so didn't consider them, but I like this. Enclosed is v6, incorporating these fixes and docs tweaks. Best, David v6-0001-Add-additional-filtering-options-to-pg_waldump.patch Description: Binary data
Re: Concurrent deadlock scenario with DROP INDEX on partitioned index
Tom Lane wrote: > Hence, I propose the attached. 0001 is pure refactoring: it hopefully > clears up the confusion about which "relkind" is which, and it also > saves a couple of redundant syscache fetches in RemoveRelations. > Then 0002 adds the actual bug fix as well as a test case that does > deadlock with unpatched code. The proposed patches look great and make much more sense. I see you've already squashed and committed in 7b6ec86532c2ca585d671239bba867fe380448ed. Thanks! -- Jimmy Yih (VMware) Gaurab Dey (VMware)
Re: Estimating HugePages Requirements?
On Tue, Mar 15, 2022 at 03:44:39PM -0700, Nathan Bossart wrote: > A simple approach could be to just set log_min_messages to PANIC before > exiting. I've attached a patch for this. With this patch, we'll still see > a FATAL if we try to use 'postgres -C' for a runtime-computed GUC on a > running server, and there will be no extra output as long as the user sets > log_min_messages to INFO or higher (i.e., not a DEBUG* value). For > comparison, 'postgres -C' for a non-runtime-computed GUC does not emit > extra output as long as the user sets log_min_messages to DEBUG2 or higher. I created a commitfest entry for this: https://commitfest.postgresql.org/38/3596/ -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: freeing bms explicitly
On Mon, Mar 21, 2022 at 3:05 PM Tom Lane wrote: > Zhihong Yu writes: > >> I was looking at calls to bms_free() in PG code. > >> e.g. src/backend/commands/publicationcmds.c line 362 > >> bms_free(bms); > >> The above is just an example, there're other calls to bms_free(). > >> Since the bms is allocated from some execution context, I wonder why > this > >> call is needed. > >> > >> When the underlying execution context wraps up, isn't the bms freed ? > > Yeah, that's kind of pointless --- and the pfree(rfnode) after it is even > more pointless, since it'll free only the top node of that expression > tree. Not to mention the string returned by TextDatumGetCString, and > whatever might be leaked during the underlying catalog accesses. > > If we were actually worried about transient space consumption of this > function, it'd be necessary to do a lot more than this. It doesn't > look to me like it's worth worrying about though -- it doesn't seem > like it could be hit more than once per query in normal cases. > > regards, tom lane > Thanks Tom for replying. What do you think of the following patch ? Cheers rfcolumn-free.patch Description: Binary data
Re: freeing bms explicitly
Zhihong Yu writes: >> I was looking at calls to bms_free() in PG code. >> e.g. src/backend/commands/publicationcmds.c line 362 >> bms_free(bms); >> The above is just an example, there're other calls to bms_free(). >> Since the bms is allocated from some execution context, I wonder why this >> call is needed. >> >> When the underlying execution context wraps up, isn't the bms freed ? Yeah, that's kind of pointless --- and the pfree(rfnode) after it is even more pointless, since it'll free only the top node of that expression tree. Not to mention the string returned by TextDatumGetCString, and whatever might be leaked during the underlying catalog accesses. If we were actually worried about transient space consumption of this function, it'd be necessary to do a lot more than this. It doesn't look to me like it's worth worrying about though -- it doesn't seem like it could be hit more than once per query in normal cases. regards, tom lane
Re: logical decoding and replication of sequences
On 3/21/22 14:05, Peter Eisentraut wrote: > On 20.03.22 23:55, Tomas Vondra wrote: >> Attached is a rebased patch, addressing most of the remaining issues. > > This looks okay to me, if the two FIXMEs are addressed. Remember to > also update protocol.sgml if you change LOGICAL_REP_MSG_SEQUENCE. Thanks. Do we want to use a different constant for the sequence message? I've used 'X' for the WIP patch, but maybe there's a better value? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: shared-memory based stats collector - v66
On Sun, Mar 20, 2022 at 4:56 PM Melanie Plageman
wrote:
>
> Addressed all of these points in
> v2-0001-add-replica-cleanup-tests.patch
>
> also added a new test file in
> v2-0002-Add-TAP-test-for-discarding-stats-after-crash.patch
> testing correct behavior after a crash and when stats file is invalid
>
Attached is the last of the tests confirming clean up for stats in the
shared stats hashtable (these are for the subscription stats).
I thought that maybe these tests could now use
pg_stat_force_next_flush() instead of poll_query_until() but I wasn't
sure how to ensure that the error has happened and the pending entry has
been added before setting force_next_flush.
I also added in tests that resetting subscription stats works as
expected.
- Melanie
From ffb83cc6ad2941f1d01b42b55dd0615a011d59cf Mon Sep 17 00:00:00 2001
From: Melanie Plageman
Date: Mon, 21 Mar 2022 14:52:55 -0400
Subject: [PATCH] add subscriber stats reset and drop tests
---
src/test/subscription/t/026_stats.pl | 303 ---
1 file changed, 230 insertions(+), 73 deletions(-)
diff --git a/src/test/subscription/t/026_stats.pl b/src/test/subscription/t/026_stats.pl
index a42ea3170e..e86bfb4fea 100644
--- a/src/test/subscription/t/026_stats.pl
+++ b/src/test/subscription/t/026_stats.pl
@@ -18,83 +18,240 @@ my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init(allows_streaming => 'logical');
$node_subscriber->start;
-# Initial table setup on both publisher and subscriber. On subscriber we
-# create the same tables but with primary keys. Also, insert some data that
-# will conflict with the data replicated from publisher later.
-$node_publisher->safe_psql(
- 'postgres',
- qq[
-BEGIN;
-CREATE TABLE test_tab1 (a int);
-INSERT INTO test_tab1 VALUES (1);
-COMMIT;
-]);
-$node_subscriber->safe_psql(
- 'postgres',
- qq[
-BEGIN;
-CREATE TABLE test_tab1 (a int primary key);
-INSERT INTO test_tab1 VALUES (1);
-COMMIT;
-]);
-
-# Setup publication.
-my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
-$node_publisher->safe_psql('postgres',
- "CREATE PUBLICATION tap_pub FOR TABLE test_tab1;");
+
+sub create_sub_pub_w_errors
+{
+ my ($publisher, $subscriber, $db, $table_name) = @_;
+ # Initial table setup on both publisher and subscriber. On subscriber we
+ # create the same tables but with primary keys. Also, insert some data that
+ # will conflict with the data replicated from publisher later.
+ $publisher->safe_psql(
+ $db,
+ qq[
+ BEGIN;
+ CREATE TABLE $table_name(a int);
+ INSERT INTO $table_name VALUES (1);
+ COMMIT;
+ ]);
+ $subscriber->safe_psql(
+ $db,
+ qq[
+ BEGIN;
+ CREATE TABLE $table_name(a int primary key);
+ INSERT INTO $table_name VALUES (1);
+ COMMIT;
+ ]);
+
+ # Set up publication.
+ my $pub_name = $table_name . '_pub';
+ my $publisher_connstr = $publisher->connstr . qq( dbname=$db);
+
+ $publisher->safe_psql($db,
+ qq(CREATE PUBLICATION $pub_name FOR TABLE $table_name));
+
+ # Create subscription. The tablesync for table on subscription will enter into
+ # infinite error loop due to violating the unique constraint.
+ my $sub_name = $table_name . '_sub';
+ $subscriber->safe_psql($db,
+ qq(CREATE SUBSCRIPTION $sub_name CONNECTION '$publisher_connstr' PUBLICATION $pub_name)
+ );
+
+ $publisher->wait_for_catchup($sub_name);
+
+ # TODO: can this be replaced with pg_stat_force_next_flush() and a test
+ # that sync error > 0?
+ # Wait for the tablesync error to be reported.
+ $node_subscriber->poll_query_until(
+ $db,
+ qq[
+ SELECT sync_error_count > 0
+ FROM pg_stat_subscription_stats
+ WHERE subname = '$sub_name'
+ ]) or die qq(Timed out while waiting for tablesync errors for subscription '$sub_name');
+
+ # Truncate test_tab1 so that tablesync worker can continue.
+ $subscriber->safe_psql($db, qq(TRUNCATE $table_name));
+
+ # Wait for initial tablesync to finish.
+ $subscriber->poll_query_until(
+ $db,
+ qq[
+ SELECT count(1) = 1 FROM pg_subscription_rel
+ WHERE srrelid = '$table_name'::regclass AND srsubstate in ('r', 's')
+ ]) or die qq(Timed out while waiting for subscriber to synchronize data for table '$table_name'.);
+
+ # Check test table on the subscriber has one row.
+ my $result = $subscriber->safe_psql($db, qq(SELECT a FROM $table_name));
+ is($result, qq(1), qq(Check that table '$table_name' now has 1 row.));
+
+ # Insert data to test table on the publisher, raising an error on the
+ # subscriber due to violation of the unique constraint on test table.
+ $publisher->safe_psql($db, qq(INSERT INTO $table_name VALUES (1)));
+
+ # TODO: can this be replaced with a pg_stat_force_next_flush() and a test
+ # that apply error > 0?
+ # Wait for the apply error to be reported.
+ $subscriber->poll_query_until(
+ $db,
+ qq[
+ SELECT apply_error_count > 0
+ FROM pg_stat_subscription_stats
+ WHERE subname = '$sub_name'
+ ]) or die qq(Timed out while waiting for apply error for subscription '$sub_name');
+
+ # Truncate test table so that apply
Re: [PATCH] add relation and block-level filtering to pg_waldump
On Tue, Mar 22, 2022 at 6:14 AM David Christensen
wrote:
> Updated to include the V3 fixes as well as the unsigned int/enum fix.
Hi David,
I ran this though pg_indent and adjusted some remaining
non-project-style whitespace, and took it for a spin. Very minor
comments:
pg_waldump: error: could not parse valid relation from ""/ (expecting
"tablespace OID/database OID/relation filenode")
-> There was a stray "/" in that message, which I've removed in the attached.
pg_waldump: error: could not parse valid relation from "1664/0/1262"
(expecting "tablespace OID/database OID/relation filenode")
-> Why not? Shared relations like pg_database have invalid database
OID, so I think it should be legal to write --relation=1664/0/1262. I
took out that restriction.
+ if (sscanf(optarg, "%u",
) != 1 ||
+ forknum >= MAX_FORKNUM)
+ {
+ pg_log_error("could
not parse valid fork number (0..%d) \"%s\"",
+
MAX_FORKNUM - 1, optarg);
+ goto bad_argument;
+ }
I guess you did this because init fork references aren't really
expected in the WAL, but I think it's more consistent to allow up to
MAX_FORKNUM, not least because your documentation mentions 3 as a
valid value. So I adjust this to allow MAX_FORKNUM. Make sense?
Here are some more details I noticed, as a likely future user of this
very handy feature, which I haven't changed, because they seem more
debatable and you might disagree...
1. I think it'd be less surprising if the default value for --fork
wasn't 0... why not show all forks?
2. I think it'd be less surprising if --fork without --relation
either raised an error (like --block without --relation), or were
allowed, with the meaning "show me this fork of all relations".
3. It seems funny to have no short switch for --fork when everything
else has one... what about -F?
From 596181e9dfe2db9d5338b3ac3c899d230fe0fc78 Mon Sep 17 00:00:00 2001
From: David Christensen
Date: Fri, 25 Feb 2022 12:52:56 -0600
Subject: [PATCH v5] Add additional filtering options to pg_waldump.
This feature allows you to only output records that are touch a specific
RelFileNode and optionally BlockNumber and ForkNum in this relation. We
also add the independent ability to filter Full Page Write records.
Author: David Christensen
Reviewed-by: Peter Geoghegan
Reviewed-by: Japin Li
Reviewed-by: Bharath Rupireddy
Reviewed-by: Cary Huang
Reviewed-by: Thomas Munro
Discussion: https://postgr.es/m/lzzgmgm6e5.fsf%40veeddrois.attlocal.net
---
doc/src/sgml/ref/pg_waldump.sgml | 48 +++
src/bin/pg_waldump/pg_waldump.c | 132 ++-
2 files changed, 179 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml
index 5735a161ce..f157175764 100644
--- a/doc/src/sgml/ref/pg_waldump.sgml
+++ b/doc/src/sgml/ref/pg_waldump.sgml
@@ -100,6 +100,44 @@ PostgreSQL documentation
+
+ -k block
+ --block=block
+
+
+Display only records touching the given block. (Requires also
+providing the relation via --relation.)
+
+
+
+
+
+ --fork=fork
+
+
+When using the --relation filter, output only records
+from the given fork. The valid values here are 0
+for the main fork, 1 for the Free Space
+Map, 2 for the Visibility Map,
+and 3 for the Init fork. If unspecified, defaults
+to the main fork.
+
+
+
+
+
+ -l tbl/db/rel
+ --relation=tbl/db/rel
+
+
+Display only records touching the given relation. The relation is
+specified via tablespace OID, database OID, and relfilenode separated
+by slashes, for instance, 1234/12345/12345. This
+is the same format used for relations in the WAL dump output.
+
+
+
+
-n limit
--limit=limit
@@ -183,6 +221,16 @@ PostgreSQL documentation
+
+ -w
+ --fullpage
+
+
+ Filter records to only those which have full page writes.
+
+
+
+
-x xid
--xid=xid
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index fc081adfb8..eb0c9b2dcb 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -55,6 +55,12 @@ typedef struct XLogDumpConfig
bool filter_by_rmgr_enabled;
TransactionId filter_by_xid;
bool filter_by_xid_enabled;
+ RelFileNode filter_by_relation;
+ bool filter_by_relation_enabled;
+ BlockNumber filter_by_relation_block;
+ bool filter_by_relation_block_enabled;
+ ForkNumber filter_by_relation_forknum;
+ bool filter_by_fpw;
} XLogDumpConfig;
typedef struct Stats
@@
Re: freeing bms explicitly
> > Hi, > I was looking at calls to bms_free() in PG code. > > e.g. src/backend/commands/publicationcmds.c line 362 > > bms_free(bms); > > The above is just an example, there're other calls to bms_free(). > Since the bms is allocated from some execution context, I wonder why this > call is needed. > > When the underlying execution context wraps up, isn't the bms freed ? > > Cheers > > >
