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

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?

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

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

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