Re: error context for vacuum to include block number
От | Masahiko Sawada |
---|---|
Тема | Re: error context for vacuum to include block number |
Дата | |
Msg-id | CA+fd4k5fvzwxMk2vxd8L40YmhmNoriSCPR5CX3-meva6dvvL8A@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: error context for vacuum to include block number (Justin Pryzby <pryzby@telsasoft.com>) |
Ответы |
Re: error context for vacuum to include block number
(Justin Pryzby <pryzby@telsasoft.com>)
|
Список | pgsql-hackers |
On Sat, 15 Feb 2020 at 00:34, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Fri, Feb 14, 2020 at 12:30:25PM +0900, Masahiko Sawada wrote: > > * I think the function name is too generic. init_vacuum_error_callback > > or init_vacuum_errcallback is better. > > > * The comment of this function is not accurate since this function is > > not only for heap vacuum but also index vacuum. How about just > > "Initialize vacuum error callback"? > > > * I think it's easier to read the code if we set the relname and > > indname in the same order. > > > * The comment I wrote in the previous mail seems better, because in > > this function the reader might get confused that 'rel' is a relation > > or an index depending on the phase but that comment helps it. > > Fixed these > > > * rel->rd_index->indexrelid should be rel->rd_index->indrelid. > > Ack. I think that's been wrong since I first wrote it two weeks ago :( > The error is probably more obvious due to the switch statement you proposed. > > Thanks for continued reviews. Thank you for updating the patch! 1. + /* Setup error traceback support for ereport() */ + init_vacuum_error_callback(&errcallback, &errcbarg, onerel, PROGRESS_VACUUM_PHASE_SCAN_HEAP); + /* + * Setup error traceback support for ereport() + */ + init_vacuum_error_callback(&errcallback, &errcbarg, onerel, PROGRESS_VACUUM_PHASE_VACUUM_HEAP); + /* Setup error traceback support for ereport() */ + init_vacuum_error_callback(&errcallback, &errcbarg, indrel, PROGRESS_VACUUM_PHASE_VACUUM_INDEX); + /* Setup error traceback support for ereport() */ + init_vacuum_error_callback(&errcallback, &errcbarg, indrel, PROGRESS_VACUUM_PHASE_INDEX_CLEANUP); +/* Initialize vacuum error callback */ +static void +init_vacuum_error_callback(ErrorContextCallback *errcallback, vacuum_error_callback_arg *errcbarg, Relation rel, int phase) The above lines need a new line. 2. static void lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats) { int tupindex; int npages; PGRUsage ru0; Buffer vmbuffer = InvalidBuffer; ErrorContextCallback errcallback; vacuum_error_callback_arg errcbarg; /* Report that we are now vacuuming the heap */ pgstat_progress_update_param(PROGRESS_VACUUM_PHASE, PROGRESS_VACUUM_PHASE_VACUUM_HEAP); /* * Setup error traceback support for ereport() */ init_vacuum_error_callback(&errcallback, &errcbarg, onerel, PROGRESS_VACUUM_PHASE_VACUUM_HEAP); error_context_stack = &errcallback; pg_rusage_init(&ru0); npages = 0; : In lazy_vacuum_heap, we set the error context and then call pg_rusage_init whereas lazy_vacuum_index and lazy_cleanup_index does the opposite. And lazy_scan_heap also call pg_rusage first. I think lazy_vacuum_heap should follow them for consistency. That is, we can set error context after pages = 0. 3. We have 2 other phases: PROGRESS_VACUUM_PHASE_TRUNCATE and PROGRESS_VACUUM_PHASE_FINAL_CLEANUP. I think it's better to set the error context in lazy_truncate_heap as well. What do you think? I'm not sure it's worth to set the error context for FINAL_CLENAUP but we should add the case of FINAL_CLENAUP to vacuum_error_callback as no-op or explain it as a comment even if we don't. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Tomas VondraДата:
Сообщение: Re: 1 Status of vertical clustered index - 2 Join using(fk_constraint) suggestion - 3 Status of pgsql's parser autonomization
Следующее
От: maxzorДата:
Сообщение: Re: 1 Status of vertical clustered index - 2 Join using(fk_constraint) suggestion - 3 Status of pgsql's parser autonomization