Re: Explicit subtransactions for PL/Tcl

From: Victor Wagner <vitus(at)wagner(dot)pp(dot)ru>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Explicit subtransactions for PL/Tcl
Date: 2017-03-09 06:48:52
Message-ID: 20170309094852.02786482@fafnir.local.vm
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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.

With best regards, Victor

--
Victor Wagner <vitus(at)wagner(dot)pp(dot)ru>

Attachment Content-Type Size
tcl_subtransaction_02.patch text/x-patch 6.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Stehule 2017-03-09 08:25:14 Re: Explicit subtransactions for PL/Tcl
Previous Message Ashutosh Bapat 2017-03-09 06:23:17 Re: foreign partition DDL regression tests