On Tue, Jan 08, 2019 at 06:46:11PM +0000, Bossart, Nathan wrote: > This was done in order to maintain the current behavior that > appendQualifiedRelation() gives us. I found that skipping the > search_path handling here forced us to specify the schema in the > argument for --table in most cases. At the very least, I could add a > comment here to highlight the importance of fully qualifying > everything in the catalog query. What do you think?
A comment sounds like a good thing. And we really shouldn't hijack search_path even for one query... > Looks good to me, except for one small thing in the documentation: > > + <para> > + Disable all page-skipping behavior during processing based on > + the visibility map, similarly to the option > + <literal>DISABLE_PAGE_SKIPPING</literal> for > <command>VACUUM</command>. > + </para> > > I think the "similarly to the option" part is slightly misleading. > It's not just similar, it _is_ using that option in the generated > commands. Perhaps we could point to the VACUUM documentation for more > information about this one. Sure. If you have any suggestions, please feel free. Adding a link to VACUUM documentation sounds good to me, as long as we avoid duplicating the description related to DISABLE_PAGE_SKIPPING on the VACUUM page. > Good point. I think allowing multiple different relation size options > here would be confusing, too (e.g. --min-relation-size versus > --min-total-relation-size). IMO pg_total_relation_size() is the way > to go here, as we'll most likely need to process the indexes and TOAST > tables, too. If/when there is a DISABLE_INDEX_CLEANUP option for > VACUUM, we'd then want to use pg_table_size() when --min-relation-size > and --disable-index-cleanup are used together in vacuumdb. > Thoughts? Yes, using pg_total_relation_size() looks like the best option to me here as well, still this does not make me 100% comfortable from the user perspective. > I've realized that I forgot to add links to the XID/MXID wraparound > documentation like you suggested a while back. I'll make sure that > gets included in the next patch set, too. Nice, thanks. -- Michael
signature.asc
Description: PGP signature
