Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-29 Thread Robert Haas 
On Mon, Mar 28, 2016 at 11:00 PM, Kouhei Kaigai <kai...@ak.jp.nec.com> wrote:
>> -Original Message-
>> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
>> Sent: Tuesday, March 29, 2016 10:54 AM
>> To: Kaigai Kouhei(海外 浩平)
>> Cc: Petr Jelinek; pgsql-hackers@postgresql.org
>> Subject: Re: [HACKERS] Reworks of CustomScan serialization/deserialization
>>
>> On Mon, Mar 28, 2016 at 9:36 PM, Kouhei Kaigai <kai...@ak.jp.nec.com> wrote:
>> > I don't have a strong reason to keep these stuff in separate files.
>> > Both stuffs covers similar features and amount of code are enough small.
>> > So, the attached v4 just merged custom-node.[ch] stuff into extensible.
>> >
>> > Once we put similar routines closely, it may be better to consolidate
>> > these routines.
>> > As long as EXTNODENAME_MAX_LEN == CUSTOM_NAME_MAX_LEN, both features
>> > have identical structure layout, so it is easy to call an internal
>> > common function to register or find out a table of callbacks according
>> > to the function actually called by other modules.
>> >
>> > I'm inclined to think to replace EXTNODENAME_MAX_LEN and
>> > CUSTOM_NAME_MAX_LEN by NAMEDATALEN again, to fit structure layout.
>>
>> I don't think that we need both EXTNODENAME_MAX_LEN and
>> CUSTOM_NAME_MAX_LEN; we can use EXTNODENAME_MAX_LEN for both.  I'm
>> opposed to using NAMEDATALEN for anything unrelated to the size of a
>> Name.  If it's not being stored in a catalog, it doesn't need to care.
>>
> OK, I adjusted the v4 patch to use EXTNODENAME_MAX_LEN for both.
>
> The structure of hash entry was revised as follows, then registered via
> an internal common function: RegisterExtensibleNodeEntry, and found out
> via also an internal common function: GetExtensibleNodeEntry.
>
> typedef struct
> {
> charextnodename[EXTNODENAME_MAX_LEN];
> const void *extnodemethods;
>  } ExtensibleNodeEntry;
>
> ExtensibleNodeMethods and CustomScanMethods shall be stored in
> 'extensible_node_methods' and 'custom_scan_methods' separatedly.
> The entrypoint functions calls above internal common functions with
> appropriate HTAB variable.
>
> It will be re-usable if we would have further extensible nodes in the
> future versions.

Committed with a bit of wordsmithing.

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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-28 Thread Kouhei Kaigai 
> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> Sent: Tuesday, March 29, 2016 10:54 AM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Petr Jelinek; pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Reworks of CustomScan serialization/deserialization
> 
> On Mon, Mar 28, 2016 at 9:36 PM, Kouhei Kaigai <kai...@ak.jp.nec.com> wrote:
> > I don't have a strong reason to keep these stuff in separate files.
> > Both stuffs covers similar features and amount of code are enough small.
> > So, the attached v4 just merged custom-node.[ch] stuff into extensible.
> >
> > Once we put similar routines closely, it may be better to consolidate
> > these routines.
> > As long as EXTNODENAME_MAX_LEN == CUSTOM_NAME_MAX_LEN, both features
> > have identical structure layout, so it is easy to call an internal
> > common function to register or find out a table of callbacks according
> > to the function actually called by other modules.
> >
> > I'm inclined to think to replace EXTNODENAME_MAX_LEN and
> > CUSTOM_NAME_MAX_LEN by NAMEDATALEN again, to fit structure layout.
> 
> I don't think that we need both EXTNODENAME_MAX_LEN and
> CUSTOM_NAME_MAX_LEN; we can use EXTNODENAME_MAX_LEN for both.  I'm
> opposed to using NAMEDATALEN for anything unrelated to the size of a
> Name.  If it's not being stored in a catalog, it doesn't need to care.
>
OK, I adjusted the v4 patch to use EXTNODENAME_MAX_LEN for both.

The structure of hash entry was revised as follows, then registered via
an internal common function: RegisterExtensibleNodeEntry, and found out
via also an internal common function: GetExtensibleNodeEntry.

typedef struct
{
charextnodename[EXTNODENAME_MAX_LEN];
const void *extnodemethods;
 } ExtensibleNodeEntry;

ExtensibleNodeMethods and CustomScanMethods shall be stored in
'extensible_node_methods' and 'custom_scan_methods' separatedly.
The entrypoint functions calls above internal common functions with
appropriate HTAB variable.

