Обсуждение: [COMMITTERS] pgsql: pageinspect: Try to fix some bugs in previous commit.
pageinspect: Try to fix some bugs in previous commit. Commit 08bf6e529587e1e9075d013d859af2649c32a511 seems not to have used the correct *GetDatum and PG_GETARG_* macros for the SQL types in some cases, and some of the SQL types seem to have been poorly chosen, too. Try to fix it. I'm not sure if this is the reason why the buildfarm is currently unhappy with this code, but it seems like a good place to start. Buildfarm unhappiness reported by Tom Lane. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/ed807fda6d5102537daa5d725e716555cbc49f44 Modified Files -------------- contrib/pageinspect/hashfuncs.c | 28 +++++++++++++-------------- contrib/pageinspect/pageinspect--1.5--1.6.sql | 10 +++++----- 2 files changed, 19 insertions(+), 19 deletions(-)
Robert Haas <rhaas@postgresql.org> writes:
> pageinspect: Try to fix some bugs in previous commit.
This is a first step, but there's more :-(. Poking at it now on
dromedary.
regards, tom lane
On Thu, Feb 2, 2017 at 10:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <rhaas@postgresql.org> writes: >> pageinspect: Try to fix some bugs in previous commit. > > This is a first step, but there's more :-(. Poking at it now on > dromedary. Ugh, yes. Sorry, I should have checked this more carefully before commit. I mentioned the problem in a previous review and failed to notice that it hadn't been fixed. Are you taking care of it at this point or should I keep at it? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> Ugh, yes. Sorry, I should have checked this more carefully before
> commit. I mentioned the problem in a previous review and failed to
> notice that it hadn't been fixed. Are you taking care of it at this
> point or should I keep at it?
I'm about to push a fix that removes the crashes (or at least the ones
I see on dromedary), but there is still a problem, which is that the
expected output seems inherently platform-dependent:
*** /Users/tgl/pgsql/contrib/pageinspect/expected/hash.out Thu Feb 2 21:30:54 2017
--- /Users/tgl/pgsql/contrib/pageinspect/results/hash.out Thu Feb 2 22:55:43 2017
***************
*** 52,58 ****
magic | 105121344
version | 2
ntuples | 1
! ffactor | 307
bsize | 8152
bmsize | 4096
bmshift | 15
--- 52,58 ----
magic | 105121344
version | 2
ntuples | 1
! ffactor | 384
bsize | 8152
bmsize | 4096
bmshift | 15
***************
*** 107,113 ****
live_items | 1
dead_items | 0
page_size | 8192
! free_size | 8128
hasho_prevblkno | 4294967295
hasho_nextblkno | 4294967295
hasho_bucket | 2
--- 107,113 ----
live_items | 1
dead_items | 0
page_size | 8192
! free_size | 8132
hasho_prevblkno | 4294967295
hasho_nextblkno | 4294967295
hasho_bucket | 2
======================================================================
I think probably both of those are unavoidable 32-bit v 64-bit
differences due to available space on a page changing with MAXALIGN.
What do you want to do about those?
regards, tom lane
On Thu, Feb 2, 2017 at 11:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> Ugh, yes. Sorry, I should have checked this more carefully before >> commit. I mentioned the problem in a previous review and failed to >> notice that it hadn't been fixed. Are you taking care of it at this >> point or should I keep at it? > > I'm about to push a fix that removes the crashes (or at least the ones > I see on dromedary), For comparison, a patch I wrote by inspection is attached. > but there is still a problem, which is that the > expected output seems inherently platform-dependent: > > I think probably both of those are unavoidable 32-bit v 64-bit > differences due to available space on a page changing with MAXALIGN. > What do you want to do about those? How about we have the test just select a named list of fields instead of selecting *? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Feb 2, 2017 at 11:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm about to push a fix that removes the crashes (or at least the ones
>> I see on dromedary),
> For comparison, a patch I wrote by inspection is attached.
Hm, some of what you have here matches what I just pushed, but not all.
I just made the C code agree with what the SQL declarations for the
functions say. I'm pretty dubious that the SQL declarations are entirely
sensible as to which values need to be of what width, but I'll leave that
decision for somebody who's been paying closer attention to the hash code.
>> I think probably both of those are unavoidable 32-bit v 64-bit
>> differences due to available space on a page changing with MAXALIGN.
>> What do you want to do about those?
> How about we have the test just select a named list of fields instead
> of selecting *?
Yeah, that's one possible answer. We could also maintain two
expected-files for 32 bit v 64 bit, but I'm not sure it's worth
the trouble.
regards, tom lane
On Fri, Feb 3, 2017 at 9:46 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Feb 2, 2017 at 11:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I'm about to push a fix that removes the crashes (or at least the ones >>> I see on dromedary), > >> For comparison, a patch I wrote by inspection is attached. > > Hm, some of what you have here matches what I just pushed, but not all. > > I just made the C code agree with what the SQL declarations for the > functions say. I'm pretty dubious that the SQL declarations are entirely > sensible as to which values need to be of what width, but I'll leave that > decision for somebody who's been paying closer attention to the hash code. > I have gone through all the of the SQL declarations and it seems hash_metapage_info(...,OUT procid int4,..) is not consistent. procid is unsigned int, so isn't it better to use the corresponding datatype as int8 in SQL function hash_metapage_info then use Int64GetDatum? The other inconsistency in the code appears to be in the usage of UInt64GetDatum and Int64GetDatum for same SQL datatype. For ex. SQL declaration of hasho_prevblkno is int8 (hash_page_stats(.. , OUT hasho_prevblkno int8,..)) and we use Int64GetDatum to fill the corresponding C value. However for SQL declaration of maxbucket is int8 (hash_metapage_info(..,OUT maxbucket int8,)) and we use UInt64GetDatum() to fetch the value. I think it is better to be consistent here. >>> I think probably both of those are unavoidable 32-bit v 64-bit >>> differences due to available space on a page changing with MAXALIGN. >>> What do you want to do about those? > >> How about we have the test just select a named list of fields instead >> of selecting *? > > Yeah, that's one possible answer. We could also maintain two > expected-files for 32 bit v 64 bit, but I'm not sure it's worth > the trouble. > I think for now selecting named fields is sufficient. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [COMMITTERS] pgsql: pageinspect: Try to fix some bugs in previous commit.
От
Ashutosh Sharma
Дата:
On Fri, Feb 3, 2017 at 4:36 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Fri, Feb 3, 2017 at 9:46 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> On Thu, Feb 2, 2017 at 11:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> I'm about to push a fix that removes the crashes (or at least the ones >>>> I see on dromedary), >> >>> For comparison, a patch I wrote by inspection is attached. >> >> Hm, some of what you have here matches what I just pushed, but not all. >> >> I just made the C code agree with what the SQL declarations for the >> functions say. I'm pretty dubious that the SQL declarations are entirely >> sensible as to which values need to be of what width, but I'll leave that >> decision for somebody who's been paying closer attention to the hash code. >> 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. > > I have gone through all the of the SQL declarations and it seems > hash_metapage_info(...,OUT procid int4,..) is not consistent. procid > is unsigned int, so isn't it better to use the corresponding datatype > as int8 in SQL function hash_metapage_info then use Int64GetDatum? > > The other inconsistency in the code appears to be in the usage of > UInt64GetDatum and Int64GetDatum for same SQL datatype. For ex. SQL > declaration of hasho_prevblkno is int8 (hash_page_stats(.. , OUT > hasho_prevblkno int8,..)) and we use Int64GetDatum to fill the > corresponding C value. However for SQL declaration of maxbucket is > int8 (hash_metapage_info(..,OUT maxbucket int8,)) and we use > UInt64GetDatum() to fetch the value. I think it is better to be > consistent here. I am sorry but I am not quite able to understand the purpose of typecasting unsigned integer to signed type at few places and then using corresponding macros to represent it as Datum. I mean at few places we have done typecasting of unsigned inetgers to signed type whereas at some places we have kept it as it is. > >>>> I think probably both of those are unavoidable 32-bit v 64-bit >>>> differences due to available space on a page changing with MAXALIGN. >>>> What do you want to do about those? >> >>> How about we have the test just select a named list of fields instead >>> of selecting *? >> >> Yeah, that's one possible answer. We could also maintain two >> expected-files for 32 bit v 64 bit, but I'm not sure it's worth >> the trouble. >> > > I think for now selecting named fields is sufficient. +1. Attached is the patch that has this changes. 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.
Вложения
Amit Kapila <amit.kapila16@gmail.com> writes:
> I have gone through all the of the SQL declarations and it seems
> hash_metapage_info(...,OUT procid int4,..) is not consistent. procid
> is unsigned int, so isn't it better to use the corresponding datatype
> as int8 in SQL function hash_metapage_info then use Int64GetDatum?
Isn't procid an OID? I'd use OID or maybe even regprocedure on
the SQL side.
regards, tom lane
On Thu, Feb 2, 2017 at 11:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Feb 2, 2017 at 11:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I'm about to push a fix that removes the crashes (or at least the ones >>> I see on dromedary), > >> For comparison, a patch I wrote by inspection is attached. > > Hm, some of what you have here matches what I just pushed, but not all. > > I just made the C code agree with what the SQL declarations for the > functions say. Doesn't look like it to me. You changed a bunch of places that say UInt32GetDatum to UInt64GetDatum, but the SQL type certainly isn't unsigned. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Feb 3, 2017 at 8:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Amit Kapila <amit.kapila16@gmail.com> writes: >> I have gone through all the of the SQL declarations and it seems >> hash_metapage_info(...,OUT procid int4,..) is not consistent. procid >> is unsigned int, so isn't it better to use the corresponding datatype >> as int8 in SQL function hash_metapage_info then use Int64GetDatum? > > Isn't procid an OID? > It is RegProcedure. > I'd use OID or maybe even regprocedure on > the SQL side. > I think we can use either one of those. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Feb 2, 2017 at 11:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I just made the C code agree with what the SQL declarations for the
>> functions say.
> Doesn't look like it to me. You changed a bunch of places that say
> UInt32GetDatum to UInt64GetDatum, but the SQL type certainly isn't
> unsigned.
The machines don't care about that. They do care about the width of
the datum. Particularly on 32-bit hardware, where one width is
pass-by-val and the other isn't. (Also, if your point is that you
wish we had a uint64 SQL type, I doubt we're going there.)
What needs to be resolved to decide if any of this is actually sane is to
figure out which of these values need to be int64 on the SQL side because
(a) they could practically exceed the range of signed int32 and (b) it
would bother us to show such values as negative rather than large
positive. I suspect that not all the things currently declared as int64
really need to be. I also remain unhappy that we can't manage to be
consistent about what a BlockNumber parameter is represented as.
regards, tom lane