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