It will be re-usable if we would have further extensible nodes in the
future versions.

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


pgsql-v9.6-custom-scan-serialization-reworks.5.patch
Description: pgsql-v9.6-custom-scan-serialization-reworks.5.patch

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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-28 Thread Robert Haas 
On Mon, Mar 28, 2016 at 9:36 PM, Kouhei Kaigai  wrote:
> I don't have a strong reason to keep these stuff in separate files.
> Both stuffs covers similar features and amount of code are enough small.
> So, the attached v4 just merged custom-node.[ch] stuff into extensible.
>
> Once we put similar routines closely, it may be better to consolidate
> these routines.
> As long as EXTNODENAME_MAX_LEN == CUSTOM_NAME_MAX_LEN, both features
> have identical structure layout, so it is easy to call an internal
> common function to register or find out a table of callbacks according
> to the function actually called by other modules.
>
> I'm inclined to think to replace EXTNODENAME_MAX_LEN and
> CUSTOM_NAME_MAX_LEN by NAMEDATALEN again, to fit structure layout.

I don't think that we need both EXTNODENAME_MAX_LEN and
CUSTOM_NAME_MAX_LEN; we can use EXTNODENAME_MAX_LEN for both.  I'm
opposed to using NAMEDATALEN for anything unrelated to the size of a
Name.  If it's not being stored in a catalog, it doesn't need to care.

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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-28 Thread Kouhei Kaigai 
> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> Sent: Friday, March 25, 2016 12:27 AM
> To: Petr Jelinek
> Cc: Kaigai Kouhei(海外 浩平); pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Reworks of CustomScan serialization/deserialization
> 
> On Wed, Mar 23, 2016 at 1:36 PM, Petr Jelinek <p...@2ndquadrant.com> wrote:
> > Ok, I am happy with it, marked it as ready for committer (it was marked as
> > committed although it wasn't committed).
> 
> Thanks for fixing the status.   I had forgotten about this thread.
> 
> I can't really endorse the naming conventions here.  I mean, we've got
> the main extensible nodes stuff in extensible.h, and then we've got
> this stuff in custom_node.h (BTW, there is a leftover reference to
> custom-node.h).  There's no hint in the naming that this relates to
> scans, and why is it extensible in one place and custom in another?
> 
> I'm not quite sure how to clean this up.  At a minimum, I think we
> should standardize on "custom_scan.h" instead of "custom_node.h".  I
> think that would be clearer.  But I'm wondering if we should bite the
> bullet and rename everything from "custom" to "extensible" and declare
> it all in "extensible.h".
>
I don't have a strong reason to keep these stuff in separate files.
Both stuffs covers similar features and amount of code are enough small.
So, the attached v4 just merged custom-node.[ch] stuff into extensible.

Once we put similar routines closely, it may be better to consolidate
these routines.
As long as EXTNODENAME_MAX_LEN == CUSTOM_NAME_MAX_LEN, both features
have identical structure layout, so it is easy to call an internal
common function to register or find out a table of callbacks according
to the function actually called by other modules.

I'm inclined to think to replace EXTNODENAME_MAX_LEN and
CUSTOM_NAME_MAX_LEN by NAMEDATALEN again, to fit structure layout.

> src/backend/nodes/custom_node.c:45: indent with spaces.
> +}
> 
Oops, thanks,

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



pgsql-v9.6-custom-scan-serialization-reworks.4.patch
Description: pgsql-v9.6-custom-scan-serialization-reworks.4.patch

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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-24 Thread Robert Haas 
On Wed, Mar 23, 2016 at 1:36 PM, Petr Jelinek  wrote:
> Ok, I am happy with it, marked it as ready for committer (it was marked as
> committed although it wasn't committed).

Thanks for fixing the status.   I had forgotten about this thread.

I can't really endorse the naming conventions here.  I mean, we've got
the main extensible nodes stuff in extensible.h, and then we've got
this stuff in custom_node.h (BTW, there is a leftover reference to
custom-node.h).  There's no hint in the naming that this relates to
scans, and why is it extensible in one place and custom in another?

I'm not quite sure how to clean this up.  At a minimum, I think we
should standardize on "custom_scan.h" instead of "custom_node.h".  I
think that would be clearer.  But I'm wondering if we should bite the
bullet and rename everything from "custom" to "extensible" and declare
it all in "extensible.h".

src/backend/nodes/custom_node.c:45: indent with spaces.
+}

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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-23 Thread Petr Jelinek 

