Horton

Re: Re: proposal: schema variables

Pavel Stehule <pavel.stehule@gmail.com>

View thread
From: Pavel Stehule <pavel.stehule@gmail.com>
To: jian he <jian.universality@gmail.com>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>, Laurenz Albe <laurenz.albe@cybertec.at>, Erik Rijkers <er@xs4all.nl>, Michael Paquier <michael@paquier.xyz>, Amit Kapila <amit.kapila16@gmail.com>, DUVAL REMI <REMI.DUVAL@cheops.fr>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-01-06T12:21:09Z
Lists: pgsql-hackers, pgsql-performance

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Move WAL sequence code into its own file

  2. Add ExplainState argument to pg_plan_query() and planner().

  3. Don't include access/htup_details.h in executor/tuptable.h

  4. Refactor to avoid code duplication in transformPLAssignStmt.

  5. Avoid including commands/dbcommands.h in so many places

  6. Restrict psql meta-commands in plain-text dumps.

  7. Split func.sgml into more manageable pieces

  8. Fix squashing algorithm for query texts

  9. EXPLAIN: Always use two fractional digits for row counts.

  10. Preliminary refactoring of plpgsql expression construction.

  11. plpgsql: pure parser and reentrant scanner

  12. Add some sanity checks in executor for query ID reporting

  13. Fix misleading error message context

  14. Add macros for looping through a List without a ListCell.

Attachments

Hi

po 6. 1. 2025 v 8:59 odesílatel jian he <jian.universality@gmail.com>
napsal:

> comment out the changes in
> src/backend/utils/cache/plancache.c
>
>     // /* process session variables */
>     // if (OidIsValid(parsetree->resultVariable))
>     // {
>     //     if (acquire)
>     //         LockDatabaseObject(VariableRelationId,
> parsetree->resultVariable,
>     //                            0, AccessShareLock);
>     //     else
>     //         UnlockDatabaseObject(VariableRelationId,
> parsetree->resultVariable,
>     //                              0, AccessShareLock);
>     // }
>
>     // else if (IsA(node, Param))
>     // {
>     //     Param       *p = (Param *) node;
>     //     if (p->paramkind == PARAM_VARIABLE)
>     //     {
>     //         if (acquire)
>     //             LockDatabaseObject(VariableRelationId, p->paramvarid,
>     //                                0, AccessShareLock);
>     //         else
>     //             UnlockDatabaseObject(VariableRelationId, p->paramvarid,
>     //                                  0, AccessShareLock);
>     //     }
>     // }
> the regress tests are still successful, that means these code changes
> don't have related tests.
>

the locking is tested by isolation test in patch05


>
>
> SetSessionVariable(Oid varid, Datum value, bool isNull)
> {
>     create_sessionvars_hashtables();
>     svar = (SVariable) hash_search(sessionvars, &varid, HASH_ENTER,
> &found);
>     if (!found)
>         setup_session_variable(svar, varid);
>     /* if this fails, it won't change the stored value */
>     set_session_variable(svar, value, isNull);
> }
> after set_session_variable,
> we want to make sure that svar->is_valid is true,
> svar->value = value and  svar->isnull= isNull.
> Based on this, I've simplified the function set_session_variable,
> refer v1-0001-minor-refactoring-set_session_variable.no-cfbot
>

Unfortunately, I don't think it is possible. See comments

/*
 * Assign a new value to the session variable.  It is copied to
 * SVariableMemoryContext if necessary.
 *
 * **** If any error happens, the existing value won't be modified. ****
 */

So the variable cannot be released before memory for new content is
allocated

The current code is a little bit more complex, but significantly reduces
the risk so the variable will hold unexpected value.

The current code ensures stability of LET command - when it is successful,
then value is changed, or when it is not successful, then variable is not
changed.

Proposed code breaks it. There is risk of palloc exception, and that means,
after unsuccessful LET the variable can be initialized. So there are three
ending states, not just two.

pfree theoretically can raise an exception, but in this case it should be
code our design error (using wrong allocator)


>
> we use PlannerGlobal
> {
>    Oid            basenodeSessionVarid;
>    Bitmapset *checkSelectPermVarids;
> }
> to solve the self-assigned corner case SELECT privilege.
> (let v1.a =v1.a; in this case, we need have SELECT priv for v1.a
> but let v1.a = 1, we don't need SELECT priv for v1.a).
>
> i found out these two field value(information) most case is the same
> as PlannerGlobal.sessionVariables;
> I came up with another solution, introduce a bool
> (Query.is_Variable_assigned),
> and get rid of PlannerGlobal.basenodeSessionVarid,
> PlannerGlobal.checkSelectPermVarids.
> not sure it make sense to you, refer
> v1-0002-refactoring-LET-statement-self-assign-privileg.no-cfbot
>

It is true, so bitmapset checkSelectPermVarids contains almost all the same
data like sessionVariables.
But checkSelectPermVarids allows fast checking if a variable is used
already, and sessionVariables list ensures necessary order.

My implementation needs more memory (one bitmapset), but I think it is very
simple and less error prone (doesn't depend on iteration order).

In this case and this moment I prefer my bitmapset based solution. It can
be optimized maybe later - I thought about a dedicated item in
sessionVariables for the basenode parameter.
This should be a clear winner for value passed types, but for varlena types
(for long varlena types), it can force passing content of some session
variable twice.

Regards

Pavel