Hi,
On 2019-09-14 15:02:36 +0900, Michael Paquier wrote:
> On Fri, Sep 13, 2019 at 06:38:31PM -0700, Andres Freund wrote:
> > On 2019-09-10 12:05:25 +0900, Michael Paquier wrote:
> >> On Mon, Sep 09, 2019 at 05:27:04AM -0700, Andres Freund wrote:
> >> Attached is an updated patch? How does it look? I have left the
> >> parts of readfuncs.c for now as there are more issues behind that than
> >> doing a single switch, short reads are one, long reads a second.
> >
> > Hm? I don't know what you mean by those issues.
>
> I think that we have more issues than it looks. For example:
> - Switching UINT to use pg_strtouint32() causes an incompatibility
> issue compared to atoui().
"An incompatibility" is, uh, vague.
> - Switching INT to use pg_strtoint32() causes a set of warnings as for
> example with AttrNumber:
> 72 | (void) pg_strtoint32(token, &local_node->fldname)
> | ^~~~~~~~~~~~~~~~~~~~~
> | |
> | AttrNumber * {aka short int *}
> And it is not like we should use a cast either, as we could hide real
> issues. Hence it seems to me that we need to have a new routine
> definition for shorter integers and switch more flags to that.
Yea.
> - Switching LONG to use pg_strtoint64() leads to another set of
> issues, particularly one could see an assertion failure related to Agg
> nodes. I am not sure either that we should use int64 here as the size
> can be at least 32b.
That seems pretty clearly something that needs to be debugged before
applying this series. If there's such a failure, it indicates that
there's either a problem in this patchset, or a pre-existing problem in
readfuncs.
> - 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.
> 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 problme applying this separately, but I really don't think
it's wise to apply this before these problems have been debugged.
Greetings,
Andres Freund