Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sZ32e-00G3pj-TI for pgsql-hackers@arkaria.postgresql.org; Wed, 31 Jul 2024 06:42:32 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1sZ32d-001vdX-8F for pgsql-hackers@arkaria.postgresql.org; Wed, 31 Jul 2024 06:42:31 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sZ32c-001vdP-Rl for pgsql-hackers@lists.postgresql.org; Wed, 31 Jul 2024 06:42:30 +0000 Received: from mail-yb1-xb2d.google.com ([2607:f8b0:4864:20::b2d]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1sZ32a-002O7h-WF for pgsql-hackers@lists.postgresql.org; Wed, 31 Jul 2024 06:42:30 +0000 Received: by mail-yb1-xb2d.google.com with SMTP id 3f1490d57ef6-e0b9589a72dso2299575276.3 for ; Tue, 30 Jul 2024 23:42:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1722408147; x=1723012947; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=7B42OZmnTqYGgQWmDWmKL4yOe41otfFcKID3Pqfuue0=; b=jNNq2nZdGZ3L3aa0u46Qdk8lY7NB3VN7Oy/t4OQ1W6BIeuwD1x3x4f2eg0S4J2Cc3Q G5GrojXnhbmLZn72Y4EMRElT1owGFkoK7CMrLlXhRNyew/3jIvUyY+HbkGd8xYKa3rd3 1hvI2JJOztbb3wCClTe3FU8A1+AXPVSaSlKAKvmAP5UdZ9nNlq2RavfaGeDh7D6OI5eB MjvT/OucRmairPE3xz5tITVqgmZIRHGPGEE1TVJYWtWiz+NG3FmPSvyqYom2eX3XmUw9 P/bVZFoPS8kBFZpx7CROI768rn7MCTkS5Xnmzo7gpVefG7asdWjaT0A+WpWUUxDeoPVG kgyw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722408147; x=1723012947; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=7B42OZmnTqYGgQWmDWmKL4yOe41otfFcKID3Pqfuue0=; b=JEXmgRV3ROhNxPXkWTmBLj/yHUaEa1eBW8Som0V3nRmvo0Qa13hCHwzLUh9aqO07UV B31Y/3/nx0mutiKY0zV43b2FVGutp3jxSPyBYVwwkd05j7J2FMw0/AQVmAcbmTp97wts HFWR1v8+1s8lPCCC6LB47eyyKlQlD++LBsGK+nET/sg2c/H3bAkA86daE6j/cJ80PsBf BTLtrPkZsaPxZkz5gOxMzv5C43ejz7ltxzAJ1SixbdEqQ0ijfC570a16dq1hOEiyWaIJ q/0/tbq5K6el8iMHbbG0mM2jg40h45jgZP/xi3EC3+RlBWCYuy/w5gms/bccCVLUsbJh Y7hQ== X-Forwarded-Encrypted: i=1; AJvYcCWQsTZ+txkjCboY4LQRk43uLcXLp8zeo2u/GeF4i96jBScJoiDJ9Fz9TqdLonu+cI0vkCp6rY/GWkMe9y22omEAfbDNfHGssEE7e9NiJrSj5Dh/ X-Gm-Message-State: AOJu0Yy6mWCSYlGBVM5/7ue7Hfmdevazfja2TWdD2+bX7aVEhIma2nQ1 0XM42wnl78+kPye4cDxKbbs6v45R2FoGGPJArIBlRKlyJPYKpc+ke0HsIZRP3sTnZft3YlDEsVl NFhQKVhM3YJovxfw7hFyrQO3zI3w= X-Google-Smtp-Source: AGHT+IG+eRlv8c67mWlJWrfFeAVlOJIld9Y7acBWx0G7FyMuxu84fE8ZGXs4zfMTMhcN1U7LWLGZPPWIeJrHMXppbbI= X-Received: by 2002:a05:6902:11ca:b0:e0b:be99:1fd3 with SMTP id 3f1490d57ef6-e0bbe992080mr435001276.48.1722408146977; Tue, 30 Jul 2024 23:42:26 -0700 (PDT) MIME-Version: 1.0 References: <20200924035637.GF28585@paquier.xyz> <20201001033824.GC8130@paquier.xyz> <51a9a68e8a998d04df17417d45c1dbd4@xs4all.nl> <89817942c99da01cd5e7850fe418436b@xs4all.nl> <56ca532c37eb0b540961f74a7bd5db39@xs4all.nl> <8181bd3abc647bdae5a4f78e71e62478a98c75f4.camel@cybertec.at> <9e67d49deb18270eddb95e602c83f02b98459843.camel@cybertec.at> <3b662dc5b615d4c20a55e8e2fbe6fc00fe00609d.camel@cybertec.at> <6996931e8c9edf3b82223e74e92326a7ed06c1d6.camel@cybertec.at> <67aa68a7e6dfb44c0cbbdf7f97cadfede4269ce5.camel@cybertec.at> In-Reply-To: <67aa68a7e6dfb44c0cbbdf7f97cadfede4269ce5.camel@cybertec.at> From: Pavel Stehule Date: Wed, 31 Jul 2024 08:41:50 +0200 Message-ID: Subject: Re: proposal: schema variables To: Laurenz Albe Cc: Erik Rijkers , Michael Paquier , Amit Kapila , DUVAL REMI , PostgreSQL Hackers Content-Type: multipart/alternative; boundary="000000000000c24e87061e856557" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000c24e87061e856557 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable =C3=BAt 30. 7. 2024 v 21:46 odes=C3=ADlatel Laurenz Albe napsal: > This is my review of the second patch in the series. > > Again, I mostly changed code comments and documentation. > > Noteworthy remarks: > > - A general reminder: single line comments should start with a lower case > letter and have to period in the end: > > /* this is a typical single line comment */ > > - Variable as parameter: > > CREATE VARIABLE var AS date; > LET var =3D current_date; > PREPARE stmt(date) AS SELECT $1; > EXECUTE stmt(var); > ERROR: paramid of PARAM_VARIABLE param is out of range > > Is that working as intended? I don't understand the error message. > > Perhaps there is a bug in src/backend/executor/execExpr.c. > > - IdentifyVariable > > > --- a/src/backend/catalog/namespace.c > > +++ b/src/backend/catalog/namespace.c > > [...] > > +/* > > + * IdentifyVariable - try to find variable identified by list of > names. > > [...] > > The function comment doesn't make clear to me what the function does. > Perhaps that is so confusing because the function seems to serve severa= l > purposes (during parsing, in ANALYZE, and to identify shadowed variable= s > in a later patch). > > I rewrote the function comment, but didn't touch the code or code > comments. > > Perhaps part of the reason why this function is so complicated is that > you support things like this: > > CREATE SCHEMA sch; > CREATE TYPE cp AS (a integer, b integer); > CREATE VARIABLE sch.v AS cp; > LET sch.v =3D (1, 2); > SELECT sch.v.b; > b > =E2=95=90=E2=95=90=E2=95=90 > 2 > (1 row) > > test=3D# SELECT test.sch.v.b; > b > =E2=95=90=E2=95=90=E2=95=90 > 2 > (1 row) > > We don't support that for tables: > > CREATE TABLE tab (c cp); > INSERT INTO tab VALUES (ROW(42, 43)); > SELECT tab.c.b FROM tab; > ERROR: cross-database references are not implemented: tab.c.b > > You have to use extra parentheses: > > SELECT (tab.c).b FROM tab; > b > =E2=95=90=E2=95=90=E2=95=90=E2=95=90 > 43 > (1 row) > > I'd say that this should be the same for session variables. > What do you think? > > Code comments: > > - There are lots of variables declared at the beginning, but they are > only > used in code blocks further down. For clarity, you should declare a > variable in the code block where it is used. > > - + /* > + * In this case, "a" is used as catalog name - > check it. > + */ > + if (strcmp(a, get_database_name(MyDatabaseId)) != =3D > 0) > + { > + if (!noerror) > + ereport(ERROR, > + > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + errmsg("cross-database reference= s > are not implemented: %s", > + NameListToString(names)))= ); > + } > + else > + { > > There needn't be an "else", since the ereport() will never return. > Less indentation is better. > > - src/backend/commands/session_variable.c > > There is one comment that confuses me and that I did not edit: > > > +Datum > > +GetSessionVariable(Oid varid, bool *isNull, Oid *typid) > > +{ > > + SVariable svar; > > + > > + svar =3D get_session_variable(varid); > > + > > + /* > > + * Although svar is freshly validated in this point, the > svar->is_valid can > > + * be false, due possible accepting invalidation message inside > domain > > + * check. Now, the validation is done after lock, that can also > accept > > + * invalidation message, so validation should be trustful. > > + * > > + * For now, we don't need to repeat validation. Only svar should > be valid > > + * pointer. > > + */ > > + Assert(svar); > > + > > + *typid =3D svar->typid; > > + > > + return copy_session_variable_value(svar, isNull); > > +} > > - src/backend/executor/execMain.c > > > @@ -196,6 +198,51 @@ standard_ExecutorStart(QueryDesc *queryDesc, int > eflags) > > Assert(queryDesc->sourceText !=3D NULL); > > estate->es_sourceText =3D queryDesc->sourceText; > > > > + /* > > + * The executor doesn't work with session variables directly. > Values of > > + * related session variables are copied to dedicated array, and > this array > > + * is passed to executor. > > + */ > > Why? Is that a performance measure? The comment should explain that. > > - parallel safety > > > --- a/src/backend/optimizer/util/clauses.c > > +++ b/src/backend/optimizer/util/clauses.c > > @@ -935,6 +936,13 @@ max_parallel_hazard_walker(Node *node, > max_parallel_hazard_context *context) > > if (param->paramkind =3D=3D PARAM_EXTERN) > > return false; > > > > + /* We doesn't support passing session variables to workers */ > > + if (param->paramkind =3D=3D PARAM_VARIABLE) > > + { > > + if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, > context)) > > + return true; > > + } > > Even if a later patch alleviates that restriction, this patch should > document that > session variables imply "parallel restricted". I have added that to > doc/src/sgml/parallel.sgml. > > Attached are the first two patches with my edits (the first patch is > unchanged; > I just add it again to keep the cfbot happy. > Probably you didn't attach new files - the second patch is not complete. Or you didn't make changes there? Regards Pavel > > I hope to get to review the other patches at some later time. > > Yours, > Laurenz Albe > --000000000000c24e87061e856557 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
=C3=BAt 30. 7. 2024 v=C2=A021:46 odes= =C3=ADlatel Laurenz Albe <la= urenz.albe@cybertec.at> napsal:
This is my review of the second patch in the series.=

Again, I mostly changed code comments and documentation.

Noteworthy remarks:

- A general reminder: single line comments should start with a lower case =C2=A0 letter and have to period in the end:

=C2=A0 /* this is a typical single line comment */

- Variable as parameter:

=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

=C2=A0 Is that working as intended?=C2=A0 I don't understand the error = message.

=C2=A0 Perhaps there is a bug in src/backend/executor/execExpr.c.

- IdentifyVariable

=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 > [...]

=C2=A0 The function comment doesn't make clear to me what the function = does.
=C2=A0 Perhaps that is so confusing because the function seems to serve sev= eral
=C2=A0 purposes (during parsing, in ANALYZE, and to identify shadowed varia= bles
=C2=A0 in a later patch).

=C2=A0 I rewrote the function comment, but didn't touch the code or cod= e comments.

=C2=A0 Perhaps part of the reason why this function is so complicated is th= at
=C2=A0 you support things like this:

=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
=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)

