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 autovacu
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 Tue, Mar 31, 2020 at 8:53 AM Justin Pryzby 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
On Tue, Mar 31, 2020 at 07:50:45AM +0530, Amit Kapila wrote:
> On Mon, Mar 30, 2020 at 9:56 PM Justin Pryzby 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 co
On Mon, Mar 30, 2020 at 9:56 PM Justin Pryzby 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 i
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
On Fri, Mar 27, 2020 at 5:03 AM Justin Pryzby 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
On Sun, Mar 29, 2020 at 11:35 AM Amit Kapila wrote:
>
> On Sun, Mar 29, 2020 at 9:04 AM Masahiko Sawada
> wrote:
> >
> > On Sat, 28 Mar 2020 at 13:23, Amit Kapila wrote:
> > >
> > >
> > > Please find attached the updated patch with all the changes discussed.
> > > Let me know if I have missed an
On Sun, Mar 29, 2020 at 9:04 AM Masahiko Sawada
wrote:
>
> On Sat, 28 Mar 2020 at 13:23, Amit Kapila 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.
>
On Sat, 28 Mar 2020 at 13:23, Amit Kapila wrote:
>
> On Sat, Mar 28, 2020 at 7:04 AM Justin Pryzby 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
> > > wrote:
> > > >
> > > > On Sat, Mar 28, 2020 at 06:28:38AM +053
On Sat, Mar 28, 2020 at 7:04 AM Justin Pryzby 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 wrote:
> > >
> > > On Sat, Mar 28, 2020 at 06:28:38AM +0530, Amit Kapila wrote:
> > > > > Hm, but I caused a crash *without* addi
On Sat, Mar 28, 2020 at 06:59:10AM +0530, Amit Kapila wrote:
> On Sat, Mar 28, 2020 at 6:46 AM Justin Pryzby 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() coul
On Sat, Mar 28, 2020 at 6:46 AM Justin Pryzby 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()
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.
>
On Sat, Mar 28, 2020 at 12:34 AM Justin Pryzby 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.
> > > >
>
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 &&
> > erri
On Fri, Mar 27, 2020 at 1:29 PM Masahiko Sawada
wrote:
>
> On Fri, 27 Mar 2020 at 07:17, Justin Pryzby 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
On Fri, 27 Mar 2020 at 07:17, Justin Pryzby 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 argument
On Fri, Mar 27, 2020 at 11:46 AM Justin Pryzby 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
> > > wrote:
> > > >
> > > > > Hm, I was just wondering wh
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 wrote:
> > >
> > > > Hm, I was just wondering what happens if an error happens *during*
> > > > update_vacuum_error_cbarg(
On Fri, Mar 27, 2020 at 09:49:29AM +0530, Amit Kapila wrote:
> On Fri, Mar 27, 2020 at 3:47 AM Justin Pryzby 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 set
On Fri, Mar 27, 2020 at 3:47 AM Justin Pryzby 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
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; "rever
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 Engli
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_sc
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
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;
> boolall_frozen;
> + LVRelStats olderrcbarg;
>
On Thu, 26 Mar 2020 at 15:34, Amit Kapila wrote:
>
> On Thu, Mar 26, 2020 at 12:03 PM Amit Kapila wrote:
> >
> > On Thu, Mar 26, 2020 at 10:11 AM Justin Pryzby wrote:
> > >
> > > Seems fine. Rather than saying "different phases" I, would say:
> > > "The index vacuum and heap vacuum phases may b
On Thu, Mar 26, 2020 at 12:03 PM Amit Kapila wrote:
>
> On Thu, Mar 26, 2020 at 10:11 AM Justin Pryzby 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
On Thu, Mar 26, 2020 at 10:11 AM Justin Pryzby 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 sugg
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 trunc
On Wed, Mar 25, 2020 at 6:11 PM Justin Pryzby wrote:
>
> On Wed, Mar 25, 2020 at 09:27:44PM +0900, Masahiko Sawada wrote:
> > On Wed, 25 Mar 2020 at 20:24, Amit Kapila wrote:
> > >
> > > On Wed, Mar 25, 2020 at 3:42 PM Justin Pryzby
> > > wrote:
> > > >
> > > > Attached patch addressing these.
On Wed, Mar 25, 2020 at 09:27:44PM +0900, Masahiko Sawada wrote:
> On Wed, 25 Mar 2020 at 20:24, Amit Kapila wrote:
> >
> > On Wed, Mar 25, 2020 at 3:42 PM Justin Pryzby wrote:
> > >
> > > Attached patch addressing these.
> > >
> >
> > Thanks, you forgot to remove the below declaration which I ha
On Wed, 25 Mar 2020 at 20:24, Amit Kapila wrote:
>
> On Wed, Mar 25, 2020 at 3:42 PM Justin Pryzby 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 onere
On Wed, Mar 25, 2020 at 04:54:41PM +0530, Amit Kapila wrote:
> On Wed, Mar 25, 2020 at 3:42 PM Justin Pryzby 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 p
On Wed, Mar 25, 2020 at 3:42 PM Justin Pryzby 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_
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 reintroduce
On Wed, 25 Mar 2020 at 14:08, Justin Pryzby 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
> > wrote:
> > >
> > > On Wed, 25 Mar 2020 at 12:44, Amit Kapila wrote:
> > > >
> > > > On Tue, Mar 24, 2020 at 7:51 PM Masahik
On Wed, Mar 25, 2020 at 10:22:21AM +0530, Amit Kapila wrote:
> On Wed, Mar 25, 2020 at 10:05 AM Masahiko Sawada
> wrote:
> >
> > On Wed, 25 Mar 2020 at 12:44, Amit Kapila wrote:
> > >
> > > On Tue, Mar 24, 2020 at 7:51 PM Masahiko Sawada
> > > wrote:
> > > >
> > > >
> > > > I got the point. But
On Wed, Mar 25, 2020 at 10:05 AM Masahiko Sawada
wrote:
>
> On Wed, 25 Mar 2020 at 12:44, Amit Kapila wrote:
> >
> > On Tue, Mar 24, 2020 at 7:51 PM Masahiko Sawada
> > wrote:
> > >
> > >
> > > I got the point. But if we set the error context before that, I think
> > > we need to change the erro
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
On Wed, 25 Mar 2020 at 12:44, Amit Kapila wrote:
>
> On Tue, Mar 24, 2020 at 7:51 PM Masahiko Sawada
> wrote:
> >
> > On Tue, 24 Mar 2020 at 22:37, Amit Kapila wrote:
> > >
> > > On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada
> > > wrote:
> > > >
> > > >
> > > > I've read through the latest ve
On Wed, Mar 25, 2020 at 8:49 AM Justin Pryzby 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 VacuumEr
On Tue, Mar 24, 2020 at 7:51 PM Masahiko Sawada
wrote:
>
> On Tue, 24 Mar 2020 at 22:37, Amit Kapila wrote:
> >
> > On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada
> > wrote:
> > >
> > >
> > > I've read through the latest version patch (v31), here are my comments:
> > >
> > > 1.
> > > +/
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
>From
On Tue, 24 Mar 2020 at 22:37, Amit Kapila wrote:
>
> On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada
> wrote:
> >
> >
> > I've read through the latest version patch (v31), here are my comments:
> >
> > 1.
> > +/* Update error traceback information */
> > +olderrcbarg = *vacrelstat
On Tue, Mar 24, 2020 at 7:18 PM Justin Pryzby 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
> > wrote:
> > > 1.
> > > +/* Update error traceback information */
> > > +olderrcbarg = *vacrelstats;
> > > +
On Tue, Mar 24, 2020 at 07:07:03PM +0530, Amit Kapila wrote:
> On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada
> wrote:
> > 1.
> > +/* Update error traceback information */
> > +olderrcbarg = *vacrelstats;
> > +update_vacuum_error_cbarg(vacrelstats,
> > +
On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada
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,
> +
On Tue, 24 Mar 2020 at 18:19, Amit Kapila wrote:
>
> On Tue, Mar 24, 2020 at 2:37 PM Masahiko Sawada
> wrote:
> >
> > On Tue, 24 Mar 2020 at 13:53, Amit Kapila wrote:
> > >
> > > On Tue, Mar 24, 2020 at 9:46 AM Justin Pryzby
> > > wrote:
> > > >
> > > > On Mon, Mar 23, 2020 at 02:25:14PM +0530
On Tue, Mar 24, 2020 at 2:37 PM Masahiko Sawada
wrote:
>
> On Tue, 24 Mar 2020 at 13:53, Amit Kapila wrote:
> >
> > On Tue, Mar 24, 2020 at 9:46 AM Justin Pryzby wrote:
> > >
> > > On Mon, Mar 23, 2020 at 02:25:14PM +0530, Amit Kapila wrote:
> > > > > Yea, and it would be misleading if we report
On Tue, 24 Mar 2020 at 13:53, Amit Kapila wrote:
>
> On Tue, Mar 24, 2020 at 9:46 AM Justin Pryzby 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 w
On Tue, Mar 24, 2020 at 9:46 AM Justin Pryzby 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
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:
> >
On Sat, Mar 21, 2020 at 1:33 PM Justin Pryzby 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
On Mon, Mar 23, 2020 at 1:46 PM Justin Pryzby 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 comm
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
On Sat, 21 Mar 2020 at 16:30, Amit Kapila wrote:
>
> On Fri, Mar 20, 2020 at 8:24 PM Justin Pryzby 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
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
On Fri, Mar 20, 2020 at 8:24 PM Justin Pryzby 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:
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
On Fri, Mar 20, 2020 at 12:21 PM Justin Pryzby 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 wrote:
> > That makes sense. I have a few more comments:
> >
> > 1.
> > + VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
> > +} errcb_phase;
On Fri, Mar 20, 2020 at 11:24:25AM +0530, Amit Kapila wrote:
> On Fri, Mar 20, 2020 at 5:59 AM Justin Pryzby 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
On Fri, Mar 20, 2020 at 5:59 AM Justin Pryzby 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(vacre
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,
> |
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 i
On Thu, Mar 19, 2020 at 9:38 AM Justin Pryzby 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
On Thu, Mar 19, 2020 at 08:20:51AM +0530, Amit Kapila wrote:
> On Tue, Mar 17, 2020 at 11:51 AM Amit Kapila wrote:
> > On Tue, Mar 17, 2020 at 9:52 AM Amit Kapila wrote:
> > >
> > > Another minor point, don't we need to remove the error stack by doing
> > > "error_context_stack = errcallback.prev
On Tue, Mar 17, 2020 at 11:51 AM Amit Kapila wrote:
>
> On Tue, Mar 17, 2020 at 9:52 AM Amit Kapila 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.
On Tue, Mar 17, 2020 at 9:52 AM Amit Kapila 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_I
On Tue, Mar 17, 2020 at 9:21 AM Justin Pryzby 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
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 seem
On Mon, Mar 16, 2020 at 7:47 PM Alvaro Herrera 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 = vac
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
On Thu, Mar 5, 2020 at 3:22 AM Justin Pryzby 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_vacu
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->bl
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 upd
On Wed, 4 Mar 2020 at 04:32, 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 vacuum
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
> >
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\"",
> > +
On Fri, 21 Feb 2020 at 02:02, Alvaro Herrera 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 do
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.
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 = errcallb
Alvaro Herrera 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 i
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 cru
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 c
On Mon, 17 Feb 2020 at 15:14, Justin Pryzby 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
>
> > An
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
On Mon, 17 Feb 2020 at 12:57, Justin Pryzby 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 s
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
>
On Sat, 15 Feb 2020 at 00:34, Justin Pryzby 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 th
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
On Fri, 14 Feb 2020 at 08:52, Justin Pryzby 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
On Tue, Jan 21, 2020 at 8:11 AM Andres Freund 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
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 argumen
On Sat, 8 Feb 2020 at 10:01, Justin Pryzby 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
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 oc
On Sun, 2 Feb 2020 at 15:03, Justin Pryzby 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;
> > +
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 inde
On Tue, 28 Jan 2020 at 07:50, Justin Pryzby wrote:
>
> On Mon, Jan 27, 2020 at 03:59:58PM +0900, Masahiko Sawada wrote:
> > On Mon, 27 Jan 2020 at 14:38, Justin Pryzby wrote:
> > > On Sun, Jan 26, 2020 at 12:29:38PM -0800, Andres Freund wrote:
> > > > > CONTEXT: while vacuuming relation "public.
1 - 100 of 138 matches
Mail list logo
