Re: proposal: schema variables

po 9. 12. 2024 v 7:16 odesílatel jian he <jian.universality@gmail.com>
napsal:

> On Mon, Dec 9, 2024 at 2:33 AM Pavel Stehule <pavel.stehule@gmail.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.
>

moved to patch 11


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


> ----------------------------------------------
> 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.
>

these features uses generic code, so I didn't wrote regress test, but I
don't see any
reason why don't use your examples in regress tests.


>
> * 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.
>

fixed


>
> * 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.
>

it was wrong, REVOKE VARIABLE should be moved up

fixed


>
>
> *  <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"?
>

done


>
> *   <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"?
>
>
done


> * 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.
>

done

----------------------------------------------
> *
> +   <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>
>
>
moved this para to patch02


>
>
> * 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?
>

I did it to 01 patch.

The line between 01 and 02 patch can be fuzzy sometimes little bit

Just note: applying only the 01 patch does not make any sense.  These
patches (01 and 02) are separated just for review
and testing, but for any usage both patches should be committed or none.



>
>  * 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={}
> ?
>

fixed - there was missing pstrdup(varname) in the related part in
getObjectIdentityParts

I used your example in regress test

new patchset attached with necessary rebase

Regards

Pavel