On 23/03/16 09:14, Kouhei Kaigai wrote:

On 15/03/16 05:03, Kouhei Kaigai wrote:

Petr,

The attached patch is the revised one that follows the new extensible-
node routine.

It is almost same the previous version except for:
- custom-apis.[ch] was renamed to custom-node.[ch]
- check for the length of custom-scan-method name followed
 the manner of RegisterExtensibleNodeMethods()



Hi,

looks good, only nitpick I have is that it probably should be
custom_node.h with underscore given that we use underscore everywhere
(except for libpq and for some reason atomic ops).


Sorry for my response late.

The attached patch just renamed custom-node.[ch] by custom_node.[ch].
Other portions are not changed from the previous revison.



Forgot to attach?


Yes Thanks,



Ok, I am happy with it, marked it as ready for committer (it was marked 
as committed although it wasn't committed).


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-23 Thread Kouhei Kaigai 
> >> On 15/03/16 05:03, Kouhei Kaigai wrote:
> >>> Petr,
> >>>
> >>> The attached patch is the revised one that follows the new extensible-
> >>> node routine.
> >>>
> >>> It is almost same the previous version except for:
> >>> - custom-apis.[ch] was renamed to custom-node.[ch]
> >>> - check for the length of custom-scan-method name followed
> >>> the manner of RegisterExtensibleNodeMethods()
> >>>
> >>
> >> Hi,
> >>
> >> looks good, only nitpick I have is that it probably should be
> >> custom_node.h with underscore given that we use underscore everywhere
> >> (except for libpq and for some reason atomic ops).
> >>
> > Sorry for my response late.
> >
> > The attached patch just renamed custom-node.[ch] by custom_node.[ch].
> > Other portions are not changed from the previous revison.
> >
> 
> Forgot to attach?
>
Yes Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 



pgsql-v9.6-custom-scan-serialization-reworks.3.patch
Description: pgsql-v9.6-custom-scan-serialization-reworks.3.patch

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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-23 Thread Petr Jelinek 

On 23/03/16 08:34, Kouhei Kaigai wrote:

-Original Message-
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Petr Jelinek
Sent: Thursday, March 17, 2016 5:06 PM
To: Kaigai Kouhei(海外 浩平)
Cc: Robert Haas; pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] Reworks of CustomScan serialization/deserialization

On 15/03/16 05:03, Kouhei Kaigai wrote:

Petr,

The attached patch is the revised one that follows the new extensible-
node routine.

It is almost same the previous version except for:
- custom-apis.[ch] was renamed to custom-node.[ch]
- check for the length of custom-scan-method name followed
the manner of RegisterExtensibleNodeMethods()



Hi,

looks good, only nitpick I have is that it probably should be
custom_node.h with underscore given that we use underscore everywhere
(except for libpq and for some reason atomic ops).


Sorry for my response late.

The attached patch just renamed custom-node.[ch] by custom_node.[ch].
Other portions are not changed from the previous revison.



Forgot to attach?

--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-23 Thread Kouhei Kaigai 
> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Petr Jelinek
> Sent: Thursday, March 17, 2016 5:06 PM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Robert Haas; pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Reworks of CustomScan serialization/deserialization
> 
> On 15/03/16 05:03, Kouhei Kaigai wrote:
> > Petr,
> >
> > The attached patch is the revised one that follows the new extensible-
> > node routine.
> >
> > It is almost same the previous version except for:
> > - custom-apis.[ch] was renamed to custom-node.[ch]
> > - check for the length of custom-scan-method name followed
> >the manner of RegisterExtensibleNodeMethods()
> >
> 
> Hi,
> 
> looks good, only nitpick I have is that it probably should be
> custom_node.h with underscore given that we use underscore everywhere
> (except for libpq and for some reason atomic ops).
>
Sorry for my response late.

The attached patch just renamed custom-node.[ch] by custom_node.[ch].
Other portions are not changed from the previous revison.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kai...@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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-19 Thread Petr Jelinek 

On 15/03/16 05:03, Kouhei Kaigai wrote:

Petr,

The attached patch is the revised one that follows the new extensible-
node routine.

It is almost same the previous version except for:
- custom-apis.[ch] was renamed to custom-node.[ch]
- check for the length of custom-scan-method name followed
   the manner of RegisterExtensibleNodeMethods()



Hi,

looks good, only nitpick I have is that it probably should be 
custom_node.h with underscore given that we use underscore everywhere 
(except for libpq and for some reason atomic ops).


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-14 Thread Kouhei Kaigai 
Petr,

