Re: 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.

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