CustomScan in a larger structure (RE: CustomScan support on readfuncs.c)

On Mon, Nov 9, 2015 at 10:26 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

It is a relevant topic of readfuncs support for custom-scan.

Unlike CustomPath and CustomScanState, we don't allow custom-scan
provider to define own and larger structure that embeds CustomScan
at head of the structure, to support copyObject().
Thus, custom-scan provider needs to have own logic to pack and
unpack private fields, like:
https://github.com/pg-strom/devel/blob/master/src/gpujoin.c#L112

At present, only create_customscan_plan() and _copyCustomScan()
can produce CustomScan node.
The create_customscan_plan() invokes PlanCustomPath callback,
thus, CSP can return a pointer of CustomScan field within
a larger structure. So, we don't adjust this interface.
On the other hands, _copyCustomScan() allocates a new node using
makeNode(CustomScan), thus, here is no space for the larger
structure if CSP wants to use to keep private values without
packing and unpacking.
Only CSP knows exact size of the larger structure, so all we can
do is ask CSP to allocate a new node and copy its private fields.
This patch newly adds NodeCopyCustomScan callback to support
copyObject.

Also, v9.6 will support nodeToString/stringToNode on plan nodes.
So, we need to re-define the role of TextOutCustomScan callback
that is also defined at v9.5.
If CSP extends the CustomScan to save private values on the extra
area, only CSP knows what values and which data types are stored,
thus, all we can do is ask CSP to serialize and deserialize its
private fields without inconsistency.
Even though TextOutCustomScan callback shall be once ripped out
to support readfuncs.c, a pair of TextOut and TextRead callback
will re-define its responsibility again; when CSP defines
a larger structure that embeds CustomScan, a pair of these
callbacks are used to serialize/deserialize the entire custom
defined objects.

I don't see this as being a particularly good idea. The same issue
exists for FDWs, and we're just living with it in that case. There
was talk previously of improving it, and maybe some day somebody will,
but I can't get excited about it. Writing serialization and
deserialization code is annoying but completely insufficient, in my
mind, to justify the hassle of creating a system for this that will be
used by very, very little code.

If we do want to improve it, I'm not sure this is the way to go,
either. I think there could be other designs where we focus on making
the serialization and deserialization options better, rather than
letting people tack stuff onto the struct.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 2015-11-11 14:59:33 -0500, Robert Haas wrote:

I don't see this as being a particularly good idea. The same issue
exists for FDWs, and we're just living with it in that case.

It's absolutely horrible there. I don't see why that's a justification
for much. To deal with the lack of extensible copy/out/readfuncs I've
just had to copy the entirety of readfuncs.c into an extension. Or you
build replacements for those (as e.g. postgres_fdw essentially has
done).

If we do want to improve it, I'm not sure this is the way to go,
either. I think there could be other designs where we focus on making
the serialization and deserialization options better, rather than
letting people tack stuff onto the struct.

Just better serialization doesn't actually help all that much. Being
able to conveniently access data directly, i.e. as fields in a struct,
makes code rather more readable. And in many cases more
efficient. Having to serialize internal datastructures unconditionally,
just so copyfuncs.c works if actually used, makes for a fair amount of
inefficiency (forced deserialization, even when not copying) and uglier
code.

Andres

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

On Wed, Nov 11, 2015 at 3:29 PM, Andres Freund <andres@anarazel.de> wrote:

On 2015-11-11 14:59:33 -0500, Robert Haas wrote:

I don't see this as being a particularly good idea. The same issue
exists for FDWs, and we're just living with it in that case.

It's absolutely horrible there. I don't see why that's a justification
for much. To deal with the lack of extensible copy/out/readfuncs I've
just had to copy the entirety of readfuncs.c into an extension. Or you
build replacements for those (as e.g. postgres_fdw essentially has
done).

If we do want to improve it, I'm not sure this is the way to go,
either. I think there could be other designs where we focus on making
the serialization and deserialization options better, rather than
letting people tack stuff onto the struct.

Just better serialization doesn't actually help all that much. Being
able to conveniently access data directly, i.e. as fields in a struct,
makes code rather more readable. And in many cases more
efficient. Having to serialize internal datastructures unconditionally,
just so copyfuncs.c works if actually used, makes for a fair amount of
inefficiency (forced deserialization, even when not copying) and uglier
code.

Fair enough, but I'm still not very interested in having something for
CustomScan only.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Nov 11, 2015 at 3:29 PM, Andres Freund <andres@anarazel.de> wrote:

Just better serialization doesn't actually help all that much. Being
able to conveniently access data directly, i.e. as fields in a struct,
makes code rather more readable. And in many cases more
efficient. Having to serialize internal datastructures unconditionally,
just so copyfuncs.c works if actually used, makes for a fair amount of
inefficiency (forced deserialization, even when not copying) and uglier
code.

Fair enough, but I'm still not very interested in having something for
CustomScan only.

I'm with Robert here. The fact is that postgres_fdw and other FDWs are
living just fine with the restrictions we put in to allow copyfuncs.c to
copy ForeignScan, and there's been no sufficient justification offered
as to why CustomScan can't play by those rules.

Yes, it would be nice to be able to use a more-tailored struct definition,
but it's not *necessary*. We should not contort the core system
enormously in order to provide a bit more notational cleanliness to
extensions.

I'd be fine with fixing this if a nice solution were to present itself,
but so far nothing's been offered that wasn't horrid.

BTW, the "inefficiency" argument is junk, unless you provide some hard
evidence of a case where it hurts a lot. If we were forcing people to
use serialized representations at execution time, it would be meaningful,
but we're not; you can convert as needed at executor setup time.

regards, tom lane

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

On Wed, Nov 11, 2015 at 3:29 PM, Andres Freund <andres@anarazel.de> wrote:

On 2015-11-11 14:59:33 -0500, Robert Haas wrote:

I don't see this as being a particularly good idea. The same issue
exists for FDWs, and we're just living with it in that case.

It's absolutely horrible there. I don't see why that's a justification
for much. To deal with the lack of extensible copy/out/readfuncs I've
just had to copy the entirety of readfuncs.c into an extension. Or you
build replacements for those (as e.g. postgres_fdw essentially has
done).

