From: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, "Bossart, Nathan" <bossartn(at)amazon(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: New vacuum option to do only freezing |
Date: | 2019-03-29 16:26:28 |
Message-ID: | CAD21AoAWwB=Jo540fJTnW2mLARC6ht-4NSE_Q=nRbMcJS88bWw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Mar 29, 2019 at 10:46 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Fri, Mar 29, 2019 at 2:16 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > Attached updated patches. These patches are applied on top of 0001
> > patch on parallel vacuum thread[1].
>
> + bool index_cleanup = true; /* by default */
>
> I think we should instead initialize index_cleanup to the reloption
> value, if there is one, or true if none, and then let it be overridden
> by the loop that follows, where whatever the user specifies in the SQL
> command is processed. That way, any explicitly-specified option
> within the command itself wins, and the reloption sets the default.
> As you have it, index cleanup is disabled when the reloption is set to
> false even if the user wrote VACUUM (INDEX_CLEANUP TRUE).
>
Yeah, but since multiple relations might be specified in VACUUM
command we need to process index_cleanup option after opened each
relations. Maybe we need to process all options except for
INDEX_CLEANUP in ExecVacuum() and pass VacuumStmt down to vacuum_rel()
and process it in manner of you suggested after opened the relation.
Is that right?
> + appendStringInfo(&buf,
> + _("%.0f tuples and %.0f item identifiers are left
> as dead.\n"),
> + vacrelstats->nleft_dead_tuples,
> + vacrelstats->nleft_dead_itemids);
>
> I tend to think we should omit this line entirely if both values are
> 0, as will very often be the case.
Fixed.
>
> + if ((params->options & VACOPT_FULL) != 0 &&
> + (params->options & VACOPT_INDEX_CLEANUP) == 0)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("VACUUM option INDEX_CLEANUP cannot be set to
> false with FULL")));
>
> I think it would be better to just ignore the INDEX_CLEANUP option
> when FULL is specified.
Okay, but why do we ignore that in this case while we complain in the
case of FULL and DISABLE_PAGE_SKIPPING?
>
> I wasn't all that happy with the documentation changes you proposed.
> Please find attached a proposed set of doc changes.
Thank you! I've incorporated these changes.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2019-03-29 16:29:33 | Re: partitioned tables referenced by FKs |
Previous Message | Tom Lane | 2019-03-29 16:25:00 | Re: PostgreSQL pollutes the file system |