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> wrote: > > > 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 order > > to update the funcid as the data in those expressions could be FDW specific. > > 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 driver > does not violate, fdw_exprs and fdw_private shall be reproduced on the worker > 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 me > 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 all 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); > > > > And similarly for READ_OID_ARRAY, READ_BOOL_ARRAY, READ_INT_ARRAY. > > > Like this? > https://github.com/kaigai/sepgsql/blob/readfuncs/src/backend/nodes/readfuncs.c#L133 > > I think outfuncs.c also have similar macro to centralize the format of array. > Actually, most of boolean array are displayed using booltostr(), however, only > _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
