On Fri, Mar 20, 2020 at 5:59 AM Justin Pryzby <pry...@telsasoft.com> wrote: > > On Thu, Mar 19, 2020 at 03:29:31PM -0500, Justin Pryzby wrote: > > I was going to suggest that we could do that by passing in a pointer to a > > local > > variable "LVRelStats olderrcbarg", like: > > | update_vacuum_error_cbarg(vacrelstats, > > VACUUM_ERRCB_PHASE_SCAN_HEAP, > > | blkno, NULL, &olderrcbarg); > > > > and then later call: > > |update_vacuum_error_cbarg(vacrelstats, olderrcbarg.phase, > > | olderrcbarg.blkno, > > | olderrcbarg.indname, > > | NULL); > > > > I implemented it in a separate patch, but it may be a bad idea, due to > > freeing > > indname. To exercise it, I tried to cause a crash by changing "else if > > (errcbarg->indname)" to "if" without else, but wasn't able to cause a crash, > > probably just due to having a narrow timing window. > > I realized it was better for the caller to just assign the struct on its own. >
That makes sense. I have a few more comments: 1. + VACUUM_ERRCB_PHASE_INDEX_CLEANUP, +} errcb_phase; Why do you need a comma after the last element in the above enum? 2. + /* Setup error traceback support for ereport() */ + update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_SCAN_HEAP, + InvalidBlockNumber, NULL); + errcallback.callback = vacuum_error_callback; + errcallback.arg = vacrelstats; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; Why do we need to call update_vacuum_error_cbarg at the above place after we have added a new one inside for.. loop? 3. + * free_oldindex is true if the previous "indname" should be freed. It must be + * false if the caller has copied the old LVRelSTats, /LVRelSTats/LVRelStats 4. /* Clear the error traceback phase */ update_vacuum_error_cbarg(vacrelstats, - VACUUM_ERRCB_PHASE_UNKNOWN, InvalidBlockNumber, - NULL); + olderrcbarg.phase, + olderrcbarg.blkno, + olderrcbarg.indname, + true); At this and similar places, change the comment to something like: "Reset the old phase information for error traceback". 5. Subject: [PATCH v28 3/5] Drop reltuples --- src/backend/access/heap/vacuumlazy.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) Is this patch directly related to the main patch (vacuum errcontext to show block being processed) or is it an independent improvement of code? 6. [PATCH v28 4/5] add callback for truncation + VACUUM_ERRCB_PHASE_TRUNCATE, + VACUUM_ERRCB_PHASE_TRUNCATE_PREFETCH, Do we really need separate phases for truncate and truncate_prefetch? We have only one phase for a progress update, similarly, I think having one phase for error reporting should be sufficient. It will also reduce the number of places where we need to call update_vacuum_error_cbarg. I think we can set VACUUM_ERRCB_PHASE_TRUNCATE before count_nondeletable_pages and reset it at the place you are doing right now in the patch. 7. Is there a reason to keep the truncate phase patch separate from the main patch? If not, let's merge them. 8. Can we think of some easy way to add tests for this patch? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
