Re: Schema variables - new implementation for Postgres 15

Hi

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.

Erik's patches are merged. Thank you for them.

Regards

Pavel