Re: Schema variables - new implementation for Postgres 15

čt 8. 9. 2022 v 9:18 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:

> Hi,
>
> On Tue, Sep 06, 2022 at 06:23:12PM +0800, Julien Rouhaud wrote:
> > On Tue, Sep 06, 2022 at 08:43:59AM +0200, Pavel Stehule wrote:
> > > Hi
> > >
> > > After talking with Julian I removed "debug" fields name and nsname from
> > > SVariable structure. When it is possible it is better to read these
> fields
> > > from catalog without risk of obsoletely or necessity to refresh these
> > > fields. In other cases we display only oid of variable instead name and
> > > nsname (It is used just for debug purposes).
> >
> > Thanks!  I'm just adding back the forgotten Cc list.
>
> About the last change:
>
> +static void
> +pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue)
> +{
> [...]
> +           elog(DEBUG1, "session variable \"%s.%s\" (oid:%u) should be
> rechecked (forced by sinval)",
> +
> get_namespace_name(get_session_variable_namespace(svar->varid)),
> +                        get_session_variable_name(svar->varid),
> +                        svar->varid);
>
>
fixed


> There's no guarantee that the variable still exists in cache (for variables
> dropped in the current transaction), or even that the callback is called
> while
> in a transaction state, so we should only display the oid here.
>
> FTR just to be sure I ran all the new regression tests (with this fix)
> with CCA
> and log_min_messages = DEBUG1 and it didn't hit any problem, so it doesn't
> seem
> that there's any other issue hidden somewhere.
>
>
> Other than that I don't see any remaining problems in session_variable.c.
> I
> still have a few nitpicking comments though:
>
> +static SVariable
> +prepare_variable_for_reading(Oid varid)
> +{
> [...]
> +                       /* Store result before releasing Executor memory */
> +                       set_session_variable(svar, value, isnull, true);
> +
> +                       MemoryContextSwitchTo(oldcxt);
> +
> +                       FreeExecutorState(estate);
>
> The comment and code is a bit misleading, as it's not immediately obvious
> that
> set_session_variable() doesn't rely on the current memory contex for
> allocations.  Simply moving the MemoryContextSwitchTo() before the
> set_session_variable() would be better.
>

changed


>
> +typedef struct SVariableData
> +{
> [...]
> +       bool            is_domain;
> +       Oid                     basetypeid;
> +       void       *domain_check_extra;
> +       LocalTransactionId domain_check_extra_lxid;
>
> AFAICS basetypeid isn't needed anymore.
>
>
removed


>
> + /* Both lists hold fields of SVariableXActActionItem type */
> + static List *xact_on_commit_drop_actions = NIL;
> + static List *xact_on_commit_reset_actions = NIL;
>
> Is it possible to merge both in a single list?  I don't think that there's
> much
> to gain trying to separate those.  They shouldn't contain a lot of
> entries, and
> they're usually scanned at the same time anyway.
>
> This is especially important as one of the tricky parts of this patchset is
> maintaining those lists across subtransactions, and since both have the
> same
> heuristics all the related code is duplicated.
>
> I see that in AtPreEOXact_SessionVariable_on_xact_actions() both lists are
> handled interleaved with the xact_recheck_varids, but I don't see any
> reason
> why we couldn't process both action lists first and then process the
> rechecks.
> I did a quick test and don't see any failure in the regression tests.
>

Originally it was not possible, because there was no xact_reset_varids
list, and without this list the processing
ON_COMMIT_DROP started DROP VARIABLE command, and there was a request for
ON_COMMIT_RESET action.
Now, it is possible, because in RemoveSessionVariable is conditional
execution:

<--><--><-->if (!svar->eox_reset)
<--><--><--><-->register_session_variable_xact_action(varid,
<--><--><--><--><--><--><--><--><--><--><--><--><-->  SVAR_ON_COMMIT_RESET);
<--><-->}

So when we process ON_COMMIT_DROP actions, we know that the reset will not
be processed by ON_COMMIT_RESET action,
and then these lists can be merged.

so I merged these two lists to one



>
>
> +void
> +RemoveSessionVariable(Oid varid)
> +{
>
> I looks like a layering violation to have (part of) the code for CREATE
> VARIABLE in pg_variable.[ch] and the code for DROP VARIABLE in
> session_variable.[ch].
>
> I think it was done mostly because it was the initial
> sync_sessionvars_all()
> that was responsible to avoid cleaning up memory for variables dropped in
> the
> current transaction, but that's not a requirement anymore.  So I don't see
> anything preventing us from moving RemoveSessionVariable() in pg_variable,
> and
> export some function in session_variable to do the additional work for
> properly
> maintaining the hash table if needed (with that knowledge held in
> session_variable, not in pg_variable).  You should only need to pass the
> oid of
> the variable and the eoxaction.
>

I am not sure if the proposed change helps. With it I need to break
encapsulation. Now, all implementation details are hidden in
session_variable.c.

I understand that the operation Define and Remove are different from
operations Set and Get, but all are commands, and all need access to
sessionvars and some lists.


>
> Simlarly, why not move DefineSessionVariable() in pg_variable and expose
> some
> API in session_variable to register the needed SVAR_ON_COMMIT_DROP action?
>
> Also, while not a problem I don't think that the CommandCounterIncrement()
> is
> necessary in DefineSessionVariable().  CREATE VARIABLE is a single
> operation
> and you can't have anything else running in the same ProcessUtility() call.
> And since cd3e27464cc you have the guarantee that a
> CommandCounterIncrement()
> will happen at the end of the utility command processing.
>

removed


>
> While at it, maybe it would be good to add some extra tests in
> src/test/modules/test_extensions.  I'm thinking a version 1.0 that creates
> a
> variable and initialize the value (and and extra step after creating the
> extension to make sure that the value is really set), and an upgrade to 2.0
> that creates a temp variable on commit drop, that has to fail due to the
> dependecy on the extension.
>

In updated patches I replaced used cacheMemoryContext by
TopTransactionContext what is more correct (I think)

Regards

Pavel