Re: Schema variables - new implementation for Postgres 15

po 1. 8. 2022 v 6:54 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:

> 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(at)gmail(dot)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.
>

This is the first implementation and my strategy is "to be safe and to be
strict". I did tests I know, so the test of compatibility of composite
types can be more tolerant. But I use this test to test my identity against
oid overflow, and I don't feel comfortable if I write this test too
tolerantly. For implementation of a more precious test I need to save a
signature of attributes. So the test should not be done just on
compatibility of types from TupleDesc, but it should to check attributes
oid's. I had an idea to implement it in the next stage, and for this stage
just to require compatibility of the vector of types.

Can this enhanced check be implemented later or do you think so it should
be implemented now? I'll check how much new code it needs.

>
> 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.
>

My bad - I used gmail as a spellchecker, and it wrote some white spaces
there :-/

should be fixed now