On Wed, 8 Mar 2017 16:49:33 +0100 Pavel Stehule <pavel.steh...@gmail.com> wrote:
> > I did a review of this patch First, thank you for you effort. I already begin to think that nobody is really interesting in PL/Tcl stuff. > 1. This functionality has sense and nobody was against this feature. > > 2. This patch does what is proposed - it introduce new TCL function > that wraps PostgreSQL subtransaction > > 3. This patch is really simple due massive using subtransactions > already - every SPI called from TCL is wrapped to subtransaction. > > 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. You are right. At least sect2 can be added later whenever this second command will appear. > 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 Really, I haven't found such tests in PL/Python suite, so I haven't translated it to Tcl. It might be good idea to add such test. > 6. The code has some issues with white chars > > 7. I don't understand why TopMemoryContext is used there? Maybe some > already used context should be there. > > +<->BeginInternalSubTransaction(NULL); > +<->MemoryContextSwitchTo(TopTransactionContext); > +<-> I've copied this code from PL/Python subtransaction object and it has following comment: /* Be sure that cells of explicit_subtransactions list are long-lived */ But in Tcl case wi don't have to maintain our own stack in the form of list. We use C-language stack and keep our data in the local variables. So, probably changing of memory contexts is not necessary at all. But it require a bit more investigation and testing. With best regards, Victor -- Victor Wagner <vi...@wagner.pp.ru> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers