Re: Schema variables - new implementation for Postgres 15

st 2. 2. 2022 v 15:09 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:

> Hi,
>
> On Sun, Jan 30, 2022 at 08:09:18PM +0100, Pavel Stehule wrote:
> >
> > rebase after 02b8048ba5dc36238f3e7c3c58c5946220298d71
>
> Here are a few comments, mostly about pg_variable.c and
> sessionvariable.c.  I
> stopped before reading the whole patch as I have some concern about the
> sinval
> machanism, which ould change a bit the rest of the patch.  I'm also
> attaching a
> patch (with .txt extension to avoid problem with the cfbot) with some
> comment
> update propositions.
>

merged, thank you


>
> In sessionvariable.c, why VariableEOXAction and VariableEOXActionCodes?
> Can't
> the parser emit directly the char value, like e.g. relpersistence?
>
>
good idea, it reduces some not too useful code.

removed



> extraneous returns for 2 functions:
>
> +void
> +get_session_variable_type_typmod_collid(Oid varid, Oid *typid, int32
> *typmod,
> +                                       Oid *collid)
> +{
> [...]
> +   return;
> +}
>
> +void
> +initVariable(Variable *var, Oid varid, bool fast_only)
> +{
> [...]
> +   return;
> +}
>

removed, fixed


> VariableCreate():
>
> Maybe add a bunch of AssertArg() for all the mandatory parametrers?
>
>
done



> Also, the check for variable already existing should be right after the
> AssertArg(), and using SearchSysCacheExistsX().
>
> Maybe also adding an Assert(OidIsValid(xxxoid)) just after the
> CatalogTupleInsert(), similarly to some other creation functions?
>
>
>
done


> event-triggers.sgml needs updating for the firing matrix, as session
> variable
> are compatible with even triggers.
>

done


>
> +typedef enum SVariableXActAction
> +{
> +   ON_COMMIT_DROP,     /* used for ON COMMIT DROP */
> +   ON_COMMIT_RESET,    /* used for DROP VARIABLE */
> +   RESET,              /* used for ON TRANSACTION END RESET */
> +   RECHECK             /* recheck if session variable is living */
> +} SVariableXActAction;
>
> The names seem a bit generic, maybe add a prefix like SVAR_xxx?
>

done


>
> ON_COMMIT_RESET is also confusing as it looks like an SQL clause.  Maybe
> PERFORM_DROP or something?
>
>
In this case, I think so the name of this variable is accurate.

see comment

<-->/*
<--> * and if this transaction or subtransaction will be committed,
<--> * we want to enforce variable cleaning. (we don't need to wait for
<--> * sinval message). The cleaning action for one session variable
<--> * can be repeated in the action list, and it doesn't do any problem
<--> * (so we don't need to ensure uniqueness). We need separate action
<--> * than RESET, because RESET is executed on any transaction end,
<--> * but we want to execute cleaning only when thecurrent transaction
<--> * will be committed.
<--> */
<-->register_session_variable_xact_action(varid, SVAR_ON_COMMIT_RESET);



> +static List *xact_drop_actions = NIL;
> +static List *xact_reset_actions = NIL;
>
> Maybe add a comment saying both are lists of SVariableXActAction?
>

done


>
> +typedef SVariableData * SVariable;
>
> looks like a missing bump to typedefs.list.
>

done

>
> +char *
> +get_session_variable_name(Oid varid)
> +{
> +   HeapTuple   tup;
> +   Form_pg_variable varform;
> +   char       *varname;
> +
> +   tup = SearchSysCache1(VARIABLEOID, ObjectIdGetDatum(varid));
> +
> +   if (!HeapTupleIsValid(tup))
> +       elog(ERROR, "cache lookup failed for session variable %u", varid);
> +
> +   varform = (Form_pg_variable) GETSTRUCT(tup);
> +
> +   varname = NameStr(varform->varname);
> +
> +   ReleaseSysCache(tup);
> +
> +   return varname;
> +}
>
> This kind of function should return a palloc'd copy of the name.
>

