Обсуждение: 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@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