[PATCH 1/1] Fix printing last progress report line in client programs.

public inbox for [email protected]  
help / color / mirror / Atom feed

[PATCH 1/1] Fix printing last progress report line in client programs.
24+ messages / 4 participants
[nested] [flat]

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* [PATCH 1/1] Fix printing last progress report line in client programs.
@ 2020-08-12 20:27  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw)

A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
 src/bin/pg_checksums/pg_checksums.c | 3 ++-
 src/bin/pg_rewind/pg_rewind.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
 		if (showprogress)
 		{
 			progress_report(true);
-			fprintf(stderr, "\n");	/* Need to move to next line */
+			if (isatty(fileno(stderr)))
+				fprintf(stderr, "\n");	/* Need to move to next line */
 		}
 
 		printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
 	executeFileMap();
 
 	progress_report(true);
-	printf("\n");
+	if (showprogress && isatty(fileno(stderr)))
+		fprintf(stderr, "\n");	/* Need to move to next line */
 
 	if (showprogress)
 		pg_log_info("creating backup label and updating control file");
-- 
2.20.1


--------------D7F229EC53544D857CE3A506--





^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* Re: Introduce XID age and inactive timeout based replication slot invalidation
@ 2024-02-20 06:35  Bharath Rupireddy <[email protected]>
  0 siblings, 1 reply; 24+ messages in thread

From: Bharath Rupireddy @ 2024-02-20 06:35 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

On Fri, Feb 9, 2024 at 1:12 PM Bertrand Drouvot
<[email protected]> wrote:
>
> I think "conflict" is an important topic and does contain several reasons. The
> slot "first" conflict and then leads to slot "invalidation".
>
> > They both are the same internally, so why
> > confuse the users?
>
> I don't think that would confuse the users, I do think that would be easier to
> check for conflicting slots.

I've added a separate column for invalidation reasons for now. I'll
see how others think on this as the time goes by.

> I did not look closely at the code, just played a bit with the patch and was able
> to produce something like:
>
> postgres=# select slot_name,slot_type,active,active_pid,wal_status,invalidation_reason from pg_replication_slots;
>   slot_name  | slot_type | active | active_pid | wal_status | invalidation_reason
> -------------+-----------+--------+------------+------------+---------------------
>  rep1        | physical  | f      |            | reserved   |
>  master_slot | physical  | t      |    1482441 | unreserved | wal_removed
> (2 rows)
>
> does that make sense to have an "active/working" slot "ivalidated"?

Thanks. Can you please provide the steps to generate this error? Are
you setting max_slot_wal_keep_size on primary to generate
"wal_removed"?

Attached v5 patch set after rebasing and addressing review comments.
Please review it further.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com


Attachments:

  [application/octet-stream] v5-0001-Track-invalidation_reason-in-pg_replication_slots.patch (6.3K, ../../CALj2ACWgACB4opnbqi=x7Hc4aqcgkXoLsh1VB+gfidXaDQNu_Q@mail.gmail.com/2-v5-0001-Track-invalidation_reason-in-pg_replication_slots.patch)
  download | inline diff:
From 7aac2652261475cce0dcffd359f17bdbc25ac418 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Tue, 20 Feb 2024 05:40:44 +0000
Subject: [PATCH v5 1/4] Track invalidation_reason in pg_replication_slots

Currently the reason for replication slot invalidation is not
tracked in pg_replication_slots. A recent commit 007693f2a added
conflict_reason to show the reasons for slot invalidation, but
only for logical slots. This commit adds invalidation_reason to
pg_replication_slots to show invalidation reasons for both
physical and logical slots.
---
 doc/src/sgml/system-views.sgml       | 32 ++++++++++++++++++++++++++++
 src/backend/catalog/system_views.sql |  3 ++-
 src/backend/replication/slotfuncs.c  | 21 +++++++++++++++++-
 src/include/catalog/pg_proc.dat      |  6 +++---
 src/test/regress/expected/rules.out  |  5 +++--
 5 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index be90edd0e2..cce88c14bb 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -2581,6 +2581,38 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
       </para></entry>
      </row>
 
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>invalidation_reason</structfield> <type>text</type>
+      </para>
+      <para>
+       The reason for the slot's invalidation. <literal>NULL</literal> if the
+       slot is currently actively being used. The non-NULL values indicate that
+       the slot is marked as invalidated. Possible values are:
+       <itemizedlist spacing="compact">
+        <listitem>
+         <para>
+          <literal>wal_removed</literal> means that the required WAL has been
+          removed.
+         </para>
+        </listitem>
+        <listitem>
+         <para>
+          <literal>rows_removed</literal> means that the required rows have
+          been removed.
+         </para>
+        </listitem>
+        <listitem>
+         <para>
+          <literal>wal_level_insufficient</literal> means that the
+          primary doesn't have a <xref linkend="guc-wal-level"/> sufficient to
+          perform logical decoding.
+         </para>
+        </listitem>
+       </itemizedlist>
+      </para></entry>
+     </row>
+
     </tbody>
    </tgroup>
   </table>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 04227a72d1..c39f0d73d3 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1025,7 +1025,8 @@ CREATE VIEW pg_replication_slots AS
             L.two_phase,
             L.conflict_reason,
             L.failover,
-            L.synced
+            L.synced,
+            L.invalidation_reason
     FROM pg_get_replication_slots() AS L
             LEFT JOIN pg_database D ON (L.datoid = D.oid);
 
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index d2fa5e669a..472248e569 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -239,7 +239,7 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
 Datum
 pg_get_replication_slots(PG_FUNCTION_ARGS)
 {
-#define PG_GET_REPLICATION_SLOTS_COLS 17
+#define PG_GET_REPLICATION_SLOTS_COLS 18
 	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
 	XLogRecPtr	currlsn;
 	int			slotno;
@@ -437,6 +437,25 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 
 		values[i++] = BoolGetDatum(slot_contents.data.synced);
 
+		switch (slot_contents.data.invalidated)
+		{
+			case RS_INVAL_NONE:
+				nulls[i++] = true;
+				break;
+
+			case RS_INVAL_WAL_REMOVED:
+				values[i++] = CStringGetTextDatum(SLOT_INVAL_WAL_REMOVED_TEXT);
+				break;
+
+			case RS_INVAL_HORIZON:
+				values[i++] = CStringGetTextDatum(SLOT_INVAL_HORIZON_TEXT);
+				break;
+
+			case RS_INVAL_WAL_LEVEL:
+				values[i++] = CStringGetTextDatum(SLOT_INVAL_WAL_LEVEL_TEXT);
+				break;
+		}
+
 		Assert(i == PG_GET_REPLICATION_SLOTS_COLS);
 
 		tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9c120fc2b7..a6bfc36426 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -11127,9 +11127,9 @@
   proname => 'pg_get_replication_slots', prorows => '10', proisstrict => 'f',
   proretset => 't', provolatile => 's', prorettype => 'record',
   proargtypes => '',
-  proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,text,bool,bool}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
-  proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,conflict_reason,failover,synced}',
+  proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,text,bool,bool,text}',
+  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
+  proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,conflict_reason,failover,synced,invalidation_reason}',
   prosrc => 'pg_get_replication_slots' },
 { oid => '3786', descr => 'set up a logical replication slot',
   proname => 'pg_create_logical_replication_slot', provolatile => 'v',
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index b7488d760e..0646874236 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1475,8 +1475,9 @@ pg_replication_slots| SELECT l.slot_name,
     l.two_phase,
     l.conflict_reason,
     l.failover,
-    l.synced
-   FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, conflict_reason, failover, synced)
+    l.synced,
+    l.invalidation_reason
+   FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, conflict_reason, failover, synced, invalidation_reason)
      LEFT JOIN pg_database d ON ((l.datoid = d.oid)));
 pg_roles| SELECT pg_authid.rolname,
     pg_authid.rolsuper,
