Re: 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

Nathan Bossart <nathandbossart(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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