Message-ID: <04ec666686e9e21cb515617df06885c66f3d34ce.camel@cybertec.at>
Subject: Re: proposal: schema variables
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Pavel Stehule <pavel.stehule@gmail.com>
Cc: Erik Rijkers <er@xs4all.nl>, Michael Paquier <michael@paquier.xyz>, Amit
 Kapila <amit.kapila16@gmail.com>, DUVAL REMI <REMI.DUVAL@cheops.fr>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: Tue, 27 Aug 2024 08:15:42 +0200
In-Reply-To: 
 <CAFj8pRDF-F5diNTXr206QfQFauFNK1vu_7NARXd0dtdwGTWLMQ@mail.gmail.com>
References: 
	<CAFj8pRBoGBKopkiTa4ki3dMgy-cSTRZ-BQPKOq7=Tk0SSNsowA@mail.gmail.com>
	 <CAFj8pRBCiWn12H9FHymYs17fk68nRd9Xpn+SYf18TLdb2YSUrQ@mail.gmail.com>
	 <CAA4eK1JV-Ox0oRFdXnPqXSzM84wTR5QFkRzCpNVF_+0FNjS5Mg@mail.gmail.com>
	 <CAFj8pRByCCcgDkeXyafAnH6LgxtAyCVwg6yfJAhyTY6GLscfZg@mail.gmail.com>
	 <CAFj8pRBvUonC_ug3F=w1Q55Dp=DggojvAeL7Vmh14Q-WhFHxzw@mail.gmail.com>
	 <CAFj8pRDj72P-f=SUygtOXnTOBQ0RzmL_fN=wLfaCzcbPVpGgzw@mail.gmail.com>
	 <CAFj8pRDD2GQaJ_iDT4vSVe658+oHRXU2S2af7Y1-it9jaP8VFg@mail.gmail.com>
	 <20200924035637.GF28585@paquier.xyz>
	 <CAFj8pRAFktynx5wkanv5SRuzXkZgXu77XpVACiSE=v7i1xHFbw@mail.gmail.com>
	 <CAFj8pRA=bn_g5T2AZduy5gNOQoOnUJ+pMHmnRMHi6mR0n=TAsA@mail.gmail.com>
	 <20201001033824.GC8130@paquier.xyz>
	 <CAFj8pRC9de05HSb4tEHDUwJ98+4Wh30W-rJrNOPnTz6ARcv0Fw@mail.gmail.com>
	 <CAFj8pRBC5Wz1xHKKBmKsM0xYN0+PdSZ5oXPsk5SZt+VprdUW3A@mail.gmail.com>
	 <CAFj8pRAh4pzMoZrKCLt9h+Lr2L=vhgs2PjAF45uLbp_7sijM5w@mail.gmail.com>
	 <CAFj8pRA-kxQ1oErcuDeUKYsrgwB5XGLhquatwxOe3dCVy1gcyQ@mail.gmail.com>
	 <51a9a68e8a998d04df17417d45c1dbd4@xs4all.nl>
	 <CAFj8pRBbt2xhY9PyabOY0ZN+Aig6ee3oCon-DM9qi0Uw_3qfbw@mail.gmail.com>
	 <CAFj8pRDSa52J7kPmCYXgq1BBbu3YBXwpdSOVpjgU=hnE2k04Cw@mail.gmail.com>
	 <CAFj8pRD+QiWOoPrFk2NnPs3t5Eaf4X=aGRV-9ww11cnPP+fV4g@mail.gmail.com>
	 <bdeb7611c6dbf22226c302bcc01d178b@xs4all.nl>
	 <CAFj8pRCGTjqHvH9oeiSf4T6Bydhk9pm033DxxibgF+B7SHC6MQ@mail.gmail.com>
	 <CAFj8pRAzNDhFgbZnT0T0mJ7ygA1Qje1Hc0TiKwXM8++kGooPYg@mail.gmail.com>
	 <89817942c99da01cd5e7850fe418436b@xs4all.nl>
	 <CAFj8pRBarjJYfkN-0-i=JRZJ4PTOYC+K7XgAhfdDqWGqRiPkyw@mail.gmail.com>
	 <CAFj8pRDqdWdCULxd5asbKs5C4e9kT2TuKBkR5L-e1=hP5wF2uw@mail.gmail.com>
	 <56ca532c37eb0b540961f74a7bd5db39@xs4all.nl>
	 <CAFj8pRCPW56pFr0F0BcasdXjFeo3SFixNSpWKaBk0ibvznum-A@mail.gmail.com>
	 <CAFj8pRD1Feit93CgwmYm1Q=X+M+AZqffCEZPFQ7qEMNHZRN4fA@mail.gmail.com>
	 <CAFj8pRCc=B9-FRQg5eWDSkGwS2vpkq88hR6042cmPPizHuEGSA@mail.gmail.com>
	 <CAFj8pRBk8x7afUXKLBOU-Ctg6A7QJvTAGGVEi0b6Jc8YTe8nUg@mail.gmail.com>
	 <CAFj8pRCSwHQ4BJUbjF2YEausK1Z6+ejMyedpqAnWJbG+FEJDLw@mail.gmail.com>
	 <CAFj8pRAbY+N+UqjqgESL5x-bsGmV+aVyyUkxUSgaGDZToZjDqQ@mail.gmail.com>
	 <CAFj8pRBzKcqzj=23BHfv1QaXHt=2_SN=uhdR3rb_dAVQoit7ug@mail.gmail.com>
	 <CAFj8pRCi-n6SzkAB+OHG=TZvL13xxta_qgffBLDOY0HEBqDhvg@mail.gmail.com>
	 <8181bd3abc647bdae5a4f78e71e62478a98c75f4.camel@cybertec.at>
	 <CAFj8pRBaD0_bMrCREWnVLfcTMdc0v7ns7Rt=sEvd1EoFmLfarQ@mail.gmail.com>
	 <9e67d49deb18270eddb95e602c83f02b98459843.camel@cybertec.at>
	 <CAFj8pRDXo-RRcy2VFDm_vzv3Eaaz6Ex=X19up=x8W4COyBNmaQ@mail.gmail.com>
	 <f9f4d536993e48990bd7eb38e18798c4572b6700.camel@cybertec.at>
	 <CAFj8pRDgraz64HHcCoFGCKDM1Cxv1ukELsqfsxux4JP3PM6RyQ@mail.gmail.com>
	 <3b662dc5b615d4c20a55e8e2fbe6fc00fe00609d.camel@cybertec.at>
	 <CAFj8pRAsjr0pzQpQeRrQ-wR-ew1oTc6qkW=4355KveN1jKQ+1Q@mail.gmail.com>
	 <c29fb07fb1b55725e175e5625a5261baeab4a977.camel@cybertec.at>
	 <6996931e8c9edf3b82223e74e92326a7ed06c1d6.camel@cybertec.at>
	 <67aa68a7e6dfb44c0cbbdf7f97cadfede4269ce5.camel@cybertec.at>
	 <CAFj8pRDF-F5diNTXr206QfQFauFNK1vu_7NARXd0dtdwGTWLMQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.4 (3.52.4-1.fc40) 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/04ec666686e9e21cb515617df06885c66f3d34ce.camel%40cybertec.at>