-- 
2.34.1



  [application/octet-stream] v5-0002-Add-XID-based-replication-slot-invalidation.patch (13.4K, ../../CALj2ACWgACB4opnbqi=x7Hc4aqcgkXoLsh1VB+gfidXaDQNu_Q@mail.gmail.com/3-v5-0002-Add-XID-based-replication-slot-invalidation.patch)
  download | inline diff:
From cdd1fcb57b757c2f46eb3c398a29bcde47761f6e Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Tue, 20 Feb 2024 05:43:58 +0000
Subject: [PATCH v5 2/4] Add XID based replication slot invalidation

Currently postgres has the ability to invalidate inactive
replication slots based on the amount of WAL (set via
max_slot_wal_keep_size GUC) that will be needed for the slots in
case they become active. However, choosing a default value for
max_slot_wal_keep_size is tricky. Because the amount of WAL a
customer generates, and their allocated storage will vary greatly
in production, making it difficult to pin down a one-size-fits-all
value. It is often easy for developers to set an XID age (age of
slot's xmin or catalog_xmin) of say 1 or 1.5 billion, after which
the slots get invalidated.

To achieve the above, postgres uses replication slot xmin (the
oldest transaction that this slot needs the database to retain) or
catalog_xmin (the oldest transaction affecting the system catalogs
that this slot needs the database to retain), and a new GUC
max_slot_xid_age. The checkpointer then looks at all replication
slots invalidating the slots based on the age set.
---
 doc/src/sgml/config.sgml                      |  21 ++++
 src/backend/access/transam/xlog.c             |  10 ++
 src/backend/replication/slot.c                |  43 +++++++
 src/backend/replication/slotfuncs.c           |   8 ++
 src/backend/utils/misc/guc_tables.c           |  10 ++
 src/backend/utils/misc/postgresql.conf.sample |   1 +
 src/include/replication/slot.h                |   4 +
 src/test/recovery/t/050_invalidate_slots.pl   | 108 ++++++++++++++++++
 8 files changed, 205 insertions(+)
 create mode 100644 src/test/recovery/t/050_invalidate_slots.pl

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index ffd711b7f2..6c1c5f421f 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4405,6 +4405,27 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-max-slot-xid-age" xreflabel="max_slot_xid_age">
+      <term><varname>max_slot_xid_age</varname> (<type>integer</type>)
+      <indexterm>
+       <primary><varname>max_slot_xid_age</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Invalidate replication slots whose <literal>xmin</literal> (the oldest
+        transaction that this slot needs the database to retain) or
+        <literal>catalog_xmin</literal> (the oldest transaction affecting the
+        system catalogs that this slot needs the database to retain) has reached
+        the age specified by this setting. A value of zero (which is default)
+        disables this feature. Users can set this value anywhere from zero to
+        two billion. This parameter can only be set in the
+        <filename>postgresql.conf</filename> file or on the server command
+        line.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-track-commit-timestamp" xreflabel="track_commit_timestamp">
       <term><varname>track_commit_timestamp</varname> (<type>boolean</type>)
       <indexterm>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 50c347a679..5fe76c46a1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7159,6 +7159,11 @@ CreateCheckPoint(int flags)
 	if (PriorRedoPtr != InvalidXLogRecPtr)
 		UpdateCheckPointDistanceEstimate(RedoRecPtr - PriorRedoPtr);
 
+	/* Invalidate replication slots based on xmin or catalog_xmin age */
+	if (max_slot_xid_age > 0)
+		InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0,
+										   InvalidOid, InvalidTransactionId);
+
 	/*
 	 * Delete old log files, those no longer needed for last checkpoint to
 	 * prevent the disk holding the xlog from growing full.
@@ -7603,6 +7608,11 @@ CreateRestartPoint(int flags)
 	 */
 	XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size);
 
+	/* Invalidate replication slots based on xmin or catalog_xmin age */
+	if (max_slot_xid_age > 0)
+		InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0,
+										   InvalidOid, InvalidTransactionId);
+
 	/*
 	 * Retreat _logSegNo using the current end of xlog replayed or received,
 	 * whichever is later.
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index a142855bd3..67d6bd849e 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -102,6 +102,7 @@ ReplicationSlot *MyReplicationSlot = NULL;
 /* GUC variable */
 int			max_replication_slots = 10; /* the maximum number of replication
 										 * slots */
+int			max_slot_xid_age = 0;
 
 static void ReplicationSlotShmemExit(int code, Datum arg);
 static void ReplicationSlotDropPtr(ReplicationSlot *slot);
@@ -1416,6 +1417,9 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause,
 		case RS_INVAL_WAL_LEVEL:
 			appendStringInfoString(&err_detail, _("Logical decoding on standby requires wal_level >= logical on the primary server."));
 			break;
+		case RS_INVAL_XID_AGE:
+			appendStringInfoString(&err_detail, _("The replication slot's xmin or catalog_xmin reached the age specified by max_slot_xid_age."));
+			break;
 		case RS_INVAL_NONE:
 			pg_unreachable();
 	}
@@ -1532,6 +1536,42 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
 					if (SlotIsLogical(s))
 						conflict = cause;
 					break;