fixed


> +void
> +ResetSessionVariables(void)
> [...]
> +   list_free_deep(xact_drop_actions);
> +   xact_drop_actions = NIL;
> +
> +   list_free_deep(xact_reset_actions);
> +   xact_drop_actions = NIL;
> +}
>
> The 2nd chunk should be xact_reset_actions = NIL
>

fixed


>
> +static void register_session_variable_xact_action(Oid varid,
> SVariableXActAction action);
> +static void delete_session_variable_xact_action(Oid varid,
> SVariableXActAction action);
>
> The naming is a bit confusing, maybe
> unregister_session_cable_xact_action() for
> consistency?
>

changed


>
> +void
> +register_session_variable_xact_action(Oid varid,
> +                                     SVariableXActAction action)
>
> the function is missing the static keyword.
>

fixed


>
> In AtPreEOXact_SessionVariable_on_xact_actions(), those 2 instructions are
> executed twice (once in the middle and once at the end):
>
>         list_free_deep(xact_drop_actions);
>         xact_drop_actions = NIL;
>
>
fixed


>
>
> +    * If this entry was created during the current transaction,
> +    * creating_subid is the ID of the creating subxact; if created in a
> prior
> +    * transaction, creating_subid is zero.
>
> I don't see any place in the code where creating_subid can be zero? It
> looks
> like it's only there for future transactional implementation, but for now
> this
> attribute seems unnecessary?
>

The comment is not 100% valid. I removed the sentence about zero value of
creating_subid.

I think so this attribute is necessary for correct behave, because these
related actions lists should be always correct - you should not to drop
variables 2x

and there are possible things like

begin;
create variable xx as int on transaction end reset;
let xx =100;
select xx;
savepoint s1;
drop variable xx;
rollback to s1;
rollback;

In the first version I had simplified code, and I remember, there was a
problem when variables were modified in subtransaction or dropped, then I
got messages related to missing objects. Implemented code is based on an
already used pattern in Postgres.


>                 /* at transaction end recheck sinvalidated variables */
>                 RegisterXactCallback(sync_sessionvars_xact_callback, NULL);
>
> I don't think it's ok to use xact callback for in-core code.  The function
> explicitly says:
>
> > * These functions are intended for use by dynamically loaded modules.
> > * For built-in modules we generally just hardwire the appropriate calls
> > * (mainly because it's easier to control the order that way, where
> needed).
>

It was a serious issue - after checking, I removed all related code. The
sinval handler is called without hash only after ANALYZE command. In this
case, we don't need to run any action.


> Also, this function and AtPreEOXact_SessionVariable_on_xact_actions() are
> skipping all or part of the processing if there is no active transaction.
> Is
> that really ok?
>

This part was +/- ok, although I can use just isCommit, but there was a
bug. I cannot clean xact_reset_actions every time. It can be done just when
isCommit. I fixed this issue
Fixed memory leaks there.


>
> I'm particularly sceptical about
> AtPreEOXact_SessionVariable_on_xact_actions
> and the RECHECK actions, as the xact_reset_actions list is reset whether
> the
> recheck was done or not, so it seems to me that it could be leaking some
> entries in the hash table.  If the database has a lot of object, it seems
> possible (while unlikely) that a subsequent CREATE VARIABLE can get the
> same
> oid leading to incorrect results?
>
>
it was buggy, I fixed it


> If that's somehow ok, wouldn't it be better to rearrange the code to call
> those
> functions less often, and only when they can do their work, or at least
> split
> the recheck in some different function / list?
>
> +static void
> +pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue)
> [...]
> +   if (hashvalue != 0)
> +   {
> [...]
> +   }
> +   else
> +       sync_sessionvars_all = true;
>
> The rechecks being somewhat expensive, I think it could be a win to remove
> all
> pending rechecks when setting the sync_sessionvars_all.
>

I removed it

I am sending an updated and rebased patch.

Regards

Pavel