Re: Schema variables - new implementation for Postgres 15 - Mailing list pgsql-hackers

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.eisentraut@enterprisedb.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"?

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

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