Date: Tue, 7 May 2024 13:40:51 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>, pgsql-hackers@postgresql.org
Subject: Re: pg_sequence_last_value() for unlogged sequences on standbys
Message-ID: <20240507184051.GA2600328@nathanxps13>
References: <20240501005730.GA594666@nathanxps13>
 <1846474.1714525564@sss.pgh.pa.us>
 <20240501011317.GC594666@nathanxps13>
 <20240501020531.GA721953@nathanxps13>
 <ZjG5ia8R_001EvYx@paquier.xyz>
 <20240503204908.GA1737291@nathanxps13>
 <2746846.1714771326@sss.pgh.pa.us>
 <ZjYDvGuHaf_Cx5Pb@paquier.xyz>
 <20240507171033.GA2524814@nathanxps13>
 <4179588.1715103856@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="y0ulUmNC+osPPQO6"
Content-Disposition: inline
In-Reply-To: <4179588.1715103856@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/20240507184051.GA2600328%40nathanxps13>
Precedence: bulk


--y0ulUmNC+osPPQO6
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, May 07, 2024 at 01:44:16PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> 	char relpersist = seqrel->rd_rel->relpersistence;
> 
>> 	if (relpersist == RELPERSISTENCE_PERMANENT ||
>> 		(relpersist == RELPERSISTENCE_UNLOGGED && !RecoveryInProgress()) ||
>> 		!RELATION_IS_OTHER_TEMP(seqrel))
>> 	{
>> 		...
>> 	}
> 
> Should be AND'ing not OR'ing the !TEMP condition, no?  Also I liked
> your other formulation of the persistence check better.

Yes, that's a silly mistake on my part.  I changed it to

	if ((RelationIsPermanent(seqrel) || !RecoveryInProgress()) &&
		!RELATION_IS_OTHER_TEMP(seqrel))
	{
		...
	}

in the attached v2.

>> I personally think that would be fine to back-patch since pg_sequences
>> already filters it out anyway.
> 
> +1 to include that, as it offers a defense if someone invokes this
> function directly.  In HEAD we could then rip out the test in the
> view.

I apologize for belaboring this point, but I don't see how we would be
comfortable removing that check unless we are okay with other sessions'
temporary sequences appearing in the view, albeit with a NULL last_value.
This check lives in the WHERE clause today, so if we remove it, we'd no
longer exclude those sequences.  Michael and you seem united on this, so I
have a sinking feeling that I'm missing something terribly obvious.

> BTW, I think you also need something like
> 
> -	int64		result;
> +	int64		result = 0;
> 
> Your compiler may not complain about result being possibly
> uninitialized, but IME others will.

Good call.

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

--y0ulUmNC+osPPQO6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Fix-pg_sequence_last_value-for-non-permanent-sequ.patch"

From 974f56896add92983b664c11fd25010ef29ac42c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 30 Apr 2024 20:54:51 -0500
Subject: [PATCH v2 1/1] Fix pg_sequence_last_value() for non-permanent
 sequences on standbys.

---
 src/backend/commands/sequence.c       | 22 ++++++++++++++++------
 src/test/recovery/t/001_stream_rep.pl |  8 ++++++++
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 46103561c3..9d7468d7bb 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1780,8 +1780,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
 	Buffer		buf;
 	HeapTupleData seqtuple;
 	Form_pg_sequence_data seq;
-	bool		is_called;
-	int64		result;
+	bool		is_called = false;
+	int64		result = 0;
 
 	/* open and lock sequence */
 	init_sequence(relid, &elm, &seqrel);
@@ -1792,12 +1792,22 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
 				 errmsg("permission denied for sequence %s",
 						RelationGetRelationName(seqrel))));
 
-	seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+	/*
+	 * For the benefit of the pg_sequences system view, we return NULL for
+	 * temporary and unlogged sequences on standbys instead of throwing an
+	 * error.  We also always return NULL for other sessions' temporary
+	 * sequences.
+	 */
+	if ((RelationIsPermanent(seqrel) || !RecoveryInProgress()) &&
+		!RELATION_IS_OTHER_TEMP(seqrel))
+	{
+		seq = read_seq_tuple(seqrel, &buf, &seqtuple);
 
-	is_called = seq->is_called;
-	result = seq->last_value;
+		is_called = seq->is_called;
+		result = seq->last_value;
 
-	UnlockReleaseBuffer(buf);
+		UnlockReleaseBuffer(buf);
+	}
 	sequence_close(seqrel, NoLock);
 
 	if (is_called)
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 5311ade509..4c698b5ce1 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -95,6 +95,14 @@ $result = $node_standby_2->safe_psql('postgres', "SELECT * FROM seq1");
 print "standby 2: $result\n";
 is($result, qq(33|0|t), 'check streamed sequence content on standby 2');
 
+# Check pg_sequence_last_value() returns NULL for unlogged sequence on standby
+$node_primary->safe_psql('postgres',
+	"CREATE UNLOGGED SEQUENCE ulseq; SELECT nextval('ulseq')");
+$node_primary->wait_for_replay_catchup($node_standby_1);
+is($node_standby_1->safe_psql('postgres',
+	"SELECT pg_sequence_last_value('ulseq'::regclass) IS NULL"),
+	't', 'pg_sequence_last_value() on unlogged sequence on standby 1');
+
 # Check that only READ-only queries can run on standbys
 is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
 	3, 'read-only queries on standby 1');
-- 
2.25.1


--y0ulUmNC+osPPQO6--