Re: Schema variables - new implementation for Postgres 15

ne 26. 3. 2023 v 13:32 odesílatel Julien Rouhaud <rjuju123@gmail.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.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"?
>

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