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