+				case RS_INVAL_XID_AGE:
+					{
+						TransactionId xid_cur = ReadNextTransactionId();
+						TransactionId xid_limit;
+						TransactionId xid_slot;
+
+						if (TransactionIdIsNormal(s->data.xmin))
+						{
+							xid_slot = s->data.xmin;
+
+							xid_limit = xid_slot + max_slot_xid_age;
+							if (xid_limit < FirstNormalTransactionId)
+								xid_limit += FirstNormalTransactionId;
+
+							if (TransactionIdFollowsOrEquals(xid_cur, xid_limit))
+							{
+								conflict = cause;
+								break;
+							}
+						}
+						if (TransactionIdIsNormal(s->data.catalog_xmin))
+						{
+							xid_slot = s->data.catalog_xmin;
+
+							xid_limit = xid_slot + max_slot_xid_age;
+							if (xid_limit < FirstNormalTransactionId)
+								xid_limit += FirstNormalTransactionId;
+
+							if (TransactionIdFollowsOrEquals(xid_cur, xid_limit))
+							{
+								conflict = cause;
+								break;
+							}
+						}
+					}
+					break;
 				case RS_INVAL_NONE:
 					pg_unreachable();
 			}
@@ -1686,6 +1726,7 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
  * - RS_INVAL_HORIZON: requires a snapshot <= the given horizon in the given
  *   db; dboid may be InvalidOid for shared relations
  * - RS_INVAL_WAL_LEVEL: is logical
+ * - RS_INVAL_XID_AGE: slot's xmin or catalog_xmin has reached the age
  *
  * NB - this runs as part of checkpoint, so avoid raising errors if possible.
  */
@@ -2304,6 +2345,8 @@ GetSlotInvalidationCause(char *conflict_reason)
 		return RS_INVAL_HORIZON;
 	else if (strcmp(conflict_reason, SLOT_INVAL_WAL_LEVEL_TEXT) == 0)
 		return RS_INVAL_WAL_LEVEL;
+	else if (strcmp(conflict_reason, SLOT_INVAL_XID_AGED_TEXT) == 0)
+		return RS_INVAL_XID_AGE;
 	else
 		Assert(0);
 
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 472248e569..7c1145bb75 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -430,6 +430,10 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 				case RS_INVAL_WAL_LEVEL:
 					values[i++] = CStringGetTextDatum(SLOT_INVAL_WAL_LEVEL_TEXT);
 					break;
+
+				case RS_INVAL_XID_AGE:
+					values[i++] = CStringGetTextDatum(SLOT_INVAL_XID_AGED_TEXT);
+					break;
 			}
 		}
 
@@ -454,6 +458,10 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 			case RS_INVAL_WAL_LEVEL:
 				values[i++] = CStringGetTextDatum(SLOT_INVAL_WAL_LEVEL_TEXT);
 				break;
+
+			case RS_INVAL_XID_AGE:
+				values[i++] = CStringGetTextDatum(SLOT_INVAL_XID_AGED_TEXT);
+				break;
 		}
 
 		Assert(i == PG_GET_REPLICATION_SLOTS_COLS);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 70652f0a3f..298bb8ea85 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2913,6 +2913,16 @@ struct config_int ConfigureNamesInt[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"max_slot_xid_age", PGC_SIGHUP, REPLICATION_SENDING,
+			gettext_noop("Age of the transaction ID at which a replication slot gets invalidated."),
+			gettext_noop("The transaction is the oldest transaction (including the one affecting the system catalogs) that a replication slot needs the database to retain.")
+		},
+		&max_slot_xid_age,
+		0, 0, 2000000000,
+		NULL, NULL, NULL
+	},
+
 	{
 		{"commit_delay", PGC_SUSET, WAL_SETTINGS,
 			gettext_noop("Sets the delay in microseconds between transaction commit and "
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index e10755972a..96fe198c23 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -325,6 +325,7 @@
 #wal_sender_timeout = 60s	# in milliseconds; 0 disables
 #track_commit_timestamp = off	# collect timestamp of transaction commit
 				# (change requires restart)
+#max_slot_xid_age = 0
 
 # - Primary Server -
 
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index e706ca834c..8216c35481 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -50,6 +50,8 @@ typedef enum ReplicationSlotInvalidationCause
 	RS_INVAL_HORIZON,
 	/* wal_level insufficient for slot */
 	RS_INVAL_WAL_LEVEL,
+	/* slot's xmin or catalog_xmin has reached the age */
+	RS_INVAL_XID_AGE,
 } ReplicationSlotInvalidationCause;
 
 /*
@@ -59,6 +61,7 @@ typedef enum ReplicationSlotInvalidationCause
 #define SLOT_INVAL_WAL_REMOVED_TEXT "wal_removed"
 #define SLOT_INVAL_HORIZON_TEXT     "rows_removed"
 #define SLOT_INVAL_WAL_LEVEL_TEXT   "wal_level_insufficient"
+#define SLOT_INVAL_XID_AGED_TEXT	"xid_aged"
 
 /*
  * On-Disk data of a replication slot, preserved across restarts.
@@ -229,6 +232,7 @@ extern PGDLLIMPORT ReplicationSlot *MyReplicationSlot;
 
 /* GUCs */
 extern PGDLLIMPORT int max_replication_slots;
+extern PGDLLIMPORT int max_slot_xid_age;
 
 /* shmem initialization functions */
 extern Size ReplicationSlotsShmemSize(void);
diff --git a/src/test/recovery/t/050_invalidate_slots.pl b/src/test/recovery/t/050_invalidate_slots.pl
new file mode 100644
index 0000000000..2f482b56e8
--- /dev/null
+++ b/src/test/recovery/t/050_invalidate_slots.pl
@@ -0,0 +1,108 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+# Test for replication slots invalidation
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+use Time::HiRes qw(usleep);
+
+# Initialize primary node, setting wal-segsize to 1MB
+my $primary = PostgreSQL::Test::Cluster->new('primary');
+$primary->init(allows_streaming => 1, extra => ['--wal-segsize=1']);
+$primary->append_conf(
+	'postgresql.conf', q{
+checkpoint_timeout = 1h
+});
+$primary->start;
+$primary->safe_psql(
+	'postgres', qq[
+    SELECT pg_create_physical_replication_slot('sb1_slot');
+]);
+
+# Take backup
+my $backup_name = 'my_backup';
+$primary->backup($backup_name);
+
+# Create a standby linking to the primary using the replication slot
+my $standby1 = PostgreSQL::Test::Cluster->new('standby1');
+$standby1->init_from_backup($primary, $backup_name, has_streaming => 1);
+
+# Enable hs_feedback. The slot should gain an xmin. We set the status interval
+# so we'll see the results promptly.
+$standby1->append_conf(
+	'postgresql.conf', q{
+primary_slot_name = 'sb1_slot'
+hot_standby_feedback = on
+wal_receiver_status_interval = 1
+});
+$standby1->start;
+
+# Create some content on primary to move xmin
+$primary->safe_psql('postgres',
+	"CREATE TABLE tab_int AS SELECT generate_series(1,10) AS a");
+
+# Wait until standby has replayed enough data
+$primary->wait_for_catchup($standby1);
+
+$primary->poll_query_until(
+	'postgres', qq[
+	SELECT xmin IS NOT NULL
+		FROM pg_catalog.pg_replication_slots
+		WHERE slot_name = 'sb1_slot';
+]) or die "Timed out waiting for slot xmin to advance";
+
+$primary->safe_psql(
+	'postgres', qq[
+	ALTER SYSTEM SET max_slot_xid_age = 500;
+]);
+$primary->reload;
+
+# Stop standby to make the replication slot's xmin on primary to age
+$standby1->stop;
+
+my $logstart = -s $primary->logfile;
+
+# Do some work to advance xmin
+$primary->safe_psql(
+	'postgres', q{
+do $$
+begin
+  for i in 10000..11000 loop
+    -- use an exception block so that each iteration eats an XID
+    begin
+      insert into tab_int values (i);
+    exception
+      when division_by_zero then null;
+    end;
+  end loop;
+end$$;
+});
+
+my $invalidated = 0;
+for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
+{
+	$primary->safe_psql('postgres', "CHECKPOINT");
+	if ($primary->log_contains(
+			'invalidating obsolete replication slot "sb1_slot"', $logstart))
+	{
+		$invalidated = 1;
+		last;
+	}
+	usleep(100_000);
+}
+ok($invalidated, 'check that slot sb1_slot invalidation has been logged');
+
+# Wait for the inactive replication slots to be invalidated.
+$primary->poll_query_until(
+	'postgres', qq[
+	SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+		WHERE slot_name = 'sb1_slot' AND
+		invalidation_reason = 'xid_aged';
+])
+  or die
+  "Timed out while waiting for replication slot sb1_slot to be invalidated";
+
+done_testing();
-- 
2.34.1



  [application/octet-stream] v5-0003-Track-inactive-replication-slot-information.patch (9.8K, ../../CALj2ACWgACB4opnbqi=x7Hc4aqcgkXoLsh1VB+gfidXaDQNu_Q@mail.gmail.com/4-v5-0003-Track-inactive-replication-slot-information.patch)
  download | inline diff:
From 4f8e82845a03015f5870933e7712fde5a10ebdfd Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Tue, 20 Feb 2024 05:44:19 +0000
Subject: [PATCH v5 3/4] Track inactive replication slot information

Currently postgres doesn't track metrics like the time at which
the slot became inactive, and the total number of times the slot
became inactive in its lifetime. This commit adds two new metrics
last_inactive_at of type timestamptz and inactive_count of type numeric
to ReplicationSlotPersistentData. Whenever a slot becomes
inactive, the current timestamp and inactive count are persisted
to disk.

These metrics are useful in the following ways:
- To improve replication slot monitoring tools. For instance, one
can build a monitoring tool that signals a) when replication slots
is lying inactive for a day or so using last_inactive_at metric,
b) when a replication slot is becoming inactive too frequently
using last_inactive_at metric.