If we do want to improve it, I'm not sure this is the way to go,
either. I think there could be other designs where we focus on making
the serialization and deserialization options better, rather than
letting people tack stuff onto the struct.

Just better serialization doesn't actually help all that much. Being
able to conveniently access data directly, i.e. as fields in a struct,
makes code rather more readable. And in many cases more
efficient. Having to serialize internal datastructures unconditionally,
just so copyfuncs.c works if actually used, makes for a fair amount of
inefficiency (forced deserialization, even when not copying) and uglier
code.

Fair enough, but I'm still not very interested in having something for
CustomScan only.

I agree with we have no reason why only custom-scan is allowed to have
serialize/deserialize capability. I can implement an equivalent stuff
for foreign-scan also, and it is helpful for extension authors, especially,
who tries to implement remote join feature because FDW driver has to
keep larger number of private fields than scan.

Also, what is the reason why we allow extensions to define a larger
structure which contains CustomPath or CustomScanState? It seems to
me that these types are not (fully) supported by the current copyfuncs,
outfuncs and readfuncs, aren't it?
(Although outfuncs.c supports path-nodes, thus CustomPathMethods has
TextOut callback but no copy/read handler at this moment.)

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Nov 11, 2015 at 3:29 PM, Andres Freund <andres@anarazel.de> wrote:

Just better serialization doesn't actually help all that much. Being
able to conveniently access data directly, i.e. as fields in a struct,
makes code rather more readable. And in many cases more
efficient. Having to serialize internal datastructures unconditionally,
just so copyfuncs.c works if actually used, makes for a fair amount of
inefficiency (forced deserialization, even when not copying) and uglier
code.

Fair enough, but I'm still not very interested in having something for
CustomScan only.

I'm with Robert here. The fact is that postgres_fdw and other FDWs are
living just fine with the restrictions we put in to allow copyfuncs.c to
copy ForeignScan, and there's been no sufficient justification offered
as to why CustomScan can't play by those rules.

So, I'd like to propose copy/out/read callbacks for both of ForeignScan
and CustomScan.

Yes, it would be nice to be able to use a more-tailored struct definition,
but it's not *necessary*. We should not contort the core system
enormously in order to provide a bit more notational cleanliness to
extensions.

I'd be fine with fixing this if a nice solution were to present itself,
but so far nothing's been offered that wasn't horrid.

BTW, the "inefficiency" argument is junk, unless you provide some hard
evidence of a case where it hurts a lot. If we were forcing people to
use serialized representations at execution time, it would be meaningful,
but we're not; you can convert as needed at executor setup time.

Indeed, it is a "nice to have" feature. We can survive without luxury
goods but more expensive tax.

Once an extension tries to implement own join logic, it shall have
much larger number of private fields than usual scan logic.
Andres got a shock when he looked at GpuJoin code of PG-Strom before
because of its pack/unpack routine at:
https://github.com/pg-strom/devel/blob/master/src/gpujoin.c#L112
The custom_private/fdw_private have to be safe to copyObject(), thus,
we have to pack/unpack private variables to/from a List structure.
When we design serialization/deserialization of Plan nodes, it means
ForeignScan and CustomScan has to pay double cost for this.

I never say it is a "necessary" feature, but "very nice to have".

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

On Wed, Nov 11, 2015 at 3:29 PM, Andres Freund <andres@anarazel.de> wrote:

On 2015-11-11 14:59:33 -0500, Robert Haas wrote:

I don't see this as being a particularly good idea. The same issue
exists for FDWs, and we're just living with it in that case.

It's absolutely horrible there. I don't see why that's a justification
for much. To deal with the lack of extensible copy/out/readfuncs I've
just had to copy the entirety of readfuncs.c into an extension. Or you
build replacements for those (as e.g. postgres_fdw essentially has
done).

If we do want to improve it, I'm not sure this is the way to go,
either. I think there could be other designs where we focus on making
the serialization and deserialization options better, rather than
letting people tack stuff onto the struct.

Just better serialization doesn't actually help all that much. Being
able to conveniently access data directly, i.e. as fields in a struct,
makes code rather more readable. And in many cases more
efficient. Having to serialize internal datastructures unconditionally,
just so copyfuncs.c works if actually used, makes for a fair amount of
inefficiency (forced deserialization, even when not copying) and uglier
code.

Fair enough, but I'm still not very interested in having something for
CustomScan only.

I agree with we have no reason why only custom-scan is allowed to have
serialize/deserialize capability. I can implement an equivalent stuff
for foreign-scan also, and it is helpful for extension authors, especially,
who tries to implement remote join feature because FDW driver has to
keep larger number of private fields than scan.

I tried to make a patch to support serialization/deserialization on both
of ForeignScan and CustomScan, but have not tested yet.

One exceptional stuff in ForeignScan is ForeignPath in outfuncs.c, because
ForeignPath itself does not have identifier to get callback functions (it
is kept in RelOptInfo; that is sufficient in planning phase), thus, we have
no way to write out if ForeignPath is a part of larger structure.
We ought to ignore it at this point. How about your opinion?

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

On Wed, Nov 11, 2015 at 11:13 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

I agree with we have no reason why only custom-scan is allowed to have
serialize/deserialize capability. I can implement an equivalent stuff
for foreign-scan also, and it is helpful for extension authors, especially,
who tries to implement remote join feature because FDW driver has to
keep larger number of private fields than scan.

Also, what is the reason why we allow extensions to define a larger
structure which contains CustomPath or CustomScanState? It seems to
me that these types are not (fully) supported by the current copyfuncs,
outfuncs and readfuncs, aren't it?
(Although outfuncs.c supports path-nodes, thus CustomPathMethods has
TextOut callback but no copy/read handler at this moment.)

I feel like we're sort of plunging blindly down a path of trying to
make certain parts of the system extensible and pluggable without
really having a clear vision of where we want to end up. Somehow I
feel like we ought to start by thinking about a framework for *node*
extensibility; then, within that, we can talk about what that means
for particular types of nodes.

