Re: [HACKERS] autonomous transactions

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

Here’s review of Background Sessions v2 patch.
===Purpose===
Patch provides API for controlling other backend. Also, patch contains Python API support for calling C API.

===Overall status===
Patch applies to current HEAD clearly.
Contains new test and passes existing tests.
Contains sufficient documentation.
Contains 2 TODO items. Not sure it's OK to it leave so.
Another patch from this commitfest (pg_background) is based on this patch.

===Suggestions===
I haven’t found a way to safely acquire status of session (without possibility of ereport(ERROR)).
I do not see how to pass massive data, except by one long char* SQL. All the values have to be formatted as text.
BackgroundSessionStart() result do not contain PID. This functionality is expected by pg_background (though, can be
addedseparately by pg_background). I suppose, this is done to prevent API users from accessing internals of
BackgroundSessionstructure. But some information have to be public, anyway.
 
bgsession.c code contains very little comments.
I do not think that switch inside a switch (see bgsession_worker_main()) is easy to follow.

===Conclusion===
There’s always something to improve, but I think that this patch is ready for committer.

PS. I’ve read the db_link patches, but this review does not apply to them. I suppose db_link refactoring would be
usefuland functionality is added, so I think these patches deserve separate commitfest entry.
 

Best regards, Andrey Borodin.

The new status of this patch is: Ready for Committer