Re: New vacuum option to do only freezing

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

In response to

Responses

Browse pgsql-hackers by date

  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