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