Precedence: bulk

On Thu, 2024-08-15 at 07:55 +0200, Pavel Stehule wrote:
> =C3=BAt 30. 7. 2024 v=C2=A021:46 odes=C3=ADlatel Laurenz Albe <laurenz.al=
be@cybertec.at> napsal:
> > - A general reminder: single line comments should start with a lower ca=
se
> > =C2=A0 letter and have to period in the end:
>=20
> Should it be "have not to period in the end" ?

I made a typo.  I mean "have *no* period in the end".

> I fixed almost all without parts related to psql and executor - there alm=
ost all
> current comments break the mentioned rule. So I use the style used in the=
se files.
> I can fix it (if you like it) - or it can be fixed generally in a separat=
ed patch set.

Thanks.  I also noticed that a lot of existing comments break that rule,
so I think that it is fine if you stick with the way that the surrounding
code does it.

> > - Variable as parameter:
> >=20
> > =C2=A0 CREATE VARIABLE var AS date;
> > =C2=A0 LET var =3D current_date;
> > =C2=A0 PREPARE stmt(date) AS SELECT $1;
> > =C2=A0 EXECUTE stmt(var);
> > =C2=A0 ERROR:=C2=A0 paramid of PARAM_VARIABLE param is out of range
> >=20
> > =C2=A0 Is that working as intended?=C2=A0 I don't understand the error =
message.
>=20
> fixed in 0002 patch (variables cannot be used as EXECUTE argument) and 00=
14 (enable usage variables as an argument of EXECUTE)

Thanks.

