On Thu, Mar 14, 2019 at 6:41 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Mar 13, 2019 at 1:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > I don't have a strong opinion but the using a Node would be more
> > suitable in the future when we add more options to vacuum. And it
> > seems to me that it's unlikely to change a Node to a plain struct. So
> > there is an idea of doing it now anyway if we might need to do it
> > someday.
>
> I just tried to apply 0001 again and noticed a conflict in the
> autovac_table structure in postmaster.c.
>
> That conflict got me thinking: aren't parameters and options an awful
> lot alike? Why do we need to pass around a VacuumOptions structure
> *and* a VacuumParams structure to all of these functions? Couldn't we
> just have one? That led to the attached patch, which just gets rid of
> the separate options flag and folds it into VacuumParams.
Indeed. I like this approach. The comment of vacuum() says,
* options is a bitmask of VacuumOption flags, indicating what to do.
* (snip)
* params contains a set of parameters that can be used to customize the
* behavior.
It seems to me that the purpose of both variables are different. But
it would be acceptable even if we merge them.
BTW your patch seems to not apply to the current HEAD cleanly and to
need to update the comment of vacuum().
> If we took
> this approach, the degree of parallelism would just be another thing
> that would get added to VacuumParams, and VacuumOptions wouldn't end
> up existing at all.
>
Agreed.
> This patch does not address the question of what the *parse tree*
> representation of the PARALLEL option should look like; the idea would
> be that ExecVacuum() would need to extra the value for that option and
> put it into VacuumParams just as it already does for various other
> things in VacuumParams. Maybe the most natural approach would be to
> convert the grammar productions for the VACUUM options list so that
> they just build a list of DefElems, and then have ExecVacuum() iterate
> over that list and make sense of it, as for example ExplainQuery()
> already does.
>
Agreed. That change would help for the discussion changing VACUUM
option syntax to field-and-value style.
Attached the updated patch you proposed and the patch that converts
the grammer productions for the VACUUM option on top of the former
patch. The latter patch moves VacuumOption to vacuum.h since the
parser no longer needs such information.
If we take this direction I will change the parallel vacuum patch so
that it adds new PARALLEL option and adds 'nworkers' to VacuumParams.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center