Re: Re: proposal: schema variables

Hi

pá 20. 12. 2024 v 8:58 odesílatel jian he <jian.universality@gmail.com>
napsal:

> hi.
> review is based on
> v20241219-0002-Storage-for-session-variables-and-SQL-interface.patch
> and
> v20241219-0001-Enhancing-catalog-for-support-session-variables-and-.patch.
>
> in doc/src/sgml/catalogs.sgml
>
> <row>
> <entry role="catalog_table_entry"><para role="column_definition">
> <structfield>defaclobjtype</structfield> <type>char</type>
> </para>
> <para>
> Type of object this entry is for:
> <literal>r</literal> = relation (table, view),
> <literal>S</literal> = sequence,
> <literal>f</literal> = function,
> <literal>T</literal> = type,
> <literal>n</literal> = schema
> </para></entry>
> </row>
> this need updated for session variable?
>

yes, fixed


>
> psql meta command \ddp
> src/bin/psql/describe.c listDefaultACLs
> also need to change.
>

fixed


> ----------------<<>>-------------------------
> +-- show variable
> +SELECT public.svar;
> +SELECT public.svar.c;
> +SELECT (public.svar).*;
> +
> +-- the variable is shadowed, raise error
> +SELECT public.svar.c FROM public.svar;
> +
> +-- can be fixed by alias
> +SELECT public.svar.c FROM public.svar x
>
> The above tests are duplicated in session_variables.sql.
>

It looks like duplicated test, but it isn't - look on session_variables.out

+-- the variable is shadowed, raise error
+SELECT public.svar.c FROM public.svar;
+ERROR:  column svar.c does not exist
+LINE 1: SELECT public.svar.c FROM public.svar;
+               ^
+-- can be fixed by alias
+SELECT public.svar.c FROM public.svar x;
+  c..
+-----
+ 300
+(1 row)



> ----------------<<>>-------------------------
> --- a/src/include/nodes/plannodes.h
> +++ b/src/include/nodes/plannodes.h
> @@ -49,7 +49,7 @@ typedef struct PlannedStmt
>
>      NodeTag        type;
>
> -    CmdType        commandType;    /*
> select|insert|update|delete|merge|utility */
> +    CmdType        commandType;    /*
> select|let|insert|update|delete|merge|utility */
>
> since we don't have CMD_LET CmdType.
> so this comment change is not necessary?
>

true, removed


> ----------------<<>>-------------------------
> the following are simple tests that triggers makeParamSessionVariable error
> messages. we don't have related tests, we can add it on
> session_variables.sql.
>
> create type t1 as (a int, b int);
> CREATE VARIABLE v1 text;
> CREATE VARIABLE v2 as t1;
> select v1.a;
> select v2.c;
>
>
done


> also
> select v2.c;
> ERROR:  could not identify column "c" in variable
> LINE 1: select v2.c;
>                ^
> the error message is no good. i think we want:
> ERROR:  could not identify column "c" in  variable "public.v1"
>

done

----------------<<>>-------------------------
> +    /*
> +     * Check for duplicates. Note that this does not really prevent
> +     * duplicates, it's here just to provide nicer error message in common
> +     * case. The real protection is the unique key on the catalog.
> +     */
> +    if (SearchSysCacheExists2(VARIABLENAMENSP,
> +                              PointerGetDatum(varName),
> +                              ObjectIdGetDatum(varNamespace)))
> +    {
> +    }
> I am confused by these comments. i think the SearchSysCacheExists2
> do actually prevent duplicates.
> I noticed that publication_add_relation also has similar comments.
>

RowExclusiveLock doesn't block concurrent inserts. So theoretically, two
sessions can try
to create variables with the same name and same schema. Using syscache
cache cannot
protect against this scenario. The real protection is only a unique index.
It is same like

IF NOT EXISTS(SELECT * FROM foo WHERE id = 10) THEN
  INSERT INTO foo(id) VALUES(10)
END IF;