> > =C2=A0 > --- a/src/backend/catalog/namespace.c
> > =C2=A0 > +++ b/src/backend/catalog/namespace.c
> > =C2=A0 > [...]
> > =C2=A0 > +/*
> > =C2=A0 > + * IdentifyVariable - try to find variable identified by list=
 of names.
> > =C2=A0 > [...]
> >=20
> > =C2=A0 Perhaps part of the reason why this function is so complicated i=
s that
> > =C2=A0 you support things like this:
> >=20
> > =C2=A0 =C2=A0 CREATE SCHEMA sch;
> > =C2=A0 =C2=A0 CREATE TYPE cp AS (a integer, b integer);
> > =C2=A0 =C2=A0 CREATE VARIABLE sch.v AS cp;
> > =C2=A0 =C2=A0 LET sch.v =3D (1, 2);
> > =C2=A0 =C2=A0 SELECT sch.v.b;
> > =C2=A0 =C2=A0 =C2=A0b=20
> > =C2=A0 =C2=A0 =E2=95=90=E2=95=90=E2=95=90
> > =C2=A0 =C2=A0 =C2=A02
> > =C2=A0 =C2=A0 (1 row)
> >=20
> > =C2=A0 =C2=A0 test=3D# SELECT test.sch.v.b;
> > =C2=A0 =C2=A0 =C2=A0b=20
> > =C2=A0 =C2=A0 =E2=95=90=E2=95=90=E2=95=90
> > =C2=A0 =C2=A0 =C2=A02
> > =C2=A0 =C2=A0 (1 row)
> >=20
> > =C2=A0 We don't support that for tables:
> >=20
> > =C2=A0 =C2=A0 CREATE TABLE tab (c cp);
> > =C2=A0 =C2=A0 INSERT INTO tab VALUES (ROW(42, 43));
> > =C2=A0 =C2=A0 SELECT tab.c.b FROM tab;
> > =C2=A0 =C2=A0 ERROR:=C2=A0 cross-database references are not implemente=
d: tab.c.b
> >=20
> > =C2=A0 You have to use extra parentheses:
> >=20
> > =C2=A0 =C2=A0 SELECT (tab.c).b FROM tab;
> > =C2=A0 =C2=A0 =C2=A0b=C2=A0=20
> > =C2=A0 =C2=A0 =E2=95=90=E2=95=90=E2=95=90=E2=95=90
> > =C2=A0 =C2=A0 =C2=A043
> > =C2=A0 =C2=A0 (1 row)
> >=20
> > =C2=A0 I'd say that this should be the same for session variables.
> > =C2=A0 What do you think?
>=20
> I prefer the current state, but I don't have a very strong opinion about =
it.
> It can save 115 lines of almost trivial code, but I think the user experi=
ence
> can be much worse.=C2=A0 Using composite types in tables is not too commo=
n a
> pattern (and when I read some recommendations for Oracle, it is an antipa=
ttern),
> but usage of composite variables is common (it can hold a row). Moreover,
> we talked long about possible identifier's collisions, and the pattern
> schema.variable is very good protection against possible collisions.
> Probably the pattern catalog.schema.variable.field is not too interesting
> but the support has 50 lines.
>=20
> More, the plpgsql supports pattern label.variable.field, so can be little=
 bit
> unfriendly if session variables doesn't support "similar" pattern

I see your point, and I'm fine with leaving it as it is.

>=20
>=20
> > - src/backend/commands/session_variable.c
> >=20
> > =C2=A0 There is one comment that confuses me and that I did not edit:
> >=20
> > =C2=A0 > +Datum
> > =C2=A0 > +GetSessionVariable(Oid varid, bool *isNull, Oid *typid)
> > =C2=A0 > +{
> > =C2=A0 > +=C2=A0 =C2=A0SVariable=C2=A0 =C2=A0svar;
> > =C2=A0 > +
> > =C2=A0 > +=C2=A0 =C2=A0svar =3D get_session_variable(varid);
> > =C2=A0 > +
> > =C2=A0 > +=C2=A0 =C2=A0/*
> > =C2=A0 > +=C2=A0 =C2=A0 * Although svar is freshly validated in this po=
int, the svar->is_valid can
> > =C2=A0 > +=C2=A0 =C2=A0 * be false, due possible accepting invalidation=
 message inside domain
> > =C2=A0 > +=C2=A0 =C2=A0 * check. Now, the validation is done after lock=
, that can also accept
> > =C2=A0 > +=C2=A0 =C2=A0 * invalidation message, so validation should be=
 trustful.
> > =C2=A0 > +=C2=A0 =C2=A0 *
> > =C2=A0 > +=C2=A0 =C2=A0 * For now, we don't need to repeat validation. =
Only svar should be valid
> > =C2=A0 > +=C2=A0 =C2=A0 * pointer.
> > =C2=A0 > +=C2=A0 =C2=A0 */
>=20
> This comment is related to assertions. Before I had there `Assert(svar->i=
s_valid)`,
> because I expected it. But it was not always true. And although it is tru=
e,
> we don't need to validate a variable, because at this moment, the variabl=
e
> should be locked, and then we can return content safely.