- To implement timeout-based inactive replication slot management
capability in postgres.

Increases SLOT_VERSION due to the added two new metrics.
---
 doc/src/sgml/system-views.sgml       | 20 +++++++++++++
 src/backend/catalog/system_views.sql |  4 ++-
 src/backend/replication/slot.c       | 43 ++++++++++++++++++++++------
 src/backend/replication/slotfuncs.c  | 15 +++++++++-
 src/include/catalog/pg_proc.dat      |  6 ++--
 src/include/replication/slot.h       |  6 ++++
 src/test/regress/expected/rules.out  |  6 ++--
 7 files changed, 84 insertions(+), 16 deletions(-)

diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index cce88c14bb..0dfd472b02 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -2771,6 +2771,26 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
        ID of role
       </para></entry>
      </row>
+
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>last_inactive_at</structfield> <type>timestamptz</type>
+      </para>
+      <para>
+        The time at which the slot became inactive.
+        <literal>NULL</literal> if the slot is currently actively being
+        used.
+      </para></entry>
+     </row>
+
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>inactive_count</structfield> <type>numeric</type>
+      </para>
+      <para>
+        The total number of times the slot became inactive in its lifetime.
+      </para></entry>
+     </row>
     </tbody>
    </tgroup>
   </table>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index c39f0d73d3..a5a78a9910 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1026,7 +1026,9 @@ CREATE VIEW pg_replication_slots AS
             L.conflict_reason,
             L.failover,
             L.synced,
-            L.invalidation_reason
+            L.invalidation_reason,
+            L.last_inactive_at,
+            L.inactive_count
     FROM pg_get_replication_slots() AS L
             LEFT JOIN pg_database D ON (L.datoid = D.oid);
 
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 67d6bd849e..ce51c6d909 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -91,7 +91,7 @@ typedef struct ReplicationSlotOnDisk
 	sizeof(ReplicationSlotOnDisk) - ReplicationSlotOnDiskConstantSize
 
 #define SLOT_MAGIC		0x1051CA1	/* format identifier */
-#define SLOT_VERSION	5		/* version for new files */
+#define SLOT_VERSION	6		/* version for new files */
 
 /* Control array for replication slot management */
 ReplicationSlotCtlData *ReplicationSlotCtl = NULL;
