Re: Schema variables - new implementation for Postgres 15

From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Julien Rouhaud <rjuju123(at)gmail(dot)com>
Cc: Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, Joel Jacobson <joel(at)compiler(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Schema variables - new implementation for Postgres 15
Date: 2022-01-30 13:15:31
Message-ID: CAFj8pRDbH4vpP5BTieD4Mw5OtdoLBb0ucqZhYB5c1SFPcqVoxg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

so 29. 1. 2022 v 6:19 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:

> Hi,
>
> On Fri, Jan 28, 2022 at 07:51:08AM +0100, Pavel Stehule wrote:
> > st 26. 1. 2022 v 8:23 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com>
> napsal:
> >
> > > + The <literal>ON TRANSACTION END RESET</literal>
> > > + clause causes the session variable to be reset to its default
> value
> > > when
> > > + the transaction is committed or rolled back.
> > >
> > > As far as I can see this clauses doesn't play well with IMMUTABLE
> > > VARIABLE, as
> > > you can reassign a value once the transaction ends. Same for DISCARD [
> > > ALL |
> > > VARIABLES ], or LET var = NULL (or DEFAULT if no default value). Is
> that
> > > intended?
> > >
> >
> > I think so it is expected. The life scope of assigned (immutable) value
> is
> > limited to transaction (when ON TRANSACTION END RESET).
> > DISCARD is used for reset of session, and after it, you can write the
> value
> > first time.
> >
> > I enhanced doc in IMMUTABLE clause
>
> I think it's still somewhat unclear:
>
> - done, no other change will be allowed in the session lifetime.
> + done, no other change will be allowed in the session variable
> content's
> + lifetime. The lifetime of content of session variable can be
> + controlled by <literal>ON TRANSACTION END RESET</literal> clause.
> + </para>
>
> The "session variable content lifetime" is quite peculiar, as the ON
> TRANSACTION END RESET is adding transactional behavior to something that's
> not
> supposed to be transactional, so more documentation about it seems
> appropriate.
>
> Also DISCARD can be used any time so that's a totally different aspect of
> the
> immutable variable content lifetime that's not described here.
>

fixed

>
> NULL handling also seems inconsistent. An explicit default NULL value
> makes it
> truly immutable, but manually assigning NULL is a different codepath that
> has a
> different user behavior:
>
> # create immutable variable var_immu int default null;
> CREATE VARIABLE
>
> # let var_immu = 1;
> ERROR: 22005: session variable "var_immu" is declared IMMUTABLE
>
> # create immutable variable var_immu2 int ;
> CREATE VARIABLE
>
> # let var_immu2 = null;
> LET
>
> # let var_immu2 = null;
> LET
>
> # let var_immu2 = 1;
> LET
>
> For var_immu2 I think that the last 2 queries should have errored out.
>

ok, I changed this behave

>
> > > In revoke.sgml:
> > > + REVOKE [ GRANT OPTION FOR ]
> > > + { { READ | WRITE } [, ...] | ALL [ PRIVILEGES ] }
> > > + ON VARIABLE <replaceable>variable_name</replaceable> [, ...]
> > > + FROM { [ GROUP ] <replaceable
> > > class="parameter">role_name</replaceable> | PUBLIC } [, ...]
> > > + [ CASCADE | RESTRICT ]
> > >
> > > there's no extra documentation for that, and therefore no
> clarification on
> > > variable_name.
> > >
> >
> > This is same like function_name, domain_name, ...
>
> Ah right.
>
> > > pg_variable.c:
> > > Do we really need both session_variable_get_name() and
> > > get_session_variable_name()?
> > >
> >
> > They are different - first returns possibly qualified name, second
> returns
> > only name. Currently it is used just for error messages in
> > transformAssignmentIndirection, and I think so it is good for consistency
> > with other usage of this routine (transformAssignmentIndirection).
>
> I agree that consistency with other usage is a good thing, but both
> functions
> have very similar and confusing names. Usually when you need the qualified
> name the calling code just takes care of doing so. Wouldn't it be better
> to
> add say get_session_variable_namespace() and construct the target string
> in the
> calling code?
>

ok, I rewrote related code

>
> Also, I didn't dig a lot but I didn't see other usage with optionally
> qualified
> name there? I'm not sure how it would make sense anyway since LET
> semantics
> are different and the current call for session variable emit incorrect
> messages:
>

changed

> # create table tt(id integer);
> CREATE TABLE
>
> # create variable vv tt;
> CREATE VARIABLE
>
> # let vv.meh = 1;
> ERROR: 42703: cannot assign to field "meh" of column "meh" because there
> is no such column in data type tt
> LINE 1: let vv.meh = 1;
>

fixed

postgres=# create table tt(id integer); create variable vv tt;
CREATE TABLE
CREATE VARIABLE
postgres=# let vv.meh = 1;
ERROR: cannot assign to field "meh" of column or variable "vv" because
there is no such column in data type tt
LINE 1: let vv.meh = 1;
^

Regards

Pavel

Attachment Content-Type Size
v20220130-0002-This-patch-changes-error-message-column-doesn-t-exis.patch text/x-patch 29.1 KB
v20220130-0001-session-variables.patch text/x-patch 324.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bharath Rupireddy 2022-01-30 13:22:48 Re: Remove extra includes of "access/xloginsert.h" when "access/xlog.h" is included
Previous Message osumi.takamichi@fujitsu.com 2022-01-30 08:04:48 RE: logical replication empty transactions