I guess my main problem is the word "trustful".  I don't recognize that wor=
d.
Perhaps you can reword the comment along the lines of your above explanatio=
n.

>=20
> > - src/backend/executor/execMain.c
> >=20
> > =C2=A0 > @@ -196,6 +198,51 @@ standard_ExecutorStart(QueryDesc *queryDe=
sc, int eflags)
> > =C2=A0 >=C2=A0 =C2=A0 =C2=A0Assert(queryDesc->sourceText !=3D NULL);
> > =C2=A0 >=C2=A0 =C2=A0 =C2=A0estate->es_sourceText =3D queryDesc->source=
Text;
> > =C2=A0 >
> > =C2=A0 > +=C2=A0 =C2=A0/*
> > =C2=A0 > +=C2=A0 =C2=A0 * The executor doesn't work with session variab=
les directly. Values of
> > =C2=A0 > +=C2=A0 =C2=A0 * related session variables are copied to dedic=
ated array, and this array
> > =C2=A0 > +=C2=A0 =C2=A0 * is passed to executor.
> > =C2=A0 > +=C2=A0 =C2=A0 */
> >=20
> > =C2=A0 Why?=C2=A0 Is that a performance measure?=C2=A0 The comment shou=
ld explain that.
>=20
> Session variables are volatile internally - some function that uses LET s=
tatements
> can be called more times inside a query. This behavior is not consistent =
with
> plpgsql's variables or external parameters. And if we want to support par=
allel
> queries, then volatile session variables can be pretty messy (and unusabl=
e).
> If we want the same behaviour for queries with or without parallel suppor=
t,
> then the session variables should be "stable" (like stable functions).
> Simple implementation is using "snapshot" of values of used session varia=
bles
> when query is started. This "snapshot" is immutable, so we don't need to =
make
> a copy more times.
>=20
> I changed this comment

Thanks.

> > - parallel safety
> >=20
> > =C2=A0 > --- a/src/backend/optimizer/util/clauses.c
> > =C2=A0 > +++ b/src/backend/optimizer/util/clauses.c
> > =C2=A0 > @@ -935,6 +936,13 @@ max_parallel_hazard_walker(Node *node, ma=
x_parallel_hazard_context *context)
> > =C2=A0 >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (param->paramkind =3D=3D P=
ARAM_EXTERN)
> > =C2=A0 >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return false;
> > =C2=A0 >
> > =C2=A0 > +=C2=A0 =C2=A0 =C2=A0 =C2=A0/* We doesn't support passing sess=
ion variables to workers */
> > =C2=A0 > +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (param->paramkind =3D=3D PARAM_=
VARIABLE)
> > =C2=A0 > +=C2=A0 =C2=A0 =C2=A0 =C2=A0{
> > =C2=A0 > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (max_parallel_haz=
ard_test(PROPARALLEL_RESTRICTED, context))
> > =C2=A0 > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return=
 true;
> > =C2=A0 > +=C2=A0 =C2=A0 =C2=A0 =C2=A0}
> >=20
> > =C2=A0 Even if a later patch alleviates that restriction, this patch sh=
ould document that
> > =C2=A0 session variables imply "parallel restricted".=C2=A0 I have adde=
d that to doc/src/sgml/parallel.sgml.
> >=20
>=20
> merged (and removed in 0015)

Thanks.


I hope I can do some more review at some point in the future...

I sincerely hope that this patch set gets merged at some point.
The big obstacle is that it is so large.  That's probably because it is pre=
tty
feature-complete (but, as we have found, not totally free of bugs).

Judging from the amount of time I put into my review so far, I guess that t=
his
patch set would keep a committer busy for several weeks.  Perhaps the only =
way to
get this done would be to make you a committer...

Yours,
Laurenz Albe