@@ -346,6 +346,8 @@ ReplicationSlotCreate(const char *name, bool db_specific,
 	slot->data.two_phase_at = InvalidXLogRecPtr;
 	slot->data.failover = failover;
 	slot->data.synced = synced;
+	slot->data.last_inactive_at = 0;
+	slot->data.inactive_count = 0;
 
 	/* and then data only present in shared memory */
 	slot->just_dirtied = false;
@@ -572,6 +574,17 @@ retry:
 
 	if (am_walsender)
 	{
+		if (s->data.persistency == RS_PERSISTENT)
+		{
+			SpinLockAcquire(&s->mutex);
+			s->data.last_inactive_at = 0;
+			SpinLockRelease(&s->mutex);
+
+			/* Write this slot to disk */
+			ReplicationSlotMarkDirty();
+			ReplicationSlotSave();
+		}
+
 		ereport(log_replication_commands ? LOG : DEBUG1,
 				SlotIsLogical(s)
 				? errmsg("acquired logical replication slot \"%s\"",
@@ -639,16 +652,20 @@ ReplicationSlotRelease(void)
 		ConditionVariableBroadcast(&slot->active_cv);
 	}
 
-	MyReplicationSlot = NULL;
-
-	/* might not have been set when we've been a plain slot */
-	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
-	MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING;
-	ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
-	LWLockRelease(ProcArrayLock);
-
 	if (am_walsender)
 	{
+		if (slot->data.persistency == RS_PERSISTENT)
+		{
+			SpinLockAcquire(&slot->mutex);
+			slot->data.last_inactive_at = GetCurrentTimestamp();
+			slot->data.inactive_count++;
+			SpinLockRelease(&slot->mutex);
+
+			/* Write this slot to disk */
+			ReplicationSlotMarkDirty();
+			ReplicationSlotSave();
+		}
+
 		ereport(log_replication_commands ? LOG : DEBUG1,
 				is_logical
 				? errmsg("released logical replication slot \"%s\"",
@@ -658,6 +675,14 @@ ReplicationSlotRelease(void)
 
 		pfree(slotname);
 	}
+
+	MyReplicationSlot = NULL;
+
+	/* might not have been set when we've been a plain slot */
+	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+	MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING;
+	ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
+	LWLockRelease(ProcArrayLock);
 }
 
 /*
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 7c1145bb75..6a12db27fd 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -239,10 +239,11 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
 Datum
 pg_get_replication_slots(PG_FUNCTION_ARGS)
 {
-#define PG_GET_REPLICATION_SLOTS_COLS 18
+#define PG_GET_REPLICATION_SLOTS_COLS 20
 	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
 	XLogRecPtr	currlsn;
 	int			slotno;
+	char		buf[256];
 
 	/*
 	 * We don't require any special permission to see this function's data
@@ -464,6 +465,18 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 				break;
 		}
 
+		if (slot_contents.data.last_inactive_at > 0)
+			values[i++] = TimestampTzGetDatum(slot_contents.data.last_inactive_at);
+		else
+			nulls[i++] = true;
+
+		/* Convert to numeric. */
+		snprintf(buf, sizeof buf, UINT64_FORMAT, slot_contents.data.inactive_count);
+		values[i++] = DirectFunctionCall3(numeric_in,
+										  CStringGetDatum(buf),
+										  ObjectIdGetDatum(0),
+										  Int32GetDatum(-1));
+
 		Assert(i == PG_GET_REPLICATION_SLOTS_COLS);
 
 		tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index a6bfc36426..3d4ace624e 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -11127,9 +11127,9 @@
   proname => 'pg_get_replication_slots', prorows => '10', proisstrict => 'f',
   proretset => 't', provolatile => 's', prorettype => 'record',
   proargtypes => '',
-  proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,text,bool,bool,text}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
-  proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,conflict_reason,failover,synced,invalidation_reason}',
+  proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,text,bool,bool,text,timestamptz,numeric}',
+  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
+  proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,conflict_reason,failover,synced,invalidation_reason,last_inactive_at,inactive_count}',
   prosrc => 'pg_get_replication_slots' },
 { oid => '3786', descr => 'set up a logical replication slot',
   proname => 'pg_create_logical_replication_slot', provolatile => 'v',
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index 8216c35481..87a56aa28a 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -133,6 +133,12 @@ typedef struct ReplicationSlotPersistentData
 	 * for logical slots on the primary server.
 	 */
 	bool		failover;
+
+	/* When did this slot become inactive last time? */
+	TimestampTz last_inactive_at;
+
+	/* How many times the slot has been inactive? */
+	uint64		inactive_count;
 } ReplicationSlotPersistentData;
 
 /*
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 0646874236..35fcf9d3d0 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1476,8 +1476,10 @@ pg_replication_slots| SELECT l.slot_name,
     l.conflict_reason,
     l.failover,
     l.synced,
-    l.invalidation_reason
-   FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, conflict_reason, failover, synced, invalidation_reason)
+    l.invalidation_reason,
+    l.last_inactive_at,
+    l.inactive_count
+   FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, conflict_reason, failover, synced, invalidation_reason, last_inactive_at, inactive_count)
      LEFT JOIN pg_database d ON ((l.datoid = d.oid)));
 pg_roles| SELECT pg_authid.rolname,
     pg_authid.rolsuper,
-- 
2.34.1



  [application/octet-stream] v5-0004-Add-inactive_timeout-based-replication-slot-inval.patch (12.7K, ../../CALj2ACWgACB4opnbqi=x7Hc4aqcgkXoLsh1VB+gfidXaDQNu_Q@mail.gmail.com/5-v5-0004-Add-inactive_timeout-based-replication-slot-inval.patch)
  download | inline diff:
From 598e036d72f47375b1071c01b2de546dea5c1681 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Tue, 20 Feb 2024 05:45:54 +0000
Subject: [PATCH v5 4/4] Add inactive_timeout based replication slot
 invalidation

Currently postgres has the ability to invalidate inactive
replication slots based on the amount of WAL (set via
max_slot_wal_keep_size GUC) that will be needed for the slots in
case they become active. However, choosing a default value for
max_slot_wal_keep_size is tricky. Because the amount of WAL a
customer generates, and their allocated storage will vary greatly
in production, making it difficult to pin down a one-size-fits-all
value. It is often easy for developers to set a timeout of say 1
or 2 or 3 days, after which the inactive slots get dropped.

To achieve the above, postgres uses replication slot metric
inactive_at (the time at which the slot became inactive), and a
new GUC inactive_replication_slot_timeout. The checkpointer then
looks at all replication slots invalidating the inactive slots
based on the timeout set.
---
 doc/src/sgml/config.sgml                      | 18 +++++
 src/backend/access/transam/xlog.c             | 10 +++
 src/backend/replication/slot.c                | 21 +++++
 src/backend/replication/slotfuncs.c           |  8 ++
 src/backend/utils/misc/guc_tables.c           | 12 +++
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 src/include/replication/slot.h                |  4 +
 src/test/recovery/meson.build                 |  1 +
 src/test/recovery/t/050_invalidate_slots.pl   | 79 +++++++++++++++++++
 9 files changed, 154 insertions(+)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6c1c5f421f..4904541607 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4426,6 +4426,24 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-inactive-replication-slot-timeout" xreflabel="inactive_replication_slot_timeout">
+      <term><varname>inactive_replication_slot_timeout</varname> (<type>integer</type>)
+      <indexterm>
+       <primary><varname>inactive_replication_slot_timeout</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Invalidate replication slots that are inactive for longer than this
+        amount of time at the next checkpoint. If this value is specified
+        without units, it is taken as seconds. A value of zero (which is
+        default) disables the timeout mechanism. This parameter can only be
+        set in the <filename>postgresql.conf</filename> file or on the server
+        command line.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-track-commit-timestamp" xreflabel="track_commit_timestamp">
       <term><varname>track_commit_timestamp</varname> (<type>boolean</type>)
       <indexterm>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5fe76c46a1..ad8786c9f7 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7164,6 +7164,11 @@ CreateCheckPoint(int flags)
 		InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0,
 										   InvalidOid, InvalidTransactionId);
 
+	/* Invalidate inactive replication slots based on timeout */
+	if (inactive_replication_slot_timeout > 0)
+		InvalidateObsoleteReplicationSlots(RS_INVAL_INACTIVE_TIMEOUT, 0,
+										   InvalidOid, InvalidTransactionId);
+
 	/*
 	 * Delete old log files, those no longer needed for last checkpoint to
 	 * prevent the disk holding the xlog from growing full.
@@ -7613,6 +7618,11 @@ CreateRestartPoint(int flags)
 		InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0,
 										   InvalidOid, InvalidTransactionId);
 
+	/* Invalidate inactive replication slots based on timeout */
+	if (inactive_replication_slot_timeout > 0)
+		InvalidateObsoleteReplicationSlots(RS_INVAL_INACTIVE_TIMEOUT, 0,
+										   InvalidOid, InvalidTransactionId);
+
 	/*
 	 * Retreat _logSegNo using the current end of xlog replayed or received,
 	 * whichever is later.
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index ce51c6d909..aef027e4f6 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -103,6 +103,7 @@ ReplicationSlot *MyReplicationSlot = NULL;
 int			max_replication_slots = 10; /* the maximum number of replication
 										 * slots */
 int			max_slot_xid_age = 0;
