Re: Schema variables - new implementation for Postgres 15

> On Sat, Nov 18, 2023 at 06:28:53PM +0100, Pavel Stehule wrote:
> so 18. 11. 2023 v 15:54 odesílatel Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
> napsal:
> > As a side note, I'm intended to go one more time through the first few
> > patches introducing the basic functionality, and then mark it as ready
> > in CF. I can't break the patch in testing since quite long time, and for
> > most parts the changes make sense to me.
>
> I marked pg_session_variables function as PARALLEL RESTRICTED, and did
> rebase

So, after one week of uninterrupted evening reviews I've made it through
the first four patches :)

It's a decent job -- more than once, looking at the code, I thought I
could construct a case when it's going to blow up, but everything was
working just fine. Yet, I think the patch still has to be reshaped a bit
before moving forward. I've got a couple proposals of different nature:
high level changes (you probably won't like some of them, but I'm sure
they're going to be useful), technical code-level improvements/comments,
and few language changes. With those changes in mind I would be
satisfied with the patch, and hopefully they would also make it easier
for a potential committer to pick it up.

# High level proposals

* I would suggest reducing the scope of the patch as much as possible,
and not just by trimming on the edges, but rather following Phileas
Fogg's example with the steamboat Henrietta -- get rid of all
non-essential parts. This will make this rather large patch more
approachable for others.

For that one can concentrate only on the first two patches plus the
fourth one (memory cleanup after dropping variables), leaving DISCARD,
ON TRANSACTION END, DEFAULT, IMMUTABLE for the follow-up in the
future.

Another thing in this context would be to evaluate plpgsql support for
this feature. You know the use case better than me, how important it
is? Is it an intrinsic part of the feature, or session variables could
be still valuable enough even without plpgsql? From what I see
postponing plgpsql will make everything about ~800 lines lighter (most
likely more), and also allow to ignore couple of concerns about the
implementation (about this later).

* The new GUC session_variables_ambiguity_warning is definitely going to
cause many objections, it's another knob to manage very subtle
behaviour detail very few people will ever notice. I see the point
behind warning about ambiguity, so probably it makes sense to bite the
bullet and decide one way or another. The proposal is to warn always
in potentially ambiguous situations, and if concerns are high about
logging too much, maybe do the warning on lower logging levels.

# Code-level observations

* It feels a bit awkward to have varid assignment logic in a separate
function, what about adding an argument with varid to
CreateVariableDestReceiver? SetVariableDestReceiverVarid still could
be used for CreateDestReceiver.

/*
* Initially create a DestReceiver object.
*/
DestReceiver *
CreateVariableDestReceiver(void)

/*
* Set parameters for a VariableDestReceiver.
* Should be called right after creating the DestReceiver.
*/
void
SetVariableDestReceiverVarid(DestReceiver *self, Oid varid)

* It's worth it to add a commentary here explaining why it's fine to use
InvalidOid here:

if (pstmt->commandType != CMD_UTILITY)
- ExplainOnePlan(pstmt, into, es, query_string, paramLI, queryEnv,
+ ExplainOnePlan(pstmt, into, InvalidOid, es, query_string, paramLI, queryEnv,
&planduration, (es->buffers ? &bufusage : NULL));

My understanding is that since LetStmt is CMD_UTILITY, this branch
will never be visited for a session variable.

* IIUC this one is introduced to exclude session variables from the normal
path with EXPR_KIND_UPDATE_TARGET:

+ EXPR_KIND_ASSIGN_VARIABLE, /* PL/pgSQL assignment target - disallow
+ * session variables */

But the name doesn't sound right, maybe longer
EXPR_KIND_UPDATE_TARGET_NO_VARS is better?

* I'm curious about this one, which exactly part does this change cover?

@@ -4888,21 +4914,43 @@ substitute_actual_parameters_mutator(Node *node,
- if (param->paramkind != PARAM_EXTERN)
+ if (param->paramkind != PARAM_EXTERN &&
+ param->paramkind != PARAM_VARIABLE)
elog(ERROR, "unexpected paramkind: %d", (int) param->paramkind);

I've commented it out, but no tests were affected.

* Does it mean there could be theoretically two LET statements at the
same time with different command type, one CMD_UTILITY, one
CMD_SELECT? Can it cause any issues?

+ /*
+ * Inside PL/pgSQL we don't want to execute LET statement as utility
+ * command, because it disallow to execute expression as simple
+ * expression. So for PL/pgSQL we have extra path, and we return SELECT.
+ * Then it can be executed by exec_eval_expr. Result is dirrectly assigned
+ * to target session variable inside PL/pgSQL LET statement handler. This
+ * is extra code, extra path, but possibility to get faster execution is
+ * too attractive.
+ */
+ if (stmt->plpgsql_mode)
+ return query;
+

* This probably requires more explanation, is warning the only reason
for this change?

+ *
+ * The session variables should not be used as target of PL/pgSQL assign
+ * statement. So we should to use special parser expr kind, that disallow
+ * usage of session variables. This block unwanted (in this context)
+ * possible warning so target PL/pgSQL's variable shadows some session
+ * variable.
*/
target = transformExpr(pstate, (Node *) cref,
- EXPR_KIND_UPDATE_TARGET);
+ EXPR_KIND_ASSIGN_VARIABLE);

* It would be great to have more commentaries here:

typedef struct
{
DestReceiver pub;
Oid varid;
Oid typid;
int32 typmod;
int typlen;
int slot_offset;
int rows;
} SVariableState;

For example, why does it make sense to have a field rows, where we
interested to only know the fact that there is exactly one column?

* Why there is SetSessionVariableWithSecurityCheck, but no
GetSessionVariableWithSecurityCheck? Instead, object_aclcheck is done
in standard_ExecutorStart, which looks a bit out of place.

* pg_session_variables -- you mention it exists only for testing. What
about moving it out into a separate patch for the sake of slimming
down? It looks like it's used only in tests for "memory cleanup"
patch, maybe they could be restructured to not require this function.

* Probably it's time to drop unnecessary historical notes, like this:

* Note: originally we enhanced a list xact_recheck_varids here. Unfortunately
* it was not safe and a little bit too complex, because the sinval callback
* function can be called when we iterate over xact_recheck_varids list.
* Another issue was the possibility of being out of memory when we enhanced
* the list. So now we just switch flag in related entry sessionvars hash table.
* We need to iterate over hash table on every sinval message, so extra two
* iteration over this hash table is not significant overhead (and we skip
* entries that don't require recheck). Now we do not have any memory allocation
* in the sinval handler (This note can be removed before commit).

* The second patch "Storage for session variables and SQL interface",
mentions DISCARD command:

/*
* There is no guarantee of sessionvars being initialized, even when
* receiving an invalidation callback, as DISCARD [ ALL | VARIABLES ]
* destroys the hash table entirely.
*/

This command is implemented in another patch later one, so this
comment probably belong there.

* This comment mentions a "direct access, without buffering":

/*
* Direct access to session variable (without buffering). Because
* returned value can be used (without an assignement) after the
* referenced session variables is updated, we have to use an copy
* of stored value every time.
*/
*op->resvalue = GetSessionVariableWithTypeCheck(op->d.vparam.varid,
op->resnull,
op->d.vparam.vartype);

But GetSessionVariableWithTypeCheck goes through get_session_variable
and searches in the hash table. What "buffering" means in this
context?

* GetSessionVariableWithTypeCheck(Oid varid, bool *isNull, Oid expected_typid)

Should the "WithTypeCheck" part be an argument of the
GetSessionVariable? To reduce the code duplication a bit.

* Just out of curiosity, why TopTransactionContext?

/*
* Store domain_check extra in TopTransactionContext. When we are in
* other transaction, the domain_check_extra cache is not valid
* anymore.
*/
if (svar->domain_check_extra_lxid != MyProc->lxid)
svar->domain_check_extra = NULL;

domain_check(svar->value, svar->isnull,
svar->typid, &svar->domain_check_extra,
TopTransactionContext);

* In SVariableData it would be great to have more comments around
freeval, domain_check_extra, domain_check_extra_lxid.

* Nitpicking, but the term "shadowing" for ambiguity between a session
variable and a table column might be confusing, one can imagine there
is a connection between those two objects and one actively follows
("shadows") the other one.

* The second patch "Storage for session variables and SQL interface"
mentions in the documentation default and temporary variables:

<para>
The value of a session variable is local to the current session. Retrieving
a variable's value returns either a <literal>NULL</literal> or a default
value, unless its value has been set to something else in the current
session using the <command>LET</command> command. The content of a variable
is not transactional. This is the same as regular variables in PL languages.
The session variables can be persistent or can be temporary. In both cases,
the content of session variables is temporary and not shared (like an
content of temporary tables).
</para>

They're implemented in the following patches, so it belongs there.

* Nitpicking, maybe merge those two conditions together for readability?

if (!needs_validation)
return;

/*
* Reset, this flag here, before we start the validation. It can be set to
* on by incomming sinval message.
*/
needs_validation = false;

if (!sessionvars)
return;

* This one is not very clear, what is the difference between "somewhere
inside a transaction" and "at the end of a transaction"?

/*
* This routine can be called somewhere inside transaction or at an transaction
* end. When atEOX argument is false, then we are inside transaction, and we
* don't want to throw entries related to session variables dropped in current
* transaction.
*/

# Language topic

Since this patch introduces a large body of documentation and
commentaries, I think it would benefit from a native speaker review.
I've stumbled upon few examples (attached with proposed wording, without
a diff extension to not confuse the CF bot), but otherwise if anyone
follows this thread, texts review is appreciated.