Re: Explicit subtransactions for PL/Tcl

2017-03-09 7:48 GMT+01:00 Victor Wagner <vitus(at)wagner(dot)pp(dot)ru>:

> On Wed, 8 Mar 2017 16:49:33 +0100
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
> >
> > I did a review of this patch
> >
> I'm attaching new version of patch with the issues pointed by you fixed.
>
> >
> > 4. A documentation is good - although I am not sure if it is well
> > structured - is <sect2> level necessary? Probably there will not be
> > any other similar command.
>
> Removed <sect2>. Although it is questionable - now there is no
> subsection with name of the command in the title. But surrounding
> section describes only one command,.
>
>
>
> > 5. There are a basic regress tests, and all tests passed, but I miss a
> > path, where subtransaction is commited - now rollback is every time
>
> Added test with successful subtransaction commit. Just to be sure it is
> really commited.
>
>
> > 6. The code has some issues with white chars
>
> Fixed where I've found it. Now, hopefully my code uses same identation
> rules as other pltcl.c
>
>
> > 7. I don't understand why TopMemoryContext is used there? Maybe some
> > already used context should be there.
> >
> > +<->BeginInternalSubTransaction(NULL);
> > +<->MemoryContextSwitchTo(TopTransactionContext);
> > +<->
>
> It turns out that was really an error in the first version of patch.
> This context manipulation was copied from PL/Python context manager by
> mistake.
>
> PL/Python changes to TopTransactionContext in order to manage
> stack of subtransaction data (which we don't need in Tcl, because we
> can use C language stack and store neccessary data in function
> automatic variables. Really python doesn't need this stack to, because
> Python context manager is a python language object and can keep state
> inside it).
>
> Although we still need to keep MemoryContext and ResourceOwner and
> restore them upon transaction finish.
>
>
is this patch complete? I don't see new regress tests

Regards

Pavel

> With best regards, Victor
>
> --
> Victor Wagner <vitus(at)wagner(dot)pp(dot)ru>
>