Re: Add 64-bit XIDs into PostgreSQL 15
Hi, > Agree. > > To me it seems like we didn't reach a consensus regarding switching to > 64-bit XIDs. Given that and the fact that the patchset is rather > difficult to rebase (and review) I suggest focusing on something we > reached a consensus for. I'm going to close a CF entry for this > particular thread as RwF unless anyone objects. We can always return > to this later, preferably knowing that there is a particular committer > who has time and energy for merging this. I started a new thread and opened a new CF entry for Int64 GUCs: https://commitfest.postgresql.org/50/5253/ -- Best regards, Aleksander Alekseev
Re: Add 64-bit XIDs into PostgreSQL 15
Michael, Maxim, > Apparently, the original thread will inevitably disintegrate into many > separate ones. > For me, looks like some kind of road map. One for 64-bit GUCs, another one to > remove > short file names from SLRUs and, to make things more complicated, [1] for cf > entry [0], > to get rid of MultiXactOffset wraparound by switching to 64 bits. > > [0] https://commitfest.postgresql.org/49/5205/ > [1] > https://www.postgresql.org/message-id/flat/CACG%3DezaWg7_nt-8ey4aKv2w9LcuLthHknwCawmBgEeTnJrJTcw%40mail.gmail.com Agree. To me it seems like we didn't reach a consensus regarding switching to 64-bit XIDs. Given that and the fact that the patchset is rather difficult to rebase (and review) I suggest focusing on something we reached a consensus for. I'm going to close a CF entry for this particular thread as RwF unless anyone objects. We can always return to this later, preferably knowing that there is a particular committer who has time and energy for merging this. -- Best regards, Aleksander Alekseev
Re: Add 64-bit XIDs into PostgreSQL 15
Apparently, the original thread will inevitably disintegrate into many separate ones. For me, looks like some kind of road map. One for 64-bit GUCs, another one to remove short file names from SLRUs and, to make things more complicated, [1] for cf entry [0], to get rid of MultiXactOffset wraparound by switching to 64 bits. [0] https://commitfest.postgresql.org/49/5205/ [1] https://www.postgresql.org/message-id/flat/CACG%3DezaWg7_nt-8ey4aKv2w9LcuLthHknwCawmBgEeTnJrJTcw%40mail.gmail.com -- Best regards, Maxim Orlov.
Re: Add 64-bit XIDs into PostgreSQL 15
On Thu, Jul 25, 2024 at 02:09:10PM +0300, Heikki Linnakangas wrote: > On 25/07/2024 13:19, Peter Eisentraut wrote: >> Conversely, if there is still some threshold (not disaster, but >> efficiency or something else), would it still be useful to keep these >> settings well below 2^31? In which case, we might not need 64-bit GUCs. > > Yeah, I don't think it's critical. It makes sense to switch to 64 bit GUCs, > so that you can make those settings higher, but it's not critical or > strictly required for the rest of the work. It looks like we have some sort of consensus to introduce these GUC APIs, then? I'd suggest to split that into its own thread because it can be treated as an independent subject. (I know that this was discussed previously, but it's been some time and this is going to require a rebased version anyway, so..) > Another approach is to make the GUCs to mean "thousands of XIDs", similar to > how many of our memory settings are in kB rather than bytes. that might be a > super confusing change for existing settings though. I find that a bit confusing as well, still that would be OK in terms of compatibility as long as you enforce the presence of a unit and leave the default behavior alone. So it does not sound that bad to be, either. It is also a bit simpler to set for users. Less zeros to deal with. Aleksander Alekseev has proposed to remove short file names from SLRUs to cimplify its internals, as of this thread, so that's one less topic to deal with here: https://www.postgresql.org/message-id/CAJ7c6TOy7fUW9MuNeOWor3cSFnQg9tgz%3DmjXHDb94GORtM_Eyg%40mail.gmail.com I am unclear about the rest of the thread. Considering v55-0004 and v55-0003 as two different topics, what should we do with the other things? It does not seem to me that we have a clear consensus on if we are going to do something about some epoch:xid -> 64b XID switch, and what are the arguments in play here. The thread has been long, so I may be just missing the details but a summary would be nice. -- Michael signature.asc Description: PGP signature
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Sat, Aug 10, 2024 at 10:50:55AM -0700, Noah Misch wrote: > On Sat, Jul 27, 2024 at 07:24:33AM +0900, Michael Paquier wrote: >> I've just applied now the remaining pieces down to 17. > > Comparing commit c9e2457 to the patch in zqgvzsbw5tgkq...@paquier.xyz, the > commit lacks the slru.c portion. And a portion of multixact.c as well, thanks! I am pretty sure that I have messed up with a `git add` while doing a rebase on this dev branch. I'll take care of it. -- Michael signature.asc Description: PGP signature
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Sat, Jul 27, 2024 at 07:24:33AM +0900, Michael Paquier wrote: > I've just applied now the remaining pieces down to 17. Comparing commit c9e2457 to the patch in zqgvzsbw5tgkq...@paquier.xyz, the commit lacks the slru.c portion.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Fri, Jul 26, 2024 at 11:50:48PM +0300, Alexander Korotkov wrote: > Thanks to everybody for working on this. It's pity I didn't notice > this is v17 open item on me. Sorry for this. No problem. I've just applied now the remaining pieces down to 17. -- Michael signature.asc Description: PGP signature
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Fri, Jul 26, 2024 at 3:42 AM Noah Misch wrote: > On Thu, Jul 25, 2024 at 10:52:13AM +0900, Michael Paquier wrote: > > On Wed, Jul 24, 2024 at 06:00:59AM -0700, Noah Misch wrote: > > > I'm still seeing need for s/int/int64/ at: > > > I am attaching a patch for all these you have spotted, switching the > > logs to use %llx. Does that look fine for you? > > Yes. I think that completes the project. Thanks to everybody for working on this. It's pity I didn't notice this is v17 open item on me. Sorry for this. I tool a look on commits and on other slru code for similar issue. Everything looks good for me. -- Regards, Alexander Korotkov Supabase
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Thu, Jul 25, 2024 at 10:52:13AM +0900, Michael Paquier wrote: > On Wed, Jul 24, 2024 at 06:00:59AM -0700, Noah Misch wrote: > > I'm still seeing need for s/int/int64/ at: > I am attaching a patch for all these you have spotted, switching the > logs to use %llx. Does that look fine for you? Yes. I think that completes the project.
Re: Add 64-bit XIDs into PostgreSQL 15
Maybe a setting similar to max_wal_size could be better for that? +1 Thanks Peter Eisentraut 于2024年7月25日周四 21:31写道： > On 25.07.24 13:09, Heikki Linnakangas wrote: > >> However, if there is no more disaster threshold at 2^31, what is the > >> guidance for setting these? Or more radically, why even run > >> transaction-count-based vacuum at all? > > > > To allow the CLOG to be truncated. There's no disaster anymore, but > > without freezing, the clog will grow indefinitely. > > Maybe a setting similar to max_wal_size could be better for that? > > > >
Re: Add 64-bit XIDs into PostgreSQL 15
On 25.07.24 13:09, Heikki Linnakangas wrote: However, if there is no more disaster threshold at 2^31, what is the guidance for setting these? Or more radically, why even run transaction-count-based vacuum at all? To allow the CLOG to be truncated. There's no disaster anymore, but without freezing, the clog will grow indefinitely. Maybe a setting similar to max_wal_size could be better for that?
Re: Add 64-bit XIDs into PostgreSQL 15
On Thu, Jul 25, 2024 at 5:19 PM Peter Eisentraut wrote: > On 23.07.24 11:13, Aleksander Alekseev wrote: > >> Here is the fix. It can be tested like this: > >> [...] > > > > PFA the rebased patchset. > > I'm wondering about the 64-bit GUCs. > > At first, it makes sense that if there are settings that are counted in > terms of transactions, and transaction numbers are 64-bit integers, then > those settings should accept 64-bit integers. > > But what is the purpose and effect of setting those parameters to such > huge numbers? For example, what is the usability of being able to set > > vacuum_failsafe_age = 5000 > Also in the rebased patch set I cannot find the above, so I cannot evaluate what it does. In the past I have pushed for some mechanism to produce warnings like we currently have approaching xid wraparound when a certain threshold is met. Is this that mechanism? > > I think in the world of 32-bit transaction IDs, you can intuitively > interpret most of these "transaction age" settings as "percent toward > disaster". For example, > > vacuum_freeze_table_age = 15000 > > is 7% toward disaster, and > > vacuum_failsafe_age = 16 > > is 75% toward disaster. > > However, if there is no more disaster threshold at 2^31, what is the > guidance for setting these? Or more radically, why even run > transaction-count-based vacuum at all? > > Conversely, if there is still some threshold (not disaster, but > efficiency or something else), would it still be useful to keep these > settings well below 2^31? In which case, we might not need 64-bit GUCs. > > Your 0004 patch adds support for 64-bit GUCs but doesn't actually > convert any existing GUCs to use that. (Unlike the reloptions, which > your patch coverts.) And so there is no documentation about these > questions. > > -- Best Wishes, Chris Travers Efficito: Hosted Accounting and ERP. Robust and Flexible. No vendor lock-in. http://www.efficito.com/learn_more
Re: Add 64-bit XIDs into PostgreSQL 15
On 25/07/2024 13:19, Peter Eisentraut wrote: I'm wondering about the 64-bit GUCs. At first, it makes sense that if there are settings that are counted in terms of transactions, and transaction numbers are 64-bit integers, then those settings should accept 64-bit integers. But what is the purpose and effect of setting those parameters to such huge numbers? For example, what is the usability of being able to set vacuum_failsafe_age = 5000 I think in the world of 32-bit transaction IDs, you can intuitively interpret most of these "transaction age" settings as "percent toward disaster". For example, vacuum_freeze_table_age = 15000 is 7% toward disaster, and vacuum_failsafe_age = 16 is 75% toward disaster. However, if there is no more disaster threshold at 2^31, what is the guidance for setting these? Or more radically, why even run transaction-count-based vacuum at all? To allow the CLOG to be truncated. There's no disaster anymore, but without freezing, the clog will grow indefinitely. Conversely, if there is still some threshold (not disaster, but efficiency or something else), would it still be useful to keep these settings well below 2^31? In which case, we might not need 64-bit GUCs. Yeah, I don't think it's critical. It makes sense to switch to 64 bit GUCs, so that you can make those settings higher, but it's not critical or strictly required for the rest of the work. Another approach is to make the GUCs to mean "thousands of XIDs", similar to how many of our memory settings are in kB rather than bytes. that might be a super confusing change for existing settings though. -- Heikki Linnakangas Neon (https://neon.tech)
Re: Add 64-bit XIDs into PostgreSQL 15
On 23.07.24 11:13, Aleksander Alekseev wrote: Here is the fix. It can be tested like this: [...] PFA the rebased patchset. I'm wondering about the 64-bit GUCs. At first, it makes sense that if there are settings that are counted in terms of transactions, and transaction numbers are 64-bit integers, then those settings should accept 64-bit integers. But what is the purpose and effect of setting those parameters to such huge numbers? For example, what is the usability of being able to set vacuum_failsafe_age = 5000 I think in the world of 32-bit transaction IDs, you can intuitively interpret most of these "transaction age" settings as "percent toward disaster". For example, vacuum_freeze_table_age = 15000 is 7% toward disaster, and vacuum_failsafe_age = 16 is 75% toward disaster. However, if there is no more disaster threshold at 2^31, what is the guidance for setting these? Or more radically, why even run transaction-count-based vacuum at all? Conversely, if there is still some threshold (not disaster, but efficiency or something else), would it still be useful to keep these settings well below 2^31? In which case, we might not need 64-bit GUCs. Your 0004 patch adds support for 64-bit GUCs but doesn't actually convert any existing GUCs to use that. (Unlike the reloptions, which your patch coverts.) And so there is no documentation about these questions.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Wed, Jul 24, 2024 at 06:00:59AM -0700, Noah Misch wrote:
> I'm still seeing need for s/int/int64/ at:
Nice catches! I've missed these.
> - "pagesegno" variable
> - return value of MultiXactIdToOffsetSegment()
Only used in four places for two elog(DEBUG1) entries with %x.
> - return value of MXOffsetToMemberSegment()
Also used in four places for two elog(DEBUG1) entries with %x, plus
three callers in PerformMembersTruncation(), nothing fancy.
> Only the first should be a live bug, since multixact isn't electing the higher
> pageno ceiling.
Yes, and it makes a switch to long segment names everywhere easier.
There is a patch in the air to do that, without the pg_upgrade
additions required to do the transfer, though.
I am attaching a patch for all these you have spotted, switching the
logs to use %llx. Does that look fine for you?
--
Michael
From d2bae5b020ab6a5182a52f4a8e58a39a7d3a5947 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Thu, 25 Jul 2024 10:47:51 +0900
Subject: [PATCH] Fix a couple of extra spots missing the int->int64 switch for
SLRU pages
---
src/backend/access/transam/multixact.c | 39 +-
src/backend/access/transam/slru.c | 2 +-
2 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/src/backend/access/transam/multixact.c
b/src/backend/access/transam/multixact.c
index 675affe4f7..b7b47ef076 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -120,7 +120,7 @@ MultiXactIdToOffsetEntry(MultiXactId multi)
return multi % MULTIXACT_OFFSETS_PER_PAGE;
}
-static inline int
+static inline int64
MultiXactIdToOffsetSegment(MultiXactId multi)
{
return MultiXactIdToOffsetPage(multi) / SLRU_PAGES_PER_SEGMENT;
@@ -174,7 +174,7 @@ MXOffsetToMemberPage(MultiXactOffset offset)
return offset / MULTIXACT_MEMBERS_PER_PAGE;
}
-static inline int
+static inline int64
MXOffsetToMemberSegment(MultiXactOffset offset)
{
return MXOffsetToMemberPage(offset) / SLRU_PAGES_PER_SEGMENT;
@@ -3039,10 +3039,10 @@ SlruScanDirCbFindEarliest(SlruCtl ctl, char *filename,
int64 segpage, void *data
static void
PerformMembersTruncation(MultiXactOffset oldestOffset, MultiXactOffset
newOldestOffset)
{
- const int maxsegment =
MXOffsetToMemberSegment(MaxMultiXactOffset);
- int startsegment =
MXOffsetToMemberSegment(oldestOffset);
- int endsegment =
MXOffsetToMemberSegment(newOldestOffset);
- int segment = startsegment;
+ const int64 maxsegment = MXOffsetToMemberSegment(MaxMultiXactOffset);
+ int64 startsegment = MXOffsetToMemberSegment(oldestOffset);
+ int64 endsegment = MXOffsetToMemberSegment(newOldestOffset);
+ int64 segment = startsegment;
/*
* Delete all the segments but the last one. The last segment can still
@@ -3050,7 +3050,8 @@ PerformMembersTruncation(MultiXactOffset oldestOffset,
MultiXactOffset newOldest
*/
while (segment != endsegment)
{
- elog(DEBUG2, "truncating multixact members segment %x",
segment);
+ elog(DEBUG2, "truncating multixact members segment %llx",
+(unsigned long long) segment);
SlruDeleteSegment(MultiXactMemberCtl, segment);
/* move to next segment, handling wraparound correctly */
@@ -3201,14 +3202,14 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid
newOldestMultiDB)
}
elog(DEBUG1, "performing multixact truncation: "
-"offsets [%u, %u), offsets segments [%x, %x), "
-"members [%u, %u), members segments [%x, %x)",
+"offsets [%u, %u), offsets segments [%llx, %llx), "
+"members [%u, %u), members segments [%llx, %llx)",
oldestMulti, newOldestMulti,
-MultiXactIdToOffsetSegment(oldestMulti),
-MultiXactIdToOffsetSegment(newOldestMulti),
+(unsigned long long) MultiXactIdToOffsetSegment(oldestMulti),
+(unsigned long long)
MultiXactIdToOffsetSegment(newOldestMulti),
oldestOffset, newOldestOffset,
-MXOffsetToMemberSegment(oldestOffset),
-MXOffsetToMemberSegment(newOldestOffset));
+(unsigned long long) MXOffsetToMemberSegment(oldestOffset),
+(unsigned long long) MXOffsetToMemberSegment(newOldestOffset));
/*
* Do truncation, and the WAL logging of the truncation, in a critical
@@ -3461,14 +3462,14 @@ multixact_redo(XLogReaderState *record)
SizeOfMultiXactTruncate);
elog(DEBUG1, "replaying multixact truncation: "
-"offsets [%u, %u), offsets segments [%x, %x), "
-"members [%u, %u), members segments [%x, %x)",
+"offsets
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Tue, Jul 23, 2024 at 06:01:44PM +0900, Michael Paquier wrote: > Hearing nothing but cicadas as now is their season, I have taken the > initiative here to address this open item. > > 0001 felt a bit too complicated in slru.h, so I have simplified it and > kept all the details in slru.c with SlruFileName(). > > I have reviewed all the code that uses SLRUs, and spotted three more > problematic code paths in predicate.c that needed an update like the > others for some pagenos. I've added these, and applied 0002. We > should be good now. I'm still seeing need for s/int/int64/ at: - "pagesegno" variable - return value of MultiXactIdToOffsetSegment() - return value of MXOffsetToMemberSegment() - callers of previous two Only the first should be a live bug, since multixact isn't electing the higher pageno ceiling.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, > Hearing nothing but cicadas as now is their season, I have taken the > initiative here to address this open item. > > 0001 felt a bit too complicated in slru.h, so I have simplified it and > kept all the details in slru.c with SlruFileName(). > > I have reviewed all the code that uses SLRUs, and spotted three more > problematic code paths in predicate.c that needed an update like the > others for some pagenos. I've added these, and applied 0002. We > should be good now. Thank you! -- Best regards, Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Fri, Jul 12, 2024 at 12:44:54PM +0300, Aleksander Alekseev wrote: > Fair enough. Here is the updated patchset. Hearing nothing but cicadas as now is their season, I have taken the initiative here to address this open item. 0001 felt a bit too complicated in slru.h, so I have simplified it and kept all the details in slru.c with SlruFileName(). I have reviewed all the code that uses SLRUs, and spotted three more problematic code paths in predicate.c that needed an update like the others for some pagenos. I've added these, and applied 0002. We should be good now. -- Michael signature.asc Description: PGP signature
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi,
> A comment about v3-0001.
>
> -* If true, use long segment filenames formed from lower 48 bits of
> the
> -* segment number, e.g. pg_xact/1234. Otherwise, use short
> -* filenames formed from lower 16 bits of the segment number e.g.
> -* pg_xact/1234.
> +* If true, use long segment filenames. Use short filenames otherwise.
> +* See SlruFileName().
>
> We're losing some details here even if SlruFileName() has some
> explanations, because one would need to read through the snprintf's
> 04X to know that short file names include 4 characters. I'm OK to
> mention SlruFileName() rather than duplicate the knowledge here, but
> SlruFileName() should also be updated to mention the same level of
> details with some examples of file names.
Fair enough. Here is the updated patchset.
--
Best regards,
Aleksander Alekseev
From 0b8d67ab6cc582b356e639175f85d30982f2fb2c Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev
Date: Wed, 26 Jun 2024 12:59:15 +0300
Subject: [PATCH v4 1/2] Refactor the comments about formatting SLRU segment
filenames
The comment for SlruCtlData.long_segment_names was just wrong, an oversight
of commit 4ed8f0913bfd.
While on it, additionally clarify what SlruFileName() does and how it uses
SlruCtlData.long_segment_names.
Aleksander Alekseev. Reported by Noah Misch. Reviewed by Michael Paquier.
---
src/backend/access/transam/slru.c | 22 ++
src/include/access/slru.h | 15 +++
2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 77b05cc0a7..168df26065 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -72,6 +72,28 @@
#include "storage/shmem.h"
#include "utils/guc_hooks.h"
+/*
+ * Converts segment number to the filename of the segment.
+ *
+ * path: should point to a buffer at least MAXPGPATH characters long.
+ *
+ * If ctl->long_segment_names is true, segno can be in 0 .. (2^60-1) range and
+ * the resulting filename will be like:
+ *
+ * slru_dir/123456789ABCDEF (15 characters)
+ *
+ * This is the recommended way of formatting for all new code.
+ *
+ * If ctl->long_segment_names is false, segno can be in 0 .. (2^24-1) range.
+ * The resulting filename will be one of:
+ *
+ * slru_dir/1234 if0 <= segno < 2^16
+ * slru_dir/12345if 2^16 <= segno < 2^20
+ * slru_dir/123456 if 2^20 <= segno < 2^24
+ *
+ * This is the legacy way of formatting that is kept for backward compatibility.
+ * New code shouldn't use it.
+ */
static inline int
SlruFileName(SlruCtl ctl, char *path, int64 segno)
{
diff --git a/src/include/access/slru.h b/src/include/access/slru.h
index 8a8d191873..c836fa8534 100644
--- a/src/include/access/slru.h
+++ b/src/include/access/slru.h
@@ -134,10 +134,17 @@ typedef struct SlruCtlData
bits16 bank_mask;
/*
- * If true, use long segment filenames formed from lower 48 bits of the
- * segment number, e.g. pg_xact/1234. Otherwise, use short
- * filenames formed from lower 16 bits of the segment number e.g.
- * pg_xact/1234.
+ * If true, use long segment filenames. Use short filenames otherwise.
+ *
+ * Each segment contains SLRU_PAGES_PER_SEGMENT pages, i.e.:
+ *
+ * Segment #Contains pages
+ * 00 .. (SLRU_PAGES_PER_SEGMENT-1)
+ * 1SLRU_PAGES_PER_SEGMENT .. (SLRU_PAGES_PER_SEGMENT*2-1)
+ * ... etc ..
+ *
+ * The filename of the segment is formed from its number. For more details
+ * regarding exact rules see SlruFileName().
*/
bool long_segment_names;
--
2.45.2
From 6811fa964cce2f2f308793ca8d627d7c6dc3f1b2 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev
Date: Wed, 26 Jun 2024 14:02:42 +0300
Subject: [PATCH v4 2/2] Use int64 for page numbers in clog.c & async.c
Oversight of 4ed8f0913bfd
Aleksander Alekseev, Noah Misch, Michael Paquier
---
src/backend/access/transam/clog.c | 4 ++--
src/backend/commands/async.c | 22 +++---
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index 44c253246b..e6f79320e9 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -445,7 +445,7 @@ TransactionGroupUpdateXidStatus(TransactionId xid, XidStatus status,
PGPROC *proc = MyProc;
uint32 nextidx;
uint32 wakeidx;
- int prevpageno;
+ int64 prevpageno;
LWLock *prevlock = NULL;
/* We should definitely have an XID whose status needs to be updated. */
@@ -577,7 +577,7 @@ TransactionGroupUpdateXidStatus(TransactionId xid, XidStatus status,
while (nextidx != INVALID_PROC_NUMBER)
{
PGPROC *nextproc = &ProcGlobal->allProcs[nextidx];
- int thispageno = nextproc->clogGroupMemberPage;
+ int64 thispageno = nextproc->clogGroupMemberPage;
/*
* If the page to update belongs to a different bank than the p
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Thu, Jul 11, 2024 at 01:11:05PM +0300, Aleksander Alekseev wrote: > Thanks, Michael. I prepared a corrected patchset. A comment about v3-0001. -* If true, use long segment filenames formed from lower 48 bits of the -* segment number, e.g. pg_xact/1234. Otherwise, use short -* filenames formed from lower 16 bits of the segment number e.g. -* pg_xact/1234. +* If true, use long segment filenames. Use short filenames otherwise. +* See SlruFileName(). We're losing some details here even if SlruFileName() has some explanations, because one would need to read through the snprintf's 04X to know that short file names include 4 characters. I'm OK to mention SlruFileName() rather than duplicate the knowledge here, but SlruFileName() should also be updated to mention the same level of details with some examples of file names. -- Michael signature.asc Description: PGP signature
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, > The proposed patch looks rather incomplete to me, based on the fact > that this stuff has a lot of inconsistencies with the types used when > manipulating 64b SLRU pages. Some of them are harder to catch as the > variables don't specifically refer to pages. > > So, even after v2, there are two more of these in asyncQueueUsage() > with the two QUEUE_POS_PAGE() for the head and tail positions: > int headPage = QUEUE_POS_PAGE(QUEUE_HEAD); > int tailPage = QUEUE_POS_PAGE(QUEUE_TAIL); > > asyncQueueReadAllNotifications() also has one: > int curpage = QUEUE_POS_PAGE(pos); > > asyncQueueAdvanceTail() declares the following: > int oldtailpage; > int newtailpage; > int boundary; > > AsyncQueueControl.stopPage is an int. > > And that's only for async.c. Alexander K., as the owner of the open > item, are you planning to look at that? Thanks, Michael. I prepared a corrected patchset. -- Best regards, Aleksander Alekseev v3-0001-Fix-the-comment-for-SlruCtlData.long_segment_name.patch Description: Binary data v3-0002-Use-int64-for-page-numbers-in-clog.c-async.c.patch Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, Jul 08, 2024 at 12:30:09PM +0300, Aleksander Alekseev wrote: > TWIMC this is currently listed as an open item for PG17 [1]. > Sorry if everyone interested is already aware. > > [1]: https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items The proposed patch looks rather incomplete to me, based on the fact that this stuff has a lot of inconsistencies with the types used when manipulating 64b SLRU pages. Some of them are harder to catch as the variables don't specifically refer to pages. So, even after v2, there are two more of these in asyncQueueUsage() with the two QUEUE_POS_PAGE() for the head and tail positions: int headPage = QUEUE_POS_PAGE(QUEUE_HEAD); int tailPage = QUEUE_POS_PAGE(QUEUE_TAIL); asyncQueueReadAllNotifications() also has one: int curpage = QUEUE_POS_PAGE(pos); asyncQueueAdvanceTail() declares the following: int oldtailpage; int newtailpage; int boundary; AsyncQueueControl.stopPage is an int. And that's only for async.c. Alexander K., as the owner of the open item, are you planning to look at that? -- Michael signature.asc Description: PGP signature
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, > Here is the corrected patchset. TWIMC this is currently listed as an open item for PG17 [1]. Sorry if everyone interested is already aware. [1]: https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items -- Best regards, Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, > > I prepared the patch for clog.c. The rest of the warnings don't strike > > me as something we should immediately act on unless we have a bug > > report. Or perhaps there is a particular warning that worries you? > > Is "int" acceptable or unacceptable in the following grep match? > > src/backend/commands/async.c:1274: int headPage = > QUEUE_POS_PAGE(QUEUE_HEAD); Good catch. We better use int64s here. Here is the corrected patchset. -- Best regards, Aleksander Alekseev v2-0001-Fix-the-comment-for-SlruCtlData.long_segment_name.patch Description: Binary data v2-0002-Use-int64-for-page-numbers-in-clog.c-async.c.patch Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Wed, Jun 26, 2024 at 02:09:58PM +0300, Aleksander Alekseev wrote: > > This yielded commit 4ed8f09 "Index SLRUs by 64-bit integers rather than by > > 32-bit integers" and left some expressions coercing SLRU page numbers to > > int. > > Two sources: > > > > grep -i 'int\b.*page' $(git grep -l SimpleLruInit) > > make && touch $(git grep -l SimpleLruInit) && make PROFILE=-Wconversion > > 2>&1 | less -p '.int. from .int64. may alter its value' > > > > (Not every match needs to change.) > > I examined the new warnings introduced by 4ed8f09. Most of them seem > to be harmless, for instance: [...] > I prepared the patch for clog.c. The rest of the warnings don't strike > me as something we should immediately act on unless we have a bug > report. Or perhaps there is a particular warning that worries you? Is "int" acceptable or unacceptable in the following grep match? src/backend/commands/async.c:1274: int headPage = QUEUE_POS_PAGE(QUEUE_HEAD);
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi Noah,
> This yielded commit 4ed8f09 "Index SLRUs by 64-bit integers rather than by
> 32-bit integers" and left some expressions coercing SLRU page numbers to int.
> Two sources:
>
> grep -i 'int\b.*page' $(git grep -l SimpleLruInit)
> make && touch $(git grep -l SimpleLruInit) && make PROFILE=-Wconversion 2>&1
> | less -p '.int. from .int64. may alter its value'
>
> (Not every match needs to change.)
I examined the new warnings introduced by 4ed8f09. Most of them seem
to be harmless, for instance:
```
slru.c:657:43: warning: conversion from ‘int64’ {aka ‘long int’} to
‘int’ may change value [-Wconversion]
657 | int rpageno = pageno %
SLRU_PAGES_PER_SEGMENT;
```
```
slru.c: In function ‘SlruReportIOError’:
slru.c:962:43: warning: conversion from ‘int64’ {aka ‘long int’} to
‘int’ may change value [-Wconversion]
962 | int rpageno = pageno %
SLRU_PAGES_PER_SEGMENT;
```
Interestingly the patch decreased the overall number of warnings.
I prepared the patch for clog.c. The rest of the warnings don't strike
me as something we should immediately act on unless we have a bug
report. Or perhaps there is a particular warning that worries you?
> > @@ -127,7 +127,15 @@ typedef struct SlruCtlData
> >* the behavior of this callback has no functional implications.) Use
> >* SlruPagePrecedesUnitTests() in SLRUs meeting its criteria.
> >*/
> > - bool(*PagePrecedes) (int, int);
> > + bool(*PagePrecedes) (int64, int64);
> > +
> > + /*
> > + * If true, use long segment filenames formed from lower 48 bits of
> > the
> > + * segment number, e.g. pg_xact/1234. Otherwise, use short
> > + * filenames formed from lower 16 bits of the segment number e.g.
> > + * pg_xact/1234.
> > + */
> > + boollong_segment_names;
>
> SlruFileName() makes 15-character (60-bit) file names. Where does the 48-bit
> limit arise? How does the SlruFileName() comment about a 24-bit limit for
> short names relate this comment's 16-bit limit?
Yes, this comment is wrong. Here is a fix.
[1]:
https://www.postgresql.org/message-id/CAJ7c6TNMuKWUuMfh5KWgJJBoJGqPHYdZeN4t%2BLB6WdRLbDfVTw%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
v1-0002-Use-int64-for-page-numbers-in-clog.c.patch
Description: Binary data
v1-0001-Fix-the-comment-for-SlruCtlData.long_segment_name.patch
Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, Nov 27, 2023 at 01:43:26AM +0200, Alexander Korotkov wrote: > v61 looks good to me. I'm going to push it as long as there are no > objections. This yielded commit 4ed8f09 "Index SLRUs by 64-bit integers rather than by 32-bit integers" and left some expressions coercing SLRU page numbers to int. Two sources: grep -i 'int\b.*page' $(git grep -l SimpleLruInit) make && touch $(git grep -l SimpleLruInit) && make PROFILE=-Wconversion 2>&1 | less -p '.int. from .int64. may alter its value' (Not every match needs to change.) > --- a/src/include/access/slru.h > +++ b/src/include/access/slru.h > @@ -127,7 +127,15 @@ typedef struct SlruCtlData >* the behavior of this callback has no functional implications.) Use >* SlruPagePrecedesUnitTests() in SLRUs meeting its criteria. >*/ > - bool(*PagePrecedes) (int, int); > + bool(*PagePrecedes) (int64, int64); > + > + /* > + * If true, use long segment filenames formed from lower 48 bits of the > + * segment number, e.g. pg_xact/1234. Otherwise, use short > + * filenames formed from lower 16 bits of the segment number e.g. > + * pg_xact/1234. > + */ > + boollong_segment_names; SlruFileName() makes 15-character (60-bit) file names. Where does the 48-bit limit arise? How does the SlruFileName() comment about a 24-bit limit for short names relate this comment's 16-bit limit? nm
Re: Add 64-bit XIDs into PostgreSQL 15
On Wed, Dec 13, 2023 at 5:56 PM Maxim Orlov wrote: > > Hi! > > Just to keep this thread up to date, here's a new version after recent > changes in SLRU. > I'm also change order of the patches in the set, to make adding initdb MOX > options after the > "core 64 xid" patch, since MOX patch is unlikely to be committed and now for > test purpose only. I tried to apply the patch but it is failing at the Head. It is giving the following error: Hunk #1 succeeded at 601 (offset 5 lines). patching file src/backend/replication/slot.c patching file src/backend/replication/walreceiver.c patching file src/backend/replication/walsender.c Hunk #1 succeeded at 2434 (offset 160 lines). patching file src/backend/storage/ipc/procarray.c Hunk #1 succeeded at 1115 with fuzz 2. Hunk #3 succeeded at 1286 with fuzz 2. Hunk #7 FAILED at 4341. Hunk #8 FAILED at 4899. Hunk #9 FAILED at 4959. 3 out of 10 hunks FAILED -- saving rejects to file src/backend/storage/ipc/procarray.c.rej patching file src/backend/storage/ipc/standby.c Hunk #1 FAILED at 1043. Hunk #2 FAILED at 1370. 2 out of 2 hunks FAILED -- saving rejects to file src/backend/storage/ipc/standby.c.rej Please send the Re-base version of the patch. Thanks and Regards, Shubham Khanna.
Re: Add 64-bit XIDs into PostgreSQL 15
Hi Pavel Borisov Many thanks Best whish Pavel Borisov 于2023年12月15日周五 17:13写道： > Hi, Wenhui! > > On Fri, 15 Dec 2023 at 05:52, wenhui qiu wrote: > >> Hi Maxim Orlov >> Good news,xid64 has achieved a successful first phase,I tried to >> change the path status (https://commitfest.postgresql.org/43/3594/) ,But >> it seems incorrect >> >> Maxim Orlov 于2023年12月13日周三 20:26写道： >> >>> Hi! >>> >>> Just to keep this thread up to date, here's a new version after recent >>> changes in SLRU. >>> I'm also change order of the patches in the set, to make adding initdb >>> MOX options after the >>> "core 64 xid" patch, since MOX patch is unlikely to be committed and now >>> for test purpose only. >>> >> > If the patch is RwF the CF entry is finished and can't be enabled, rather > the patch needs to be submitted in a new entry, which I have just done. > https://commitfest.postgresql.org/46/4703/ > > Please feel free to submit your review. > > Kind regards, > Pavel Borisov, > Supabase >
Re: Add 64-bit XIDs into PostgreSQL 15
Hi, Wenhui! On Fri, 15 Dec 2023 at 05:52, wenhui qiu wrote: > Hi Maxim Orlov > Good news,xid64 has achieved a successful first phase,I tried to > change the path status (https://commitfest.postgresql.org/43/3594/) ,But > it seems incorrect > > Maxim Orlov 于2023年12月13日周三 20:26写道： > >> Hi! >> >> Just to keep this thread up to date, here's a new version after recent >> changes in SLRU. >> I'm also change order of the patches in the set, to make adding initdb >> MOX options after the >> "core 64 xid" patch, since MOX patch is unlikely to be committed and now >> for test purpose only. >> > If the patch is RwF the CF entry is finished and can't be enabled, rather the patch needs to be submitted in a new entry, which I have just done. https://commitfest.postgresql.org/46/4703/ Please feel free to submit your review. Kind regards, Pavel Borisov, Supabase
Re: Add 64-bit XIDs into PostgreSQL 15
Hi Maxim Orlov Good news,xid64 has achieved a successful first phase,I tried to change the path status (https://commitfest.postgresql.org/43/3594/) ,But it seems incorrect Maxim Orlov 于2023年12月13日周三 20:26写道： > Hi! > > Just to keep this thread up to date, here's a new version after recent > changes in SLRU. > I'm also change order of the patches in the set, to make adding initdb MOX > options after the > "core 64 xid" patch, since MOX patch is unlikely to be committed and now > for test purpose only. > > -- > Best regards, > Maxim Orlov. >
回复: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi orlo...@gmail.com That's good news, I think we can continue discuss for (https://commitfest.postgresql.org/43/3594/) Best regards 发件人: John Naylor 发送时间: 2023年12月4日 18:22 收件人: Pavel Borisov 抄送: Alexander Korotkov ; Tom Lane ; Alexander Lakhin ; Maxim Orlov ; Aleksander Alekseev ; Postgres hackers ; Heikki Linnakangas ; Japin Li ; Andres Freund ; Michael Paquier ; Peter Eisentraut 主题: Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15) On Mon, Dec 4, 2023 at 3:12 PM Pavel Borisov wrote: > I think this is complete and could be closed. Done.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, Dec 4, 2023 at 3:12 PM Pavel Borisov wrote: > I think this is complete and could be closed. Done.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, 4 Dec 2023 at 10:34, John Naylor wrote: > On Thu, Nov 30, 2023 at 4:37 PM Alexander Korotkov > wrote: > > > > On Thu, Nov 30, 2023 at 10:29 AM Pavel Borisov > wrote: > > > Agree. The fix is attached. > > > > What an oversight. > > Thank you, pushed! > > With that, is there any more work pending, or can we close the CF entry? > I think this is complete and could be closed. Regards, Pavel
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Thu, Nov 30, 2023 at 4:37 PM Alexander Korotkov wrote: > > On Thu, Nov 30, 2023 at 10:29 AM Pavel Borisov wrote: > > Agree. The fix is attached. > > What an oversight. > Thank you, pushed! With that, is there any more work pending, or can we close the CF entry?
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Thu, Nov 30, 2023 at 10:29 AM Pavel Borisov wrote: > Agree. The fix is attached. What an oversight. Thank you, pushed! -- Regards, Alexander Korotkov
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Thu, 30 Nov 2023 at 08:03, Tom Lane wrote: > Alexander Lakhin writes: > > And a warning: > > $ CC=gcc-12 CFLAGS="-Wall -Wextra -Wno-unused-parameter > -Wno-sign-compare -Wno-clobbered > > -Wno-missing-field-initializers" ./configure -q && make -s > > slru.c:63:1: warning: ‘inline’ is not at beginning of declaration > [-Wold-style-declaration] > > 63 | static int inline > >| ^~ > > > Maybe it's worth fixing before committing... > > This should have been fixed before commit, because there are now a > dozen buildfarm animals complaining about it, as well as who-knows- > how-many developers' compilers. > > calliphoridae | 2023-11-30 02:48:59 | > /home/bf/bf-build/calliphoridae/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > canebrake | 2023-11-29 14:22:10 | > /home/bf/bf-build/canebrake/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > culicidae | 2023-11-30 02:49:06 | > /home/bf/bf-build/culicidae/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > desmoxytes| 2023-11-30 03:11:15 | > /home/bf/bf-build/desmoxytes/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > flaviventris | 2023-11-30 02:53:19 | > /home/bf/bf-build/flaviventris/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > francolin | 2023-11-30 02:26:08 | > ../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not > at beginning of declaration [-Wold-style-declaration] > grassquit | 2023-11-30 02:58:36 | > /home/bf/bf-build/grassquit/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > komodoensis | 2023-11-30 03:07:52 | > /home/bf/bf-build/komodoensis/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > phycodurus| 2023-11-29 14:29:02 | > /home/bf/bf-build/phycodurus/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > piculet | 2023-11-30 02:32:57 | > ../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not > at beginning of declaration [-Wold-style-declaration] > pogona| 2023-11-29 14:22:31 | > /home/bf/bf-build/pogona/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > rorqual | 2023-11-30 02:32:41 | > ../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not > at beginning of declaration [-Wold-style-declaration] > serinus | 2023-11-30 02:47:05 | > /home/bf/bf-build/serinus/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > skink | 2023-11-29 14:23:05 | > /home/bf/bf-build/skink-master/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > taipan| 2023-11-30 03:03:52 | > /home/bf/bf-build/taipan/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > tamandua | 2023-11-30 02:49:50 | > /home/bf/bf-build/tamandua/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: > warning: 'inline' is not at beginning of declaration > [-Wold-style-declaration] > > regards, tom lane > Agree. The fix is attached. Regards, Pavel 0001-Fix-warning-due-non-standard-inline-declaration-in-4.patch Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Alexander Lakhin writes: > And a warning: > $ CC=gcc-12 CFLAGS="-Wall -Wextra -Wno-unused-parameter -Wno-sign-compare > -Wno-clobbered > -Wno-missing-field-initializers" ./configure -q && make -s > slru.c:63:1: warning: ‘inline’ is not at beginning of declaration > [-Wold-style-declaration] > 63 | static int inline > | ^~ > Maybe it's worth fixing before committing... This should have been fixed before commit, because there are now a dozen buildfarm animals complaining about it, as well as who-knows- how-many developers' compilers. calliphoridae | 2023-11-30 02:48:59 | /home/bf/bf-build/calliphoridae/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] canebrake | 2023-11-29 14:22:10 | /home/bf/bf-build/canebrake/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] culicidae | 2023-11-30 02:49:06 | /home/bf/bf-build/culicidae/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] desmoxytes| 2023-11-30 03:11:15 | /home/bf/bf-build/desmoxytes/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] flaviventris | 2023-11-30 02:53:19 | /home/bf/bf-build/flaviventris/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] francolin | 2023-11-30 02:26:08 | ../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] grassquit | 2023-11-30 02:58:36 | /home/bf/bf-build/grassquit/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] komodoensis | 2023-11-30 03:07:52 | /home/bf/bf-build/komodoensis/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] phycodurus| 2023-11-29 14:29:02 | /home/bf/bf-build/phycodurus/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] piculet | 2023-11-30 02:32:57 | ../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] pogona| 2023-11-29 14:22:31 | /home/bf/bf-build/pogona/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] rorqual | 2023-11-30 02:32:41 | ../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] serinus | 2023-11-30 02:47:05 | /home/bf/bf-build/serinus/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] skink | 2023-11-29 14:23:05 | /home/bf/bf-build/skink-master/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] taipan| 2023-11-30 03:03:52 | /home/bf/bf-build/taipan/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] tamandua | 2023-11-30 02:49:50 | /home/bf/bf-build/tamandua/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration] regards, tom lane
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Андрей, привет! Текущее положение у меня такое. . *pg_stats and range statisticsTracking statements entry timestamp in pg_stat_statements* Уже закоммичены. *XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)* Если всё будет и замечаний не возникнет ок, завтра утром закоммичу. *Add semi-join pushdown to postgres_fdw* Переработал патч, сделал обработку условий более аккурантно. Хочу попросить ещё 2 часа на финальное ревью и коммит. *May be BUG. Periodic burst growth of the checkpoint_req counter on replica.* Сегодня вечером планирую доделать и выложить review. -- Regards, Alexander Korotkov -- Regards, Alexander Korotkov On Mon, Nov 27, 2023 at 9:00 AM Alexander Lakhin wrote: > Hello Alexander, > > 27.11.2023 02:43, Alexander Korotkov wrote: > > > v61 looks good to me. I'm going to push it as long as there are no > objections. > > > > I've looked at the patch set and found a typo: > occured > > And a warning: > $ CC=gcc-12 CFLAGS="-Wall -Wextra -Wno-unused-parameter -Wno-sign-compare > -Wno-clobbered > -Wno-missing-field-initializers" ./configure -q && make -s > slru.c:63:1: warning: ‘inline’ is not at beginning of declaration > [-Wold-style-declaration] > 63 | static int inline >| ^~ > > Maybe it's worth fixing before committing... > > Best regards, > Alexander >
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hello Alexander, 27.11.2023 02:43, Alexander Korotkov wrote: v61 looks good to me. I'm going to push it as long as there are no objections. I've looked at the patch set and found a typo: occured And a warning: $ CC=gcc-12 CFLAGS="-Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wno-clobbered -Wno-missing-field-initializers" ./configure -q && make -s slru.c:63:1: warning: ‘inline’ is not at beginning of declaration [-Wold-style-declaration] 63 | static int inline | ^~ Maybe it's worth fixing before committing... Best regards, Alexander
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Alexander, Maxim, Thank you for revisions. On Thu, Nov 9, 2023 at 6:22 PM Maxim Orlov wrote: > Aleksander Alekseev, > > > Maxim, > > I see both of us accounted for Alexanders feedback and submitted v59. > > Your newer version seems to have issues on cfbot, so resubmitting the > > previous patchset that passes the tests. Please feel free to add > > changes. > > For unknown reasons, I do not receive any of your emails from after > 2023-11-07 11:57:12 (Message-ID: > CAJ7c6TN1hKqNPGrNcq96SUyD=z61rakgxf8iqq36qr90oud...@mail.gmail.com). > Even after resend. > > Anyway, PFA patch set of version 61. I've made some minor changes in the > 0001 and add 004 in order to test actual 64-bit SLRU pages. > > As for CF bot had failed on my v59 patch set, this is because of the bug. > It's manifested because of added 64-bit pages tests. > The problem was in segno calculation, since we convert it from file name > using strtol call. But strtol return long, > which is 4 byte long in x86. > > - segno = (int) strtol(clde->d_name, NULL, 16); > + segno = strtoi64(clde->d_name, NULL, 16); v61 looks good to me. I'm going to push it as long as there are no objections. -- Regards, Alexander Korotkov
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Aleksander Alekseev, > Maxim, > I see both of us accounted for Alexanders feedback and submitted v59. > Your newer version seems to have issues on cfbot, so resubmitting the > previous patchset that passes the tests. Please feel free to add > changes. For unknown reasons, I do not receive any of your emails from after 2023-11-07 11:57:12 (Message-ID: CAJ7c6TN1hKqNPGrNcq96SUyD= z61rakgxf8iqq36qr90oud...@mail.gmail.com). Even after resend. Anyway, PFA patch set of version 61. I've made some minor changes in the 0001 and add 004 in order to test actual 64-bit SLRU pages. As for CF bot had failed on my v59 patch set, this is because of the bug. It's manifested because of added 64-bit pages tests. The problem was in segno calculation, since we convert it from file name using strtol call. But strtol return long, which is 4 byte long in x86. - segno = (int) strtol(clde->d_name, NULL, 16); + segno = strtoi64(clde->d_name, NULL, 16); -- Best regards, Maxim Orlov. v61-0003-Make-use-FullTransactionId-in-2PC-filenames.patch Description: Binary data v61-0004-Add-SLRU-tests-for-64-bit-page-case.patch Description: Binary data v61-0002-Use-larger-segment-file-names-for-pg_notify.patch Description: Binary data v61-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Maxim,
I see both of us accounted for Alexanders feedback and submitted v59.
Your newer version seems to have issues on cfbot, so resubmitting the
previous patchset that passes the tests. Please feel free to add
changes.
> See SlruCorrectSegmentFilenameLength():
>
> ```
> if (ctl->long_segment_names)
> return (len == 15); /* see SlruFileName() */
> else
> /*
> * Commit 638cf09e76d allowed 5-character lengths. Later commit
> * 73c986adde5 allowed 6-character length.
> *
> * XXX should we still consider such names to be valid?
> */
> return (len == 4 || len == 5 || len == 6);
> ```
>
> Should we just drop it or check that segno is <= 0xFF?
I also choose to change this Assert and to add a corresponding comment:
else
{
- Assert(segno >= 0 && segno <= 0x);
+ /*
+* Despite the fact that %04X format string is used up to 24 bit
+* integers are allowed. See SlruCorrectSegmentFilenameLength()
+*/
+ Assert(segno >= 0 && segno <= 0xFF);
return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
(unsigned int) segno);
}
--
Best regards,
Aleksander Alekseev
v60-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch
Description: Binary data
v60-0003-Make-use-FullTransactionId-in-2PC-filenames.patch
Description: Binary data
v60-0002-Use-larger-segment-file-names-for-pg_notify.patch
Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, 6 Nov 2023 at 16:07, Alexander Korotkov
wrote:
> Hi!
>
> On Wed, Jul 5, 2023 at 4:46 PM Aleksander Alekseev <
> aleksan...@timescale.com> wrote:
> > PFE the corrected patchset v58.
>
> I'd like to revive this thread.
>
Hi! Great news!
> BTW, there is a typo in a word "exceeed".
>
Fixed.
>
> +static int inline
> +SlruFileName(SlruCtl ctl, char *path, int64 segno)
> +{
> ...
> +}
>
> I think it worth adding asserts here to verify there is no overflow making
> us mapping different segments into the same files.
>
Agree, assertion added.
> + return occupied == max_notify_queue_pages;
>
> I'm not sure if the current code could actually allow to occupy more than
> max_notify_queue_pages. Probably not even in extreme cases. But I still
> think it will more safe and easier to read to write "occupied >=
> max_notify_queue"_pages here.
>
Fixed.
> diff --git a/src/test/modules/test_slru/test_slru.c
> b/src/test/modules/test_slru/test_slru.c
>
> The actual 64-bitness of SLRU pages isn't much exercised in our automated
> tests. It would be too exhausting to make pg_notify actually use higher
> than 2**32 page numbers. Thus, I think test/modules/test_slru is a good
> place to give high page numbers a good test.
>
PFA, I've add test for a 64-bit SLRU pages.
By the way, there is another one useful thing we may do here. For now
pg_commit_ts functionality is rather strange: if it was enabled, then
disabled and then enabled again all the data from before will be
discarded. Meanwhile, users expected to have their commit timestamps for
all transactions, which were "logged" when this feature was enabled. It's
weird.
AFICS, the only reason for this behaviour is becouse of transaction
wraparound. It may occur while the feature is disabled end it is safe to
simply remove all the data from previous period. If we switch to
FullTransactionId in commit_ts we can overcome this limitation. But I'm
not sure if it worth to try to fix this in current patchset, since it is
already non trivial.
--
Best regards,
Maxim Orlov.
v59-0003-Make-use-FullTransactionId-in-2PC-filenames.patch
Description: Binary data
v59-0004-Add-SLRU-tests-for-64-bit-page-case.patch
Description: Binary data
v59-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch
Description: Binary data
v59-0002-Use-larger-segment-file-names-for-pg_notify.patch
Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi again,
> PFA the corrected patchset v59.
On second thought, I believe this Assert is incorrect:
```
+else
+{
+Assert(segno >= 0 && segno <= 0x);
+return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
+(unsigned int) segno);
+}
```
See SlruCorrectSegmentFilenameLength():
```
if (ctl->long_segment_names)
return (len == 15); /* see SlruFileName() */
else
/*
* Commit 638cf09e76d allowed 5-character lengths. Later commit
* 73c986adde5 allowed 6-character length.
*
* XXX should we still consider such names to be valid?
*/
return (len == 4 || len == 5 || len == 6);
```
Should we just drop it or check that segno is <= 0xFF?
--
Best regards,
Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi Alexander,
> -#define TransactionIdToCTsPage(xid) \
> - ((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
> +
> +/*
> + * Although we return an int64 the actual value can't currently exceeed
> 2**32.
> + */
> +static inline int64
> +TransactionIdToCTsPage(TransactionId xid)
> +{
> + return xid / (int64) COMMIT_TS_XACTS_PER_PAGE;
> +}
>
> Is there any reason we transform macro into a function? If not, I propose to
> leave this as a macro. BTW, there is a typo in a word "exceeed".
I kept the inline function, as we agreed above.
Typo fixed.
> +static int inline
> +SlruFileName(SlruCtl ctl, char *path, int64 segno)
> +{
> + if (ctl->long_segment_names)
> + /*
> +* We could use 16 characters here but the disadvantage would be that
> +* the SLRU segments will be hard to distinguish from WAL segments.
> +*
> +* For this reason we use 15 characters. It is enough but also means
> +* that in the future we can't decrease SLRU_PAGES_PER_SEGMENT easily.
> +*/
> + return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
> + (long long) segno);
> + else
> + return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
> + (unsigned int) segno);
> +}
>
> I think it worth adding asserts here to verify there is no overflow making us
> mapping different segments into the same files.
Added. I noticed a off-by-one error in the code snippet proposed
above, so my code differs a bit.
> + return occupied == max_notify_queue_pages;
>
> I'm not sure if the current code could actually allow to occupy more than
> max_notify_queue_pages. Probably not even in extreme cases. But I still
> think it will more safe and easier to read to write "occupied >=
> max_notify_queue"_pages here.
Fixed.
> diff --git a/src/test/modules/test_slru/test_slru.c
> b/src/test/modules/test_slru/test_slru.c
>
> The actual 64-bitness of SLRU pages isn't much exercised in our automated
> tests. It would be too exhausting to make pg_notify actually use higher than
> 2**32 page numbers. Thus, I think test/modules/test_slru is a good place to
> give high page numbers a good test.
Fixed. I choose not to change any numbers in the test in order to
check any corner cases, etc. The code patches for long_segment_names =
true and long_segment_names = false are almost the same thus it will
not improve code coverage. Using the current numbers will allow to
easily switch back to long_segment_names = false in the test if
necessary.
PFA the corrected patchset v59.
--
Best regards,
Aleksander Alekseev
v59-0003-Make-use-FullTransactionId-in-2PC-filenames.patch
Description: Binary data
v59-0002-Use-larger-segment-file-names-for-pg_notify.patch
Description: Binary data
v59-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch
Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, Nov 6, 2023 at 4:38 PM Aleksander Alekseev
wrote:
> > > PFE the corrected patchset v58.
> >
> > I'd like to revive this thread.
>
> Many thanks for your comments and suggestions.
>
> > I think it worth adding asserts here to verify there is no overflow
making us mapping different segments into the same files.
>
> Sorry, I didn't understand this one. Maybe you could provide the exact
code?
I actually meant this.
static int inline
SlruFileName(SlruCtl ctl, char *path, int64 segno)
{
if (ctl->long_segment_names)
{
/*
* We could use 16 characters here but the disadvantage would be
that
* the SLRU segments will be hard to distinguish from WAL segments.
*
* For this reason we use 15 characters. It is enough but also means
* that in the future we can't decrease SLRU_PAGES_PER_SEGMENT
easily.
*/
Assert(segno >= 0 && segno <= 0x1000);
return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
(long long) segno);
}
else
{
Assert(segno >= 0 && segno <= 0x1);
return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
(unsigned int) segno);
}
}
As I now get, snprintf() wouldn't just truncate the high signs, instead it
will use more characters. But I think assertions are useful anyway.
--
Regards,
Alexander Korotkov
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Alexander, > > PFE the corrected patchset v58. > > I'd like to revive this thread. Many thanks for your comments and suggestions. > I think it worth adding asserts here to verify there is no overflow making us > mapping different segments into the same files. Sorry, I didn't understand this one. Maybe you could provide the exact code? -- Best regards, Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, > > > If I remember right, the compiler will make equivalent code from > > > inline functions and macros, and functions has an additional benefit: > > > the compiler will report type mismatch if any. That was the only > > > reason. > > > > Then it's OK to leave it as an inline function. +1 > > BTW, I'm a bit puzzled on who should be the first author now? > Thanks! It's long, I agree, but the activity around this was huge and > involved many people, the patch itself already has more than > half-hundred iterations to date. I think at least people mentioned in > the commit message of v58 are fair to have author status. > > As for the first, I'm not sure, it's hard for me to evaluate what is > more important, the initial prototype, or the final improvement > iterations. I don't think the existing order in a commit message has > some meaning. Maybe it's worth throwing a dice. Personally I was not aware that the order of the authors in a commit message carries any information. To my knowledge it doesn't not. I believe this patchset is a team effort, so I suggest keeping the commit message as is or shuffling the authors randomly. I believe we should do our best to reflect the input of people who previously contributed to the effort, if anyone are aware of them, and add them to the commit message if they are not there yet. Pretty sure Git will forgive us if the list ends up being long. Hopefully so will people who we mistakenly forget. -- Best regards, Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, 6 Nov 2023 at 18:01, Alexander Korotkov wrote:
>
> On Mon, Nov 6, 2023 at 3:42 PM Pavel Borisov wrote:
> >
> > On Mon, 6 Nov 2023 at 17:07, Alexander Korotkov
> > wrote:
> > > On Wed, Jul 5, 2023 at 4:46 PM Aleksander Alekseev
> > > wrote:
> > > > PFE the corrected patchset v58.
> > >
> > > I'd like to revive this thread.
> > >
> > > This patchset is extracted from a larger patchset implementing 64-bit
> > > xids. It converts page numbers in SLRUs into 64 bits. The most SLRUs
> > > save the same file naming scheme, thus their on-disk representation
> > > remains the same. However, the patch 0002 converts pg_notify to actually
> > > use a new naming scheme. Therefore pg_notify can benefit from
> > > simplification and getting rid of wraparound.
> > >
> > > -#define TransactionIdToCTsPage(xid) \
> > > - ((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
> > > +
> > > +/*
> > > + * Although we return an int64 the actual value can't currently exceeed
> > > 2**32.
> > > + */
> > > +static inline int64
> > > +TransactionIdToCTsPage(TransactionId xid)
> > > +{
> > > + return xid / (int64) COMMIT_TS_XACTS_PER_PAGE;
> > > +}
> > >
> > > Is there any reason we transform macro into a function? If not, I
> > > propose to leave this as a macro. BTW, there is a typo in a word
> > > "exceeed".
> > If I remember right, the compiler will make equivalent code from
> > inline functions and macros, and functions has an additional benefit:
> > the compiler will report type mismatch if any. That was the only
> > reason.
>
> Then it's OK to leave it as an inline function.
>
> > Also, I looked at v58-0001 and don't quite agree with mentioning the
> > authors of the original 64-xid patch, from which the current patch is
> > derived, as just "privious input" persons.
>
> +1, for converting all "previous input" persons as additional authors.
> That would be a pretty long list of authors though.
> BTW, I'm a bit puzzled on who should be the first author now?
Thanks! It's long, I agree, but the activity around this was huge and
involved many people, the patch itself already has more than
half-hundred iterations to date. I think at least people mentioned in
the commit message of v58 are fair to have author status.
As for the first, I'm not sure, it's hard for me to evaluate what is
more important, the initial prototype, or the final improvement
iterations. I don't think the existing order in a commit message has
some meaning. Maybe it's worth throwing a dice.
Regards, Pavel
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, Nov 6, 2023 at 3:42 PM Pavel Borisov wrote:
>
> On Mon, 6 Nov 2023 at 17:07, Alexander Korotkov wrote:
> > On Wed, Jul 5, 2023 at 4:46 PM Aleksander Alekseev
> > wrote:
> > > PFE the corrected patchset v58.
> >
> > I'd like to revive this thread.
> >
> > This patchset is extracted from a larger patchset implementing 64-bit xids.
> > It converts page numbers in SLRUs into 64 bits. The most SLRUs save the
> > same file naming scheme, thus their on-disk representation remains the
> > same. However, the patch 0002 converts pg_notify to actually use a new
> > naming scheme. Therefore pg_notify can benefit from simplification and
> > getting rid of wraparound.
> >
> > -#define TransactionIdToCTsPage(xid) \
> > - ((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
> > +
> > +/*
> > + * Although we return an int64 the actual value can't currently exceeed
> > 2**32.
> > + */
> > +static inline int64
> > +TransactionIdToCTsPage(TransactionId xid)
> > +{
> > + return xid / (int64) COMMIT_TS_XACTS_PER_PAGE;
> > +}
> >
> > Is there any reason we transform macro into a function? If not, I propose
> > to leave this as a macro. BTW, there is a typo in a word "exceeed".
> If I remember right, the compiler will make equivalent code from
> inline functions and macros, and functions has an additional benefit:
> the compiler will report type mismatch if any. That was the only
> reason.
Then it's OK to leave it as an inline function.
> Also, I looked at v58-0001 and don't quite agree with mentioning the
> authors of the original 64-xid patch, from which the current patch is
> derived, as just "privious input" persons.
+1, for converting all "previous input" persons as additional authors.
That would be a pretty long list of authors though.
BTW, I'm a bit puzzled on who should be the first author now?
--
Regards,
Alexander Korotkov
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, 6 Nov 2023 at 17:07, Alexander Korotkov wrote:
>
> Hi!
>
> On Wed, Jul 5, 2023 at 4:46 PM Aleksander Alekseev
> wrote:
> > PFE the corrected patchset v58.
>
> I'd like to revive this thread.
>
> This patchset is extracted from a larger patchset implementing 64-bit xids.
> It converts page numbers in SLRUs into 64 bits. The most SLRUs save the same
> file naming scheme, thus their on-disk representation remains the same.
> However, the patch 0002 converts pg_notify to actually use a new naming
> scheme. Therefore pg_notify can benefit from simplification and getting rid
> of wraparound.
>
> -#define TransactionIdToCTsPage(xid) \
> - ((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
> +
> +/*
> + * Although we return an int64 the actual value can't currently exceeed
> 2**32.
> + */
> +static inline int64
> +TransactionIdToCTsPage(TransactionId xid)
> +{
> + return xid / (int64) COMMIT_TS_XACTS_PER_PAGE;
> +}
>
> Is there any reason we transform macro into a function? If not, I propose to
> leave this as a macro. BTW, there is a typo in a word "exceeed".
If I remember right, the compiler will make equivalent code from
inline functions and macros, and functions has an additional benefit:
the compiler will report type mismatch if any. That was the only
reason.
Also, I looked at v58-0001 and don't quite agree with mentioning the
authors of the original 64-xid patch, from which the current patch is
derived, as just "privious input" persons.
Regards, Pavel Borisov
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi!
On Wed, Jul 5, 2023 at 4:46 PM Aleksander Alekseev
wrote:
> PFE the corrected patchset v58.
I'd like to revive this thread.
This patchset is extracted from a larger patchset implementing 64-bit
xids. It converts page numbers in SLRUs into 64 bits. The most SLRUs save
the same file naming scheme, thus their on-disk representation remains the
same. However, the patch 0002 converts pg_notify to actually use a new
naming scheme. Therefore pg_notify can benefit from simplification and
getting rid of wraparound.
-#define TransactionIdToCTsPage(xid) \
- ((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
+
+/*
+ * Although we return an int64 the actual value can't currently exceeed
2**32.
+ */
+static inline int64
+TransactionIdToCTsPage(TransactionId xid)
+{
+ return xid / (int64) COMMIT_TS_XACTS_PER_PAGE;
+}
Is there any reason we transform macro into a function? If not, I propose
to leave this as a macro. BTW, there is a typo in a word "exceeed".
+static int inline
+SlruFileName(SlruCtl ctl, char *path, int64 segno)
+{
+ if (ctl->long_segment_names)
+ /*
+* We could use 16 characters here but the disadvantage would be
that
+* the SLRU segments will be hard to distinguish from WAL segments.
+*
+* For this reason we use 15 characters. It is enough but also means
+* that in the future we can't decrease SLRU_PAGES_PER_SEGMENT
easily.
+*/
+ return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
+ (long long) segno);
+ else
+ return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
+ (unsigned int) segno);
+}
I think it worth adding asserts here to verify there is no overflow making
us mapping different segments into the same files.
+ return occupied == max_notify_queue_pages;
I'm not sure if the current code could actually allow to occupy more than
max_notify_queue_pages. Probably not even in extreme cases. But I still
think it will more safe and easier to read to write "occupied >=
max_notify_queue"_pages here.
diff --git a/src/test/modules/test_slru/test_slru.c
b/src/test/modules/test_slru/test_slru.c
The actual 64-bitness of SLRU pages isn't much exercised in our automated
tests. It would be too exhausting to make pg_notify actually use higher
than 2**32 page numbers. Thus, I think test/modules/test_slru is a good
place to give high page numbers a good test.
--
Regards,
Alexander Korotkov
回复: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
> > This patch makes the filenames always 12 characters wide (for SLRUs that
> > opt-in to the new naming). That's actually not enough for the full range
> > that a 64 bit page number can represent. Should we make it 16 characters
> > now, to avoid repeating the same mistake we made earlier? Or make it
> > more configurable, on a per-SLRU basis. One reason I don't want to just
> > make it 16 characters is that WAL segment filenames are also 16 hex
> > characters, which could cause confusion.
>
> Good catch. I propose the following solution:
>
> ```
> SlruFileName(SlruCtl ctl, char *path, int64 segno)
> {
> if (ctl->long_segment_names)
> -return snprintf(path, MAXPGPATH, "%s/%012llX", ctl->Dir,
> +/*
> + * We could use 16 characters here but the disadvantage would be that
> + * the SLRU segments will be hard to distinguish from WAL segments.
> + *
> + * For this reason we use 15 characters. It is enough but also means
> + * that in the future we can't decrease SLRU_PAGES_PER_SEGMENT
> easily.
> + */
> +return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
> (long long) segno);
> else
> return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
> ```
>
> PFE the corrected patchset v58.
Good idea
发件人: Aleksander Alekseev
发送时间: 2023年9月4日 22:41
收件人: Postgres hackers
抄送: Heikki Linnakangas ; Maxim Orlov ;
Jacob Champion ; Japin Li ;
Andres Freund ; Michael Paquier ;
Pavel Borisov ; Peter Eisentraut
; Alexander Korotkov
主题: Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into
PostgreSQL 15)
Hi,
> > Reviewing this now. I think it's almost ready to be committed.
> >
> > There's another big effort going on to move SLRUs to the regular buffer
> > cache (https://commitfest.postgresql.org/43/3514/). I wonder how moving
> > to 64 bit page numbers will affect that. BlockNumber is still 32 bits,
> > after all.
>
> Somehow I didn't pay too much attention to this effort, thanks. I will
> familiarize myself with the patch. Intuitively I don't think that the
> patchse should block each other.
>
> > This patch makes the filenames always 12 characters wide (for SLRUs that
> > opt-in to the new naming). That's actually not enough for the full range
> > that a 64 bit page number can represent. Should we make it 16 characters
> > now, to avoid repeating the same mistake we made earlier? Or make it
> > more configurable, on a per-SLRU basis. One reason I don't want to just
> > make it 16 characters is that WAL segment filenames are also 16 hex
> > characters, which could cause confusion.
>
> Good catch. I propose the following solution:
>
> ```
> SlruFileName(SlruCtl ctl, char *path, int64 segno)
> {
> if (ctl->long_segment_names)
> -return snprintf(path, MAXPGPATH, "%s/%012llX", ctl->Dir,
> +/*
> + * We could use 16 characters here but the disadvantage would be that
> + * the SLRU segments will be hard to distinguish from WAL segments.
> + *
> + * For this reason we use 15 characters. It is enough but also means
> + * that in the future we can't decrease SLRU_PAGES_PER_SEGMENT
> easily.
> + */
> +return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
> (long long) segno);
> else
> return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
> ```
>
> PFE the corrected patchset v58.
While triaging the patches for the September CF [1] a consensus was
reached that the patchset needs another round of review. Also I
removed myself from the list of reviewers in order to make it clear
that a review from somebody else would be appreciated.
[1]: https://postgr.es/m/0737f444-59bb-ac1d-2753-873c40da0840%40eisentraut.org
--
Best regards,
Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi,
> > Reviewing this now. I think it's almost ready to be committed.
> >
> > There's another big effort going on to move SLRUs to the regular buffer
> > cache (https://commitfest.postgresql.org/43/3514/). I wonder how moving
> > to 64 bit page numbers will affect that. BlockNumber is still 32 bits,
> > after all.
>
> Somehow I didn't pay too much attention to this effort, thanks. I will
> familiarize myself with the patch. Intuitively I don't think that the
> patchse should block each other.
>
> > This patch makes the filenames always 12 characters wide (for SLRUs that
> > opt-in to the new naming). That's actually not enough for the full range
> > that a 64 bit page number can represent. Should we make it 16 characters
> > now, to avoid repeating the same mistake we made earlier? Or make it
> > more configurable, on a per-SLRU basis. One reason I don't want to just
> > make it 16 characters is that WAL segment filenames are also 16 hex
> > characters, which could cause confusion.
>
> Good catch. I propose the following solution:
>
> ```
> SlruFileName(SlruCtl ctl, char *path, int64 segno)
> {
> if (ctl->long_segment_names)
> -return snprintf(path, MAXPGPATH, "%s/%012llX", ctl->Dir,
> +/*
> + * We could use 16 characters here but the disadvantage would be that
> + * the SLRU segments will be hard to distinguish from WAL segments.
> + *
> + * For this reason we use 15 characters. It is enough but also means
> + * that in the future we can't decrease SLRU_PAGES_PER_SEGMENT
> easily.
> + */
> +return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
> (long long) segno);
> else
> return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
> ```
>
> PFE the corrected patchset v58.
While triaging the patches for the September CF [1] a consensus was
reached that the patchset needs another round of review. Also I
removed myself from the list of reviewers in order to make it clear
that a review from somebody else would be appreciated.
[1]: https://postgr.es/m/0737f444-59bb-ac1d-2753-873c40da0840%40eisentraut.org
--
Best regards,
Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi,
> Reviewing this now. I think it's almost ready to be committed.
>
> There's another big effort going on to move SLRUs to the regular buffer
> cache (https://commitfest.postgresql.org/43/3514/). I wonder how moving
> to 64 bit page numbers will affect that. BlockNumber is still 32 bits,
> after all.
Somehow I didn't pay too much attention to this effort, thanks. I will
familiarize myself with the patch. Intuitively I don't think that the
patchse should block each other.
> This patch makes the filenames always 12 characters wide (for SLRUs that
> opt-in to the new naming). That's actually not enough for the full range
> that a 64 bit page number can represent. Should we make it 16 characters
> now, to avoid repeating the same mistake we made earlier? Or make it
> more configurable, on a per-SLRU basis. One reason I don't want to just
> make it 16 characters is that WAL segment filenames are also 16 hex
> characters, which could cause confusion.
Good catch. I propose the following solution:
```
SlruFileName(SlruCtl ctl, char *path, int64 segno)
{
if (ctl->long_segment_names)
-return snprintf(path, MAXPGPATH, "%s/%012llX", ctl->Dir,
+/*
+ * We could use 16 characters here but the disadvantage would be that
+ * the SLRU segments will be hard to distinguish from WAL segments.
+ *
+ * For this reason we use 15 characters. It is enough but also means
+ * that in the future we can't decrease SLRU_PAGES_PER_SEGMENT easily.
+ */
+return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
(long long) segno);
else
return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
```
PFE the corrected patchset v58.
--
Best regards,
Aleksander Alekseev
v58-0003-Make-use-FullTransactionId-in-2PC-filenames.patch
Description: Binary data
v58-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch
Description: Binary data
v58-0002-Use-larger-segment-file-names-for-pg_notify.patch
Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On 09/03/2023 17:21, Aleksander Alekseev wrote:
v57-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch
Reviewing this now. I think it's almost ready to be committed.
There's another big effort going on to move SLRUs to the regular buffer
cache (https://commitfest.postgresql.org/43/3514/). I wonder how moving
to 64 bit page numbers will affect that. BlockNumber is still 32 bits,
after all.
+/*
+ * An internal function used by SlruScanDirectory().
+ *
+ * Returns true if a file with a name of a given length may be a correct
+ * SLRU segment.
+ */
+static inline bool
+SlruCorrectSegmentFilenameLength(SlruCtl ctl, size_t len)
+{
+ if (ctl->long_segment_names)
+ return (len == 12);
+ else
+
+ /*
+* XXX Should we still consider 5 and 6 to be a correct length?
It
+* looks like it was previously allowed but now SlruFileName()
can't
+* return such a name.
+*/
+ return (len == 4 || len == 5 || len == 6);
+}
Huh, I didn't realize that 5 and 6 character SLRU segment names are
possible. But indeed they are. Commit 638cf09e76d allowed 5-character
lengths:
commit 638cf09e76d70dd83d8123e7079be6c0532102d2
Author: Alvaro Herrera
Date: Thu Jan 2 18:17:29 2014 -0300
Handle 5-char filenames in SlruScanDirectory
Original users of slru.c were all producing 4-digit filenames, so that
was all that that code was prepared to handle. Changes to multixact.c
in the course of commit 0ac5ad5134f made pg_multixact/members create
5-digit filenames once a certain threshold was reached, which
SlruScanDirectory wasn't prepared to deal with; in particular,
5-digit-name files were not removed during truncation. Change that
routine to make it aware of those files, and have it process them just
like any others.
Right now, some pg_multixact/members directories will contain a mixture
of 4-char and 5-char filenames. A future commit is expected fix things
so that each slru.c user declares the correct maximum width for the
files it produces, to avoid such unsightly mixtures.
Noticed while investigating bug #8673 reported by Serge Negodyuck.
And later commit 73c986adde5, which introduced commit_ts, allowed 6
characters. With 8192 block size, pg_commit_ts segments are indeed 5
chars wide, and with 512 block size, 6 chars are needed.
This patch makes the filenames always 12 characters wide (for SLRUs that
opt-in to the new naming). That's actually not enough for the full range
that a 64 bit page number can represent. Should we make it 16 characters
now, to avoid repeating the same mistake we made earlier? Or make it
more configurable, on a per-SLRU basis. One reason I don't want to just
make it 16 characters is that WAL segment filenames are also 16 hex
characters, which could cause confusion.
--
Heikki Linnakangas
Neon (https://neon.tech)
Re: Add 64-bit XIDs into PostgreSQL 15
Hi, > This patch hasn't applied in quite some time, and the thread has moved to > discussing higher lever items rather than the suggested patch, so I'm closing > this as Returned with Feedback. Please feel free to resubmit when there is > renewed interest and a concensus on how/what to proceed with. Yes, this thread awaits several other patches to be merged [1] in order to continue, so it makes sense to mark it as RwF for the time being. Thanks! [1]: https://commitfest.postgresql.org/43/3489/ -- Best regards, Aleksander Alekseev
Re: Add 64-bit XIDs into PostgreSQL 15
This patch hasn't applied in quite some time, and the thread has moved to discussing higher lever items rather than the suggested patch, so I'm closing this as Returned with Feedback. Please feel free to resubmit when there is renewed interest and a concensus on how/what to proceed with. -- Daniel Gustafsson
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, > As suggested before by Heikki Linnakangas, I've added a patch for making 2PC > transaction state 64-bit. > At first, my intention was to rebuild all twophase interface to use > FullTransactionId. But doing this in a proper > manner would lead to switching from TransactionId to FullTransactionId in > PGPROC and patch become too > big to handle here. > > So I decided to keep it simple for now and use wrap logic trick and calc > FullTransactionId on current epoch, > since the span of active xids cannot exceed one epoch at any given time. > > Patches 1 and 2 are the same as above. Thanks! 0003 LGTM. I'll mark the CF entry as RfC. > I propose that we try to finish 1 and 2 for v16. And maybe 6. I think > that's doable. It doesn't have any great user-visible benefits yet, but > we need to start somewhere. +1. However there are only a few days left if we are going to do this within March CF... -- Best regards, Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi! As suggested before by Heikki Linnakangas, I've added a patch for making 2PC transaction state 64-bit. At first, my intention was to rebuild all twophase interface to use FullTransactionId. But doing this in a proper manner would lead to switching from TransactionId to FullTransactionId in PGPROC and patch become too big to handle here. So I decided to keep it simple for now and use wrap logic trick and calc FullTransactionId on current epoch, since the span of active xids cannot exceed one epoch at any given time. Patches 1 and 2 are the same as above. -- Best regards, Maxim Orlov. v57-0002-Use-larger-segment-file-names-for-pg_notify.patch Description: Binary data v57-0003-Make-use-FullTransactionId-in-2PC-filenames.patch Description: Binary data v57-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, > 1. Use internal 64 bit page numbers in SLRUs without changing segments naming. > 2. Use the larger segment file names in async.c, to lift the current 8 > GB limit on the max number of pending notifications. > [...] > > Where our main focus for PG16 is going to be 1 and 2, and we may try > to deliver 6 and 7 too if time will permit. > > Maxim and I agreed (offlist) that I'm going to submit 1 and 2. The > patch 1 is ready, please see the attachment. I'm currently working on > 2 and going to submit it in a bit. It seems to be relatively > straightforward but I don't want to rush things and want to make sure > I didn't miss something. PFA the patch v57 which now includes both 1 and 2. -- Best regards, Aleksander Alekseev v57-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch Description: Binary data v57-0002-Use-larger-segment-file-names-for-pg_notify.patch Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Tue, 7 Mar 2023 at 15:38, Heikki Linnakangas wrote: > > That is true for pg_multixact/offsets. We will indeed need to add an > epoch and introduce the concept of FullMultiXactIds for that. However, > we can change pg_multixact/members independently of that. We can extend > MultiXactOffset from 32 to 64 bits, and eliminate pg_multixact/members > wraparound, while keeping multi-xids 32 bits wide. > > Yes, you're totally correct. If it will be committable that way, I'm all for that. -- Best regards, Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, > Here's how I see the development path for this [...] > So, in my view, the plan should be [...] > Thoughts? The plan looks great! I would also explicitly include this: > As we start to refactor these things, I also think it would be good to > have more explicit tracking of the valid range of SLRU pages in each > SLRU. Take pg_subtrans for example: it's not very clear what pages have > been initialized, especially during different stages of startup. It > would be good to have clear start and end page numbers, and throw an > error if you try to look up anything outside those bounds. Same for all > other SLRUs. So the plan becomes: 1. Use internal 64 bit page numbers in SLRUs without changing segments naming. 2. Use the larger segment file names in async.c, to lift the current 8 GB limit on the max number of pending notifications. 3. Extend pg_xact to 64-bits. 4. Extend pg_subtrans to 64-bits. 5. Extend pg_multixact so that pg_multixact/members is addressed by 64-bit offsets. 6. Extend pg_twophase.c, to use FullTransactionIds. (a bonus thing) 7. More explicit tracking of the valid range of SLRU pages in each SLRU Where our main focus for PG16 is going to be 1 and 2, and we may try to deliver 6 and 7 too if time will permit. Maxim and I agreed (offlist) that I'm going to submit 1 and 2. The patch 1 is ready, please see the attachment. I'm currently working on 2 and going to submit it in a bit. It seems to be relatively straightforward but I don't want to rush things and want to make sure I didn't miss something. > I wonder if we should actually add an artificial limit, as a GUC. Yes, I think we need some sort of limit. Using a GUC would be the most straightforward approach. Alternatively we could derive the limit from the existing GUCs, similarly to how we derive the default value of wal_buffers from shared_buffers [1]. However, off the top of my head we only have max_wal_size and it doesn't strike me as a good candidate for deriving something for NOTIFY / LISTEN. So I'm going to add max_notify_segments GUC with the default value of 65536 as it is now. If the new GUC will receive a push back from the community we can always use a hard-coded value instead, or no limit at all. [1]: https://www.postgresql.org/docs/current/runtime-config-wal.html#GUC-WAL-BUFFERS -- Best regards, Aleksander Alekseev v56-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On 07/03/2023 13:38, Maxim Orlov wrote: As for making pg_multixact 64 bit, I spend the last couple of days to make proper pg_upgrade for pg_multixact's and for pg_xact's with wraparound and I've understood, that it is not a simple task compare to pg_xact's. The problem is, we do not have epoch for multixacts, so we do not have ability to "overcome" wraparound. The solution may be adding some kind of epoch for multixacts or make them 64 bit in "main" 64-xid patch, but in perspective of this thread, in my view, this should be last in line here. That is true for pg_multixact/offsets. We will indeed need to add an epoch and introduce the concept of FullMultiXactIds for that. However, we can change pg_multixact/members independently of that. We can extend MultiXactOffset from 32 to 64 bits, and eliminate pg_multixact/members wraparound, while keeping multi-xids 32 bits wide. - Heikki
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Tue, 17 Jan 2023 at 16:33, Aleksander Alekseev wrote: > Hi hackers, > > Maxim, perhaps you could share with us what your reasoning was here? > > I'm really sorry for late response, but better late than never. Yes, we can not access shared memory without lock. In this particular case, we use XidGenLock. That is why we use lock argument to take it is it was not taken previously. Actually, we may place assertion in this insist. As for xid compare: we do not compare xids here, we are checking for wraparound, so, AFAICS, this code is correct. On Mon, 6 Mar 2023 at 22:48, Heikki Linnakangas wrote: > > 1. Use 64 bit page numbers in SLRUs (this patch) > > 2. Use the larger segment file names in async.c, to lift the current 8 > GB limit on the max number of pending notifications. > > 3. Extend pg_multixact so that pg_multixact/members is addressed by > 64-bit offsets. > > 4. Extend pg_subtrans to 64-bits. > > 5. Extend pg_xact to 64-bits. > > > 6. (a bonus thing that I noticed while thinking of pg_xact.) Extend > pg_twophase.c, to use FullTransactionIds. > > Currently, the twophase state files in pg_twophase are named according > to the 32 bit Xid of the transaction. Let's switch to FullTransactionId > there. > > ... > > I propose that we try to finish 1 and 2 for v16. And maybe 6. I think > that's doable. It doesn't have any great user-visible benefits yet, but > we need to start somewhere. > > - Heikki > > Yes, this is a great idea! My only concern here is that we're going in circles here. You see, patch 1 is what was proposed in the beginning of this thread. Anyway, I will be happy if we are being able to push this topic forward. As for making pg_multixact 64 bit, I spend the last couple of days to make proper pg_upgrade for pg_multixact's and for pg_xact's with wraparound and I've understood, that it is not a simple task compare to pg_xact's. The problem is, we do not have epoch for multixacts, so we do not have ability to "overcome" wraparound. The solution may be adding some kind of epoch for multixacts or make them 64 bit in "main" 64-xid patch, but in perspective of this thread, in my view, this should be last in line here. In pg_xact we do not have such a problem, we do have epoch for transacions, so conversion should be pretty obvious: -> 0001 -> 0001 ... 0FFE -> 0FFE 0FFF -> 0FFF -> 0001 0001 -> 00010001 So, in my view, the plan should be: 1. Use internal 64 bit page numbers in SLRUs without changing segments naming. 2. Use the larger segment file names in async.c, to lift the current 8 GB limit on the max number of pending notifications. 3. Extend pg_xact to 64-bits. 4. Extend pg_subtrans to 64-bits. 5. Extend pg_multixact so that pg_multixact/members is addressed by 64-bit offsets. 6. Extend pg_twophase.c, to use FullTransactionIds. (a bonus thing) Thoughts? -- Best regards, Maxim Orlov. On Mon, 6 Mar 2023 at 22:48, Heikki Linnakangas wrote: > On 01/03/2023 12:21, Aleksander Alekseev wrote: > > Hi, > > > >> I'm surprised that these patches extend the page numbering to 64 bits, > >> but never actually uses the high bits. The XID "epoch" is not used, and > >> pg_xact still wraps around and the segment names are still reused. I > >> thought we could stop doing that. > > > > To clarify, the idea is to let CLOG grow indefinitely and simply store > > FullTransactionId -> TransactionStatus (two bits). Correct? > > Correct. > > > I didn't investigate this in much detail but it may affect quite some > > amount of code since TransactionIdDidCommit() and > > TransactionIdDidCommit() currently both deal with TransactionId, not > > FullTransactionId. IMO, this would be a nice change however, assuming > > we are ready for it. > > Yep, it's a lot of code churn.. > > > In the previous version of the patch there was an attempt to derive > > FullTransactionId from TransactionId but it was wrong for the reasons > > named above in the thread. Thus is was removed and the patch > > simplified. > > Yeah, it's tricky to get it right. Clearly we need to do it at some > point though. > > All in all, this is a big effort. I spent some more time reviewing this > in the last few days, and thought a lot about what the path forward here > could be. And I haven't looked at the actual 64-bit XIDs patch set yet, > just this patch to use 64-bit addressing in SLRUs. > > This patch is the first step, but we have a bit of a chicken and egg > problem, because this patch on its own isn't very interesting, but on > the other hand, we need it to work on the follow up items. Here's how I > see the development path for this (and again, this is just for the > 64-bit SLRUs work, not the bigger 64-bit-XIDs-in-heapam effort): > > 1. Use 64 bit page numbers in SLRUs (this patch) > > I would like to make one change here: I'd prefer to keep the old 4-digit > segment names, until we actually start to use the wider address space. > Let's allow each SLRU to specify how many dig
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On 01/03/2023 12:21, Aleksander Alekseev wrote: Hi, I'm surprised that these patches extend the page numbering to 64 bits, but never actually uses the high bits. The XID "epoch" is not used, and pg_xact still wraps around and the segment names are still reused. I thought we could stop doing that. To clarify, the idea is to let CLOG grow indefinitely and simply store FullTransactionId -> TransactionStatus (two bits). Correct? Correct. I didn't investigate this in much detail but it may affect quite some amount of code since TransactionIdDidCommit() and TransactionIdDidCommit() currently both deal with TransactionId, not FullTransactionId. IMO, this would be a nice change however, assuming we are ready for it. Yep, it's a lot of code churn.. In the previous version of the patch there was an attempt to derive FullTransactionId from TransactionId but it was wrong for the reasons named above in the thread. Thus is was removed and the patch simplified. Yeah, it's tricky to get it right. Clearly we need to do it at some point though. All in all, this is a big effort. I spent some more time reviewing this in the last few days, and thought a lot about what the path forward here could be. And I haven't looked at the actual 64-bit XIDs patch set yet, just this patch to use 64-bit addressing in SLRUs. This patch is the first step, but we have a bit of a chicken and egg problem, because this patch on its own isn't very interesting, but on the other hand, we need it to work on the follow up items. Here's how I see the development path for this (and again, this is just for the 64-bit SLRUs work, not the bigger 64-bit-XIDs-in-heapam effort): 1. Use 64 bit page numbers in SLRUs (this patch) I would like to make one change here: I'd prefer to keep the old 4-digit segment names, until we actually start to use the wider address space. Let's allow each SLRU to specify how many digits to use in the filenames, so that we convert one SLRU at a time. If we do that, and don't change any of the existing SLRUs to actually use the wider space of page and segment numbers yet, this patch becomes just refactoring with no on-disk format changes. No pg_upgrade needed. The next patches will start to make use of the wider address space, one SLRU at a time. 2. Use the larger segment file names in async.c, to lift the current 8 GB limit on the max number of pending notifications. No one actually minds the limit, it's quite generous as it is. But there is some code and complexity in async.c to avoid the wraparound that could be made simpler if we used longer SLRU segment names and avoided the wraparound altogether. I wonder if we should actually add an artificial limit, as a GUC. If there are gigabytes of notifications queued up, something's probably wrong with the system, and you're not going to be happy if we just remove the limit so it can grow to terabytes until you run out of disk space. 3. Extend pg_multixact so that pg_multixact/members is addressed by 64-bit offsets. Currently, multi-XIDs can wrap around, requiring anti-wraparound freezing, but independently of that, the pg_multixact/members SLRU can also wrap around. We track both, and trigger anti-wraparound if either SLRU is about to wrap around. If we redefine MultiXactOffset as a 64-bit integer, we can avoid the pg_multixact/members wraparound altogether. A downside is that pg_multixact/offsets will take twice as much space, but I think that's a good tradeoff. Or perhaps we can play tricks like store a single 64-bit offset on each pg_multixact/offsets page, and a 32-bit offset from that for each XID, to avoid making it so much larger. This would reduce the need to do anti-wraparound VACUUMs on systems that use multixacts heavily. Needs pg_upgrade support. 4. Extend pg_subtrans to 64-bits. This isn't all that interesting because the active region of pg_subtrans cannot be wider than 32 bits anyway, because you'll still reach the general 32-bit XID wraparound. But it might be less confusing in some places. I actually started to write a patch to do this, to see how complicated it is. It quickly proliferates into expanding other XIDs to 64-bits, like TransactionXmin, frozenXid calculation in vacuum.c, known-assigned XID tracking in procarray.c. etc. It's going to be necessary to convert 32-bit XIDs to FullTransactionIds at some boundaries, and I'm not sure where exactly that should happen. It's easier to do the conversions close to subtrans.c, but then I'm not sure how much it gets us in terms of reducing confusion. It's easy to get confused with the epochs during conversions, as you noted. On the other hand, if we change much more of the backend to use FullTransactionIds, the patch becomes much more invasive. Nice thing with pg_subtrans, though, is that it doesn't require pg_upgrade support. 5. Extend pg_xact to 64-bits. Similar to pg_subtrans, really, but needs pg_upgrade support. 6. (a bonu
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, > I'm surprised that these patches extend the page numbering to 64 bits, > but never actually uses the high bits. The XID "epoch" is not used, and > pg_xact still wraps around and the segment names are still reused. I > thought we could stop doing that. To clarify, the idea is to let CLOG grow indefinitely and simply store FullTransactionId -> TransactionStatus (two bits). Correct? I didn't investigate this in much detail but it may affect quite some amount of code since TransactionIdDidCommit() and TransactionIdDidCommit() currently both deal with TransactionId, not FullTransactionId. IMO, this would be a nice change however, assuming we are ready for it. In the previous version of the patch there was an attempt to derive FullTransactionId from TransactionId but it was wrong for the reasons named above in the thread. Thus is was removed and the patch simplified. -- Best regards, Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On 28/02/2023 16:17, Maxim Orlov wrote: Either I do not understand something, or the files from pg_commit_ts directory are not copied. Huh, yeah you're right, pg_upgrade doesn't copy pg_commit_ts. - Heikki
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, 27 Feb 2023 at 12:10, Heikki Linnakangas wrote: > (CC list trimmed, gmail wouldn't let me send otherwise) > > That sounds right for pg_serial, pg_notify, and pg_subtrans. But not for > pg_commit_ts and the pg_multixacts. > > This needs tests for pg_upgrading those SLRUs, after 0, 1 and N > wraparounds. > > Yep, that's my fault. I've forgotten about pg_multixacts. But for the pg_commit_ts it's a bit complicated story. The thing is, if we do upgrade, old files from pg_commit_ts not copied into a new server. For example, I've checked one more time on the current master branch: 1). initdb 2). add "track_commit_timestamp = on" into postgresql.conf 3). pgbench 4). $ ls pg_commit_ts/ 0005 000A 000F 0014 0019 001E 0023... ...009A 009F 00A4 00A9 00AE 00B3 00B8 00BD 00C2 5). do pg_upgrade 6). $ ls pg_commit_ts/ 00C2 Either I do not understand something, or the files from pg_commit_ts directory are not copied. -- Best regards, Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
(CC list trimmed, gmail wouldn't let me send otherwise) On 22/02/2023 16:29, Maxim Orlov wrote: On Tue, 21 Feb 2023 at 16:59, Aleksander Alekseev mailto:aleksan...@timescale.com>> wrote: One thing that still bothers me is that during the upgrade we only migrate the CLOG segments (i.e. pg_xact / pg_clog) and completely ignore all the rest of SLRUs: * pg_commit_ts * pg_multixact/offsets * pg_multixact/members * pg_subtrans * pg_notify * pg_serial Hi! We do ignore these values, since in order to pg_upgrade the server it must be properly stopped and no transactions can outlast this moment. That sounds right for pg_serial, pg_notify, and pg_subtrans. But not for pg_commit_ts and the pg_multixacts. This needs tests for pg_upgrading those SLRUs, after 0, 1 and N wraparounds. I'm surprised that these patches extend the page numbering to 64 bits, but never actually uses the high bits. The XID "epoch" is not used, and pg_xact still wraps around and the segment names are still reused. I thought we could stop doing that. Certainly if we start supporting 64-bit XIDs properly, that will need to change and we will pg_upgrade will need to rename the segments again. The previous versions of these patches did that, but I think you changed tact in response to Robert's suggestion at [1]: Lest we miss the forest for the trees, there is an aspect of this patch that I find to be an extremely good idea and think we should try to get committed even if the rest of the patch set ends up in the rubbish bin. Specifically, there are a couple of patches in here that have to do with making SLRUs indexed by 64-bit integers rather than by 32-bit integers. We've had repeated bugs in the area of handling SLRU wraparound in the past, some of which have caused data loss. Just by chance, I ran across a situation just yesterday where an SLRU wrapped around on disk for reasons that I don't really understand yet and chaos ensued. Switching to an indexing system for SLRUs that does not ever wrap around would probably enable us to get rid of a whole bunch of crufty code, and would also likely improve the general reliability of the system in situations where wraparound is threatened. It seems like a really, really good idea. These new versions of this patch don't achieve the goal of avoiding wraparound. I think the previous versions that did that was the right approach. [1] https://www.postgresql.org/message-id/CA%2BTgmoZFmTGjgkmjgkcm2-vQq3_TzcoMKmVimvQLx9oJLbye0Q%40mail.gmail.com - Heikki
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Tue, 21 Feb 2023 at 16:59, Aleksander Alekseev wrote: > Hi, > > One thing that still bothers me is that during the upgrade we only > migrate the CLOG segments (i.e. pg_xact / pg_clog) and completely > ignore all the rest of SLRUs: > > * pg_commit_ts > * pg_multixact/offsets > * pg_multixact/members > * pg_subtrans > * pg_notify > * pg_serial Hi! We do ignore these values, since in order to pg_upgrade the server it must be properly stopped and no transactions can outlast this moment. -- Best regards, Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, > The convert_pg_xact_segments() function is still obviously > overengineered. As I understand, all it has to do is simply renaming > pg_xact/ to pg_xact/. Unfortunately I used up all the > mana for today and have to take a long rest in order to continue. PFA the corrected patch v55. One thing that still bothers me is that during the upgrade we only migrate the CLOG segments (i.e. pg_xact / pg_clog) and completely ignore all the rest of SLRUs: * pg_commit_ts * pg_multixact/offsets * pg_multixact/members * pg_subtrans * pg_notify * pg_serial My knowledge in this area is somewhat limited and I can't tell whether this is OK. I will investigate but also I could use some feedback from the reviewers. -- Best regards, Aleksander Alekseev v55-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, > OK, here is the patchset v53 where I mostly modified the commit > messages. It is explicitly said that 0001 modifies the WAL records and > why we decided to do it in this patch. Additionally any mention of > 64-bit XIDs is removed since it is not guaranteed that the rest of the > patches are going to be accepted. 64-bit SLRU page numbering is a > valuable change per se. > > Changing the status of the CF entry to RfC apparently was a bit > premature. It looks like the patchset can use a few more rounds of > review. > > In 0002: > > [...] > > Maxim, perhaps you could share with us what your reasoning was here? I played with the patch a bit and managed to figure out what you tried to accomplish. Unfortunately generally you can't derive a FullTransactionId from a TransactionId, and you can't access ShmemVariableCache fields without taking a lock unless during the startup when there are no concurrent processes. I don't think this patch should do anything but change the SLRU indexing from 32-bit to 64-bit one. Trying to address the wraparounds would be nice but I'm afraid we are not quite there yet. Also I found strage little changes that seemed to be unrelated to the patch. I believe they ended up here by accident (used to be a part of 64-bit XIDs patchset) and removed them. PFA the cleaned up version of the patch. I noticed that splitting it into parts doesn't help much with the review or testing, nor seems it likely that the patches are going to be merged separately one by one. For these reasons I merged everything into a single patch. The convert_pg_xact_segments() function is still obviously overengineered. As I understand, all it has to do is simply renaming pg_xact/ to pg_xact/. Unfortunately I used up all the mana for today and have to take a long rest in order to continue. -- Best regards, Aleksander Alekseev v54-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi hackers,
> Yeah, pg_upgrade will briefly start and stop the old server to make
> sure all the WAL is replayed, and won't transfer any of the files
> over. AFAIK, major-version WAL changes are fine; it was the previous
> claim that we could do it in a minor version that I was unsure about.
OK, here is the patchset v53 where I mostly modified the commit
messages. It is explicitly said that 0001 modifies the WAL records and
why we decided to do it in this patch. Additionally any mention of
64-bit XIDs is removed since it is not guaranteed that the rest of the
patches are going to be accepted. 64-bit SLRU page numbering is a
valuable change per se.
Changing the status of the CF entry to RfC apparently was a bit
premature. It looks like the patchset can use a few more rounds of
review.
In 0002:
```
-#define TransactionIdToCTsPage(xid) \
-((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
+static inline int64
+TransactionIdToCTsPageInternal(TransactionId xid, bool lock)
+{
+FullTransactionIdfxid,
+nextXid;
+uint32epoch;
+
+if (lock)
+LWLockAcquire(XidGenLock, LW_SHARED);
+
+/* make a local copy */
+nextXid = ShmemVariableCache->nextXid;
+
+if (lock)
+LWLockRelease(XidGenLock);
+
+epoch = EpochFromFullTransactionId(nextXid);
+if (xid > XidFromFullTransactionId(nextXid))
+--epoch;
+
+fxid = FullTransactionIdFromEpochAndXid(epoch, xid);
+
+return fxid.value / (uint64) COMMIT_TS_XACTS_PER_PAGE;
+}
```
I'm pretty confident that shared memory can't be accessed like this,
without taking a lock. Although it may work on x64 generally we can
get garbage, unless nextXid is accessed atomically and has a
corresponding atomic type. On top of that I'm pretty sure
TransactionIds can't be compared with the regular comparison
operators. All in all, so far I don't understand why this piece of
code should be so complicated.
The same applies to:
```
-#define TransactionIdToPage(xid) ((xid) / (TransactionId)
SUBTRANS_XACTS_PER_PAGE)
+static inline int64
+TransactionIdToPageInternal(TransactionId xid, bool lock)
```
... in subtrans.c
Maxim, perhaps you could share with us what your reasoning was here?
--
Best regards,
Aleksander Alekseev
v53-0002-Utilize-64-bit-SLRU-page-numbers-in-SLRU-callers.patch
Description: Binary data
v53-0001-Use-64-bit-numbering-of-SLRU-pages.patch
Description: Binary data
v53-0003-pg_upgrade-from-32-bit-to-64-bit-SLRU-page-numbe.patch
Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Wed, Jan 11, 2023 at 1:48 AM Aleksander Alekseev wrote: > After reading [1] carefully it looks like we shouldn't worry about > this. The upgrade procedure explicitly requires to run `pg_ctl stop` > during step 8 of the upgrade procedure, i.e. not in the immediate mode > [2]. Yeah, pg_upgrade will briefly start and stop the old server to make sure all the WAL is replayed, and won't transfer any of the files over. AFAIK, major-version WAL changes are fine; it was the previous claim that we could do it in a minor version that I was unsure about. Thanks! --Jacob
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi Maxim, > Secondly, shouldn't we introduce a new WAL record type in order to > make the code backward compatible with previous PG versions? I'm not > 100% sure how the upgrade procedure works in all the details. If it > requires the DBMS to be gracefully shut down before the upgrade then > we are probably fine here. After reading [1] carefully it looks like we shouldn't worry about this. The upgrade procedure explicitly requires to run `pg_ctl stop` during step 8 of the upgrade procedure, i.e. not in the immediate mode [2]. It also has explicit instructions regarding the replicas. From what I can tell there is no way they will see WAL records they wouldn't understand. [1]: https://www.postgresql.org/docs/current/pgupgrade.html [2]: https://www.postgresql.org/docs/current/app-pg-ctl.html -- Best regards, Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi Maxim, > Here is a new patch set. > I've added comments and make use GetClogDirName call in copy_subdir_files. Jacob Champion pointed out (offlist, cc:'ed) that we may be wrong on this one: > 0001 patch changes the SLRU internals without affecting the callers. > 0001 - should make SLRU internally 64–bit, no effects from "outside" ... and apparently Jacob is right. Besides other things 0001 modifies CLOG_ZEROPAGE and CLOG_TRUNCATE WAL records - it makes changes to WriteZeroPageXlogRec() and WriteTruncateXlogRec() and corresponding changes to clog_desc() and clog_redo(). Firstly, it means that the patch doesn't change what it claims to change. I think these changes should be part of 0002. Secondly, shouldn't we introduce a new WAL record type in order to make the code backward compatible with previous PG versions? I'm not 100% sure how the upgrade procedure works in all the details. If it requires the DBMS to be gracefully shut down before the upgrade then we are probably fine here. -- Best regards, Aleksander Alekseev
Re: Add 64-bit XIDs into PostgreSQL 15
Hi Maxim, > Anyway. Let's discuss on-disk page format, shall we? Here are my two cents. > AFAICS, we have a following options: > [...] > 2. Put special in every page where base for XIDs are stored. This is what we > have done in the current patch set. The approach of using special space IMO is fine. I'm still a bit sceptical about the need to introduce a new entity "64-bit base XID" while we already have 32-bit XID epochs that will do the job. I suspect that having fewer entities helps to reason about the code and that it is important in the long run, but maybe it's just me. In any case, I don't have a strong opinion here. Additionally, I think we should be clear about the long term goals. As Peter G. pointed out above: > I think that we'll be able to get rid of freezing in a few years time. IMO eventually getting rid of freezing and "magic" XIDs will simplify the maintenance of the project and also make the behaviour of the system much more predictable. The user will have to worry only about the disk space reclamation. If we have a consensus that this is the final goal then we should definitely be moving toward 64-bit XIDs and perhaps even include a corresponding PoC to the patchset. If we want to keep freezing indefinitely then, as Chris Travers argued, 64-bit XIDs don't bring that much value and maybe the community should be focusing on something else. -- Best regards, Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi! Here is a new patch set. I've added comments and make use GetClogDirName call in copy_subdir_files. -- Best regards, Maxim Orlov. v52-0003-Make-pg_upgrade-from-32-bit-to-64-bit-SLRU.patch Description: Binary data v52-0002-Use-64-bit-pages-representation-in-SLRU-callers.patch Description: Binary data v52-0001-Use-internal-64-bit-numbering-of-SLRU-pages.patch Description: Binary data
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Fri, 6 Jan 2023 at 09:51, Japin Li wrote: > > For v51-0003. We can use GetClogDirName instead of GET_MAJOR_VERSION in > copy_subdir_files(). > Of course! Tanks! I'll address this in the next iteration, v52. -- Best regards, Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi Andrey, > Hi! I think that 64-bit xids are a very important feature and I want > to help advance it. That's why I want to try to understand a patch > better. Thanks for your interest to the patchset! > Do I get it right that the proposed v51 patchset only changes the SLRU > filenames and type of pageno representation? Is SLRU wraparound still > exactly there after 0x byte? OK, let me give some background then. I suspect you already know this, but this can be useful for other reviewers. Additionally we have two rather large threads on our hands and it's easy to lose track of things. SLRU is basically a general-purpose LRU implementation with ReadPage() / WritePage() interface with the only exception that instead of something like Page* object it operates slot numbers (array indexes). SLRU is used as an underlying container for several internal PostgreSQL structures, most importantly CLOG. Despite the name CLOG is not a log (journal) but rather a large bit array. For every transaction it stores two bits that reflect the status of the transaction (more detail in clog.c / clog.h). Currently SLRU operates 32-bit page numbers. What we currently agreed on [1] and what we are trying to achieve in this thread is to make SLRU pages 64-bit. The rest of the 64-bit XIDs is discussed in another thread [2]. As Robert Haas put it: > Specifically, there are a couple of patches in here that > have to do with making SLRUs indexed by 64-bit integers rather than by > 32-bit integers. We've had repeated bugs in the area of handling SLRU > wraparound in the past, some of which have caused data loss. Just by > chance, I ran across a situation just yesterday where an SLRU wrapped > around on disk for reasons that I don't really understand yet and > chaos ensued. Switching to an indexing system for SLRUs that does not > ever wrap around would probably enable us to get rid of a whole bunch > of crufty code, and would also likely improve the general reliability > of the system in situations where wraparound is threatened. It seems > like a really, really good idea. So our goal here is to eliminate wrap-around for SLRU. It means that if I save something to the page 0x12345678 it will stay there forever. Other parts of the system however have to form proper 64-bit page numbers in order to make it work. If they don't the wrap-around is possible for these particular subsystems (but not SLRU per se). > Also, I do not understand what is the reason for splitting 1st and 2nd > steps. Certainly, there must be some logic behind it, but I just can't > grasp it... 0001 patch changes the SLRU internals without affecting the callers. It also preserves the short SLRU filenames which means nothing changes for an outside observer. All it changes is PostgreSQL binary. It can be merged any time and even backported to the previous versions if we want to. The 0002 patch makes changes to the callers and also enlarges SLRU filenames. For sure we could do everything at once, but it would complicate testing and more importantly code review. Personally I believe Maxim did a great job here. Both patches were easy to read and understand (relatively, of course). > And the purpose of the 3rd step with pg_upgrade changes is a complete > mystery for me. 0001 and 0002 will work fine for new PostgreSQL instances. But if you have an instance that already has on-disk state we have to move the SLRU segments accordingly. This is what 0003 does. That's the theory at least. Personally I still have to meditate a bit more on the code in order to get a good understanding of it, especially the parts that deal with transaction epochs because this is something I have limited experience with. Also I wouldn't exclude the possibility of bugs. Particularly this part of 0003: ``` +oldseg.segno = pageno / SLRU_PAGES_PER_SEGMENT; +oldseg.pageno = pageno % SLRU_PAGES_PER_SEGMENT; + +newseg.segno = pageno / SLRU_PAGES_PER_SEGMENT; +newseg.pageno = pageno % SLRU_PAGES_PER_SEGMENT; ``` looks suspicious to me. I agree that adding a couple of additional comments could be appropriate, especially when it comes to epochs. [1]: https://www.postgresql.org/message-id/CA%2BTgmoZFmTGjgkmjgkcm2-vQq3_TzcoMKmVimvQLx9oJLbye0Q%40mail.gmail.com [2]: https://www.postgresql.org/message-id/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com -- Best regards, Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
> > Do I get it right that the proposed v51 patchset only changes the SLRU > filenames and type of pageno representation? Is SLRU wraparound still > exactly there after 0x byte? > After applying the whole patch set, SLRU will become 64–bit without a wraparound. Thus, no wraparound should be there. 0001 - should make SLRU internally 64–bit, no effects from "outside" 0002 - should make SLRU callers 64–bit, SLRU segment files naming are changed 0003 - make upgrade from previous versions feasible > Also, I do not understand what is the reason for splitting 1st and 2nd > steps. Certainly, there must be some logic behind it, but I just can't > grasp it... > As we did discuss somewhere in the beginning of the discussion, we try to make every commit as independent as possible. Thus, it is much easier to review and commit. I see no problem to meld these commits into one, if consensus will be reached. > And the purpose of the 3rd step with pg_upgrade changes is a complete > mystery for me. Please excuse my incompetence in the topic, but maybe > some commit message or comments would help. What kind of xact segments > conversion we do? Why is it only necessary for xacts, but not other > SLRUs? > The purpose of the third patch is to make upgrade feasible. Since we've change pg_xact files naming, Postgres could not read status of "old" transactions from "old" pg_xact files. So, we have to convert those files. The major problem here is that we must handle possible segment wraparound (in "old" cluster). The whole idea for an upgrade is to read SLRU pages for pg_xact one by one and write it in a "new" filename. Maybe, It's just a little bit complicated, since the algorithm is intended to deal with different SLRU pages per segment in "new" and "old" clusters. But, on the other hand, it is already created in original patch set of 64–bit XIDs and will be useful in the future. AFAICS, arguably, any variant of 64–bit XIDs should lead to increase of an amount of SLRU pages per segment. And as for other SLRUs, they cannot survive pg_upgrade mostly by the fact, that cluster must be stopped upon upgrade. Thus, no conversion needed. -- Best regards, Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, 19 Dec 2022 at 22:40, Maxim Orlov wrote:
> Hi!
>
> As a result of discussion in the thread [0], Robert Haas proposed to focus
> on making SLRU 64 bit, as a first step towards 64 bit XIDs.
> Here is the patch set.
>
> In overall, code of this patch set is based on the existing code from [0]
> and may be simplified, due to the fact, that SLRU_PAGES_PER_SEGMENT is not
> meant to be changed now.
> But I decided to leave it that way. At least for now.
>
> As always, reviews and opinions are very welcome.
>
For v51-0003. We can use GetClogDirName instead of GET_MAJOR_VERSION in
copy_subdir_files().
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 1c49c63444..3934978b97 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -857,10 +857,7 @@ copy_xact_xlog_xid(void)
pfree(new_path);
}
else
- copy_subdir_files(GET_MAJOR_VERSION(old_cluster.major_version)
<= 906 ?
- "pg_clog" : "pg_xact",
-
GET_MAJOR_VERSION(new_cluster.major_version) <= 906 ?
- "pg_clog" : "pg_xact");
+ copy_subdir_files(GetClogDirName(old_cluster),
GetClogDirName(new_cluster));
prep_status("Setting oldest XID for new cluster");
exec_prog(UTILITY_LOG_FILE, NULL, true, true,
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, Dec 19, 2022 at 6:41 AM Maxim Orlov wrote: > > As always, reviews and opinions are very welcome. Hi! I think that 64-bit xids are a very important feature and I want to help advance it. That's why I want to try to understand a patch better. Do I get it right that the proposed v51 patchset only changes the SLRU filenames and type of pageno representation? Is SLRU wraparound still exactly there after 0x byte? The thing is we had some nasty bugs because SLRU wraparound is tricky. And I think it would be beneficial if we could get to continuous SLRU space. But the patch seems to avoid addressing this problem. Also, I do not understand what is the reason for splitting 1st and 2nd steps. Certainly, there must be some logic behind it, but I just can't grasp it... And the purpose of the 3rd step with pg_upgrade changes is a complete mystery for me. Please excuse my incompetence in the topic, but maybe some commit message or comments would help. What kind of xact segments conversion we do? Why is it only necessary for xacts, but not other SLRUs? Thank you for working on this important project! Best regards, Andrey Borodin.
Re: Add 64-bit XIDs into PostgreSQL 15
Hi Maxim Orlov: >AFAICS, we have a following options: >1. Making "true" 64�Cbit XIDs. I.e. making every tuple have 64�Cbit xmin and >>xmax fields. >2. Put special in every page where base for XIDs are stored. This is what we >>have done in the current patch set. >3. Put base for XIDs in a fork. >4. Make explicit 64�Cbit XIDs for concrete relations. I.e. CREATE TABLE foo >>WITH (xid8) of smth. I think the first solution will not be agreed by the core committers, they will consider that the change is too big and will affect the stability of PostgreSQL,I think the second solution is actually quite good, and you've been working on it now,and there are successful cases (opengauss is implemented in this way,In order to save space and be compatible with older versions, opengauss design is to store the xmin/xmax of the head of the tuple in two parts, the xmin/xmax of the head of the tuple is the number of uint32; the header of the page stores the 64-bit xid_base, which is the xid_base of the current page.),I think it's best to stick to this solution now. Opengauss tuple structure: [cid:3fae289c-7f88-46be-a775-2d93b1a9c41e] Best wish 发件人: Maxim Orlov 发送时间: 2022年12月28日 18:14 收件人: Pavel Borisov 抄送: Robert Haas ; Chris Travers ; Bruce Momjian ; Aleksander Alekseev ; pgsql-hackers@lists.postgresql.org ; Chris Travers ; Peter Geoghegan ; Fedor Sigaev ; Alexander Korotkov ; Konstantin Knizhnik ; Nikita Glukhov ; Yura Sokolov ; Simon Riggs 主题: Re: Add 64-bit XIDs into PostgreSQL 15 Hi! I want to make a quick summary here. 1. An overall consensus has been reached: we shall focus on committing SLRU changes first. 2. I've created an appropriate patch set here [0]. 3. How [0] is waiting for a review. As always, all opinions will be welcome. 4. While discussing error/warning messages and some other stuff, this thread was marked as "Waiting on Author". 5. I do rebase this patch set once in a week, but do not post it here, since there is no need in it. See (1). 6. For now, I don't understand what changes I have to make here. So, does "Waiting on Author" is appropriate status here? Anyway. Let's discuss on-disk page format, shall we? AFAICS, we have a following options: 1. Making "true" 64�Cbit XIDs. I.e. making every tuple have 64�Cbit xmin and xmax fields. 2. Put special in every page where base for XIDs are stored. This is what we have done in the current patch set. 3. Put base for XIDs in a fork. 4. Make explicit 64�Cbit XIDs for concrete relations. I.e. CREATE TABLE foo WITH (xid8) of smth. There were opinions that the proposed solution (2) is not the optimal. It would be great to hear your concerns and thoughts. [0] https://www.postgresql.org/message-id/CACG%3Dezav34TL%2BfGXD5vJ48%3DQbQBL9BiwkOTWduu9yRqie-h%2BDg%40mail.gmail.com -- Best regards, Maxim Orlov.
Re: Add 64-bit XIDs into PostgreSQL 15
Hi! I want to make a quick summary here. 1. An overall consensus has been reached: we shall focus on committing SLRU changes first. 2. I've created an appropriate patch set here [0]. 3. How [0] is waiting for a review. As always, all opinions will be welcome. 4. While discussing error/warning messages and some other stuff, this thread was marked as "Waiting on Author". 5. I do rebase this patch set once in a week, but do not post it here, since there is no need in it. See (1). 6. For now, I don't understand what changes I have to make here. So, does "Waiting on Author" is appropriate status here? Anyway. Let's discuss on-disk page format, shall we? AFAICS, we have a following options: 1. Making "true" 64–bit XIDs. I.e. making every tuple have 64–bit xmin and xmax fields. 2. Put special in every page where base for XIDs are stored. This is what we have done in the current patch set. 3. Put base for XIDs in a fork. 4. Make explicit 64–bit XIDs for concrete relations. I.e. CREATE TABLE foo WITH (xid8) of smth. There were opinions that the proposed solution (2) is not the optimal. It would be great to hear your concerns and thoughts. [0] https://www.postgresql.org/message-id/CACG%3Dezav34TL%2BfGXD5vJ48%3DQbQBL9BiwkOTWduu9yRqie-h%2BDg%40mail.gmail.com -- Best regards, Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi! As a result of discussion in the thread [0], Robert Haas proposed to focus on making SLRU 64 bit, as a first step towards 64 bit XIDs. Here is the patch set. In overall, code of this patch set is based on the existing code from [0] and may be simplified, due to the fact, that SLRU_PAGES_PER_SEGMENT is not meant to be changed now. But I decided to leave it that way. At least for now. As always, reviews and opinions are very welcome. Should we change status for this thread to "need review"? [0] https://www.postgresql.org/message-id/flat/CA%2BTgmoZFmTGjgkmjgkcm2-vQq3_TzcoMKmVimvQLx9oJLbye0Q%40mail.gmail.com#03a4ab82569bb7b112db4a2f352d96b9 -- Best regards, Maxim Orlov. v51-0003-Make-pg_upgrade-from-32-bit-to-64-bit-SLRU.patch Description: Binary data v51-0001-Use-internal-64-bit-numbering-of-SLRU-pages.patch Description: Binary data v51-0002-Use-64-bit-pages-representation-in-SLRU-callers.patch Description: Binary data
Re: Add 64-bit XIDs into PostgreSQL 15
On Fri, 9 Dec 2022 at 16:54, adherent postgres < adherent_postg...@hotmail.com> wrote: > Hi Aleksander Alekseev > I think the xids 32bit transformation project has been dragged on for too > long. Huawei's openGauss referenced this patch to implement xids 64bit, and > Postgrespro also implemented xids 64bit, which is enough to prove that > their worries are redundant.I think postgresql has no reason not to > implement xid 64 bit. What about your opinion? > Yeah, I totally agree, the time has come. With a high transaction load, Postgres become more and more difficult to maintain. The problem is in the overall complexity of the patch set. We need more reviewers. Since committing such a big patch is not viable once at a time, from the start of the work we did split it into several logical parts. The evolution concept is more preferable in this case. As far as I can see, there is overall consensus to commit SLRU related changes first. -- Best regards, Maxim Orlov.
回复: Add 64-bit XIDs into PostgreSQL 15
Hi Pavel Borisov Now the disk performance has been improved many times, and the capacity has also been increased many times,The wal log already supports lz4 and zstd compression, I think each XLOG record size will increase at least by 4 bytes which is not a big problem.What about your opinion? Best whish 发件人: Pavel Borisov 发送时间: 2022年12月9日 22:13 收件人: adherent postgres 抄送: Aleksander Alekseev ; pgsql-hackers@lists.postgresql.org ; Chris Travers ; Chris Travers ; Bruce Momjian 主题: Re: Add 64-bit XIDs into PostgreSQL 15 Hi, Adherent! On Fri, 9 Dec 2022 at 17:54, adherent postgres wrote: > > Hi Aleksander Alekseev > I think the xids 32bit transformation project has been dragged on for too > long. Huawei's openGauss referenced this patch to implement xids 64bit, and > Postgrespro also implemented xids 64bit, which is enough to prove that their > worries are redundant.I think postgresql has no reason not to implement xid > 64 bit. What about your opinion? I agree it's high time to commit 64xids into PostgreSQL. If you can do your review of the whole proposed patchset or only the part that is likely to be committed earlier [1] it would help a lot! I'd recommend beginning with the last version of the patch in thread [1]. First, it is easier. Also, this review is going to be useful sooner and will help a committer on January commitfest a lot. [1]: https://www.postgresql.org/message-id/CAFiTN-uudj2PY8GsUzFtLYFpBoq_rKegW3On_8ZHdxB1mVv3-A%40mail.gmail.com Regards, Pavel Borisov, Supabase
Re: Add 64-bit XIDs into PostgreSQL 15
Hi, Adherent! On Fri, 9 Dec 2022 at 17:54, adherent postgres wrote: > > Hi Aleksander Alekseev > I think the xids 32bit transformation project has been dragged on for too > long. Huawei's openGauss referenced this patch to implement xids 64bit, and > Postgrespro also implemented xids 64bit, which is enough to prove that their > worries are redundant.I think postgresql has no reason not to implement xid > 64 bit. What about your opinion? I agree it's high time to commit 64xids into PostgreSQL. If you can do your review of the whole proposed patchset or only the part that is likely to be committed earlier [1] it would help a lot! I'd recommend beginning with the last version of the patch in thread [1]. First, it is easier. Also, this review is going to be useful sooner and will help a committer on January commitfest a lot. [1]: https://www.postgresql.org/message-id/CAFiTN-uudj2PY8GsUzFtLYFpBoq_rKegW3On_8ZHdxB1mVv3-A%40mail.gmail.com Regards, Pavel Borisov, Supabase
Re: Add 64-bit XIDs into PostgreSQL 15
> So I'd vote for an evolutionary approach and give my +1 for > undertaking efforts to first committing [1] to 16. > > [1]: > https://www.postgresql.org/message-id/CAFiTN-uudj2PY8GsUzFtLYFpBoq_rKegW3On_8ZHdxB1mVv3-A%40mail.gmail.com > > Kind regards, > Pavel Borisov, > Supabase. > +1 Totally support the idea. Let's focus on committing SLRU changes. -- Best regards, Maxim Orlov.
Re: Add 64-bit XIDs into PostgreSQL 15
Hi Aleksander Alekseev I think the xids 32bit transformation project has been dragged on for too long. Huawei's openGauss referenced this patch to implement xids 64bit, and Postgrespro also implemented xids 64bit, which is enough to prove that their worries are redundant.I think postgresql has no reason not to implement xid 64 bit. What about your opinion? Best whish 发件人: Aleksander Alekseev 发送时间: 2022年12月9日 20:49 收件人: pgsql-hackers@lists.postgresql.org 抄送: adherent postgres ; Chris Travers ; Chris Travers ; Bruce Momjian 主题: Re: Add 64-bit XIDs into PostgreSQL 15 Hi adherent, > Robertmhaas said that the project Zheap is > dead（https://twitter.com/andy_pavlo/status/1590703943176589312）, which means > that we cannot use Zheap to deal with the issue of xid wraparound and dead > tuples in tables. The dead tuple issue is not a big deal because I can still > use pg_repack to handle, although pg_repack will cause wal log to increase > dramatically and may take one or two days to handle a large table. During > this time the database can be accessed by external users, but the xid > wraparound will cause PostgreSQL to be down, which is a disaster for DBAs. > Maybe you are not a DBA, or your are from a small country, Database system > tps is very low, so xid32 is enough for your database system , Oracle's scn > was also 32bits, however, Oracle realized the issue and changed scn to 64 > bits. The transaction id in mysql is 48 bits. MySQL didn't fix the > transaction id wraparound problem because they think that 48 bits is enough > for the transaction id. This project has been running for almost 1 year and > now it is coming to an end. I strongly disagree with your idea of stopping > this patch, and I suspect you are a saboteur. I strongly disagree with your > viewpoint, as it is not a fundamental way to solve the xid wraparound > problem. The PostgreSQL community urgently needs developers who solve > problems like this, not bury one' head in the sand This is not uncommon for people on the mailing list to have disagreements. This is part of the process, we all are looking for consensus. It's true that different people have different use cases in mind and different backgrounds as well. It doesn't mean these use cases are wrong and/or the experience is irrelevant and/or the received feedback should be just discarded. Although I also expressed my disagreement with Chris before, let's not assume any bad intent and especially sabotage as you put it. (Unless you have a strong proof of this of course which I doubt you have.) We want all kinds of feedback to be welcomed here. I'm sure our goal here is mutual, to make PostgreSQL even better than it is now. The only problem is that the definition of "better" varies sometimes. I see you believe that 64-bit XIDs are going to be useful. That's great! Tell us more about your case and how the patch is going to help with it. Also, maybe you could test your load with the applied patchset and tell us whether it makes things better or worse? Personally I would love hearing this from you. -- Best regards, Aleksander Alekseev
Re: Add 64-bit XIDs into PostgreSQL 15
Hi, Robert! > Lest we miss the forest for the trees, there is an aspect of this > patch that I find to be an extremely good idea and think we should try > to get committed even if the rest of the patch set ends up in the > rubbish bin. Specifically, there are a couple of patches in here that > have to do with making SLRUs indexed by 64-bit integers rather than by > 32-bit integers. We've had repeated bugs in the area of handling SLRU > wraparound in the past, some of which have caused data loss. Just by > chance, I ran across a situation just yesterday where an SLRU wrapped > around on disk for reasons that I don't really understand yet and > chaos ensued. Switching to an indexing system for SLRUs that does not > ever wrap around would probably enable us to get rid of a whole bunch > of crufty code, and would also likely improve the general reliability > of the system in situations where wraparound is threatened. It seems > like a really, really good idea. I totally support the idea that the part related to SLRU is worth committing whether it is being the first step to 64xid or separately. This subset is discussed in a separate thread [1]. It seems that we need more time to reach a consensus on the implementation of a whole big thing. Just this discussion is a complicated thing and reveals many different aspects concurrently in one thread. So I'd vote for an evolutionary approach and give my +1 for undertaking efforts to first committing [1] to 16. [1]: https://www.postgresql.org/message-id/CAFiTN-uudj2PY8GsUzFtLYFpBoq_rKegW3On_8ZHdxB1mVv3-A%40mail.gmail.com Kind regards, Pavel Borisov, Supabase.
Re: Add 64-bit XIDs into PostgreSQL 15
Hi adherent, > Robertmhaas said that the project Zheap is > dead（https://twitter.com/andy_pavlo/status/1590703943176589312）, which means > that we cannot use Zheap to deal with the issue of xid wraparound and dead > tuples in tables. The dead tuple issue is not a big deal because I can still > use pg_repack to handle, although pg_repack will cause wal log to increase > dramatically and may take one or two days to handle a large table. During > this time the database can be accessed by external users, but the xid > wraparound will cause PostgreSQL to be down, which is a disaster for DBAs. > Maybe you are not a DBA, or your are from a small country, Database system > tps is very low, so xid32 is enough for your database system , Oracle's scn > was also 32bits, however, Oracle realized the issue and changed scn to 64 > bits. The transaction id in mysql is 48 bits. MySQL didn't fix the > transaction id wraparound problem because they think that 48 bits is enough > for the transaction id. This project has been running for almost 1 year and > now it is coming to an end. I strongly disagree with your idea of stopping > this patch, and I suspect you are a saboteur. I strongly disagree with your > viewpoint, as it is not a fundamental way to solve the xid wraparound > problem. The PostgreSQL community urgently needs developers who solve > problems like this, not bury one' head in the sand This is not uncommon for people on the mailing list to have disagreements. This is part of the process, we all are looking for consensus. It's true that different people have different use cases in mind and different backgrounds as well. It doesn't mean these use cases are wrong and/or the experience is irrelevant and/or the received feedback should be just discarded. Although I also expressed my disagreement with Chris before, let's not assume any bad intent and especially sabotage as you put it. (Unless you have a strong proof of this of course which I doubt you have.) We want all kinds of feedback to be welcomed here. I'm sure our goal here is mutual, to make PostgreSQL even better than it is now. The only problem is that the definition of "better" varies sometimes. I see you believe that 64-bit XIDs are going to be useful. That's great! Tell us more about your case and how the patch is going to help with it. Also, maybe you could test your load with the applied patchset and tell us whether it makes things better or worse? Personally I would love hearing this from you. -- Best regards, Aleksander Alekseev
回复: Add 64-bit XIDs into PostgreSQL 15
Hi Chris Travers Robertmhaas said that the project Zheap is dead（https://twitter.com/andy_pavlo/status/1590703943176589312）, which means that we cannot use Zheap to deal with the issue of xid wraparound and dead tuples in tables. The dead tuple issue is not a big deal because I can still use pg_repack to handle, although pg_repack will cause wal log to increase dramatically and may take one or two days to handle a large table. During this time the database can be accessed by external users, but the xid wraparound will cause PostgreSQL to be down, which is a disaster for DBAs. Maybe you are not a DBA, or your are from a small country, Database system tps is very low, so xid32 is enough for your database system , Oracle's scn was also 32bits, however, Oracle realized the issue and changed scn to 64 bits. The transaction id in mysql is 48 bits. MySQL didn't fix the transaction id wraparound problem because they think that 48 bits is enough for the transaction id. This project has been running for almost 1 year and now it is coming to an end. I strongly disagree with your idea of stopping this patch, and I suspect you are a saboteur. I strongly disagree with your viewpoint, as it is not a fundamental way to solve the xid wraparound problem. The PostgreSQL community urgently needs developers who solve problems like this, not bury one' head in the sand Best whish 发件人: Peter Geoghegan 发送时间: 2022年12月1日 0:35 收件人: Robert Haas 抄送: Chris Travers ; Bruce Momjian ; Aleksander Alekseev ; pgsql-hackers@lists.postgresql.org ; Chris Travers ; Fedor Sigaev ; Alexander Korotkov ; Konstantin Knizhnik ; Nikita Glukhov ; Yura Sokolov ; Maxim Orlov ; Pavel Borisov ; Simon Riggs 主题: Re: Add 64-bit XIDs into PostgreSQL 15 On Wed, Nov 30, 2022 at 8:13 AM Robert Haas wrote: > I haven't checked the patches to see whether they look correct, and > I'm concerned in particular about upgrade scenarios. But if there's a > way we can get that part committed, I think it would be a clear win. +1 -- Peter Geoghegan
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Wed, Dec 7, 2022 at 9:50 AM Andres Freund wrote: > performing post-bootstrap initialization ... > ../src/backend/access/transam/slru.c:1520:9: runtime error: load of > misaligned address 0x7fff6362db8c for type 'int64', which requires 8 byte > alignment > 0x7fff6362db8c: note: pointer points here > 01 00 00 00 00 00 00 00 d0 02 00 00 00 00 00 00 d0 02 00 00 00 00 00 00 > 01 00 00 00 00 00 00 00 I bet that this alignment issue can be fixed by using PGAlignedBlock instead of a raw char buffer for a page. (I'm guessing, I haven't directly checked.) -- Peter Geoghegan
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Hi, On 2022-12-07 11:40:08 +0300, Aleksander Alekseev wrote: > Hi Michael, > > > The CF bot is showing some failures here. You may want to > > double-check. > > Thanks! PFA v48. This causes a lot of failures with ubsan: https://cirrus-ci.com/task/6035600772431872 performing post-bootstrap initialization ... ../src/backend/access/transam/slru.c:1520:9: runtime error: load of misaligned address 0x7fff6362db8c for type 'int64', which requires 8 byte alignment 0x7fff6362db8c: note: pointer points here 01 00 00 00 00 00 00 00 d0 02 00 00 00 00 00 00 d0 02 00 00 00 00 00 00 01 00 00 00 00 00 00 00 ^ ==18947==Using libbacktrace symbolizer. #0 0x564d7c45cc6b in SlruScanDirCbReportPresence ../src/backend/access/transam/slru.c:1520 #1 0x564d7c45cd88 in SlruScanDirectory ../src/backend/access/transam/slru.c:1595 #2 0x564d7c44872c in TruncateCLOG ../src/backend/access/transam/clog.c:889 #3 0x564d7c62ecd7 in vac_truncate_clog ../src/backend/commands/vacuum.c:1779 #4 0x564d7c6320a8 in vac_update_datfrozenxid ../src/backend/commands/vacuum.c:1642 #5 0x564d7c632a78 in vacuum ../src/backend/commands/vacuum.c:537 #6 0x564d7c63347d in ExecVacuum ../src/backend/commands/vacuum.c:273 #7 0x564d7ca4afea in standard_ProcessUtility ../src/backend/tcop/utility.c:866 #8 0x564d7ca4b723 in ProcessUtility ../src/backend/tcop/utility.c:530 #9 0x564d7ca46e81 in PortalRunUtility ../src/backend/tcop/pquery.c:1158 #10 0x564d7ca4755d in PortalRunMulti ../src/backend/tcop/pquery.c:1315 #11 0x564d7ca47c02 in PortalRun ../src/backend/tcop/pquery.c:791 #12 0x564d7ca40ecb in exec_simple_query ../src/backend/tcop/postgres.c:1238 #13 0x564d7ca43c01 in PostgresMain ../src/backend/tcop/postgres.c:4551 #14 0x564d7ca441a4 in PostgresSingleUserMain ../src/backend/tcop/postgres.c:4028 #15 0x564d7c74d883 in main ../src/backend/main/main.c:197 #16 0x7fde7793dd09 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x23d09) #17 0x564d7c2d30c9 in _start (/tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres+0x8530c9) Greetings, Andres Freund
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, Nov 21, 2022 at 12:21:09PM +0300, Aleksander Alekseev wrote: > After merging 1489b1ce [1] the patchset needed a rebase. PFA v47. > > [1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=1489b1ce The CF bot is showing some failures here. You may want to double-check. -- Michael signature.asc Description: PGP signature
Re: Add 64-bit XIDs into PostgreSQL 15
On Wed, Nov 30, 2022 at 8:13 AM Robert Haas wrote: > I haven't checked the patches to see whether they look correct, and > I'm concerned in particular about upgrade scenarios. But if there's a > way we can get that part committed, I think it would be a clear win. +1 -- Peter Geoghegan
