Thread: 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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
- v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.master
- v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v16
- v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v15
- v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v14
- v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v13
- v4-0001-Fix-pg_sequence_last_value-for-unlogged-sequences.patch.v12
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
Attachment
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