Re: Schema variables - new implementation for Postgres 15

Hi

pá 11. 8. 2023 v 17:58 odesílatel Dmitry Dolgov <9erthalion6(at)gmail(dot)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