Date: Sat, 29 Jan 2022 13:19:46 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Pavel Stehule <pavel.stehule@gmail.com>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>,
	Joel Jacobson <joel@compiler.org>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Schema variables - new implementation for Postgres 15
Message-ID: <20220129051946.outrihp7phagjeey@jrouhaud>
References: <20220123081034.tcg7tob6fu3pmlpr@jrouhaud>
 <CAFj8pRCoB-DdZB_J2iR80dg-GmRKzbVbqbj0O2LJvaryox6wxA@mail.gmail.com>
 <20220123085245.ojgzvfdc6zet5wbp@jrouhaud>
 <CAFj8pRCPuEJ+vv793vbJ2ZjoXMiVw=G4WGNJCmktjGVpkj4Yrg@mail.gmail.com>
 <CAOBaU_YrgXc=8GtcrY3UxWqcW0mP1BRkK0d8S1rqw99JOAHbxQ@mail.gmail.com>
 <CAFj8pRBO-LuSurrYNwQ5-ooJaUMa8QMy9=B83bRN2e1dz3OcbA@mail.gmail.com>
 <20220125051845.xiqkj6hmwvsfpwhw@jrouhaud>
 <CAFj8pRD_8J5VYcwgHja8tr6rNLqJCFEOSgq5HzckZxhGzNwdzA@mail.gmail.com>
 <20220126072306.xxkfqma6rwfdkxff@jrouhaud>
 <CAFj8pRCZ9fVAC6h-s8fHS9muXmPwvHcwAX3omRJm9vUaQTMUcA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAFj8pRCZ9fVAC6h-s8fHS9muXmPwvHcwAX3omRJm9vUaQTMUcA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220129051946.outrihp7phagjeey%40jrouhaud>
Precedence: bulk

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

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.

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

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:

# 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;