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 added separately by pg_background). I suppose, this is done to prevent API users from accessing internals of BackgroundSession structure. 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 useful and 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
