From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
Cc: | "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Peter Geoghegan <pg(at)bowt(dot)ie>, Andres Freund <andres(at)anarazel(dot)de> |
Subject: | Re: parallel vacuum comments |
Date: | 2021-12-16 04:55:52 |
Message-ID: | CAA4eK1KSSBy+ptmXZBYMgfrxFDbA2VaTG7Wsu2BTwE94SEQ50Q@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Dec 15, 2021 at 1:33 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've attached an updated patch. The patch incorporated several changes
> from the last version:
>
> * Rename parallel_vacuum_begin() to parallel_vacuum_init()
> * Unify the terminology; use "index bulk-deletion" and "index cleanup"
> instead of "index vacuum" and "index cleanup".
>
I am not sure it is a good idea to do this as part of the main patch
as the intention of that is to just refactor parallel vacuum code. I
suggest doing this as a separate patch. Also, can we move the common
code to be shared between vacuumparallel.c and vacuumlazy.c as a
separate patch?
Few other comments and questions:
============================
1. /* Outsource everything to parallel variant */
- parallel_vacuum_process_all_indexes(vacrel, true);
+ LVSavedErrInfo saved_err_info;
+
+ /*
+ * Outsource everything to parallel variant. Since parallel vacuum will
+ * set the error context on an error we temporarily disable setting our
+ * error context.
+ */
+ update_vacuum_error_info(vacrel, &saved_err_info,
+ VACUUM_ERRCB_PHASE_UNKNOWN,
+ InvalidBlockNumber, InvalidOffsetNumber);
+
+ parallel_vacuum_bulkdel_all_indexes(vacrel->pvs, vacrel->old_live_tuples);
+
+ /* Revert to the previous phase information for error traceback */
+ restore_vacuum_error_info(vacrel, &saved_err_info);
Is this change because you want a separate error callback for parallel
vacuum? If so, I suggest we can discuss this as a separate patch from
the refactoring patch.
2. Is introducing bulkdel_one_index/cleanup_one_index related to new
error context, or "Unify the terminology" task? Is there any other
reason for the same?
3. Why did you introduce
parallel_vacuum_bulkdel_all_indexes()/parallel_vacuum_cleanup_all_indexes()?
Is it because of your task "Unify the terminology"?
4.
@@ -3086,7 +2592,6 @@ lazy_cleanup_one_index(Relation indrel,
IndexBulkDeleteResult *istat,
ivinfo.report_progress = false;
ivinfo.estimated_count = estimated_count;
ivinfo.message_level = elevel;
-
ivinfo.num_heap_tuples = reltuples;
This seems like an unrelated change.
--
With Regards,
Amit Kapila.
From | Date | Subject | |
---|---|---|---|
Next Message | Justin Pryzby | 2021-12-16 04:58:04 | Re: pg_dump versus ancient server versions |
Previous Message | Tom Lane | 2021-12-16 04:55:41 | Re: pg_dump versus ancient server versions |