The attached patch is the revised one that follows the new extensible-
node routine.

It is almost same the previous version except for:
- custom-apis.[ch] was renamed to custom-node.[ch]
- check for the length of custom-scan-method name followed
  the manner of RegisterExtensibleNodeMethods()

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


> -Original Message-
> From: Robert Haas [mailto:robertmh...@gmail.com]
> Sent: Tuesday, March 15, 2016 2:54 AM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Petr Jelinek; pgsql-hackers@postgresql.org
> Subject: ##freemail## Re: [HACKERS] Reworks of CustomScan
> serialization/deserialization
> 
> On Sun, Mar 13, 2016 at 9:53 PM, Kouhei Kaigai <kai...@ak.jp.nec.com> wrote:
> > OK, I split the previous small patch into two tiny patches.
> > The one is bugfix around max length of the extnodename.
> > The other replaces Assert() by ereport() according to the upthread 
> > discussion.
> 
> Committed, except that (1) I replaced ereport() with elog(), because I
> can't see making translators care about this message; and (2) I
> reworded the error message a bit.
> 
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company


pgsql-v9.6-custom-scan-serialization-reworks.2.patch
Description: pgsql-v9.6-custom-scan-serialization-reworks.2.patch

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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-14 Thread Kouhei Kaigai 
> On Sun, Mar 13, 2016 at 9:53 PM, Kouhei Kaigai  wrote:
> > OK, I split the previous small patch into two tiny patches.
> > The one is bugfix around max length of the extnodename.
> > The other replaces Assert() by ereport() according to the upthread 
> > discussion.
> 
> Committed, except that (1) I replaced ereport() with elog(), because I
> can't see making translators care about this message; and (2) I
> reworded the error message a bit.
>
Thanks, and I got the point why ereport() is suggested for the error
message that may be visible to users, instead of elog().

--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-14 Thread Robert Haas 
On Sun, Mar 13, 2016 at 9:53 PM, Kouhei Kaigai  wrote:
> OK, I split the previous small patch into two tiny patches.
> The one is bugfix around max length of the extnodename.
> The other replaces Assert() by ereport() according to the upthread discussion.

Committed, except that (1) I replaced ereport() with elog(), because I
can't see making translators care about this message; and (2) I
reworded the error message a 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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-13 Thread Kouhei Kaigai 
> On 14/03/16 02:53, Kouhei Kaigai wrote:
> >> -Original Message-
> >> From: pgsql-hackers-ow...@postgresql.org
> >> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Petr Jelinek
> >> Sent: Friday, March 11, 2016 12:27 AM
> >> To: Kaigai Kouhei(海外 浩平); pgsql-hackers@postgresql.org
> >> Subject: Re: [HACKERS] Reworks of CustomScan serialization/deserialization
> >>
> >> On 10/03/16 08:08, Kouhei Kaigai wrote:
> >>>>
> >>>>>> Also in RegisterCustomScanMethods
> >>>>>> +  Assert(strlen(methods->CustomName) <= CUSTOM_NAME_MAX_LEN);
> >>>>>>
> >>>>>> Shouldn't this be actually "if" with ereport() considering this is
> >>>>>> public API and extensions can pass anything there? (for that matter 
> >>>>>> same
> >>>>>> is true for RegisterExtensibleNodeMethods but that's already 
> >>>>>> committed).
> >>>>>>
> >>>>> Hmm. I don't have clear answer which is better. The reason why I put
> >>>>> Assert() here is that only c-binary extension uses this interface, thus,
> >>>>> author will fix up the problem of too long name prior to its release.
> >>>>> Of course, if-with-ereport() also informs extension author the name is
> >>>>> too long.
> >>>>> One downside of Assert() may be, it makes oversight if --enable-cassert
> >>>>> was not specified.
> >>>>>
> >>>>
> >>>> Well that's exactly my problem, this should IMHO throw error even
> >>>> without --enable-cassert. It's not like it's some performance sensitive
> >>>> API where if would be big problem, ensuring correctness of the input is
> >>>> more imporant here IMHO.
> >>>>
> >>> We may need to fix up RegisterExtensibleNodeMethods() first.
> >>>
> >>> Also, length limitation is (EXTNODENAME_MAX_LEN-1) because the last byte
> >>> is consumed by '\0' character. In fact, hash, match and keycopy function
> >>> of HTAB for string keys deal with the first (keysize - 1) bytes.
> >>> So, strkey(extnodename) == EXTNODENAME_MAX_LEN is not legal.
> >>>
> >>
> >> Yes, my thoughts as well but that can be separate tiny patch that does
> >> not have to affect this one. In my opinion if we fixed this one it would
> >> be otherwise ready to go in, and I definitely prefer this approach to
> >> the previous one.
> >>
> > OK, I split the previous small patch into two tiny patches.
> > The one is bugfix around max length of the extnodename.
> > The other replaces Assert() by ereport() according to the upthread 
> > discussion.
> >
> 
> Okay, it's somewhat akin to hairsplitting but works for me. Do you plan
> to do same thing with the CustomScan patch itself as well?.
>
Yes. I'll fixup the patch to follow the same manner.

