Re: [PROPOSAL] VACUUM Progress Checker. - Mailing list pgsql-hackers

From Rahila Syed
Subject Re: [PROPOSAL] VACUUM Progress Checker.
Date
Msg-id CAH2L28tbnGCzJh12CDKtS5cCQjMy=R9xPk=V5U_m7gRTJ0z50A@mail.gmail.com
Whole thread Raw
In response to Re: [PROPOSAL] VACUUM Progress Checker.  (Robert Haas <robertmhaas@gmail.com>)
Responses Re: [PROPOSAL] VACUUM Progress Checker.  (Robert Haas <robertmhaas@gmail.com>)
List pgsql-hackers
Hello,

Server crash was reported on running  vacuum progress checker view on 32-bit machine.
Please find attached a fix for the same.

Crash was because 32 bit machine considers int8 as being passed by reference while creating the tuple descriptor. At the time of filling the tuple store, the code (heap_fill_tuple) checks this tuple descriptor before inserting the value into the tuple store. It finds the attribute type pass by reference and hence it treats the value as a pointer when it is not and thus it fails at the time of memcpy.

This happens because appropriate conversion function is not employed while storing the value of that particular attribute into the values array before copying it into tuple store.

-                               values[i+3] = UInt32GetDatum(beentry->st_progress_param[i]);
+                               values[i+3] = Int64GetDatum(beentry->st_progress_param[i]);           


Attached patch fixes this.

Thank you,
Rahila Syed

On Wed, Mar 16, 2016 at 11:30 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Mar 16, 2016 at 6:44 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>Sorta.  Committed after renaming what you called heap blocks vacuumed
>>back to heap blocks scanned, adding heap blocks vacuumed, removing the
>>overall progress meter which I don't believe will be anything close to
>>accurate, fixing some stylistic stuff, arranging to update multiple
>>counters automatically where it could otherwise produce confusion,
>>moving the new view near similar ones in the file, reformatting it to
>>follow the style of the rest of the file, exposing the counter
>>#defines via a header file in case extensions want to use them, and
>>overhauling and substantially expanding the documentation
>
> We have following lines,
>
>         /* report that everything is scanned and vacuumed */
>         pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED,
> blkno);
>         pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED,
> blkno);
>
>
> which appear before final vacuum cycle happens for any remaining dead tuples
> which may span few pages if I am not mistaken.
>
> IMO, reporting final count of heap_blks_scanned is correct here, but
> reporting final heap_blks_vacuumed can happen after the final VACUUM cycle
> for more accuracy.

You are quite right.  Good catch.  Fixed that, and applied Vinayak's
patch too, and fixed another mistake I saw while I was at it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachment

pgsql-hackers by date:

Previous
From: Alvaro Herrera
Date:
Subject: Re: WIP: Access method extendability
Next
From: Robert Haas
Date:
Subject: Re: [PROPOSAL] VACUUM Progress Checker.