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

Hi,

On Wed, Jul 27, 2022 at 09:59:18PM +0200, Pavel Stehule wrote:
> 
> ne 24. 7. 2022 v 13:12 odesílatel Julien Rouhaud <rjuju123@gmail.com>
> napsal:
> 
> > 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?
> >
> 
> I am sending an updated patch. I had to modify sinval message handling.
> Previous implementation was not robust and correct (there was some
> possibility, so value stored in session's variable was lost after aborted
> drop variable. There are new regress tests requested by Julien and some
> others describing the mentioned issue. I rewrote the implementation's
> description part in sessionvariable.c.

Thanks a lot, that's very helpful!

I looked at the new description and I'm not sure that I understand the need for
the "format change" code that tries to detect whether the underlying types was
modified.  It seems quite fragile, wouldn't it be better to have the same
behavior as for relation (detect and prevent such changes in the first place),
since both cases share the same requirements about underlying data types?  For
instance, it should be totally acceptable to drop an attribute from a custom
data type if a session variable is using it, same as if a table is using it but
as is it would be rejected for session variables.

While at it, the new comments contain a lot of non breakable spaces rather than
normal spaces.  I also just realized that there's a sessionvariable.c while the
header is named session_variable.h.