This fragment doesn't protect us against duplicit id in table foo. The real
protection can
be done only by unique index, but when you use this fragment, then you can
reduce
error messages like `unique index violation` and that can benefit in some
scenarios.

I think this comment is correct, but enhancig is welcome.



> ----------------<<>>-------------------------
> typedef struct LetStmt
> {
>     NodeTag        type;
>     List       *target;            /* target variable */
>     Node       *query;            /* source expression */
>     int            location;
> } LetStmt;
> here, location should be a type of ParseLoc.
>

changed


> ----------------<<>>-------------------------
> in 0001, 0002, function SetSessionVariableWithSecurityCheck
> never being used.
>

moved to patch 18 plpgsql-implementation-for-LET-statement

> ----------------<<>>-------------------------
> +/*
> + * transformLetStmt -
> + *      transform an Let Statement
> + */
> +static Query *
> +transformLetStmt(ParseState *pstate, LetStmt *stmt)
> ...
> +    /*
> +     * Save target session variable ID.  This value is used multiple
> times: by
> +     * the query rewriter (for getting related defexpr), by planner (for
> +     * acquiring an AccessShareLock on target variable), and by the
> executor
> +     * (we need to know the target variable ID).
> +     */
> +    query->resultVariable = varid;
>
> "defexpr", do you mean "default expression"?
> Generally letsmt is like: "let variable = (SelectStmt)"
> is there nothing related to the DEFAULT expression?
> "(we need to know the target variable ID)." in ExecuteLetStmt that is true.
> but I commented out the following lines, the regress test still
> succeeded.
>

This comment is partially obsolete - early implementations of LET
statements supports syntax LET var = DEFAULT.
I removed this feature, because the implementation was not trivial and the
benefit is not too high.

I changed this comment to

<-->/*
<--> * Save target session variable ID. It is used later for
<--> * acquiring an AccessShareLock on target variable, setting
<--> * plan dependency and finally for creating VariableDestReceiver.
<--> */



>
> extract_query_dependencies_walker
> /* record dependency on the target variable of a LET command */
> // if (OidIsValid(query->resultVariable))
> //     record_plan_variable_dependency(context, query->resultVariable);
>

I checked regress tests, and plan invalidation is tested there, but not for
target variable.
I enhanced this test to check the invalidation of plans when dropped
variable is used
as target.

SET plan_cache_mode TO force_generic_plan;

LET var1 = 3.14;
SELECT plpgsqlfx();
LET var1 = 3.14 * 2;
SELECT plpgsqlfx();

SELECT plpgsqlfx2(10.0);
SELECT var2;

DROP VARIABLE var1;
DROP VARIABLE var2;

-- dependency (plan invalidation) should work
CREATE VARIABLE var1 AS numeric;
CREATE VARIABLE var2 AS numeric;

LET var1 = 3.14 * 3;
SELECT plpgsqlfx();
LET var1 = 3.14 * 4;
SELECT plpgsqlfx();

SELECT plpgsqlfx2(10.0);
SELECT var2;

DROP VARIABLE var1;
DROP VARIABLE var2;
DROP FUNCTION plpgsqlfx();
DROP FUNCTION plpgsqlfx2();

SET plan_cache_mode TO DEFAULT;





>
> ScanQueryForLocks
> // /* process session variables */
> // if (OidIsValid(parsetree->resultVariable))
> // {
> //     if (acquire)
> //         LockDatabaseObject(VariableRelationId,
> parsetree->resultVariable,
> //                            0, AccessShareLock);
> //     else
> //         UnlockDatabaseObject(VariableRelationId,
> parsetree->resultVariable,
> //                              0, AccessShareLock);
> // }
>

This is protection against dropping an session variable when it is used. I
think so this is tested by isolation tester
in Patch 05




> ----------------<<>>-------------------------
> in transformLetStmt, we already did ACL privilege check,
> we don't need do it again at ExecuteLetStmt?
>

It is redundant, but surely it should be checked in the executor stage. I
removed this check from transformLetStmt.

Regards

Pavel