út 30. 7. 2024 v 21:46 odesílatel Laurenz Albe <laurenz.a...@cybertec.at>
napsal:
> This is my review of the second patch in the series.
>
> Again, I mostly changed code comments and documentation.
>
> Noteworthy remarks:
>
> - A general reminder: single line comments should start with a lower case
> letter and have to period in the end:
>
> /* this is a typical single line comment */
>
> - Variable as parameter:
>
> CREATE VARIABLE var AS date;
> LET var = current_date;
> PREPARE stmt(date) AS SELECT $1;
> EXECUTE stmt(var);
> ERROR: paramid of PARAM_VARIABLE param is out of range
>
> Is that working as intended? I don't understand the error message.
>
> Perhaps there is a bug in src/backend/executor/execExpr.c.
>
> - IdentifyVariable
>
> > --- a/src/backend/catalog/namespace.c
> > +++ b/src/backend/catalog/namespace.c
> > [...]
> > +/*
> > + * IdentifyVariable - try to find variable identified by list of
> names.
> > [...]
>
> The function comment doesn't make clear to me what the function does.
> Perhaps that is so confusing because the function seems to serve several
> purposes (during parsing, in ANALYZE, and to identify shadowed variables
> in a later patch).
>
> I rewrote the function comment, but didn't touch the code or code
> comments.
>
> Perhaps part of the reason why this function is so complicated is that
> you support things like this:
>
> CREATE SCHEMA sch;
> CREATE TYPE cp AS (a integer, b integer);
> CREATE VARIABLE sch.v AS cp;
> LET sch.v = (1, 2);
> SELECT sch.v.b;
> b
> ═══
> 2
> (1 row)
>
> test=# SELECT test.sch.v.b;
> b
> ═══
> 2
> (1 row)
>
> We don't support that for tables:
>
> CREATE TABLE tab (c cp);
> INSERT INTO tab VALUES (ROW(42, 43));
> SELECT tab.c.b FROM tab;
> ERROR: cross-database references are not implemented: tab.c.b
>
> You have to use extra parentheses:
>
> SELECT (tab.c).b FROM tab;
> b
> ════
> 43
> (1 row)
>
> I'd say that this should be the same for session variables.
> What do you think?
>
> Code comments:
>
> - There are lots of variables declared at the beginning, but they are
> only
> used in code blocks further down. For clarity, you should declare a
> variable in the code block where it is used.
>
> - + /*
> + * In this case, "a" is used as catalog name -
> check it.
> + */
> + if (strcmp(a, get_database_name(MyDatabaseId)) !=
> 0)
> + {
> + if (!noerror)
> + ereport(ERROR,
> +
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cross-database references
> are not implemented: %s",
> + NameListToString(names))));
> + }
> + else
> + {
>
> There needn't be an "else", since the ereport() will never return.
> Less indentation is better.
>
> - src/backend/commands/session_variable.c
>
> There is one comment that confuses me and that I did not edit:
>
> > +Datum
> > +GetSessionVariable(Oid varid, bool *isNull, Oid *typid)
> > +{
> > + SVariable svar;
> > +
> > + svar = get_session_variable(varid);
> > +
> > + /*
> > + * Although svar is freshly validated in this point, the
> svar->is_valid can
> > + * be false, due possible accepting invalidation message inside
> domain
> > + * check. Now, the validation is done after lock, that can also
> accept
> > + * invalidation message, so validation should be trustful.
> > + *
> > + * For now, we don't need to repeat validation. Only svar should
> be valid
> > + * pointer.
> > + */
> > + Assert(svar);
> > +
> > + *typid = svar->typid;
> > +
> > + return copy_session_variable_value(svar, isNull);
> > +}
>
> - src/backend/executor/execMain.c
>
> > @@ -196,6 +198,51 @@ standard_ExecutorStart(QueryDesc *queryDesc, int
> eflags)
> > Assert(queryDesc->sourceText != NULL);
> > estate->es_sourceText = queryDesc->sourceText;
> >
> > + /*
> > + * The executor doesn't work with session variables directly.
> Values of
> > + * related session variables are copied to dedicated array, and
> this array
> > + * is passed to executor.
> > + */
>
> Why? Is that a performance measure? The comment should explain that.
>
> - parallel safety
>
> > --- a/src/backend/optimizer/util/clauses.c
> > +++ b/src/backend/optimizer/util/clauses.c
> > @@ -935,6 +936,13 @@ max_parallel_hazard_walker(Node *node,
> max_parallel_hazard_context *context)
> > if (param->paramkind == PARAM_EXTERN)
> > return false;
> >
> > + /* We doesn't support passing session variables to workers */
> > + if (param->paramkind == PARAM_VARIABLE)
> > + {
> > + if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED,
> context))
> > + return true;
> > + }
>
> Even if a later patch alleviates that restriction, this patch should
> document that
> session variables imply "parallel restricted". I have added that to
> doc/src/sgml/parallel.sgml.
>
> Attached are the first two patches with my edits (the first patch is
> unchanged;
> I just add it again to keep the cfbot happy.
>
Probably you didn't attach new files - the second patch is not complete. Or
you didn't make changes there?
Regards
Pavel
>
> I hope to get to review the other patches at some later time.
>
> Yours,
> Laurenz Albe
>
