On Wed, 22 Jan 2020 at 10:17, Justin Pryzby <pry...@telsasoft.com> wrote: > > On Tue, Jan 21, 2020 at 05:54:59PM -0300, Alvaro Herrera wrote: > > > On Tue, Jan 21, 2020 at 03:11:35PM +0900, Masahiko Sawada wrote: > > > > Some of them conflicts with the current HEAD(62c9b52231). Please rebase > > > > them. > > > > > > Sorry, it's due to other vacuum patch in this branch. > > > > > > New patches won't conflict, except for the 0005, so I renamed it for > > > cfbot. > > > If it's deemed to be useful, I'll make a separate branch for the others. > > > > I think you have to have some other patches applied before these, > > because in the context lines for some of the hunks there are > > double-slash comments. > > And I knew that, so (re)tested that the extracted patches would apply, but it > looks like maybe the patch checker is less smart (or more strict) than git, so > it didn't work after all.
Thank you for updating the patches! I'm not sure it's worth to have patches separately but I could apply all patches cleanly. Here is my comments for the code applied all patches: 1. + /* Used by the error callback */ + char *relname; + char *relnamespace; + BlockNumber blkno; + int stage; /* 0: scan heap; 1: vacuum heap; 2: vacuum index */ +} LVRelStats; * The comment should be updated as we use both relname and relnamespace for ereporting. * We can leave both blkno and stage that are used only for error context reporting put both relname and relnamespace to top of LVRelStats. * The 'stage' is missing to support index cleanup. * Maybe we need a comment for 'blkno'. 2. @@ -748,8 +742,31 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, vacrelstats->scanned_pages = 0; vacrelstats->tupcount_pages = 0; vacrelstats->nonempty_pages = 0; + + /* Setup error traceback support for ereport() */ + vacrelstats->relnamespace = get_namespace_name(RelationGetNamespace(onerel)); + vacrelstats->relname = RelationGetRelationName(onerel); + vacrelstats->blkno = InvalidBlockNumber; /* Not known yet */ + vacrelstats->stage = 0; + + errcallback.callback = vacuum_error_callback; + errcallback.arg = (void *) vacrelstats; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; + vacrelstats->latestRemovedXid = InvalidTransactionId; + if (aggressive) + ereport(elevel, + (errmsg("aggressively vacuuming \"%s.%s\"", + vacrelstats->relnamespace, + vacrelstats->relname))); + else + ereport(elevel, + (errmsg("vacuuming \"%s.%s\"", + vacrelstats->relnamespace, + vacrelstats->relname))); * It seems to me strange that only initialization of latestRemovedXid is done after error callback initialization. * Maybe we can initialize relname and relnamespace in heap_vacuum_rel rather than in lazy_scan_heap. BTW variables of vacrelstats are initialized different places: some of them in heap_vacuum_rel and others in lazy_scan_heap. I think we can gather those that can be initialized at that time to heap_vacuum_rel. 3. /* Work on all the indexes, then the heap */ + /* Don't use the errcontext handler outside this function */ + error_context_stack = errcallback.previous; lazy_vacuum_all_indexes(onerel, Irel, indstats, vacrelstats, lps, nindexes); - /* Remove tuples from heap */ lazy_vacuum_heap(onerel, vacrelstats); + error_context_stack = &errcallback; Maybe we can do like: /* Pop the error context stack */ error_context_stack = errcallback.previous; /* Work on all the indexes, then the heap */ lazy_vacuum_all_indexes(onerel, Irel, indstats, vacrelstats, lps, nindexes); /* Remove tuples from heap */ lazy_vacuum_heap(onerel, vacrelstats); /* Push again the error context of heap scan */ error_context_stack = &errcallback; 4. + /* Setup error traceback support for ereport() */ + /* vacrelstats->relnamespace already set */ + /* vacrelstats->relname already set */ These comments can be merged like: /* * Setup error traceback for ereport(). Both relnamespace and * relname are already set. */ 5. + /* Setup error traceback support for ereport() */ + vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(indrel)); + vacrelstats.relname = RelationGetRelationName(indrel); + vacrelstats.blkno = InvalidBlockNumber; /* Not used */ Why do we need to initialize blkno in spite of not using? 6. +/* + * Error context callback for errors occurring during vacuum. + */ +static void +vacuum_error_callback(void *arg) +{ + LVRelStats *cbarg = arg; + + if (cbarg->stage == 0) + errcontext(_("while scanning block %u of relation \"%s.%s\""), + cbarg->blkno, cbarg->relnamespace, cbarg->relname); + else if (cbarg->stage == 1) + errcontext(_("while vacuuming block %u of relation \"%s.%s\""), + cbarg->blkno, cbarg->relnamespace, cbarg->relname); + else if (cbarg->stage == 2) + errcontext(_("while vacuuming relation \"%s.%s\""), + cbarg->relnamespace, cbarg->relname); +} * 'vacrelstats' would be a better name than 'cbarg'. * In index vacuum, how about "while vacuuming index \"%s.%s\""? Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services