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>
> > 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.


> 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).


> 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);

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
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

Attachment: read_funcs_v2.patch
Description: Binary data

Attachment: read_funcs_test_v2.patch
Description: Binary data

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to