+int			inactive_replication_slot_timeout = 0;
 
 static void ReplicationSlotShmemExit(int code, Datum arg);
 static void ReplicationSlotDropPtr(ReplicationSlot *slot);
@@ -1445,6 +1446,9 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause,
 		case RS_INVAL_XID_AGE:
 			appendStringInfoString(&err_detail, _("The replication slot's xmin or catalog_xmin reached the age specified by max_slot_xid_age."));
 			break;
+		case RS_INVAL_INACTIVE_TIMEOUT:
+			appendStringInfoString(&err_detail, _("The slot has been inactive for more than the time specified by inactive_replication_slot_timeout."));
+			break;
 		case RS_INVAL_NONE:
 			pg_unreachable();
 	}
@@ -1597,6 +1601,20 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
 						}
 					}
 					break;
+				case RS_INVAL_INACTIVE_TIMEOUT:
+					if (s->data.last_inactive_at > 0)
+					{
+						TimestampTz now;
+
+						Assert(s->data.persistency == RS_PERSISTENT);
+						Assert(s->active_pid == 0);
+
+						now = GetCurrentTimestamp();
+						if (TimestampDifferenceExceeds(s->data.last_inactive_at, now,
+													   inactive_replication_slot_timeout * 1000))
+							conflict = cause;
+					}
+					break;
 				case RS_INVAL_NONE:
 					pg_unreachable();
 			}
@@ -1752,6 +1770,7 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
  *   db; dboid may be InvalidOid for shared relations
  * - RS_INVAL_WAL_LEVEL: is logical
  * - RS_INVAL_XID_AGE: slot's xmin or catalog_xmin has reached the age
+ * - RS_INVAL_INACTIVE_TIMEOUT: inactive slot timeout occurs
  *
  * NB - this runs as part of checkpoint, so avoid raising errors if possible.
  */
@@ -2372,6 +2391,8 @@ GetSlotInvalidationCause(char *conflict_reason)
 		return RS_INVAL_WAL_LEVEL;
 	else if (strcmp(conflict_reason, SLOT_INVAL_XID_AGED_TEXT) == 0)
 		return RS_INVAL_XID_AGE;
+	else if (strcmp(conflict_reason, SLOT_INVAL_INACTIVE_TIMEOUT) == 0)
+		return RS_INVAL_INACTIVE_TIMEOUT;
 	else
 		Assert(0);
 
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 6a12db27fd..b5f077f9f3 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -435,6 +435,10 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 				case RS_INVAL_XID_AGE:
 					values[i++] = CStringGetTextDatum(SLOT_INVAL_XID_AGED_TEXT);
 					break;
+
+				case RS_INVAL_INACTIVE_TIMEOUT:
+					values[i++] = CStringGetTextDatum(SLOT_INVAL_INACTIVE_TIMEOUT);
+					break;
 			}
 		}
 
@@ -463,6 +467,10 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 			case RS_INVAL_XID_AGE:
 				values[i++] = CStringGetTextDatum(SLOT_INVAL_XID_AGED_TEXT);
 				break;
+
+			case RS_INVAL_INACTIVE_TIMEOUT:
+				values[i++] = CStringGetTextDatum(SLOT_INVAL_INACTIVE_TIMEOUT);
+				break;
 		}
 
 		if (slot_contents.data.last_inactive_at > 0)
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 298bb8ea85..28780b8c87 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2923,6 +2923,18 @@ struct config_int ConfigureNamesInt[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"inactive_replication_slot_timeout", PGC_SIGHUP, REPLICATION_SENDING,
+			gettext_noop("Sets the amount of time to wait before invalidating an "
+						 "inactive replication slot."),
+			NULL,
+			GUC_UNIT_S
+		},
+		&inactive_replication_slot_timeout,
+		0, 0, INT_MAX,
+		NULL, NULL, NULL
+	},
+
 	{
 		{"commit_delay", PGC_SUSET, WAL_SETTINGS,
 			gettext_noop("Sets the delay in microseconds between transaction commit and "
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 96fe198c23..ccb79e5a67 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -252,6 +252,7 @@
 #recovery_prefetch = try	# prefetch pages referenced in the WAL?
 #wal_decode_buffer_size = 512kB	# lookahead window used for prefetching
 				# (change requires restart)
+#inactive_replication_slot_timeout = 0	# in seconds; 0 disables
 
 # - Archiving -
 
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index 87a56aa28a..c5174f7c8d 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -52,6 +52,8 @@ typedef enum ReplicationSlotInvalidationCause
 	RS_INVAL_WAL_LEVEL,
 	/* slot's xmin or catalog_xmin has reached the age */
 	RS_INVAL_XID_AGE,
+	/* inactive slot timeout has occurred */
+	RS_INVAL_INACTIVE_TIMEOUT,
 } ReplicationSlotInvalidationCause;
 
 /*
@@ -62,6 +64,7 @@ typedef enum ReplicationSlotInvalidationCause
 #define SLOT_INVAL_HORIZON_TEXT     "rows_removed"
 #define SLOT_INVAL_WAL_LEVEL_TEXT   "wal_level_insufficient"
 #define SLOT_INVAL_XID_AGED_TEXT	"xid_aged"
+#define SLOT_INVAL_INACTIVE_TIMEOUT	"inactive_timeout"
 
 /*
  * On-Disk data of a replication slot, preserved across restarts.
@@ -239,6 +242,7 @@ extern PGDLLIMPORT ReplicationSlot *MyReplicationSlot;
 /* GUCs */
 extern PGDLLIMPORT int max_replication_slots;
 extern PGDLLIMPORT int max_slot_xid_age;
+extern PGDLLIMPORT int inactive_replication_slot_timeout;
 
 /* shmem initialization functions */
 extern Size ReplicationSlotsShmemSize(void);
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index bf087ac2a9..e07b941d73 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -46,6 +46,7 @@ tests += {
       't/038_save_logical_slots_shutdown.pl',
       't/039_end_of_wal.pl',
       't/040_standby_failover_slots_sync.pl',
+      't/050_invalidate_slots.pl',
     ],
   },
 }
