On Fri, Mar 31, 2023 at 8:05 PM David Rowley <dgrowle...@gmail.com> wrote: > > On Sat, 1 Apr 2023 at 12:57, Melanie Plageman <melanieplage...@gmail.com> > wrote: > > I've attached v7 with that commit dropped and with support for parallel > > vacuum workers to use the same number of buffers in their own Buffer > > Access Strategy ring as the main vacuum phase did. I also updated the > > docs to indicate that vacuum_buffer_usage_limit is per backend (not per > > instance of VACUUM). > > (was just replying about v6-0002 when this came in. Replying here instead) > > For v7-0001, can we just get rid of both of those static globals? I'm > gobsmacked by the existing "A few variables that don't seem worth > passing around as parameters" comment. Not wanting to pass parameters > around is a horrible excuse for adding global variables, even static > ones.
Makes sense to me. > Attached is what I propose in .diff form so that the CFbot can run on > your v7 patches without picking this up. Your diff LGTM. Earlier upthread in [1], Bharath had mentioned in a review comment about removing the global variables that he would have expected the analogous global in analyze.c to also be removed (vac_strategy [and analyze.c also has anl_context]). I looked into doing this, and this is what I found out (see full rationale in [2]): > it is a bit harder to remove it from analyze because acquire_func > doesn't take the buffer access strategy as a parameter and > acquire_sample_rows uses the vac_context global variable to pass to > table_scan_analyze_next_block(). I don't know if this is worth mentioning in the commit removing the other globals? Maybe it will just make it more confusing... > I considered if we could switch memory contexts before calling > expand_vacuum_rel() and get_all_vacuum_rels(), but I see, at least in > the case of expand_vacuum_rel() that we'd probably want to list_free() > the output of find_all_inheritors() to save that from leaking into the > vac_context. It seems safe just to switch into the vac_context only > when we really want to keep that memory around. (I do think switching > in each iteration of the foreach(part_lc, part_oids) loop is > excessive, however. Just not enough for me to want to change it) Yes, I see what you mean. Your decision makes sense to me. - Melanie [1] https://www.postgresql.org/message-id/CALj2ACXKgAQpKsCPi6ox%2BK5JLDB9TAxeObyVOfrmgTjqmc0aAA%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAAKRu_brtqmd4e7kwEeKjySP22y4ywF32M7pvpi%2Bx5txgF0%2Big%40mail.gmail.com
