From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Strange assertion using VACOPT_FREEZE in vacuum.c |
Date: | 2015-03-06 06:39:07 |
Message-ID: | CAB7nPqR-PR05ZpOrb-VTGELRWj3vmr2=noebn7Yic3wSNf9DcQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Mar 6, 2015 at 12:44 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Mar 6, 2015 at 12:42 AM, Robert Haas wrote:
>> On Thu, Mar 5, 2015 at 9:58 AM, Alvaro Herrera wrote:
>>> Here's a simple idea to improve the patch: make VacuumParams include
>>> VacuumStmt and the for_wraparound and do_toast flags. ISTM that reduces
>>> the number of arguments to be passed down in a couple of places. In
>>> particular:
>>>
>>> vacuum(VacuumParams *params, BufferAccessStrategy bstrategy, bool isTopLevel)
>>>
>>> vacuum_rel(VacuumParams *params)
>>>
>>> Does that sound more attractive?
>>
>> I dislike passing down parser nodes straight into utility commands.
>> It tends to make those those functions hard for in-core users to call,
>> and also to lead to security vulnerabilities where we look up the same
>> names more than once and just hope that we get the same OID every
>> time. Stuffing the VacuumStmt pointer inside the VacuumParams object
>> doesn't, for me, help anything. It'd be a lot more interesting if we
>> could get rid of that altogether.
>
> Do you mean removing totally VacuumStmt from the stack? We would then
> need to add relation and va_cols as additional arguments of things
> like vacuum_rel, analyze_rel, do_analyze_rel or similar.
>
> FWIW, adding do_toast and for_wraparound into VacuumParams makes sense
> to me, but not VacuumStmt. It has little meaning as VacuumParams
> should be used for parameters.
But code may tell more than words, so here is some. I noticed that
moving for_wraparound in VacuumParams makes more sense than relid and
do_toast as those values need special handling when vacuum_rel is
called for a toast relation. For the patches that are separated for
clarity:
- 0001 is the previous one
- 0002 removes VacuumStmt from the call stack of ANALYZE and VACUUM routines
- 0003 moves for_wraparound in VacuumParams.
Regards,
--
Michael
Attachment | Content-Type | Size |
---|---|---|
0001-Move-freeze-parameters-of-VacuumStmt-into-a-separate.patch | application/x-patch | 15.8 KB |
0002-Eliminate-VacuumStmt-from-lower-level-routines-of-AN.patch | application/x-patch | 12.4 KB |
0003-Add-wraparound-control-parameters-in-VacuumStmt.patch | application/x-patch | 5.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | David Rowley | 2015-03-06 07:11:09 | Re: Combining Aggregates |
Previous Message | Ashutosh Bapat | 2015-03-06 06:01:14 | Re: Combining Aggregates |