For example, suppose do something like this:

typedef struct
{
NodeTag tag;
char *extnodename;
} ExtensibleNode;

typedef stuct
{
char *extnodename;
char *library_name;
char *function_name;
Size node_size;
ExtensibleNode *(*nodeCopy)(ExtensibleNode *);
bool (*nodeEqual)(ExtensibleNode *, ExtensibleNode *);
void (*nodeOut)(StringInfo str, ExtensibleNode *);
ExtensibleNode (*nodeRead)(void);
} ExtensibleNodeMethods;

extern void RegisterExtensibleNodeMethods(ExtensibleNodeMethods *);
extern ExtensibleNodeMethods *GetExtensibleNodeMethods(char *extnodename);

This provides a generic infrastructure so that we don't have to keep
inventing the same stuff over and over, rather than coming up with one
way to do it for ForeignScans and another way for CustomScan and
another way for CustomScanState.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Wed, Nov 11, 2015 at 11:13 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

I agree with we have no reason why only custom-scan is allowed to have
serialize/deserialize capability. I can implement an equivalent stuff
for foreign-scan also, and it is helpful for extension authors, especially,
who tries to implement remote join feature because FDW driver has to
keep larger number of private fields than scan.

Also, what is the reason why we allow extensions to define a larger
structure which contains CustomPath or CustomScanState? It seems to
me that these types are not (fully) supported by the current copyfuncs,
outfuncs and readfuncs, aren't it?
(Although outfuncs.c supports path-nodes, thus CustomPathMethods has
TextOut callback but no copy/read handler at this moment.)

I feel like we're sort of plunging blindly down a path of trying to
make certain parts of the system extensible and pluggable without
really having a clear vision of where we want to end up. Somehow I
feel like we ought to start by thinking about a framework for *node*
extensibility; then, within that, we can talk about what that means
for particular types of nodes.

For example, suppose do something like this:

typedef struct
{
NodeTag tag;
char *extnodename;
} ExtensibleNode;

typedef stuct
{
char *extnodename;
char *library_name;
char *function_name;
Size node_size;
ExtensibleNode *(*nodeCopy)(ExtensibleNode *);
bool (*nodeEqual)(ExtensibleNode *, ExtensibleNode *);
void (*nodeOut)(StringInfo str, ExtensibleNode *);
ExtensibleNode (*nodeRead)(void);
} ExtensibleNodeMethods;

extern void RegisterExtensibleNodeMethods(ExtensibleNodeMethods *);
extern ExtensibleNodeMethods *GetExtensibleNodeMethods(char *extnodename);

This provides a generic infrastructure so that we don't have to keep
inventing the same stuff over and over, rather than coming up with one
way to do it for ForeignScans and another way for CustomScan and
another way for CustomScanState.

Wow! I love the idea.

I guess we will put a pointer to static ExtensibleNodeMethods structure
on ForeignScan, CustomScan, CustomScanState and etc...

Like:
typedef struct ForeignScan
{
Scan scan;
Oid fs_server; /* OID of foreign server */
List *fdw_exprs; /* expressions that FDW may evaluate */
List *fdw_private; /* private data for FDW */
List *fdw_scan_tlist; /* optional tlist describing scan tuple */
List *fdw_recheck_quals; /* original quals not in scan.plan.qual */
Bitmapset *fs_relids; /* RTIs generated by this scan */
bool fsSystemCol; /* true if any "system column" is needed */
+ const ExtensibleNodeMethods *methods;
} ForeignScan;

We may be able to put ExtensibleNodeMethods on the head of CustomScanMethods
structure in case of CustomScan.

Let me write down the steps for deserialization. Please correct me if it is
not what you are thinking.
First, stringToNode picks up a token that shall have node label, like
"SEQSCAN", "CUSTOMSCAN" and others. Once we can identify the following
tokens belong to CustomScan node, it can make advance the pg_strtok pointer
until "methods" field. The method field is consists of a pair of library_name
and symbol_name, then it tries to load the library and resolve the symbol.
Even if library was not loaded on the worker side, this step allows to ensure
the library gets loaded, thus, PG_init() can RegisterExtensibleNodeMethods.
Next, it looks up the table of extensible nodes by a pair of extnodename,
library_name and symbol_name, to get ExtensibleNodeMethods table in this
process address space.
Once it can get nodeRead() callback, we can continue deserialization of
private fields.

I have two concerns on the above proposition.
1) 'node_size' field implies user defined structure to have fixed length.
Extension may want to use variable length structure. The example below
shows GpuJoinState has multiple innerState structure according to the
number of inner relations joined at once.
https://github.com/pg-strom/devel/blob/master/src/gpujoin.c#L240

2) It may be valuable if 'nodeRead(void)' can take an argument of the
knows/common portion of ForeignScan and etc... It allows extension
to adjust number of private fields according to the known part.
For example, I can expect flexible length private fields according
to the length of custom_subplans list.

How about your thought?

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

On Mon, Nov 16, 2015 at 9:03 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

I guess we will put a pointer to static ExtensibleNodeMethods structure
on ForeignScan, CustomScan, CustomScanState and etc...

I think that makes it confusing. What I'd prefer to do is have only
the name stored in the node, and then people who need methods can look
up those methods based on the name.

Let me write down the steps for deserialization. Please correct me if it is
not what you are thinking.
First, stringToNode picks up a token that shall have node label, like
"SEQSCAN", "CUSTOMSCAN" and others. Once we can identify the following
tokens belong to CustomScan node, it can make advance the pg_strtok pointer
until "methods" field. The method field is consists of a pair of library_name
and symbol_name, then it tries to load the library and resolve the symbol.

No. It reads the "extnodename" field (or whatever we decide to call
it) and calls GetExtensibleNodeMethods() on that name. That name
returns the appropriate structure. C libraries that get loaded into
the server can register their extensible node types at _PG_init()
time.

Even if library was not loaded on the worker side, this step allows to ensure
the library gets loaded, thus, PG_init() can RegisterExtensibleNodeMethods.

A parallel worker will load the same loadable modules which the master
has got loaded. If you use some other kind of background worker, of
course, you're on your own.

