Re: Reworks of CustomScan serialization/deserialization

On 14/03/16 02:53, Kouhei Kaigai wrote:
>> -----Original Message-----
>> From: pgsql-hackers-owner(at)postgresql(dot)org
>> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Petr Jelinek
>> Sent: Friday, March 11, 2016 12:27 AM
>> To: Kaigai Kouhei(海外 浩平); pgsql-hackers(at)postgresql(dot)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