On Mon, Sep 16, 2019 at 10:08:19AM -0700, Andres Freund wrote: > On 2019-09-14 15:02:36 +0900, Michael Paquier wrote: >> - Switching OID to use pg_strtoint32 causes a failure with initdb. > > Needs to be debugged too. Although I suspect this might just be that you > need to use unsigned variant.
No, that's not it. My last message had a typo as I used the unsigned variant. Anyway, by switching the routines of readfuncs.c to use the new _check wrappers it is easy to see what the actual issue is. And here we go with one example: "FATAL: invalid input syntax for type uint32: "12089 :relkind v" So, the root of the problem is that the optimized conversion routines would complain if the end of the string includes incorrect characters when converting a node from text, which is not something that strtoXX complains about. So our wrappers atooid() and atoui() accept more types of strings in input as they rely on the system's strtol(). And that counts for the failures with UINT and OID. atoi() also is more flexible on that, which explains the failures for INT, as well as atol() for LONG (this shows a failure in the regression tests, not at initdb time though). One may think that this restriction does not actually apply to READ_UINT64_FIELD because the routine involves no other things than queryId. However once I enable -DWRITE_READ_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DCOPY_PARSE_PLAN_TREES in the builds, queryId parsing also complains with the patch. So except if we redesign the node reads we are bound to keep around the wrapper of strtoXX on HEAD called pg_strtouint64() to avoid an incompatibility when parsing the 64-bit query ID. We could keep that isolated in readfuncs.c close to the declarations of atoui() and strtobool() though. This also points out that pg_strtouint64 of HEAD is inconsistent with its signed relatives in the treatment of the input string. The code paths of the patch calling pg_strtouint64_check to parse completion tags (spi.c and pg_stat_statements.c) should complain in those cases as the format of the tags for SELECT and COPY is fixed. In order to unify the parsing interface and put all the conversion routines in a single place, I still think that the patch has value so I would still keep it (with a fix for the queryId parsing of course), but there is much more to it. >> So while I agree with you that a switch should be doable, there is a >> large set of issues to ponder about here, and the patch already does a >> lot, so I really think that we had better do a closer lookup at those >> issues separately, once the basics are in place, and consider them if >> they actually make sense. There is much more than just doing a direct >> switch in this area with the family of ato*() system calls. > > I have no problem applying this separately, but I really don't think > it's wise to apply this before these problems have been debugged. Sure. No problem with that line of reasoning. -- Michael
signature.asc
Description: PGP signature
