Re: Schema variables - new implementation for Postgres 15 - Mailing list pgsql-hackers

> On Thu, Aug 03, 2023 at 08:15:13AM +0200, Pavel Stehule wrote:
> Hi
>
> fresh rebase

Thanks for continuing efforts. The new patch structure looks better to
me (although the boundary between patches 0001 and 0002 is somewhat
fuzzy, e.g. the function NameListToString is used already in the first
one, but defined in the second). Couple of commentaries along the way:

* Looks like it's common to use BKI_DEFAULT when defining catalog
entities, something like BKI_DEFAULT(-1) for typmod, BKI_DEFAULT(0) for
collation, etc. Does it make sense to put few default values into
pg_variable as well?

* The first patch contains:

    diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
    @@ -2800,6 +2800,8 @@ AbortTransaction(void)
        AtAbort_Portals();
        smgrDoPendingSyncs(false, is_parallel_worker);
        AtEOXact_LargeObject(false);
    +
    +    /* 'false' means it's abort */
        AtAbort_Notify();
        AtEOXact_RelationMap(false, is_parallel_worker);
        AtAbort_Twophase();

What does the commentary refer to, is it needed?

* I see ExplainOneQuery got a new argument:

     static void ExplainOneQuery(Query *query, int cursorOptions,
    -                            IntoClause *into, ExplainState *es,
    +                            IntoClause *into, Oid targetvar, ExplainState *es,
                                const char *queryString, ParamListInfo params,
                                QueryEnvironment *queryEnv);

From what I understand it represents a potential session variable to be
explained. Isn't it too specific for this interface, could it be put
somewhere else? To be honest, I don't have any suggestions myself, but
it feels a bit out of place here.

* Session variable validity logic is not always clear, at least to me,
producing following awkward pieces of code:

    +        if (!svar->is_valid)
    +        {
    +            if (is_session_variable_valid(svar))
    +                svar->is_valid = true;

I get it as there are two ways how a variable could be invalid?

* It's not always easy to follow which failure modes are taken care of. E.g.

    +     * Don't try to use possibly invalid data from svar. And we don't want to
    +     * overwrite invalid svar immediately. The datumCopy can fail, and in this
    +     * case, the stored value will be invalid still.

I couldn't find any similar precautions, how exactly datumCopy can fail,
are you referring to palloc/memcpy failures?

Another confusing example was this one at the end of set_session_variable:

    +    /*
    +     * XXX While unlikely, an error here is possible. It wouldn't leak memory
    +     * as the allocated chunk has already been correctly assigned to the
    +     * session variable, but would contradict this function contract, which is
    +     * that this function should either succeed or leave the current value
    +     * untouched.
    +     */
    +    elog(DEBUG1, "session variable \"%s.%s\" (oid:%u) has new value",
    +         get_namespace_name(get_session_variable_namespace(svar->varid)),
    +         get_session_variable_name(svar->varid),
    +         svar->varid);

It's not clear, which exactly error you're talking about, it's the last
instruction in the function.

Maybe it would be beneficial to have some overarching description, all
in one place, about how session variables implementation handles various
failures?