Thread: error context for vacuum to include block number
On Wed, Aug 07, 2019 at 04:51:54PM -0700, Andres Freund wrote: https://www.postgresql.org/message-id/20190807235154.erbmr4o4bo6vgnjv%40alap3.anarazel.de | Ugh :( | | We really need to add a error context to vacuumlazy that shows which | block is being processed. I eeked out a minimal patch. I renamed "StringInfoData buf", since it wasn't nice to mask it by "Buffer buf". postgres=# SET statement_timeout=99;vacuum t; SET 2019-11-20 14:52:49.521 CST [6319] ERROR: canceling statement due to statement timeout 2019-11-20 14:52:49.521 CST [6319] CONTEXT: block 596 2019-11-20 14:52:49.521 CST [6319] STATEMENT: vacuum t; Justin
Attachment
Find attached updated patch: . Use structure to include relation name. . Split into a separate patch rename of "StringInfoData buf". 2019-11-27 20:04:53.640 CST [14244] ERROR: canceling statement due to statement timeout 2019-11-27 20:04:53.640 CST [14244] CONTEXT: block 2314 of relation t 2019-11-27 20:04:53.640 CST [14244] STATEMENT: vacuum t; I tried to use BufferGetTag() to avoid using a 2ndary structure, but fails if the buffer is not pinned.
Attachment
On Fri, Dec 06, 2019 at 10:23:25AM -0600, Justin Pryzby wrote: > Find attached updated patch: > . Use structure to include relation name. > . Split into a separate patch rename of "StringInfoData buf". > > 2019-11-27 20:04:53.640 CST [14244] ERROR: canceling statement due to statement timeout > 2019-11-27 20:04:53.640 CST [14244] CONTEXT: block 2314 of relation t > 2019-11-27 20:04:53.640 CST [14244] STATEMENT: vacuum t; > > I tried to use BufferGetTag() to avoid using a 2ndary structure, but fails if > the buffer is not pinned. No problem from me to add more context directly in lazy_scan_heap(). + // errcallback.arg = (void *) &buf; The first patch is full of that, please make sure to clean it up. Let's keep also the message simple, still I think that it should be a bit more explicative: 1) Let's the schema name, and quote the relation name. 2) Let's mention the scanning (or vacuuming) operation. So I would suggest the following instead: "while scanning block %u of relation \"%s.%s\"" -- Michael
Attachment
On Wed, Dec 11, 2019 at 09:15:07PM +0900, Michael Paquier wrote: > On Fri, Dec 06, 2019 at 10:23:25AM -0600, Justin Pryzby wrote: > > Find attached updated patch: > > . Use structure to include relation name. > > . Split into a separate patch rename of "StringInfoData buf". > > > > 2019-11-27 20:04:53.640 CST [14244] ERROR: canceling statement due to statement timeout > > 2019-11-27 20:04:53.640 CST [14244] CONTEXT: block 2314 of relation t > > 2019-11-27 20:04:53.640 CST [14244] STATEMENT: vacuum t; > > > > I tried to use BufferGetTag() to avoid using a 2ndary structure, but fails if > > the buffer is not pinned. > > No problem from me to add more context directly in lazy_scan_heap(). Do you mean without a callback ? I think that's necessary, since the IO errors would happen within ReadBufferExtended, but we don't want to polute that with errcontext. And cannot call errcontext on its own: FATAL: errstart was not called > So I would suggest the following instead: > "while scanning block %u of relation \"%s.%s\"" Done in the attached.
Attachment
On 2019-Dec-11, Justin Pryzby wrote:
> @@ -635,6 +644,15 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
> else
> skipping_blocks = false;
>
> + /* Setup error traceback support for ereport() */
> + errcallback.callback = vacuum_error_callback;
> + cbarg.relname = relname;
> + cbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
> + cbarg.blkno = 0; /* Not known yet */
Shouldn't you use InvalidBlockNumber for this initialization?
> @@ -658,6 +676,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
>
> pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
>
> + cbarg.blkno = blkno;
I would put this before pgstat_progress_update_param, just out of
paranoia.
> @@ -817,7 +837,6 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
>
> buf = ReadBufferExtended(onerel, MAIN_FORKNUM, blkno,
> RBM_NORMAL, vac_strategy);
> -
> /* We need buffer cleanup lock so that we can prune HOT chains. */
> if (!ConditionalLockBufferForCleanup(buf))
> {
Lose this hunk?
> @@ -2354,3 +2376,15 @@ heap_page_is_all_visible(Relation rel, Buffer buf,
>
> return all_visible;
> }
> +
> +/*
> + * Error context callback for errors occurring during vacuum.
> + */
> +static void
> +vacuum_error_callback(void *arg)
> +{
> + vacuum_error_callback_arg *cbarg = arg;
> +
> + errcontext("while scanning block %u of relation \"%s.%s\"",
> + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> +}
I would put this function around line 1512 (just after lazy_scan_heap)
rather than at bottom of file. (And move its prototype accordingly, to
line 156.) Or do you intend that this is going to be used for
lazy_vacuum_heap too? Maybe it should.
Patch looks good to me otherwise.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
Thanks for working on this!
On 2019-12-11 08:36:48 -0600, Justin Pryzby wrote:
> On Wed, Dec 11, 2019 at 09:15:07PM +0900, Michael Paquier wrote:
> > On Fri, Dec 06, 2019 at 10:23:25AM -0600, Justin Pryzby wrote:
> > > Find attached updated patch:
> > > . Use structure to include relation name.
> > > . Split into a separate patch rename of "StringInfoData buf".
> > >
> > > 2019-11-27 20:04:53.640 CST [14244] ERROR: canceling statement due to statement timeout
> > > 2019-11-27 20:04:53.640 CST [14244] CONTEXT: block 2314 of relation t
> > > 2019-11-27 20:04:53.640 CST [14244] STATEMENT: vacuum t;
> > >
> > > I tried to use BufferGetTag() to avoid using a 2ndary structure, but fails if
> > > the buffer is not pinned.
The tag will not add all that informative details, because the
relfilenode isn't easily mappable to the table name or such.
> diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
> index 043ebb4..9376989 100644
> --- a/src/backend/access/heap/vacuumlazy.c
> +++ b/src/backend/access/heap/vacuumlazy.c
> @@ -138,6 +138,12 @@ typedef struct LVRelStats
> bool lock_waiter_detected;
> } LVRelStats;
>
> +typedef struct
> +{
> + char *relname;
> + char *relnamespace;
> + BlockNumber blkno;
> +} vacuum_error_callback_arg;
Hm, wonder if could be worthwhile to not use a separate struct here, but
instead extend one of the existing structs to contain the necessary
information. Or perhaps have one new struct with all the necessary
information. There's already quite a few places that do
get_namespace_name(), for example.
> @@ -524,6 +531,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
> PROGRESS_VACUUM_MAX_DEAD_TUPLES
> };
> int64 initprog_val[3];
> + ErrorContextCallback errcallback;
> + vacuum_error_callback_arg cbarg;
Not a fan of "cbarg", too generic.
> pg_rusage_init(&ru0);
>
> @@ -635,6 +644,15 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
> else
> skipping_blocks = false;
>
> + /* Setup error traceback support for ereport() */
> + errcallback.callback = vacuum_error_callback;
> + cbarg.relname = relname;
> + cbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
> + cbarg.blkno = 0; /* Not known yet */
> + errcallback.arg = (void *) &cbarg;
> + errcallback.previous = error_context_stack;
> + error_context_stack = &errcallback;
> +
> for (blkno = 0; blkno < nblocks; blkno++)
> {
> Buffer buf;
> @@ -658,6 +676,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
>
> pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
>
> + cbarg.blkno = blkno;
> +
> if (blkno == next_unskippable_block)
> {
> /* Time to advance next_unskippable_block */
> @@ -817,7 +837,6 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
>
> buf = ReadBufferExtended(onerel, MAIN_FORKNUM, blkno,
> RBM_NORMAL, vac_strategy);
> -
> /* We need buffer cleanup lock so that we can prune HOT chains. */
> if (!ConditionalLockBufferForCleanup(buf))
> {
> @@ -1388,6 +1407,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
> RecordPageWithFreeSpace(onerel, blkno, freespace);
> }
>
> + /* Pop the error context stack */
> + error_context_stack = errcallback.previous;
> +
> /* report that everything is scanned and vacuumed */
> pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
>
> @@ -2354,3 +2376,15 @@ heap_page_is_all_visible(Relation rel, Buffer buf,
>
> return all_visible;
> }
I think this will misattribute errors that happen when in the:
/*
* If we are close to overrunning the available space for dead-tuple
* TIDs, pause and do a cycle of vacuuming before we tackle this page.
*/
section of lazy_scan_heap(). That will
a) scan the index, during which we presumably don't want the same error
context, as it'd be quite misleading: The block that was just scanned
in the loop isn't actually likely to be the culprit for an index
problem. And we'd not mention the fact that the problem is occurring
in the index.
b) will report the wrong block, when in lazy_vacuum_heap().
Greetings,
Andres Freund
On Wed, Dec 11, 2019 at 12:33:53PM -0300, Alvaro Herrera wrote:
> On 2019-Dec-11, Justin Pryzby wrote:
> > + cbarg.blkno = 0; /* Not known yet */
> Shouldn't you use InvalidBlockNumber for this initialization?
..
> > pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
> > + cbarg.blkno = blkno;
> I would put this before pgstat_progress_update_param, just out of
> paranoia.
..
> Lose this hunk?
Addressed those.
> Or do you intend that this is going to be used for lazy_vacuum_heap too?
> Maybe it should.
Done in a separate patch.
On Wed, Dec 11, 2019 at 08:54:25AM -0800, Andres Freund wrote:
> Hm, wonder if could be worthwhile to not use a separate struct here, but
> instead extend one of the existing structs to contain the necessary
> information. Or perhaps have one new struct with all the necessary
> information. There's already quite a few places that do
> get_namespace_name(), for example.
Didn't find a better struct to use yet.
> > + vacuum_error_callback_arg cbarg;
> Not a fan of "cbarg", too generic.
..
> I think this will misattribute errors that happen when in the:
Probably right. Attached should address it.
On Wed, Dec 11, 2019 at 08:54:25AM -0800, Andres Freund wrote:
> > +typedef struct
> > +{
> > + char *relname;
> > + char *relnamespace;
> > + BlockNumber blkno;
> > +} vacuum_error_callback_arg;
>
> Hm, wonder if could be worthwhile to not use a separate struct here, but
> instead extend one of the existing structs to contain the necessary
> information. Or perhaps have one new struct with all the necessary
> information. There's already quite a few places that do
> get_namespace_name(), for example.
> Not a fan of "cbarg", too generic.
> I think this will misattribute errors that happen when in the:
I think that's addressed after deduplicating in attached.
Deduplication revealed 2nd progress call, which seems to have been included
redundantly at c16dc1aca.
- /* Remove tuples from heap */
- pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
- PROGRESS_VACUUM_PHASE_VACUUM_HEAP);
Justin
Attachment
On Thu, Dec 12, 2019 at 09:08:31PM -0600, Justin Pryzby wrote:
> On Wed, Dec 11, 2019 at 08:54:25AM -0800, Andres Freund wrote:
>> Hm, wonder if could be worthwhile to not use a separate struct here, but
>> instead extend one of the existing structs to contain the necessary
>> information. Or perhaps have one new struct with all the necessary
>> information. There's already quite a few places that do
>> get_namespace_name(), for example.
>
> Didn't find a better struct to use yet.
Yes, I am too wondering what Andres has in mind here. It is not like
you can use VacuumRelation as the OID of the relation may not have
been stored.
> On Wed, Dec 11, 2019 at 08:54:25AM -0800, Andres Freund wrote:>
> I think that's addressed after deduplicating in attached.
>
> Deduplication revealed 2nd progress call, which seems to have been included
> redundantly at c16dc1aca.
>
> - /* Remove tuples from heap */
> - pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
> - PROGRESS_VACUUM_PHASE_VACUUM_HEAP);
What is the purpose of 0001 in the context of this thread? One could
say the same about 0002 and 0003. Anyway, you are right with 0002 as
the progress value for PROGRESS_VACUUM_PHASE gets updated twice in a
row with the same value. So let's clean up that.
The refactoring in 0003 is interesting, so I would be tempted to merge
it. Now you have one small issue in it:
- /*
- * Forget the now-vacuumed tuples, and press on, but be careful
- * not to reset latestRemovedXid since we want that value to be
- * valid.
- */
+ lazy_vacuum_heap_index(onerel, vacrelstats, Irel, nindexes, indstats);
vacrelstats->num_dead_tuples = 0;
- vacrelstats->num_index_scans++;
You are moving this comment within lazy_vacuum_heap_index, but it
applies to num_dead_tuples and not num_index_scans, no? You should
keep the incrementation of num_index_scans within the routine though.
--
Michael
Attachment
On Fri, Dec 13, 2019 at 10:28:50PM +0900, Michael Paquier wrote: >> v4-0001-Rename-buf-to-avoid-shadowing-buf-of-another-type.patch >> v4-0002-Remove-redundant-call-to-vacuum-progress.patch >> v4-0003-deduplication.patch >> v4-0004-vacuum-errcontext-to-show-block-being-processed.patch >> v4-0005-add-errcontext-callback-in-lazy_vacuum_heap-too.patch > What is the purpose of 0001 in the context of this thread? One could > say the same about 0002 and 0003. Anyway, you are right with 0002 as > the progress value for PROGRESS_VACUUM_PHASE gets updated twice in a > row with the same value. So let's clean up that. It's related code which I cleaned up before adding new stuff. Not essential, thus separate (0002 should be backpatched). > The refactoring in 0003 is interesting, so I would be tempted to merge > it. Now you have one small issue in it: > - /* > - * Forget the now-vacuumed tuples, and press on, but be careful > - * not to reset latestRemovedXid since we want that value to be > - * valid. > - */ > + lazy_vacuum_heap_index(onerel, vacrelstats, Irel, nindexes, indstats); > vacrelstats->num_dead_tuples = 0; > - vacrelstats->num_index_scans++; > You are moving this comment within lazy_vacuum_heap_index, but it > applies to num_dead_tuples and not num_index_scans, no? You should > keep the incrementation of num_index_scans within the routine though. Thank you, fixed. -- Justin Pryzby System Administrator Telsasoft +1-952-707-8581
Attachment
On Fri, Dec 13, 2019 at 04:47:35PM -0600, Justin Pryzby wrote: > It's related code which I cleaned up before adding new stuff. Not essential, > thus separate (0002 should be backpatched). The issue just causes some extra work and that's not a bug, so applied without a backpatch. >> The refactoring in 0003 is interesting, so I would be tempted to merge >> it. Now you have one small issue in it: >> - /* >> - * Forget the now-vacuumed tuples, and press on, but be careful >> - * not to reset latestRemovedXid since we want that value to be >> - * valid. >> - */ >> + lazy_vacuum_heap_index(onerel, vacrelstats, Irel, nindexes, indstats); >> vacrelstats->num_dead_tuples = 0; >> - vacrelstats->num_index_scans++; >> You are moving this comment within lazy_vacuum_heap_index, but it >> applies to num_dead_tuples and not num_index_scans, no? You should >> keep the incrementation of num_index_scans within the routine though. > > Thank you, fixed. For 0003, I think that lazy_vacuum_heap_index() can be confusing as those indexes are unrelated to heap. Why not naming it just lazy_vacuum_all_indexes()? The routine should also have a header describing it. -- Michael
Attachment
On Sun, Dec 15, 2019 at 10:07:08PM +0900, Michael Paquier wrote:
> On Fri, Dec 13, 2019 at 04:47:35PM -0600, Justin Pryzby wrote:
> > It's related code which I cleaned up before adding new stuff. Not essential,
> > thus separate (0002 should be backpatched).
>
> The issue just causes some extra work and that's not a bug, so applied
> without a backpatch.
Thanks
> For 0003, I think that lazy_vacuum_heap_index() can be confusing as
> those indexes are unrelated to heap. Why not naming it just
> lazy_vacuum_all_indexes()? The routine should also have a header
> describing it.
I named it so because it calls both lazy_vacuum_index
("PROGRESS_VACUUM_PHASE_VACUUM_INDEX") and
lazy_vacuum_heap("PROGRESS_VACUUM_PHASE_VACUUM_HEAP")
I suppose you don't think the other way around is better?
lazy_vacuum_index_heap
Justin
On Sun, Dec 15, 2019 at 10:27:12AM -0600, Justin Pryzby wrote:
> I named it so because it calls both lazy_vacuum_index
> ("PROGRESS_VACUUM_PHASE_VACUUM_INDEX") and
> lazy_vacuum_heap("PROGRESS_VACUUM_PHASE_VACUUM_HEAP")
>
> I suppose you don't think the other way around is better?
> lazy_vacuum_index_heap
Dunno. Let's see if others have other thoughts on the matter. FWIW,
I have a long history at naming things in a way others don't like :)
--
Michael
Attachment
At Mon, 16 Dec 2019 11:49:56 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Sun, Dec 15, 2019 at 10:27:12AM -0600, Justin Pryzby wrote:
> > I named it so because it calls both lazy_vacuum_index
> > ("PROGRESS_VACUUM_PHASE_VACUUM_INDEX") and
> > lazy_vacuum_heap("PROGRESS_VACUUM_PHASE_VACUUM_HEAP")
> >
> > I suppose you don't think the other way around is better?
> > lazy_vacuum_index_heap
>
> Dunno. Let's see if others have other thoughts on the matter. FWIW,
> I have a long history at naming things in a way others don't like :)
lazy_vacuum_heap_index() seems confusing to me. I read the name as
Michael did before looking the above explanation.
lazy_vacuum_heap_and_index() is clearer to me.
lazy_vacuum_heap_with_index() could also work but I'm not sure it's
further better.
I see some function names like that, and some others that have two
verbs bonded by "_and_".
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Mon, Dec 16, 2019 at 11:49:56AM +0900, Michael Paquier wrote:
> On Sun, Dec 15, 2019 at 10:27:12AM -0600, Justin Pryzby wrote:
> > I named it so because it calls both lazy_vacuum_index
> > ("PROGRESS_VACUUM_PHASE_VACUUM_INDEX") and
> > lazy_vacuum_heap("PROGRESS_VACUUM_PHASE_VACUUM_HEAP")
> >
> > I suppose you don't think the other way around is better?
> > lazy_vacuum_index_heap
>
> Dunno. Let's see if others have other thoughts on the matter. FWIW,
> I have a long history at naming things in a way others don't like :)
I renamed.
And deduplicated two more hunks into a 2nd function.
(I'm also including the changes I mentioned here ... in case anyone cares to
comment or review).
https://www.postgresql.org/message-id/20191220171132.GB30414%40telsasoft.com
Attachment
- v6-0001-Rename-buf-to-avoid-shadowing-buf-of-another-type.patch
- v6-0002-deduplication.patch
- v6-0003-dedup2-skip_blocks.patch
- v6-0004-vacuum-errcontext-to-show-block-being-processed.patch
- v6-0005-add-errcontext-callback-in-lazy_vacuum_heap-too.patch
- v6-0006-Print-debug-line-before-starting-each-vacuum-step.patch
On Mon, Dec 23, 2019 at 07:24:28PM -0600, Justin Pryzby wrote: > I renamed. Hmm. I have found what was partially itching me for patch 0002, and that's actually the fact that we don't do the error reporting for heap within lazy_vacuum_heap() because the code relies too much on updating two progress parameters at the same time, on top of the fact that you are mixing multiple concepts with this refactoring. One problem is that if this code is refactored in the future, future callers of lazy_vacuum_heap() would miss the update of the progress reporting. Splitting things improves also the readability of the code, so attached is the refactoring I would do for this portion of the set. It is also more natural to increment num_index_scans when the reporting happens on consistency grounds. (Please note that I have not indented yet the patch.) -- Michael
Attachment
On Tue, Dec 24, 2019 at 01:19:09PM +0900, Michael Paquier wrote: > (Please note that I have not indented yet the patch.) And one indentation later, committed this one after an extra lookup as of 1ab41a3. -- Michael
Attachment
On Tue, Dec 24, 2019 at 01:19:09PM +0900, Michael Paquier wrote: > On Mon, Dec 23, 2019 at 07:24:28PM -0600, Justin Pryzby wrote: > > I renamed. > > Hmm. I have found what was partially itching me for patch 0002, and > that's actually the fact that we don't do the error reporting for heap > within lazy_vacuum_heap() because the code relies too much on updating > two progress parameters at the same time, on top of the fact that you > are mixing multiple concepts with this refactoring. One problem is > that if this code is refactored in the future, future callers of > lazy_vacuum_heap() would miss the update of the progress reporting. > Splitting things improves also the readability of the code, so > attached is the refactoring I would do for this portion of the set. > It is also more natural to increment num_index_scans when the I agree that's better. I don't see any reason why the progress params need to be updated atomically. So rebasified against your patch.
Attachment
- v7-0001-Rename-buf-to-avoid-shadowing-buf-of-another-type.patch
- v7-0002-michael-dedup.patch
- v7-0003-dedup2-skip_blocks.patch
- v7-0004-vacuum-errcontext-to-show-block-being-processed.patch
- v7-0005-add-errcontext-callback-in-lazy_vacuum_heap-too.patch
- v7-0006-Print-debug-line-before-starting-each-vacuum-step.patch
On Thu, Dec 26, 2019 at 10:57 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > I agree that's better. > I don't see any reason why the progress params need to be updated atomically. > So rebasified against your patch. I am not sure whether it's important enough to make a stink about, but it bothers me a bit that this is being dismissed as unimportant. The problem is that, if the updates are not atomic, then somebody might see the data after one has been updated and the other has not yet been updated. The result is that when the phase is PROGRESS_VACUUM_PHASE_VACUUM_INDEX, someone reading the information can't tell whether the number of index scans reported is the number *previously* performed or the number performed including the one that just finished. The race to see the latter state is narrow, so it probably wouldn't come up often, but it does seem like it would be confusing if it did happen. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: error context for vacuum to include block number (atomicprogress update)
From
Justin Pryzby
Date:
On Sat, Dec 28, 2019 at 07:21:31PM -0500, Robert Haas wrote: > On Thu, Dec 26, 2019 at 10:57 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > I agree that's better. > > I don't see any reason why the progress params need to be updated atomically. > > So rebasified against your patch. > > I am not sure whether it's important enough to make a stink about, but > it bothers me a bit that this is being dismissed as unimportant. The > problem is that, if the updates are not atomic, then somebody might > see the data after one has been updated and the other has not yet been > updated. The result is that when the phase is > PROGRESS_VACUUM_PHASE_VACUUM_INDEX, someone reading the information > can't tell whether the number of index scans reported is the number > *previously* performed or the number performed including the one that > just finished. The race to see the latter state is narrow, so it > probably wouldn't come up often, but it does seem like it would be > confusing if it did happen. What used to be atomic was this: - hvp_val[0] = PROGRESS_VACUUM_PHASE_VACUUM_HEAP; - hvp_val[1] = vacrelstats->num_index_scans + 1; => switch from PROGRESS_VACUUM_PHASE_VACUUM INDEX to HEAP and increment index_vacuum_count, which is documented as the "Number of completed index vacuum cycles." Now, it 1) increments the number of completed scans; and, 2) then progresses phase to HEAP, so there's a window where the number of completed scans is incremented, and it still says VACUUM_INDEX. Previously, if it said VACUUM_INDEX, one could assume that index_vacuum_count would increase at least once more, and that's no longer true. If someone sees VACUUM_INDEX and some NUM_INDEX_VACUUMS, and then later sees VACUUM_HEAP or other later stage, with same (maybe final) value of NUM_INDEX_VACUUMS, that's different than previous behavior. It seems to me that a someone or their tool monitoring pg_stat shouldn't be confused by this change, since: 1) there's no promise about how high NUM_INDEX_VACUUMS will or won't go; and, 2) index_vacuum_count didn't do anything strange like decreasing, or increased before the scans were done; and, 3) the vacuum can finish at any time, and the monitoring process presumably knows that when the PID is gone, it's finished, even if it missed intermediate updates; The behavior is different from before, but I think that's ok: the number of scans is accurate, and the PHASE is accurate, even though it'll change a moment later. I see there's similar case here: | /* report all blocks vacuumed; and that we're cleaning up */ | pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno); | pgstat_progress_update_param(PROGRESS_VACUUM_PHASE, | PROGRESS_VACUUM_PHASE_INDEX_CLEANUP); heap_blks_scanned is documented as "Number of heap blocks SCANNED", and it increments exactly to heap_blks_total. Would someone be confused if heap_blks_scanned==heap_blks_total AND phase=='scanning heap' ? I think they'd just expect PHASE to be updated a moment later. (And if it wasn't, I agree they should then be legitimately confused or concerned). Actually, the doc says: |If heap_blks_scanned is less than heap_blks_total, the system will return to |scanning the heap after this phase is completed; otherwise, it will begin |cleaning up indexes AFTER THIS PHASE IS COMPLETED. I read that to mean that it's okay if heap_blks_scanned==heap_blks_total when scanning/vacuuming heap. Justin
On Thu, Dec 26, 2019 at 09:57:04AM -0600, Justin Pryzby wrote: > So rebasified against your patch. Rebased against your patch in master this time.
Attachment
Re: error context for vacuum to include block number (atomicprogress update)
From
Michael Paquier
Date:
On Sun, Dec 29, 2019 at 02:17:47PM -0600, Justin Pryzby wrote: > The behavior is different from before, but I think that's ok: the number of > scans is accurate, and the PHASE is accurate, even though it'll change a moment > later. pgstat_progress_update_multi_param() is useful when it comes to update multiple parameters at the same time consistently in a given progress phase. For example, in vacuum, when beginning the heap scan, the number of blocks to scan and the max number of dead tuples has to be updated at the same as the phase name, as things have to be reported consistently, so that's critical to be consistent IMO. Now, in this case, we are discussing about updating a parameter which is related to the index vacuuming phase, while switching at the same time to a different phase. I think that splitting both is not confusing here because the number of times vacuum indexes have been done is unrelated to the heap cleanup happening afterwards. On top of that the new code is more readable, and future callers of lazy_vacuum_heap() will never miss to update the progress reporting to the new phase. While on it, a "git grep -n" is showing me two places where we could care more about being consistent by using the multi-param version of progress reports when beginning a new progress phase: - reindex_index() - ReindexRelationConcurrently() One can also note the switch to PROGRESS_VACUUM_PHASE_INDEX_CLEANUP in lazy_scan_heap() but it can be discarded for the same reason as what has been refactored recently with the index vacuuming. -- Michael
Attachment
Rebased against 40d964ec997f64227bc0ff5e058dc4a5770a70a9
I moved some unrelated patches to a separate thread ("vacuum verbose detail logs are unclear")
Attachment
Hi, On 2020-01-19 23:41:59 -0600, Justin Pryzby wrote: > /* > + * Return whether skipping blocks or not. > + * Except when aggressive is set, we want to skip pages that are > + * all-visible according to the visibility map, but only when we can skip > + * at least SKIP_PAGES_THRESHOLD consecutive pages. Since we're reading > + * sequentially, the OS should be doing readahead for us, so there's no > + * gain in skipping a page now and then; that's likely to disable > + * readahead and so be counterproductive. Also, skipping even a single > + * page means that we can't update relfrozenxid, so we only want to do it > + * if we can skip a goodly number of pages. > + * > + * When aggressive is set, we can't skip pages just because they are > + * all-visible, but we can still skip pages that are all-frozen, since > + * such pages do not need freezing and do not affect the value that we can > + * safely set for relfrozenxid or relminmxid. > + * > + * Before entering the main loop, establish the invariant that > + * next_unskippable_block is the next block number >= blkno that we can't > + * skip based on the visibility map, either all-visible for a regular scan > + * or all-frozen for an aggressive scan. We set it to nblocks if there's > + * no such block. We also set up the skipping_blocks flag correctly at > + * this stage. > + * > + * Note: The value returned by visibilitymap_get_status could be slightly > + * out-of-date, since we make this test before reading the corresponding > + * heap page or locking the buffer. This is OK. If we mistakenly think > + * that the page is all-visible or all-frozen when in fact the flag's just > + * been cleared, we might fail to vacuum the page. It's easy to see that > + * skipping a page when aggressive is not set is not a very big deal; we > + * might leave some dead tuples lying around, but the next vacuum will > + * find them. But even when aggressive *is* set, it's still OK if we miss > + * a page whose all-frozen marking has just been cleared. Any new XIDs > + * just added to that page are necessarily newer than the GlobalXmin we > + * computed, so they'll have no effect on the value to which we can safely > + * set relfrozenxid. A similar argument applies for MXIDs and relminmxid. > + * > + * We will scan the table's last page, at least to the extent of > + * determining whether it has tuples or not, even if it should be skipped > + * according to the above rules; except when we've already determined that > + * it's not worth trying to truncate the table. This avoids having > + * lazy_truncate_heap() take access-exclusive lock on the table to attempt > + * a truncation that just fails immediately because there are tuples in > + * the last page. This is worth avoiding mainly because such a lock must > + * be replayed on any hot standby, where it can be disruptive. > + */ FWIW, I think we should just flat out delete all this logic, and replace it with a few explicit PrefetchBuffer() calls. Just by chance I literally just now sped up a VACUUM by more than a factor of 10, by manually prefetching buffers. At least the linux kernel readahead logic doesn't deal well with reading and writing to different locations in the same file, and that's what the ringbuffer pretty invariably leads to for workloads that aren't cached. Partially so I'll find it when I invariably search for this in the future: select pg_prewarm(relid, 'buffer', 'main', blocks_done, least(blocks_done+100000, blocks_total)) from pg_stat_progress_create_indexwhere phase = 'building index: scanning table' and datid = (SELECT oid FROM pg_database WHEREdatname = current_database()); \watch 0.5 > From 623c725c8add0670b28cdbfceca1824ba5b0647c Mon Sep 17 00:00:00 2001 > From: Justin Pryzby <pryzbyj@telsasoft.com> > Date: Thu, 12 Dec 2019 20:54:37 -0600 > Subject: [PATCH v9 2/3] vacuum errcontext to show block being processed > > As requested here. > https://www.postgresql.org/message-id/20190807235154.erbmr4o4bo6vgnjv%40alap3.anarazel.de > --- > src/backend/access/heap/vacuumlazy.c | 37 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c > index 9849685..c96abdf 100644 > --- a/src/backend/access/heap/vacuumlazy.c > +++ b/src/backend/access/heap/vacuumlazy.c > @@ -289,6 +289,12 @@ typedef struct LVRelStats > bool lock_waiter_detected; > } LVRelStats; > > +typedef struct > +{ > + char *relname; > + char *relnamespace; > + BlockNumber blkno; > +} vacuum_error_callback_arg; > > /* A few variables that don't seem worth passing around as parameters */ > static int elevel = -1; > @@ -358,6 +364,7 @@ static void end_parallel_vacuum(Relation *Irel, IndexBulkDeleteResult **stats, > LVParallelState *lps, int nindexes); > static LVSharedIndStats *get_indstats(LVShared *lvshared, int n); > static bool skip_parallel_vacuum_index(Relation indrel, LVShared *lvshared); > +static void vacuum_error_callback(void *arg); > > > /* > @@ -803,6 +810,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, > PROGRESS_VACUUM_MAX_DEAD_TUPLES > }; > int64 initprog_val[3]; > + ErrorContextCallback errcallback; > + vacuum_error_callback_arg errcbarg; > > pg_rusage_init(&ru0); > > @@ -879,6 +888,15 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, > next_unskippable_block = 0; > skipping_blocks = skip_blocks(onerel, params, &next_unskippable_block, nblocks, &vmbuffer, aggressive); > > + /* Setup error traceback support for ereport() */ > + errcbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); > + errcbarg.relname = relname; > + errcbarg.blkno = InvalidBlockNumber; /* Not known yet */ > + errcallback.callback = vacuum_error_callback; > + errcallback.arg = (void *) &errcbarg; > + errcallback.previous = error_context_stack; > + error_context_stack = &errcallback; > + > for (blkno = 0; blkno < nblocks; blkno++) > { > Buffer buf; > @@ -900,6 +918,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, > #define FORCE_CHECK_PAGE() \ > (blkno == nblocks - 1 && should_attempt_truncation(params, vacrelstats)) > > + errcbarg.blkno = blkno; > + > pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno); > > if (blkno == next_unskippable_block) > @@ -966,8 +986,11 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, > } > > /* 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); > + error_context_stack = &errcallback; > > /* Remove tuples from heap */ > lazy_vacuum_heap(onerel, vacrelstats); > @@ -1575,6 +1598,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, > RecordPageWithFreeSpace(onerel, blkno, freespace); > } Alternatively we could push another context for each index inside lazy_vacuum_all_indexes(). There's been plenty bugs in indexes triggering problems, so that could be worthwhile. > +/* > + * Error context callback for errors occurring during vacuum. > + */ > +static void > +vacuum_error_callback(void *arg) > +{ > + vacuum_error_callback_arg *cbarg = arg; > + errcontext("while scanning block %u of relation \"%s.%s\"", > + cbarg->blkno, cbarg->relnamespace, cbarg->relname); > +} > -- > 2.7.4 > I think it might be useful to expand the message to explain which part of vacuuming this is about. But I'd leave that for a later patch. > From 27a0c085d8d965252ebb8eb2e47362f27fa4203e Mon Sep 17 00:00:00 2001 > From: Justin Pryzby <pryzbyj@telsasoft.com> > Date: Thu, 12 Dec 2019 20:34:03 -0600 > Subject: [PATCH v9 3/3] add errcontext callback in lazy_vacuum_heap, too > > --- > src/backend/access/heap/vacuumlazy.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c > index c96abdf..f380437 100644 > --- a/src/backend/access/heap/vacuumlazy.c > +++ b/src/backend/access/heap/vacuumlazy.c > @@ -1639,6 +1639,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, > > /* Remove tuples from heap */ > lazy_vacuum_heap(onerel, vacrelstats); > + error_context_stack = errcallback.previous; > } This I do not get. I didn't yet fully wake up, so I might just be slow? Greetings, Andres Freund
Hi, On 2019-12-12 21:08:31 -0600, Justin Pryzby wrote: > On Wed, Dec 11, 2019 at 12:33:53PM -0300, Alvaro Herrera wrote: > On Wed, Dec 11, 2019 at 08:54:25AM -0800, Andres Freund wrote: > > Hm, wonder if could be worthwhile to not use a separate struct here, but > > instead extend one of the existing structs to contain the necessary > > information. Or perhaps have one new struct with all the necessary > > information. There's already quite a few places that do > > get_namespace_name(), for example. > > Didn't find a better struct to use yet. I was thinking that you could just use LVRelStats. Greetings, Andres Freund
On Mon, Jan 20, 2020 at 11:11:20AM -0800, Andres Freund wrote: > This I do not get. I didn't yet fully wake up, so I might just be slow? It was needlessly cute at the cost of clarity (meant to avoid setting error_context_stack in lazy_scan_heap and again immediately on its return). On Mon, Jan 20, 2020 at 11:13:05AM -0800, Andres Freund wrote: > I was thinking that you could just use LVRelStats. Done. On Mon, Jan 20, 2020 at 11:11:20AM -0800, Andres Freund wrote: > Alternatively we could push another context for each index inside > lazy_vacuum_all_indexes(). There's been plenty bugs in indexes > triggering problems, so that could be worthwhile. Did this too, although I'm not sure what kind of errors it'd find (?) I considered elimating other uses of RelationGetRelationName, or looping over vacrelstats->blkno instead of local blkno. I did that in an additional patch (that will cause conflicts if you try to apply it, due to other vacuum patch in this branch). CREATE TABLE t AS SELECT generate_series(1,99999)a; postgres=# SET client_min_messages=debug;SET statement_timeout=39; VACUUM (VERBOSE, PARALLEL 0) t; INFO: vacuuming "public.t" 2020-01-20 15:46:14.993 CST [20056] ERROR: canceling statement due to statement timeout 2020-01-20 15:46:14.993 CST [20056] CONTEXT: while scanning block 211 of relation "public.t" 2020-01-20 15:46:14.993 CST [20056] STATEMENT: VACUUM (VERBOSE, PARALLEL 0) t; ERROR: canceling statement due to statement timeout CONTEXT: while scanning block 211 of relation "public.t" SELECT 'CREATE INDEX ON t(a)' FROM generate_series(1,11);\gexec UPDATE t SET a=a+1; postgres=# SET client_min_messages=debug;SET statement_timeout=99; VACUUM (VERBOSE, PARALLEL 0) t; INFO: vacuuming "public.t" DEBUG: "t_a_idx": vacuuming index 2020-01-20 15:47:36.338 CST [20139] ERROR: canceling statement due to statement timeout 2020-01-20 15:47:36.338 CST [20139] CONTEXT: while vacuuming relation "public.t_a_idx" 2020-01-20 15:47:36.338 CST [20139] STATEMENT: VACUUM (VERBOSE, PARALLEL 0) t; ERROR: canceling statement due to statement timeout CONTEXT: while vacuuming relation "public.t_a_idx" I haven't found a good way of exercizing the "vacuuming heap" path, though.
Attachment
On Tue, 21 Jan 2020 at 06:49, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Mon, Jan 20, 2020 at 11:11:20AM -0800, Andres Freund wrote: > > This I do not get. I didn't yet fully wake up, so I might just be slow? > > It was needlessly cute at the cost of clarity (meant to avoid setting > error_context_stack in lazy_scan_heap and again immediately on its return). > > On Mon, Jan 20, 2020 at 11:13:05AM -0800, Andres Freund wrote: > > I was thinking that you could just use LVRelStats. > > Done. > > On Mon, Jan 20, 2020 at 11:11:20AM -0800, Andres Freund wrote: > > Alternatively we could push another context for each index inside > > lazy_vacuum_all_indexes(). There's been plenty bugs in indexes > > triggering problems, so that could be worthwhile. > > Did this too, although I'm not sure what kind of errors it'd find (?) > > I considered elimating other uses of RelationGetRelationName, or looping over > vacrelstats->blkno instead of local blkno. I did that in an additional patch > (that will cause conflicts if you try to apply it, due to other vacuum patch in > this branch). > > CREATE TABLE t AS SELECT generate_series(1,99999)a; > > postgres=# SET client_min_messages=debug;SET statement_timeout=39; VACUUM (VERBOSE, PARALLEL 0) t; > INFO: vacuuming "public.t" > 2020-01-20 15:46:14.993 CST [20056] ERROR: canceling statement due to statement timeout > 2020-01-20 15:46:14.993 CST [20056] CONTEXT: while scanning block 211 of relation "public.t" > 2020-01-20 15:46:14.993 CST [20056] STATEMENT: VACUUM (VERBOSE, PARALLEL 0) t; > ERROR: canceling statement due to statement timeout > CONTEXT: while scanning block 211 of relation "public.t" > > SELECT 'CREATE INDEX ON t(a)' FROM generate_series(1,11);\gexec > UPDATE t SET a=a+1; > > postgres=# SET client_min_messages=debug;SET statement_timeout=99; VACUUM (VERBOSE, PARALLEL 0) t; > INFO: vacuuming "public.t" > DEBUG: "t_a_idx": vacuuming index > 2020-01-20 15:47:36.338 CST [20139] ERROR: canceling statement due to statement timeout > 2020-01-20 15:47:36.338 CST [20139] CONTEXT: while vacuuming relation "public.t_a_idx" > 2020-01-20 15:47:36.338 CST [20139] STATEMENT: VACUUM (VERBOSE, PARALLEL 0) t; > ERROR: canceling statement due to statement timeout > CONTEXT: while vacuuming relation "public.t_a_idx" > > I haven't found a good way of exercizing the "vacuuming heap" path, though. Some of them conflicts with the current HEAD(62c9b52231). Please rebase them. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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.
Attachment
- v11-0001-vacuum-errcontext-to-show-block-being-processed.patch
- v11-0002-add-errcontext-callback-in-lazy_vacuum_heap-too.patch
- v11-0003-Add-vacrelstats.stage-and-distinct-context-messa.patch
- v11-0004-errcontext-for-lazy_vacuum_index.patch
- v11-0005-Avoid-extra-calls-like-GetRelationName.patch.NOTPATCH
On 2020-Jan-21, Justin Pryzby 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. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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. 3rd attempt (sorry for the noise).
Attachment
On 2020-Jan-21, Justin Pryzby wrote: > 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. Honestly, I think we should be scared of a patch applier that ignored differences in context lines. After all, that's why those context lines are there -- so that they provide additional location cues for the lines being modified. If you allow random other lines to be there, you could be inserting stuff in arbitrarily erroneous places. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, 22 Jan 2020 at 10:17, Justin Pryzby <pryzby@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
On Mon, Jan 20, 2020 at 11:11:20AM -0800, Andres Freund wrote: > > @@ -966,8 +986,11 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, > > /* 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); > > + error_context_stack = &errcallback; > > Alternatively we could push another context for each index inside > lazy_vacuum_all_indexes(). There's been plenty bugs in indexes > triggering problems, so that could be worthwhile. Is the callback for index vacuum useful without a block number? FYI, I have another patch which would add DEBUG output before each stage, which would be just as much information, and without needing to use a callback. It's 0004 here: https://www.postgresql.org/message-id/20200121134934.GY26045%40telsasoft.com @@ -1752,9 +1753,12 @@ lazy_vacuum_all_indexes(Relation onerel, Relation *Irel, { int idx; - for (idx = 0; idx < nindexes; idx++) + for (idx = 0; idx < nindexes; idx++) { + ereport(DEBUG1, (errmsg("\"%s\": vacuuming index", + RelationGetRelationName(Irel[idx])))); lazy_vacuum_index(Irel[idx], &stats[idx], vacrelstats->dead_tuples,
Thanks for reviewing
On Wed, Jan 22, 2020 at 05:37:06PM +0900, Masahiko Sawada wrote:
> I'm not sure it's worth to have patches separately but I could apply
The later patches expanded on the initial scope, and to my understanding the
1st callback is desirable but the others are maybe still at question.
> 1. * 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.
Updated in the 0005 - still not sure if that patch will be desirable, though.
Also, I realized that it's we cannot use vacrelstats->blkno instead of local
blkno variable, since vacrelstats->blkno is used simultaneously by scan heap
and vacuum heap).
> * The 'stage' is missing to support index cleanup.
But the callback isn't used during index cleanup ?
> * It seems to me strange that only initialization of latestRemovedXid
> is done after error callback initialization.
Yes, that was silly - I thought it was just an artifact of diff's inability to
express rearranged code any better.
> * 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.
I think that's already true ? But topic for a separate patch, if not.
> 3. Maybe we can do like:
done
> 4. These comments can be merged like:
done
> 5. Why do we need to initialize blkno in spite of not using?
removed
> 6.
> + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> * 'vacrelstats' would be a better name than 'cbarg'.
Really? I'd prefer to avoid repeating long variable three times:
vacrelstats->blkno, vacrelstats->relnamespace, vacrelstats->relname);
> * In index vacuum, how about "while vacuuming index \"%s.%s\""?
Yes. I'm still unclear if this is useful without a block number, or why it
wouldn't be better to write DEBUG1 log with index name before vacuuming each.
Justin
Attachment
Hi, On 2020-01-22 17:17:26 -0600, Justin Pryzby wrote: > On Mon, Jan 20, 2020 at 11:11:20AM -0800, Andres Freund wrote: > > > @@ -966,8 +986,11 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, > > > /* 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); > > > + error_context_stack = &errcallback; > > > > Alternatively we could push another context for each index inside > > lazy_vacuum_all_indexes(). There's been plenty bugs in indexes > > triggering problems, so that could be worthwhile. > > Is the callback for index vacuum useful without a block number? Yea, it is. Without at least that context I don't think we even will reliably know which index we're dealing with in case of an error. > FYI, I have another patch which would add DEBUG output before each stage, which > would be just as much information, and without needing to use a callback. > It's 0004 here: I don't think that is equivalent at all. With a context I see the context in the log in case of an error. With a DEBUG message I need to be able to reproduce the error (without even knowing which relation etc). Greetings, Andres Freund
Hi, On 2020-01-20 15:49:29 -0600, Justin Pryzby wrote: > On Mon, Jan 20, 2020 at 11:11:20AM -0800, Andres Freund wrote: > On Mon, Jan 20, 2020 at 11:11:20AM -0800, Andres Freund wrote: > > Alternatively we could push another context for each index inside > > lazy_vacuum_all_indexes(). There's been plenty bugs in indexes > > triggering problems, so that could be worthwhile. > > Did this too, although I'm not sure what kind of errors it'd find (?) What do you mean with "kind of errors"? We had index corruptions that caused index vacuuming to fail, and there was no way to diagnose which table / index it was so far? > postgres=# SET client_min_messages=debug;SET statement_timeout=99; VACUUM (VERBOSE, PARALLEL 0) t; > INFO: vacuuming "public.t" > DEBUG: "t_a_idx": vacuuming index > 2020-01-20 15:47:36.338 CST [20139] ERROR: canceling statement due to statement timeout > 2020-01-20 15:47:36.338 CST [20139] CONTEXT: while vacuuming relation "public.t_a_idx" > 2020-01-20 15:47:36.338 CST [20139] STATEMENT: VACUUM (VERBOSE, PARALLEL 0) t; > ERROR: canceling statement due to statement timeout > CONTEXT: while vacuuming relation "public.t_a_idx" It'd be a bit nicer if it said index "public.t_a_idx" for relation "public.t". Greetings, Andres Freund
On Sun, Jan 26, 2020 at 12:29:38PM -0800, Andres Freund wrote:
> > postgres=# SET client_min_messages=debug;SET statement_timeout=99; VACUUM (VERBOSE, PARALLEL 0) t;
> > INFO: vacuuming "public.t"
> > DEBUG: "t_a_idx": vacuuming index
> > 2020-01-20 15:47:36.338 CST [20139] ERROR: canceling statement due to statement timeout
> > 2020-01-20 15:47:36.338 CST [20139] CONTEXT: while vacuuming relation "public.t_a_idx"
> > 2020-01-20 15:47:36.338 CST [20139] STATEMENT: VACUUM (VERBOSE, PARALLEL 0) t;
> > ERROR: canceling statement due to statement timeout
> > CONTEXT: while vacuuming relation "public.t_a_idx"
>
> It'd be a bit nicer if it said index "public.t_a_idx" for relation "public.t".
I think that tips the scale in favour of making vacrelstats a global.
I added that as a 1st patch, and squished the callback patches into one.
Also, it seems to me we shouldn't repeat the namespace of the index *and* its
table. I tried looking for consistency here:
grep -r '\\"%s.%s\\"' --incl='*.c' |grep '\\"%s\\"'
src/backend/commands/cluster.c: (errmsg("clustering \"%s.%s\" using index scan on \"%s\"",
src/backend/access/heap/vacuumlazy.c: errcontext(_("while vacuuming index \"%s\" on table \"%s.%s\""),
grep -r 'index \\".* table \\"' --incl='*.c'
src/backend/catalog/index.c: (errmsg("building index \"%s\" on table \"%s\" serially",
src/backend/catalog/index.c: (errmsg_plural("building index \"%s\" on table \"%s\" with
requestfor %d parallel worker",
src/backend/catalog/index.c: "building index \"%s\" on table
\"%s\"with request for %d parallel workers",
src/backend/catalog/catalog.c: errmsg("index \"%s\" does not belong to table \"%s\"",
src/backend/commands/indexcmds.c: (errmsg("%s %s will create implicit index \"%s\" for
table\"%s\"",
src/backend/commands/tablecmds.c: errmsg("index \"%s\" for table \"%s\" does not
exist",
src/backend/commands/tablecmds.c: errmsg("index \"%s\" for table \"%s\" does not
exist",
src/backend/commands/tablecmds.c: errdetail("The index \"%s\" belongs to
aconstraint in table \"%s\" but no constraint exists for index \"%s\".",
src/backend/commands/cluster.c: errmsg("index \"%s\" for table \"%s\" does not
exist",
src/backend/parser/parse_utilcmd.c: errmsg("index \"%s\" does not belong to table
\"%s\"",
Attachment
It occured to me that there's an issue with sharing vacrelstats between scan/vacuum, since blkno and stage are set by the heap/index vacuum routines, but not reset on their return to heap scan. Not sure if we should reset them, or go back to using a separate struct, like it was here: https://www.postgresql.org/message-id/20200120054159.GT26045%40telsasoft.com On Sun, Jan 26, 2020 at 11:38:13PM -0600, Justin Pryzby wrote: > From 592a77554f99b5ff9035c55bf19a79a1443ae59e Mon Sep 17 00:00:00 2001 > From: Justin Pryzby <pryzbyj@telsasoft.com> > Date: Thu, 12 Dec 2019 20:54:37 -0600 > Subject: [PATCH v14 2/3] vacuum errcontext to show block being processed > > As requested here. > https://www.postgresql.org/message-id/20190807235154.erbmr4o4bo6vgnjv%40alap3.anarazel.de > --- > src/backend/access/heap/vacuumlazy.c | 85 +++++++++++++++++++++++++++++++++++- > 1 file changed, 84 insertions(+), 1 deletion(-) > > diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c > index 114428b..a62dc79 100644 > --- a/src/backend/access/heap/vacuumlazy.c > +++ b/src/backend/access/heap/vacuumlazy.c > @@ -290,8 +290,14 @@ typedef struct LVRelStats > int num_index_scans; > TransactionId latestRemovedXid; > bool lock_waiter_detected; > -} LVRelStats; > > + /* Used by the error callback */ > + char *relname; > + char *relnamespace; > + BlockNumber blkno; > + char *indname; > + int stage; /* 0: scan heap; 1: vacuum heap; 2: vacuum index */ > +} LVRelStats; > > /* A few variables that don't seem worth passing around as parameters */ > static int elevel = -1; > @@ -360,6 +366,7 @@ static void end_parallel_vacuum(Relation *Irel, IndexBulkDeleteResult **stats, > LVParallelState *lps, int nindexes); > static LVSharedIndStats *get_indstats(LVShared *lvshared, int n); > static bool skip_parallel_vacuum_index(Relation indrel, LVShared *lvshared); > +static void vacuum_error_callback(void *arg); > > > /* > @@ -721,6 +728,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, > PROGRESS_VACUUM_MAX_DEAD_TUPLES > }; > int64 initprog_val[3]; > + ErrorContextCallback errcallback; > > pg_rusage_init(&ru0); > > @@ -867,6 +875,17 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, > else > skipping_blocks = false; > > + /* Setup error traceback support for ereport() */ > + vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); > + vacrelstats.relname = relname; > + 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; > + > for (blkno = 0; blkno < nblocks; blkno++) > { > Buffer buf; > @@ -888,6 +907,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, > #define FORCE_CHECK_PAGE() \ > (blkno == nblocks - 1 && should_attempt_truncation(params)) > > + vacrelstats.blkno = blkno; > + > pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno); > > if (blkno == next_unskippable_block) > @@ -984,12 +1005,18 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, > vmbuffer = InvalidBuffer; > } > > + /* 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, > lps, nindexes); > /* Remove tuples from heap */ > lazy_vacuum_heap(onerel); > > + /* Replace error context while continuing heap scan */ > + error_context_stack = &errcallback; > + > /* > * Forget the now-vacuumed tuples, and press on, but be careful > * not to reset latestRemovedXid since we want that value to be > @@ -1593,6 +1620,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, > RecordPageWithFreeSpace(onerel, blkno, freespace); > } > > + /* Pop the error context stack */ > + error_context_stack = errcallback.previous; > + > /* report that everything is scanned and vacuumed */ > pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno); > > @@ -1768,11 +1798,24 @@ lazy_vacuum_heap(Relation onerel) > int npages; > PGRUsage ru0; > Buffer vmbuffer = InvalidBuffer; > + ErrorContextCallback errcallback; > > /* 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() > + * ->relnamespace and ->relname are already set > + */ > + vacrelstats.blkno = InvalidBlockNumber; /* Not known yet */ > + vacrelstats.stage = 1; > + > + errcallback.callback = vacuum_error_callback; > + errcallback.arg = (void *) &vacrelstats; > + errcallback.previous = error_context_stack; > + error_context_stack = &errcallback; > + > pg_rusage_init(&ru0); > npages = 0; > > @@ -1787,6 +1830,7 @@ lazy_vacuum_heap(Relation onerel) > vacuum_delay_point(); > > tblk = ItemPointerGetBlockNumber(&vacrelstats.dead_tuples->itemptrs[tupindex]); > + vacrelstats.blkno = tblk; > buf = ReadBufferExtended(onerel, MAIN_FORKNUM, tblk, RBM_NORMAL, > vac_strategy); > if (!ConditionalLockBufferForCleanup(buf)) > @@ -1807,6 +1851,9 @@ lazy_vacuum_heap(Relation onerel) > npages++; > } > > + /* Pop the error context stack */ > + error_context_stack = errcallback.previous; > + > if (BufferIsValid(vmbuffer)) > { > ReleaseBuffer(vmbuffer); > @@ -2314,6 +2361,8 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, > IndexVacuumInfo ivinfo; > const char *msg; > PGRUsage ru0; > + ErrorContextCallback errcallback; > + LVRelStats errcbarg; /* Used for error callback, only */ > > pg_rusage_init(&ru0); > > @@ -2325,10 +2374,24 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats, > ivinfo.num_heap_tuples = reltuples; > ivinfo.strategy = vac_strategy; > > + /* Setup error traceback support for ereport() */ > + errcbarg.relnamespace = get_namespace_name(RelationGetNamespace(indrel)); > + errcbarg.indname = RelationGetRelationName(indrel); > + errcbarg.relname = vacrelstats.relname; > + errcbarg.stage = 2; > + > + errcallback.callback = vacuum_error_callback; > + errcallback.arg = (void *) &errcbarg; > + errcallback.previous = error_context_stack; > + error_context_stack = &errcallback; > + > /* Do bulk deletion */ > *stats = index_bulk_delete(&ivinfo, *stats, > lazy_tid_reaped, (void *) dead_tuples); > > + /* Pop the error context stack */ > + error_context_stack = errcallback.previous; > + > if (IsParallelWorker()) > msg = gettext_noop("scanned index \"%s\" to remove %d row versions by parallel vacuum worker"); > else > @@ -3371,3 +3434,23 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) > table_close(onerel, ShareUpdateExclusiveLock); > pfree(stats); > } > + > +/* > + * 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 index \"%s\" on table \"%s.%s\""), > + cbarg->indname, cbarg->relnamespace, cbarg->relname); > + > +} > -- > 2.7.4 >
On Mon, 27 Jan 2020 at 14:38, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Sun, Jan 26, 2020 at 12:29:38PM -0800, Andres Freund wrote: > > > postgres=# SET client_min_messages=debug;SET statement_timeout=99; VACUUM (VERBOSE, PARALLEL 0) t; > > > INFO: vacuuming "public.t" > > > DEBUG: "t_a_idx": vacuuming index > > > 2020-01-20 15:47:36.338 CST [20139] ERROR: canceling statement due to statement timeout > > > 2020-01-20 15:47:36.338 CST [20139] CONTEXT: while vacuuming relation "public.t_a_idx" > > > 2020-01-20 15:47:36.338 CST [20139] STATEMENT: VACUUM (VERBOSE, PARALLEL 0) t; > > > ERROR: canceling statement due to statement timeout > > > CONTEXT: while vacuuming relation "public.t_a_idx" > > > > It'd be a bit nicer if it said index "public.t_a_idx" for relation "public.t". > > I think that tips the scale in favour of making vacrelstats a global. > I added that as a 1st patch, and squished the callback patches into one. Hmm I don't think it's a good idea to make vacrelstats global. If we want to display the relation name and its index name in error context we might want to define a new struct dedicated for error context reporting. That is it has blkno, stage and relation name and schema name for both table and index and then we set these variables of callback argument before performing a vacuum phase. We don't change LVRelStats at all. Although the patch replaces get_namespace_name and RelationGetRelationName but we use namespace name of relation at only two places and almost ereport/elog messages use only relation name gotten by RelationGetRelationName which is a macro to access the relation name in Relation struct. So I think adding relname to LVRelStats would not be a big benefit. Similarly, adding table namespace to LVRelStats would be good to avoid calling get_namespace_name whereas I'm not sure it's worth to have it because it's expected not to be really many times. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jan 27, 2020 at 03:59:58PM +0900, Masahiko Sawada wrote: > On Mon, 27 Jan 2020 at 14:38, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Sun, Jan 26, 2020 at 12:29:38PM -0800, Andres Freund wrote: > > > > CONTEXT: while vacuuming relation "public.t_a_idx" > > > > > > It'd be a bit nicer if it said index "public.t_a_idx" for relation "public.t". > > > > I think that tips the scale in favour of making vacrelstats a global. > > I added that as a 1st patch, and squished the callback patches into one. > > Hmm I don't think it's a good idea to make vacrelstats global. If we > want to display the relation name and its index name in error context > we might want to define a new struct dedicated for error context > reporting. That is it has blkno, stage and relation name and schema > name for both table and index and then we set these variables of > callback argument before performing a vacuum phase. We don't change > LVRelStats at all. On Mon, Jan 27, 2020 at 12:14:38AM -0600, Justin Pryzby wrote: > It occured to me that there's an issue with sharing vacrelstats between > scan/vacuum, since blkno and stage are set by the heap/index vacuum routines, > but not reset on their return to heap scan. Not sure if we should reset them, > or go back to using a separate struct, like it was here: > https://www.postgresql.org/message-id/20200120054159.GT26045%40telsasoft.com I went back to this, original, way of doing it. The parallel vacuum patch made it harder to pass the table around :( And has to be separately tested: | SET statement_timeout=0; DROP TABLE t; CREATE TABLE t AS SELECT generate_series(1,99999)a; CREATE INDEX ON t(a); CREATEINDEX ON t(a); UPDATE t SET a=1+a; SET statement_timeout=99;VACUUM(VERBOSE, PARALLEL 2) t; I had to allocate space for the table name within the LVShared struct, not just a pointer, otherwise it would variously crash or fail to output the index name. I think pointers can't be passed to parallel process except using some heavyweight thing like shm_toc_... I guess the callback could also take the index relid instead of name, and use something like IndexGetRelation(). > Although the patch replaces get_namespace_name and > RelationGetRelationName but we use namespace name of relation at only > two places and almost ereport/elog messages use only relation name > gotten by RelationGetRelationName which is a macro to access the > relation name in Relation struct. So I think adding relname to > LVRelStats would not be a big benefit. Similarly, adding table > namespace to LVRelStats would be good to avoid calling > get_namespace_name whereas I'm not sure it's worth to have it because > it's expected not to be really many times. Right, I only tried that to save a few LOC and maybe make shorter lines. It's not important so I'll drop that patch.
Attachment
On Tue, 28 Jan 2020 at 07:50, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Mon, Jan 27, 2020 at 03:59:58PM +0900, Masahiko Sawada wrote: > > On Mon, 27 Jan 2020 at 14:38, Justin Pryzby <pryzby@telsasoft.com> wrote: > > > On Sun, Jan 26, 2020 at 12:29:38PM -0800, Andres Freund wrote: > > > > > CONTEXT: while vacuuming relation "public.t_a_idx" > > > > > > > > It'd be a bit nicer if it said index "public.t_a_idx" for relation "public.t". > > > > > > I think that tips the scale in favour of making vacrelstats a global. > > > I added that as a 1st patch, and squished the callback patches into one. > > > > Hmm I don't think it's a good idea to make vacrelstats global. If we > > want to display the relation name and its index name in error context > > we might want to define a new struct dedicated for error context > > reporting. That is it has blkno, stage and relation name and schema > > name for both table and index and then we set these variables of > > callback argument before performing a vacuum phase. We don't change > > LVRelStats at all. > > On Mon, Jan 27, 2020 at 12:14:38AM -0600, Justin Pryzby wrote: > > It occured to me that there's an issue with sharing vacrelstats between > > scan/vacuum, since blkno and stage are set by the heap/index vacuum routines, > > but not reset on their return to heap scan. Not sure if we should reset them, > > or go back to using a separate struct, like it was here: > > https://www.postgresql.org/message-id/20200120054159.GT26045%40telsasoft.com > > I went back to this, original, way of doing it. > The parallel vacuum patch made it harder to pass the table around :( > And has to be separately tested: > > | SET statement_timeout=0; DROP TABLE t; CREATE TABLE t AS SELECT generate_series(1,99999)a; CREATE INDEX ON t(a); CREATEINDEX ON t(a); UPDATE t SET a=1+a; SET statement_timeout=99;VACUUM(VERBOSE, PARALLEL 2) t; > > I had to allocate space for the table name within the LVShared struct, not just > a pointer, otherwise it would variously crash or fail to output the index name. > I think pointers can't be passed to parallel process except using some > heavyweight thing like shm_toc_... > > I guess the callback could also take the index relid instead of name, and use > something like IndexGetRelation(). > > > Although the patch replaces get_namespace_name and > > RelationGetRelationName but we use namespace name of relation at only > > two places and almost ereport/elog messages use only relation name > > gotten by RelationGetRelationName which is a macro to access the > > relation name in Relation struct. So I think adding relname to > > LVRelStats would not be a big benefit. Similarly, adding table > > namespace to LVRelStats would be good to avoid calling > > get_namespace_name whereas I'm not sure it's worth to have it because > > it's expected not to be really many times. > > Right, I only tried that to save a few LOC and maybe make shorter lines. > It's not important so I'll drop that patch. Thank you for updating the patch. Here is some review comments: 1. +typedef struct +{ + char *relnamespace; + char *relname; + char *indname; /* If vacuuming index */ I think "Non-null if vacuuming index" is better. And tablename is better than relname for accuracy? 2. + BlockNumber blkno; + int stage; /* 0: scan heap; 1: vacuum heap; 2: vacuum index */ +} vacuum_error_callback_arg; Why do we not support index cleanup phase? 3. /* 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); I think it's an unnecessary removal. 4. 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() + * ->relnamespace and ->relname are already set + */ + errcbarg.blkno = InvalidBlockNumber; /* Not known yet */ + errcbarg.stage = 1; relnamespace and relname of errcbarg is not set as it is initialized in this function. 5. @@ -177,6 +177,7 @@ typedef struct LVShared * the lazy vacuum. */ Oid relid; + char relname[NAMEDATALEN]; /* tablename, used for error callback */ Hmm I think it's not a good idea to have LVShared have relname because the parallel vacuum worker being able to know the table name by oid and it consumes DSM memory. To pass the relation name down to lazy_vacuum_index I thought to add new argument relname to some functions but in parallel vacuum case there are multiple functions until we reach lazy_vacuum_index. So I think it doesn't make sense to add a new argument to all those functions. How about getting relation name from index relation? That is, in lazy_vacuum_index we can get table oid from the index relation by IndexGetRelation() and therefore we can get the table name which is in palloc'd memory. That way we don't need to add relname to any existing struct such as LVRelStats and LVShared. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Thanks for reviewing again
On Sun, Feb 02, 2020 at 10:45:12AM +0900, Masahiko Sawada wrote:
> Thank you for updating the patch. Here is some review comments:
>
> 1.
> +typedef struct
> +{
> + char *relnamespace;
> + char *relname;
> + char *indname; /* If vacuuming index */
>
> I think "Non-null if vacuuming index" is better.
Actually it's undefined garbage (not NULL) if not vacuuming index.
> And tablename is better than relname for accuracy?
The existing code uses relname, so I left that, since it's strange to
start using tablename and then write things like:
| errcbarg.tblname = relname;
...
| errcontext(_("while scanning block %u of relation \"%s.%s\""),
| cbarg->blkno, cbarg->relnamespace, cbarg->tblname);
Also, mat views can be vacuumed.
> 2.
> + BlockNumber blkno;
> + int stage; /* 0: scan heap; 1: vacuum heap; 2: vacuum index */
> +} vacuum_error_callback_arg;
>
> Why do we not support index cleanup phase?
The patch started out just handling scan_heap. The added vacuum_heap. Then
added vacuum_index. Now, I've added index cleanup.
> 4.
> + /*
> + * Setup error traceback support for ereport()
> + * ->relnamespace and ->relname are already set
> + */
> + errcbarg.blkno = InvalidBlockNumber; /* Not known yet */
> + errcbarg.stage = 1;
>
> relnamespace and relname of errcbarg is not set as it is initialized
> in this function.
Thanks. That's an oversight from switching back to local vars instead of
LVRelStats while updating the patch while out of town..
I don't know how to consistently test the vacuum_heap case, but rechecked it just now.
postgres=# SET client_min_messages=debug; SET statement_timeout=0; UPDATE t SET a=1+a; SET statement_timeout=150;
VACUUM(VERBOSE,PARALLEL 1) t;
...
2020-02-01 23:11:06.482 CST [26609] ERROR: canceling statement due to statement timeout
2020-02-01 23:11:06.482 CST [26609] CONTEXT: while vacuuming block 33 of relation "public.t"
> 5.
> @@ -177,6 +177,7 @@ typedef struct LVShared
> * the lazy vacuum.
> */
> Oid relid;
> + char relname[NAMEDATALEN]; /* tablename, used for error callback */
>
> How about getting relation
> name from index relation? That is, in lazy_vacuum_index we can get
> table oid from the index relation by IndexGetRelation() and therefore
> we can get the table name which is in palloc'd memory. That way we
> don't need to add relname to any existing struct such as LVRelStats
> and LVShared.
See attached
Also, I think we shouldn't show a block number if it's "Invalid", to avoid
saying "while vacuuming block 4294967295 of relation ..."
For now, I made it not show any errcontext at all in that case.
Attachment
On Sun, 2 Feb 2020 at 15:03, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> Thanks for reviewing again
>
> On Sun, Feb 02, 2020 at 10:45:12AM +0900, Masahiko Sawada wrote:
> > Thank you for updating the patch. Here is some review comments:
> >
> > 1.
> > +typedef struct
> > +{
> > + char *relnamespace;
> > + char *relname;
> > + char *indname; /* If vacuuming index */
> >
> > I think "Non-null if vacuuming index" is better.
>
> Actually it's undefined garbage (not NULL) if not vacuuming index.
So how about something like "set index name only during vacuuming
index". My point is that the current comment seems to be unclear to me
what describing.
>
> > And tablename is better than relname for accuracy?
>
> The existing code uses relname, so I left that, since it's strange to
> start using tablename and then write things like:
>
> | errcbarg.tblname = relname;
> ...
> | errcontext(_("while scanning block %u of relation \"%s.%s\""),
> | cbarg->blkno, cbarg->relnamespace, cbarg->tblname);
>
> Also, mat views can be vacuumed.
ok, agreed.
>
> > 2.
> > + BlockNumber blkno;
> > + int stage; /* 0: scan heap; 1: vacuum heap; 2: vacuum index */
> > +} vacuum_error_callback_arg;
> >
> > Why do we not support index cleanup phase?
>
> The patch started out just handling scan_heap. The added vacuum_heap. Then
> added vacuum_index. Now, I've added index cleanup.
>
> > 4.
> > + /*
> > + * Setup error traceback support for ereport()
> > + * ->relnamespace and ->relname are already set
> > + */
> > + errcbarg.blkno = InvalidBlockNumber; /* Not known yet */
> > + errcbarg.stage = 1;
> >
> > relnamespace and relname of errcbarg is not set as it is initialized
> > in this function.
>
> Thanks. That's an oversight from switching back to local vars instead of
> LVRelStats while updating the patch while out of town..
>
> I don't know how to consistently test the vacuum_heap case, but rechecked it just now.
>
> postgres=# SET client_min_messages=debug; SET statement_timeout=0; UPDATE t SET a=1+a; SET statement_timeout=150;
VACUUM(VERBOSE,PARALLEL 1) t;
> ...
> 2020-02-01 23:11:06.482 CST [26609] ERROR: canceling statement due to statement timeout
> 2020-02-01 23:11:06.482 CST [26609] CONTEXT: while vacuuming block 33 of relation "public.t"
>
> > 5.
> > @@ -177,6 +177,7 @@ typedef struct LVShared
> > * the lazy vacuum.
> > */
> > Oid relid;
> > + char relname[NAMEDATALEN]; /* tablename, used for error callback */
> >
> > How about getting relation
> > name from index relation? That is, in lazy_vacuum_index we can get
> > table oid from the index relation by IndexGetRelation() and therefore
> > we can get the table name which is in palloc'd memory. That way we
> > don't need to add relname to any existing struct such as LVRelStats
> > and LVShared.
>
> See attached
>
> Also, I think we shouldn't show a block number if it's "Invalid", to avoid
> saying "while vacuuming block 4294967295 of relation ..."
>
> For now, I made it not show any errcontext at all in that case.
Thank you for updating the patch!
Here is the comment for v16 patch:
1.
+ ErrorContextCallback errcallback = { error_context_stack,
vacuum_error_callback, &errcbarg, };
I think it's better to initialize individual fields because we might
need to fix it as well when fields of ErrorContextCallback are
changed.
2.
+ /* Replace error context while continuing heap scan */
+ error_context_stack = &errcallback;
/*
* Forget the now-vacuumed tuples, and press on, but be careful
* not to reset latestRemovedXid since we want that value to be
* valid.
*/
dead_tuples->num_tuples = 0;
/*
* Vacuum the Free Space Map to make newly-freed space visible on
* upper-level FSM pages. Note we have not yet processed blkno.
*/
FreeSpaceMapVacuumRange(onerel, next_fsm_block_to_vacuum, blkno);
next_fsm_block_to_vacuum = blkno;
/* Report that we are once again scanning the heap */
pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
PROGRESS_VACUUM_PHASE_SCAN_HEAP);
}
I think we can set the error context for heap scan again after
freespace map vacuum finishing, maybe after reporting the new phase.
Otherwise the user will get confused if an error occurs during
freespace map vacuum. And I think the comment is unclear, how about
"Set the error context fro heap scan again"?
3.
+ if (cbarg->blkno!=InvalidBlockNumber)
+ errcontext(_("while scanning block %u of relation \"%s.%s\""),
+ cbarg->blkno, cbarg->relnamespace, cbarg->relname);
We can use BlockNumberIsValid macro instead.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Feb 04, 2020 at 01:58:20PM +0900, Masahiko Sawada wrote:
> Here is the comment for v16 patch:
>
> 2.
> I think we can set the error context for heap scan again after
> freespace map vacuum finishing, maybe after reporting the new phase.
> Otherwise the user will get confused if an error occurs during
> freespace map vacuum. And I think the comment is unclear, how about
> "Set the error context fro heap scan again"?
Good point
> 3.
> + if (cbarg->blkno!=InvalidBlockNumber)
> + errcontext(_("while scanning block %u of relation \"%s.%s\""),
> + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
>
> We can use BlockNumberIsValid macro instead.
Thanks. See attached, now squished together patches.
I added functions to initialize the callbacks, so error handling is out of the
way and minimally distract from the rest of vacuum.
Attachment
On Sat, 8 Feb 2020 at 10:01, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Tue, Feb 04, 2020 at 01:58:20PM +0900, Masahiko Sawada wrote:
> > Here is the comment for v16 patch:
> >
> > 2.
> > I think we can set the error context for heap scan again after
> > freespace map vacuum finishing, maybe after reporting the new phase.
> > Otherwise the user will get confused if an error occurs during
> > freespace map vacuum. And I think the comment is unclear, how about
> > "Set the error context fro heap scan again"?
>
> Good point
>
> > 3.
> > + if (cbarg->blkno!=InvalidBlockNumber)
> > + errcontext(_("while scanning block %u of relation \"%s.%s\""),
> > + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> >
> > We can use BlockNumberIsValid macro instead.
>
> Thanks. See attached, now squished together patches.
>
> I added functions to initialize the callbacks, so error handling is out of the
> way and minimally distract from the rest of vacuum.
Thank you for updating the patch! Here is the review comments:
1.
+static void vacuum_error_callback(void *arg);
+static void init_error_context_heap(ErrorContextCallback
*errcallback, vacuum_error_callback_arg *errcbarg, Relation onerel,
int phase);
+static void init_error_context_index(ErrorContextCallback
*errcallback, vacuum_error_callback_arg *errcbarg, Relation indrel,
int phase);
You need to add a newline to follow the limit line lengths so that the
code is readable in an 80-column window. Or please run pgindent.
2.
+/* Initialize error context for heap operations */
+static void
+init_error_context_heap(ErrorContextCallback *errcallback,
vacuum_error_callback_arg *errcbarg, Relation onerel, int phase)
+{
+ errcbarg->relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+ errcbarg->relname = RelationGetRelationName(onerel);
+ errcbarg->indname = NULL; /* Not used for heap */
+ errcbarg->blkno = InvalidBlockNumber; /* Not known yet */
+ errcbarg->phase = phase;
+
+ errcallback->callback = vacuum_error_callback;
+ errcallback->arg = errcbarg;
+ errcallback->previous = error_context_stack;
+ error_context_stack = errcallback;
+}
I think that making initialization process of errcontext argument a
function is good. But maybe we can merge these two functions into one.
init_error_context_heap and init_error_context_index actually don't
only initialize the callback arguments but also push the vacuum
errcallback, in spite of the function name having 'init'. Also I think
it might be better to only initialize the callback arguments in this
function and to set errcallback by caller, rather than to wrap pushing
errcallback by a function. How about the following function
initializing the vacuum callback arguments?
static void
init_vacuum_error_callback_arg(vacuum_error_callback_arg *errcbarg,
Relation rel, int phase)
{
errcbarg->relnamespace = get_namespace_name(RelationGetNamespace(onerel));
errcbarg->blkno = InvalidBlockNumber;
errcbarg->phase = phase;
switch (phase) {
case PROGRESS_VACUUM_PHASE_SCAN_HEAP:
case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
errcbarg->relname = RelationGetRelationName(rel);
errcbarg->indname = NULL;
break;
case PROGRESS_VACUUM_PHASE_VACUUM_INDEX:
case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
/* rel is an index relation in index vacuum case */
errcbarg->relname = get_rel_name(indrel->rd_index->indexrelid);
errcbarg->indname = RelationGetRelationName(rel);
break;
}
}
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Feb 13, 2020 at 02:55:53PM +0900, Masahiko Sawada wrote:
> You need to add a newline to follow the limit line lengths so that the
> code is readable in an 80-column window. Or please run pgindent.
For now I :set tw=80
> 2.
> I think that making initialization process of errcontext argument a
> function is good. But maybe we can merge these two functions into one.
Thanks, this is better, and I used that.
> init_error_context_heap and init_error_context_index actually don't
> only initialize the callback arguments but also push the vacuum
> errcallback, in spite of the function name having 'init'. Also I think
> it might be better to only initialize the callback arguments in this
> function and to set errcallback by caller, rather than to wrap pushing
> errcallback by a function.
However I think it's important not to repeat this 4 times:
errcallback->callback = vacuum_error_callback;
errcallback->arg = errcbarg;
errcallback->previous = error_context_stack;
error_context_stack = errcallback;
So I kept the first 3 of those in the function and copied only assignment to
the global. That helps makes the heap scan function clear, which assigns to it
twice.
BTW, for testing, I'm able to consistently hit the "vacuuming block" case like
this:
SET statement_timeout=0; DROP TABLE t; CREATE TABLE t(i int); CREATE INDEX ON t(i); INSERT INTO t SELECT
generate_series(1,99999);UPDATE t SET i=i-1; SET statement_timeout=111; SET vacuum_cost_delay=3; SET
vacuum_cost_page_dirty=0;SET vacuum_cost_page_hit=11; SET vacuum_cost_limit=33; SET statement_timeout=3333; VACUUM
VERBOSEt;
Thanks for re-reviewing.
--
Justin
Attachment
On Tue, Jan 21, 2020 at 8:11 AM Andres Freund <andres@anarazel.de> wrote: > FWIW, I think we should just flat out delete all this logic, and replace > it with a few explicit PrefetchBuffer() calls. Just by chance I > literally just now sped up a VACUUM by more than a factor of 10, by > manually prefetching buffers. At least the linux kernel readahead logic > doesn't deal well with reading and writing to different locations in the > same file, and that's what the ringbuffer pretty invariably leads to for > workloads that aren't cached. Interesting. Andrew Gierth made a similar observation on FreeBSD, and showed that by patching his kernel to track sequential writes and sequential reads separately he could improve performance, and I reproduced the same speedup in a patch of my own based on his description (that, erm, I've lost). It's not only VACUUM, it's anything that is writing to a lot of sequential blocks, since the writeback trails along behind by some distance (maybe a ring buffer, maybe all of shared buffers, whatever). The OS sees you flipping back and forth between single block reads and writes and thinks it's random. I didn't investigate this much but it seemed that ZFS was somehow smart enough to understand what was happening at some level but other filesystems were not.
On Fri, 14 Feb 2020 at 08:52, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
Thank you for updating the patch.
> On Thu, Feb 13, 2020 at 02:55:53PM +0900, Masahiko Sawada wrote:
> > You need to add a newline to follow the limit line lengths so that the
> > code is readable in an 80-column window. Or please run pgindent.
>
> For now I :set tw=80
>
> > 2.
> > I think that making initialization process of errcontext argument a
> > function is good. But maybe we can merge these two functions into one.
>
> Thanks, this is better, and I used that.
>
> > init_error_context_heap and init_error_context_index actually don't
> > only initialize the callback arguments but also push the vacuum
> > errcallback, in spite of the function name having 'init'. Also I think
> > it might be better to only initialize the callback arguments in this
> > function and to set errcallback by caller, rather than to wrap pushing
> > errcallback by a function.
>
> However I think it's important not to repeat this 4 times:
> errcallback->callback = vacuum_error_callback;
> errcallback->arg = errcbarg;
> errcallback->previous = error_context_stack;
> error_context_stack = errcallback;
>
> So I kept the first 3 of those in the function and copied only assignment to
> the global. That helps makes the heap scan function clear, which assigns to it
> twice.
Okay. Here is the review comments for v18 patch:
1.
+/* Initialize error context for heap operations */
+static void
+init_error_context(ErrorContextCallback *errcallback,
vacuum_error_callback_arg *errcbarg, Relation rel, int phase)
* 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"?
2.
+{
+ switch (phase)
+ {
+ case PROGRESS_VACUUM_PHASE_SCAN_HEAP:
+ case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
+ errcbarg->relname = RelationGetRelationName(rel);
+ errcbarg->indname = NULL; /* Not used for heap */
+ break;
+
+ case PROGRESS_VACUUM_PHASE_VACUUM_INDEX:
+ case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
+ /* indname is the index being processed,
relname is its relation */
+ errcbarg->indname = RelationGetRelationName(rel);
+ errcbarg->relname =
get_rel_name(rel->rd_index->indexrelid);
* 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.
* rel->rd_index->indexrelid should be rel->rd_index->indrelid.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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. -- Justin
Attachment
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
On Mon, Feb 17, 2020 at 10:47:47AM +0900, Masahiko Sawada wrote:
> Thank you for updating the patch!
>
> 1.
> The above lines need a new line.
Done, thanks.
> 2.
> 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.
Right. Maybe I did it the other way because the two uses of
PROGRESS_VACUUM_PHASE_VACUUM_HEAP were right next to each other.
> 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.
I don't have strong feelings either way.
I looked a bit at the truncation phase. It also truncates the FSM and VM
forks, which could be misleading if the error was in one of those files and not
the main filenode.
I'd have to find a way to test it...
...and was pleasantly surprised to see that earlier phases don't choke if I
(re)create a fake 4GB table like:
postgres=# CREATE TABLE trunc(i int);
CREATE TABLE
postgres=# \x\t
Expanded display is on.
Tuples only is on.
postgres=# SELECT relfilenode FROM pg_class WHERE oid='trunc'::regclass;
relfilenode | 59068
postgres=# \! bash -xc 'truncate -s 1G ./pgsql13.dat111/base/12689/59068{,.{1..3}}'
+ truncate -s 1G ./pgsql13.dat111/base/12689/59074 ./pgsql13.dat111/base/12689/59074.1
./pgsql13.dat111/base/12689/59074.2./pgsql13.dat111/base/12689/59074.3
postgres=# \timing
Timing is on.
postgres=# SET client_min_messages=debug; SET statement_timeout='13s'; VACUUM VERBOSE trunc;
INFO: vacuuming "public.trunc"
INFO: "trunc": found 0 removable, 0 nonremovable row versions in 524288 out of 524288 pages
DETAIL: 0 dead row versions cannot be removed yet, oldest xmin: 2098
There were 0 unused item identifiers.
Skipped 0 pages due to buffer pins, 0 frozen pages.
524288 pages are entirely empty.
CPU: user: 5.00 s, system: 1.50 s, elapsed: 6.52 s.
ERROR: canceling statement due to statement timeout
CONTEXT: while truncating relation "public.trunc" to 0 blocks
The callback surrounding RelationTruncate() seems hard to hit unless you add
CHECK_FOR_INTERRUPTS(); the same was true for index cleanup.
The truncation uses a prefetch, which is more likely to hit any lowlevel error,
so I added callback there, too.
BTW, for the index cases, I didn't like repeating the namespace here, but WDYT ?
|CONTEXT: while vacuuming index "public.t_i_idx3" of relation "t"
Thanks for rerere-reviewing.
--
Justin
Attachment
On Mon, 17 Feb 2020 at 12:57, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Feb 17, 2020 at 10:47:47AM +0900, Masahiko Sawada wrote:
> > Thank you for updating the patch!
> >
Thank you for updating the patch.
> > 1.
> > The above lines need a new line.
>
> Done, thanks.
>
> > 2.
> > 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.
>
> Right. Maybe I did it the other way because the two uses of
> PROGRESS_VACUUM_PHASE_VACUUM_HEAP were right next to each other.
>
> > 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.
>
> I don't have strong feelings either way.
>
> I looked a bit at the truncation phase. It also truncates the FSM and VM
> forks, which could be misleading if the error was in one of those files and not
> the main filenode.
>
> I'd have to find a way to test it...
> ...and was pleasantly surprised to see that earlier phases don't choke if I
> (re)create a fake 4GB table like:
>
> postgres=# CREATE TABLE trunc(i int);
> CREATE TABLE
> postgres=# \x\t
> Expanded display is on.
> Tuples only is on.
> postgres=# SELECT relfilenode FROM pg_class WHERE oid='trunc'::regclass;
> relfilenode | 59068
>
> postgres=# \! bash -xc 'truncate -s 1G ./pgsql13.dat111/base/12689/59068{,.{1..3}}'
> + truncate -s 1G ./pgsql13.dat111/base/12689/59074 ./pgsql13.dat111/base/12689/59074.1
./pgsql13.dat111/base/12689/59074.2./pgsql13.dat111/base/12689/59074.3
>
> postgres=# \timing
> Timing is on.
> postgres=# SET client_min_messages=debug; SET statement_timeout='13s'; VACUUM VERBOSE trunc;
> INFO: vacuuming "public.trunc"
> INFO: "trunc": found 0 removable, 0 nonremovable row versions in 524288 out of 524288 pages
> DETAIL: 0 dead row versions cannot be removed yet, oldest xmin: 2098
> There were 0 unused item identifiers.
> Skipped 0 pages due to buffer pins, 0 frozen pages.
> 524288 pages are entirely empty.
> CPU: user: 5.00 s, system: 1.50 s, elapsed: 6.52 s.
> ERROR: canceling statement due to statement timeout
> CONTEXT: while truncating relation "public.trunc" to 0 blocks
>
Yeah lazy_scan_heap deals with all dummy files as new empty pages.
> The callback surrounding RelationTruncate() seems hard to hit unless you add
> CHECK_FOR_INTERRUPTS(); the same was true for index cleanup.
>
> The truncation uses a prefetch, which is more likely to hit any lowlevel error,
> so I added callback there, too.
>
> BTW, for the index cases, I didn't like repeating the namespace here, but WDYT ?
> |CONTEXT: while vacuuming index "public.t_i_idx3" of relation "t"
The current message looks good to me because we cannot have a table
and its index in the different schema.
1.
pg_rusage_init(&ru0);
npages = 0;
/*
* Setup error traceback support for ereport()
*/
init_vacuum_error_callback(&errcallback, &errcbarg, onerel,
PROGRESS_VACUUM_PHASE_VACUUM_HEAP);
error_context_stack = &errcallback;
tupindex = 0;
Oops it seems to me that it's better to set error context after
tupindex = 0. Sorry for my bad.
And the above comment can be written in a single line as other same comments.
2.
@@ -2568,6 +2643,12 @@ count_nondeletable_pages(Relation onerel,
LVRelStats *vacrelstats)
BlockNumber blkno;
BlockNumber prefetchedUntil;
instr_time starttime;
+ ErrorContextCallback errcallback;
+ vacuum_error_callback_arg errcbarg;
+
+ /* Setup error traceback support for ereport() */
+ init_vacuum_error_callback(&errcallback, &errcbarg, onerel,
+ PROGRESS_VACUUM_PHASE_TRUNCATE);
Hmm I don't think it's a good idea to have count_nondeletable_pages
set the error context of PHASE_TRUNCATE. Because the patch sets the
error context during RelationTruncate that actually truncates the heap
but count_nondeletable_pages doesn't truncate it. I think setting the
error context only during RelationTruncate would be a good start. We
can hear other opinions from other hackers. Some hackers may want to
set the error context for whole lazy_truncate_heap.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Feb 17, 2020 at 02:18:11PM +0900, Masahiko Sawada wrote: > Oops it seems to me that it's better to set error context after > tupindex = 0. Sorry for my bad. I take your point but did it differently - see what you think > And the above comment can be written in a single line as other same comments. Thanks :) > Hmm I don't think it's a good idea to have count_nondeletable_pages > set the error context of PHASE_TRUNCATE. I think if we don't do it there then we shouldn't bother handling PHASE_TRUNCATE at all, since that's what's likely to hit filesystem or other lowlevel errors, before lazy_truncate_heap() hits them. > Because the patch sets the > error context during RelationTruncate that actually truncates the heap > but count_nondeletable_pages doesn't truncate it. I would say that ReadBuffer called by the prefetch in count_nondeletable_pages() is called during the course of truncation, the same as ReadBuffer called during the course of vacuuming can be attributed to vacuuming. > I think setting the error context only during RelationTruncate would be a > good start. We can hear other opinions from other hackers. Some hackers may > want to set the error context for whole lazy_truncate_heap. I avoided doing that since it has several "return" statements, each of which would need to "Pop the error context stack", which is at risk of being forgotten and left unpopped by anyone who adds or changes flow control. Also, I just added this to the TRUNCATE case, even though that should never happen: if (BlockNumberIsValid(cbarg->blkno))... -- Justin
Attachment
On Mon, 17 Feb 2020 at 15:14, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Mon, Feb 17, 2020 at 02:18:11PM +0900, Masahiko Sawada wrote: > > Oops it seems to me that it's better to set error context after > > tupindex = 0. Sorry for my bad. > > I take your point but did it differently - see what you think > > > And the above comment can be written in a single line as other same comments. > > Thanks :) > > > Hmm I don't think it's a good idea to have count_nondeletable_pages > > set the error context of PHASE_TRUNCATE. > > I think if we don't do it there then we shouldn't bother handling > PHASE_TRUNCATE at all, since that's what's likely to hit filesystem or other > lowlevel errors, before lazy_truncate_heap() hits them. > > > Because the patch sets the > > error context during RelationTruncate that actually truncates the heap > > but count_nondeletable_pages doesn't truncate it. > > I would say that ReadBuffer called by the prefetch in > count_nondeletable_pages() is called during the course of truncation, the same > as ReadBuffer called during the course of vacuuming can be attributed to > vacuuming. Why do we want to include only count_nondeletable_pages in spite of that there are also several places where we may wait: waiting for lock, get the number of blocks etc. User may cancel vacuum during them but user will not be able to know that vacuum is in truncation phase. If we want to set the error callback during operation that actually doesn't truncate heap like count_nondeletable_pages we should set it for whole lazy_truncate_heap. Otherwise I think we should set it for only RelationTruncate. > > > I think setting the error context only during RelationTruncate would be a > > good start. We can hear other opinions from other hackers. Some hackers may > > want to set the error context for whole lazy_truncate_heap. > > I avoided doing that since it has several "return" statements, each of which > would need to "Pop the error context stack", which is at risk of being > forgotten and left unpopped by anyone who adds or changes flow control. I imagined that we can add some goto and pop the error callback there. But since it might make the code bad I suggested to set the error callback for only RelationTruncate as the first step Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Rebased on top of 007491979461ff10d487e1da9bcc87f2fd834f26 Also, I was thinking that lazy_scan_heap doesn't needs to do this: + /* Pop the error context stack while calling vacuum */ + error_context_stack = errcallback.previous; ... + /* Set the error context while continuing heap scan */ + error_context_stack = &errcallback; It seems to me that's not actually necessary, since lazy_vacuum_heap will just *push* a context handler onto the stack, and then pop it back off. We don't need to pop our context beforehand. We also vacuum the FSM, and one might say that we shouldn't report "...while scanning block number..." if it was "vacuuming FSM" instead of "scanning heap", to which I would reply that either: vacuuming FSM could be considered a part of scanning heap?? Or, maybe we should add an additional callback for that, which is only not very nice since we'd need to add a PROGRESS enum for which we don't actually report PROGRESS (or stop using that enum). I tested using variations on this that works as expected, that context is correct during vacuum while scanning and after vacuum while scanning: template1=# SET statement_timeout=0; SET maintenance_work_mem='1MB'; DROP TABLE tt; CREATE UNLOGGED TABLE tt(i int); INSERTINTO tt SELECT generate_series(1,399999); CREATE INDEX ON tt(i); UPDATE tt SET i=i-1; SET statement_timeout=1222; VACUUMVERBOSE tt;
Attachment
This is, by far, the most complex error context callback we've tried to
write ... Easy stuff first:
In the error context function itself, you don't need the _() around the
strings: errcontext() is marked as a gettext trigger and it does the
translation itself, so the manually added _() is just cruft.
When reporting index names, make sure to attach the namespace to the
table, not to the index. Example:
case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
- errcontext(_("while cleaning up index \"%s.%s\" of relation \"%s\""),
- cbarg->relnamespace, cbarg->indname, cbarg->relname);
+ errcontext("while cleaning up index \"%s\" of relation \"%s.%s\"",
+ cbarg->indname, cbarg->relnamespace, cbarg->relname);
I think it would be worthwhile to have the "truncate wait" phase as a
separate thing from the truncate itself, since it requires acquiring a
possibly taken lock. This suggests that using the progress enum is not
a 100% solution ... or maybe it suggests that the progress enum too
needs to report the truncate-wait phase separately. (I like the latter
myself, actually.)
On 2020-Feb-19, Justin Pryzby wrote:
> Also, I was thinking that lazy_scan_heap doesn't needs to do this:
>
> + /* Pop the error context stack while calling vacuum */
> + error_context_stack = errcallback.previous;
> ...
> + /* Set the error context while continuing heap scan */
> + error_context_stack = &errcallback;
>
> It seems to me that's not actually necessary, since lazy_vacuum_heap will just
> *push* a context handler onto the stack, and then pop it back off.
So if you don't pop before pushing, you'll end up with two context
lines, right?
I find that arrangement a bit confusing. I think it would make sense to
initialize the context callback just *once* for a vacuum run, and from
that point onwards, just update the errcbarg struct to match what
you're currently doing -- not continually pop/push error callback stack
entries. See below ...
(This means you need to pass the "cbarg" as new argument to some of the
called functions, so that they can update it.)
Another point is that this patch seems to be leaking memory each time
you set relation/index/namespace name, since you never free those and
they are changed over and over.
In init_vacuum_error_callback() you don't need the "switch(phase)" bit;
instead, test rel->rd_rel->relkind, and if it's RELKIND_INDEX then you
put the relname as indexname, otherwise set it to NULL (after freeing
the previous value, if there's one). Note that with this, you only need
to set the relation name (table name) in the first call! IOW you should
split init_vacuum_error_callback() in two functions: one "init" to call
at start of vacuum, where you set relnamespace and relname; the other
function is update_vacuum_error_callback() (or you find a better name
for that) and it sets the phase, and optionally the block number and
index name (these last two get reset to InvalidBlkNum/ NULL if not
passed by caller). I'm not really sure what this means for the parallel
index vacuuming stuff; probably you'll need a special case for that: the
parallel children will need to "init" on their own, right?
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Another point is that this patch seems to be leaking memory each time
> you set relation/index/namespace name, since you never free those and
> they are changed over and over.
One other point is that this code seems to be trying to ensure that
the error context callback itself won't need to touch the catalog cache or
relcache, which is an important safety feature ... but it's failing at
that goal, because RelationGetRelationName() is going to hand back a
pointer to a string in the relcache. You need another pstrdup for that.
regards, tom lane
On Thu, Feb 20, 2020 at 02:02:36PM -0300, Alvaro Herrera wrote: > On 2020-Feb-19, Justin Pryzby wrote: > > > Also, I was thinking that lazy_scan_heap doesn't needs to do this: > > > > + /* Pop the error context stack while calling vacuum */ > > + error_context_stack = errcallback.previous; > > ... > > + /* Set the error context while continuing heap scan */ > > + error_context_stack = &errcallback; > > > > It seems to me that's not actually necessary, since lazy_vacuum_heap will just > > *push* a context handler onto the stack, and then pop it back off. > > So if you don't pop before pushing, you'll end up with two context > lines, right? Hm, looks like you're right, but that's not what I intended (and I didn't hit that in my test). > I think it would make sense to > initialize the context callback just *once* for a vacuum run, and from > that point onwards, just update the errcbarg struct to match what > you're currently doing -- not continually pop/push error callback stack > entries. See below ... Originally, the patch only supported "scanning heap", and set the callback strictly, to avoid having callback installed when calling other functions (like vacuuming heap/indexes). Then incrementally added callbacks in increasing number of places. We only need one errcontext. And possibly you're right that the callback could always be in place (?). But what about things like vacuuming FSM ? I think we'd need another "phase" for that (or else invent a PHASE_IGNORE to do nothing). Would VACUUM_FSM be added to progress reporting, too? We're also talking about new phase for TRUNCATE_PREFETCH and TRUNCATE_WAIT. Regarding the cbarg, at one point I took a suggestion from Andres to use the LVRelStats struct. I got rid of that since I didn't like sharing "blkno" between heap scanning and heap vacuuming, and needs to be reset when switching back to scanning heap. I experimented now going back to that now. The only utility is in having an single allocation of relname/space. -- Justin
Attachment
On 2020-Feb-27, Justin Pryzby wrote:
> Originally, the patch only supported "scanning heap", and set the callback
> strictly, to avoid having callback installed when calling other functions (like
> vacuuming heap/indexes).
>
> Then incrementally added callbacks in increasing number of places. We only
> need one errcontext. And possibly you're right that the callback could always
> be in place (?). But what about things like vacuuming FSM ? I think we'd need
> another "phase" for that (or else invent a PHASE_IGNORE to do nothing). Would
> VACUUM_FSM be added to progress reporting, too? We're also talking about new
> phase for TRUNCATE_PREFETCH and TRUNCATE_WAIT.
I think we should use a separate enum. It's simple enough, and there's
no reason to use the same enum for two different things if it seems to
complicate matters.
> Regarding the cbarg, at one point I took a suggestion from Andres to use the
> LVRelStats struct. I got rid of that since I didn't like sharing "blkno"
> between heap scanning and heap vacuuming, and needs to be reset when switching
> back to scanning heap. I experimented now going back to that now. The only
> utility is in having an single allocation of relname/space.
I'm unsure about reusing that struct. Not saying don't do it, just ...
unsure. It possibly has other responsibilities.
I don't think there's a reason to keep 0002 separate.
Regarding this,
> + case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
> + if (BlockNumberIsValid(cbarg->blkno))
> + errcontext("while vacuuming block %u of relation \"%s.%s\"",
> + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> + break;
I think you should still call errcontext() when blkno is invalid. In
fact, just remove the "if" line altogether and let it show whatever
value is there. It should work okay. We don't expect the value to be
invalid anyway.
Maybe it would make sense to make the LVRelStats struct members be char
arrays rather than pointers. Then you memcpy() or strlcpy() them
instead of palloc/free.
Please don't cuddle your braces.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, 21 Feb 2020 at 02:02, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> This is, by far, the most complex error context callback we've tried to
> write ... Easy stuff first:
>
> In the error context function itself, you don't need the _() around the
> strings: errcontext() is marked as a gettext trigger and it does the
> translation itself, so the manually added _() is just cruft.
>
> When reporting index names, make sure to attach the namespace to the
> table, not to the index. Example:
>
> case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
> - errcontext(_("while cleaning up index \"%s.%s\" of relation \"%s\""),
> - cbarg->relnamespace, cbarg->indname, cbarg->relname);
> + errcontext("while cleaning up index \"%s\" of relation \"%s.%s\"",
> + cbarg->indname, cbarg->relnamespace, cbarg->relname);
>
> I think it would be worthwhile to have the "truncate wait" phase as a
> separate thing from the truncate itself, since it requires acquiring a
> possibly taken lock. This suggests that using the progress enum is not
> a 100% solution ... or maybe it suggests that the progress enum too
> needs to report the truncate-wait phase separately. (I like the latter
> myself, actually.)
>
> On 2020-Feb-19, Justin Pryzby wrote:
>
> > Also, I was thinking that lazy_scan_heap doesn't needs to do this:
> >
> > + /* Pop the error context stack while calling vacuum */
> > + error_context_stack = errcallback.previous;
> > ...
> > + /* Set the error context while continuing heap scan */
> > + error_context_stack = &errcallback;
> >
> > It seems to me that's not actually necessary, since lazy_vacuum_heap will just
> > *push* a context handler onto the stack, and then pop it back off.
>
> So if you don't pop before pushing, you'll end up with two context
> lines, right?
>
> I find that arrangement a bit confusing. I think it would make sense to
> initialize the context callback just *once* for a vacuum run, and from
> that point onwards, just update the errcbarg struct to match what
> you're currently doing -- not continually pop/push error callback stack
> entries. See below ...
I was concerned about fsm vacuum; vacuum error context might show heap
scan while actually doing fsm vacuum. But perhaps we can update
callback args for that. That would be helpful for user to distinguish
that the problem seems to be either in heap vacuum or in fsm vacuum.
>
> (This means you need to pass the "cbarg" as new argument to some of the
> called functions, so that they can update it.)
>
> Another point is that this patch seems to be leaking memory each time
> you set relation/index/namespace name, since you never free those and
> they are changed over and over.
>
> In init_vacuum_error_callback() you don't need the "switch(phase)" bit;
> instead, test rel->rd_rel->relkind, and if it's RELKIND_INDEX then you
> put the relname as indexname, otherwise set it to NULL (after freeing
> the previous value, if there's one). Note that with this, you only need
> to set the relation name (table name) in the first call! IOW you should
> split init_vacuum_error_callback() in two functions: one "init" to call
> at start of vacuum, where you set relnamespace and relname; the other
> function is update_vacuum_error_callback() (or you find a better name
> for that) and it sets the phase, and optionally the block number and
> index name (these last two get reset to InvalidBlkNum/ NULL if not
> passed by caller). I'm not really sure what this means for the parallel
> index vacuuming stuff; probably you'll need a special case for that: the
> parallel children will need to "init" on their own, right?
Right. In that case, I think parallel vacuum worker needs to init the
callback args at parallel_vacuum_main(). Other functions that parallel
vacuum worker could call are also called by the leader process.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Feb 27, 2020 at 09:09:42PM -0300, Alvaro Herrera wrote:
> > + case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
> > + if (BlockNumberIsValid(cbarg->blkno))
> > + errcontext("while vacuuming block %u of relation \"%s.%s\"",
> > + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> > + break;
>
> I think you should still call errcontext() when blkno is invalid.
In my experience while testing, the conditional avoids lots of CONTEXT noise
from interrupted autovacuum, at least. I couldn't easily reproduce it with the
current patch, though, maybe due to less pushing and popping.
> Maybe it would make sense to make the LVRelStats struct members be char
> arrays rather than pointers. Then you memcpy() or strlcpy() them
> instead of palloc/free.
I had done that in the v15 patch, to allow passing it to parallel workers.
But I don't think it's really needed.
On Tue, Mar 03, 2020 at 10:05:42PM +0900, Masahiko Sawada wrote:
> I was concerned about fsm vacuum; vacuum error context might show heap
> scan while actually doing fsm vacuum. But perhaps we can update
> callback args for that. That would be helpful for user to distinguish
> that the problem seems to be either in heap vacuum or in fsm vacuum.
Done in the attached. But I think non-error reporting of additional progress
phases is out of scope for this patch.
> On Fri, 21 Feb 2020 at 02:02, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > parallel children will need to "init" on their own, right?
> Right. In that case, I think parallel vacuum worker needs to init the
> callback args at parallel_vacuum_main(). Other functions that parallel
> vacuum worker could call are also called by the leader process.
In the previous patch, I added this to vacuum_one_index. But I noticed that
sometimes reported multiple CONTEXT lines (while vacuuming..while scanning),
which isn't intended. I was hacked around that by setting ->previous=NULL, but
your way in parallel main() seems better.
--
Justin
Attachment
On 2020-Mar-03, Justin Pryzby wrote:
> On Thu, Feb 27, 2020 at 09:09:42PM -0300, Alvaro Herrera wrote:
> > > + case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
> > > + if (BlockNumberIsValid(cbarg->blkno))
> > > + errcontext("while vacuuming block %u of relation \"%s.%s\"",
> > > + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> > > + break;
> >
> > I think you should still call errcontext() when blkno is invalid.
>
> In my experience while testing, the conditional avoids lots of CONTEXT noise
> from interrupted autovacuum, at least. I couldn't easily reproduce it with the
> current patch, though, maybe due to less pushing and popping.
I think you're saying that the code had the bug that too many lines were
reported because of excessive stack pushes, and you worked around it by
making the errcontext() be conditional; and that now the bug is fixed by
avoiding the push/pop games -- which explains why you can no longer
reproduce it. I don't see why you want to keep the no-longer-needed
workaround.
Your use of the progress-report enum now has two warts -- the "-1"
value, and this one,
> +#define PROGRESS_VACUUM_PHASE_VACUUM_FSM 7 /* For error reporting only */
I'd rather you define a new enum, in lazyvacuum.c.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, 4 Mar 2020 at 04:32, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Feb 27, 2020 at 09:09:42PM -0300, Alvaro Herrera wrote:
> > > + case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
> > > + if (BlockNumberIsValid(cbarg->blkno))
> > > + errcontext("while vacuuming block %u of relation \"%s.%s\"",
> > > + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> > > + break;
> >
> > I think you should still call errcontext() when blkno is invalid.
>
> In my experience while testing, the conditional avoids lots of CONTEXT noise
> from interrupted autovacuum, at least. I couldn't easily reproduce it with the
> current patch, though, maybe due to less pushing and popping.
>
> > Maybe it would make sense to make the LVRelStats struct members be char
> > arrays rather than pointers. Then you memcpy() or strlcpy() them
> > instead of palloc/free.
>
> I had done that in the v15 patch, to allow passing it to parallel workers.
> But I don't think it's really needed.
>
> On Tue, Mar 03, 2020 at 10:05:42PM +0900, Masahiko Sawada wrote:
> > I was concerned about fsm vacuum; vacuum error context might show heap
> > scan while actually doing fsm vacuum. But perhaps we can update
> > callback args for that. That would be helpful for user to distinguish
> > that the problem seems to be either in heap vacuum or in fsm vacuum.
>
> Done in the attached. But I think non-error reporting of additional progress
> phases is out of scope for this patch.
Thank you for updating the patch. But we have two more places where we
do fsm vacuum.
/*
* Periodically do incremental FSM vacuuming to make newly-freed
* space visible on upper FSM pages. Note: although we've cleaned
* the current block, we haven't yet updated its FSM entry (that
* happens further down), so passing end == blkno is correct.
*/
if (blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
{
FreeSpaceMapVacuumRange(onerel, next_fsm_block_to_vacuum,
blkno);
next_fsm_block_to_vacuum = blkno;
and
/*
* Vacuum the remainder of the Free Space Map. We must do this whether or
* not there were indexes.
*/
if (blkno > next_fsm_block_to_vacuum)
FreeSpaceMapVacuumRange(onerel, next_fsm_block_to_vacuum, blkno);
---
static void vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats,
LVShared *lvshared, LVSharedIndStats
*shared_indstats,
- LVDeadTuples *dead_tuples);
+ LVDeadTuples *dead_tuples, LVRelStats
*vacrelstats);
static void lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
- LVDeadTuples *dead_tuples, double reltuples);
+ LVDeadTuples *dead_tuples, double
reltuples, LVRelStats *vacrelstats);
These functions have LVDeadTuples and LVRelStats but LVDeadTuples can
be referred by LVRelStats. If we want to use LVRelStats as callback
argument, we can remove function arguments that can be referred by
LVRelStats.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Mar 04, 2020 at 04:21:06PM +0900, Masahiko Sawada wrote: > Thank you for updating the patch. But we have two more places where we > do fsm vacuum. Oops, thanks. I realized that vacuum_page is called not only from lazy_vacuum_heap, but also directly from lazy_scan_heap, which failed to update errcbarg. So I changed to update errcbarg in vacuum_page. What about these other calls ? I think granularity of individual function calls requires a debugger, but is it fine issue if errors here are attributed to (say) "scanning heap" ? GetRecordedFreeSpace heap_*_freeze_tuple heap_page_prune HeapTupleSatisfiesVacuum LockBufferForCleanup MarkBufferDirty Page*AllVisible PageGetHeapFreeSpace RecordPageWithFreeSpace visibilitymap_* VM_ALL_FROZEN > These functions have LVDeadTuples and LVRelStats but LVDeadTuples can > be referred by LVRelStats. If we want to use LVRelStats as callback > argument, we can remove function arguments that can be referred by > LVRelStats. That doesn't work easily with parallel vacuum, which passes not vacrelstats->dead_tuples, but a dead_tuples pulled out of shm_toc. But it was easy enough to remove "reltuples". -- Justin
Attachment
On Tue, Mar 03, 2020 at 04:49:00PM -0300, Alvaro Herrera wrote:
> On 2020-Mar-03, Justin Pryzby wrote:
> > On Thu, Feb 27, 2020 at 09:09:42PM -0300, Alvaro Herrera wrote:
> > > > + case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
> > > > + if (BlockNumberIsValid(cbarg->blkno))
> > > > + errcontext("while vacuuming block %u of relation \"%s.%s\"",
> > >
> > > I think you should still call errcontext() when blkno is invalid.
> >
> > In my experience while testing, the conditional avoids lots of CONTEXT noise
> > from interrupted autovacuum, at least. I couldn't easily reproduce it with the
> > current patch, though, maybe due to less pushing and popping.
>
> I think you're saying that the code had the bug that too many lines were
> reported because of excessive stack pushes, and you worked around it by
> making the errcontext() be conditional; and that now the bug is fixed by
> avoiding the push/pop games -- which explains why you can no longer
> reproduce it. I don't see why you want to keep the no-longer-needed
> workaround.
No - the issue I observed from autovacuum ("while scanning block number
4294967295") was unrelated to showing multiple context lines (that issue I only
saw in the v22 patch, and was related to vacuum_one_index being used by both
leader and parallel workers).
The locations showing a block number first set vacrelstats->blkno to
InvalidBlockNumber, and then later update the vacrelstats->blkno from a loop
variable. I think today's v24 patch makes it harder to hit the window where
it's set to InvalidBlockNumber, by moving VACUUM_HEAP context into
vacuum_page(), but I don't think we should rely on an absence of
CHECK_FOR_INTERRUPTS() to avoid misleading noise context.
--
Justin
On Thu, Mar 5, 2020 at 3:22 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Wed, Mar 04, 2020 at 04:21:06PM +0900, Masahiko Sawada wrote: > > Thank you for updating the patch. But we have two more places where we > > do fsm vacuum. > > Oops, thanks. > > I realized that vacuum_page is called not only from lazy_vacuum_heap, but also > directly from lazy_scan_heap, which failed to update errcbarg. So I changed to > update errcbarg in vacuum_page. > > What about these other calls ? I think granularity of individual function > calls requires a debugger, but is it fine issue if errors here are attributed > to (say) "scanning heap" ? > > GetRecordedFreeSpace > heap_*_freeze_tuple > heap_page_prune > HeapTupleSatisfiesVacuum > LockBufferForCleanup > MarkBufferDirty > Page*AllVisible > PageGetHeapFreeSpace > RecordPageWithFreeSpace > visibilitymap_* > VM_ALL_FROZEN > I think we can keep granularity the same as we have for progress update functionality which means "scanning heap" is fine. On similar lines, it is not clear whether it is a good idea to keep a phase like VACUUM_ERRCB_PHASE_VACUUM_FSM as it has added additional updates in multiple places in the code. Few other comments: 1. + /* Init vacrelstats for use as error callback by parallel worker: */ + vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); It looks a bit odd that the comment is ended with semicolon (:), is there any reason for same? 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; .. .. + /* Init vacrelstats for use as error callback by parallel worker: */ + vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); + vacrelstats.relname = pstrdup(RelationGetRelationName(onerel)); + vacrelstats.indname = NULL; + vacrelstats.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */ + + /* Setup error traceback support for ereport() */ + errcallback.callback = vacuum_error_callback; + errcallback.arg = &vacrelstats; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; + I think the code can be bit simplified if we have a function setup_vacuum_error_ctx which takes necessary parameters and fill the required vacrelstats params, setup errcallback. Then we can use update_vacuum_error_cbarg at required places. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On 2020-Mar-16, Amit Kapila wrote: > 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; > .. > .. > + /* Init vacrelstats for use as error callback by parallel worker: */ > + vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); > + vacrelstats.relname = pstrdup(RelationGetRelationName(onerel)); > + vacrelstats.indname = NULL; > + vacrelstats.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */ > + > + /* Setup error traceback support for ereport() */ > + errcallback.callback = vacuum_error_callback; > + errcallback.arg = &vacrelstats; > + errcallback.previous = error_context_stack; > + error_context_stack = &errcallback; > + > > I think the code can be bit simplified if we have a function > setup_vacuum_error_ctx which takes necessary parameters and fill the > required vacrelstats params, setup errcallback. Then we can use > update_vacuum_error_cbarg at required places. Heh, he had that and I took it away -- it looked unnatural. I thought changing error_context_stack inside such a function, then resetting it back to "previous" outside the function, was too leaky an abstraction. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Mar 16, 2020 at 7:47 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2020-Mar-16, Amit Kapila wrote: > > > 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; > > .. > > .. > > + /* Init vacrelstats for use as error callback by parallel worker: */ > > + vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(onerel)); > > + vacrelstats.relname = pstrdup(RelationGetRelationName(onerel)); > > + vacrelstats.indname = NULL; > > + vacrelstats.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */ > > + > > + /* Setup error traceback support for ereport() */ > > + errcallback.callback = vacuum_error_callback; > > + errcallback.arg = &vacrelstats; > > + errcallback.previous = error_context_stack; > > + error_context_stack = &errcallback; > > + > > > > I think the code can be bit simplified if we have a function > > setup_vacuum_error_ctx which takes necessary parameters and fill the > > required vacrelstats params, setup errcallback. Then we can use > > update_vacuum_error_cbarg at required places. > > Heh, he had that and I took it away -- it looked unnatural. I thought > changing error_context_stack inside such a function, then resetting it > back to "previous" outside the function, was too leaky an abstraction. > We could have something like setup_parser_errposition_callback and cancel_parser_errposition_callback which might look a bit better. I thought to avoid having similar code at different places and it might look a bit cleaner especially because we are adding code to an already large function like lazy_scan_heap(), but if you don't like the idea, then we can leave it as it is. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 03, 2020 at 10:05:42PM +0900, Masahiko Sawada wrote: > I was concerned about fsm vacuum; vacuum error context might show heap > scan while actually doing fsm vacuum. But perhaps we can update > callback args for that. That would be helpful for user to distinguish > that the problem seems to be either in heap vacuum or in fsm vacuum. On Tue, Mar 03, 2020 at 04:49:00PM -0300, Alvaro Herrera wrote: > Your use of the progress-report enum now has two warts -- the "-1" > value, and this one, > > > +#define PROGRESS_VACUUM_PHASE_VACUUM_FSM 7 /* For error reporting only */ > > I'd rather you define a new enum, in lazyvacuum.c. On Mon, Mar 16, 2020 at 11:44:25AM +0530, Amit Kapila wrote: > > On Wed, Mar 04, 2020 at 04:21:06PM +0900, Masahiko Sawada wrote: > > > Thank you for updating the patch. But we have two more places where we > > > do fsm vacuum. > > > > Oops, thanks. ... > it is not clear whether it is a good idea to keep a phase like > VACUUM_ERRCB_PHASE_VACUUM_FSM as it has added additional updates in > multiple places in the code. I think you're suggesting to rip out VACUUM_ERRCB_PHASE_VACUUM_FSM, and allow reporting any errors there with an error context like "while scanning heap". An alternative in the three places using VACUUM_ERRCB_PHASE_VACUUM_FSM is to set: |phase = VACUUM_ERRCB_PHASE_UNKNOWN; to avoid reporting any error context until another phase is set. -- Justin
On Tue, Mar 17, 2020 at 9:21 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Tue, Mar 03, 2020 at 10:05:42PM +0900, Masahiko Sawada wrote: > > I was concerned about fsm vacuum; vacuum error context might show heap > > scan while actually doing fsm vacuum. But perhaps we can update > > callback args for that. That would be helpful for user to distinguish > > that the problem seems to be either in heap vacuum or in fsm vacuum. > > On Tue, Mar 03, 2020 at 04:49:00PM -0300, Alvaro Herrera wrote: > > Your use of the progress-report enum now has two warts -- the "-1" > > value, and this one, > > > > > +#define PROGRESS_VACUUM_PHASE_VACUUM_FSM 7 /* For error reporting only */ > > > > I'd rather you define a new enum, in lazyvacuum.c. > > On Mon, Mar 16, 2020 at 11:44:25AM +0530, Amit Kapila wrote: > > > On Wed, Mar 04, 2020 at 04:21:06PM +0900, Masahiko Sawada wrote: > > > > Thank you for updating the patch. But we have two more places where we > > > > do fsm vacuum. > > > > > > Oops, thanks. > ... > > it is not clear whether it is a good idea to keep a phase like > > VACUUM_ERRCB_PHASE_VACUUM_FSM as it has added additional updates in > > multiple places in the code. > > I think you're suggesting to rip out VACUUM_ERRCB_PHASE_VACUUM_FSM, and allow > reporting any errors there with an error context like "while scanning heap". > Right, because that is what we do for progress updates. > An alternative in the three places using VACUUM_ERRCB_PHASE_VACUUM_FSM is to > set: > > |phase = VACUUM_ERRCB_PHASE_UNKNOWN; > > to avoid reporting any error context until another phase is set. > Right, that is an alternative, but not sure if it is worth adding additional code. I am trying to see if we can get this functionality without adding code at too many places primarily because the code in this area is already complex, so adding more things can make it difficult to understand. Another minor point, don't we need to remove the error stack by doing "error_context_stack = errcallback.previous;" in parallel_vacuum_main? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 17, 2020 at 9:52 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > Another minor point, don't we need to remove the error stack by doing > "error_context_stack = errcallback.previous;" in parallel_vacuum_main? > Few other comments: 1. The error in lazy_vacuum_heap can either have phase VACUUM_ERRCB_PHASE_INDEX_* or VACUUM_ERRCB_PHASE_VACUUM_HEAP depending on when it occurs. If it occurs the first time it enters that function before a call to lazy_vacuum_page, it will use phase VACUUM_ERRCB_PHASE_INDEX_*, otherwise, it would use VACUUM_ERRCB_PHASE_VACUUM_HEAP. The reason is lazy_vacuum_index or lazy_cleanup_index won't reset the phase after leaving that function. 2. Also once we set phase as VACUUM_ERRCB_PHASE_VACUUM_HEAP via lazy_vacuum_page, it doesn't seem to be reset to VACUUM_ERRCB_PHASE_SCAN_HEAP even when we do scanning of the heap. I think you need to set phase VACUUM_ERRCB_PHASE_SCAN_HEAP inside loop. I think we need to be a bit more careful in setting/resetting the phase information correctly so that it doesn't display the wrong info in the context in an error message. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 17, 2020 at 11:51 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Mar 17, 2020 at 9:52 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > Another minor point, don't we need to remove the error stack by doing > > "error_context_stack = errcallback.previous;" in parallel_vacuum_main? > > > > Few other comments: > 1. The error in lazy_vacuum_heap can either have phase > VACUUM_ERRCB_PHASE_INDEX_* or VACUUM_ERRCB_PHASE_VACUUM_HEAP depending > on when it occurs. If it occurs the first time it enters that > function before a call to lazy_vacuum_page, it will use phase > VACUUM_ERRCB_PHASE_INDEX_*, otherwise, it would use > VACUUM_ERRCB_PHASE_VACUUM_HEAP. The reason is lazy_vacuum_index or > lazy_cleanup_index won't reset the phase after leaving that function. > > 2. Also once we set phase as VACUUM_ERRCB_PHASE_VACUUM_HEAP via > lazy_vacuum_page, it doesn't seem to be reset to > VACUUM_ERRCB_PHASE_SCAN_HEAP even when we do scanning of the heap. I > think you need to set phase VACUUM_ERRCB_PHASE_SCAN_HEAP inside loop. > > I think we need to be a bit more careful in setting/resetting the > phase information correctly so that it doesn't display the wrong info > in the context in an error message. > Justin, are you planning to work on the pending comments? If you want, I can try to fix some of these. We have less time left for this CF, so we need to do things a bit quicker. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Thu, Mar 19, 2020 at 08:20:51AM +0530, Amit Kapila wrote: > On Tue, Mar 17, 2020 at 11:51 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Mar 17, 2020 at 9:52 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > Another minor point, don't we need to remove the error stack by doing > > > "error_context_stack = errcallback.previous;" in parallel_vacuum_main? It's a good idea, thanks. > > Few other comments: > > 1. The error in lazy_vacuum_heap can either have phase > > VACUUM_ERRCB_PHASE_INDEX_* or VACUUM_ERRCB_PHASE_VACUUM_HEAP depending > > on when it occurs. If it occurs the first time it enters that > > function before a call to lazy_vacuum_page, it will use phase > > VACUUM_ERRCB_PHASE_INDEX_*, otherwise, it would use > > VACUUM_ERRCB_PHASE_VACUUM_HEAP. The reason is lazy_vacuum_index or > > lazy_cleanup_index won't reset the phase after leaving that function. I think you mean that lazy_vacuum_heap() calls ReadBuffer itself, so needs to be in phase VACUUM_HEAP even before it calls vacuum_page(). > > 2. Also once we set phase as VACUUM_ERRCB_PHASE_VACUUM_HEAP via > > lazy_vacuum_page, it doesn't seem to be reset to > > VACUUM_ERRCB_PHASE_SCAN_HEAP even when we do scanning of the heap. I > > think you need to set phase VACUUM_ERRCB_PHASE_SCAN_HEAP inside loop. You're right. PHASE_SCAN_HEAP was set, but only inside a conditional. Both those issues are due to a change in the most recent patch. In the previous patch, the PHASE_VACUUM_HEAP was set only by lazy_vacuum_heap(), and I moved it recently to vacuum_page. But it needs to be copied, as you point out. That's unfortunate due to a lack of symmetry: lazy_vacuum_page does its own progress update, which suggests to me that it should also set its own error callback. It'd be nicer if EITHER the calling functions did that (scan_heap() and vacuum_heap()) or if it was sufficient for the called function (vacuum_page()) to do it. > > I think we need to be a bit more careful in setting/resetting the > > phase information correctly so that it doesn't display the wrong info > > in the context in an error message. > > Justin, are you planning to work on the pending comments? If you > want, I can try to fix some of these. We have less time left for this > CF, so we need to do things a bit quicker. Thanks for reminding. -- Justin
Attachment
On Thu, Mar 19, 2020 at 9:38 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Thu, Mar 19, 2020 at 08:20:51AM +0530, Amit Kapila wrote: > > > > Few other comments: > > > 1. The error in lazy_vacuum_heap can either have phase > > > VACUUM_ERRCB_PHASE_INDEX_* or VACUUM_ERRCB_PHASE_VACUUM_HEAP depending > > > on when it occurs. If it occurs the first time it enters that > > > function before a call to lazy_vacuum_page, it will use phase > > > VACUUM_ERRCB_PHASE_INDEX_*, otherwise, it would use > > > VACUUM_ERRCB_PHASE_VACUUM_HEAP. The reason is lazy_vacuum_index or > > > lazy_cleanup_index won't reset the phase after leaving that function. > > I think you mean that lazy_vacuum_heap() calls ReadBuffer itself, so needs to > be in phase VACUUM_HEAP even before it calls vacuum_page(). > Right. > > > 2. Also once we set phase as VACUUM_ERRCB_PHASE_VACUUM_HEAP via > > > lazy_vacuum_page, it doesn't seem to be reset to > > > VACUUM_ERRCB_PHASE_SCAN_HEAP even when we do scanning of the heap. I > > > think you need to set phase VACUUM_ERRCB_PHASE_SCAN_HEAP inside loop. > > You're right. PHASE_SCAN_HEAP was set, but only inside a conditional. > I think if we do it inside for loop, then we don't need to set it conditionally at multiple places. I have changed like that in the attached patch, see if that makes sense to you. > Both those issues are due to a change in the most recent patch. In the > previous patch, the PHASE_VACUUM_HEAP was set only by lazy_vacuum_heap(), and I > moved it recently to vacuum_page. But it needs to be copied, as you point out. > > That's unfortunate due to a lack of symmetry: lazy_vacuum_page does its own > progress update, which suggests to me that it should also set its own error > callback. It'd be nicer if EITHER the calling functions did that (scan_heap() > and vacuum_heap()) or if it was sufficient for the called function > (vacuum_page()) to do it. > Right, but adding in callers will spread at multiple places. I have made a few additional changes in the attached. (a) Removed VACUUM_ERRCB_PHASE_VACUUM_FSM as I think we have to add it at many places, you seem to have added for FreeSpaceMapVacuumRange() but not for RecordPageWithFreeSpace(), (b) Reset the phase to VACUUM_ERRCB_PHASE_UNKNOWN after finishing the work for a particular phase, so that the new phase shouldn't continue in the callers. I have another idea to make (b) better. How about if a call to update_vacuum_error_cbarg returns information of old phase (blkno, phase, and indname) along with what it is doing now and then once the work for the current phase is over it can reset it back with old phase information? This way the callee after finishing the new phase work would be able to reset back to the old phase. This will work something similar to our MemoryContextSwitchTo. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Attachment
On Thu, Mar 19, 2020 at 03:18:32PM +0530, Amit Kapila wrote: > > You're right. PHASE_SCAN_HEAP was set, but only inside a conditional. > > I think if we do it inside for loop, then we don't need to set it > conditionally at multiple places. I have changed like that in the > attached patch, see if that makes sense to you. Yes, makes sense, and it's right near pgstat_progress_update_param, which is nice. > > Both those issues are due to a change in the most recent patch. In the > > previous patch, the PHASE_VACUUM_HEAP was set only by lazy_vacuum_heap(), and I > > moved it recently to vacuum_page. But it needs to be copied, as you point out. > > > > That's unfortunate due to a lack of symmetry: lazy_vacuum_page does its own > > progress update, which suggests to me that it should also set its own error > > callback. It'd be nicer if EITHER the calling functions did that (scan_heap() > > and vacuum_heap()) or if it was sufficient for the called function > > (vacuum_page()) to do it. > > Right, but adding in callers will spread at multiple places. > > I have made a few additional changes in the attached. (a) Removed > VACUUM_ERRCB_PHASE_VACUUM_FSM as I think we have to add it at many > places, you seem to have added for FreeSpaceMapVacuumRange() but not > for RecordPageWithFreeSpace(), (b) Reset the phase to > VACUUM_ERRCB_PHASE_UNKNOWN after finishing the work for a particular > phase, so that the new phase shouldn't continue in the callers. > > I have another idea to make (b) better. How about if a call to > update_vacuum_error_cbarg returns information of old phase (blkno, > phase, and indname) along with what it is doing now and then once the > work for the current phase is over it can reset it back with old phase > information? This way the callee after finishing the new phase work > would be able to reset back to the old phase. This will work > something similar to our MemoryContextSwitchTo. 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. As written, we only pfree indname if we do actually "reset" the cbarg, which is in the two routines handling indexes. It's probably a good idea to pass the indname rather than the relation in any case. I rebased the rest of my patches on top of yours. -- Justin
Attachment
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. Which gives me an excuse for resending patch, which is needed since I spent too much time testing this that I failed to update the tip commit for the new argument. > It's probably a good idea to pass the indname rather than the relation in any > case. I included that with 0001. I also fixed the argument name in the prototype (Relation rel vs indrel). And removed these, which were the whole motivation behind saving the values. |Set the error context while continuing heap scan -- Justin
Attachment
On Fri, Mar 20, 2020 at 5:59 AM Justin Pryzby <pryzby@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
On Fri, Mar 20, 2020 at 11:24:25AM +0530, Amit Kapila wrote: > On Fri, Mar 20, 2020 at 5:59 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > 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? It's not needed but a common convention to avoid needing a two-line patch in order to add a line at the end, like: - foo + foo, + bar > 2. update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_SCAN_HEAP, InvalidBlockNumber, NULL); > > Why do we need to call update_vacuum_error_cbarg at the above place > after we have added a new one inside for.. loop? If we're going to update the error_context_stack global point to our callback, without our vacrelstats arg, it'd better be initialized. I changed to do vacrelstats->phase = UNKNOWN after its allocation in heap_vacuum_rel(). That matches parallel_vacuum_main(). > 4. At this and similar places, change the comment to something like: > "Reset the old phase information for error traceback". I did this: /* Revert back to the old phase information for error traceback */ > 5. Subject: [PATCH v28 3/5] Drop reltuples > > Is this patch directly related to the main patch (vacuum errcontext to > show block being processed) or is it an independent improvement of > code? It's a cleanup after implementing the new feature. I left it as a separate patch to make review easier of the essential patch and of the cleanup. See here: https://www.postgresql.org/message-id/CA%2Bfd4k4JA3YkP6-HUqHOqu6cTGqqZUhBfsMqQ4WXkD0Y8uotUg%40mail.gmail.com > 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? The context is that there was a request to add err context for (yet another) phase, TRUNCATE. But I insisted on adding it to prefetch, too, since it does ReadBuffer. But there was an objection that the error might be misleading if it said "while truncating" but it was actually "prefetching to truncate". > 7. Is there a reason to keep the truncate phase patch separate from > the main patch? If not, let's merge them. They were separate since it's the most-recently added part, and (as now) there's still discussion about it. > 8. Can we think of some easy way to add tests for this patch? Is it possible to make an corrupted index which errors during scan during regress tests ? Thanks for looking. -- Justin
Attachment
On Fri, Mar 20, 2020 at 12:21 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Fri, Mar 20, 2020 at 11:24:25AM +0530, Amit Kapila wrote: > > On Fri, Mar 20, 2020 at 5:59 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > 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? > > It's not needed but a common convention to avoid needing a two-line patch in > order to add a line at the end, like: > > - foo > + foo, > + bar > I don't think this is required and we don't have this at other places, so I removed it. Apart from that, I made a few additional changes (a) moved the typedef to a different palace as it was looking odd in-between other struct defines, (b) renamed the enum ErrCbPhase as that suits more to nearby other trypedefs (c) added/edited comments at few places, (d) ran pgindent. See, how the attached looks? I have written a commit message as well, see if I have missed anyone is from the credit list? > > > 8. Can we think of some easy way to add tests for this patch? > > Is it possible to make an corrupted index which errors during scan during > regress tests ? > I don't think so. For now, let's focus on the main patch. Once that is committed, we can look into the other code rearrangement/cleanup patches. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Attachment
On Fri, Mar 20, 2020 at 04:58:08PM +0530, Amit Kapila wrote: > See, how the attached looks? I have written a commit message as well, > see if I have missed anyone is from the credit list? Thanks for looking again. Couple tweaks: +/* Phases of vacuum during which an error can occur. */ Can you say: "during which we report error context" Otherwise it sounds like we've somehow precluded errors from happening anywhere else, which I don't think we can claim. In the commit messsage: |The additional information displayed will be block number for errors |occurred while processing heap and index name for errors occurred |while processing the index. => error occurring |This will help us in diagnosing the problems that occurred during a |vacuum. For ex. due to corruption if we get some error while vacuuming, => problems that occur Maybe it should say that this will help both 1) admins who have corruption due to hardware (say); and, 2) developer's with corruption due to a bug. -- Justin
On Fri, Mar 20, 2020 at 8:24 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Fri, Mar 20, 2020 at 04:58:08PM +0530, Amit Kapila wrote: > > See, how the attached looks? I have written a commit message as well, > > see if I have missed anyone is from the credit list? > > Thanks for looking again. > > Couple tweaks: > I have addressed your comments in the attached patch. Today, while testing error messages from various phases, I noticed that the patch fails to display error context if the error occurs during the truncate phase. The reason was that we had popped the error stack in lazy_scan_heap due to which it never calls the callback. I think we need to set up callback at a higher level as is done in the attached patch. I have done the testing by inducing errors in various phases and it prints the required information. Let me know what you think of the attached? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Attachment
On Sat, Mar 21, 2020 at 01:00:03PM +0530, Amit Kapila wrote: > I have addressed your comments in the attached patch. Today, while > testing error messages from various phases, I noticed that the patch > fails to display error context if the error occurs during the truncate > phase. The reason was that we had popped the error stack in > lazy_scan_heap due to which it never calls the callback. I think we > need to set up callback at a higher level as is done in the attached > patch. I have done the testing by inducing errors in various phases > and it prints the required information. Let me know what you think of > the attached? Thanks. My tests with TRUNCATE were probably back when we had multiple push/pop cycles of local error callbacks. This passes my tests. -- Justin
On Sat, 21 Mar 2020 at 16:30, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Mar 20, 2020 at 8:24 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > On Fri, Mar 20, 2020 at 04:58:08PM +0530, Amit Kapila wrote:
> > > See, how the attached looks? I have written a commit message as well,
> > > see if I have missed anyone is from the credit list?
> >
> > Thanks for looking again.
> >
> > Couple tweaks:
> >
>
> I have addressed your comments in the attached patch. Today, while
> testing error messages from various phases, I noticed that the patch
> fails to display error context if the error occurs during the truncate
> phase. The reason was that we had popped the error stack in
> lazy_scan_heap due to which it never calls the callback. I think we
> need to set up callback at a higher level as is done in the attached
> patch. I have done the testing by inducing errors in various phases
> and it prints the required information. Let me know what you think of
> the attached?
I've looked at the current version patch.
+/* Phases of vacuum during which we report error context. */
+typedef enum
+{
+ VACUUM_ERRCB_PHASE_UNKNOWN,
+ VACUUM_ERRCB_PHASE_SCAN_HEAP,
+ VACUUM_ERRCB_PHASE_VACUUM_INDEX,
+ VACUUM_ERRCB_PHASE_VACUUM_HEAP,
+ VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
+ VACUUM_ERRCB_PHASE_TRUNCATE
+} ErrCbPhase;
I've already commented on earlier patch but I personally think we'd be
better to report freespace map vacuum as a separate phase. The
progress report of vacuum command is used to know the progress but
this error context would be useful for diagnostic of failure such as
disk corruption. For visibility map, I think the visibility map bit
that are processed during vacuum is exactly corresponding to the block
number but since freespace map vacuum processes the range of blocks
I've sometimes had trouble with identifying the cause of the problem.
What do you think?
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Mar 23, 2020 at 04:39:54PM +0900, Masahiko Sawada wrote:
> I've already commented on earlier patch but I personally think we'd be
> better to report freespace map vacuum as a separate phase. The
> progress report of vacuum command is used to know the progress but
> this error context would be useful for diagnostic of failure such as
> disk corruption. For visibility map, I think the visibility map bit
> that are processed during vacuum is exactly corresponding to the block
> number but since freespace map vacuum processes the range of blocks
> I've sometimes had trouble with identifying the cause of the problem.
Yea, and it would be misleading if we reported "while scanning block..of
relation" if we actually failed while writing its FSM.
My previous patches did this:
+ case VACUUM_ERRCB_PHASE_VACUUM_FSM:
+ errcontext("while vacuuming free space map of relation \"%s.%s\"",
+ cbarg->relnamespace, cbarg->relname);
+ break;
Are you suggesting it should report the start (or end?) block number ?
--
Justin
On Mon, Mar 23, 2020 at 1:46 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Mar 23, 2020 at 04:39:54PM +0900, Masahiko Sawada wrote:
> > I've already commented on earlier patch but I personally think we'd be
> > better to report freespace map vacuum as a separate phase. The
> > progress report of vacuum command is used to know the progress but
> > this error context would be useful for diagnostic of failure such as
> > disk corruption. For visibility map, I think the visibility map bit
> > that are processed during vacuum is exactly corresponding to the block
> > number but since freespace map vacuum processes the range of blocks
> > I've sometimes had trouble with identifying the cause of the problem.
>
What extra information we can print that can help? The main problem I
see is that we need to sprinkle errorcallback update function at many
more places. We can think of writing a wrapper function for FSM calls
used in a vacuum, but I think those can be used only for vacuum.
> Yea, and it would be misleading if we reported "while scanning block..of
> relation" if we actually failed while writing its FSM.
>
> My previous patches did this:
>
> + case VACUUM_ERRCB_PHASE_VACUUM_FSM:
> + errcontext("while vacuuming free space map of relation \"%s.%s\"",
> + cbarg->relnamespace, cbarg->relname);
> + break;
>
In what kind of errors will this help?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Sat, Mar 21, 2020 at 1:33 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Sat, Mar 21, 2020 at 01:00:03PM +0530, Amit Kapila wrote: > > I have addressed your comments in the attached patch. Today, while > > testing error messages from various phases, I noticed that the patch > > fails to display error context if the error occurs during the truncate > > phase. The reason was that we had popped the error stack in > > lazy_scan_heap due to which it never calls the callback. I think we > > need to set up callback at a higher level as is done in the attached > > patch. I have done the testing by inducing errors in various phases > > and it prints the required information. Let me know what you think of > > the attached? > > Thanks. My tests with TRUNCATE were probably back when we had multiple > push/pop cycles of local error callbacks. > > This passes my tests. > Today, I have done some additional testing with parallel workers and it seems to display the appropriate errors. See below: postgres=# create table t1(c1 int, c2 char(500), c3 char(500)); CREATE TABLE postgres=# insert into t1 values(generate_series(1,300000),'aaaa', 'bbbb'); INSERT 0 300000 postgres=# delete from t1 where c1 > 200000; DELETE 100000 postgres=# vacuum t1; ERROR: Error induced during index vacuum CONTEXT: while vacuuming index "idx_t1_c3" of relation "public.t1" parallel worker while vacuuming index "idx_t1_c2" of relation "public.t1" Here, you can see that the index names displayed in two messages are different, basically, the leader backend got the error generated in worker when it was vacuuming the other index. I have used the attached patch to induce error. I think the patch is in good shape now and I am happy with it. We can think of proceeding with this unless we want the further enhancement for FSM which I am not sure is a good idea. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Attachment
On Mon, Mar 23, 2020 at 02:25:14PM +0530, Amit Kapila wrote:
> > Yea, and it would be misleading if we reported "while scanning block..of
> > relation" if we actually failed while writing its FSM.
> >
> > My previous patches did this:
> >
> > + case VACUUM_ERRCB_PHASE_VACUUM_FSM:
> > + errcontext("while vacuuming free space map of relation \"%s.%s\"",
> > + cbarg->relnamespace, cbarg->relname);
> > + break;
> >
>
> In what kind of errors will this help?
If there's an I/O error on an _fsm file, for one.
--
Justin
On Tue, Mar 24, 2020 at 9:46 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Mar 23, 2020 at 02:25:14PM +0530, Amit Kapila wrote:
> > > Yea, and it would be misleading if we reported "while scanning block..of
> > > relation" if we actually failed while writing its FSM.
> > >
> > > My previous patches did this:
> > >
> > > + case VACUUM_ERRCB_PHASE_VACUUM_FSM:
> > > + errcontext("while vacuuming free space map of relation \"%s.%s\"",
> > > + cbarg->relnamespace, cbarg->relname);
> > > + break;
> > >
> >
> > In what kind of errors will this help?
>
> If there's an I/O error on an _fsm file, for one.
>
If there is a read or write failure, then we give error like below
which already has required information.
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not read block %u in file \"%s\": %m",
blocknum, FilePathName(v->mdfd_vfd))));
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Tue, 24 Mar 2020 at 13:53, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 24, 2020 at 9:46 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > On Mon, Mar 23, 2020 at 02:25:14PM +0530, Amit Kapila wrote:
> > > > Yea, and it would be misleading if we reported "while scanning block..of
> > > > relation" if we actually failed while writing its FSM.
> > > >
> > > > My previous patches did this:
> > > >
> > > > + case VACUUM_ERRCB_PHASE_VACUUM_FSM:
> > > > + errcontext("while vacuuming free space map of relation \"%s.%s\"",
> > > > + cbarg->relnamespace, cbarg->relname);
> > > > + break;
> > > >
> > >
> > > In what kind of errors will this help?
> >
> > If there's an I/O error on an _fsm file, for one.
> >
>
> If there is a read or write failure, then we give error like below
> which already has required information.
> ereport(ERROR,
> (errcode_for_file_access(),
> errmsg("could not read block %u in file \"%s\": %m",
> blocknum, FilePathName(v->mdfd_vfd))));
Yeah, you're right. We, however, cannot see that the error happened
while recording freespace or while vacuuming freespace map but perhaps
we can see the situation by seeing the error message in most cases. So
I'm okay with the current set of phases.
I'll also test the current version of patch today.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Mar 24, 2020 at 2:37 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Tue, 24 Mar 2020 at 13:53, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Mar 24, 2020 at 9:46 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > On Mon, Mar 23, 2020 at 02:25:14PM +0530, Amit Kapila wrote:
> > > > > Yea, and it would be misleading if we reported "while scanning block..of
> > > > > relation" if we actually failed while writing its FSM.
> > > > >
> > > > > My previous patches did this:
> > > > >
> > > > > + case VACUUM_ERRCB_PHASE_VACUUM_FSM:
> > > > > + errcontext("while vacuuming free space map of relation \"%s.%s\"",
> > > > > + cbarg->relnamespace, cbarg->relname);
> > > > > + break;
> > > > >
> > > >
> > > > In what kind of errors will this help?
> > >
> > > If there's an I/O error on an _fsm file, for one.
> > >
> >
> > If there is a read or write failure, then we give error like below
> > which already has required information.
> > ereport(ERROR,
> > (errcode_for_file_access(),
> > errmsg("could not read block %u in file \"%s\": %m",
> > blocknum, FilePathName(v->mdfd_vfd))));
>
> Yeah, you're right. We, however, cannot see that the error happened
> while recording freespace or while vacuuming freespace map but perhaps
> we can see the situation by seeing the error message in most cases. So
> I'm okay with the current set of phases.
>
> I'll also test the current version of patch today.
>
okay, thanks.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Tue, 24 Mar 2020 at 18:19, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 24, 2020 at 2:37 PM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Tue, 24 Mar 2020 at 13:53, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Mar 24, 2020 at 9:46 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > >
> > > > On Mon, Mar 23, 2020 at 02:25:14PM +0530, Amit Kapila wrote:
> > > > > > Yea, and it would be misleading if we reported "while scanning block..of
> > > > > > relation" if we actually failed while writing its FSM.
> > > > > >
> > > > > > My previous patches did this:
> > > > > >
> > > > > > + case VACUUM_ERRCB_PHASE_VACUUM_FSM:
> > > > > > + errcontext("while vacuuming free space map of relation \"%s.%s\"",
> > > > > > + cbarg->relnamespace, cbarg->relname);
> > > > > > + break;
> > > > > >
> > > > >
> > > > > In what kind of errors will this help?
> > > >
> > > > If there's an I/O error on an _fsm file, for one.
> > > >
> > >
> > > If there is a read or write failure, then we give error like below
> > > which already has required information.
> > > ereport(ERROR,
> > > (errcode_for_file_access(),
> > > errmsg("could not read block %u in file \"%s\": %m",
> > > blocknum, FilePathName(v->mdfd_vfd))));
> >
> > Yeah, you're right. We, however, cannot see that the error happened
> > while recording freespace or while vacuuming freespace map but perhaps
> > we can see the situation by seeing the error message in most cases. So
> > I'm okay with the current set of phases.
> >
> > I'll also test the current version of patch today.
> >
>
> okay, thanks.
I've read through the latest version patch (v31), here are my comments:
1.
+ /* Update error traceback information */
+ olderrcbarg = *vacrelstats;
+ update_vacuum_error_cbarg(vacrelstats,
+ VACUUM_ERRCB_PHASE_TRUNCATE,
new_rel_pages, NULL,
+ false);
+
/*
* Scan backwards from the end to verify that the end pages actually
* contain no tuples. This is *necessary*, not optional, because
* other backends could have added tuples to these pages whilst we
* were vacuuming.
*/
new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
We need to set the error context after setting new_rel_pages.
2.
+ vacrelstats->relnamespace =
get_namespace_name(RelationGetNamespace(onerel));
+ vacrelstats->relname = pstrdup(RelationGetRelationName(onerel));
I think we can pfree these two variables to avoid a memory leak during
vacuum on multiple relations.
3.
+/* Phases of vacuum during which we report error context. */
+typedef enum
+{
+ VACUUM_ERRCB_PHASE_UNKNOWN,
+ VACUUM_ERRCB_PHASE_SCAN_HEAP,
+ VACUUM_ERRCB_PHASE_VACUUM_INDEX,
+ VACUUM_ERRCB_PHASE_VACUUM_HEAP,
+ VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
+ VACUUM_ERRCB_PHASE_TRUNCATE
+} ErrCbPhase;
Comparing to the vacuum progress phases, there is not a phase for
final cleanup which includes updating the relation stats. Is there any
reason why we don't have that phase for ErrCbPhase?
The type name ErrCbPhase seems to be very generic name, how about
VacErrCbPhase or VacuumErrCbPhase?
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
>
> I've read through the latest version patch (v31), here are my comments:
>
> 1.
> + /* Update error traceback information */
> + olderrcbarg = *vacrelstats;
> + update_vacuum_error_cbarg(vacrelstats,
> + VACUUM_ERRCB_PHASE_TRUNCATE,
> new_rel_pages, NULL,
> + false);
> +
> /*
> * Scan backwards from the end to verify that the end pages actually
> * contain no tuples. This is *necessary*, not optional, because
> * other backends could have added tuples to these pages whilst we
> * were vacuuming.
> */
> new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
>
> We need to set the error context after setting new_rel_pages.
>
We want to cover the errors raised in count_nondeletable_pages(). In
an earlier version of the patch, we had TRUNCATE_PREFETCH phase which
use to cover those errors, but that was not good as we were
setting/resetting it multiple times and it was not clear such a
separate phase would add any value.
> 2.
> + vacrelstats->relnamespace =
> get_namespace_name(RelationGetNamespace(onerel));
> + vacrelstats->relname = pstrdup(RelationGetRelationName(onerel));
>
> I think we can pfree these two variables to avoid a memory leak during
> vacuum on multiple relations.
>
Yeah, I had also thought about it but I noticed that we are not
freeing for vacrelstats. Also, I think the memory is allocated in
TopTransactionContext which should be freed via
CommitTransactionCommand before vacuuming of the next relation, so not
sure if there is much value in freeing those variables.
> 3.
> +/* Phases of vacuum during which we report error context. */
> +typedef enum
> +{
> + VACUUM_ERRCB_PHASE_UNKNOWN,
> + VACUUM_ERRCB_PHASE_SCAN_HEAP,
> + VACUUM_ERRCB_PHASE_VACUUM_INDEX,
> + VACUUM_ERRCB_PHASE_VACUUM_HEAP,
> + VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
> + VACUUM_ERRCB_PHASE_TRUNCATE
> +} ErrCbPhase;
>
> Comparing to the vacuum progress phases, there is not a phase for
> final cleanup which includes updating the relation stats. Is there any
> reason why we don't have that phase for ErrCbPhase?
>
I think we can add it if we want, but it was not clear to me if that is helpful.
> The type name ErrCbPhase seems to be very generic name, how about
> VacErrCbPhase or VacuumErrCbPhase?
>
It sounds like a better name.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 24, 2020 at 07:07:03PM +0530, Amit Kapila wrote: > On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote: > > 1. > > + /* Update error traceback information */ > > + olderrcbarg = *vacrelstats; > > + update_vacuum_error_cbarg(vacrelstats, > > + VACUUM_ERRCB_PHASE_TRUNCATE, > > new_rel_pages, NULL, > > + false); > > + > > /* > > * Scan backwards from the end to verify that the end pages actually > > * contain no tuples. This is *necessary*, not optional, because > > * other backends could have added tuples to these pages whilst we > > * were vacuuming. > > */ > > new_rel_pages = count_nondeletable_pages(onerel, vacrelstats); > > > > We need to set the error context after setting new_rel_pages. > > We want to cover the errors raised in count_nondeletable_pages(). In > an earlier version of the patch, we had TRUNCATE_PREFETCH phase which > use to cover those errors, but that was not good as we were > setting/resetting it multiple times and it was not clear such a > separate phase would add any value. I insisted on covering count_nondeletable_pages since it calls ReadBuffer(), but I think we need to at least set vacrelsats->blkno = new_rel_pages, since it may be different, right ? > > 2. > > + vacrelstats->relnamespace = > > get_namespace_name(RelationGetNamespace(onerel)); > > + vacrelstats->relname = pstrdup(RelationGetRelationName(onerel)); > > > > I think we can pfree these two variables to avoid a memory leak during > > vacuum on multiple relations. > > Yeah, I had also thought about it but I noticed that we are not > freeing for vacrelstats. Also, I think the memory is allocated in > TopTransactionContext which should be freed via > CommitTransactionCommand before vacuuming of the next relation, so not > sure if there is much value in freeing those variables. One small reason to free them is that (as Tom mentioned upthread) it's good to ensure that those variables are their own allocation, and not depending on being able to access relcache or anything else during an unexpected error. -- Justin
On Tue, Mar 24, 2020 at 7:18 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Tue, Mar 24, 2020 at 07:07:03PM +0530, Amit Kapila wrote: > > On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote: > > > 1. > > > + /* Update error traceback information */ > > > + olderrcbarg = *vacrelstats; > > > + update_vacuum_error_cbarg(vacrelstats, > > > + VACUUM_ERRCB_PHASE_TRUNCATE, > > > new_rel_pages, NULL, > > > + false); > > > + > > > /* > > > * Scan backwards from the end to verify that the end pages actually > > > * contain no tuples. This is *necessary*, not optional, because > > > * other backends could have added tuples to these pages whilst we > > > * were vacuuming. > > > */ > > > new_rel_pages = count_nondeletable_pages(onerel, vacrelstats); > > > > > > We need to set the error context after setting new_rel_pages. > > > > We want to cover the errors raised in count_nondeletable_pages(). In > > an earlier version of the patch, we had TRUNCATE_PREFETCH phase which > > use to cover those errors, but that was not good as we were > > setting/resetting it multiple times and it was not clear such a > > separate phase would add any value. > > I insisted on covering count_nondeletable_pages since it calls ReadBuffer(), > but I think we need to at least set vacrelsats->blkno = new_rel_pages, since it > may be different, right ? > yeah, that makes sense. > > > 2. > > > + vacrelstats->relnamespace = > > > get_namespace_name(RelationGetNamespace(onerel)); > > > + vacrelstats->relname = pstrdup(RelationGetRelationName(onerel)); > > > > > > I think we can pfree these two variables to avoid a memory leak during > > > vacuum on multiple relations. > > > > Yeah, I had also thought about it but I noticed that we are not > > freeing for vacrelstats. Also, I think the memory is allocated in > > TopTransactionContext which should be freed via > > CommitTransactionCommand before vacuuming of the next relation, so not > > sure if there is much value in freeing those variables. > > One small reason to free them is that (as Tom mentioned upthread) it's good to > ensure that those variables are their own allocation, and not depending on > being able to access relcache or anything else during an unexpected error. > That is a good reason to allocate them separately but not for doing retail free especially when the caller of the function will free the context in which that is allocated. I think Sawada-San's concern was that it will leak memory across the vacuum of multiple relations but that is not the case here. Won't it look odd if we are freeing memory for members of vacrelstats but not for vacrelstats itself? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, 24 Mar 2020 at 22:37, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> >
> > I've read through the latest version patch (v31), here are my comments:
> >
> > 1.
> > + /* Update error traceback information */
> > + olderrcbarg = *vacrelstats;
> > + update_vacuum_error_cbarg(vacrelstats,
> > + VACUUM_ERRCB_PHASE_TRUNCATE,
> > new_rel_pages, NULL,
> > + false);
> > +
> > /*
> > * Scan backwards from the end to verify that the end pages actually
> > * contain no tuples. This is *necessary*, not optional, because
> > * other backends could have added tuples to these pages whilst we
> > * were vacuuming.
> > */
> > new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
> >
> > We need to set the error context after setting new_rel_pages.
> >
>
> We want to cover the errors raised in count_nondeletable_pages(). In
> an earlier version of the patch, we had TRUNCATE_PREFETCH phase which
> use to cover those errors, but that was not good as we were
> setting/resetting it multiple times and it was not clear such a
> separate phase would add any value.
I got the point. But if we set the error context before that, I think
we need to change the error context message. The error context message
of heap truncation phase is "while truncating relation \"%s.%s\" to %u
blocks", but cbarg->blkno will be the number of blocks of the current
relation.
case VACUUM_ERRCB_PHASE_TRUNCATE:
if (BlockNumberIsValid(cbarg->blkno))
errcontext("while truncating relation \"%s.%s\" to %u blocks",
cbarg->relnamespace, cbarg->relname, cbarg->blkno);
break;
>
> > 2.
> > + vacrelstats->relnamespace =
> > get_namespace_name(RelationGetNamespace(onerel));
> > + vacrelstats->relname = pstrdup(RelationGetRelationName(onerel));
> >
> > I think we can pfree these two variables to avoid a memory leak during
> > vacuum on multiple relations.
> >
>
> Yeah, I had also thought about it but I noticed that we are not
> freeing for vacrelstats. Also, I think the memory is allocated in
> TopTransactionContext which should be freed via
> CommitTransactionCommand before vacuuming of the next relation, so not
> sure if there is much value in freeing those variables.
Right, thank you for explanation. I was concerned memory leak because
relation name and schema name could be large compared to vacrelstats
but I agree to free them at commit time.
>
> > 3.
> > +/* Phases of vacuum during which we report error context. */
> > +typedef enum
> > +{
> > + VACUUM_ERRCB_PHASE_UNKNOWN,
> > + VACUUM_ERRCB_PHASE_SCAN_HEAP,
> > + VACUUM_ERRCB_PHASE_VACUUM_INDEX,
> > + VACUUM_ERRCB_PHASE_VACUUM_HEAP,
> > + VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
> > + VACUUM_ERRCB_PHASE_TRUNCATE
> > +} ErrCbPhase;
> >
> > Comparing to the vacuum progress phases, there is not a phase for
> > final cleanup which includes updating the relation stats. Is there any
> > reason why we don't have that phase for ErrCbPhase?
> >
>
> I think we can add it if we want, but it was not clear to me if that is helpful.
Yeah, me too. The most likely place where an error happens is
vac_update_relstats but the error message seems to be enough.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Mar 24, 2020 at 09:47:30PM +0900, Masahiko Sawada wrote: > We need to set the error context after setting new_rel_pages. Done > The type name ErrCbPhase seems to be very generic name, how about > VacErrCbPhase or VacuumErrCbPhase? Done. Thanks for your review comments. -- Justin
Attachment
On Tue, Mar 24, 2020 at 7:51 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Tue, 24 Mar 2020 at 22:37, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > >
> > > I've read through the latest version patch (v31), here are my comments:
> > >
> > > 1.
> > > + /* Update error traceback information */
> > > + olderrcbarg = *vacrelstats;
> > > + update_vacuum_error_cbarg(vacrelstats,
> > > + VACUUM_ERRCB_PHASE_TRUNCATE,
> > > new_rel_pages, NULL,
> > > + false);
> > > +
> > > /*
> > > * Scan backwards from the end to verify that the end pages actually
> > > * contain no tuples. This is *necessary*, not optional, because
> > > * other backends could have added tuples to these pages whilst we
> > > * were vacuuming.
> > > */
> > > new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
> > >
> > > We need to set the error context after setting new_rel_pages.
> > >
> >
> > We want to cover the errors raised in count_nondeletable_pages(). In
> > an earlier version of the patch, we had TRUNCATE_PREFETCH phase which
> > use to cover those errors, but that was not good as we were
> > setting/resetting it multiple times and it was not clear such a
> > separate phase would add any value.
>
> I got the point. But if we set the error context before that, I think
> we need to change the error context message. The error context message
> of heap truncation phase is "while truncating relation \"%s.%s\" to %u
> blocks", but cbarg->blkno will be the number of blocks of the current
> relation.
>
> case VACUUM_ERRCB_PHASE_TRUNCATE:
> if (BlockNumberIsValid(cbarg->blkno))
> errcontext("while truncating relation \"%s.%s\" to %u blocks",
> cbarg->relnamespace, cbarg->relname, cbarg->blkno);
> break;
>
Do you mean to say that actually we are just prefetching or reading
the pages in count_nondeletable_pages() but the message doesn't have
any such indication? If not that, what problem do you see with the
message? What is your suggestion?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Wed, Mar 25, 2020 at 8:49 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Tue, Mar 24, 2020 at 09:47:30PM +0900, Masahiko Sawada wrote: > > We need to set the error context after setting new_rel_pages. > > Done > > > The type name ErrCbPhase seems to be very generic name, how about > > VacErrCbPhase or VacuumErrCbPhase? > > Done. > > Thanks for your review comments. > @@ -870,6 +904,12 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, else skipping_blocks = false; + /* Setup error traceback support for ereport() */ + errcallback.callback = vacuum_error_callback; + errcallback.arg = vacrelstats; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; I think by mistake you have re-introduced this chunk of code. We don't need this as we have done it in the caller. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, 25 Mar 2020 at 12:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 24, 2020 at 7:51 PM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Tue, 24 Mar 2020 at 22:37, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada
> > > <masahiko.sawada@2ndquadrant.com> wrote:
> > > >
> > > >
> > > > I've read through the latest version patch (v31), here are my comments:
> > > >
> > > > 1.
> > > > + /* Update error traceback information */
> > > > + olderrcbarg = *vacrelstats;
> > > > + update_vacuum_error_cbarg(vacrelstats,
> > > > + VACUUM_ERRCB_PHASE_TRUNCATE,
> > > > new_rel_pages, NULL,
> > > > + false);
> > > > +
> > > > /*
> > > > * Scan backwards from the end to verify that the end pages actually
> > > > * contain no tuples. This is *necessary*, not optional, because
> > > > * other backends could have added tuples to these pages whilst we
> > > > * were vacuuming.
> > > > */
> > > > new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
> > > >
> > > > We need to set the error context after setting new_rel_pages.
> > > >
> > >
> > > We want to cover the errors raised in count_nondeletable_pages(). In
> > > an earlier version of the patch, we had TRUNCATE_PREFETCH phase which
> > > use to cover those errors, but that was not good as we were
> > > setting/resetting it multiple times and it was not clear such a
> > > separate phase would add any value.
> >
> > I got the point. But if we set the error context before that, I think
> > we need to change the error context message. The error context message
> > of heap truncation phase is "while truncating relation \"%s.%s\" to %u
> > blocks", but cbarg->blkno will be the number of blocks of the current
> > relation.
> >
> > case VACUUM_ERRCB_PHASE_TRUNCATE:
> > if (BlockNumberIsValid(cbarg->blkno))
> > errcontext("while truncating relation \"%s.%s\" to %u blocks",
> > cbarg->relnamespace, cbarg->relname, cbarg->blkno);
> > break;
> >
>
> Do you mean to say that actually we are just prefetching or reading
> the pages in count_nondeletable_pages() but the message doesn't have
> any such indication? If not that, what problem do you see with the
> message? What is your suggestion?
I meant that with the patch, suppose that the table has 100 blocks and
we're truncating it to 50 blocks in RelationTruncate(), the error
context message will be "while truncating relation "aaa.bbb" to 100
blocks", which is not correct. I think it should be "while truncating
relation "aaa.bbb" to 50 blocks". We can know the relation can be
truncated to 50 blocks by the result of count_nondeletable_pages(). So
if we update the arguments before it we will use the number of blocks
of relation before truncation.
My suggestion is either that we change the error message to, for
example, "while truncating relation "aaa.bbb" having 100 blocks", or
that we change the patch so that we can use "50 blocks" in the error
context message.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Mar 25, 2020 at 01:34:43PM +0900, Masahiko Sawada wrote:
> I meant that with the patch, suppose that the table has 100 blocks and
> we're truncating it to 50 blocks in RelationTruncate(), the error
> context message will be "while truncating relation "aaa.bbb" to 100
> blocks", which is not correct.
> I think it should be "while truncating
> relation "aaa.bbb" to 50 blocks". We can know the relation can be
> truncated to 50 blocks by the result of count_nondeletable_pages(). So
> if we update the arguments before it we will use the number of blocks
> of relation before truncation.
Hm, yea, at that point it's:
|new_rel_pages = RelationGetNumberOfBlocks(onerel);
..so we can do better.
> My suggestion is either that we change the error message to, for
> example, "while truncating relation "aaa.bbb" having 100 blocks", or
> that we change the patch so that we can use "50 blocks" in the error
> context message.
We could do:
update_vacuum_error_cbarg(vacrelstats,
VACUUM_ERRCB_PHASE_TRUNCATE,
InvalidBlockNumber, NULL, false);
new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
vacrelstats->blkno = new_rel_pages;
...
case VACUUM_ERRCB_PHASE_TRUNCATE:
if (BlockNumberIsValid(cbarg->blkno))
errcontext("while truncating relation \"%s.%s\" to %u blocks",
cbarg->relnamespace, cbarg->relname, cbarg->blkno);
else
/* Error happened before/during count_nondeletable_pages() */
errcontext("while truncating relation \"%s.%s\"",
cbarg->relnamespace, cbarg->relname);
break;
--
Justin
On Wed, Mar 25, 2020 at 10:05 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Wed, 25 Mar 2020 at 12:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Mar 24, 2020 at 7:51 PM Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > >
> > > I got the point. But if we set the error context before that, I think
> > > we need to change the error context message. The error context message
> > > of heap truncation phase is "while truncating relation \"%s.%s\" to %u
> > > blocks", but cbarg->blkno will be the number of blocks of the current
> > > relation.
> > >
> > > case VACUUM_ERRCB_PHASE_TRUNCATE:
> > > if (BlockNumberIsValid(cbarg->blkno))
> > > errcontext("while truncating relation \"%s.%s\" to %u blocks",
> > > cbarg->relnamespace, cbarg->relname, cbarg->blkno);
> > > break;
> > >
> >
> > Do you mean to say that actually we are just prefetching or reading
> > the pages in count_nondeletable_pages() but the message doesn't have
> > any such indication? If not that, what problem do you see with the
> > message? What is your suggestion?
>
> I meant that with the patch, suppose that the table has 100 blocks and
> we're truncating it to 50 blocks in RelationTruncate(), the error
> context message will be "while truncating relation "aaa.bbb" to 100
> blocks", which is not correct. I think it should be "while truncating
> relation "aaa.bbb" to 50 blocks". We can know the relation can be
> truncated to 50 blocks by the result of count_nondeletable_pages(). So
> if we update the arguments before it we will use the number of blocks
> of relation before truncation.
>
Won't the latest patch by Justin will fix this as he has updated the
block count after count_nondeletable_pages? Apart from that, I feel
the first call to update_vacuum_error_cbarg in lazy_truncate_heap
should have input parameter as vacrelstats->nonempty_pages instead of
new_rel_pages to indicate the remaining pages after truncation?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Wed, Mar 25, 2020 at 10:22:21AM +0530, Amit Kapila wrote:
> On Wed, Mar 25, 2020 at 10:05 AM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Wed, 25 Mar 2020 at 12:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Mar 24, 2020 at 7:51 PM Masahiko Sawada
> > > <masahiko.sawada@2ndquadrant.com> wrote:
> > > >
> > > >
> > > > I got the point. But if we set the error context before that, I think
> > > > we need to change the error context message. The error context message
> > > > of heap truncation phase is "while truncating relation \"%s.%s\" to %u
> > > > blocks", but cbarg->blkno will be the number of blocks of the current
> > > > relation.
> > > >
> > > > case VACUUM_ERRCB_PHASE_TRUNCATE:
> > > > if (BlockNumberIsValid(cbarg->blkno))
> > > > errcontext("while truncating relation \"%s.%s\" to %u blocks",
> > > > cbarg->relnamespace, cbarg->relname, cbarg->blkno);
> > > > break;
> > > >
> > >
> > > Do you mean to say that actually we are just prefetching or reading
> > > the pages in count_nondeletable_pages() but the message doesn't have
> > > any such indication? If not that, what problem do you see with the
> > > message? What is your suggestion?
> >
> > I meant that with the patch, suppose that the table has 100 blocks and
> > we're truncating it to 50 blocks in RelationTruncate(), the error
> > context message will be "while truncating relation "aaa.bbb" to 100
> > blocks", which is not correct. I think it should be "while truncating
> > relation "aaa.bbb" to 50 blocks". We can know the relation can be
> > truncated to 50 blocks by the result of count_nondeletable_pages(). So
> > if we update the arguments before it we will use the number of blocks
> > of relation before truncation.
> >
>
> Won't the latest patch by Justin will fix this as he has updated the
> block count after count_nondeletable_pages? Apart from that, I feel
The issue is if the error happens *during* count_nondeletable_pages().
We don't want it to say "truncating relation to 100 blocks".
> the first call to update_vacuum_error_cbarg in lazy_truncate_heap
> should have input parameter as vacrelstats->nonempty_pages instead of
> new_rel_pages to indicate the remaining pages after truncation?
Yea, I think that addresses the issue.
--
Justin
On Wed, 25 Mar 2020 at 14:08, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Wed, Mar 25, 2020 at 10:22:21AM +0530, Amit Kapila wrote:
> > On Wed, Mar 25, 2020 at 10:05 AM Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > On Wed, 25 Mar 2020 at 12:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Tue, Mar 24, 2020 at 7:51 PM Masahiko Sawada
> > > > <masahiko.sawada@2ndquadrant.com> wrote:
> > > > >
> > > > >
> > > > > I got the point. But if we set the error context before that, I think
> > > > > we need to change the error context message. The error context message
> > > > > of heap truncation phase is "while truncating relation \"%s.%s\" to %u
> > > > > blocks", but cbarg->blkno will be the number of blocks of the current
> > > > > relation.
> > > > >
> > > > > case VACUUM_ERRCB_PHASE_TRUNCATE:
> > > > > if (BlockNumberIsValid(cbarg->blkno))
> > > > > errcontext("while truncating relation \"%s.%s\" to %u blocks",
> > > > > cbarg->relnamespace, cbarg->relname, cbarg->blkno);
> > > > > break;
> > > > >
> > > >
> > > > Do you mean to say that actually we are just prefetching or reading
> > > > the pages in count_nondeletable_pages() but the message doesn't have
> > > > any such indication? If not that, what problem do you see with the
> > > > message? What is your suggestion?
> > >
> > > I meant that with the patch, suppose that the table has 100 blocks and
> > > we're truncating it to 50 blocks in RelationTruncate(), the error
> > > context message will be "while truncating relation "aaa.bbb" to 100
> > > blocks", which is not correct. I think it should be "while truncating
> > > relation "aaa.bbb" to 50 blocks". We can know the relation can be
> > > truncated to 50 blocks by the result of count_nondeletable_pages(). So
> > > if we update the arguments before it we will use the number of blocks
> > > of relation before truncation.
> > >
> >
> > Won't the latest patch by Justin will fix this as he has updated the
> > block count after count_nondeletable_pages? Apart from that, I feel
>
> The issue is if the error happens *during* count_nondeletable_pages().
> We don't want it to say "truncating relation to 100 blocks".
Right.
>
> > the first call to update_vacuum_error_cbarg in lazy_truncate_heap
> > should have input parameter as vacrelstats->nonempty_pages instead of
> > new_rel_pages to indicate the remaining pages after truncation?
>
> Yea, I think that addresses the issue.
+1
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Mar 25, 2020 at 09:16:46AM +0530, Amit Kapila wrote: > I think by mistake you have re-introduced this chunk of code. We > don't need this as we have done it in the caller. Yes, sorry. I used too much of git-am and git-rebase to make sure I didn't lose your changes and instead reintroduced them. On Wed, Mar 25, 2020 at 02:16:35PM +0900, Masahiko Sawada wrote: > > > Won't the latest patch by Justin will fix this as he has updated the > > > block count after count_nondeletable_pages? Apart from that, I feel > > > > The issue is if the error happens *during* count_nondeletable_pages(). > > We don't want it to say "truncating relation to 100 blocks". > > Right. > > > > the first call to update_vacuum_error_cbarg in lazy_truncate_heap > > > should have input parameter as vacrelstats->nonempty_pages instead of > > > new_rel_pages to indicate the remaining pages after truncation? > > > > Yea, I think that addresses the issue. Attached patch addressing these. -- Justin
Attachment
On Wed, Mar 25, 2020 at 3:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > Attached patch addressing these. > Thanks, you forgot to remove the below declaration which I have removed in attached. @@ -724,20 +758,20 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, PROGRESS_VACUUM_MAX_DEAD_TUPLES }; int64 initprog_val[3]; + ErrorContextCallback errcallback; Apart from this, I have ran pgindent and now I think it is in good shape. Do you have any other comments? Sawada-San, can you also check the attached patch and let me know if you have any additional comments. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Attachment
On Wed, Mar 25, 2020 at 04:54:41PM +0530, Amit Kapila wrote:
> On Wed, Mar 25, 2020 at 3:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > Attached patch addressing these.
> >
>
> Thanks, you forgot to remove the below declaration which I have
> removed in attached.
Yes I saw..
> Apart from this, I have ran pgindent and now I think it is in good
> shape. Do you have any other comments? Sawada-San, can you also
I did just notice/remember while testing trucate that autovacuum does this:
src/backend/postmaster/autovacuum.c: errcontext("automatic vacuum of table \"%s.%s.%s\"",
And that appears to be interacting correctly. For example if you add an
elog(ERROR) and run UPDATE/DELETE, and wait autovacuum_naptime, then it shows
both contexts.
--
Justin
On Wed, 25 Mar 2020 at 20:24, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Mar 25, 2020 at 3:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > Attached patch addressing these.
> >
>
> Thanks, you forgot to remove the below declaration which I have
> removed in attached.
>
> @@ -724,20 +758,20 @@ lazy_scan_heap(Relation onerel, VacuumParams
> *params, LVRelStats *vacrelstats,
> PROGRESS_VACUUM_MAX_DEAD_TUPLES
> };
> int64 initprog_val[3];
> + ErrorContextCallback errcallback;
>
> Apart from this, I have ran pgindent and now I think it is in good
> shape. Do you have any other comments? Sawada-San, can you also
> check the attached patch and let me know if you have any additional
> comments.
>
Thank you for updating the patch! I have a question about the following code:
+ /* Update error traceback information */
+ olderrcbarg = *vacrelstats;
+ update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_TRUNCATE,
+ vacrelstats->nonempty_pages, NULL, false);
+
/*
* Scan backwards from the end to verify that the end pages actually
* contain no tuples. This is *necessary*, not optional, because
* other backends could have added tuples to these pages whilst we
* were vacuuming.
*/
new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
+ vacrelstats->blkno = new_rel_pages;
if (new_rel_pages >= old_rel_pages)
{
/* can't do anything after all */
UnlockRelation(onerel, AccessExclusiveLock);
return;
}
/*
* Okay to truncate.
*/
RelationTruncate(onerel, new_rel_pages);
+ /* Revert back to the old phase information for error traceback */
+ update_vacuum_error_cbarg(vacrelstats,
+ olderrcbarg.phase,
+ olderrcbarg.blkno,
+ olderrcbarg.indname,
+ true);
vacrelstats->nonempty_pages is the last non-empty block while
new_rel_pages, the result of count_nondeletable_pages(), is the number
of blocks that we can truncate to in this attempt. Therefore
vacrelstats->nonempty_pages <= new_rel_pages. This means that we set a
lower block number to arguments and then set a higher block number
after count_nondeletable_pages, and then revert it back to
VACUUM_ERRCB_PHASE_SCAN_HEAP phase and the number of blocks of
relation before truncation, after RelationTruncate(). It can be
repeated until no more truncating can be done. Why do we need to
revert back to the scan heap phase? If we can use
vacrelstats->nonempty_pages in the error context message as the
remaining blocks after truncation I think we can update callback
arguments once at the beginning of lazy_truncate_heap() and don't
revert to the previous phase, and pop the error context after exiting.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Mar 25, 2020 at 09:27:44PM +0900, Masahiko Sawada wrote:
> On Wed, 25 Mar 2020 at 20:24, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Mar 25, 2020 at 3:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > Attached patch addressing these.
> > >
> >
> > Thanks, you forgot to remove the below declaration which I have
> > removed in attached.
> >
> > @@ -724,20 +758,20 @@ lazy_scan_heap(Relation onerel, VacuumParams
> > *params, LVRelStats *vacrelstats,
> > PROGRESS_VACUUM_MAX_DEAD_TUPLES
> > };
> > int64 initprog_val[3];
> > + ErrorContextCallback errcallback;
> >
> > Apart from this, I have ran pgindent and now I think it is in good
> > shape. Do you have any other comments? Sawada-San, can you also
> > check the attached patch and let me know if you have any additional
> > comments.
> >
>
> Thank you for updating the patch! I have a question about the following code:
>
> + /* Update error traceback information */
> + olderrcbarg = *vacrelstats;
> + update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_TRUNCATE,
> + vacrelstats->nonempty_pages, NULL, false);
> +
> /*
> * Scan backwards from the end to verify that the end pages actually
> * contain no tuples. This is *necessary*, not optional, because
> * other backends could have added tuples to these pages whilst we
> * were vacuuming.
> */
> new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
> + vacrelstats->blkno = new_rel_pages;
>
> if (new_rel_pages >= old_rel_pages)
> {
> /* can't do anything after all */
> UnlockRelation(onerel, AccessExclusiveLock);
> return;
> }
>
> /*
> * Okay to truncate.
> */
> RelationTruncate(onerel, new_rel_pages);
>
> + /* Revert back to the old phase information for error traceback */
> + update_vacuum_error_cbarg(vacrelstats,
> + olderrcbarg.phase,
> + olderrcbarg.blkno,
> + olderrcbarg.indname,
> + true);
>
> vacrelstats->nonempty_pages is the last non-empty block while
> new_rel_pages, the result of count_nondeletable_pages(), is the number
> of blocks that we can truncate to in this attempt. Therefore
> vacrelstats->nonempty_pages <= new_rel_pages. This means that we set a
> lower block number to arguments and then set a higher block number
> after count_nondeletable_pages, and then revert it back to
> VACUUM_ERRCB_PHASE_SCAN_HEAP phase and the number of blocks of
> relation before truncation, after RelationTruncate(). It can be
> repeated until no more truncating can be done. Why do we need to
> revert back to the scan heap phase? If we can use
> vacrelstats->nonempty_pages in the error context message as the
> remaining blocks after truncation I think we can update callback
> arguments once at the beginning of lazy_truncate_heap() and don't
> revert to the previous phase, and pop the error context after exiting.
Perhaps. We need to "revert back" for the vacuum phases, which can be called
multiple times, but we don't need to do that here.
In the future, if we decided to add something for final cleanup phase (say),
it's fine (and maybe better) to exit truncate_heap() without resetting the
argument, and we'd immediately set it to CLEANUP.
I think the same thing applies to lazy_cleanup_index, too. It can be called
from a parallel worker, but we never "go back" to a heap scan.
--
Justin
On Wed, Mar 25, 2020 at 6:11 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Wed, Mar 25, 2020 at 09:27:44PM +0900, Masahiko Sawada wrote:
> > On Wed, 25 Mar 2020 at 20:24, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Mar 25, 2020 at 3:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > >
> > > > Attached patch addressing these.
> > > >
> > >
> > > Thanks, you forgot to remove the below declaration which I have
> > > removed in attached.
> > >
> > > @@ -724,20 +758,20 @@ lazy_scan_heap(Relation onerel, VacuumParams
> > > *params, LVRelStats *vacrelstats,
> > > PROGRESS_VACUUM_MAX_DEAD_TUPLES
> > > };
> > > int64 initprog_val[3];
> > > + ErrorContextCallback errcallback;
> > >
> > > Apart from this, I have ran pgindent and now I think it is in good
> > > shape. Do you have any other comments? Sawada-San, can you also
> > > check the attached patch and let me know if you have any additional
> > > comments.
> > >
> >
> > Thank you for updating the patch! I have a question about the following code:
> >
> > + /* Update error traceback information */
> > + olderrcbarg = *vacrelstats;
> > + update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_TRUNCATE,
> > + vacrelstats->nonempty_pages, NULL, false);
> > +
> > /*
> > * Scan backwards from the end to verify that the end pages actually
> > * contain no tuples. This is *necessary*, not optional, because
> > * other backends could have added tuples to these pages whilst we
> > * were vacuuming.
> > */
> > new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
> > + vacrelstats->blkno = new_rel_pages;
> >
> > if (new_rel_pages >= old_rel_pages)
> > {
> > /* can't do anything after all */
> > UnlockRelation(onerel, AccessExclusiveLock);
> > return;
> > }
> >
> > /*
> > * Okay to truncate.
> > */
> > RelationTruncate(onerel, new_rel_pages);
> >
> > + /* Revert back to the old phase information for error traceback */
> > + update_vacuum_error_cbarg(vacrelstats,
> > + olderrcbarg.phase,
> > + olderrcbarg.blkno,
> > + olderrcbarg.indname,
> > + true);
> >
> > vacrelstats->nonempty_pages is the last non-empty block while
> > new_rel_pages, the result of count_nondeletable_pages(), is the number
> > of blocks that we can truncate to in this attempt. Therefore
> > vacrelstats->nonempty_pages <= new_rel_pages. This means that we set a
> > lower block number to arguments and then set a higher block number
> > after count_nondeletable_pages, and then revert it back to
> > VACUUM_ERRCB_PHASE_SCAN_HEAP phase and the number of blocks of
> > relation before truncation, after RelationTruncate(). It can be
> > repeated until no more truncating can be done. Why do we need to
> > revert back to the scan heap phase? If we can use
> > vacrelstats->nonempty_pages in the error context message as the
> > remaining blocks after truncation I think we can update callback
> > arguments once at the beginning of lazy_truncate_heap() and don't
> > revert to the previous phase, and pop the error context after exiting.
>
> Perhaps. We need to "revert back" for the vacuum phases, which can be called
> multiple times, but we don't need to do that here.
>
Yeah, but I think it would be better if are consistent because we have
no control what the caller of the function intends to do after
finishing the current phase. I think we can add some comments where
we set up the context (in heap_vacuum_rel) like below so that the idea
is more clear.
"The idea is to set up an error context callback to display additional
information with any error during vacuum. During different phases of
vacuum (heap scan, heap vacuum, index vacuum, index clean up, heap
truncate), we update the error context callback to display appropriate
information.
Note that different phases of vacuum overlap with each other, so once
a particular phase is over, we need to revert back to the old phase to
keep the phase information up-to-date."
What do you think?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Thu, Mar 26, 2020 at 09:50:53AM +0530, Amit Kapila wrote: > > > after count_nondeletable_pages, and then revert it back to > > > VACUUM_ERRCB_PHASE_SCAN_HEAP phase and the number of blocks of > > > relation before truncation, after RelationTruncate(). It can be > > > repeated until no more truncating can be done. Why do we need to > > > revert back to the scan heap phase? If we can use > > > vacrelstats->nonempty_pages in the error context message as the > > > remaining blocks after truncation I think we can update callback > > > arguments once at the beginning of lazy_truncate_heap() and don't > > > revert to the previous phase, and pop the error context after exiting. > > > > Perhaps. We need to "revert back" for the vacuum phases, which can be called > > multiple times, but we don't need to do that here. > > Yeah, but I think it would be better if are consistent because we have > no control what the caller of the function intends to do after > finishing the current phase. I think we can add some comments where > we set up the context (in heap_vacuum_rel) like below so that the idea > is more clear. > > "The idea is to set up an error context callback to display additional > information with any error during vacuum. During different phases of > vacuum (heap scan, heap vacuum, index vacuum, index clean up, heap > truncate), we update the error context callback to display appropriate > information. > > Note that different phases of vacuum overlap with each other, so once > a particular phase is over, we need to revert back to the old phase to > keep the phase information up-to-date." Seems fine. Rather than saying "different phases" I, would say: "The index vacuum and heap vacuum phases may be called multiple times in the middle of the heap scan phase." But actually I think the concern is not that we unnecessarily "Revert back to the old phase" but that we do it in a *loop*. Which I agree doesn't make sense, to go back and forth between "scanning heap" and "truncating". So I think we should either remove the "revert back", or otherwise put it after/outside the "while" loop, and change the "return" paths to use "break". -- Justin
On Thu, Mar 26, 2020 at 10:11 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > Seems fine. Rather than saying "different phases" I, would say: > "The index vacuum and heap vacuum phases may be called multiple times in the > middle of the heap scan phase." > Okay, I have slightly adjusted the wording as per your suggestion. > But actually I think the concern is not that we unnecessarily "Revert back to > the old phase" but that we do it in a *loop*. Which I agree doesn't make > sense, to go back and forth between "scanning heap" and "truncating". > Fair point. I have moved the change to the truncate phase at the caller of lazy_heap_truncate() which should address this concern. Sawada-San, does this address your concern? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Thu, Mar 26, 2020 at 12:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Thu, Mar 26, 2020 at 10:11 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > Seems fine. Rather than saying "different phases" I, would say: > > "The index vacuum and heap vacuum phases may be called multiple times in the > > middle of the heap scan phase." > > > > Okay, I have slightly adjusted the wording as per your suggestion. > > > But actually I think the concern is not that we unnecessarily "Revert back to > > the old phase" but that we do it in a *loop*. Which I agree doesn't make > > sense, to go back and forth between "scanning heap" and "truncating". > > > > Fair point. I have moved the change to the truncate phase at the > caller of lazy_heap_truncate() which should address this concern. > Sawada-San, does this address your concern? > Forgot to attach the patch, doing now. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Attachment
On Thu, 26 Mar 2020 at 15:34, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Mar 26, 2020 at 12:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Mar 26, 2020 at 10:11 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > Seems fine. Rather than saying "different phases" I, would say:
> > > "The index vacuum and heap vacuum phases may be called multiple times in the
> > > middle of the heap scan phase."
> > >
> >
> > Okay, I have slightly adjusted the wording as per your suggestion.
> >
> > > But actually I think the concern is not that we unnecessarily "Revert back to
> > > the old phase" but that we do it in a *loop*. Which I agree doesn't make
> > > sense, to go back and forth between "scanning heap" and "truncating".
> > >
> >
> > Fair point. I have moved the change to the truncate phase at the
> > caller of lazy_heap_truncate() which should address this concern.
> > Sawada-San, does this address your concern?
> >
>
> Forgot to attach the patch, doing now.
Thank you for updating the patch! The changes around
lazy_truncate_heap() looks good to me.
I have two comments;
1.
@@ -1844,9 +1914,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber
blkno, Buffer buffer,
int uncnt = 0;
TransactionId visibility_cutoff_xid;
bool all_frozen;
+ LVRelStats olderrcbarg;
pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno);
+ /* Update error traceback information */
+ olderrcbarg = *vacrelstats;
+ update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_VACUUM_HEAP,
+ blkno, NULL, false);
Since we update vacrelstats->blkno during in the loop in
lazy_vacuum_heap() we unnecessarily update blkno twice to the same
value. Also I think we don't need to revert back the callback
arguments in lazy_vacuum_page(). Perhaps we can either remove the
change of lazy_vacuum_page() or move the code updating
vacrelstats->blkno to the beginning of lazy_vacuum_page(). I prefer
the latter.
2.
+/*
+ * Update vacuum error callback for the current phase, block, and index.
+ *
+ * free_oldindname is true if the previous "indname" should be freed.
It must be
+ * false if the caller has copied the old LVRelStats, to avoid keeping a
+ * pointer to a freed allocation. In which case, the caller should call again
+ * with free_oldindname as true to avoid a leak.
+ */
+static void
+update_vacuum_error_cbarg(LVRelStats *errcbarg, int phase, BlockNumber blkno,
+ char *indname, bool free_oldindname)
I'm not sure why "free_oldindname" is necessary. Since we initialize
vacrelstats->indname with NULL and revert the callback arguments at
the end of functions that needs update them, vacrelstats->indname is
NULL at the beginning of lazy_vacuum_index() and lazy_cleanup_index().
And we make a copy of index name in update_vacuum_error_cbarg(). So I
think we can pfree the old index name if errcbarg->indname is not NULL.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Mar 26, 2020 at 08:56:54PM +0900, Masahiko Sawada wrote: > 1. > @@ -1844,9 +1914,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber > blkno, Buffer buffer, > int uncnt = 0; > TransactionId visibility_cutoff_xid; > bool all_frozen; > + LVRelStats olderrcbarg; > > pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno); > > + /* Update error traceback information */ > + olderrcbarg = *vacrelstats; > + update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_VACUUM_HEAP, > + blkno, NULL, false); > > Since we update vacrelstats->blkno during in the loop in > lazy_vacuum_heap() we unnecessarily update blkno twice to the same > value. Also I think we don't need to revert back the callback > arguments in lazy_vacuum_page(). Perhaps we can either remove the > change of lazy_vacuum_page() or move the code updating > vacrelstats->blkno to the beginning of lazy_vacuum_page(). I prefer > the latter. We want the error callback to be in place during lazy_scan_heap, since it calls ReadBufferExtended(). We can't remove the change in lazy_vacuum_page, since it's also called from lazy_scan_heap, if there are no indexes. We want lazy_vacuum_page to "revert back" since we go from "scanning heap" to "vacuuming heap". lazy_vacuum_page was the motivation for saving and restoring the called arguments, otherwise lazy_scan_heap() would have to clean up after the function it called, which was unclean. Now, every function cleans up after itself. Does that address your comment ? > +static void > +update_vacuum_error_cbarg(LVRelStats *errcbarg, int phase, BlockNumber blkno, > + char *indname, bool free_oldindname) > > I'm not sure why "free_oldindname" is necessary. Since we initialize > vacrelstats->indname with NULL and revert the callback arguments at > the end of functions that needs update them, vacrelstats->indname is > NULL at the beginning of lazy_vacuum_index() and lazy_cleanup_index(). > And we make a copy of index name in update_vacuum_error_cbarg(). So I > think we can pfree the old index name if errcbarg->indname is not NULL. We want to avoid doing this: olderrcbarg = *vacrelstats // saves a pointer update_vacuum_error_cbarg(... NULL); // frees the pointer and sets indname to NULL update_vacuum_error_cbarg(... olderrcbarg.oldindnam) // puts back the pointer, which has been freed // hit an error, and the callback accesses the pfreed pointer I think that's only an issue for lazy_vacuum_index(). And I think you're right: we only save state when the calling function has a indname=NULL, so we never "put back" a non-NULL indname. We go from having a indname=NULL at lazy_scan_heap to not not-NULL at lazy_vacuum_index, and never the other way around. So once we've "reverted back", 1) the pointer is null; and, 2) the callback function doesn't access it for the previous/reverted phase anyway. Hm, I was just wondering what happens if an error happens *during* update_vacuum_error_cbarg(). It seems like if we set errcbarg->phase=VACUUM_INDEX before setting errcbarg->indname=indname, then an error would cause a crash. And if we pfree and set indname before phase, it'd be a problem when going from an index phase to non-index phase. So maybe we have to set errcbarg->phase=VACUUM_ERRCB_PHASE_UNKNOWN while in the function, and errcbarg->phase=phase last. -- Justin
On Thu, Mar 26, 2020 at 10:04:57AM -0500, Justin Pryzby wrote: > Does that address your comment ? I hope so. > > I'm not sure why "free_oldindname" is necessary. Since we initialize > > vacrelstats->indname with NULL and revert the callback arguments at > > the end of functions that needs update them, vacrelstats->indname is > > NULL at the beginning of lazy_vacuum_index() and lazy_cleanup_index(). > > And we make a copy of index name in update_vacuum_error_cbarg(). So I > > think we can pfree the old index name if errcbarg->indname is not NULL. > > We want to avoid doing this: > olderrcbarg = *vacrelstats // saves a pointer > update_vacuum_error_cbarg(... NULL); // frees the pointer and sets indname to NULL > update_vacuum_error_cbarg(... olderrcbarg.oldindnam) // puts back the pointer, which has been freed > // hit an error, and the callback accesses the pfreed pointer > > I think that's only an issue for lazy_vacuum_index(). > > And I think you're right: we only save state when the calling function has a > indname=NULL, so we never "put back" a non-NULL indname. We go from having a > indname=NULL at lazy_scan_heap to not not-NULL at lazy_vacuum_index, and never > the other way around. So once we've "reverted back", 1) the pointer is null; > and, 2) the callback function doesn't access it for the previous/reverted phase > anyway. I removed the free_oldindname argument. > Hm, I was just wondering what happens if an error happens *during* > update_vacuum_error_cbarg(). It seems like if we set > errcbarg->phase=VACUUM_INDEX before setting errcbarg->indname=indname, then an > error would cause a crash. And if we pfree and set indname before phase, it'd > be a problem when going from an index phase to non-index phase. So maybe we > have to set errcbarg->phase=VACUUM_ERRCB_PHASE_UNKNOWN while in the function, > and errcbarg->phase=phase last. And addressed that. Also, I realized that lazy_cleanup_index has an early "return", so the "Revert back" was ineffective. We talked about how that wasn't needed, since we never go back to a previous phase. Amit wanted to keep it there for consistency, but I'd prefer to put any extra effort into calling out the special treatment needed/given to lazy_vacuum_heap/index, rather than making everything "consistent". Amit: I also moved the TRUNCATE_HEAP bit back to truncate_heap(), since 1) it's odd if we don't have anything in truncate_heap() about error reporting except for "vacrelstats->blkno = blkno"; and, 2) it's nice to set the err callback arg right after pgstat_progress, and outside of any loop. In previous versions, it was within the loop, because it closely wrapped RelationTruncate() and count_nondeletable_pages() - a previous version used separate phases. -- Justin
Attachment
On 2020-Mar-26, Justin Pryzby wrote: > On Thu, Mar 26, 2020 at 10:04:57AM -0500, Justin Pryzby wrote: > > And I think you're right: we only save state when the calling function has a > > indname=NULL, so we never "put back" a non-NULL indname. We go from having a > > indname=NULL at lazy_scan_heap to not not-NULL at lazy_vacuum_index, and never > > the other way around. > > I removed the free_oldindname argument. Hah, I was wondering about that free_oldindname business this morning as well. > > ... So once we've "reverted back", 1) the pointer is null; and, 2) > > the callback function doesn't access it for the previous/reverted > > phase anyway. BTW I'm pretty sure this "revert back" phrasing is not good English -- you should just use "revert". Maybe get some native speaker's opinion on it. And speaking of language, I find the particle "cbarg" rather very ugly, and it's *everywhere* -- function name, function argument, local variable, enum values, enum name. It even spread to the typedefs.list file! Is this a new virus??? Put some soap in it! Can't we use "info" or "state" or something similar, less infectious, instead? Thanks -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Mar 26, 2020 at 07:49:51PM -0300, Alvaro Herrera wrote: > > > ... So once we've "reverted back", 1) the pointer is null; and, 2) > > > the callback function doesn't access it for the previous/reverted > > > phase anyway. > > BTW I'm pretty sure this "revert back" phrasing is not good English -- > you should just use "revert". Maybe get some native speaker's opinion > on it. I'm a native speaker; "revert back" might be called redundant but I think it's common usage. > And speaking of language, I find the particle "cbarg" rather very ugly, > and it's *everywhere* -- function name, function argument, local > variable, enum values, enum name. It even spread to the typedefs.list > file! Is this a new virus??? Put some soap in it! Can't we use "info" > or "state" or something similar, less infectious, instead? I renamed it since it was kind of opaque looking. It's in all the same places, so equally infectious; but I hope you like it better. Cheers, -- Justin
Attachment
On 2020-Mar-26, Justin Pryzby wrote: > On Thu, Mar 26, 2020 at 07:49:51PM -0300, Alvaro Herrera wrote: > > BTW I'm pretty sure this "revert back" phrasing is not good English -- > > you should just use "revert". Maybe get some native speaker's opinion > > on it. > > I'm a native speaker; "revert back" might be called redundant but I think it's > common usage. Oh, okay. > > And speaking of language, I find the particle "cbarg" rather very ugly, > > I renamed it since it was kind of opaque looking. It's in all the same places, > so equally infectious; but I hope you like it better. I like it much better, thanks :-) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Mar 27, 2020 at 3:47 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > Hm, I was just wondering what happens if an error happens *during* > > update_vacuum_error_cbarg(). It seems like if we set > > errcbarg->phase=VACUUM_INDEX before setting errcbarg->indname=indname, then an > > error would cause a crash. > > Can't that be avoided if you check if cbarg->indname is non-null in vacuum_error_callback as we are already doing for VACUUM_ERRCB_PHASE_TRUNCATE? > > And if we pfree and set indname before phase, it'd > > be a problem when going from an index phase to non-index phase. How is it possible that we move to the non-index phase without clearing indname as we always revert back the old phase information? I think it is possible only if we don't clear indname after the phase is over. > > So maybe we > > have to set errcbarg->phase=VACUUM_ERRCB_PHASE_UNKNOWN while in the function, > > and errcbarg->phase=phase last. I find that a bit ad-hoc, if possible, let's try to avoid it. > > And addressed that. > > Also, I realized that lazy_cleanup_index has an early "return", so the "Revert > back" was ineffective. > We can call it immediately after index_vacuum_cleanup to avoid that. > We talked about how that wasn't needed, since we never > go back to a previous phase. Amit wanted to keep it there for consistency, but > I'd prefer to put any extra effort into calling out the special treatment > needed/given to lazy_vacuum_heap/index, rather than making everything > "consistent". > Apart from being consistent, the point was it doesn't seem good that API being called to assume that there is nothing more the caller can do. It might be problematic if we later want to enhance or add something to the caller. > Amit: I also moved the TRUNCATE_HEAP bit back to truncate_heap(), There is no problem with it. We can do it either way and I have also considered it the way you have done but decide to keep in the caller because of the previous point I mentioned (not sure if it a good idea that API being called can assume that there is nothing more the caller can do after this). -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Fri, Mar 27, 2020 at 09:49:29AM +0530, Amit Kapila wrote:
> On Fri, Mar 27, 2020 at 3:47 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > > Hm, I was just wondering what happens if an error happens *during*
> > > update_vacuum_error_cbarg(). It seems like if we set
> > > errcbarg->phase=VACUUM_INDEX before setting errcbarg->indname=indname, then an
> > > error would cause a crash.
> > >
>
> Can't that be avoided if you check if cbarg->indname is non-null in
> vacuum_error_callback as we are already doing for
> VACUUM_ERRCB_PHASE_TRUNCATE?
>
> > > And if we pfree and set indname before phase, it'd
> > > be a problem when going from an index phase to non-index phase.
>
> How is it possible that we move to the non-index phase without
> clearing indname as we always revert back the old phase information?
The crash scenario I'm trying to avoid would be like statement_timeout or other
asynchronous event occurring between two non-atomic operations.
I said that there's an issue no matter what order we set indname/phase;
If we wrote:
|cbarg->indname = indname;
|cbarg->phase = phase;
..and hit a timeout (or similar) between setting indname=NULL but before
setting phase=VACUUM_INDEX, then we can crash due to null pointer.
But if we write:
|cbarg->phase = phase;
|if (cbarg->indname) {pfree(cbarg->indname);}
|cbarg->indname = indname ? pstrdup(indname) : NULL;
..then we can still crash if we timeout between freeing cbarg->indname and
setting it to null, due to acccessing a pfreed allocation.
> > > So maybe we
> > > have to set errcbarg->phase=VACUUM_ERRCB_PHASE_UNKNOWN while in the function,
> > > and errcbarg->phase=phase last.
>
> I find that a bit ad-hoc, if possible, let's try to avoid it.
I think we can do what you suggesting, if the callback checks if (cbarg->indname!=NULL).
We'd have to write:
// Must set indname *before* updating phase, in case an error occurs before
// phase is set, to avoid crashing if we're going from an index phase to a
// non-index phase (which should not read indname). Must not free indname
// until it's set to null.
char *tmp = cbarg->indname;
cbarg->indname = indname ? pstrdup(indname) : NULL;
cbarg->phase = phase;
if (tmp){pfree(tmp);}
Do you think that's better ?
--
Justin
On Thu, Mar 26, 2020 at 11:44:24PM -0500, Justin Pryzby wrote:
> On Fri, Mar 27, 2020 at 09:49:29AM +0530, Amit Kapila wrote:
> > On Fri, Mar 27, 2020 at 3:47 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > > Hm, I was just wondering what happens if an error happens *during*
> > > > update_vacuum_error_cbarg(). It seems like if we set
> > > > errcbarg->phase=VACUUM_INDEX before setting errcbarg->indname=indname, then an
> > > > error would cause a crash.
> > > >
> >
> > Can't that be avoided if you check if cbarg->indname is non-null in
> > vacuum_error_callback as we are already doing for
> > VACUUM_ERRCB_PHASE_TRUNCATE?
> >
> > > > And if we pfree and set indname before phase, it'd
> > > > be a problem when going from an index phase to non-index phase.
> >
> > How is it possible that we move to the non-index phase without
> > clearing indname as we always revert back the old phase information?
>
> The crash scenario I'm trying to avoid would be like statement_timeout or other
> asynchronous event occurring between two non-atomic operations.
>
> I said that there's an issue no matter what order we set indname/phase;
> If we wrote:
> |cbarg->indname = indname;
> |cbarg->phase = phase;
> ..and hit a timeout (or similar) between setting indname=NULL but before
> setting phase=VACUUM_INDEX, then we can crash due to null pointer.
>
> But if we write:
> |cbarg->phase = phase;
> |if (cbarg->indname) {pfree(cbarg->indname);}
> |cbarg->indname = indname ? pstrdup(indname) : NULL;
> ..then we can still crash if we timeout between freeing cbarg->indname and
> setting it to null, due to acccessing a pfreed allocation.
If "phase" is updated before "indname", I'm able to induce a synthetic crash
like this:
+if (errinfo->phase==VACUUM_ERRCB_PHASE_VACUUM_INDEX && errinfo->indname==NULL)
+{
+kill(getpid(), SIGINT);
+pg_sleep(1); // that's needed since signals are delivered asynchronously
+}
And another crash if we do this after pfree but before setting indname.
+if (errinfo->phase==VACUUM_ERRCB_PHASE_VACUUM_INDEX && errinfo->indname!=NULL)
+{
+kill(getpid(), SIGINT);
+pg_sleep(1);
+}
I'm not sure if those are possible outside of "induced" errors. Maybe the
function is essentially atomic due to no CHECK_FOR_INTERRUPTS or similar?
--
Justin
On Fri, Mar 27, 2020 at 11:46 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Mar 26, 2020 at 11:44:24PM -0500, Justin Pryzby wrote:
> > On Fri, Mar 27, 2020 at 09:49:29AM +0530, Amit Kapila wrote:
> > > On Fri, Mar 27, 2020 at 3:47 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > >
> > > > > Hm, I was just wondering what happens if an error happens *during*
> > > > > update_vacuum_error_cbarg(). It seems like if we set
> > > > > errcbarg->phase=VACUUM_INDEX before setting errcbarg->indname=indname, then an
> > > > > error would cause a crash.
> > > > >
> > >
> > > Can't that be avoided if you check if cbarg->indname is non-null in
> > > vacuum_error_callback as we are already doing for
> > > VACUUM_ERRCB_PHASE_TRUNCATE?
> > >
> > > > > And if we pfree and set indname before phase, it'd
> > > > > be a problem when going from an index phase to non-index phase.
> > >
> > > How is it possible that we move to the non-index phase without
> > > clearing indname as we always revert back the old phase information?
> >
> > The crash scenario I'm trying to avoid would be like statement_timeout or other
> > asynchronous event occurring between two non-atomic operations.
> >
> > I said that there's an issue no matter what order we set indname/phase;
> > If we wrote:
> > |cbarg->indname = indname;
> > |cbarg->phase = phase;
> > ..and hit a timeout (or similar) between setting indname=NULL but before
> > setting phase=VACUUM_INDEX, then we can crash due to null pointer.
> >
> > But if we write:
> > |cbarg->phase = phase;
> > |if (cbarg->indname) {pfree(cbarg->indname);}
> > |cbarg->indname = indname ? pstrdup(indname) : NULL;
> > ..then we can still crash if we timeout between freeing cbarg->indname and
> > setting it to null, due to acccessing a pfreed allocation.
>
> If "phase" is updated before "indname", I'm able to induce a synthetic crash
> like this:
>
> +if (errinfo->phase==VACUUM_ERRCB_PHASE_VACUUM_INDEX && errinfo->indname==NULL)
> +{
> +kill(getpid(), SIGINT);
> +pg_sleep(1); // that's needed since signals are delivered asynchronously
> +}
>
> And another crash if we do this after pfree but before setting indname.
>
> +if (errinfo->phase==VACUUM_ERRCB_PHASE_VACUUM_INDEX && errinfo->indname!=NULL)
> +{
> +kill(getpid(), SIGINT);
> +pg_sleep(1);
> +}
>
> I'm not sure if those are possible outside of "induced" errors. Maybe the
> function is essentially atomic due to no CHECK_FOR_INTERRUPTS or similar?
>
Yes, this is exactly the point. I think unless you have
CHECK_FOR_INTERRUPTS in that function, the problems you are trying to
think won't happen.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Fri, 27 Mar 2020 at 07:17, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Thu, Mar 26, 2020 at 10:04:57AM -0500, Justin Pryzby wrote: > > Does that address your comment ? > > I hope so. Thank you for updating the patch. I'm concerned a bit about overhead of frequently updating and reverting the callback arguments in lazy_vacuum_page(). We call that function every time when we vacuum a page, but if the table has an index, we actually don't need to update the callback arguments in that function. But I hope it's negligible since all operation will be performed on memory. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Mar 27, 2020 at 1:29 PM Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote: > > On Fri, 27 Mar 2020 at 07:17, Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > On Thu, Mar 26, 2020 at 10:04:57AM -0500, Justin Pryzby wrote: > > > Does that address your comment ? > > > > I hope so. > > Thank you for updating the patch. I'm concerned a bit about overhead > of frequently updating and reverting the callback arguments in > lazy_vacuum_page(). We call that function every time when we vacuum a > page, but if the table has an index, we actually don't need to update > the callback arguments in that function. But I hope it's negligible > since all operation will be performed on memory. > Right, it will be a few instructions. I think if there is any overhead of this, we can easily avoid that by (a) adding a check in update_vacuum_error_cbarg which tells if the phase is getting changed or not and if it is not changed, then return, (b) pass additional in lazy_vacuum_page() to indicate whether we need to change the phase, (c) just invoke update_vacuum_error_cbarg() in the caller. The current way appears to be a bit neat than these options, so not sure if there is an advantage in changing it. Anyway, if we see any problem with that it is trivial to change it. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Fri, Mar 27, 2020 at 11:50:30AM +0530, Amit Kapila wrote:
> > > The crash scenario I'm trying to avoid would be like statement_timeout or other
> > > asynchronous event occurring between two non-atomic operations.
> > >
> > +if (errinfo->phase==VACUUM_ERRCB_PHASE_VACUUM_INDEX && errinfo->indname==NULL)
> > +{
> > +kill(getpid(), SIGINT);
> > +pg_sleep(1); // that's needed since signals are delivered asynchronously
> > +}
> > I'm not sure if those are possible outside of "induced" errors. Maybe the
> > function is essentially atomic due to no CHECK_FOR_INTERRUPTS or similar?
>
> Yes, this is exactly the point. I think unless you have
> CHECK_FOR_INTERRUPTS in that function, the problems you are trying to
> think won't happen.
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. I'm
not convinced that the function couldn't be interrupted by a signal.
--
Justin
On Sat, Mar 28, 2020 at 12:34 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Fri, Mar 27, 2020 at 11:50:30AM +0530, Amit Kapila wrote:
> > > > The crash scenario I'm trying to avoid would be like statement_timeout or other
> > > > asynchronous event occurring between two non-atomic operations.
> > > >
> > > +if (errinfo->phase==VACUUM_ERRCB_PHASE_VACUUM_INDEX && errinfo->indname==NULL)
> > > +{
> > > +kill(getpid(), SIGINT);
> > > +pg_sleep(1); // that's needed since signals are delivered asynchronously
> > > +}
> > > I'm not sure if those are possible outside of "induced" errors. Maybe the
> > > function is essentially atomic due to no CHECK_FOR_INTERRUPTS or similar?
> >
> > Yes, this is exactly the point. I think unless you have
> > CHECK_FOR_INTERRUPTS in that function, the problems you are trying to
> > think won't happen.
>
> 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?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
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. -- Justin
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. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
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. -- Justin
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? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Attachment
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
On Sun, Mar 29, 2020 at 9:04 AM Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote: > > On Sat, 28 Mar 2020 at 13:23, Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > 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. > Okay, I will push this tomorrow. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Sun, Mar 29, 2020 at 11:35 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Sun, Mar 29, 2020 at 9:04 AM Masahiko Sawada > <masahiko.sawada@2ndquadrant.com> wrote: > > > > On Sat, 28 Mar 2020 at 13:23, Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > > > > 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. > > > > Okay, I will push this tomorrow. > Pushed. I see one buildfarm failure [1] but that doesn't seem to be related to this patch. [1] - https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=petalura&dt=2020-03-30%2002%3A20%3A03 -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Fri, Mar 27, 2020 at 5:03 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > Now that the main patch is committed, I have reviewed the other two patches. v37-0002-Drop-reltuples 1. @@ -2289,11 +2289,10 @@ vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats, /* Do vacuum or cleanup of the index */ if (lvshared->for_cleanup) - lazy_cleanup_index(indrel, stats, lvshared->reltuples, - lvshared->estimated_count, vacrelstats); + lazy_cleanup_index(indrel, stats, vacrelstats); else lazy_vacuum_index(indrel, stats, dead_tuples, - lvshared->reltuples, vacrelstats); + vacrelstats); I don't think the above change is correct. How will vacrelstats have correct values when vacuum_one_index is called via parallel workers (via parallel_vacuum_main)? The v37-0003-Avoid-some-calls-to-RelationGetRelationName.patch looks good to me. I have added the commit message in the patch. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Attachment
On Mon, Mar 30, 2020 at 02:31:53PM +0530, Amit Kapila wrote: > Now that the main patch is committed, I have reviewed the other two patches. Thanks for that On Mon, Mar 30, 2020 at 02:31:53PM +0530, Amit Kapila wrote: > The v37-0003-Avoid-some-calls-to-RelationGetRelationName.patch looks > good to me. I have added the commit message in the patch. I realized the 0003 patch has an error in lazy_vacuum_index; it should be: - RelationGetRelationName(indrel), + vacrelstats->indname, That was maybe due to originally using a separate errinfo for each phase, with one "char *relname" and no "char *indrel". > I don't think the above change is correct. How will vacrelstats have > correct values when vacuum_one_index is called via parallel workers > (via parallel_vacuum_main)? You're right: parallel main's vacrelstats was added by this patchset and only the error context fields were initialized. I fixed it up in the attached by also setting vacrelstats->new_rel_tuples and old_live_tuples. It's not clear if this is worth it just to save an argument to two functions? -- Justin
Attachment
On Mon, Mar 30, 2020 at 9:56 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Mon, Mar 30, 2020 at 02:31:53PM +0530, Amit Kapila wrote: > > The v37-0003-Avoid-some-calls-to-RelationGetRelationName.patch looks > > good to me. I have added the commit message in the patch. > > I realized the 0003 patch has an error in lazy_vacuum_index; it should be: > > - RelationGetRelationName(indrel), > + vacrelstats->indname, > Hmm, it is like that in the patch I have sent yesterday. Are you referring to the patch I have sent yesterday or some older version? One thing I have noticed is that there is some saving by using vacrelstats->relnamespace as that avoids sys cache lookup. OTOH, using vacrelstats->relname doesn't save much, but maybe for the sake of consistency, we can use it. > That was maybe due to originally using a separate errinfo for each phase, with > one "char *relname" and no "char *indrel". > > > I don't think the above change is correct. How will vacrelstats have > > correct values when vacuum_one_index is called via parallel workers > > (via parallel_vacuum_main)? > > You're right: parallel main's vacrelstats was added by this patchset and only > the error context fields were initialized. I fixed it up in the attached by > also setting vacrelstats->new_rel_tuples and old_live_tuples. It's not clear > if this is worth it just to save an argument to two functions? > Right, it is not clear to me whether that is an improvement, so I suggest let's leave that patch for now. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 31, 2020 at 07:50:45AM +0530, Amit Kapila wrote: > On Mon, Mar 30, 2020 at 9:56 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > On Mon, Mar 30, 2020 at 02:31:53PM +0530, Amit Kapila wrote: > > > The v37-0003-Avoid-some-calls-to-RelationGetRelationName.patch looks > > > good to me. I have added the commit message in the patch. > > > > I realized the 0003 patch has an error in lazy_vacuum_index; it should be: > > > > - RelationGetRelationName(indrel), > > + vacrelstats->indname, > > > > Hmm, it is like that in the patch I have sent yesterday. Are you > referring to the patch I have sent yesterday or some older version? Oh good. That was a recent fix I made, and I was afraid I'd never sent it, and not sure if you'd used it. Looks like it was fixed since v36... As you can see, I'm losing track of my branches. It will be nice to finally put this to rest. > One thing I have noticed is that there is some saving by using > vacrelstats->relnamespace as that avoids sys cache lookup. OTOH, > using vacrelstats->relname doesn't save much, but maybe for the sake > of consistency, we can use it. Mostly I wrote that to avoid repeatedly calling functions/macro with long name. I consider it a minor cleanup. I think we should put them to use. The LVRelStats describes them as not being specifically for the error context. -- Justin
On Tue, Mar 31, 2020 at 8:53 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Tue, Mar 31, 2020 at 07:50:45AM +0530, Amit Kapila wrote: > > One thing I have noticed is that there is some saving by using > > vacrelstats->relnamespace as that avoids sys cache lookup. OTOH, > > using vacrelstats->relname doesn't save much, but maybe for the sake > > of consistency, we can use it. > > Mostly I wrote that to avoid repeatedly calling functions/macro with long name. > I consider it a minor cleanup. I think we should put them to use. The > LVRelStats describes them as not being specifically for the error context. > Pushed. I think we are done here. The patch is marked as committed in CF. Thank you! -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On 2020-04-01 07:54:45 +0530, Amit Kapila wrote: > Pushed. I think we are done here. The patch is marked as committed in > CF. Thank you! Awesome! Thanks for all your work on this, all. This'll make it a lot easier to debug errors during autovacuum.
On 2020-Apr-01, Andres Freund wrote: > On 2020-04-01 07:54:45 +0530, Amit Kapila wrote: > > Pushed. I think we are done here. The patch is marked as committed in > > CF. Thank you! > > Awesome! Thanks for all your work on this, all. This'll make it a lot > easier to debug errors during autovacuum. Seconded! -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services