Thread: Re: vacuumdb changes for stats import/export
On Wed, Feb 5, 2025 at 4:44 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > [v2] I started looking just at 0001, and it seems like a fairly straightforward rearrangement. I found this comment quite hard to read: + * 'found_objs' should be a fully qualified list of objects to process, as + * returned by a previous call to vacuum_one_database(). If *found_objs is + * NULL, it is set to the results of the catalog query discussed below. If + * found_objs is NULL, the results of the catalog query are not returned. + * + * If *found_objs is NULL, this function performs a catalog query to retrieve + * the list of tables to process. When 'objects' is NULL, all tables in the I had to read it several times before I noticed the difference between "* found_objs" and "*found_objs". Maybe some extra spacing and breaks would help, or other reorganization. -- John Naylor Amazon Web Services
On Thu, Feb 27, 2025 at 04:36:04PM +0700, John Naylor wrote: > On Wed, Feb 5, 2025 at 4:44 AM Nathan Bossart <nathandbossart@gmail.com> wrote: >> [v2] > > I started looking just at 0001, and it seems like a fairly > straightforward rearrangement. Thanks for taking a look. > I found this comment quite hard to read: > > + * 'found_objs' should be a fully qualified list of objects to process, as > + * returned by a previous call to vacuum_one_database(). If *found_objs is > + * NULL, it is set to the results of the catalog query discussed below. If > + * found_objs is NULL, the results of the catalog query are not returned. > + * > + * If *found_objs is NULL, this function performs a catalog query to retrieve > + * the list of tables to process. When 'objects' is NULL, all tables in the > > I had to read it several times before I noticed the difference between > "* found_objs" and "*found_objs". Maybe some extra spacing and breaks > would help, or other reorganization. Yeah, it's pretty atrocious. I think the main problem is that the interface is just too complicated, so I'll take a step back and see if I can make it more understandable to humans. In the meantime, here's an attempt at adjusting the comment: * found_objs is a double pointer to a fully qualified list of objects to * process, as returned by a previous call to vacuum_one_database(). If * *found_objs (the once-dereferenced double pointer) is NULL, it is set to the * results of the catalog query discussed below. If found_objs (the double * pointer) is NULL, the results of the catalog query are not returned. * * If *found_objs (the once-dereferenced double pointer) is NULL, this function * performs a catalog query to retrieve the list of tables to process. When * "objects" is NULL, all tables in the database are processed. Otherwise, the * catalog query performs a lookup for the objects listed in "objects". -- nathan
On Sat, Mar 1, 2025 at 3:42 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Thu, Feb 27, 2025 at 04:36:04PM +0700, John Naylor wrote: > > I had to read it several times before I noticed the difference between > > "* found_objs" and "*found_objs". Maybe some extra spacing and breaks > > would help, or other reorganization. > > Yeah, it's pretty atrocious. I think the main problem is that the > interface is just too complicated, so I'll take a step back and see if I > can make it more understandable to humans. The interface is awkward, but on the other hand only a small part has to really know about it. It's worth trying to make it more readable if you can. > In the meantime, here's an > attempt at adjusting the comment: That's better, and if we end up with this interface, we'll want quotes around the names so the eye can tell where the "*" belong. -- John Naylor Amazon Web Services
On Mon, Mar 03, 2025 at 05:58:43PM +0700, John Naylor wrote: > On Sat, Mar 1, 2025 at 3:42 AM Nathan Bossart <nathandbossart@gmail.com> wrote: >> On Thu, Feb 27, 2025 at 04:36:04PM +0700, John Naylor wrote: >> > I had to read it several times before I noticed the difference between >> > "* found_objs" and "*found_objs". Maybe some extra spacing and breaks >> > would help, or other reorganization. >> >> Yeah, it's pretty atrocious. I think the main problem is that the >> interface is just too complicated, so I'll take a step back and see if I >> can make it more understandable to humans. > > The interface is awkward, but on the other hand only a small part has > to really know about it. It's worth trying to make it more readable if > you can. True. One small thing we could do is to require "found_objs" (the double pointer) to always be non-NULL, but that just compels some callers to provide otherwise-unused variables. I've left the interface alone for now. >> In the meantime, here's an attempt at adjusting the comment: > > That's better, and if we end up with this interface, we'll want quotes > around the names so the eye can tell where the "*" belong. I did that in v3. I also tried to break up this comment into bullet points for better separation and logical flow. -- nathan
Attachment
On Mon, Mar 3, 2025 at 11:21 PM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Mon, Mar 03, 2025 at 05:58:43PM +0700, John Naylor wrote: > True. One small thing we could do is to require "found_objs" (the double > pointer) to always be non-NULL, but that just compels some callers to > provide otherwise-unused variables. I've left the interface alone for now. One thing to consider is that a pointer named "dummy" is self-documenting. > > That's better, and if we end up with this interface, we'll want quotes > > around the names so the eye can tell where the "*" belong. > > I did that in v3. I also tried to break up this comment into bullet points > for better separation and logical flow. That's much easier to follow, thanks. -- John Naylor Amazon Web Services