Postgres Pro
Downloads
Home   >   mailing lists

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

From Pavel Stehule
Subject Re: Schema variables - new implementation for Postgres 15
Date
Msg-id CAFj8pRD+G+thdanhSTyjqXSVexgo-q5VcBJwA9yaufLAUkuS2A@mail.gmail.com
Whole thread Raw
In response to Re: Schema variables - new implementation for Postgres 15  (Dmitry Dolgov <9erthalion6@gmail.com>)
Responses Re: Schema variables - new implementation for Postgres 15
List pgsql-hackers
Tree view
Hi

pá 11. 8. 2023 v 17:58 odesílatel Dmitry Dolgov <9erthalion6@gmail.com> napsal:
> 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:

NameListToString is already buildin function. Do you think NamesFromList?

This is my oversight - there is just `+extern List *NamesFromList(List *names); ` line, but sure - it should be in 0002 patch

fixed now

For all patches I tested the possibility to compile without following patches, but this issue was not reported by the compiler.

First patch is related to the system catalog - so you can create, drop, and backup session variables. Second patch is dedicated to possibility to store and use an value to session variable


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

done


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

it was wrongly placed, it should be part as patch 0005, but it has not too valuable benefit, so I removed it
 

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

The target session variable is pushed there to be used for creating VariableDestReceiver, that is necessary for workable LET command when EXPLAIN is used with ANALYZE clause.

I reduced the changes now, but there should be still because the target session variable should be pushed to ExplainOnePlan, but PlannedStmt has not any access to the Query structure where the resultVariable is stored. But I need to inject only ExplainOnePlan - no others. This is the same reason why ExplainOnePlan has an "into" argument. In other places I can use the resultVariable from the "query" argument.

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

The flag is_valid is set by sinval message processing or by DROP VARIABLE command.

All invalid variables should be removed by remove_invalid_session_variables function, but this function ignores variables dropped in the current transaction (and this routine is called only once per transaction - it can be expensive, because it iterates over all variables currently used in session). The purpose of remove_invalid_session_variables inside get_session_variable is cleaning memory for dropped variables when the previous transaction is aborted.

But there is a possibility to revert DROP VARIABLE by using savepoint inside one transaction. And in this case we can have invalid variable (after DROP VARIABLE), that is not removed by remove_invalid_session_variables, but can be valid (and it is validated after is_session_variable_valid).

This is reggress test scenario

BEGIN;
  CREATE TEMP VARIABLE var1 AS int ON COMMIT DROP;
  LET var1 = 100;
  SAVEPOINT s1;
  DROP VARIABLE var1;
  ROLLBACK TO s1;
  SELECT var1;
 var1.
------
  100
(1 row)

COMMIT;

I did new comment there, and modified little bit the logic

attention: the logic is different before and after patch 0004 where memory cleaning is implemented



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

This comment is related to usage of svar->typbyval and svar->typbylen for datumCopy. When we accept invalidation message
for some variable and then svar->is_valid is false, then we should not use these values, and we should reread it from catalog
(be executing setup_session_variable). It is done on auxiliary svar, because there is a possible risk of failure of datumCopy, and the
contract is unchanged passed svar, when any error happens.

I changed the comment.


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

I expected only palloc failure.
 

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?

Currently, there are only two places where there can be some failure - one is related to set and datumCopy, a second to evaluation of default expressions. 

Any other possible failures like domain's exception or not null exception has not any impact on stored value.

regards

Pavel

 
Attachment

pgsql-hackers by date:

Previous
From: Daniel Gustafsson
Date:
Subject: Re: Fix error handling in be_tls_open_server()
Next
From: Pavel Stehule
Date:
Subject: Re: Schema variables - new implementation for Postgres 15