I have two concerns on the above proposition.
1) 'node_size' field implies user defined structure to have fixed length.
Extension may want to use variable length structure. The example below
shows GpuJoinState has multiple innerState structure according to the
number of inner relations joined at once.
https://github.com/pg-strom/devel/blob/master/src/gpujoin.c#L240

OK, we can replace the node_size field with an allocator callback:
Node (*nodeAlloc)(void) or something like that.

2) It may be valuable if 'nodeRead(void)' can take an argument of the
knows/common portion of ForeignScan and etc... It allows extension
to adjust number of private fields according to the known part.
For example, I can expect flexible length private fields according
to the length of custom_subplans list.

I don't understand this bit.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Mon, Nov 16, 2015 at 9:03 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

I guess we will put a pointer to static ExtensibleNodeMethods structure
on ForeignScan, CustomScan, CustomScanState and etc...

I think that makes it confusing. What I'd prefer to do is have only
the name stored in the node, and then people who need methods can look
up those methods based on the name.

OK. It is equivalent.

Let me write down the steps for deserialization. Please correct me if it is
not what you are thinking.
First, stringToNode picks up a token that shall have node label, like
"SEQSCAN", "CUSTOMSCAN" and others. Once we can identify the following
tokens belong to CustomScan node, it can make advance the pg_strtok pointer
until "methods" field. The method field is consists of a pair of library_name
and symbol_name, then it tries to load the library and resolve the symbol.

No. It reads the "extnodename" field (or whatever we decide to call
it) and calls GetExtensibleNodeMethods() on that name. That name
returns the appropriate structure. C libraries that get loaded into
the server can register their extensible node types at _PG_init()
time.

OK, I got.

Even if library was not loaded on the worker side, this step allows to ensure
the library gets loaded, thus, PG_init() can RegisterExtensibleNodeMethods.

A parallel worker will load the same loadable modules which the master
has got loaded. If you use some other kind of background worker, of
course, you're on your own.

Sorry, I oversight it. And SerializeLibraryState() and RestoreLibraryState()
indeed allow background workers even if out of parallel context to restore
the library state easily.

I have two concerns on the above proposition.
1) 'node_size' field implies user defined structure to have fixed length.
Extension may want to use variable length structure. The example below
shows GpuJoinState has multiple innerState structure according to the
number of inner relations joined at once.
https://github.com/pg-strom/devel/blob/master/src/gpujoin.c#L240

OK, we can replace the node_size field with an allocator callback:
Node (*nodeAlloc)(void) or something like that.

2) It may be valuable if 'nodeRead(void)' can take an argument of the
knows/common portion of ForeignScan and etc... It allows extension
to adjust number of private fields according to the known part.
For example, I can expect flexible length private fields according
to the length of custom_subplans list.

I don't understand this bit.

The above two points are for the case if and when extension want to use
variable length fields for its private fields.
So, nodeAlloc() callback is not a perfect answer for the use case because
length of the variable length fields shall be (usually) determined by the
value of another fields (like num_inner_rels, num_gpu_devices, ...) thus
we cannot determine the correct length before read.

Let's assume an custom-scan extension that wants to have:

typedef struct {
CustomScan cscan;
int priv_value_1;
long priv_value_2;
extra_info_t extra_subplan_info[FLEXIBLE_ARRAY_MEMBER];
/* its length equal to number of sub-plans */
} ExampleScan;

The "extnodename" within CustomScan allows to pull callback functions
to handle read node from the serialized format.
However, nodeAlloc() callback cannot determine the correct length
(= number of sub-plans in this example) prior to read 'cscan' part.

So, I'd like to suggest _readCustomScan (and other extendable nodes
also) read tokens on local CustomScan variable once, then call
Node *(nodeRead)(Node *local_node)
to allocate entire ExampleScan node and read other private fields.

The local_node is already filled up by the core backend prior to
the callback invocation, so extension can know how many sub-plans
are expected thus how many private tokens shall appear.
It also means extension can know exact length of the ExampleScan
node, so it can allocate the node as expected then fill up
remaining private tokens.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

On Thu, Nov 19, 2015 at 10:11 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

The above two points are for the case if and when extension want to use
variable length fields for its private fields.
So, nodeAlloc() callback is not a perfect answer for the use case because
length of the variable length fields shall be (usually) determined by the
value of another fields (like num_inner_rels, num_gpu_devices, ...) thus
we cannot determine the correct length before read.

Let's assume an custom-scan extension that wants to have:

typedef struct {
CustomScan cscan;
int priv_value_1;
long priv_value_2;
extra_info_t extra_subplan_info[FLEXIBLE_ARRAY_MEMBER];
/* its length equal to number of sub-plans */
} ExampleScan;

The "extnodename" within CustomScan allows to pull callback functions
to handle read node from the serialized format.
However, nodeAlloc() callback cannot determine the correct length
(= number of sub-plans in this example) prior to read 'cscan' part.

So, I'd like to suggest _readCustomScan (and other extendable nodes
also) read tokens on local CustomScan variable once, then call
Node *(nodeRead)(Node *local_node)
to allocate entire ExampleScan node and read other private fields.

The local_node is already filled up by the core backend prior to
the callback invocation, so extension can know how many sub-plans
are expected thus how many private tokens shall appear.
It also means extension can know exact length of the ExampleScan
node, so it can allocate the node as expected then fill up
remaining private tokens.

On second thought, I think we should insist that nodes have to be
fixed-size. This sounds like a mess. If the node needs a variable
amount of storage for something, it can store a pointer to a
separately-allocated array someplace inside of it. That's what
existing nodes do, and it works fine.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Thu, Nov 19, 2015 at 10:11 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

The above two points are for the case if and when extension want to use
variable length fields for its private fields.
So, nodeAlloc() callback is not a perfect answer for the use case because
length of the variable length fields shall be (usually) determined by the
value of another fields (like num_inner_rels, num_gpu_devices, ...) thus
we cannot determine the correct length before read.

Let's assume an custom-scan extension that wants to have:

