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
