2017-03-09 7:48 GMT+01:00 Victor Wagner <vi...@wagner.pp.ru>: > On Wed, 8 Mar 2017 16:49:33 +0100 > Pavel Stehule <pavel.steh...@gmail.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 <vi...@wagner.pp.ru> >
