pg_sequence_last_value() for unlogged sequences on standbys

Nathan Bossart <nathandbossart@gmail.com> 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

On Tue, Apr 30, 2024 at 09:06:04PM -0400, Tom Lane wrote:

Nathan Bossart <nathandbossart@gmail.com> 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

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

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

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

Nathan Bossart <nathandbossart@gmail.com> 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

On Fri, May 03, 2024 at 05:22:06PM -0400, Tom Lane wrote:

Nathan Bossart <nathandbossart@gmail.com> 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

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

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 <nathandbossart@gmail.com> 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

Nathan Bossart <nathandbossart@gmail.com> 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

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

Nathan Bossart <nathandbossart@gmail.com> 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

On Tue, May 07, 2024 at 03:02:01PM -0400, Tom Lane wrote:

Nathan Bossart <nathandbossart@gmail.com> 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

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

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

Committed.

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

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

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