Thread: Re: [HACKERS] [COMMITTERS] pgsql: pageinspect: Try to fix some bugs in previous commit.
Re: [HACKERS] [COMMITTERS] pgsql: pageinspect: Try to fix some bugs in previous commit.
From
Robert Haas
Date:
On Fri, Feb 3, 2017 at 7:41 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > I think UInt32GetDatum(metad->hashm_procid) looks fine, the reason > being 'hashm_procid' is defined as 32-bit unsigned integer but then we > may have to define procid as int8 in SQL function. No, you're wrong. The GetDatum you choose macro has to match the SQL type, not the type of the variable that you're passing to it. For example, if you've got an "int" in the code and the SQL type is "int8", you have to use Int64GetDatum, not Int32GetDatum. Otherwise, stuff breaks, because on some systems 64-bit integers are passed by reference, not by value, so the representation that Int32GetDatum produces isn't valid when interpreted by DatumGetInt64 later on. The latter is expecting a pointer, but the former didn't produce one. > Note: I am extremely sorry for wrongly choosing some of the SQL types > in the patch for pageinspect. I think there were few platform specific > things that too should have been addressed by me. Moreover, I feel > being the owner of this project I should have participated in this > discussion a bit earlier but as I was not subscribed to > pgsql-committers list I could not be on time. It might be a good idea to subscribe to pgsql-committers; that way you can follow what's getting committed, whether it is your patch or otherwise. But we also should perhaps have migrated this discussion to pgsql-hackers. Adjusting recipient list. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [COMMITTERS] pgsql: pageinspect: Try to fix some bugs in previous commit.
From
Ashutosh Sharma
Date:
On Fri, Feb 3, 2017 at 8:29 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Feb 3, 2017 at 7:41 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: >> I think UInt32GetDatum(metad->hashm_procid) looks fine, the reason >> being 'hashm_procid' is defined as 32-bit unsigned integer but then we >> may have to define procid as int8 in SQL function. > > No, you're wrong. The GetDatum you choose macro has to match the SQL > type, not the type of the variable that you're passing to it. For > example, if you've got an "int" in the code and the SQL type is > "int8", you have to use Int64GetDatum, not Int32GetDatum. Otherwise, > stuff breaks, because on some systems 64-bit integers are passed by > reference, not by value, so the representation that Int32GetDatum > produces isn't valid when interpreted by DatumGetInt64 later on. The > latter is expecting a pointer, but the former didn't produce one. > Thank you very much for detailed information and explanation. It is really very helpful and understandable. But, As per your explanation, GetDatum we choose need to match the SQL type, not the type of the variable used in code and I do not see any unsigned integer SQL type in PostgreSQL then I am just wondering on why do we have UInt32GetDatum or UInt64GetDatum macros. >> Note: I am extremely sorry for wrongly choosing some of the SQL types >> in the patch for pageinspect. I think there were few platform specific >> things that too should have been addressed by me. Moreover, I feel >> being the owner of this project I should have participated in this >> discussion a bit earlier but as I was not subscribed to >> pgsql-committers list I could not be on time. > > It might be a good idea to subscribe to pgsql-committers; that way you > can follow what's getting committed, whether it is your patch or > otherwise. But we also should perhaps have migrated this discussion > to pgsql-hackers. Adjusting recipient list. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company
Re: [HACKERS] [COMMITTERS] pgsql: pageinspect: Try to fix some bugs in previous commit.
From
Robert Haas
Date:
On Fri, Feb 3, 2017 at 9:14 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > On Fri, Feb 3, 2017 at 8:29 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Fri, Feb 3, 2017 at 7:41 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: >>> I think UInt32GetDatum(metad->hashm_procid) looks fine, the reason >>> being 'hashm_procid' is defined as 32-bit unsigned integer but then we >>> may have to define procid as int8 in SQL function. >> >> No, you're wrong. The GetDatum you choose macro has to match the SQL >> type, not the type of the variable that you're passing to it. For >> example, if you've got an "int" in the code and the SQL type is >> "int8", you have to use Int64GetDatum, not Int32GetDatum. Otherwise, >> stuff breaks, because on some systems 64-bit integers are passed by >> reference, not by value, so the representation that Int32GetDatum >> produces isn't valid when interpreted by DatumGetInt64 later on. The >> latter is expecting a pointer, but the former didn't produce one. >> > > Thank you very much for detailed information and explanation. It is > really very helpful and understandable. But, As per your explanation, > GetDatum we choose need to match the SQL type, not the type of the > variable used in code and I do not see any unsigned integer SQL type > in PostgreSQL then I am just wondering on why do we have > UInt32GetDatum or UInt64GetDatum macros. That's a pretty good question. UInt64GetDatum is used in exactly one place (exec_stmt_getdiag) and there's really no reason why Int64GetDatum wouldn't be more appropriate. UInt32GetDatum is used in a few more places, and some of those are used for hash values which are not exposed at the SQL level so they might be legitimate, but others like the ones in pageinspect look like fuzzy thinking that has only survived because it happens not to break anything. I suppose if we wanted to be really rigorous about this sort of thing, we should change UInt32GetDatum to do something tangibly different from Int32GetDatum, like negate all the bits. Then if somebody picked the wrong macro it would actually fail. I'm not sure that's really the best place to spend our effort, though. The moral of this episode is that it's important to at least get the right width. Currently, getting the wrong signedness doesn't actually break anything. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [COMMITTERS] pgsql: pageinspect: Try to fix some bugs in previous commit.
From
Ashutosh Sharma
Date:
>>>> I think UInt32GetDatum(metad->hashm_procid) looks fine, the reason >>>> being 'hashm_procid' is defined as 32-bit unsigned integer but then we >>>> may have to define procid as int8 in SQL function. >>> >>> No, you're wrong. The GetDatum you choose macro has to match the SQL >>> type, not the type of the variable that you're passing to it. For >>> example, if you've got an "int" in the code and the SQL type is >>> "int8", you have to use Int64GetDatum, not Int32GetDatum. Otherwise, >>> stuff breaks, because on some systems 64-bit integers are passed by >>> reference, not by value, so the representation that Int32GetDatum >>> produces isn't valid when interpreted by DatumGetInt64 later on. The >>> latter is expecting a pointer, but the former didn't produce one. >>> >> >> Thank you very much for detailed information and explanation. It is >> really very helpful and understandable. But, As per your explanation, >> GetDatum we choose need to match the SQL type, not the type of the >> variable used in code and I do not see any unsigned integer SQL type >> in PostgreSQL then I am just wondering on why do we have >> UInt32GetDatum or UInt64GetDatum macros. > > That's a pretty good question. UInt64GetDatum is used in exactly one > place (exec_stmt_getdiag) and there's really no reason why > Int64GetDatum wouldn't be more appropriate. UInt32GetDatum is used in > a few more places, and some of those are used for hash values which > are not exposed at the SQL level so they might be legitimate, but > others like the ones in pageinspect look like fuzzy thinking that has > only survived because it happens not to break anything. I suppose if > we wanted to be really rigorous about this sort of thing, we should > change UInt32GetDatum to do something tangibly different from > Int32GetDatum, like negate all the bits. Then if somebody picked the > wrong macro it would actually fail. I'm not sure that's really the > best place to spend our effort, though. The moral of this episode is > that it's important to at least get the right width. Currently, > getting the wrong signedness doesn't actually break anything. Okay, understood. Thank you very much ! -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com