Re: Reworks of CustomScan serialization/deserialization

> On 14/03/16 02:53, Kouhei Kaigai wrote:
> >> -----Original Message-----
> >> From: pgsql-hackers-owner@postgresql.org
> >> [mailto:pgsql-hackers-owner@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 <kaigai@ak.jp.nec.com>