On Wed, Sep 23, 2015 at 6:42 AM, Kouhei Kaigai <kai...@ak.jp.nec.com> wrote:
> > > On Thu, Sep 17, 2015 at 4:44 PM, Robert Haas <robertmh...@gmail.com>
> > To verify the patch, I have done 2 things, first I have added elog to
> > the newly supported read funcs and then in planner, I have used
> > nodeToString and stringToNode on planned_stmt and then used the
> > newly generated planned_stmt for further execution.  After making these
> > changes, I have ran make check-world and ensures that it covers all the
> > newly added nodes.
> >
> > Note, that as we don't populate funcid's in expressions during read, the
> > same has to be updated by traversing the tree and updating in different
> > expressions based on node type.  Attached patch (read_funcs_test_v1)
> > contains the changes required for testing the patch.  I am not very sure
> > about what do about some of the ForeignScan fields (fdw_private) in
> > to update the funcid as the data in those expressions could be FDW
> > This is anyway for test, so doesn't matter much, but the same will be
> > required to support read of ForeignScan node by worker.
> >
> Because of interface contract, it is role of FDW driver to put nodes which
> are safe to copyObject on fdw_exprs and fdw_private field. Unless FDW
> does not violate, fdw_exprs and fdw_private shall be reproduced on the
> side. (Of course, we cannot guarantee nobody has local pointer on private
> field...)
> Sorry, I cannot understand the sentence of funcid population. It seems to
> funcid is displayed as-is, and _readFuncExpr() does nothing special...?

Here I am referring to operator's funcid (refer function _readOpExpr()).
It is
hard-coded to InvalidOid during read.  Currently fix_opfuncids() is used to
fill the funcid for expressions, now I am not sure if it can be used for
fdw_private data (I have tried it, but it was failing, may be it is not at
required to fix any funcid for it) or other fields in Foreign Scan.

I have observed that in out functions, we output fdw_private field which
indicates that ideally we should be able to read it.

> Robert Haas said:
> > 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);
> >
> >
> Like this?
> I think outfuncs.c also have similar macro to centralize the format of
> Actually, most of boolean array are displayed using booltostr(), however,
> _outMergeJoin() uses "%d" format to display boolean as integer.
> It is a bit inconsistent manner.

Yes, I have also noticed the same and thought of sending a patch which I
have done just before sending this mail.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Reply via email to