Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Michael Paquier]

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)

[Alexander Korotkov]

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)

[Noah Misch]

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: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Michael Paquier]

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)",
+

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Noah Misch]

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)

[Aleksander Alekseev]

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)

[Michael Paquier]

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)

[Aleksander Alekseev]

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 = >allProcs[nextidx];
-		int			thispageno = nextproc->clogGroupMemberPage;
+		int64		thispageno = nextproc->clogGroupMemberPage;
 
 		/*
 		 * If the page to update belongs to a different bank than the previous

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Michael Paquier]

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)

[Aleksander Alekseev]

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)

[Michael Paquier]

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)

[Aleksander Alekseev]

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)

[Aleksander Alekseev]

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)

[Noah Misch]

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)

[Aleksander Alekseev]

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)

[Noah Misch]

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

回复: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Thomas wen]

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)

[John Naylor]

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)

[Pavel Borisov]

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)

[John Naylor]

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)

[Alexander Korotkov]

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)

[Pavel Borisov]

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)

[Tom Lane]

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)

[Alexander Korotkov]

Андрей, привет!

Текущее положение у меня такое.

.
*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)

[Alexander Lakhin]

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 Korotkov]

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)

[Maxim Orlov]

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)

[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.

> 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)

[Maxim Orlov]

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)

[Aleksander Alekseev]

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)

[Aleksander Alekseev]

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)

[Alexander Korotkov]

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)

[Aleksander Alekseev]

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)

[Aleksander Alekseev]

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)

[Pavel Borisov]

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)

[Alexander Korotkov]

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)

[Pavel Borisov]

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)

[Alexander Korotkov]

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)

[Thomas wen]

> > 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)

[Aleksander Alekseev]

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)

[Aleksander Alekseev]

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)

[Heikki Linnakangas]

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: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Aleksander Alekseev]

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)

[Maxim Orlov]

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)

[Aleksander Alekseev]

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)

[Maxim Orlov]

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)

[Aleksander Alekseev]

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)

[Heikki Linnakangas]

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)

[Maxim Orlov]

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 

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Heikki Linnakangas]

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 

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Aleksander Alekseev]

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)

[Heikki Linnakangas]

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)

[Maxim Orlov]

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)

[Heikki Linnakangas]

(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)

[Maxim Orlov]

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)

[Aleksander Alekseev]

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)

[Aleksander Alekseev]

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)

[Aleksander Alekseev]

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)

[Jacob Champion]

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)

[Aleksander Alekseev]

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)

[Aleksander Alekseev]

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: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Maxim Orlov]

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)

[Maxim Orlov]

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)

[Aleksander Alekseev]

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)

[Maxim Orlov]

>
> 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)

[Japin Li]

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)

[Andrey Borodin]

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: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Maxim Orlov]

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: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Peter Geoghegan]

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)

[Andres Freund]

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)

[Michael Paquier]

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: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Ian Lawrence Barwick]

2022年11月3日(木) 17:15 Maxim Orlov :
>
> Hi!
>
>>
>> This entry was marked "Ready for committer" in the CommitFest app but cfbot
>> reports the patch no longer applies.
>
> Thanks for the reminder. I think, this should work.

Thanks for the quick update, cfbot reports no issues.

I've set the CF entry back to "Ready for Committer",

Regards

Ian Barwick

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Dilip Kumar]

On Thu, Nov 3, 2022 at 1:46 PM Maxim Orlov  wrote:
>
> Hi!
>
>>
>> This entry was marked "Ready for committer" in the CommitFest app but cfbot
>> reports the patch no longer applies.
>
> Thanks for the reminder. I think, this should work.

Have we measured the WAL overhead because of this patch set? maybe
these particular patches will not impact but IIUC this is ground work
for making xid 64 bit.  So each XLOG record size will increase at
least by 4 bytes because the XLogRecord contains the xid.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Ian Lawrence Barwick]

2022年10月10日(月) 18:16 Aleksander Alekseev :
>
> Hi hackers,
>
> > Sorry about that.
>
> No problem.
>
> > Thanks, Simon. Not 100% sure who exactly is invited to vote, but just
> > in case here is +1 from me. These patches were "Ready for Committer"
> > for several months now and are unlikely to get any better. So I
> > suggest we merge them.
>
> Here is the rebased patchset number 44.

Hi

This entry was marked "Ready for committer" in the CommitFest app but cfbot
reports the patch no longer applies.

We've marked it as "Waiting on Author". As CommitFest 2022-11 is
currently underway, this would be an excellent time update the patch.

Once you think the patchset is ready for review again, you (or any
interested party) can move the patch entry forward by visiting

https://commitfest.postgresql.org/40/3489/

and changing the status to "Ready for committer".


Thanks

Ian Barwick

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Maxim Orlov]

>
> This is the wrong thread / CF entry. Please see
>

Yep, my fault. Sorry about that.

-- 
Best regards,
Maxim Orlov.

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Aleksander Alekseev]

Maxim,

> Here is a rebased version of the patch set.

This is the wrong thread / CF entry. Please see
http://cfbot.cputube.org/ and https://commitfest.postgresql.org/ and
the first email in the thread.

-- 
Best regards,
Aleksander Alekseev

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Hamid Akhtar]

On Tue, 26 Jul 2022 at 21:35, Simon Riggs 
wrote:

> On Fri, 15 Jul 2022 at 12:29, Aleksander Alekseev
>  wrote:
> > Personally I didn't expect that
> > merging patches in this thread would take that long. They are in
> > "Ready for Committer" state for a long time now and there are no known
> > issues with them other than unit tests for SLRU [1] should be merged
> > first.
>
> These patches look ready to me, including the SLRU tests.
>
> Even though they do very little, these patches touch many aspects of
> the code, so it would make sense to apply these as the last step in
> the CF.
>
> To encourage committers to take that next step, let's have a
> democratic vote on moving this forwards:
> +1 from me.
>

This set of patches no longer applies cleanly to the master branch. There
are lots of
hunks as well as failures. Please rebase the patches.

There are failures for multiple files including the one given below:

patching file src/backend/replication/logical/worker.c
Hunk #1 succeeded at 1089 (offset 1 line).
Hunk #2 succeeded at 1481 (offset 1 line).
Hunk #3 succeeded at 3322 (offset 2 lines).
Hunk #4 succeeded at 3493 (offset 2 lines).
Hunk #5 FAILED at 4009.
1 out of 5 hunks FAILED -- saving rejects to file
src/backend/replication/logical/worker.c.rej

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Simon Riggs]

On Fri, 15 Jul 2022 at 12:29, Aleksander Alekseev
 wrote:
> Personally I didn't expect that
> merging patches in this thread would take that long. They are in
> "Ready for Committer" state for a long time now and there are no known
> issues with them other than unit tests for SLRU [1] should be merged
> first.

These patches look ready to me, including the SLRU tests.

Even though they do very little, these patches touch many aspects of
the code, so it would make sense to apply these as the last step in
the CF.

To encourage committers to take that next step, let's have a
democratic vote on moving this forwards:
+1 from me.

-- 
Simon Riggshttp://www.EnterpriseDB.com/

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Aleksander Alekseev]

Pavel, Justin,

>> Is there any reason to continue with two separate threads and CF entries ?
>> The original reason was to have a smaller patch for considerate late in v15.

> I see the main goal of this split is to make discussion of this (easier) 
> thread separate to the discussion of a whole patchset which is expected to be 
> more thorough.

+1. This was done per explicit request in the first thread.

>> But right now, it just seems to cause every update to imply two email 
>> messages
>> rather than one.
>>
>> Also, since this patch series is large, and expects a lot of conflicts, it
>> seems better to update the cfapp with a "git link" [0] where you can 
>> maintain a
>> rebased branch.  It avoids the need to mail new patches to the list several
>> times more often than it's being reviewed.

Yep, this is a bit inconvenient. Personally I didn't expect that
merging patches in this thread would take that long. They are in
"Ready for Committer" state for a long time now and there are no known
issues with them other than unit tests for SLRU [1] should be merged
first.

I suggest we use "git link" for the larger patchset in the other
thread since I'm not contributing to it right now and all in all that
thread is waiting for this one. For this thread we continue using
patches since several people contribute to them.

[1]: https://commitfest.postgresql.org/38/3608/

-- 
Best regards,
Aleksander Alekseev

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Pavel Borisov]

>
> Is there any reason to continue with two separate threads and CF entries ?
> The original reason was to have a smaller patch for considerate late in
> v15.
>
> But right now, it just seems to cause every update to imply two email
> messages
> rather than one.
>
> Since the patch is split into 0001, 0002, 0003, 0004+, both can continue
> in the
> main thread.  The early patches can still be applied independently from
> each
> later patch (the same as with any other patch series).
>

I see the main goal of this split is to make discussion of this (easier)
thread separate to the discussion of a whole patchset which is expected to
be more thorough.

Also I see the chances of this thread to be committed into v16 to be much
higher than of a main patch, which will be for v17 then.

Thanks for the advice to add git thread instead of patch posting. Will try
to do this.
-- 
Best regards,
Pavel Borisov

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Justin Pryzby]

On Fri, May 13, 2022 at 04:21:29PM +0300, Maxim Orlov wrote:
> We have posted an updated version v34 of the whole patchset in [1].
> Changes of patches 0001-0003 there are identical to v33. So, no update is
> needed in this thread.

Is there any reason to continue with two separate threads and CF entries ?
The original reason was to have a smaller patch for considerate late in v15.

But right now, it just seems to cause every update to imply two email messages
rather than one.

Since the patch is split into 0001, 0002, 0003, 0004+, both can continue in the
main thread.  The early patches can still be applied independently from each
later patch (the same as with any other patch series).

Also, since this patch series is large, and expects a lot of conflicts, it
seems better to update the cfapp with a "git link" [0] where you can maintain a
rebased branch.  It avoids the need to mail new patches to the list several
times more often than it's being reviewed.

[0] Note that cirrusci will run the same checks as cfbot if you push a branch
to github.

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Maxim Orlov]

> here is the rebased v32 version of the patch.
>
> The patchset rotted a bit. Here is a rebased version.
>

We have posted an updated version v34 of the whole patchset in [1].
Changes of patches 0001-0003 there are identical to v33. So, no update is
needed in this thread.

[1]
https://www.postgresql.org/message-id/flat/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com

-- 
Best regards,
Maxim Orlov.

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Aleksander Alekseev]

Hi Peter,

> That isn't really what I meant.  When I said
>
>  > At this point, I'm more concerned that code review is still finding
>  > bugs, and that we have no test coverage for any of this
>
> this meant especially the SLRU refactoring patch.

Got it, and sorry for the confusion. I decided to invest some time
into improving the SLRU test coverage. I created a new thread, please
join the discussion [1].

Maxim, Pavel and I agreed (offlist) that I will rebase v30 patchset if
[1] will be merged.

[1]: 
https://postgr.es/m/CAJ7c6TOFoWcHOW4BVe3BG_uikCrO9B91ayx9d6rh5JZr_tPESg%40mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Peter Eisentraut]

On 29.03.22 15:09, Aleksander Alekseev wrote:

Hi hackers,


I took another look at the patchset. Personally I don't think it will get much
better than it is now. I'm inclined to change the status of the CF entry to
"Ready for Committer" unless anyone disagrees.



About v25-0002-Use-64-bit-format-to-output-XIDs.patch:
I don't see the point of applying this now. It doesn't make PG15 any
better. It's just a patch part of which we might need later.


AFAICT, this patch provides no actual functionality change, so holding
it a bit for early in the PG16 cycle wouldn't lose anything.


OK. As I understand we still have a consensus that v30-0001 (SLRU refactoring,
not the XID formatting) is targeting PG15, so I changed the CF entry to
"Ready for Committer" for this single patch. I rechecked it again on the
current `master` branch without the other patches and it is OK. cfbot is happy
with the patchset as well.


That isn't really what I meant.  When I said

> At this point, I'm more concerned that code review is still finding
> bugs, and that we have no test coverage for any of this

this meant especially the SLRU refactoring patch.

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Aleksander Alekseev]

Hi hackers,

> I took another look at the patchset. Personally I don't think it will get much
> better than it is now. I'm inclined to change the status of the CF entry to
> "Ready for Committer" unless anyone disagrees.

> > About v25-0002-Use-64-bit-format-to-output-XIDs.patch:
> > I don't see the point of applying this now. It doesn't make PG15 any
> > better. It's just a patch part of which we might need later.
>
> AFAICT, this patch provides no actual functionality change, so holding
> it a bit for early in the PG16 cycle wouldn't lose anything.

OK. As I understand we still have a consensus that v30-0001 (SLRU refactoring,
not the XID formatting) is targeting PG15, so I changed the CF entry to
"Ready for Committer" for this single patch. I rechecked it again on the
current `master` branch without the other patches and it is OK. cfbot is happy
with the patchset as well.

-- 
Best regards,
Aleksander Alekseev

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Peter Eisentraut]

On 28.03.22 12:46, Aleksander Alekseev wrote:

Personally I don't have a strong opinion here. Merging the patch sooner will
allow us to move toward 64-bit XIDs faster (e.g. to gather the feedback from
the early adopters, allow the translators to do their thing earlier, etc).
Merging it later will make PG15 more stable (you can't break anything new if
you don't change anything). As always, engineering is all about compromises.


At this point, I'm more concerned that code review is still finding 
bugs, and that we have no test coverage for any of this, so we are 
supposed to gain confidence in this patch by staring at it very hard. ;-)


AFAICT, this patch provides no actual functionality change, so holding 
it a bit for early in the PG16 cycle wouldn't lose anything.

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Aleksander Alekseev]

Hi hackers,

> Here is v30.

I took another look at the patchset. Personally I don't think it will get much
better than it is now. I'm inclined to change the status of the CF entry to
"Ready for Committer" unless anyone disagrees.

cfbot reports a problem with t/013_partition.pl but the test seems to be flaky
on `master` [1]. I couldn't find anything useful in the logs except for
"[postmaster] LOG:  received immediate shutdown request". Then I re-checked the
patchset on FreeBSD 13 locally. The patchset passed `make installcked-world`.

> About v25-0002-Use-64-bit-format-to-output-XIDs.patch:
> I don't see the point of applying this now. It doesn't make PG15 any
> better. It's just a patch part of which we might need later.

> It was proposed in [1] that we'd better cut it into separate threads and
> commit by parts, some into v15, the other into v16. So we did. In view of
> this, I can not accept that 0002 patch doesn't make v15 better. I consider
> it is separate enough to be committed as a base to further 64xid parts.

I understand how disappointing this may be.

Personally I don't have a strong opinion here. Merging the patch sooner will
allow us to move toward 64-bit XIDs faster (e.g. to gather the feedback from
the early adopters, allow the translators to do their thing earlier, etc).
Merging it later will make PG15 more stable (you can't break anything new if
you don't change anything). As always, engineering is all about compromises.

It's up to the committer to decide.

[1]: 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=frogfish=2022-03-25%2018%3A37%3A10

--
Best regards,
Aleksander Alekseev

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Pavel Borisov]

Just forgot to mention that answers in a previous message are describing
the changes that are in v26.

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Pavel Borisov]

Hi, Peter!
Thanks for your review!

About v25-0001-Use-unsigned-64-bit-numbering-of-SLRU-pages.patch:
>
> -static bool CLOGPagePrecedes(int page1, int page2);
> +static bool CLOGPagePrecedes(uint64 page1, uint64 page2);
>
> You are changing the API from signed to unsigned.  Is this intentional?
> It is not explained anywhere.  Are we sure that nothing uses, for
> example, negative values as error markers?
>
Initially, we've made SLRU pages to be int64, and reworked them into uint64
as per Andres Freund's proposal. It is not necessary for a 64xid patchset
though as maximum page number is at least several (>2) times less than the
maximum 64bit xid value. So we've returned them to be signed int64. I don't
see the reason why make SRLU unsigned for introducing 64bit xids.


>   #define SlruFileName(ctl, path, seg) \
> -   snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir, seg)
> +   snprintf(path, MAXPGPATH, "%s/%04X%08X", (ctl)->Dir, \
> +(uint32) ((seg) >> 32), (uint32) ((seg) &
> (uint64)0x))

What's going on here?  Some explanation?  Why not use something like
> %llX or whatever you need?
>
Of course, this should be simplified as %012llX and we will do this in
later stage (in 0006 patch in 64xid thread) as this should be done together
with CLOG pg_upgrade. So we've returned this to the initial state in 0001.
Thanks for the notion!

+   uint64  segno = pageno / SLRU_PAGES_PER_SEGMENT;
> +   uint64  rpageno = pageno % SLRU_PAGES_PER_SEGMENT;
> [etc.]
>
> Not clear whether segno etc. need to be changed to 64 bits, or whether
> an increase of SLRU_PAGES_PER_SEGMENT should also be considered.
>
Yes, segno should be 64bits because even multiple of SLRU_PAGES_PER_SEGMENT
and CLOG_XACTS_PER_PAGE (and similar for commit_ts and mxact) is far less
than 2^32 and the overall length of clog/commit_ts/mxact is 64bit.


> -   if ((len == 4 || len == 5 || len == 6) &&
> +   if ((len == 12 || len == 13 || len == 14) &&
>
> This was horrible before, but maybe we can take this opportunity now to
> add a comment?
>
This should also be introduced later together with SlruFileName changes. So
we've removed this change from 0001. Later in 0006 we'll add this with
comments.

-   SlruFileName(ctl, path, ftag->segno);
> +   SlruFileName(ctl, path, (uint64) ftag->segno);
>
> Maybe ftag->segno should be changed to 64 bits as well?  Not clear.
>
Same as previous.


> There is a code comment at the definition of SLRU_PAGES_PER_SEGMENT that
> has some detailed explanations of how the SLRU numbering, SLRU file
> names, and transaction IDs tie together.  This doesn't seem to apply
> anymore after this change.
>
Same as previous.

The reference page of pg_resetwal contains various pieces of information
> of how to map SLRU files back to transaction IDs.  Please check if that
> is still all up to date.
>
Same as previous.

About v25-0002-Use-64-bit-format-to-output-XIDs.patch:
>
> I don't see the point of applying this now.  It doesn't make PG15 any
> better.  It's just a patch part of which we might need later.
> Especially the issue of how we are handwaving past the epoch-enabled
> output sites disturbs me.  At least those should be omitted from this
> patch, since this patch makes these more wrong, not more right for the
> future.

 psprintf("xmax %u equals or exceeds next valid transaction ID %u:%u",
> -  xmax,
> + psprintf("xmax %llu equals or exceeds next valid transaction ID %u:%llu",
> +  (unsigned long long) xmax,
> EpochFromFullTransactionId(ctx->next_fxid),
> -  XidFromFullTransactionId(ctx->next_fxid)));
> +  (unsigned long long) XidFromFullTransactionId(ctx->
> next_fxid)));


> This %u:%u business is basically an existing workaround for the lack of
> 64-bit transaction identifiers.  Presumably, when those are available,
> all of this will be replaced by a single number display, possibly a
> single %llu.  So these sites you change here will have to be touched
> again, and so changing this now doesn't make sense.  At least that's my
> guess.  Maybe there needs to be a fuller explanation of how this is
> meant to be transitioned.

Fixed, thanks.

An alternative we might want to consider is that we use PRId64 as
> explained here:
> <
> https://www.gnu.org/software/gettext/manual/html_node/Preparing-Strings.html>.
>
>   We'd have to check whether we still support any non-GNU gettext
> implementations and to what extent they support this.  But I think it's
> something to think about if we are going to embark on a journey of much
> more int64 printf output.


There were several other ways that have met opposition above in the thread.
I guess PRId64 will also be opposed as not portable enough. Personally, I
don't see much trouble when we cast the value to be printed to more wide
value and consider this the best choice of all that was discussed. We just
stick to a portable way of printing which was recommended by Tom and in
agreement with 

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Peter Eisentraut]

On 23.03.22 10:51, Maxim Orlov wrote:

Thanks for updating the patchs. I have a little comment on 0002.

+                                errmsg_internal("found xmax %llu" "
(infomask 0x%04x) not frozen, not multi, not normal",
+   
(unsigned long long) xid, tuple->t_infomask)));



Thanks for your review! Fixed.


About v25-0001-Use-unsigned-64-bit-numbering-of-SLRU-pages.patch:

-static bool CLOGPagePrecedes(int page1, int page2);
+static bool CLOGPagePrecedes(uint64 page1, uint64 page2);

You are changing the API from signed to unsigned.  Is this intentional? 
It is not explained anywhere.  Are we sure that nothing uses, for 
example, negative values as error markers?


 #define SlruFileName(ctl, path, seg) \
-   snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir, seg)
+   snprintf(path, MAXPGPATH, "%s/%04X%08X", (ctl)->Dir, \
+(uint32) ((seg) >> 32), (uint32) ((seg) & 
(uint64)0x))


What's going on here?  Some explanation?  Why not use something like 
%llX or whatever you need?


+   uint64  segno = pageno / SLRU_PAGES_PER_SEGMENT;
+   uint64  rpageno = pageno % SLRU_PAGES_PER_SEGMENT;
[etc.]

Not clear whether segno etc. need to be changed to 64 bits, or whether 
an increase of SLRU_PAGES_PER_SEGMENT should also be considered.


-   if ((len == 4 || len == 5 || len == 6) &&
+   if ((len == 12 || len == 13 || len == 14) &&

This was horrible before, but maybe we can take this opportunity now to 
add a comment?


-   SlruFileName(ctl, path, ftag->segno);
+   SlruFileName(ctl, path, (uint64) ftag->segno);

Maybe ftag->segno should be changed to 64 bits as well?  Not clear.

There is a code comment at the definition of SLRU_PAGES_PER_SEGMENT that 
has some detailed explanations of how the SLRU numbering, SLRU file 
names, and transaction IDs tie together.  This doesn't seem to apply 
anymore after this change.


The reference page of pg_resetwal contains various pieces of information 
of how to map SLRU files back to transaction IDs.  Please check if that 
is still all up to date.



About v25-0002-Use-64-bit-format-to-output-XIDs.patch:

I don't see the point of applying this now.  It doesn't make PG15 any 
better.  It's just a patch part of which we might need later. 
Especially the issue of how we are handwaving past the epoch-enabled 
output sites disturbs me.  At least those should be omitted from this 
patch, since this patch makes these more wrong, not more right for the 
future.


An alternative we might want to consider is that we use PRId64 as 
explained here: 
. 
 We'd have to check whether we still support any non-GNU gettext 
implementations and to what extent they support this.  But I think it's 
something to think about if we are going to embark on a journey of much 
more int64 printf output.

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Japin Li]

On Wed, 23 Mar 2022 at 01:22, Maxim Orlov  wrote:
> Hi!
>
> Here is v24. Changes are:
> - correct commit messages for 0001 and 0002
> - use uint64 for SLRU page numbering instead of int64 in v23
> - fix code formatting and indent
> - and minor fixes in slru.c
>
> Reviews are very welcome!
>

Thanks for updating the patchs. I have a little comment on 0002.

+errmsg_internal("found xmax %llu" " (infomask 
0x%04x) not frozen, not multi, not normal",
+(unsigned long 
long) xid, tuple->t_infomask)));

IMO, we can remove the double quote inside the sentence.

 errmsg_internal("found xmax %llu (infomask 
0x%04x) not frozen, not multi, not normal",
 (unsigned long 
long) xid, tuple->t_infomask)));

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Aleksander Alekseev]

Hi hackers,

> Here is v23. As was suggested by Alexander above, I've changed the order of 
> the patches and improved the commit message. Now, SLRU patch is the first.

Many thanks!

> There were proposals to make use of SLRU pages numbers that are in fact 
> unsigned and change from int to uint64. I fully support this, but I'm not 
> sure this big SLRU refactoring should be done in this patchset.

If it takes a lot of effort and doesn't bring us any closer to 64-bit
XIDs, I suggest not doing this in v23-0001. I can invest some time
into this refactoring in April and create a separate CF entry, if
someone will second the idea.

> In general, I consider this patchset is ready to commit. It would be great to 
> deliver it in PG15.

+1.

v23-0002 seems to have two extra sentences in the commit message that
are outdated, but this is a minor issue. The commit message should be:

"""
Replace the %u formatting string for XIDs with %llu and cast to
unsigned long long. While actually XIDs are still 32 bit, this patch
completely supports both 32 and 64 bit.
"""

Since Peter expressed some concerns regarding v23-0002, maybe we
should discuss it a bit more. Although personally I doubt that we can
do much better than that, and as I recall this particular change was
explicitly requested by several people.

-- 
Best regards,
Aleksander Alekseev

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Pavel Borisov]

>
> > I think there needs to be a bit more soul searching here on how to
> > handle that in the future and how to transition it.  I don't think
> > targeting this patch for PG15 is useful at this point.
>
> The patches can be reordered so that we are still able to deliver SLRU
> refactoring in PG15.
>
Sure.

> > As a more general point, I don't like plastering these bulky casts all
> > over the place.  Casts hide problems.
>
> Regarding the casts, I don't like them either. I agree that it could
> be a good idea to invest a little more time into figuring out if this
> transit can be handled in a better way and deliver this change in the
> next CF. However, if no one will be able to suggest a better
> alternative, I think we should continue making progress here. The
> 32-bit XIDs are a major inconvenience for many users.
>

I'd like to add that the initial way of shifting to 64bit was based on
XID_FMT in a print formatting template which could be changed from 32 to 64
bit when shifting to 64-bit xids later. But this template is not
localizable so hackers recommended using %lld/%llu with (long
long)/(unsigned long long cast) which is a current best practice elsewhere
in the code (e.g. recent 1f8bc448680bf93a9). So I suppose we already have a
good enough way to stick to.

This approach in 0001 inherently processes both 32/64 bit xids and should
not change with later committing 64bit xids later (
https://postgr.es/m/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com
)

The thing that needs to change then is suppressing output of Epoch. It
should be done when 64-xids are committed and it is done by 0006 in the
mentioned thread. Until that I've left Epoch in the output.

Big thanks for your considerations!

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Aleksander Alekseev]

Hi Peter,

> I think there needs to be a bit more soul searching here on how to
> handle that in the future and how to transition it.  I don't think
> targeting this patch for PG15 is useful at this point.

The patches can be reordered so that we are still able to deliver SLRU
refactoring in PG15.

> As a more general point, I don't like plastering these bulky casts all
> over the place.  Casts hide problems.

Regarding the casts, I don't like them either. I agree that it could
be a good idea to invest a little more time into figuring out if this
transit can be handled in a better way and deliver this change in the
next CF. However, if no one will be able to suggest a better
alternative, I think we should continue making progress here. The
32-bit XIDs are a major inconvenience for many users.

-- 
Best regards,
Aleksander Alekseev

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Peter Eisentraut]

On 18.03.22 16:14, Maxim Orlov wrote:
Here is v22. It addresses things mentioned by Tom and Kyotaro. Also 
added Aleksander's changes from v21.


The v22-0002-Update-XID-formatting-in-the-.po-files.patch is not 
necessary.  That is done by a separate procedure.


I'm wondering about things like this:

- psprintf("xmax %u equals or exceeds next valid transaction ID %u:%u",
-  xmax,
+ psprintf("xmax %llu equals or exceeds next valid transaction ID %u:%llu",
+  (unsigned long long) xmax,
   EpochFromFullTransactionId(ctx->next_fxid),
-  XidFromFullTransactionId(ctx->next_fxid)));
+  (unsigned long long) XidFromFullTransactionId(ctx->next_fxid)));

This %u:%u business is basically an existing workaround for the lack of 
64-bit transaction identifiers.  Presumably, when those are available, 
all of this will be replaced by a single number display, possibly a 
single %llu.  So these sites you change here will have to be touched 
again, and so changing this now doesn't make sense.  At least that's my 
guess.  Maybe there needs to be a fuller explanation of how this is 
meant to be transitioned.


As a more general point, I don't like plastering these bulky casts all 
over the place.  Casts hide problems.  For example, if we currently have


elog(LOG, "xid is %u", xid);

and then xid is changed to a 64-bit type, this will give a compiler 
warning until you change the format.  If we add a (long long unsigned) 
cast here now and then somehow forget to change the type of xid, nothing 
will warn us about that.  (Note that there is also third-party code 
affected by this.)  Besides, these casts are quite ugly anyway, and I 
don't think the solution for all time should be that we have to add 
these casts just to print xids.


I think there needs to be a bit more soul searching here on how to 
handle that in the future and how to transition it.  I don't think 
targeting this patch for PG15 is useful at this point.

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Pavel Borisov]

> On 2022-03-18 18:14:52 +0300, Maxim Orlov wrote:
> > Subject: [PATCH v22 3/6] Use 64-bit pages in SLRU
> >
> > This is one step toward 64-bit XIDs.
>
> I think this should explain in more detail why this move is done. Neither
> the
> commit message nor the rest of the thread does so afaics. It's not too
> hard to
> infer, but the central reason behind a patch shouldn't need to be inferred.
>
>
> > -static bool CLOGPagePrecedes(int page1, int page2);
> > +static bool CLOGPagePrecedes(int64 page1, int64 page2);
>
> I think all of these are actually unsigned integers. If all of this stuff
> gets
> touched, perhaps worth moving to uint64 instead?
>
>
> https://www.postgresql.org/message-id/20220318231430.m5g56yk4ztlz2man%40alap3.anarazel.de


We'll try to add these and many similar changes in Slru code, thanks!

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Andres Freund]

Hi,

On 2022-03-18 18:14:52 +0300, Maxim Orlov wrote:
> Subject: [PATCH v22 3/6] Use 64-bit pages in SLRU
> 
> This is one step toward 64-bit XIDs.

I think this should explain in more detail why this move is done. Neither the
commit message nor the rest of the thread does so afaics. It's not too hard to
infer, but the central reason behind a patch shouldn't need to be inferred.


> -static bool CLOGPagePrecedes(int page1, int page2);
> +static bool CLOGPagePrecedes(int64 page1, int64 page2);

I think all of these are actually unsigned integers. If all of this stuff gets
touched, perhaps worth moving to uint64 instead?

https://www.postgresql.org/message-id/20220318231430.m5g56yk4ztlz2man%40alap3.anarazel.de

Greetings,

Andres Freund

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Pavel Borisov]

>
> > To me personally v21 looks almost OK.
>
> For some reason I didn't receive the recent e-mails from Tom and Kyotaro.
> I've just discovered them accidentally by reading the thread through the
> web interface. These comments were not addressed in v21.
>
Aleksander, as of now we're preparing a new version that addresses a thing
mentioned by Tom We'll try to add what you've done in v21, but
please check. We're going to send a patch very soon, most probably today in
several hours.

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Aleksander Alekseev]

Hi hackers,

> Here is an new version with the following changes compared to v20:
> [...]
> To me personally v21 looks almost OK.

For some reason I didn't receive the recent e-mails from Tom and Kyotaro.
I've just discovered them accidentally by reading the thread through the
web interface. These comments were not addressed in v21.

-- 
Best regards,
Aleksander Alekseev

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Pavel Borisov]

>
> On Thu, 17 Mar 2022 at 21:31, Alexander Korotkov 
> wrote:
> > On Thu, Mar 17, 2022 at 4:23 PM Peter Eisentraut
> >  wrote:
> >> On 17.03.22 14:12, Aleksander Alekseev wrote:
> >> > v19-0001 changes the format string for XIDs from %u to XID_FMT. This
> >> > refactoring allows us to switch to UINT64_FORMAT by changing one
> >> > #define in the future patches.
> >> >
> >> > Kyotaro suggested using `errmsg("blah blah  %lld ..", (long long)
> >> > xid)` instead in order to simplify localization of the error messages.
> >> > Personally I don't have a strong opinion here. Either approach will
> >> > work and will affect the error messages eventually. Please let us know
> >> > what you think.
> >>
> >> This is not a question of simplification.  Translatable messages with
> >> embedded macros won't work.  This patch isn't going to be acceptable.
> >
> > I've suspected this, but wasn't sure.  Thank you for clarification.
>
Hi, hackers!

The need to support localization is very much understood by us. We'll
deliver a patchset soon with localization based on %lld/%llu format and
explicit casts to unsigned/signed long long.
Alexander Alexeev could you wait a little bit and give us time to deliver
v20 patch which will address localization (I propose concurrent work should
stop until that to avoid conflicts)
Advice and discussion help us a lot.

Thanks!


-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com