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
