Re: error context for vacuum to include block number

Masahiko Sawada <masahiko.sawada@2ndquadrant.com>

From: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, Alvaro Herrera <alvherre@2ndquadrant.com>, Andres Freund <andres@anarazel.de>, Michael Paquier <michael@paquier.xyz>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2020-03-29T03:33:57Z
Lists: pgsql-hackers
On Sat, 28 Mar 2020 at 13:23, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Mar 28, 2020 at 7:04 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > On Sat, Mar 28, 2020 at 06:59:10AM +0530, Amit Kapila wrote:
> > > On Sat, Mar 28, 2020 at 6:46 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > >
> > > > On Sat, Mar 28, 2020 at 06:28:38AM +0530, Amit Kapila wrote:
> > > > > > Hm, but I caused a crash *without* adding CHECK_FOR_INTERRUPTS, just
> > > > > > kill+sleep.  The kill() could come from running pg_cancel_backend().  And the
> > > > > > sleep() just encourages a context switch, which can happen at any time.
> > > > >
> > > > > pg_sleep internally uses CHECK_FOR_INTERRUPTS() due to which it would
> > > > > have accepted the signal sent via pg_cancel_backend().  Can you try
> > > > > your scenario by temporarily removing CHECK_FOR_INTERRUPTS from
> > > > > pg_sleep() or maybe better by using OS Sleep call?
> > > >
> > > > Ah, that explains it.  Right, I'm not able to induce a crash with usleep().
> > > >
> > > > Do you want me to resend a patch without that change ?  I feel like continuing
> > > > to trade patches is more likely to introduce new errors or lose someone else's
> > > > changes than to make much progress.  The patch has been through enough
> > > > iterations and it's very easy to miss an issue if I try to eyeball it.
> > >
> > > I can do it but we have to agree on the other two points (a) I still
> > > feel that switching to the truncate phase should be done at the place
> > > from where we are calling lazy_truncate_heap and (b)
> > > lazy_cleanup_index should switch back the error phase after calling
> > > index_vacuum_cleanup.  I have explained my reasoning for these points
> > > a few emails back.
> >
> > I have no objection to either.  It was intuitive to me to do it how I
> > originally wrote it but I'm not wedded to it.
> >
>
> Please find attached the updated patch with all the changes discussed.
> Let me know if I have missed anything?
>

Thank you for updating the patch! Looks good to me.

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Commits

  1. Avoid calls to RelationGetRelationName() and RelationGetNamespace() in

  2. Introduce vacuum errcontext to display additional information.

  3. Fix mesurement of elapsed time during truncating heap in VACUUM.

  4. Allow vacuum command to process indexes in parallel.

  5. Refactor code dedicated to index vacuuming in vacuumlazy.c

  6. Remove duplicated progress reporting during heap scan of VACUUM