typedef struct {
CustomScan cscan;
int priv_value_1;
long priv_value_2;
extra_info_t extra_subplan_info[FLEXIBLE_ARRAY_MEMBER];
/* its length equal to number of sub-plans */
} ExampleScan;

The "extnodename" within CustomScan allows to pull callback functions
to handle read node from the serialized format.
However, nodeAlloc() callback cannot determine the correct length
(= number of sub-plans in this example) prior to read 'cscan' part.

So, I'd like to suggest _readCustomScan (and other extendable nodes
also) read tokens on local CustomScan variable once, then call
Node *(nodeRead)(Node *local_node)
to allocate entire ExampleScan node and read other private fields.

The local_node is already filled up by the core backend prior to
the callback invocation, so extension can know how many sub-plans
are expected thus how many private tokens shall appear.
It also means extension can know exact length of the ExampleScan
node, so it can allocate the node as expected then fill up
remaining private tokens.

On second thought, I think we should insist that nodes have to be
fixed-size. This sounds like a mess. If the node needs a variable
amount of storage for something, it can store a pointer to a
separately-allocated array someplace inside of it. That's what
existing nodes do, and it works fine.

OK, let's have "node_size" instead of nodeAlloc callback and other
stuffs.

Please wait for a patch.
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Kouhei Kaigai
Sent: Saturday, November 21, 2015 8:05 AM
To: Robert Haas
Cc: Andres Freund; Amit Kapila; pgsql-hackers
Subject: Re: CustomScan in a larger structure (RE: [HACKERS] CustomScan support
on readfuncs.c)

On Thu, Nov 19, 2015 at 10:11 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

The above two points are for the case if and when extension want to use
variable length fields for its private fields.
So, nodeAlloc() callback is not a perfect answer for the use case because
length of the variable length fields shall be (usually) determined by the
value of another fields (like num_inner_rels, num_gpu_devices, ...) thus
we cannot determine the correct length before read.

Let's assume an custom-scan extension that wants to have:

typedef struct {
CustomScan cscan;
int priv_value_1;
long priv_value_2;
extra_info_t extra_subplan_info[FLEXIBLE_ARRAY_MEMBER];
/* its length equal to number of sub-plans */
} ExampleScan;

The "extnodename" within CustomScan allows to pull callback functions
to handle read node from the serialized format.
However, nodeAlloc() callback cannot determine the correct length
(= number of sub-plans in this example) prior to read 'cscan' part.

So, I'd like to suggest _readCustomScan (and other extendable nodes
also) read tokens on local CustomScan variable once, then call
Node *(nodeRead)(Node *local_node)
to allocate entire ExampleScan node and read other private fields.

The local_node is already filled up by the core backend prior to
the callback invocation, so extension can know how many sub-plans
are expected thus how many private tokens shall appear.
It also means extension can know exact length of the ExampleScan
node, so it can allocate the node as expected then fill up
remaining private tokens.

On second thought, I think we should insist that nodes have to be
fixed-size. This sounds like a mess. If the node needs a variable
amount of storage for something, it can store a pointer to a
separately-allocated array someplace inside of it. That's what
existing nodes do, and it works fine.

OK, let's have "node_size" instead of nodeAlloc callback and other
stuffs.

Please wait for a patch.

I'm now implementing. The above design perfectly works on ForeignScan.
On the other hands, I'd like to have deeper consideration for CustomScan.

My recent patch adds LibraryName and SymbolName on CustomScanMethods
to lookup the method table even if library is not loaded yet.
However, this ExtensibleNodeMethods relies custom scan provider shall
be loaded, by parallel infrastructure, prior to the deserialization.
It means extension has a chance to register itself as well.

My idea is, redefine CustomScanMethod as follows:

typedef struct ExtensibleNodeMethods
{
const char *extnodename;
Size node_size;
Node *(*nodeCopy)(const Node *from);
bool (*nodeEqual)(const Node *a, const Node *b);
void (*nodeOut)(struct StringInfoData *str, const Node *node);
void (*nodeRead)(Node *node);
} ExtensibleNodeMethods;

typedef struct CustomScanMethods
{
union {
const char *CustomName;
ExtensibleNodeMethods xnode;
};
/* Create execution state (CustomScanState) from a CustomScan plan node */
Node *(*CreateCustomScanState) (struct CustomScan *cscan);
} CustomScanMethods;

It allows to pull CustomScanMethods using GetExtensibleNodeMethods
by CustomName; that is alias of extnodename in ExtensibleNodeMethods.
Thus, we don't need to care about LibraryName and SymbolName.

This kind of trick is not needed for ForeignScan because all the pointer
stuff are reproducible by catalog, however, method table of CustomScan
is not.

How about your opinion?

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

On Thu, Nov 26, 2015 at 5:27 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

I'm now implementing. The above design perfectly works on ForeignScan.
On the other hands, I'd like to have deeper consideration for CustomScan.

My recent patch adds LibraryName and SymbolName on CustomScanMethods
to lookup the method table even if library is not loaded yet.
However, this ExtensibleNodeMethods relies custom scan provider shall
be loaded, by parallel infrastructure, prior to the deserialization.
It means extension has a chance to register itself as well.

My idea is, redefine CustomScanMethod as follows:

typedef struct ExtensibleNodeMethods
{
const char *extnodename;
Size node_size;
Node *(*nodeCopy)(const Node *from);
bool (*nodeEqual)(const Node *a, const Node *b);
void (*nodeOut)(struct StringInfoData *str, const Node *node);
void (*nodeRead)(Node *node);
} ExtensibleNodeMethods;

typedef struct CustomScanMethods
{
union {
const char *CustomName;
ExtensibleNodeMethods xnode;
};
/* Create execution state (CustomScanState) from a CustomScan plan node */
Node *(*CreateCustomScanState) (struct CustomScan *cscan);
} CustomScanMethods;

It allows to pull CustomScanMethods using GetExtensibleNodeMethods
by CustomName; that is alias of extnodename in ExtensibleNodeMethods.
Thus, we don't need to care about LibraryName and SymbolName.

This kind of trick is not needed for ForeignScan because all the pointer
stuff are reproducible by catalog, however, method table of CustomScan
is not.

