Re: Large expressions in indexes can't be stored (non-TOASTable) - Mailing list pgsql-hackers

On Wed, Oct 16, 2024 at 09:12:31AM +0900, Michael Paquier wrote:
> -   if (!ctx->rel->rd_rel->reltoastrelid)
> +   if (!OidIsValid(RelationGetToastRelid(ctx->rel)))
> 
> This set of diffs in 0002 is a nice cleanup.  I'd wish for relying
> less on zero comparitons when assuming that InvalidOid is in use.

I'm wondering if there's any concern about this one causing back-patching
pain.  If so, I can just add the macro for use in new code.

> +static inline void
> +AssertHasSnapshotForToast(Relation rel)
> +{
> +    /* bootstrap mode in particular breaks this rule */
> +    if (!IsNormalProcessingMode())
> +        return;
> +
> +    /* if the relation doesn't have a TOAST table, we are good */
> +    if (!OidIsValid(RelationGetToastRelid(rel)))
> +        return;
> +
> +    Assert(HaveRegisteredOrActiveSnapshot());
> +}
> 
> Using a separate inlined routine is indeed cleaner as you have
> documented the assumptions behind the check.  Wouldn't it be better to
> use a USE_ASSERT_CHECKING block?  These two checks for normal
> processing and toastrelid are cheap lookups, but we don't need them at
> all in non-assert paths, so I'd suggest to ignore them entirely for
> the non-USE_ASSERT_CHECKING case.

I assume all of this will get compiled out in non-USE_ASSERT_CHECKING
builds as-is, but I see no problem with surrounding it with an #ifdef to be
sure.

-- 
nathan