On Sat, Sep 2, 2017 at 3:00 AM, Bossart, Nathan <bossartn@amazon.com> wrote:
> Don't we have a similar problem with makeVacuumRelation() and list_make1()?
Yeah, indeed. I forgot about this portion.
> I went ahead and moved the RangeVar, VacuumRelation, and List into local
> variables for now, but I agree that this could be improved in a separate
> patch. Perhaps these could be allocated in AutovacMemCxt. I see from
> 4873c96f that autovacuum_do_vac_analyze() used to allocate the list of OIDs
> in that "long-lived" memory context.
Indeed, and this has been removed in 9319fd89 by Álvaro as this API
did not need to be this complicated at this point, but now we have to.
I did not consider first that the list portion also needed to be
modified, perhaps because I am not coding that myself... So now that
it is becoming more complicated what about just using AutovacMemCxt?
This would simplify the list of VacuumRelation entries and the
RangeVar creation as well, and honestly this is ugly and there are no
other similar patterns in the backend code:
+ MemSet(&rel_list, 0, sizeof(rel_list));
+ NodeSetTag(&rel_list, T_List);
+ rel_list.length = 1;
+ rel_list.head = &lc;
+ rel_list.tail = &lc;
+
+ MemSet(&lc, 0, sizeof(lc));
+ lfirst(rel_list.head) = &rel;
This would become way more readable by using makeRangeVar() and the
new makeVacuumRelation. As this is partly my fault that we are at this
state, I am fine as well to remove this burden from you, Nathan, and
fix that myself in a new version. And I don't want to step on your
toes either :)
>> - $$ = (Node *)n;
>> + $$ = (Node *) n;
>> Spurious noise. And the coding pattern in gram.y is to not add a space
>> (make new code look like its surroundings as the documentation says).
>
> I've fixed this in v13.
Thanks.
--
Michael