=C2=A0 =C2=A0 test=3D# SELECT test.sch.v.b;
=C2=A0 =C2=A0 =C2=A0b
=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)

=C2=A0 We don't support that for tables:

=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 implemented: t= ab.c.b

=C2=A0 You have to use extra parentheses:

=C2=A0 =C2=A0 SELECT (tab.c).b FROM tab;
=C2=A0 =C2=A0 =C2=A0b=C2=A0
=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)

=C2=A0 I'd say that this should be the same for session variables.
=C2=A0 What do you think?

=C2=A0 Code comments:

=C2=A0 - There are lots of variables declared at the beginning, but they ar= e only
=C2=A0 =C2=A0 used in code blocks further down.=C2=A0 For clarity, you shou= ld declare a
=C2=A0 =C2=A0 variable in the code block where it is used.

=C2=A0 - +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0/*
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 * In this case, "a" is used as catalog name - check it= .
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 */
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0if (strcmp(a, get_database_name(MyDatabaseId)) !=3D 0)
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0{
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0if (!noerror)
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ereport(ERROR,
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(errcode(= ERRCODE_FEATURE_NOT_SUPPORTED),
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 errmsg(&= quot;cross-database references are not implemented: %s",
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0NameListToString(names))));
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0}
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0else
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0{

=C2=A0 =C2=A0 There needn't be an "else", since the ereport()= will never return.
=C2=A0 =C2=A0 Less indentation is better.

- src/backend/commands/session_variable.c

=C2=A0 There is one comment that confuses me and that I did not edit:

=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 poi= nt, 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 validatio= n. Only svar should be valid
=C2=A0 > +=C2=A0 =C2=A0 * pointer.
=C2=A0 > +=C2=A0 =C2=A0 */
=C2=A0 > +=C2=A0 =C2=A0Assert(svar);
=C2=A0 > +
=C2=A0 > +=C2=A0 =C2=A0*typid =3D svar->typid;
=C2=A0 > +
=C2=A0 > +=C2=A0 =C2=A0return copy_session_variable_value(svar, isNull);=
=C2=A0 > +}

