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

pá 6. 1. 2023 v 5:11 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:

> Hi,
>
> On Fri, Dec 23, 2022 at 08:38:43AM +0100, Pavel Stehule wrote:
> >
> > I am sending an updated patch, fixing the mentioned issue. Big thanks for
> > testing, and checking.
>
> There were multiple reviews in the last weeks which reported some issues,
> but
> unless I'm missing something I don't see any follow up from the reviewers
> on
> the changes?
>
> I will still wait a bit to see if they chime in while I keep looking at the
> diff since the last version of the code I reviewed.
>
> But in the meantime I already saw a couple of things that don't look right:
>
> --- a/src/backend/commands/dropcmds.c
> +++ b/src/backend/commands/dropcmds.c
> @@ -481,6 +481,11 @@ does_not_exist_skipping(ObjectType objtype, Node
> *object)
> msg = gettext_noop("publication \"%s\" does not
> exist, skipping");
> name = strVal(object);
> break;
> + case OBJECT_VARIABLE:
> + msg = gettext_noop("session variable \"%s\" does
> not exist, skipping");
> + name = NameListToString(castNode(List, object));
> + break;
> + default:
>
> case OBJECT_COLUMN:
>
> the "default:" seems like a thinko during a rebase?
>

removed

>
> +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 everywhere. 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.

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

When I checked regress tests, then debug_discard_caches is set only to zero
(in one case).

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.

updated patches attached

Regards

Pavel

>
> > > > Why do you think so? The variable has no mvcc support - it is just
> stored
> > > > value with local visibility without mvcc support. There can be
> little bit
> > > > similar issues like with global temporary tables.
> > >
> > > Yeah, sorry for not being precise, I mean global temporary tables. This
> > > is not my analysis, I've simply picked up it was mentioned a couple of
> > > times here. The points above are not meant to serve as an objection
> > > against the patch, but rather to figure out if there are any gaps left
> > > to address and come up with some sort of plan with "committed" as a
> > > final destination.
> > >
> >
> > There are some similarities, but there are a lot of differences too.
> > Handling of metadata is partially similar, but session variable is almost
> > the value cached in session memory. It has no statistics, it is not
> stored
> > in a file. Because there is different storage, I don't think there is
> some
> > intersection on implementation level.
>
> +1
>