Обсуждение: Re: CustomScan in a larger structure (RE: CustomScan support on readfuncs.c)
> -----Original Message----- > From: Robert Haas [mailto:robertmhaas@gmail.com] > Sent: Monday, February 08, 2016 11:59 PM > To: Kaigai Kouhei(海外 浩平) > Cc: Andres Freund; Amit Kapila; pgsql-hackers > Subject: ##freemail## Re: CustomScan in a larger structure (RE: [HACKERS] > CustomScan support on readfuncs.c) > > On Sun, Feb 7, 2016 at 7:28 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote: > > The new callbacks of T_ExtensibleNode will replace the necessity to > > form and deform process of private values, like as: > > https://github.com/pg-strom/devel/blob/master/src/gpujoin.c#L114 > > Yeah. > > > It transforms a bunch of internal data of CustomScan (similar to the > > extended fields in T_ExtensibleNode) to/from the node functions > > understandable forms for copy, input and output support. > > I think it implies you proposition is workable. > > > > I'd like to follow this proposition basically. > > On the other hands, I have two points I want to pay attention. > > > > 1. At this moment, it is allowed to define a larger structure that > > embeds CustomPath and CustomScanState by extension. How do we treat > > this coding manner in this case? Especially, CustomScanState has no > > private pointer dereference because it assumes an extension define > > a larger structure. Of course, we don't need to care about node > > operations on Path and PlanState nodes, but right now. > > I see no real advantage in letting a CustomPath be larger. If > custom_private can include extension-defined node types, that seems > good enough. On the other hand, if CustomScanState can be larger, > that seems fine. We don't really need any special support for that, > do we? > Yes. Right now, we have no code path that handles PlanState or its inheritance using node operations. So, it is not a real problem. > > 2. I intended to replace LibraryName and SymbolName fields from the > > CustomScanMethods structure by integration of extensible node type. > > We had to give a pair of these identifiers because custom scan provider > > has no registration points at this moment. A little concern is extension > > has to assume a particular filename of itself. > > But, probably, it shall be a separated discussion. My preference is > > preliminary registration of custom scan provider by its name, as well > > as extensible node. > > Seems like we could just leave the CustomScan stuff alone and worry > about this as a separate facility. > OK > > Towards the last question; whether *_private shall be void * or List *, > > I want to keep fdw_private and custom_private as List * pointer, because > > a new node type definition is a bit overdone job if this FDW or CSP will > > take only a few private fields with primitive data types. > > It is a preferable features when extension defines ten or more private > > fields. > > Well, I suggested Node *, not void *. A Node can be a List, but not > every Node is a List. > It is pretty good! The attached patch (primary one) implements the above idea. Now ExtensibleNode works as a basis structure of data container, regardless of CustomScan and ForeignScan. Also, fdw_private and custom_private are de-defined to Node * type from List * type. It affected to a few FDW APIs. The secondary patch is a demonstration of new ExtensibleNode using postgres_fdw extension. Its private data are expected to be packed in a list with a particular order. Self defined structure allows to keep these variables without ugly pack/unpacking. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei <kaigai@ak.jp.nec.com>
Вложения
On Wed, Feb 10, 2016 at 1:25 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote: > It is pretty good! > > The attached patch (primary one) implements the above idea. > > Now ExtensibleNode works as a basis structure of data container, > regardless of CustomScan and ForeignScan. > Also, fdw_private and custom_private are de-defined to Node * type > from List * type. It affected to a few FDW APIs. I extracted the subset of this that just creates the extensible node framework and did some cleanup - the result is attached. I will commit this if nobody objects. I think the part about whacking around the FDW API is a little more potentially objectionable to others, so I want to hold off doing that unless a few more people chime in with +1. Perhaps we could start a new thread to talk about that specific idea. This is useful even without that, though. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
> -----Original Message----- > From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas > Sent: Thursday, February 11, 2016 1:26 PM > To: Kaigai Kouhei(海外 浩平) > Cc: Andres Freund; Amit Kapila; pgsql-hackers > Subject: Re: CustomScan in a larger structure (RE: [HACKERS] CustomScan support > on readfuncs.c) > > On Wed, Feb 10, 2016 at 1:25 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote: > > It is pretty good! > > > > The attached patch (primary one) implements the above idea. > > > > Now ExtensibleNode works as a basis structure of data container, > > regardless of CustomScan and ForeignScan. > > Also, fdw_private and custom_private are de-defined to Node * type > > from List * type. It affected to a few FDW APIs. > > I extracted the subset of this that just creates the extensible node > framework and did some cleanup - the result is attached. I will > commit this if nobody objects. > I have no objection of course. > I think the part about whacking around the FDW API is a little more > potentially objectionable to others, so I want to hold off doing that > unless a few more people chime in with +1. Perhaps we could start a > new thread to talk about that specific idea. This is useful even > without that, though. > Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei <kaigai@ak.jp.nec.com>
On Thu, Feb 11, 2016 at 7:06 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote: >> -----Original Message----- >> From: pgsql-hackers-owner@postgresql.org >> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas >> Sent: Thursday, February 11, 2016 1:26 PM >> To: Kaigai Kouhei(海外 浩平) >> Cc: Andres Freund; Amit Kapila; pgsql-hackers >> Subject: Re: CustomScan in a larger structure (RE: [HACKERS] CustomScan support >> on readfuncs.c) >> >> On Wed, Feb 10, 2016 at 1:25 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote: >> > It is pretty good! >> > >> > The attached patch (primary one) implements the above idea. >> > >> > Now ExtensibleNode works as a basis structure of data container, >> > regardless of CustomScan and ForeignScan. >> > Also, fdw_private and custom_private are de-defined to Node * type >> > from List * type. It affected to a few FDW APIs. >> >> I extracted the subset of this that just creates the extensible node >> framework and did some cleanup - the result is attached. I will >> commit this if nobody objects. >> > I have no objection of course. Done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi,
On 2016-02-10 23:26:20 -0500, Robert Haas wrote:
> I think the part about whacking around the FDW API is a little more
> potentially objectionable to others, so I want to hold off doing that
> unless a few more people chime in with +1. Perhaps we could start a
> new thread to talk about that specific idea. This is useful even
> without that, though.
FWIW, I can delete a couple hundred lines of code from citusdb thanks to
this...
A quick questions about what you committed:
> @@ -527,10 +532,17 @@ extern PGDLLIMPORT Node *newNodeMacroHolder;
> */
> extern char *nodeToString(const void *obj);
>
> +struct Bitmapset; /* not to include bitmapset.h here */
> +struct StringInfoData; /* not to include stringinfo.h here */
> +extern void outToken(struct StringInfoData *str, const char *s);
> +extern void outBitmapset(struct StringInfoData *str,
> + const struct Bitmapset *bms);
> +
> /*
> * nodes/{readfuncs.c,read.c}
> */
> extern void *stringToNode(char *str);
> +extern struct Bitmapset *readBitmapset(void);
why exactly did you expose read/writeBitmapset(), and nothing else?
Afaics a lot of the other read routines are also pretty necessary from
the outside?
Greetings,
Andres Freund
On Fri, Feb 12, 2016 at 9:56 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-02-10 23:26:20 -0500, Robert Haas wrote:
>> I think the part about whacking around the FDW API is a little more
>> potentially objectionable to others, so I want to hold off doing that
>> unless a few more people chime in with +1. Perhaps we could start a
>> new thread to talk about that specific idea. This is useful even
>> without that, though.
>
> FWIW, I can delete a couple hundred lines of code from citusdb thanks to
> this...
>
> A quick questions about what you committed:
>
>> @@ -527,10 +532,17 @@ extern PGDLLIMPORT Node *newNodeMacroHolder;
>> */
>> extern char *nodeToString(const void *obj);
>>
>> +struct Bitmapset; /* not to include bitmapset.h here */
>> +struct StringInfoData; /* not to include stringinfo.h here */
>> +extern void outToken(struct StringInfoData *str, const char *s);
>> +extern void outBitmapset(struct StringInfoData *str,
>> + const struct Bitmapset *bms);
>> +
>> /*
>> * nodes/{readfuncs.c,read.c}
>> */
>> extern void *stringToNode(char *str);
>> +extern struct Bitmapset *readBitmapset(void);
>
> why exactly did you expose read/writeBitmapset(), and nothing else?
> Afaics a lot of the other read routines are also pretty necessary from
> the outside?
Because that's what KaiGai submitted and I had no reason to
second-guess it. I'm happy to expose other stuff along similar lines
if somebody writes a patch for it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
2016-02-13 0:11 GMT+09:00 Robert Haas <robertmhaas@gmail.com>:
> On Fri, Feb 12, 2016 at 9:56 AM, Andres Freund <andres@anarazel.de> wrote:
>> On 2016-02-10 23:26:20 -0500, Robert Haas wrote:
>>> I think the part about whacking around the FDW API is a little more
>>> potentially objectionable to others, so I want to hold off doing that
>>> unless a few more people chime in with +1. Perhaps we could start a
>>> new thread to talk about that specific idea. This is useful even
>>> without that, though.
>>
>> FWIW, I can delete a couple hundred lines of code from citusdb thanks to
>> this...
>>
>> A quick questions about what you committed:
>>
>>> @@ -527,10 +532,17 @@ extern PGDLLIMPORT Node *newNodeMacroHolder;
>>> */
>>> extern char *nodeToString(const void *obj);
>>>
>>> +struct Bitmapset; /* not to include bitmapset.h here */
>>> +struct StringInfoData; /* not to include stringinfo.h here */
>>> +extern void outToken(struct StringInfoData *str, const char *s);
>>> +extern void outBitmapset(struct StringInfoData *str,
>>> + const struct Bitmapset *bms);
>>> +
>>> /*
>>> * nodes/{readfuncs.c,read.c}
>>> */
>>> extern void *stringToNode(char *str);
>>> +extern struct Bitmapset *readBitmapset(void);
>>
>> why exactly did you expose read/writeBitmapset(), and nothing else?
>> Afaics a lot of the other read routines are also pretty necessary from
>> the outside?
>
> Because that's what KaiGai submitted and I had no reason to
> second-guess it. I'm happy to expose other stuff along similar lines
> if somebody writes a patch for it.
>
Although we have various service macro in outfuncs.c and readfuncs.c,
it is not a big burden for extension authors to write equivalent code,
except for string and bitmap. On the other hands, string needs _outToken,
bitmap needs _outBitmap. It is not good idea to suggest extensions to
have copy & paste code with no clear reason.
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
On 2016-02-12 15:56:45 +0100, Andres Freund wrote: > Hi, > > > On 2016-02-10 23:26:20 -0500, Robert Haas wrote: > > I think the part about whacking around the FDW API is a little more > > potentially objectionable to others, so I want to hold off doing that > > unless a few more people chime in with +1. Perhaps we could start a > > new thread to talk about that specific idea. This is useful even > > without that, though. > > FWIW, I can delete a couple hundred lines of code from citusdb thanks to > this... And I'm now working on doing that. > why exactly did you expose read/writeBitmapset(), and nothing else? > Afaics a lot of the other read routines are also pretty necessary from > the outside? I'd like to also expose at least outDatum()/readDatum() - they're not entirely trivial, so it'd be sad to copy them. What I'm wondering about right now is how an extensible node should implement the equivalent of #define WRITE_NODE_FIELD(fldname) \(appendStringInfo(str, " :" CppAsString(fldname) " "), \ _outNode(str, node->fldname)) given that _outNode isn't public, that seems to imply having to do something like #define WRITE_NODE_FIELD(fldname) \(appendStringInfo(str, " :" CppAsString(fldname) " "), \ appendStringInfo(str, nodeToString(node->fldname))) i.e. essentially doubling memory overhead. Istm we should make outNode() externally visible? Greetings, Andres Freund
On Fri, Mar 4, 2016 at 2:37 PM, Andres Freund <andres@anarazel.de> wrote: > On 2016-02-12 15:56:45 +0100, Andres Freund wrote: >> Hi, >> >> >> On 2016-02-10 23:26:20 -0500, Robert Haas wrote: >> > I think the part about whacking around the FDW API is a little more >> > potentially objectionable to others, so I want to hold off doing that >> > unless a few more people chime in with +1. Perhaps we could start a >> > new thread to talk about that specific idea. This is useful even >> > without that, though. >> >> FWIW, I can delete a couple hundred lines of code from citusdb thanks to >> this... > > And I'm now working on doing that. > > >> why exactly did you expose read/writeBitmapset(), and nothing else? >> Afaics a lot of the other read routines are also pretty necessary from >> the outside? > > I'd like to also expose at least outDatum()/readDatum() - they're not > entirely trivial, so it'd be sad to copy them. > > > What I'm wondering about right now is how an extensible node should > implement the equivalent of > #define WRITE_NODE_FIELD(fldname) \ > (appendStringInfo(str, " :" CppAsString(fldname) " "), \ > _outNode(str, node->fldname)) > > given that _outNode isn't public, that seems to imply having to do > something like > > #define WRITE_NODE_FIELD(fldname) \ > (appendStringInfo(str, " :" CppAsString(fldname) " "), \ > appendStringInfo(str, nodeToString(node->fldname))) > > i.e. essentially doubling memory overhead. Istm we should make > outNode() externally visible? I suggest that you write a patch to do whatever you think best and commit it, with the understanding that future API stability isn't guaranteed if we decide what you picked is not actually for the best. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company