On Tue, May 07, 2024 at 01:44:16PM -0400, Tom Lane wrote:
> Nathan Bossart <[email protected]> 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
>From 974f56896add92983b664c11fd25010ef29ac42c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
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
