Re: Re: proposal: schema variables

Hi

čt 6. 2. 2025 v 15:49 odesílatel jian he <jian(dot)universality(at)gmail(dot)com>
napsal:

> hi.
>
> git diff --check
> shows there is some white space there.
>

yes, it was few times in documentation

fixed

>
> Adding hack mailing list discussion links in each patch would be great.
>

Unfortunately, it is not separated. The organization of patches is not
mapped to discussion, because originally there was one big patch that was
divided
and reorganized. So there was no separate discussion about catalog or
syntax. It was a pelmel and bikeshedding (lot of bikeshedding).

> Others said that the first 6 patches are essential, maybe we can only
> post the first 6 patches.
> so the test CI result would be more reliable. also people would not
> feel intimidated by
> the bigger patchset.
>
>
I understand, but I am not sure if it is a good idea. I would not repeat a
lot of bikeshedding like variables should be immutable, transactional, ...
etc..

Maybe I can repeat some higher level description of this patchset

Really essential are just patches 01-02. Just with these patches the user
gets almost all necessary and required functionality - possibility to work
with declared variables with access rights.
I don't think these patches can be reduced - maybe access rights can be
separated, but if you check source code, the part related to access rights
is few lines (and access rights cannot be supported in next steps due
possible compatibility issues). These patches are almost very simple -
mainly 01 - it is mechanical work related to maintaining a new database
class - some code, and a lot of tests.

Patches 03-05 are related to memory cleaning when variables are dropped. It
is important from a technical quality perspective, but until temporary
variables are supported the variables are not usually dropped frequently,
and then real impact for usage is not extra big. DISCARD VARIABLES can be
important for applications that use connection pooling, but this patch is
very simple again.
Patch05 is not long but does difficult work - sinval processing, cache
invalidation, contains isolation tests. It is probably the most difficult
part of this patchset. If we accept a small memory leak after the dropped
variable, then we can live without this patch (not forever - but I can
imagine passing this patch to the next major release). This is a necessary
prerequisite for temporary variables.

Patch 06 contains just plpgsql regress tests - nothing special or difficult.

Patches 07-09 can be important from user friendly usage perspective -
detection of shaded variables can help with unwanted shadowing, variable
fences can strongly reduce any potential risks of introduction of session
variables. I think variable fences can be an interesting safety tool,
concept - these patches are not small (+/- 20kB), but the code is a minimal
part of these patches. I think it can be very practical and pragmatic to
support variable fences from the start by requiring it outside SPI by
default. From my perspective, it is more important than variable shadowing
detection.

Patches 10-11 EXPLAIN, PREPARE support for LET statements are interesting
for consistency with other statements. Nothing special, nothing difficult,
nothing extra important, but interesting for consistent behaviour of all
PostgreSQL commands.

Patches 12-15 - temp variables, immutable variables, default values
support, reset at transaction end introduces a very interesting feature
that allows to do some special work with session variables and increases a
comfort for work with session variables. These patches can be postponed or
applied independently. There is not fully agreement on the syntax RESET AT
TRANSACTION END, but this is just one patch from this set, and other
patches don't depend on this patch.

Patches 16-19 are important from performance perspective and significantly
increases the performance of queries that contain session variables or the
performance of LET statements for some special cases (like simple
expressions from PLpgSQL, or parallel query execution). These patches have
zero impact on functionality, but significant impact on performance. These
patches are separated because of some deeper changes, and can be postponed,
so they are removed from patch 02. These patches are independent of others
- and can be applied later .. it is typical for PLpgSQL so the performance
was enhanced step by step from relatively bad to really good now.

Patch 20 enhances error messages to support variables - nothing special,
nothing important - these messages are not fully correct now against
plpgsql variables, and nobody protests.

Patch 21 supports transactional behaviour to session variables (the content
can be transactional). It is an unusual feature, other databases don't have
similar functionality, but for some cases can be nice, and can introduce
mutable transactional storage on read only replicas.

Patch 22 introduces the pg_dump option - short simple patch just for
consistency with already supported options of pg_dump. Nothing important,
nothing complex.

