public inbox for [email protected]
help / color / mirror / Atom feedFrom: Julien Rouhaud <[email protected]>
To: Pavel Stehule <[email protected]>
Cc: Erik Rijkers <[email protected]>
Cc: Dean Rasheed <[email protected]>
Cc: Joel Jacobson <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Schema variables - new implementation for Postgres 15
Date: Mon, 1 Aug 2022 12:53:58 +0800
Message-ID: <20220801045358.w24ac224slef637d@jrouhaud> (raw)
In-Reply-To: <CAFj8pRDbzVmBbNV5CuXnFGgp8i6S7QU9xTsb92T4NvPgPsTx4A@mail.gmail.com>
References: <CAFj8pRB_RCsTpfgmQkLi+ZFpE+Nsk4Um6Dt=G=JWKWxN4HCuQQ@mail.gmail.com>
<CAFj8pRDfJKOxodNjvru+z==mUJfJvPdNY=vE5ACsztdSvAzS-g@mail.gmail.com>
<CAFj8pRCp-JJe=SafJ67ZPuxC-SCk+8aCQ61A0JHNZ3TNRMin=g@mail.gmail.com>
<CAFj8pRA4jyBaTVrc-UF8DuR9C9OxFwz7tytCbtM0M6zuhyzrcQ@mail.gmail.com>
<CAFj8pRAJrntBk4GeSsOY5MzkEN8TvA1aT-9DLyN-cnT7xXG+xw@mail.gmail.com>
<[email protected]>
<20220721073405.qupgkjfufxbrsiv4@jrouhaud>
<CAFj8pRCYLr5qQh3NyTERH+R--Gfo2tTkSWra-2h7sJ4exVbyPA@mail.gmail.com>
<20220724111252.2hebubxtgahmxm5o@jrouhaud>
<CAFj8pRDbzVmBbNV5CuXnFGgp8i6S7QU9xTsb92T4NvPgPsTx4A@mail.gmail.com>
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 <[email protected]>
> 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.
view thread (188+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Schema variables - new implementation for Postgres 15
In-Reply-To: <20220801045358.w24ac224slef637d@jrouhaud>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox