Re: Schema variables - new implementation for Postgres 15 (typo)

Hi,

On Fri, Jan 06, 2023 at 08:02:41PM +0100, Pavel Stehule wrote:
> pá 6. 1. 2023 v 5:11 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:
>
> >
> > +Datum
> > +GetSessionVariableWithTypeCheck(Oid varid, bool *isNull, Oid
> > expected_typid)
> > +{
> > + SVariable svar;
> > +
> > + svar = prepare_variable_for_reading(varid);
> > + Assert(svar != NULL && svar->is_valid);
> > +
> > + return CopySessionVariableWithTypeCheck(varid, isNull,
> > expected_typid);
> > +
> > + if (expected_typid != svar->typid)
> > + elog(ERROR, "type of variable \"%s.%s\" is different than
> > expected",
> > +
> > get_namespace_name(get_session_variable_namespace(varid)),
> > + get_session_variable_name(varid));
> > +
> > + *isNull = svar->isnull;
> > +
> > + return svar->value;
> > +}
> >
> > there's a unconditional return in the middle of the function, which also
> > looks
> > like a thinko during a rebase since CopySessionVariableWithTypeCheck mostly
> > contains the same following code.
> >
>
> This looks like my mistake - originally I fixed an issue related to the
> usage session variable from plpgsql, and I forced a returned copy of the
> session variable's value. Now, the function
> GetSessionVariableWithTypeCheck is not used anyywhere.

Oh I didn't check if it was used in the patchset.

> It can be used only
> from extensions, where is ensured so a) the value is not changed, b) and in
> a lifetime of returned value is not called any query or any expression that
> can change the value of this variable. I fixed this code and I enhanced
> comments. I am not sure if this function should not be removed. It is not
> used by backend, but it can be handy for extensions - it reduces possible
> useless copy.

Hmm, how safe is it for third-party code to access the stored data directly
rather than a copy? If it makes extension fragile if they're not careful
enough with cache invalidation, or even give them a way to mess up with the
data directly, it's probably not a good idea to provide such an API.

> > I'm also wondering if there should be additional tests based on the last
> > scenario reported by Dmitry? (I don't see any new or similar test, but I
> > may
> > have missed it).
> >
>
> The scenario reported by Dmitry is in tests already.

Oh, so I missed it sorry about that. I did some testing using
debug_discard_cache in the past and didn't run into this issue, so it's
probably due to a more recent changes or before such a test was added.

> I am not sure if I
> have to repeat it with active debug_discard_cache. I expect this mode will
> be activated in some testing environments.

Yes, some buildfarm animal are configured to run with various
debug_discard_caches setting so it's not needed to override it in some tests
(it makes testing time really slow, which will be annoying for everyone
including old/slow buildfarm animals).

> I have no idea how to simply emulate this issue without
> debug_discard_caches on 1. It is necessary to raise the sinval message
> exactly when the variable is checked against system catalogue.

Manually testing while setting locally debug_discard_cache should be enough.

> updated patches attached

Thanks!