How about your opinion?

Anonymous unions are not C89, so this is a no-go.

I think we should just drop CustomName and replace it with
ExtensibleNodeMethods. That will break things for third-party code
that is using the existing CustomScan stuff, but there's a good chance
that nobody other than you has written any such code. And even if
someone has, updating it for this change will not be very difficult.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Thu, Nov 26, 2015 at 5:27 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

I'm now implementing. The above design perfectly works on ForeignScan.
On the other hands, I'd like to have deeper consideration for CustomScan.

My recent patch adds LibraryName and SymbolName on CustomScanMethods
to lookup the method table even if library is not loaded yet.
However, this ExtensibleNodeMethods relies custom scan provider shall
be loaded, by parallel infrastructure, prior to the deserialization.
It means extension has a chance to register itself as well.

My idea is, redefine CustomScanMethod as follows:

typedef struct ExtensibleNodeMethods
{
const char *extnodename;
Size node_size;
Node *(*nodeCopy)(const Node *from);
bool (*nodeEqual)(const Node *a, const Node *b);
void (*nodeOut)(struct StringInfoData *str, const Node *node);
void (*nodeRead)(Node *node);
} ExtensibleNodeMethods;

typedef struct CustomScanMethods
{
union {
const char *CustomName;
ExtensibleNodeMethods xnode;
};
/* Create execution state (CustomScanState) from a CustomScan plan node

*/

Node *(*CreateCustomScanState) (struct CustomScan *cscan);
} CustomScanMethods;

It allows to pull CustomScanMethods using GetExtensibleNodeMethods
by CustomName; that is alias of extnodename in ExtensibleNodeMethods.
Thus, we don't need to care about LibraryName and SymbolName.

This kind of trick is not needed for ForeignScan because all the pointer
stuff are reproducible by catalog, however, method table of CustomScan
is not.

How about your opinion?

Anonymous unions are not C89, so this is a no-go.

I think we should just drop CustomName and replace it with
ExtensibleNodeMethods. That will break things for third-party code
that is using the existing CustomScan stuff, but there's a good chance
that nobody other than you has written any such code. And even if
someone has, updating it for this change will not be very difficult.

Thanks, I have same opinion.
At this moment, CustomName is not utilized so much except for EXPLAIN
and debug output, so it is not a hard stuff to replace this field by
extnodename, even if custom scan provider does not have own structure
thus no callbacks of node operations are defined.

The attached patch adds 'extnodename' fields on ForeignPath and
ForeignScan, also embeds ExtensibleNodeMethods in CustomPathMethods,
CustomScanMethods and CustomExecMethods.

Its handlers in copyfuncs.c were enhanced to utilize the callback
to allocate a larger structure and copy private fields.
Handlers in outfuncs.c and readfuncs.c have to be symmetric. The
core backend writes out 'extnodename' prior to any private fields,
then we can identify the callback to process rest of tokens for
private fields.

RegisterExtensibleNodeMethods() tracks a pair of 'extnodename'
and ExtensibleNodeMethods itself. It saves the pointer of the
method table, but not duplicate, because _readCustomScan()
cast the method pulled by 'extnodename' thus registered table
has to be a static variable on extension; that means extension
never update ExtensibleNodeMethods once registered.

The one other patch is my test using PG-Strom, however, I didn't
have a test on FDW extensions yet.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

On 2015-12-02 08:52:20 +0000, Kouhei Kaigai wrote:

diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 26264cb..c4bb76e 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -635,8 +635,12 @@ _copyWorkTableScan(const WorkTableScan *from)
static ForeignScan *
_copyForeignScan(const ForeignScan *from)
{
-	ForeignScan *newnode = makeNode(ForeignScan);
-
+	const ExtensibleNodeMethods *methods
+		= GetExtensibleNodeMethods(from->extnodename, true);
+	ForeignScan *newnode = (ForeignScan *) newNode(!methods
+												   ? sizeof(ForeignScan)
+												   : methods->node_size,
+												   T_ForeignScan);

Changing the FDW API seems to put this squarely into 9.6
territory. Agreed?

/*
* copy node superclass fields
*/
@@ -645,6 +649,7 @@ _copyForeignScan(const ForeignScan *from)
/*
* copy remainder of node
*/
+	COPY_STRING_FIELD(extnodename);
COPY_SCALAR_FIELD(fs_server);
COPY_NODE_FIELD(fdw_exprs);
COPY_NODE_FIELD(fdw_private);
@@ -652,6 +657,8 @@ _copyForeignScan(const ForeignScan *from)
COPY_NODE_FIELD(fdw_recheck_quals);
COPY_BITMAPSET_FIELD(fs_relids);
COPY_SCALAR_FIELD(fsSystemCol);
+	if (methods && methods->nodeCopy)
+		methods->nodeCopy((Node *) newnode, (const Node *) from);

I wonder if we shouldn't instead "recurse" into a generic routine for
extensible nodes here.

+void
+RegisterExtensibleNodeMethods(const ExtensibleNodeMethods *methods)
+{
+	uint32			hash;
+	int				index;
+	ListCell	   *lc;
+	MemoryContext	oldcxt;
+
+	if (!xnodes_methods_slots)
+		xnodes_methods_slots = MemoryContextAllocZero(TopMemoryContext,
+										  sizeof(List *) * XNODES_NUM_SLOTS);
+
+	hash = hash_any(methods->extnodename, strlen(methods->extnodename));
+	index = hash % XNODES_NUM_SLOTS;
+
+	/* duplication check */
+	foreach (lc, xnodes_methods_slots[index])
+	{
+		const ExtensibleNodeMethods *curr = lfirst(lc);
+
+		if (strcmp(methods->extnodename, curr->extnodename) == 0)
+			ereport(ERROR,
+					(errcode(ERRCODE_DUPLICATE_OBJECT),
+					 errmsg("ExtensibleNodeMethods \"%s\" already exists",
+							methods->extnodename)));
+	}
+	/* ok, register this entry */
+	oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+	xnodes_methods_slots[index] = lappend(xnodes_methods_slots[index],
+										  (void *)methods);
+	MemoryContextSwitchTo(oldcxt);
+}

Why aren't you using dynahash here, and instead reimplement a hashtable?

static ForeignScan *
_readForeignScan(void)
{
+ const ExtensibleNodeMethods *methods;
READ_LOCALS(ForeignScan);

ReadCommonScan(&local_node->scan);

+ READ_STRING_FIELD(extnodename);
READ_OID_FIELD(fs_server);
READ_NODE_FIELD(fdw_exprs);
READ_NODE_FIELD(fdw_private);
@@ -1804,6 +1806,19 @@ _readForeignScan(void)
READ_BITMAPSET_FIELD(fs_relids);
READ_BOOL_FIELD(fsSystemCol);

+	methods = GetExtensibleNodeMethods(local_node->extnodename, true);
+	if (methods && methods->nodeRead)
+	{
+		ForeignScan	   *local_temp = palloc0(methods->node_size);
+
+		Assert(methods->node_size >= sizeof(ForeignScan));
+
+		memcpy(local_temp, local_node, sizeof(ForeignScan));
+		methods->nodeRead((Node *) local_temp);
+
+		pfree(local_node);
+		local_node = local_temp;
+	}
READ_DONE();
}

This imo pretty clearly shows why the 'extensible node' handling should
be delegated to separate routines.

I wonder if we shouldn't, before committing this, at least write a PoC
implementation of actual extensible nodes. I.e. new node types that are
registered at runtime by extensions. ISTM those are relatively closely
linked, and the above doesn't yet support them nicely afaics.

Greetings,

Andres Freund

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

On Wed, Dec 2, 2015 at 4:06 AM, Andres Freund <andres@anarazel.de> wrote:

On 2015-12-02 08:52:20 +0000, Kouhei Kaigai wrote:

diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 26264cb..c4bb76e 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -635,8 +635,12 @@ _copyWorkTableScan(const WorkTableScan *from)
static ForeignScan *
_copyForeignScan(const ForeignScan *from)
{
-     ForeignScan *newnode = makeNode(ForeignScan);
-
+     const ExtensibleNodeMethods *methods
+             = GetExtensibleNodeMethods(from->extnodename, true);
+     ForeignScan *newnode = (ForeignScan *) newNode(!methods
+                                                                                                ? sizeof(ForeignScan)
+                                                                                                : methods->node_size,
+                                                                                                T_ForeignScan);

Changing the FDW API seems to put this squarely into 9.6
territory. Agreed?

I don't think anybody thought this patch was 9.5 material. I didn't,
anyway. The FDW changes for the join-pushdown-EPQ problem are another
issue, but that can be discussed on the other thread.

/*
* copy node superclass fields
*/
@@ -645,6 +649,7 @@ _copyForeignScan(const ForeignScan *from)
/*
* copy remainder of node
*/
+     COPY_STRING_FIELD(extnodename);
COPY_SCALAR_FIELD(fs_server);
COPY_NODE_FIELD(fdw_exprs);
COPY_NODE_FIELD(fdw_private);
@@ -652,6 +657,8 @@ _copyForeignScan(const ForeignScan *from)
COPY_NODE_FIELD(fdw_recheck_quals);
COPY_BITMAPSET_FIELD(fs_relids);
COPY_SCALAR_FIELD(fsSystemCol);
+     if (methods && methods->nodeCopy)
+             methods->nodeCopy((Node *) newnode, (const Node *) from);

I wonder if we shouldn't instead "recurse" into a generic routine for
extensible nodes here.

I don't know what that means.

+void
+RegisterExtensibleNodeMethods(const ExtensibleNodeMethods *methods)
+{
+     uint32                  hash;
+     int                             index;
+     ListCell           *lc;
+     MemoryContext   oldcxt;
+
+     if (!xnodes_methods_slots)
+             xnodes_methods_slots = MemoryContextAllocZero(TopMemoryContext,
+                                                                               sizeof(List *) * XNODES_NUM_SLOTS);
+
+     hash = hash_any(methods->extnodename, strlen(methods->extnodename));
+     index = hash % XNODES_NUM_SLOTS;
+
+     /* duplication check */
+     foreach (lc, xnodes_methods_slots[index])
+     {
+             const ExtensibleNodeMethods *curr = lfirst(lc);
+
+             if (strcmp(methods->extnodename, curr->extnodename) == 0)
+                     ereport(ERROR,
+                                     (errcode(ERRCODE_DUPLICATE_OBJECT),
+                                      errmsg("ExtensibleNodeMethods \"%s\" already exists",
+                                                     methods->extnodename)));
+     }
+     /* ok, register this entry */
+     oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+     xnodes_methods_slots[index] = lappend(xnodes_methods_slots[index],
+                                                                               (void *)methods);
+     MemoryContextSwitchTo(oldcxt);
+}

Why aren't you using dynahash here, and instead reimplement a hashtable?

I had thought we might assume that the number of extensible nodes was
small enough that we could use an array for this and just use linear
search. But if we want to keep the door open to having enough
extensible nodes that this would be inefficient, then I agree that
dynahash is better than a hand-rolled hash table.

static ForeignScan *
_readForeignScan(void)
{
+ const ExtensibleNodeMethods *methods;
READ_LOCALS(ForeignScan);

ReadCommonScan(&local_node->scan);

+ READ_STRING_FIELD(extnodename);
READ_OID_FIELD(fs_server);
READ_NODE_FIELD(fdw_exprs);
READ_NODE_FIELD(fdw_private);
@@ -1804,6 +1806,19 @@ _readForeignScan(void)
READ_BITMAPSET_FIELD(fs_relids);
READ_BOOL_FIELD(fsSystemCol);

+     methods = GetExtensibleNodeMethods(local_node->extnodename, true);
+     if (methods && methods->nodeRead)
+     {
+             ForeignScan        *local_temp = palloc0(methods->node_size);
+
+             Assert(methods->node_size >= sizeof(ForeignScan));
+
+             memcpy(local_temp, local_node, sizeof(ForeignScan));
+             methods->nodeRead((Node *) local_temp);
+
+             pfree(local_node);
+             local_node = local_temp;
+     }
READ_DONE();
}

This imo pretty clearly shows why the 'extensible node' handling should
be delegated to separate routines.

This does look ugly. I'm not sure off-hand how to fix it.

I wonder if we shouldn't, before committing this, at least write a PoC
implementation of actual extensible nodes. I.e. new node types that are
registered at runtime by extensions. ISTM those are relatively closely
linked, and the above doesn't yet support them nicely afaics.

How is what you have in mind different from the pgstrom patch KaiGai attached?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Wed, Dec 2, 2015 at 4:06 AM, Andres Freund <andres@anarazel.de> wrote:

On 2015-12-02 08:52:20 +0000, Kouhei Kaigai wrote:

diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 26264cb..c4bb76e 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -635,8 +635,12 @@ _copyWorkTableScan(const WorkTableScan *from)
static ForeignScan *
_copyForeignScan(const ForeignScan *from)
{
-     ForeignScan *newnode = makeNode(ForeignScan);
-
+     const ExtensibleNodeMethods *methods
+             = GetExtensibleNodeMethods(from->extnodename, true);
+     ForeignScan *newnode = (ForeignScan *) newNode(!methods

+
? sizeof(ForeignScan)

+
: methods->node_size,

+

T_ForeignScan);

Changing the FDW API seems to put this squarely into 9.6
territory. Agreed?

I don't think anybody thought this patch was 9.5 material. I didn't,
anyway. The FDW changes for the join-pushdown-EPQ problem are another
issue, but that can be discussed on the other thread.

I don't expect it is 9.5 feature.
The name of attached patch was "pgsql-v9.6-custom-private.v2.patch".

/*
* copy node superclass fields
*/
@@ -645,6 +649,7 @@ _copyForeignScan(const ForeignScan *from)
/*
* copy remainder of node
*/
+     COPY_STRING_FIELD(extnodename);
COPY_SCALAR_FIELD(fs_server);
COPY_NODE_FIELD(fdw_exprs);
COPY_NODE_FIELD(fdw_private);
@@ -652,6 +657,8 @@ _copyForeignScan(const ForeignScan *from)
COPY_NODE_FIELD(fdw_recheck_quals);
COPY_BITMAPSET_FIELD(fs_relids);
COPY_SCALAR_FIELD(fsSystemCol);
+     if (methods && methods->nodeCopy)
+             methods->nodeCopy((Node *) newnode, (const Node *) from);

I wonder if we shouldn't instead "recurse" into a generic routine for
extensible nodes here.

I don't know what that means.

+void
+RegisterExtensibleNodeMethods(const ExtensibleNodeMethods *methods)
+{
+     uint32                  hash;
+     int                             index;
+     ListCell           *lc;
+     MemoryContext   oldcxt;
+
+     if (!xnodes_methods_slots)
+             xnodes_methods_slots =

MemoryContextAllocZero(TopMemoryContext,

+

sizeof(List *) * XNODES_NUM_SLOTS);

+
+     hash = hash_any(methods->extnodename, strlen(methods->extnodename));
+     index = hash % XNODES_NUM_SLOTS;
+
+     /* duplication check */
+     foreach (lc, xnodes_methods_slots[index])
+     {
+             const ExtensibleNodeMethods *curr = lfirst(lc);
+
+             if (strcmp(methods->extnodename, curr->extnodename) == 0)
+                     ereport(ERROR,
+                                     (errcode(ERRCODE_DUPLICATE_OBJECT),
+                                      errmsg("ExtensibleNodeMethods

\"%s\" already exists",

+

methods->extnodename)));

+     }
+     /* ok, register this entry */
+     oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+     xnodes_methods_slots[index] = lappend(xnodes_methods_slots[index],
+

(void *)methods);

+ MemoryContextSwitchTo(oldcxt);
+}

Why aren't you using dynahash here, and instead reimplement a hashtable?

I had thought we might assume that the number of extensible nodes was
small enough that we could use an array for this and just use linear
search. But if we want to keep the door open to having enough
extensible nodes that this would be inefficient, then I agree that
dynahash is better than a hand-rolled hash table.

Hmm. I also expected the number of extensible nodes are not so much,
so self-defined hash table is sufficient. However, it is not strong
reason. Let me revise the hash implementation.

static ForeignScan *
_readForeignScan(void)
{
+ const ExtensibleNodeMethods *methods;
READ_LOCALS(ForeignScan);

ReadCommonScan(&local_node->scan);

+ READ_STRING_FIELD(extnodename);
READ_OID_FIELD(fs_server);
READ_NODE_FIELD(fdw_exprs);
READ_NODE_FIELD(fdw_private);
@@ -1804,6 +1806,19 @@ _readForeignScan(void)
READ_BITMAPSET_FIELD(fs_relids);
READ_BOOL_FIELD(fsSystemCol);

+     methods = GetExtensibleNodeMethods(local_node->extnodename, true);
+     if (methods && methods->nodeRead)
+     {
+             ForeignScan        *local_temp =

palloc0(methods->node_size);

+
+             Assert(methods->node_size >= sizeof(ForeignScan));
+
+             memcpy(local_temp, local_node, sizeof(ForeignScan));
+             methods->nodeRead((Node *) local_temp);
+
+             pfree(local_node);
+             local_node = local_temp;
+     }
READ_DONE();
}

This imo pretty clearly shows why the 'extensible node' handling should
be delegated to separate routines.

This does look ugly. I'm not sure off-hand how to fix it.

The problem is that we cannot know what extensible node will fit on the
node currently we read unless we don't read extnodename. It also means we
cannot know exact size of the structure prior to the read of extnodename.

I can agree that the above code is never graceful, however, it is not an
avoidable restriction.

I wonder if we shouldn't, before committing this, at least write a PoC
implementation of actual extensible nodes. I.e. new node types that are
registered at runtime by extensions. ISTM those are relatively closely
linked, and the above doesn't yet support them nicely afaics.

How is what you have in mind different from the pgstrom patch KaiGai attached?

This example make node copy by copyObject, then serialize and deserialize
the CustomScan node including extra private fields. I attached it for the
purpose of PoC.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

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