Re: Schema variables - new implementation for Postgres 15

ne 26. 3. 2023 v 13:32 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com>
napsal:

> Hi,
>
> I just have a few minor wording improvements for the various comments /
> documentation you quoted.
>
> On Sun, Mar 26, 2023 at 08:53:49AM +0200, Pavel Stehule wrote:
> > út 21. 3. 2023 v 17:18 odesílatel Peter Eisentraut <
> > peter(dot)eisentraut(at)enterprisedb(dot)com> napsal:
> >
> > > - What is the purpose of struct Variable? It seems very similar to
> > > FormData_pg_variable. At least a comment would be useful.
> > >
> >
> > I wrote comment there:
> >
> >
> > /*
> > * The Variable struct is based on FormData_pg_variable struct. Against
> > * FormData_pg_variable it can hold node of deserialized expression used
> > * for calculation of default value.
> > */
>
> Did you mean "Unlike" rather than "Against"?
>

fixed

>
> > > 0002
> > >
> > > expr_kind_allows_session_variables() should have some explanation
> > > about criteria for determining which expression kinds should allow
> > > variables.
> > >
> >
> > I wrote comment there:
> >
> > /*
> > * Returns true, when expression of kind allows using of
> > * session variables.
> > + * The session's variables can be used everywhere where
> > + * can be used external parameters. Session variables
> > + * are not allowed in DDL. Session's variables cannot be
> > + * used in constraints.
> > + *
> > + * The identifier can be parsed as an session variable
> > + * only in expression's kinds where session's variables
> > + * are allowed. This is the primary usage of this function.
> > + *
> > + * Second usage of this function is for decision if
> > + * an error message "column does not exist" or "column
> > + * or variable does not exist" should be printed. When
> > + * we are in expression, where session variables cannot
> > + * be used, we raise the first form or error message.
> > */
>
> Maybe
>
> /*
> * Returns true if the given expression kind is valid for session variables
> * Session variables can be used everywhere where external parameters can
> be
> * used. Session variables are not allowed in DDL commands or in
> constraints.
> *
> * An identifier can be parsed as a session variable only for expression
> kinds
> * where session variables are allowed. This is the primary usage of this
> * function.
> *
> * Second usage of this function is to decide whether "column does not
> exist" or
> * "column or variable does not exist" error message should be printed.
> * When we are in an expression where session variables cannot be used, we
> raise
> * the first form or error message.
> */
>

changed

>
> > > session_variables_ambiguity_warning: There needs to be more
> > > information about this. The current explanation is basically just,
> > > "warn if your query is confusing". Why do I want that? Why would I
> > > not want that? What is the alternative? What are some examples?
> > > Shouldn't there be a standard behavior without a need to configure
> > > anything?
> > >
> >
> > I enhanced this entry:
> >
> > + <para>
> > + The session variables can be shadowed by column references in a
> > query. This
> > + is an expected feature. The existing queries should not be
> broken
> > by creating
> > + any session variable, because session variables are shadowed
> > always if the
> > + identifier is ambiguous. The variables should be named without
> > possibility
> > + to collision with identifiers of other database objects (column
> > names or
> > + record field names). The warnings enabled by setting
> > <varname>session_variables_ambiguity_warning</varname>
> > + should help with finding identifier's collisions.
>
> Maybe
>
> Session variables can be shadowed by column references in a query, this is
> an
> expected behavior. Previously working queries shouldn't error out by
> creating
> any session variable, so session variables are always shadowed if an
> identifier
> is ambiguous. Variables should be referenced using an unambiguous
> identifier
> without any possibility for a collision with identifier of other database
> objects (column names or record fields names). The warning messages
> emitted
> when enabling <varname>session_variables_ambiguity_warning</varname> can
> help
> finding such identifier collision.
>
> > + </para>
> > + <para>
> > + This feature can significantly increase size of logs, and then
> it
> > is
> > + disabled by default, but for testing or development
> environments it
> > + should be enabled.
>
> Maybe
>
> This feature can significantly increase log size, so it's disabled by
> default.
> For testing or development environments it's recommended to enable it if
> you
> use session variables.
>

replaced

Thank you very much for these language correctures

Regards

Pavel

p.s. I'll send updated patch after today or tomorrow - I have to fix broken
dependency check after rebase