--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kai...@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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-13 Thread Petr Jelinek 
On 14/03/16 02:53, Kouhei Kaigai wrote:
>> -Original Message-
>> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Petr Jelinek
>> Sent: Friday, March 11, 2016 12:27 AM
>> To: Kaigai Kouhei(海外 浩平); pgsql-hackers@postgresql.org
>> Subject: Re: [HACKERS] Reworks of CustomScan serialization/deserialization
>>
>> On 10/03/16 08:08, Kouhei Kaigai wrote:
>>>>
>>>>>> Also in RegisterCustomScanMethods
>>>>>> +Assert(strlen(methods->CustomName) <= CUSTOM_NAME_MAX_LEN);
>>>>>>
>>>>>> Shouldn't this be actually "if" with ereport() considering this is
>>>>>> public API and extensions can pass anything there? (for that matter same
>>>>>> is true for RegisterExtensibleNodeMethods but that's already committed).
>>>>>>
>>>>> Hmm. I don't have clear answer which is better. The reason why I put
>>>>> Assert() here is that only c-binary extension uses this interface, thus,
>>>>> author will fix up the problem of too long name prior to its release.
>>>>> Of course, if-with-ereport() also informs extension author the name is
>>>>> too long.
>>>>> One downside of Assert() may be, it makes oversight if --enable-cassert
>>>>> was not specified.
>>>>>
>>>>
>>>> Well that's exactly my problem, this should IMHO throw error even
>>>> without --enable-cassert. It's not like it's some performance sensitive
>>>> API where if would be big problem, ensuring correctness of the input is
>>>> more imporant here IMHO.
>>>>
>>> We may need to fix up RegisterExtensibleNodeMethods() first.
>>>
>>> Also, length limitation is (EXTNODENAME_MAX_LEN-1) because the last byte
>>> is consumed by '\0' character. In fact, hash, match and keycopy function
>>> of HTAB for string keys deal with the first (keysize - 1) bytes.
>>> So, strkey(extnodename) == EXTNODENAME_MAX_LEN is not legal.
>>>
>>
>> Yes, my thoughts as well but that can be separate tiny patch that does
>> not have to affect this one. In my opinion if we fixed this one it would
>> be otherwise ready to go in, and I definitely prefer this approach to
>> the previous one.
>>
> OK, I split the previous small patch into two tiny patches.
> The one is bugfix around max length of the extnodename.
> The other replaces Assert() by ereport() according to the upthread discussion.
> 

Okay, it's somewhat akin to hairsplitting but works for me. Do you plan
to do same thing with the CustomScan patch itself as well?.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-13 Thread Kouhei Kaigai 
> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Petr Jelinek
> Sent: Friday, March 11, 2016 12:27 AM
> To: Kaigai Kouhei(海外 浩平); pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Reworks of CustomScan serialization/deserialization
> 
> On 10/03/16 08:08, Kouhei Kaigai wrote:
> >>
> >>>> Also in RegisterCustomScanMethods
> >>>> +Assert(strlen(methods->CustomName) <= CUSTOM_NAME_MAX_LEN);
> >>>>
> >>>> Shouldn't this be actually "if" with ereport() considering this is
> >>>> public API and extensions can pass anything there? (for that matter same
> >>>> is true for RegisterExtensibleNodeMethods but that's already committed).
> >>>>
> >>> Hmm. I don't have clear answer which is better. The reason why I put
> >>> Assert() here is that only c-binary extension uses this interface, thus,
> >>> author will fix up the problem of too long name prior to its release.
> >>> Of course, if-with-ereport() also informs extension author the name is
> >>> too long.
> >>> One downside of Assert() may be, it makes oversight if --enable-cassert
> >>> was not specified.
> >>>
> >>
> >> Well that's exactly my problem, this should IMHO throw error even
> >> without --enable-cassert. It's not like it's some performance sensitive
> >> API where if would be big problem, ensuring correctness of the input is
> >> more imporant here IMHO.
> >>
> > We may need to fix up RegisterExtensibleNodeMethods() first.
> >
> > Also, length limitation is (EXTNODENAME_MAX_LEN-1) because the last byte
> > is consumed by '\0' character. In fact, hash, match and keycopy function
> > of HTAB for string keys deal with the first (keysize - 1) bytes.
> > So, strkey(extnodename) == EXTNODENAME_MAX_LEN is not legal.
> >
> 
> Yes, my thoughts as well but that can be separate tiny patch that does
> not have to affect this one. In my opinion if we fixed this one it would
> be otherwise ready to go in, and I definitely prefer this approach to
> the previous one.
>
OK, I split the previous small patch into two tiny patches.
The one is bugfix around max length of the extnodename.
The other replaces Assert() by ereport() according to the upthread discussion.

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



pgsql-v9.6-fix-extnodename-max-len.patch
Description: pgsql-v9.6-fix-extnodename-max-len.patch


pgsql-v9.6-replace-assert-by-ereport-on-register-extnode.patch
Description: pgsql-v9.6-replace-assert-by-ereport-on-register-extnode.patch

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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-10 Thread Petr Jelinek 
On 10/03/16 08:08, Kouhei Kaigai wrote:
>>
 Also in RegisterCustomScanMethods
 +  Assert(strlen(methods->CustomName) <= CUSTOM_NAME_MAX_LEN);

 Shouldn't this be actually "if" with ereport() considering this is
 public API and extensions can pass anything there? (for that matter same
 is true for RegisterExtensibleNodeMethods but that's already committed).

>>> Hmm. I don't have clear answer which is better. The reason why I put
>>> Assert() here is that only c-binary extension uses this interface, thus,
>>> author will fix up the problem of too long name prior to its release.
>>> Of course, if-with-ereport() also informs extension author the name is
>>> too long.
>>> One downside of Assert() may be, it makes oversight if --enable-cassert
>>> was not specified.
>>>
>>
>> Well that's exactly my problem, this should IMHO throw error even
>> without --enable-cassert. It's not like it's some performance sensitive
>> API where if would be big problem, ensuring correctness of the input is
>> more imporant here IMHO.
>>
> We may need to fix up RegisterExtensibleNodeMethods() first.
> 
> Also, length limitation is (EXTNODENAME_MAX_LEN-1) because the last byte
> is consumed by '\0' character. In fact, hash, match and keycopy function
> of HTAB for string keys deal with the first (keysize - 1) bytes.
> So, strkey(extnodename) == EXTNODENAME_MAX_LEN is not legal.
> 

Yes, my thoughts as well but that can be separate tiny patch that does
not have to affect this one. In my opinion if we fixed this one it would
be otherwise ready to go in, and I definitely prefer this approach to
the previous one.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-09 Thread Kouhei Kaigai 
> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Petr Jelinek
> Sent: Thursday, March 10, 2016 11:01 AM
> To: Kaigai Kouhei(海外 浩平); pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Reworks of CustomScan serialization/deserialization
> 
> On 10/03/16 02:18, Kouhei Kaigai wrote:
> >
> >> I am not sure I like the fact that we have this EXTNODENAME_MAX_LEN and
> >> now the CUSTOM_NAME_MAX_LEN with the same length and also they are both
> >> same lenght as NAMEDATALEN I wonder if this shouldn't be somehow
> >> squished to less defines.
> >>
> > Hmm. I just followed the manner in extensible.c, because this label was
> > initially NAMEDATALEN, then Robert changed it with EXTNODENAME_MAX_LEN.
> > I guess he avoid to apply same label on different entities - NAMEDATALEN
> > is a limitation for NameData type, but identifier of extensible-node and
> > custom-scan node are not restricted by.
> >
> 
> Makes sense.
> 
> >> Also in RegisterCustomScanMethods
> >> +  Assert(strlen(methods->CustomName) <= CUSTOM_NAME_MAX_LEN);
> >>
> >> Shouldn't this be actually "if" with ereport() considering this is
> >> public API and extensions can pass anything there? (for that matter same
> >> is true for RegisterExtensibleNodeMethods but that's already committed).
> >>
> > Hmm. I don't have clear answer which is better. The reason why I put
> > Assert() here is that only c-binary extension uses this interface, thus,
> > author will fix up the problem of too long name prior to its release.
> > Of course, if-with-ereport() also informs extension author the name is
> > too long.
> > One downside of Assert() may be, it makes oversight if --enable-cassert
> > was not specified.
> >
> 
> Well that's exactly my problem, this should IMHO throw error even
> without --enable-cassert. It's not like it's some performance sensitive
> API where if would be big problem, ensuring correctness of the input is
> more imporant here IMHO.
>
We may need to fix up RegisterExtensibleNodeMethods() first.

Also, length limitation is (EXTNODENAME_MAX_LEN-1) because the last byte
is consumed by '\0' character. In fact, hash, match and keycopy function
of HTAB for string keys deal with the first (keysize - 1) bytes.
So, strkey(extnodename) == EXTNODENAME_MAX_LEN is not legal.

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



pgsql-v9.6-extensible-namelen-check-by-ereport.patch
Description: pgsql-v9.6-extensible-namelen-check-by-ereport.patch

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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-09 Thread Petr Jelinek 
On 10/03/16 02:18, Kouhei Kaigai wrote:
> 
>> I am not sure I like the fact that we have this EXTNODENAME_MAX_LEN and
>> now the CUSTOM_NAME_MAX_LEN with the same length and also they are both
>> same lenght as NAMEDATALEN I wonder if this shouldn't be somehow
>> squished to less defines.
>>
> Hmm. I just followed the manner in extensible.c, because this label was
> initially NAMEDATALEN, then Robert changed it with EXTNODENAME_MAX_LEN.
> I guess he avoid to apply same label on different entities - NAMEDATALEN
> is a limitation for NameData type, but identifier of extensible-node and
> custom-scan node are not restricted by.
> 

Makes sense.

>> Also in RegisterCustomScanMethods
>> +Assert(strlen(methods->CustomName) <= CUSTOM_NAME_MAX_LEN);
>>
>> Shouldn't this be actually "if" with ereport() considering this is
>> public API and extensions can pass anything there? (for that matter same
>> is true for RegisterExtensibleNodeMethods but that's already committed).
>>
> Hmm. I don't have clear answer which is better. The reason why I put
> Assert() here is that only c-binary extension uses this interface, thus,
> author will fix up the problem of too long name prior to its release.
> Of course, if-with-ereport() also informs extension author the name is
> too long.
> One downside of Assert() may be, it makes oversight if --enable-cassert
> was not specified.
> 

Well that's exactly my problem, this should IMHO throw error even
without --enable-cassert. It's not like it's some performance sensitive
API where if would be big problem, ensuring correctness of the input is
more imporant here IMHO.


-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-09 Thread Kouhei Kaigai 
> On 29/02/16 13:07, Kouhei Kaigai wrote:
> >
> > I'd like to adjust a few of custom-scan interface prior to v9.6 freeze.
> >
> > The major point is serialization/deserialization mechanism.
> > Now, extension has to give LibraryName and SymbolName to reproduce
> > same CustomScanMethods on the background worker process side. Indeed,
> > it is sufficient information to pull the table of function pointers.
> >
> > On the other hands, we now have different mechanism to wrap private
> > information - extensible node. It requires extensions to register its
> > ExtensibleNodeMethods identified by name, usually, on _PG_init() time.
> > It is also reasonable way to reproduce same objects on background
> > worker side.
> >
> > However, mixture of two different ways is not good. My preference is
> > what extensible-node is doing rather than what custom-scan is currently
> > doing.
> > The attached patch allows extension to register CustomScanMethods once,
> > then readFunc.c can pull this table by CustomName in string form.
> >
> 
> Agreed, but this will break compatibility right?
>
The manner to pass a pair of library-name and symbol-name is a new feature
in v9.6, not in v9.5, so it is now the last chance to fix up the interface
requirement.

> > I'm not 100% certain whether "nodes/custom-apis.h" is the best location,
> > but somewhere we can put these declarations rather than the primitive
> > header files might be needed.
> 
> custom-apis.c does not sound like right name to me, maybe it can be just
> custom.c but custom.h might be bit too generic, maybe custom_node.h
>
OK, custom_node.h may be better.

> I am not sure I like the fact that we have this EXTNODENAME_MAX_LEN and
> now the CUSTOM_NAME_MAX_LEN with the same length and also they are both
> same lenght as NAMEDATALEN I wonder if this shouldn't be somehow
> squished to less defines.
>
Hmm. I just followed the manner in extensible.c, because this label was
initially NAMEDATALEN, then Robert changed it with EXTNODENAME_MAX_LEN.
I guess he avoid to apply same label on different entities - NAMEDATALEN
is a limitation for NameData type, but identifier of extensible-node and
custom-scan node are not restricted by.

> Also in RegisterCustomScanMethods
> + Assert(strlen(methods->CustomName) <= CUSTOM_NAME_MAX_LEN);
> 
> Shouldn't this be actually "if" with ereport() considering this is
> public API and extensions can pass anything there? (for that matter same
> is true for RegisterExtensibleNodeMethods but that's already committed).
>
Hmm. I don't have clear answer which is better. The reason why I put
Assert() here is that only c-binary extension uses this interface, thus,
author will fix up the problem of too long name prior to its release.
Of course, if-with-ereport() also informs extension author the name is
too long.
One downside of Assert() may be, it makes oversight if --enable-cassert
was not specified.

> Other than that this seems like straight conversion to same basic
> template as extensible nodes so I think it's ok.
> 

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 



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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

2016-03-09 Thread Petr Jelinek 
Hi,

On 29/02/16 13:07, Kouhei Kaigai wrote:
> 
> I'd like to adjust a few of custom-scan interface prior to v9.6 freeze.
> 
> The major point is serialization/deserialization mechanism.
> Now, extension has to give LibraryName and SymbolName to reproduce
> same CustomScanMethods on the background worker process side. Indeed,
> it is sufficient information to pull the table of function pointers.
> 
> On the other hands, we now have different mechanism to wrap private
> information - extensible node. It requires extensions to register its
> ExtensibleNodeMethods identified by name, usually, on _PG_init() time.
> It is also reasonable way to reproduce same objects on background
> worker side.
> 
> However, mixture of two different ways is not good. My preference is
> what extensible-node is doing rather than what custom-scan is currently
> doing.
> The attached patch allows extension to register CustomScanMethods once,
> then readFunc.c can pull this table by CustomName in string form.
> 

Agreed, but this will break compatibility right?

> I'm not 100% certain whether "nodes/custom-apis.h" is the best location,
> but somewhere we can put these declarations rather than the primitive
> header files might be needed.

custom-apis.c does not sound like right name to me, maybe it can be just
custom.c but custom.h might be bit too generic, maybe custom_node.h

I am not sure I like the fact that we have this EXTNODENAME_MAX_LEN and
now the CUSTOM_NAME_MAX_LEN with the same length and also they are both
same lenght as NAMEDATALEN I wonder if this shouldn't be somehow
squished to less defines.

Also in RegisterCustomScanMethods
+   Assert(strlen(methods->CustomName) <= CUSTOM_NAME_MAX_LEN);

Shouldn't this be actually "if" with ereport() considering this is
public API and extensions can pass anything there? (for that matter same
is true for RegisterExtensibleNodeMethods but that's already committed).

Other than that this seems like straight conversion to same basic
template as extensible nodes so I think it's ok.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


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

[HACKERS] Reworks of CustomScan serialization/deserialization

2016-02-29 Thread Kouhei Kaigai 
Hello,

I'd like to adjust a few of custom-scan interface prior to v9.6 freeze.

The major point is serialization/deserialization mechanism.
Now, extension has to give LibraryName and SymbolName to reproduce
same CustomScanMethods on the background worker process side. Indeed,
it is sufficient information to pull the table of function pointers.

On the other hands, we now have different mechanism to wrap private
information - extensible node. It requires extensions to register its
ExtensibleNodeMethods identified by name, usually, on _PG_init() time.
It is also reasonable way to reproduce same objects on background
worker side.

However, mixture of two different ways is not good. My preference is
what extensible-node is doing rather than what custom-scan is currently
doing.
The attached patch allows extension to register CustomScanMethods once,
then readFunc.c can pull this table by CustomName in string form.


The minor one is header file location of CustomMethods declaration.
These are currently declared at relation.h, plannodes.h and execnodes.h.
These files are very primitive, so we put these lines:

  struct ParallelContext; /* avoid including parallel.h here */
  struct shm_toc; /* avoid including shm_toc.h here */
  struct ExplainState;/* avoid including explain.h here */

to avoid inclusion of other headers here.

It seems to me CustomMethods shall be moved to somewhere appropriate,
like fdwapi.h for FDW. If we put "struct CustomMethods;" on these
primitive header files instead, it will work.

I'm not 100% certain whether "nodes/custom-apis.h" is the best location,
but somewhere we can put these declarations rather than the primitive
header files might be needed.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


pgsql-v9.6-custom-scan-serialization-reworks.1.patch
Description: pgsql-v9.6-custom-scan-serialization-reworks.1.patch

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