diff --git a/src/test/recovery/t/050_invalidate_slots.pl b/src/test/recovery/t/050_invalidate_slots.pl
index 2f482b56e8..4c66dd4a4e 100644
--- a/src/test/recovery/t/050_invalidate_slots.pl
+++ b/src/test/recovery/t/050_invalidate_slots.pl
@@ -105,4 +105,83 @@ $primary->poll_query_until(
   or die
   "Timed out while waiting for replication slot sb1_slot to be invalidated";
 
+$primary->safe_psql(
+	'postgres', qq[
+    SELECT pg_create_physical_replication_slot('sb2_slot');
+]);
+
+$primary->safe_psql(
+	'postgres', qq[
+    ALTER SYSTEM SET max_slot_xid_age = 0;
+]);
+$primary->reload;
+
+# Create a standby linking to the primary using the replication slot
+my $standby2 = PostgreSQL::Test::Cluster->new('standby2');
+$standby2->init_from_backup($primary, $backup_name, has_streaming => 1);
+$standby2->append_conf(
+	'postgresql.conf', q{
+primary_slot_name = 'sb2_slot'
+});
+$standby2->start;
+
+# Wait until standby has replayed enough data
+$primary->wait_for_catchup($standby2);
+
+# The inactive replication slot info should be null when the slot is active
+my $result = $primary->safe_psql(
+	'postgres', qq[
+	SELECT last_inactive_at IS NULL, inactive_count = 0 AS OK
+		FROM pg_replication_slots WHERE slot_name = 'sb2_slot';
+]);
+is($result, "t|t",
+	'check the inactive replication slot info for an active slot');
+
+# Set timeout so that the next checkpoint will invalidate the inactive
+# replication slot.
+$primary->safe_psql(
+	'postgres', qq[
+    ALTER SYSTEM SET inactive_replication_slot_timeout TO '1s';
+]);
+$primary->reload;
+
+$logstart = -s $primary->logfile;
+
+# Stop standby to make the replication slot on primary inactive
+$standby2->stop;
+
+# Wait for the inactive replication slot info to be updated
+$primary->poll_query_until(
+	'postgres', qq[
+	SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+		WHERE last_inactive_at IS NOT NULL AND
+		inactive_count = 1 AND slot_name = 'sb2_slot';
+])
+  or die
+  "Timed out while waiting for inactive replication slot info to be updated";
+
+$invalidated = 0;
+for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
+{
+	$primary->safe_psql('postgres', "CHECKPOINT");
+	if ($primary->log_contains(
+			'invalidating obsolete replication slot "sb2_slot"', $logstart))
+	{
+		$invalidated = 1;
+		last;
+	}
+	usleep(100_000);
+}
+ok($invalidated, 'check that slot sb2_slot invalidation has been logged');
+
+# Wait for the inactive replication slots to be invalidated.
+$primary->poll_query_until(
+	'postgres', qq[
+	SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+		WHERE slot_name = 'sb2_slot' AND
+		invalidation_reason = 'inactive_timeout';
+])
+  or die
+  "Timed out while waiting for inactive replication slot sb2_slot to be invalidated";
+
 done_testing();
-- 
2.34.1



^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* Re: Introduce XID age and inactive timeout based replication slot invalidation
@ 2024-02-21 05:25  Bharath Rupireddy <[email protected]>
  parent: Bharath Rupireddy <[email protected]>
  0 siblings, 1 reply; 24+ messages in thread

From: Bharath Rupireddy @ 2024-02-21 05:25 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

On Tue, Feb 20, 2024 at 12:05 PM Bharath Rupireddy
<[email protected]> wrote:
>
>> [...] and was able to produce something like:
> >
> > postgres=# select slot_name,slot_type,active,active_pid,wal_status,invalidation_reason from pg_replication_slots;
> >   slot_name  | slot_type | active | active_pid | wal_status | invalidation_reason
> > -------------+-----------+--------+------------+------------+---------------------
> >  rep1        | physical  | f      |            | reserved   |
> >  master_slot | physical  | t      |    1482441 | unreserved | wal_removed
> > (2 rows)
> >
> > does that make sense to have an "active/working" slot "ivalidated"?
>
> Thanks. Can you please provide the steps to generate this error? Are
> you setting max_slot_wal_keep_size on primary to generate
> "wal_removed"?

I'm able to reproduce [1] the state [2] where the slot got invalidated
first, then its wal_status became unreserved, but still the slot is
serving after the standby comes up online after it catches up with the
primary getting the WAL files from the archive. There's a good reason
for this state -
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/replication/slotfuncs.c;h=d....
This intermittent state can only happen for physical slots, not for
logical slots because logical subscribers can't get the missing
changes from the WAL stored in the archive.

And, the fact looks to be that an invalidated slot can never become
normal but still can serve a standby if the standby is able to catch
up by fetching required WAL (this is the WAL the slot couldn't keep
for the standby) from elsewhere (archive via restore_command).

As far as the 0001 patch is concerned, it reports the
invalidation_reason as long as slot_contents.data.invalidated !=
RS_INVAL_NONE. I think this is okay.

Thoughts?

[1]
./initdb -D db17
echo "max_wal_size = 128MB
max_slot_wal_keep_size = 64MB
archive_mode = on
archive_command='cp %p
/home/ubuntu/postgres/pg17/bin/archived_wal/%f'" | tee -a
db17/postgresql.conf

./pg_ctl -D db17 -l logfile17 start

./psql -d postgres -p 5432 -c "SELECT
pg_create_physical_replication_slot('sb_repl_slot', true, false);"

rm -rf sbdata logfilesbdata
./pg_basebackup -D sbdata

echo "port=5433
primary_conninfo='host=localhost port=5432 dbname=postgres user=ubuntu'
primary_slot_name='sb_repl_slot'
restore_command='cp /home/ubuntu/postgres/pg17/bin/archived_wal/%f
%p'" | tee -a sbdata/postgresql.conf

touch sbdata/standby.signal

./pg_ctl -D sbdata -l logfilesbdata start
./psql -d postgres -p 5433 -c "SELECT pg_is_in_recovery();"

./pg_ctl -D sbdata -l logfilesbdata stop

./psql -d postgres -p 5432 -c "SELECT pg_logical_emit_message(true,
'mymessage', repeat('aaaa', 10000000));"
./psql -d postgres -p 5432 -c "CHECKPOINT;"
./pg_ctl -D sbdata -l logfilesbdata start
./psql -d postgres -p 5432 -xc "SELECT * FROM pg_replication_slots;"

