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

Hi,

On Fri, Jul 22, 2022 at 10:58:25AM +0200, Pavel Stehule wrote:
> > Apparently most of the changes in catalogs.sgml didn't survive the last
> > rebase.
> > I do see the needed section in v20220709-0012-documentation.patch:
> >
> > > + <sect1 id="catalog-pg-variable">
> > > +  <title><structname>pg_variable</structname></title>
> > > [...]
> >
>
> should be fixed now

Thanks!  I confirm that the documentation compiles now.

As mentioned off-list, I still think that the main comment in sessionvariable.c
needs to be adapted to the new approach.  At the very least it still refers to
the previous 2 lists, but as far as I can see there are now 4 lists:

+ /* Both lists hold fields of SVariableXActActionItem type */
+ static List *xact_on_commit_drop_actions = NIL;
+ static List *xact_on_commit_reset_actions = NIL;
+
+ /*
+  * the ON COMMIT DROP and ON TRANSACTION END RESET variables
+  * are purged from memory every time.
+  */
+ static List *xact_reset_varids = NIL;
+
+ /*
+  * Holds list variable's id that that should be
+  * checked against system catalog if still live.
+  */
+ static List *xact_recheck_varids = NIL;

Apart from that, I'm not sure how much of the previous behavior changed.

It would be easier to review the new patchset having some up to date general
description of the approach.  If that's overall the same, just implemented
slightly differently I will just go ahead and dig into the patchset (although
the comments will still have to be changed eventually).

Also, one of the things that changes since the last version is:

@@ -1980,15 +1975,13 @@ AtEOSubXact_SessionVariable_on_xact_actions(bool isCommit, SubTransactionId mySu
     */
    foreach(cur_item, xact_on_commit_reset_actions)
    {
        SVariableXActActionItem *xact_ai =
                                  (SVariableXActActionItem *) lfirst(cur_item);

-       if (!isCommit &&
-           xact_ai->creating_subid == mySubid &&
-           xact_ai->action == SVAR_ON_COMMIT_DROP)
+       if (!isCommit && xact_ai->creating_subid == mySubid)

We previously discussed this off-line, but for some quick context the test was
buggy as it wasn't possible to have an SVAR_ON_COMMIT_DROP action in the
xact_on_commit_reset_actions list.  However I don't see any change in the
regression tests since the last version and the tests are all green in both
versions.

It means that was fixed but there's no test covering it.  The local memory
management is probably the hardest part of this patchset, so I'm a bit worried
if there's nothing that can catch a bug leading to leaked values or entries in
some processing list.  Do you think it's possible to add some test that would
have caught the previous bug?