> 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