- src/backend/executor/execMain.c

=C2=A0 > @@ -196,6 +198,51 @@ standard_ExecutorStart(QueryDesc *queryDes= c, int eflags)
=C2=A0 >=C2=A0 =C2=A0 =C2=A0Assert(queryDesc->sourceText !=3D NULL);<= br> =C2=A0 >=C2=A0 =C2=A0 =C2=A0estate->es_sourceText =3D queryDesc->s= ourceText;
=C2=A0 >
=C2=A0 > +=C2=A0 =C2=A0/*
=C2=A0 > +=C2=A0 =C2=A0 * The executor doesn't work with session var= iables directly. Values of
=C2=A0 > +=C2=A0 =C2=A0 * related session variables are copied to dedica= ted array, and this array
=C2=A0 > +=C2=A0 =C2=A0 * is passed to executor.
=C2=A0 > +=C2=A0 =C2=A0 */

=C2=A0 Why?=C2=A0 Is that a performance measure?=C2=A0 The comment should e= xplain that.

- parallel safety

=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, max= _parallel_hazard_context *context)
=C2=A0 >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (param->paramkind =3D=3D= PARAM_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 s= ession variables to workers */
=C2=A0 > +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (param->paramkind =3D=3D PARA= M_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_haza= rd_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}

=C2=A0 Even if a later patch alleviates that restriction, this patch should= document that
=C2=A0 session variables imply "parallel restricted".=C2=A0 I hav= e added that to doc/src/sgml/parallel.sgml.

Attached are the first two patches with my edits (the first patch is unchan= ged;
I just add it again to keep the cfbot happy.

Probably you didn't attach new files - the second patch is not co= mplete. Or you didn't make changes there?

Rega= rds

Pavel

=C2=A0
<= /div>

I hope to get to review the other patches at some later time.

Yours,
Laurenz Albe
--000000000000c24e87061e856557--