So really essential are patches 01, 02, 04. These patch holds almost all
valuable functionality that the people want
03, 05 are important for technical consistency - developers doesn't like
memory leaks after dropped objects (but I think real impact will be small
or +/- nothing)
08, 09 can be interesting for the possibility to force some good (safe)
style of usage from the beginning.

Others are interesting, important, but can be postponed or applied
independently. Some one can prefer functionality, someone can prefer
performance. For performance testing patches 16-19 can be interesting,
important (the performance in plpgsql is very significantly different if
the query is executed by SPI or directly - on second hand, plpgsql is not
used by majority users now). But they are separated because it is about 25%
of (01+02), and the code there is not trivial like in 01 and 02.

For domain behaviour check and other issues I need little bit more time

Regards

Pavel

>
> create domain dnn as int not null;
> CREATE VARIABLE var3 AS dnn;
> alter domain dnn drop not null;
> alter domain dnn set not null;
> ERROR: cannot alter domain "dnn" because session variable "public.var3"
> uses it
>
> This is not great.
> we allow a constraint, then we drop it, now we cannot re add it again.
> 0001 and 0002 are simple, but the size is still large.
> maybe we can not support the domain in the first 2 patches.
>
> also
> CREATE VARIABLE var3 AS dnn;
> let var3 = 11;
> create view v1 as select var3;
> select * from v1;
> you can see reconnect to session then
> ERROR: domain dnn does not allow null values
> this is not ok?
>
>
> also
> create table t(var3 int);
> CREATE POLICY p1r ON t AS RESTRICTIVE TO alice USING (var3 <> var3);
> create table t1(a int);
> CREATE POLICY p2 ON t1 AS RESTRICTIVE TO alice USING (a <> var3);
> p1r is so confusing. there is no way to understand the intention.
> p2 should also not be allowed, since var3 value is volatile,
> session reconnection will change the value.
>
>
> src/bin/pg_dump/pg_dump.h
> /*
> * The VariableInfo struct is used to represent session variables
> */
> typedef struct _VariableInfo
> {
> DumpableObject dobj;
> DumpableAcl dacl;
> Oid vartype;
> char *vartypname;
> char *varacl;
> char *rvaracl;
> char *initvaracl;
> char *initrvaracl;
> Oid varcollation;
> const char *rolname; /* name of owner, or empty string */
> } VariableInfo;
> these fields (varacl, rvaracl, initvaracl, initrvaracl) were never being
> used.
> we can remove them.
>
> CollInfo *coll;
> coll = findCollationByOid(varcollation);
> if (coll)
> appendPQExpBuffer(query, " COLLATE %s",
> fmtQualifiedDumpable(coll));
> here, it should be
> ```CollInfo *coll = NULL;```?
>
>
> I don't understand the changes made in getAdditionalACLs.
> I thought pg_init_privs had nothing to do with the session variable.
>
>
> minor issue in getVariables.
> query = createPQExpBuffer();
> resetPQExpBuffer(query);
> no need to use resetPQExpBuffer here.
>
>
> create type ab as (a int, b text);
> create type abc as (a ab, c text);
> create type abcd as (a abc, d text);
> CREATE VARIABLE var2 AS abcd;
>
> select var2.a.c;
> ERROR: cross-database references are not implemented: var2.a.c
>
> Is this error what we expected? I am not 100% sure.
> --------------------another contrived corner case.-----------------------
> create type pg_variable as (
> oid oid, vartype oid, varcreate_lsn pg_lsn,
> varname name, varnamespace oid, varowner oid,
> vartypmod int, varcollation oid, varacl aclitem[]);
> create variable pg_variable as pg_variable;
> let pg_variable = row (18041, 10116, '0/25137B0','pg_variable', 2200,
> 10,-1,0, NULL)::pg_variable;
>
> select pg_variable.oid from pg_variable where pg_variable.oid =
> pg_variable.oid;
> this query, the WHERE clause, it's really hard to distinguish session
> variable or column reference.
> I am not sure if this is fine or not.
>