On Wed, Sep 23, 2015 at 5:42 AM, Robert Haas <robertmh...@gmail.com> wrote: > > On Tue, Sep 22, 2015 at 3:21 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > > Attached patch (read_funcs_v1.patch) contains support for all the plan > > and other nodes (like SubPlan which could be required for worker) except > > CustomScan node. > > It looks like you need to update the top-of-file comment for outfuncs.c. >
Updated. > Doesn't _readCommonPlan() leak? I have tried that way to keep the code simple with a view that this will not be read in long-lived memory context, however your suggestion below makes sense and can avoid it. > I think we should avoid that. > _readCommonScan() and _readJoin() are worse: they leak multiple > objects. It should be simple enough to avoid this: just have your > helper function take a Plan * as argument and then use > READ_TEMP_LOCALS() rather than READ_LOCALS(). Then the caller can use > READ_LOCALS, call the helper to fill in all the Plan fields, and then > read the other stuff itself. > Changed as per suggestion. > Instead of passing the Plan down by casting, how about passing > &local_node->plan? And similarly for scans and joins. > Changed as per suggestion. > readAttrNumberCols uses sizeof(Oid) instead of sizeof(AttrNumber). > Fixed. > I still don't understand why we need to handle PlanInvalItem. > Actually, come to think of it, I'm not sure we need PlannedStmt > either. Let's leave those out; they seem like trouble. > As discussed below this is required and I haven't changed it. > I think it would be worth doing something like this: > > #define READ_ATTRNUMBER_ARRAY(fldname, len) \ > token = pg_strtok(&length); \ > local_node->fldname = readAttrNumberCols(len); > > And similarly for READ_OID_ARRAY, READ_BOOL_ARRAY, READ_INT_ARRAY. > Changed as per suggestion. > In general these routines are in the same order as plannodes.h, which > is good. But _readNestLoopParam is out of place. Can we move it just > after _readNestLoop? > I have kept them in order they appear in nodes.h (that way it seems easier to keep track if anything is missed), however if you want I can reorder them as per your suggestion. Note - Test is changed to verify just the output of readfuncs with changes in planner. I have removed elog related changes in readfuncs, as during last test, I have verified that make check-world covers all types of nodes that are added by patch. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
read_funcs_v2.patch
Description: Binary data
read_funcs_test_v2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
