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