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

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