Re: proposal: schema variables

On Mon, Dec 9, 2024 at 2:33 AM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
> Hi
>
again. only applied
v20241208-0001-Enhancing-catalog-for-support-session-variables-and-.patch.

/* we want SessionVariableCreatePostprocess to see the catalog changes. */
0001 doesn't have SessionVariableCreatePostprocess,
so this comment is wrong in the context of 0001.

typo:
As above, but if the variable isn't found and is_mussing is not NULL
is_mussing should be is_missing.

----------------------------------------------
issue with grant.sgml and revoke.sgml.

* there are no regress tests for WITH GRANT OPTION but it seems to work;
there are no REVOKE CASCADE tests. the following tests show
REVOKE CASCADE works.

create variable v1 as int;
GRANT select on VARIABLE v1 to alice with grant option;
set session authorization alice;
GRANT select on VARIABLE v1 to bob with grant option;
reset session authorization;

select varacl from pg_variable where varname = 'v1';
--output
{jian=rw/jian,alice=r*/jian,bob=r*/alice}
revoke all privileges on variable v1 from alice cascade;
select varacl from pg_variable where varname = 'v1';
--output
{jian=rw/jian}

revoke GRANT OPTION FOR all privileges on variable v1 from alice cascade;
also works.

* in revoke.sgml and grant.sgml.
if you look closely, " | ALL VARIABLES IN SCHEMA schema_name [, ...] }" is wrong
because there is no left-curly-bracket, but there is a right-curly-bracket.

* in revoke.sgml.
<phrase>where <replaceable
class="parameter">role_specification</replaceable> can be:</phrase>
[ GROUP ] <replaceable class="parameter">role_name</replaceable>
| PUBLIC
| CURRENT_ROLE
| CURRENT_USER
| SESSION_USER
should be at the end of the synopsis section?
otherwise it looks weird, maybe we can put the REVOKE VARIABLE code upper.
grant.sgml changes position looks fine to me.

* <para>
The <command>GRANT</command> command has two basic variants: one
that grants privileges on a database object (table, column, view,
foreign table, sequence, database, foreign-data wrapper, foreign server,
function, procedure, procedural language, large object, configuration
parameter, schema, tablespace, or type), and one that grants
membership in a role. These variants are similar in many ways, but
they are different enough to be described separately.
</para>
this <para> in grant.sgml needs to also mention "variable"?

* <para>
Privileges on databases, tablespaces, schemas, languages, and
configuration parameters are
<productname>PostgreSQL</productname> extensions.
</para>
this <para> in grant.sgml needs to also mention "variable"?

----------------------------------------------
*
+ <para>
+ Inside a query or an expression, a session variable can be
+ <quote>shadowed</quote> by a column with the same name. Similarly, the
+ name of a function or procedure argument or a PL/pgSQL variable (see
PL/pgSQL should decorated as <application>PL/pgSQL</application>

* we already support \dV and \dV+ in
v20241208-0001-Enhancing-catalog-for-support-session-variables-and-.patch
so we should update doc/src/sgml/ref/psql-ref.sgml also.
I briefly searched \dV in
v20241208-0002-Storage-for-session-variables-and-SQL-interface.patch,
no entry.

* in doc/src/sgml/ddl.sgml
<table id="privilege-abbrevs-table"> and <table
id="privileges-summary-table"> need to change for variable?
<varlistentry id="ddl-priv-select">, <varlistentry
id="ddl-priv-update"> need to change for variable?

it's kind of tricky. if we only apply
v20241208-0001-Enhancing-catalog-for-support-session-variables-and-.patch
we can not SELECT or UPDATE variable.
so how are we going to mention these privileges for variable?

* we can add some tests for EVENT TRIGGER test,
since event trigger support CREATE|DROP variable. atually, I think
there is a bug.

create variable v1 as int;
CREATE OR REPLACE FUNCTION event_trigger_report_dropped()
RETURNS event_trigger
LANGUAGE plpgsql
AS $$
DECLARE r record;
BEGIN
FOR r IN SELECT * from pg_event_trigger_dropped_objects()
LOOP
IF NOT r.normal AND NOT r.original THEN
CONTINUE;
END IF;
RAISE NOTICE 'NORMAL: orig=% normal=% istemp=% type=% identity=% name=% args=%',
r.original, r.normal, r.is_temporary, r.object_type,
r.object_identity, r.address_names, r.address_args;
END LOOP;
END; $$;

CREATE EVENT TRIGGER regress_event_trigger_report_dropped ON sql_drop
WHEN TAG IN ('DROP VARIABLE')
EXECUTE PROCEDURE event_trigger_report_dropped();

--output:
src9=# drop variable v1;
NOTICE: test_event_trigger: ddl_command_start DROP VARIABLE
NOTICE: NORMAL: orig=t normal=f istemp=f type=session variable
identity=public.v1 name={public,$} args={}
DROP VARIABLE

should i expect
NOTICE: NORMAL: orig=t normal=f istemp=f type=session variable
identity=public.v1 name={public,$} args={}
to be
NOTICE: NORMAL: orig=t normal=f istemp=f type=session variable
identity=public.v1 name={public,v1} args={}
?