Re: pg_sequence_last_value() for unlogged sequences on standbys
On Thu, May 16, 2024 at 08:33:35PM -0500, Nathan Bossart wrote: > Here is a rebased version of 0002, which I intend to commit once v18 > development begins. Committed. -- nathan
Re: pg_sequence_last_value() for unlogged sequences on standbys
Here is a rebased version of 0002, which I intend to commit once v18
development begins.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From e9cba5e4303c7fa5ad2d7d5deb23fe0b1c740b09 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 7 May 2024 14:35:34 -0500
Subject: [PATCH v5 1/1] Simplify pg_sequences a bit.
XXX: NEEDS CATVERSION BUMP
---
src/backend/catalog/system_views.sql | 6 +-
src/backend/commands/sequence.c | 12
src/test/regress/expected/rules.out | 5 +
3 files changed, 6 insertions(+), 17 deletions(-)
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 53047cab5f..b32e5c3170 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -176,11 +176,7 @@ CREATE VIEW pg_sequences AS
S.seqincrement AS increment_by,
S.seqcycle AS cycle,
S.seqcache AS cache_size,
-CASE
-WHEN has_sequence_privilege(C.oid, 'SELECT,USAGE'::text)
-THEN pg_sequence_last_value(C.oid)
-ELSE NULL
-END AS last_value
+pg_sequence_last_value(C.oid) AS last_value
FROM pg_sequence S JOIN pg_class C ON (C.oid = S.seqrelid)
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE NOT pg_is_other_temp_schema(N.oid)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 28f8522264..cd0e746577 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1783,21 +1783,17 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
- if (pg_class_aclcheck(relid, GetUserId(), ACL_SELECT | ACL_USAGE) != ACLCHECK_OK)
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied for sequence %s",
- RelationGetRelationName(seqrel;
-
/*
* We return NULL for other sessions' temporary sequences. The
* pg_sequences system view already filters those out, but this offers a
* defense against ERRORs in case someone invokes this function directly.
*
* Also, for the benefit of the pg_sequences view, we return NULL for
- * unlogged sequences on standbys instead of throwing an error.
+ * unlogged sequences on standbys and for sequences for which we lack
+ * USAGE or SELECT privileges instead of throwing an error.
*/
- if (!RELATION_IS_OTHER_TEMP(seqrel) &&
+ if (pg_class_aclcheck(relid, GetUserId(), ACL_SELECT | ACL_USAGE) == ACLCHECK_OK &&
+ !RELATION_IS_OTHER_TEMP(seqrel) &&
(RelationIsPermanent(seqrel) || !RecoveryInProgress()))
{
Buffer buf;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index ef658ad740..04b3790bdd 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1699,10 +1699,7 @@ pg_sequences| SELECT n.nspname AS schemaname,
s.seqincrement AS increment_by,
s.seqcycle AS cycle,
s.seqcache AS cache_size,
-CASE
-WHEN has_sequence_privilege(c.oid, 'SELECT,USAGE'::text) THEN pg_sequence_last_value((c.oid)::regclass)
-ELSE NULL::bigint
-END AS last_value
+pg_sequence_last_value((c.oid)::regclass) AS last_value
FROM ((pg_sequence s
JOIN pg_class c ON ((c.oid = s.seqrelid)))
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
--
2.25.1
Re: pg_sequence_last_value() for unlogged sequences on standbys
Committed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Wed, May 08, 2024 at 11:01:01AM +0900, Michael Paquier wrote:
> On Tue, May 07, 2024 at 02:39:42PM -0500, Nathan Bossart wrote:
>> Okay, phew. We can still do something like v3-0002 for v18. I'll give
>> Michael a chance to comment on 0001 before committing/back-patching that
>> one.
>
> What you are doing in 0001, and 0002 for v18 sounds fine to me.
Great. Rather than commit this on a Friday afternoon, I'll just post what
I have staged for commit early next week.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 19d9a1dd88385664e6991121e4751aba85a45639 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 10 May 2024 15:55:24 -0500
Subject: [PATCH v4 1/1] Fix pg_sequence_last_value() for unlogged sequences on
standbys.
Presently, when this function is called for an unlogged sequence on
a standby server, it will error out with a message like
ERROR: could not open file "base/5/16388": No such file or directory
Since the pg_sequences system view uses pg_sequence_last_value(),
it can error similarly. To fix, modify the function to return NULL
for unlogged sequences on standby servers. Since this bug is
present on all versions since v15, this approach is preferable to
making the ERROR nicer because we need to repair the pg_sequences
view without modifying its definition on released versions. For
consistency, this commit also modifies the function to return NULL
for other sessions' temporary sequences. The pg_sequences view
already appropriately filters out such sequences, so there's no bug
there, but we might as well offer some defense in case someone
invokes this function directly.
Unlogged sequences were first introduced in v15, but temporary
sequences are much older, so while the fix for unlogged sequences
is only back-patched to v15, the temporary sequence portion is
back-patched to all supported versions.
We could also remove the privilege check in the pg_sequences view
definition in v18 if we modify this function to return NULL for
sequences for which the current user lacks privileges, but that is
left as a future exercise for when v18 development begins.
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20240501005730.GA594666%40nathanxps13
Backpatch-through: 12
---
doc/src/sgml/system-views.sgml| 34 +++
src/backend/commands/sequence.c | 31 +---
src/test/recovery/t/001_stream_rep.pl | 8 +++
3 files changed, 60 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index a54f4a4743..9842ee276e 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -3091,15 +3091,41 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts
ppx
The last sequence value written to disk. If caching is used,
this value can be greater than the last value handed out from the
- sequence. Null if the sequence has not been read from yet. Also, if
- the current user does not have USAGE
- or SELECT privilege on the sequence, the value is
- null.
+ sequence.
+
+
+ The last_value column will read as null if any of
+ the following are true:
+
+
+
+ The sequence has not been read from yet.
+
+
+
+
+ The current user does not have USAGE or
+ SELECT privilege on the sequence.
+
+
+
+
+ The sequence is a temporary sequence created by another session.
+
+
+
+
+ The sequence is unlogged and the server is a standby.
+
+
+
+
+
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 46103561c3..28f8522264 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1777,11 +1777,8 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
SeqTableelm;
Relationseqrel;
- Buffer buf;
- HeapTupleData seqtuple;
- Form_pg_sequence_data seq;
- boolis_called;
- int64 result;
+ boolis_called = false;
+ int64 result = 0;
/* open and lock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -1792,12 +1789,28 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel;
- seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ /*
+* We return NULL for other sessions' temporary sequences. The
+* pg_sequences system view already filters those out, but this offers a
+* defense against ERRORs in case someone invokes this function
directl
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Tue, May 07, 2024 at 02:39:42PM -0500, Nathan Bossart wrote: > Okay, phew. We can still do something like v3-0002 for v18. I'll give > Michael a chance to comment on 0001 before committing/back-patching that > one. What you are doing in 0001, and 0002 for v18 sounds fine to me. -- Michael signature.asc Description: PGP signature
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Tue, May 07, 2024 at 03:02:01PM -0400, Tom Lane wrote:
> Nathan Bossart writes:
>> On Tue, May 07, 2024 at 01:44:16PM -0400, Tom Lane wrote:
>>> +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.
>
> Oh! You're right, I'm wrong. I was looking at the CASE filter, which
> we could get rid of -- but the "WHERE NOT pg_is_other_temp_schema(N.oid)"
> part has to stay.
Okay, phew. We can still do something like v3-0002 for v18. I'll give
Michael a chance to comment on 0001 before committing/back-patching that
one.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 2a37834699587eef18b50bf8d58723790bbcdde7 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 30 Apr 2024 20:54:51 -0500
Subject: [PATCH v3 1/2] 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
>From b96d1f21f6144640561360c84b361f569a2edc48 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 7 May 2024 14:35:34 -0500
Subject: [PATCH v3 2/2] Simplify pg_sequences a bit.
XXX: NEEDS CATVERSION BUMP
---
src/backend/catalog/system_views.sql | 6 +-
src/backend/commands/sequence.c | 15 +--
src/test/regress/expected/rules.out | 5 +
3 files changed, 7 insertions(+), 19 deletions(-)
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 53047cab5f..b32e5c3170 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -176,11 +176,7 @@ CREATE VIEW pg_sequences AS
S.seqincrement AS increment_by,
S.seqcycle AS cycle,
S.seqcache AS cache_size,
-CASE
-WHEN has_sequence_privilege(C.oid, 'SELECT,USAGE'::text)
-THEN pg_sequence_last_value(C.oid)
-ELSE NULL
-END AS last_value
+pg_sequence_last_value(C.oid) AS last_value
FROM pg_sequence S JOIN pg_class C ON (C.oid = S.seqrelid)
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE NOT pg_is_other_temp_schema(N.oid)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 9d7468d7bb..f129375915 100644
--- a/src/backend/commands/sequ
Re: pg_sequence_last_value() for unlogged sequences on standbys
Nathan Bossart writes: > On Tue, May 07, 2024 at 01:44:16PM -0400, Tom Lane wrote: >> +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. Oh! You're right, I'm wrong. I was looking at the CASE filter, which we could get rid of -- but the "WHERE NOT pg_is_other_temp_schema(N.oid)" part has to stay. regards, tom lane
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Tue, May 07, 2024 at 01:44:16PM -0400, Tom Lane wrote:
> Nathan Bossart 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
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
Re: pg_sequence_last_value() for unlogged sequences on standbys
Nathan Bossart writes:
> Okay, so are we okay to back-patch something like v1? Or should we also
> return NULL for other sessions' temporary schemas on primaries? That would
> change the condition to something like
> 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.
> 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.
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.
regards, tom lane
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Sat, May 04, 2024 at 06:45:32PM +0900, Michael Paquier wrote:
> On Fri, May 03, 2024 at 05:22:06PM -0400, Tom Lane wrote:
>> Nathan Bossart writes:
>>> IIUC this would cause other sessions' temporary sequences to appear in the
>>> view. Is that desirable?
>>
>> I assume Michael meant to move the test into the C code, not drop
>> it entirely --- I agree we don't want that.
>
> Yup. I meant to remove it from the script and keep only something in
> the C code to avoid the duplication, but you're right that the temp
> sequences would create more noise than now.
>
>> Moving it has some attraction, but pg_is_other_temp_schema() is also
>> used in a lot of information_schema views, so we couldn't get rid of
>> it without a lot of further hacking. Not sure we want to relocate
>> that filter responsibility in just one view.
>
> Okay.
Okay, so are we okay to back-patch something like v1? Or should we also
return NULL for other sessions' temporary schemas on primaries? That would
change the condition to something like
char relpersist = seqrel->rd_rel->relpersistence;
if (relpersist == RELPERSISTENCE_PERMANENT ||
(relpersist == RELPERSISTENCE_UNLOGGED &&
!RecoveryInProgress()) ||
!RELATION_IS_OTHER_TEMP(seqrel))
{
...
}
I personally think that would be fine to back-patch since pg_sequences
already filters it out anyway.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Fri, May 03, 2024 at 03:49:08PM -0500, Nathan Bossart wrote: > On Wed, May 01, 2024 at 12:39:53PM +0900, Michael Paquier wrote: >> By the way, shouldn't we also change the function to return NULL for a >> failed permission check? It would be possible to remove the >> has_sequence_privilege() as well, thanks to that, and a duplication >> between the code and the function view. I've been looking around a >> bit, noticing one use of this function in check_pgactivity (nagios >> agent), and its query also has a has_sequence_privilege() so returning >> NULL would simplify its definition in the long-run. I'd suspect other >> monitoring queries to do something similar to bypass permission >> errors. > > I'm okay with that, but it would be v18 material that I'd track separately > from the back-patchable fix proposed in this thread. Of course. I mean only the permission check simplification for HEAD. My apologies if my words were unclear. -- Michael signature.asc Description: PGP signature
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Fri, May 03, 2024 at 05:22:06PM -0400, Tom Lane wrote: > Nathan Bossart writes: >> IIUC this would cause other sessions' temporary sequences to appear in the >> view. Is that desirable? > > I assume Michael meant to move the test into the C code, not drop > it entirely --- I agree we don't want that. Yup. I meant to remove it from the script and keep only something in the C code to avoid the duplication, but you're right that the temp sequences would create more noise than now. > Moving it has some attraction, but pg_is_other_temp_schema() is also > used in a lot of information_schema views, so we couldn't get rid of > it without a lot of further hacking. Not sure we want to relocate > that filter responsibility in just one view. Okay. -- Michael signature.asc Description: PGP signature
Re: pg_sequence_last_value() for unlogged sequences on standbys
Nathan Bossart writes: > On Wed, May 01, 2024 at 12:39:53PM +0900, Michael Paquier wrote: >> However, it seems to me that you should also drop the >> pg_is_other_temp_schema() in system_views.sql for the definition of >> pg_sequences. Doing that on HEAD now would be OK, but there's nothing >> urgent to it so it may be better done once v18 opens up. Note that >> pg_is_other_temp_schema() is only used for this sequence view, which >> is a nice cleanup. > IIUC this would cause other sessions' temporary sequences to appear in the > view. Is that desirable? I assume Michael meant to move the test into the C code, not drop it entirely --- I agree we don't want that. Moving it has some attraction, but pg_is_other_temp_schema() is also used in a lot of information_schema views, so we couldn't get rid of it without a lot of further hacking. Not sure we want to relocate that filter responsibility in just one view. regards, tom lane
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Wed, May 01, 2024 at 12:39:53PM +0900, Michael Paquier wrote: > However, it seems to me that you should also drop the > pg_is_other_temp_schema() in system_views.sql for the definition of > pg_sequences. Doing that on HEAD now would be OK, but there's nothing > urgent to it so it may be better done once v18 opens up. Note that > pg_is_other_temp_schema() is only used for this sequence view, which > is a nice cleanup. IIUC this would cause other sessions' temporary sequences to appear in the view. Is that desirable? > By the way, shouldn't we also change the function to return NULL for a > failed permission check? It would be possible to remove the > has_sequence_privilege() as well, thanks to that, and a duplication > between the code and the function view. I've been looking around a > bit, noticing one use of this function in check_pgactivity (nagios > agent), and its query also has a has_sequence_privilege() so returning > NULL would simplify its definition in the long-run. I'd suspect other > monitoring queries to do something similar to bypass permission > errors. I'm okay with that, but it would be v18 material that I'd track separately from the back-patchable fix proposed in this thread. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Tue, Apr 30, 2024 at 09:05:31PM -0500, Nathan Bossart wrote: > This ended up being easier than I expected. While unlogged sequences are > only supported on v15 and above, temporary sequences have been around since > v7.2, so this will probably need to be back-patched to all supported > versions. Unlogged and temporary relations cannot be accessed during recovery, so I'm OK with your change to force a NULL for both relpersistences. However, it seems to me that you should also drop the pg_is_other_temp_schema() in system_views.sql for the definition of pg_sequences. Doing that on HEAD now would be OK, but there's nothing urgent to it so it may be better done once v18 opens up. Note that pg_is_other_temp_schema() is only used for this sequence view, which is a nice cleanup. By the way, shouldn't we also change the function to return NULL for a failed permission check? It would be possible to remove the has_sequence_privilege() as well, thanks to that, and a duplication between the code and the function view. I've been looking around a bit, noticing one use of this function in check_pgactivity (nagios agent), and its query also has a has_sequence_privilege() so returning NULL would simplify its definition in the long-run. I'd suspect other monitoring queries to do something similar to bypass permission errors. > The added test case won't work for v12-v14 since it uses an > unlogged sequence. That would require a BackgroundPsql to maintain the connection to the primary, so not having a test is OK by me. -- Michael signature.asc Description: PGP signature
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Tue, Apr 30, 2024 at 08:13:17PM -0500, Nathan Bossart wrote:
> Good point. I'll work on a patch along these lines, then.
This ended up being easier than I expected. While unlogged sequences are
only supported on v15 and above, temporary sequences have been around since
v7.2, so this will probably need to be back-patched to all supported
versions. The added test case won't work for v12-v14 since it uses an
unlogged sequence. I'm not sure it's worth constructing a test case for
temporary sequences.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 71008f13da88f41a205e0643129162df9d2ebc81 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 30 Apr 2024 20:54:51 -0500
Subject: [PATCH v1 1/1] Fix pg_sequence_last_value() for non-permanent
sequences on standbys.
---
src/backend/commands/sequence.c | 18 +-
src/test/recovery/t/001_stream_rep.pl | 8
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 46103561c3..659d2ad4fc 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1780,7 +1780,7 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Buffer buf;
HeapTupleData seqtuple;
Form_pg_sequence_data seq;
- bool is_called;
+ bool is_called = false;
int64 result;
/* open and lock sequence */
@@ -1792,12 +1792,20 @@ 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.
+ */
+ if (RelationIsPermanent(seqrel) || !RecoveryInProgress())
+ {
+ 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
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Tue, Apr 30, 2024 at 09:06:04PM -0400, Tom Lane wrote:
> Nathan Bossart writes:
>> If you create an unlogged sequence on a primary, pg_sequence_last_value()
>> for that sequence on a standby will error like so:
>> postgres=# select pg_sequence_last_value('test'::regclass);
>> ERROR: could not open file "base/5/16388": No such file or directory
>
>> As pointed out a few years ago [0], this function is undocumented, so
>> there's no stated contract to uphold. I lean towards just returning NULL
>> because that's what we'll have to put in the relevant pg_sequences field
>> anyway, but I can see an argument for fixing the ERROR to align with what
>> you see when you try to access unlogged relations on a standby (i.e.,
>> "cannot access temporary or unlogged relations during recovery").
>
> Yeah, I agree with putting that logic into the function. Putting
> such conditions into the SQL of a system view is risky because it
> is really, really painful to adjust the SQL in a released version.
> You could back-patch a fix for this if done at the C level, but
> I doubt we'd go to the trouble if it's done in the view.
Good point. I'll work on a patch along these lines, then.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: pg_sequence_last_value() for unlogged sequences on standbys
Nathan Bossart writes:
> If you create an unlogged sequence on a primary, pg_sequence_last_value()
> for that sequence on a standby will error like so:
> postgres=# select pg_sequence_last_value('test'::regclass);
> ERROR: could not open file "base/5/16388": No such file or directory
> As pointed out a few years ago [0], this function is undocumented, so
> there's no stated contract to uphold. I lean towards just returning NULL
> because that's what we'll have to put in the relevant pg_sequences field
> anyway, but I can see an argument for fixing the ERROR to align with what
> you see when you try to access unlogged relations on a standby (i.e.,
> "cannot access temporary or unlogged relations during recovery").
Yeah, I agree with putting that logic into the function. Putting
such conditions into the SQL of a system view is risky because it
is really, really painful to adjust the SQL in a released version.
You could back-patch a fix for this if done at the C level, but
I doubt we'd go to the trouble if it's done in the view.
regards, tom lane
pg_sequence_last_value() for unlogged sequences on standbys
If you create an unlogged sequence on a primary, pg_sequence_last_value()
for that sequence on a standby will error like so:
postgres=# select pg_sequence_last_value('test'::regclass);
ERROR: could not open file "base/5/16388": No such file or directory
This function is used by the pg_sequences system view, which fails with the
same error on standbys. The two options I see are:
* Return a better ERROR and adjust pg_sequences to avoid calling this
function for unlogged sequences on standbys.
* Return NULL from pg_sequence_last_value() if called for an unlogged
sequence on a standby.
As pointed out a few years ago [0], this function is undocumented, so
there's no stated contract to uphold. I lean towards just returning NULL
because that's what we'll have to put in the relevant pg_sequences field
anyway, but I can see an argument for fixing the ERROR to align with what
you see when you try to access unlogged relations on a standby (i.e.,
"cannot access temporary or unlogged relations during recovery").
Thoughts?
[0]
https://postgr.es/m/CAAaqYe8JL8Et2DoO0RRjGaMvy7-C6eDH-2wHXK-gp3dOssvBkQ%40mail.gmail.com
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
