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?. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
