Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
On 18.06.25 07:49, Peter Smith wrote:
I'm also suggesting that "clean" or "cleanup" may be even better than
"drop". Because if you look at related tools such as pg_basebackup,
pg_receivewal, etc., the "create" and "drop" actions all happen on the
remote instance, but what we are talking about here happens on the local
(new) instance, so a slightly different term from those might be
appropriate. Moreover, "clean(up)" has a connotation "don't need it
anymore", which is fitting for this.
I am fine with changing the name to "clean" or "cleanup" as it has
some precedence as well but would like to see if Peter or David has
any opinion on this, as they were previously involved in naming this
option.
My original comment was only considering the resulting action so at
the time I felt "cleaning" publications made less sense to me than
just saying what was really happening ("dropping" the publications).
But I was not taking into account any precedent from other command
option names. So if "clean" is already a precedent, then I am fine
with calling this option clean too.
Here is a patch with a straight rename from --remove to --clean,
dropping the short option. I'm not planning to do anything about the
argument format.
From ca732064488a712fed58f362e1033e119a591b99 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sun, 22 Jun 2025 17:42:29 +0200
Subject: [PATCH] pg_createsubscriber: Rename option --remove to --clean
After discussion, the name --remove was suboptimally chosen. --clean
has more precedent in other PostgreSQL tools.
---
doc/src/sgml/ref/pg_createsubscriber.sgml | 59 +--
src/bin/pg_basebackup/pg_createsubscriber.c | 34 +--
.../t/040_pg_createsubscriber.pl | 6 +-
3 files changed, 49 insertions(+), 50 deletions(-)
diff --git a/doc/src/sgml/ref/pg_createsubscriber.sgml
b/doc/src/sgml/ref/pg_createsubscriber.sgml
index 4b1d08d5f16..bb9cc72576c 100644
--- a/doc/src/sgml/ref/pg_createsubscriber.sgml
+++ b/doc/src/sgml/ref/pg_createsubscriber.sgml
@@ -169,36 +169,6 @@ Options
-
- -R objtype
- --remove=objtype
-
-
- Remove all objects of the specified type from specified databases on the
- target server.
-
-
-
-
-
- publications:
- The FOR ALL TABLES publications established for
this
- subscriber are always removed; specifying this object type causes all
- other publications replicated from the source server to be dropped as
- well.
-
-
-
-
-
- The objects selected to be dropped are individually logged, including
during
- a --dry-run. There is no opportunity to affect or
stop the
- dropping of the selected objects, so consider taking a backup of them
- using pg_dump.
-
-
-
-
-s dir
--socketdir=dir
@@ -259,6 +229,35 @@ Options
+
+ --clean=objtype
+
+
+ Drop all objects of the specified type from specified databases on the
+ target server.
+
+
+
+
+
+ publications:
+ The FOR ALL TABLES publications established for
this
+ subscriber are always dropped; specifying this object type causes all
+ other publications replicated from the source server to be dropped as
+ well.
+
+
+
+
+
+ The objects selected to be dropped are individually logged, including
during
+ a --dry-run. There is no opportunity to affect or
stop the
+ dropping of the selected objects, so consider taking a backup of them
+ using pg_dump.
+
+
+
+
--config-file=filename
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c
b/src/bin/pg_basebackup/pg_createsubscriber.c
index c43c0cbbba5..11f71c03801 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -46,7 +46,7 @@ struct CreateSubscriberOptions
SimpleStringList replslot_names;/* list of replication slot
names */
int recovery_timeout; /* stop recovery after
this time */
boolall_dbs;/* all option */
- SimpleStringList objecttypes_to_remove; /* list of object types to
remove */
+ SimpleStringList objecttypes_to_clean; /* list of object types to
cleanup */
};
/* per-database publication/subscription info */
@@ -71,8 +71,8 @@ struct LogicalRepInfos
{
struct LogicalRepInfo *dbinfo;
booltwo_phase; /* enable-two-phase option */
- bits32 objecttypes_to_remove; /* flags indicating which
object types
-
* to remove on subscriber */
+ bits32 objecttypes_to_clean; /* flags indicating
Re: [PATCH] Proposal to Enable/Disable Index using ALTER INDEX
Hi Shayon, On Sat, Jun 21, 2025 at 9:38 PM Shayon Mukherjee wrote: > > > > On Jun 11, 2025, at 9:00 AM, Sami Imseih wrote: > > >> IMO, having this GUC to force the use of invisible indexes is quite >> strange. In my view, it detracts from the guarantees that you're meant >> to get from disabling indexes. What if some connection has >> use_invisible_index set to true? The DBA might assume all is well >> after having seen nobody complain and then drop the index. The user >> might then complain. > > > Sure, this may occur. I can also imagine cases where an index is made > visible only for certain workloads, intentionally. But such efforts should > be coordinated by application teams and DBAs. Someone would need to modify > this GUC at the connection level, alter the database, or change the session > via application code. An ad-hoc connection enabling this GUC is unlikely to > be an issue. > > I don't see how we could provide the INVISIBLE index DDL without also > providing this boolean GUC. If a user creates an index that is initially > INVISIBLE, they need a GUC to try it out before deciding to make it > visible. > > It was also pointed out in the thread above that this GUC can serve as a > backstop for replicas if the DDL to make an index visible is delayed. > > > Hello, > > Thank you everyone for all the discussions and also to Robert Treat for > feedback and the operational considerations. > > It seems like there are multiple ways to solve this problem, which is > encouraging. From the discussion, there appears to be consensus on few things > as well, including the DDL approach, which I personally am a proponent for as > well. > > I believe this is a valuable feature for DBAs and engineers working with > large databases. Esp since it provides the confidence to "turn off" an index > to observe the impact through their observability tools and make an informed > decision about whether to drop it. If they're wrong, they can quickly > rollback by making the index visible again, rather than waiting for a full > index rebuild that can take 30 minutes to hours. > > The primary use case I have in mind is for helping engineers (ones not so > seasoned like DBAs) decide whether to drop *existing* indexes. For new > indexes, I expect most users would create them in visible mode (the default). > Or so has been my experience so far. > > The GUC component opens the door for additional workflows, such as creating > an index as initially invisible (like Sami points out) and testing its > performance before making it visible. I originally wasn't thinking it this > way, but this demonstrates the flexibility of the feature and accommodates > different development approaches. > > As Robert noted, both approaches have trade-offs around operational safety > and granular control. However, I think the DDL approach provides the right > balance of simplicity and system-wide consistency that most users need, while > the GUC still enables experimentation for those who want it. > > I'm very much committed to iterating on this patch to address any remaining > feedback and help make progress on this. Is there something we can do here in > the essence of "start small, think big", perhaps? > > Thanks > Shayon > Based on your analysis, I think the patch could be split into two parts: one focusing on the DDL approach and the other on the additional GUC control. >From reading the discussions, it seems that the GUC control depends on the DDL approach (eg. creating an index as initially invisible and making it visible later). Therefore, maybe the DDL approach can be committed first and extend the GUC control later as needed? I read the v18 patch, I think the following changes should not be included: diff --git a/src/interfaces/ecpg/test/regression.diffs b/src/interfaces/ecpg/test/regression.diffs new file mode 100644 index 00..e69de29bb2 diff --git a/src/interfaces/ecpg/test/regression.out b/src/interfaces/ecpg/test/regression.out new file mode 100644 index 00..cb633f4d71 --- /dev/null +++ b/src/interfaces/ecpg/test/regression.out @@ -0,0 +1,55 @@ +# initializing database system by copying initdb template +# using temp instance on port 65312 with PID 30031 +ok 1 - compat_informix/dec_test 563 ms +ok 2 - compat_informix/charfuncs 255 ms +ok 3 - compat_informix/rfmtdate 355 ms -- Regards Junwang Zhao
Re: fix: propagate M4 env variable to flex subprocess
On Wed, May 28, 2025 at 11:43 AM J. Javier Maestro wrote: > On Wed, May 28, 2025 at 6:08 PM Andres Freund wrote: > ... > Do you want to write a patch like that? Otherwise I can. >> > > Sure, I've attached the new patch. Let me know what you think, and if it's > OK, what are the next steps to get the patch merged in main! > I checked 0001 version, it builds and works as expected. Not to lose it, created commitfest entry: https://commitfest.postgresql.org/patch/5835/ (and marked as ready, considering others' words in this thread)
Re: commitfest – hard to create entries after system upgrade
On Sun, 22 Jun 2025 at 23:05, Nikolay Samokhvalov wrote: > > Tried to create a new entry for someone's patch that was already commented by > Peter E as looking good, but: > > 1) when I went to the "PG19-1" commitfest, I couldn't find a button/link to > open new entries there. Could it be that you were/are not logged in? It seems the button is hidden. I created an issue for that now, but this is not new behaviour: https://github.com/postgres/pgcommitfest/issues/74 > 2) at homepage, there is a link in the bottom, > https://commitfest.postgresql.org/open/new/ – but it doesn't work does't > work, redirects to home page with these words: > > > More than one open commitfest exists, redirecting to startpage instead. Yeah, that was a dumb bug which I also noticed quickly after I had deployed to prod. That's fixed now.
RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
Dear Peter, Thanks for posting the patch. Largely it seems OK. One comment: I feel most of the word "remove" can be changed to "dropped", in pg_createsubscriber.c and 040_pg_createsubscriber.pl. E.g., ``` # Confirm the physical replication slot has been removed $result = $node_p->safe_psql($db1, "SELECT count(*) FROM pg_replication_slots WHERE slot_name = '$slotname'" ); is($result, qq(0), 'the physical replication slot used as primary_slot_name has been removed' ); ``` And ``` /* Remove failover replication slots if they exist on subscriber */ drop_failover_replication_slots(dbinfos.dbinfo); ``` Best regards, Hayato Kuroda FUJITSU LIMITED
Re: Memory allocation error DDL invalidation (seen with 15.13 & 16.9)
On Sat, Jun 21, 2025 at 12:57 AM Anne Struble wrote:
>
> Hello,
> I'm writing in regards to a fix made in the last release of Postgresql
> (specifically, I've looked at versions 15.13 and 16.9). The fix in question
> is:
> Fix data loss in logical replication
> This is the change set:
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=9f21be08e
> I did find this related thread in pgsql-bugs:
> https://www.postgresql.org/message-id/CAA4eK1L7CA-A%3DVMn8fiugZ%2BCRt%2Bwz473Adrx3nxq8Ougu%3DO2kQ%40mail.gmail.com.
>
> In our implementation of row level security (which is relatively complex) we
> rely heavily on temporary tables that we create with every query.
> Unfortunately, this approach appears to be incompatible with the above fix
> because of the change made to the DDL by creating temporary tables. If
> queries are issued quickly enough while having a logical replication stream
> open, executing:
> SELECT * FROM pg_logical_slot_get_changes('slot_name', null,null)
> will result in a memory allocation error (maybe related to above bug?):
> ERROR: invalid memory alloc request size 1086094000
>
We have pushed a fix for this. See
d87d07b7ad3b782cb74566cd771ecdb2823adf6a. If possible, can you once
test the fix?
> Time: 17449.051 ms (00:17.449)
> Additionally, note that executing pg_logical_slot_get_changes is quite slow
> (17s above). Steps for reproducing (outside of our environment) and system
> information are at bottom.
>
> Would it be possible for postgres to exclude temporary table creation from
> DDL changes that send "invalidation messages from catalog-modifying
> transactions to all concurrent in-progress transactions."?
>
Oh, this is an interesting idea to investigate. However, even if
something on these lines is feasible, we may want to just have it for
HEAD, but the first thing is to try to come up with a patch. If you or
someone else can come up with a patch, I'll try to help with the
review.
>
Or will we need to reimplement our approach to RLS?
>
I think you can wait for a few weeks to see if someone can come up
with a patch to improve this area.
--
With Regards,
Amit Kapila.
Re: Huge commitfest app update upcoming: Tags, Draft CF, Help page, and automated commitfest creat/open/close
On Sun, 22 Jun 2025 at 15:47, Peter Eisentraut wrote: > I don't know about this. This could become an ongoing source of > confusion, without any clear benefit. Either the draft and the "real" > commitfest are going to be indistinguishable, because they are just two > places to look for stuff to review in various phases of maturity. Or > the draft commitfest is just not going to get any attention, which will > be annoying for those who put things there hoping to get attention. I think I agree with this. We decided to go for the "Draft" name to align with GitHub terminology. But if our usage of it is different than how people often use the feature in GitHub it seems better to have a different name. How about we rename "Drafts" to "Parking Lot" (as originally proposed for the name) and add a "Draft" tag as an option to handle the "this is not finished, but I'd love some feedback anyway" situation?
Re: Huge commitfest app update upcoming: Tags, Draft CF, Help page, and automated commitfest creat/open/close
> Sounds good to me. Unless there are big objections, I'll deploy this > on the 23rd. Sorry if this has been already reported or fixed. I tried "Personal Dashboard". https://commitfest.postgresql.org/me/ And I found "Author" column is shown as "+4207-35" which does not seem to be an author name. Likewise followings columns seem to show inappropriate contents. Best regards, -- Tatsuo Ishii SRA OSS K.K. English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Skipping schema changes in publication
> > > 5. > > > + > > > + Alter publication mypublication to add table > > > + users except column > > > + security_pin: > > > + > > > +ALTER PUBLICATION production_publication ADD TABLE users EXCEPT > > > (security_pin); > > > > > > Those tags don't seem correct. e.g. "users" and "security_pin" are not > > > (???). > > > > > > Perhaps, every other example here is wrong too and you just copied > > > them? Anyway, something here looks wrong to me. > > > > > I saw different documents and usage of tags seems not well defined. > > For example for table we are using tags in document > > create_publication.sgml, update.sgml is used, in document > > table.sgml, advanced.sgml is used, and in > > logical-replication.sgml is used. Similarly for column > > names , or are used in different > > parts of the document. > > > > I kept the changed tag to for the column for this patch. > > Do you have any suggestions? > > No, for this patch I think it is best that you just follow nearby code > (as you are already doing). I plan to raise another thread to ask what > are the guidelines for this sort of markup which is currently used > inconsistently in different places. > FYI - I created a new thread asking this markup question [1]. == [1] https://www.postgresql.org/message-id/CAHut%2BPvtf24r%2BbdPgBind84dBLPvgNL7aB%2B%3DHxAUupdPuo2gRg%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia
Re: fix: propagate M4 env variable to flex subprocess
On Tue, Jun 17, 2025 at 6:15 AM Peter Eisentraut wrote: > This patch looks right to me. > Great, thanks! I would wait for the PG19 branching at this point, unless there is a > concrete need for backpatching. This patch fixes just one of the issues I found when trying to have a reproducible PG build... and, given the fact that there are still other issues that make a PG build non-reproducible (see the other patches in https://github.com/monogres/postgres/compare/REL_16_0...monogres/patches/16.0) I'd say it's OK if it's not backported. Thank you! Javier
Re: Fixes inconsistent behavior in vacuum when it processes multiple relations
On Fri, Jun 20, 2025 at 02:16:46PM -0500, Nathan Bossart wrote: > On Fri, Jun 20, 2025 at 10:34:19AM +0900, Michael Paquier wrote: >> However, Option 1) would be my go-to option for HEAD (as of v19 >> opening for business), but I think that we should harden the code more >> than suggested and treat all VacuumParams as purely input arguments >> rather keeping some pointers to it depending on the code path we are >> dealing with, so as no callers of these inner routines is surprised by >> changes that may happen internally. Hence, reading the code of v2, >> I'd suggest to apply the same rule to vacuum_get_cutoffs(), >> do_analyze_rel() and heap_vacuum_rel(). Except if I am missing >> something, it looks like all these calls should be OK with this new >> policy. This implies also changing relation_vacuum() in tableam.h, >> which can be a HEAD-only change anyway. > > Sounds reasonable to me. Looking at the git history, the decision of being able to change VacuumParams from the reloptions in vacuum_rel() for the index cleanup dates back to a96c41feec6b (April 2019). It has been copied a couple of days later for truncate in b84dbc8eb80b (8th of May 2019). The introduction of VacuumParams comes from 0d831389749a back in 2015, where more pointers to VacuumParams have been introduced for something I've authored. Perhaps we should have documented better the fact that this structure should never be manipulated with const markers back then, but what's done is done. So I'm at the origin of the design problem: even if the original refactoring of 0d831389749a did not break things, it has encouraged the pattern we have to deal with today because vacuum_rel() has begun this practice. Anyway, here is an attempt at leaning all that. I am really tempted to add a couple of const markers to force VacuumParams to be in read-only mode, even if it means doing so for vacuum() but not touch at vacuum_rel() where we double-check the reloptions for the truncate and index cleanup options. That would be of course v19-only material. Thoughts or opinions? -- Michael From 1e2f7cd189c5e7b2dad1207938d2f9a88667dcfa Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 23 Jun 2025 08:46:35 +0900 Subject: [PATCH v4] Make leaner the use of VacuumParams in the backend The structure is marked as const where it can be, so as we force the policy that callers should not change what has been given to them. vacuum_rel() stands as an exception, as it manipulates truncate and index_cleanup based on a reloptions lookup. --- src/include/access/heapam.h | 4 +- src/include/access/tableam.h | 6 +- src/include/commands/vacuum.h| 6 +- src/backend/access/heap/vacuumlazy.c | 44 - src/backend/commands/analyze.c | 26 +++--- src/backend/commands/cluster.c | 2 +- src/backend/commands/vacuum.c| 96 ++-- src/backend/postmaster/autovacuum.c | 2 +- src/test/regress/expected/vacuum.out | 28 ++ src/test/regress/sql/vacuum.sql | 14 +++ contrib/pgstattuple/expected/pgstattuple.out | 16 contrib/pgstattuple/sql/pgstattuple.sql | 12 +++ 12 files changed, 161 insertions(+), 95 deletions(-) diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index 3a9424c19c9a..a2bd5a897f87 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -21,6 +21,7 @@ #include "access/skey.h" #include "access/table.h" /* for backward compatibility */ #include "access/tableam.h" +#include "commands/vacuum.h" #include "nodes/lockoptions.h" #include "nodes/primnodes.h" #include "storage/bufpage.h" @@ -396,9 +397,8 @@ extern void log_heap_prune_and_freeze(Relation relation, Buffer buffer, OffsetNumber *unused, int nunused); /* in heap/vacuumlazy.c */ -struct VacuumParams; extern void heap_vacuum_rel(Relation rel, - struct VacuumParams *params, BufferAccessStrategy bstrategy); + const VacuumParams params, BufferAccessStrategy bstrategy); /* in heap/heapam_visibility.c */ extern bool HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot, diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index 8713e12cbfb9..1c9e802a6b12 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -20,6 +20,7 @@ #include "access/relscan.h" #include "access/sdir.h" #include "access/xact.h" +#include "commands/vacuum.h" #include "executor/tuptable.h" #include "storage/read_stream.h" #include "utils/rel.h" @@ -36,7 +37,6 @@ extern PGDLLIMPORT bool synchronize_seqscans; struct BulkInsertStateData; struct IndexInfo; struct SampleScanState; -struct VacuumParams; struct ValidateIndexState; /* @@ -645,7 +645,7 @@ typedef struct TableAmRoutine * integrate with autovacuum's scheduling. */ void (*relation_vacuum) (Relation rel, - struct VacuumParams *params, + const Vacuum
Re: Huge commitfest app update upcoming: Tags, Draft CF, Help page, and automated commitfest creat/open/close
On 22/06/2025 16:21, David G. Johnston wrote: On Sunday, June 22, 2025, Peter Eisentraut wrote: On 20.06.25 16:41, David G. Johnston wrote: I sense there could be some confusion whether such draft patches should go into the regular commit fest or the draft commit fest, and then also when they should move between them. Draft CF patches with “Needs Review” are looking for feedback from others or otherwise aid in development for a patch that isn’t ready to be committed even if said review turns up nothing or is otherwise fully resolved. Patches in Drafts are never marked Ready to Commit, they get moved to Open first. It will be nice if people spend time providing reviews/feedback to patches in Drafts when requested. It’s purely the author’s judgement on the readiness of the patch, whether absent our policy they would mark it ready to commit or not. If they believe it is it should be moved to open, if no, it should remain in drafts. This is mostly like what happens today but with a clear delineation between reviews to help and reviews to approve commit-ability. Otherwise, it’s a place where author patches can sit without having to be bumped to the next cf every other month and where an author patch can be ignored by everyone else not authoring it. I don't know about this. This could become an ongoing source of confusion, without any clear benefit. Either the draft and the "real" commitfest are going to be indistinguishable, because they are just two places to look for stuff to review in various phases of maturity. Or the draft commitfest is just not going to get any attention, which will be annoying for those who put things there hoping to get attention. Yes, more experienced people have to want to help people who can’t just get a patch ready to commit on their own. As opposed to only reviewing things they expect to perform the formality of the review to make it ready to commit. The tooling help distinguish those cases if used properly. But people have to choose to do the things it encourages/enables. If one performs a review of a non-draft and it isn’t close to ready, encourage the author to move it into drafts as part of a teaching moment of how their expectations of done-ness and yours differ. We aren’t going to get 100% accuracy here but it’s is better information that intends to address the complaint that what we had was not fit for purpose because the information it provided was insufficient. Tags get even more granular while this provides high-level draft/non-draft delineation where drafts don’t have to keep being shuffled around. Review Need still needs review no matter where it is. That doesn’t change. +1 -- Vik Fearing
Re: minimum Meson version
On 19.06.25 14:24, Andres Freund wrote: Along the way, I also found that our meson.build always issues a warning when run on Windows/msvc, which I fixed. (Should probably be backpatched.) Agreed. Looks like that came in with bc46104fc9aa I have committed and backpatched this. I'll park the rest of the patch set until PG19 opens.
pg_waldump -R filter
Hi hackers~ Recently when I was debugging with pg_waldump, I just want to filter out all the records about a specific relfilelocator, so I used -R T/D/R. I do get records like FPI, but not SMGR_CREATE / SMGR_TRUNCATE. Quick search of code tells me that pg_waldump would only keep records with block ref, while some WAL type does not have any block ref. $ pg_waldump -R 1663/5/16398 00010001 rmgr: XLOGlen (rec/tot): 62/ 210, tx:757, lsn: 0/40C20178, prev 0/40C20130, desc: FPI , blkref #0: rel 1663/5/16398 blk 0 FPW, blkref #1: rel 1663/5/16398 blk 1 FPW $ pg_waldump 00010001 | grep 16398 rmgr: Storage len (rec/tot): 42/42, tx: 0, lsn: 0/40C1FBD0, prev 0/40C1FBA8, desc: CREATE base/5/16398 rmgr: Standby len (rec/tot): 42/42, tx:757, lsn: 0/40C1FC38, prev 0/40C1FC00, desc: LOCK xid 757 db 5 rel 16398 rmgr: XLOGlen (rec/tot): 62/ 210, tx:757, lsn: 0/40C20178, prev 0/40C20130, desc: FPI , blkref #0: rel 1663/5/16398 blk 0 FPW, blkref #1: rel 1663/5/16398 blk 1 FPW rmgr: Transaction len (rec/tot):274/ 274, tx:757, lsn: 0/40C203D0, prev 0/40C20310, desc: COMMIT 2025-06-22 04:25:41.953683 UTC; ... It seems that -R is just a helper for -B. But since it can be used without -B, would it be better for pg_waldump to keep all WAL records about a rlocator no matter it has a block ref? It might be helpful for inspecting the whole life cycle of a rlocator. -- Regards, Jingtang
Re: leafhopper / snakefly failing to build HEAD - GCC bug
Hi, On Fri, 20 Jun 2025 at 10:41, Robins Tharakan wrote: > The signature seems to match an existing GCC bug and I've updated the thread in hopes that it gets fixed sooner. Ideally these failures should auto-fix in the coming days. > I don't see much traction upstream. I'll be away for a few weeks and given that at this point, this is just noise (for us anyway), I've disabled these (aarch64) instances for now. I'll re-enable them once I'm back. - robins https://robins.in
commitfest – hard to create entries after system upgrade
Tried to create a new entry for someone's patch that was already commented by Peter E as looking good, but: 1) when I went to the "PG19-1" commitfest, I couldn't find a button/link to open new entries there. 2) at homepage, there is a link in the bottom, https://commitfest.postgresql.org/open/new/ – but it doesn't work does't work, redirects to home page with these words: > More than one open commitfest exists, redirecting to startpage instead. Then I have guessed the URL that works, https://commitfest.postgresql.org/53/new/, but I guess this needs a fix. Nik
Re: Huge commitfest app update upcoming: Tags, Draft CF, Help page, and automated commitfest creat/open/close
On Sunday, June 22, 2025, Peter Eisentraut wrote: > On 20.06.25 16:41, David G. Johnston wrote: > >> I sense there could be some confusion whether such draft patches >> should go into the regular commit fest or the draft commit fest, and >> then also when they should move between them. >> >> Draft CF patches with “Needs Review” are looking for feedback from others >> or otherwise aid in development for a patch that isn’t ready to be >> committed even if said review turns up nothing or is otherwise fully >> resolved. Patches in Drafts are never marked Ready to Commit, they get >> moved to Open first. >> >> It will be nice if people spend time providing reviews/feedback to >> patches in Drafts when requested. >> >> It’s purely the author’s judgement on the readiness of the patch, whether >> absent our policy they would mark it ready to commit or not. If they >> believe it is it should be moved to open, if no, it should remain in >> drafts. This is mostly like what happens today but with a clear >> delineation between reviews to help and reviews to approve commit-ability. >> >> Otherwise, it’s a place where author patches can sit without having to be >> bumped to the next cf every other month and where an author patch can be >> ignored by everyone else not authoring it. >> > > I don't know about this. This could become an ongoing source of > confusion, without any clear benefit. Either the draft and the "real" > commitfest are going to be indistinguishable, because they are just two > places to look for stuff to review in various phases of maturity. Or the > draft commitfest is just not going to get any attention, which will be > annoying for those who put things there hoping to get attention. > > Yes, more experienced people have to want to help people who can’t just get a patch ready to commit on their own. As opposed to only reviewing things they expect to perform the formality of the review to make it ready to commit. The tooling help distinguish those cases if used properly. But people have to choose to do the things it encourages/enables. If one performs a review of a non-draft and it isn’t close to ready, encourage the author to move it into drafts as part of a teaching moment of how their expectations of done-ness and yours differ. We aren’t going to get 100% accuracy here but it’s is better information that intends to address the complaint that what we had was not fit for purpose because the information it provided was insufficient. Tags get even more granular while this provides high-level draft/non-draft delineation where drafts don’t have to keep being shuffled around. Review Need still needs review no matter where it is. That doesn’t change. David J.
Re: Huge commitfest app update upcoming: Tags, Draft CF, Help page, and automated commitfest creat/open/close
On Mon, 16 Jun 2025 at 14:47, Jelte Fennema-Nio wrote: > > The new PG19 development cycle is starting soon. So that seemed like a > good excuse to make some big improvements to the commitfest app. These changes have now been deployed to production. Please report any problems, either as a reply or as a github issue.
Re: Minor patch; missing comment update in worker.c
On Mon, Jun 23, 2025 at 8:22 AM Hayato Kuroda (Fujitsu) wrote: > > But this is not correct anymore, 1462aad2 allows to alter two_phase option. > I was an original author, but I did oversight. > > I feel it can be fixed by referring the commit message, attached patch fixed > like > that. How do you feel? > Thanks for the report and patch. I'll look into it. -- With Regards, Amit Kapila.
DOCS: What SGML markup to use for user objects like tables, columns, etc?
Hi, I am looking for some guidelines/recommended SGML tags to use when referring in the PG documentation to any user-defined schema/table/column names. This is most commonly seen near a SQL example. Currently, it seems a bit inconsistent. The rendering also looks quite different for these different markups. EXAMPLES OF DIFFERENT USAGE === 1. as seen in create_publication.sgml, alter_publication.sgml, ddl.sgml, etc. e.g. Add tables users, departments and schema production to the publication production_publication: ALTER PUBLICATION production_publication ADD TABLE users, departments, TABLES IN SCHEMA production; === 2. , as seen in logical-replication.sgml e.g. CREATE SUBSCRIPTION mysub CONNECTION 'dbname=foo host=bar user=repuser' PUBLICATION mypub; The above will start the replication process, which synchronizes the initial table contents of the tables users and departments and then starts replicating incremental changes to those tables. === 3. , as seen in advanced.sgml e.g. Let's create two tables: A table cities and a table capitals. Naturally, capitals are also cities, so you want some way to show the capitals implicitly when you list all cities. If you're really clever you might invent some scheme like this: CREATE TABLE capitals ( name text, population real, elevation int,-- (in ft) state char(2) ); == My AI tool says the following. PostgreSQL documentation typically uses: for specific, concrete names for generic placeholders for system objects and data types TBH, this seemed like good advice to me... however there are quite a few examples not following that. Thoughts? == Kind Regards, Peter Smith. Fujitsu Australia
Re: Slot's restart_lsn may point to removed WAL segment after hard restart unexpectedly
On Fri, Jun 20, 2025 at 2:24 PM vignesh C wrote: > On Fri, 20 Jun 2025 at 05:54, Alexander Korotkov wrote: > > Dear Kuroda-san, > > > > On Thu, Jun 19, 2025 at 2:05 PM Hayato Kuroda (Fujitsu) > > wrote: > > > > > Regarding assertion failure, I've found that assert in > > > > > PhysicalConfirmReceivedLocation() conflicts with restart_lsn > > > > > previously set by ReplicationSlotReserveWal(). As I can see, > > > > > ReplicationSlotReserveWal() just picks fresh XLogCtl->RedoRecPtr lsn. > > > > > So, it doesn't seems there is a guarantee that restart_lsn never goes > > > > > backward. The commit in ReplicationSlotReserveWal() even states there > > > > > is a "chance that we have to retry". > > > > > > > > > > > > > I don't see how this theory can lead to a restart_lsn of a slot going > > > > backwards. The retry mentioned there is just a retry to reserve the > > > > slot's position again if the required WAL is already removed. Such a > > > > retry can only get the position later than the previous restart_lsn. > > > > > > We analyzed the assertion failure happened at > > > pg_basebackup/020_pg_receivewal, > > > and confirmed that restart_lsn can go backward. This meant that Assert() > > > added > > > by the ca307d5 can cause crash. > > > > > > Background > > > === > > > When pg_receivewal starts the replication and it uses the replication > > > slot, it > > > sets as the beginning of the segment where restart_lsn exists, as the > > > startpoint. > > > E.g., if the restart_lsn of the slot is 0/B000D0, pg_receivewal requests > > > WALs > > > from 0/B0. > > > More detail of this behavior, see f61e1dd2 and d9bae531. > > > > > > What happened here > > > == > > > Based on above theory, walsender sent from the beginning of segment > > > (0/B0). > > > When walreceiver receives, it tried to send reply. At that time the > > > flushed > > > location of WAL would be 0/B0. walsender sets the received lsn as > > > restart_lsn > > > in PhysicalConfirmReceivedLocation(). Here the restart_lsn went backward > > > (0/B000D0->0/B0). > > > > > > The assertion failure could happen if CHECKPOINT happened at that time. > > > Attribute last_saved_restart_lsn of the slot was 0/B000D0, but the > > > data.restart_lsn > > > was 0/B0. It could not satisfy the assertion added in > > > InvalidatePossiblyObsoleteSlot(). > > > > Thank you for your detailed explanation! > > > > > Note > > > > > > 1. > > > In this case, starting from the beginning of the segment is not a > > > problem, because > > > the checkpoint process only removes WAL files from segments that precede > > > the > > > restart_lsn's wal segment. The current segment (0/B0) will not be > > > removed, > > > so there is no risk of data loss or inconsistency. > > > > > > 2. > > > A similar pattern applies to pg_basebackup. Both use logic that adjusts > > > the > > > requested streaming position to the start of the segment, and it replies > > > the > > > received LSN as flushed. > > > > > > 3. > > > I considered the theory above, but I could not reproduce > > > 040_standby_failover_slots_sync > > > because it is a timing issue. Have someone else reproduced? > > > > > > We are still investigating failure caused at > > > 040_standby_failover_slots_sync. > > > > I didn't manage to reproduce this. But as I see from the logs [1] on > > mamba that START_REPLICATION command was issued just before assert > > trap. Could it be something similar to what I described in [2]. > > Namely: > > 1. ReplicationSlotReserveWal() sets restart_lsn for the slot. > > 2. Concurrent checkpoint flushes that restart_lsn to the disk. > > 3. PhysicalConfirmReceivedLocation() sets restart_lsn of the slot to > > the beginning of the segment. > > Here is my analysis for the 040_standby_failover_slots_sync test > failure where the physical replication slot can point to backward lsn: > In certain rare cases the restart LSN can go backwards. This scenario > can be reproduced by performing checkpoint continuously and slowing > the WAL applying. I have a patch with changes to handle this. > Here's a summary of the sequence of events: > 1) Standby confirms a new LSN (0/40369C8) when primary sends some WAL > contents: > After the standby writes the received WAL contents in XLogWalRcvWrite, > the standby sends this lsn 0/40369C8 as the confirmed flush location. > The primary acknowledges this and updates the replication slot's > restart_lsn accordingly: > 2025-06-20 14:33:21.777 IST [134998] standby1 LOG: > PhysicalConfirmReceivedLocation replication slot "sb1_slot" set > restart_lsn to 0/40369C8 > 2025-06-20 14:33:21.777 IST [134998] standby1 STATEMENT: > START_REPLICATION SLOT "sb1_slot" 0/300 TIMELINE 1 > > 2) Shortly after receiving the new LSN, a checkpoint occurs which > saves this restart_lsn: > 2025-06-20 14:33:21.780 IST [134913] LOG: checkpoint complete: wrote > 0 buffers (0.0%), wrote 0 SLRU buffers; 0 WAL file(s) added, 0 > remo
Re: Huge commitfest app update upcoming: Tags, Draft CF, Help page, and automated commitfest creat/open/close
> On Mon, 23 Jun 2025 at 03:37, Tatsuo Ishii wrote: >> And I found "Author" column is shown as "+4207-35" which does not seem >> to be an author name. Likewise followings columns seem to show >> inappropriate contents. > > Thanks for the report. That's fixed now (it was missing a header > column for the new Tags column). Thanks for the prompt response. I confirmed the fix. Best regards, -- Tatsuo Ishii SRA OSS K.K. English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: pg_recvlogical cannot create slots with failover=true
On 17.06.25 20:19, Masahiko Sawada wrote: Ideally, we should change both to maintain consistency across various slot options. OTOH, as we have already described these options as: " The --two-phase and --failover options can be specified with --create-slot.", it is clear that these are slot options. The previous version docs have a description: "The --two-phase can be specified with --create-slot to enable decoding of prepared transactions." which makes it even more clear that the two-phase is a slot option. The options are named similarly in pg_create_logical_replication_slot API and during CREATE SUBSCRIPTION, so, to some level, there is a consistency in naming of these options across all APIs/tools. But, their usage in a tool like pg_recvlogical could be perceived differently as well, so it is also okay to consider naming them differently. Also note that we have a new pg_createsubscriber --enable-two-phase. Yeah, I also noticed the precedent. It would be nice if there was more consistency between similar/related tools. I've attached the patch. Feedback is very welcome. This looks fine to me, but I would keep the old name --two-phase as well. You could mark it as deprecated. No need to make a hard break.
Re: Huge commitfest app update upcoming: Tags, Draft CF, Help page, and automated commitfest creat/open/close
On 20.06.25 16:41, David G. Johnston wrote: I sense there could be some confusion whether such draft patches should go into the regular commit fest or the draft commit fest, and then also when they should move between them. Draft CF patches with “Needs Review” are looking for feedback from others or otherwise aid in development for a patch that isn’t ready to be committed even if said review turns up nothing or is otherwise fully resolved. Patches in Drafts are never marked Ready to Commit, they get moved to Open first. It will be nice if people spend time providing reviews/feedback to patches in Drafts when requested. It’s purely the author’s judgement on the readiness of the patch, whether absent our policy they would mark it ready to commit or not. If they believe it is it should be moved to open, if no, it should remain in drafts. This is mostly like what happens today but with a clear delineation between reviews to help and reviews to approve commit-ability. Otherwise, it’s a place where author patches can sit without having to be bumped to the next cf every other month and where an author patch can be ignored by everyone else not authoring it. I don't know about this. This could become an ongoing source of confusion, without any clear benefit. Either the draft and the "real" commitfest are going to be indistinguishable, because they are just two places to look for stuff to review in various phases of maturity. Or the draft commitfest is just not going to get any attention, which will be annoying for those who put things there hoping to get attention.