[2]
postgres=# SELECT * FROM pg_replication_slots;
-[ RECORD 1 ]-------+-------------
slot_name           | sb_repl_slot
plugin              |
slot_type           | physical
datoid              |
database            |
temporary           | f
active              | t
active_pid          | 710667
xmin                |
catalog_xmin        |
restart_lsn         | 0/115D21A0
confirmed_flush_lsn |
wal_status          | unreserved
safe_wal_size       | 77782624
two_phase           | f
conflict_reason     |
failover            | f
synced              | f
invalidation_reason | wal_removed
last_inactive_at    |
inactive_count      | 1

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com






^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* Re: Introduce XID age and inactive timeout based replication slot invalidation
@ 2024-02-21 12:25  Bertrand Drouvot <[email protected]>
  parent: Bharath Rupireddy <[email protected]>
  0 siblings, 1 reply; 24+ messages in thread

From: Bertrand Drouvot @ 2024-02-21 12:25 UTC (permalink / raw)
  To: Bharath Rupireddy <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

Hi,

On Wed, Feb 21, 2024 at 10:55:00AM +0530, Bharath Rupireddy wrote:
> On Tue, Feb 20, 2024 at 12:05 PM Bharath Rupireddy
> <[email protected]> wrote:
> >
> >> [...] and was able to produce something like:
> > >
> > > postgres=# select slot_name,slot_type,active,active_pid,wal_status,invalidation_reason from pg_replication_slots;
> > >   slot_name  | slot_type | active | active_pid | wal_status | invalidation_reason
> > > -------------+-----------+--------+------------+------------+---------------------
> > >  rep1        | physical  | f      |            | reserved   |
> > >  master_slot | physical  | t      |    1482441 | unreserved | wal_removed
> > > (2 rows)
> > >
> > > does that make sense to have an "active/working" slot "ivalidated"?
> >
> > Thanks. Can you please provide the steps to generate this error? Are
> > you setting max_slot_wal_keep_size on primary to generate
> > "wal_removed"?
> 
> I'm able to reproduce [1] the state [2] where the slot got invalidated
> first, then its wal_status became unreserved, but still the slot is
> serving after the standby comes up online after it catches up with the
> primary getting the WAL files from the archive. There's a good reason
> for this state -
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/replication/slotfuncs.c;h=d....
> This intermittent state can only happen for physical slots, not for
> logical slots because logical subscribers can't get the missing
> changes from the WAL stored in the archive.
> 
> And, the fact looks to be that an invalidated slot can never become
> normal but still can serve a standby if the standby is able to catch
> up by fetching required WAL (this is the WAL the slot couldn't keep
> for the standby) from elsewhere (archive via restore_command).
> 
> As far as the 0001 patch is concerned, it reports the
> invalidation_reason as long as slot_contents.data.invalidated !=
> RS_INVAL_NONE. I think this is okay.
> 
> Thoughts?

Yeah, looking at the code I agree that looks ok. OTOH, that looks confusing,
maybe we should add a few words about it in the doc?

Looking at v5-0001:

+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>invalidation_reason</structfield> <type>text</type>
+      </para>
+      <para>

My initial thought was to put "conflict" value in this new field in case of
conflict (not to mention the conflict reason in it). With the current proposal
invalidation_reason could report the same as conflict_reason, which sounds weird
to me.

Does that make sense to you to use "conflict" as value in "invalidation_reason"
when the slot has "conflict_reason" not NULL?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com






^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* Re: Introduce XID age and inactive timeout based replication slot invalidation
@ 2024-02-21 14:40  Bharath Rupireddy <[email protected]>
  parent: Bertrand Drouvot <[email protected]>
  0 siblings, 1 reply; 24+ messages in thread

From: Bharath Rupireddy @ 2024-02-21 14:40 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

On Wed, Feb 21, 2024 at 5:55 PM Bertrand Drouvot
<[email protected]> wrote:
>
> > As far as the 0001 patch is concerned, it reports the
> > invalidation_reason as long as slot_contents.data.invalidated !=
> > RS_INVAL_NONE. I think this is okay.
> >
> > Thoughts?
>
> Yeah, looking at the code I agree that looks ok. OTOH, that looks confusing,
> maybe we should add a few words about it in the doc?

I'll think about it.

> Looking at v5-0001:
>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>invalidation_reason</structfield> <type>text</type>
> +      </para>
> +      <para>
>
> My initial thought was to put "conflict" value in this new field in case of
> conflict (not to mention the conflict reason in it). With the current proposal
> invalidation_reason could report the same as conflict_reason, which sounds weird
> to me.
>
> Does that make sense to you to use "conflict" as value in "invalidation_reason"
> when the slot has "conflict_reason" not NULL?

I'm thinking the other way around - how about we revert
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=007693f2a3ac2ac19affcb03ad43cdb36...,
that is, put in place "conflict" as a boolean and introduce
invalidation_reason the text form. So, for logical slots, whenever the
"conflict" column is true, the reason is found in invaldiation_reason
column? How does it sound? Again the debate might be "conflict" vs
"invalidation", but that looks clean IMHO.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com






^ permalink  raw  reply  [nested|flat] 24+ messages in thread

* Re: Introduce XID age and inactive timeout based replication slot invalidation
@ 2024-03-01 22:11  Nathan Bossart <[email protected]>
  parent: Bharath Rupireddy <[email protected]>
  0 siblings, 0 replies; 24+ messages in thread

From: Nathan Bossart @ 2024-03-01 22:11 UTC (permalink / raw)
  To: Bharath Rupireddy <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; PostgreSQL Hackers <[email protected]>

On Wed, Feb 21, 2024 at 08:10:00PM +0530, Bharath Rupireddy wrote:
> I'm thinking the other way around - how about we revert
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=007693f2a3ac2ac19affcb03ad43cdb36...,
> that is, put in place "conflict" as a boolean and introduce
> invalidation_reason the text form. So, for logical slots, whenever the
> "conflict" column is true, the reason is found in invaldiation_reason
> column? How does it sound? Again the debate might be "conflict" vs
> "invalidation", but that looks clean IMHO.

Would you ever see "conflict" as false and "invalidation_reason" as
non-null for a logical slot?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com






^ permalink  raw  reply  [nested|flat] 24+ messages in thread

end of thread, other threads:[~2024-03-01 22:11 UTC | newest]

Thread overview: 24+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]>
2024-02-20 06:35 Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]>
2024-02-21 05:25 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]>
2024-02-21 12:25   ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bertrand Drouvot <[email protected]>
2024-02-21 14:40     ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]>
2024-03-01 22:11       ` Re: Introduce XID age and inactive timeout based replication slot invalidation Nathan Bossart <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox