public inbox for [email protected]
help / color / mirror / Atom feedFrom: Pavel Stehule <[email protected]>
To: Laurenz Albe <[email protected]>
Cc: Erik Rijkers <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: DUVAL REMI <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: proposal: schema variables
Date: Wed, 31 Jul 2024 08:41:50 +0200
Message-ID: <CAFj8pRB4oRVXTqChg0JH0XCKv33assFGr4jmUw3ht+4C6ac2PQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
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>
<[email protected]>
<CAFj8pRAFktynx5wkanv5SRuzXkZgXu77XpVACiSE=v7i1xHFbw@mail.gmail.com>
<CAFj8pRA=bn_g5T2AZduy5gNOQoOnUJ+pMHmnRMHi6mR0n=TAsA@mail.gmail.com>
<[email protected]>
<CAFj8pRC9de05HSb4tEHDUwJ98+4Wh30W-rJrNOPnTz6ARcv0Fw@mail.gmail.com>
<CAFj8pRBC5Wz1xHKKBmKsM0xYN0+PdSZ5oXPsk5SZt+VprdUW3A@mail.gmail.com>
<CAFj8pRAh4pzMoZrKCLt9h+Lr2L=vhgs2PjAF45uLbp_7sijM5w@mail.gmail.com>
<CAFj8pRA-kxQ1oErcuDeUKYsrgwB5XGLhquatwxOe3dCVy1gcyQ@mail.gmail.com>
<[email protected]>
<CAFj8pRBbt2xhY9PyabOY0ZN+Aig6ee3oCon-DM9qi0Uw_3qfbw@mail.gmail.com>
<CAFj8pRDSa52J7kPmCYXgq1BBbu3YBXwpdSOVpjgU=hnE2k04Cw@mail.gmail.com>
<CAFj8pRD+QiWOoPrFk2NnPs3t5Eaf4X=aGRV-9ww11cnPP+fV4g@mail.gmail.com>
<[email protected]>
<CAFj8pRCGTjqHvH9oeiSf4T6Bydhk9pm033DxxibgF+B7SHC6MQ@mail.gmail.com>
<CAFj8pRAzNDhFgbZnT0T0mJ7ygA1Qje1Hc0TiKwXM8++kGooPYg@mail.gmail.com>
<[email protected]>
<CAFj8pRBarjJYfkN-0-i=JRZJ4PTOYC+K7XgAhfdDqWGqRiPkyw@mail.gmail.com>
<CAFj8pRDqdWdCULxd5asbKs5C4e9kT2TuKBkR5L-e1=hP5wF2uw@mail.gmail.com>
<[email protected]>
<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>
<[email protected]>
<CAFj8pRBaD0_bMrCREWnVLfcTMdc0v7ns7Rt=sEvd1EoFmLfarQ@mail.gmail.com>
<[email protected]>
<CAFj8pRDXo-RRcy2VFDm_vzv3Eaaz6Ex=X19up=x8W4COyBNmaQ@mail.gmail.com>
<[email protected]>
<CAFj8pRDgraz64HHcCoFGCKDM1Cxv1ukELsqfsxux4JP3PM6RyQ@mail.gmail.com>
<[email protected]>
<CAFj8pRAsjr0pzQpQeRrQ-wR-ew1oTc6qkW=4355KveN1jKQ+1Q@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
út 30. 7. 2024 v 21:46 odesílatel Laurenz Albe <[email protected]>
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 = 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 several
> purposes (during parsing, in ANALYZE, and to identify shadowed variables
> 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 = (1, 2);
> SELECT sch.v.b;
> b
> ═══
> 2
> (1 row)
>
> test=# SELECT test.sch.v.b;
> b
> ═══
> 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
> ════
> 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)) !=
> 0)
> + {
> + if (!noerror)
> + ereport(ERROR,
> +
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cross-database references
> 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 = 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 = 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 != NULL);
> > estate->es_sourceText = 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 == PARAM_EXTERN)
> > return false;
> >
> > + /* We doesn't support passing session variables to workers */
> > + if (param->paramkind == 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
>
view thread (439+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: proposal: schema variables
In-Reply-To: <CAFj8pRB4oRVXTqChg0JH0XCKv33assFGr4jmUw3ht+4C6ac2PQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox