I gave this patch a quick skim. At first I was confused by the term "catalog"; I thought it meant we stored options in a system table. But that's not what is meant at all; instead, what we do is build these "catalogs" in memory. Maybe a different term could have been used, but I'm not sure it's stricly necessary. I wouldn't really bother.
I'm confused by the "no ALTER, no lock" rule. Does it mean that if "ALTER..SET" is forbidden? Because I noticed that brin's pages_per_range is marked as such, but we do allow that option to change with ALTER..SET, so there's at least one bug there, and I would be surprised if there aren't any others. Please make sure to mark functions as static (e.g. bringetreloptcatalog). Why not pass ->struct_size and ->postprocess_fun (which I'd rename it as ->postprocess_fn, more in line with our naming style) as a parameter to allocateOptionsCatalog? Also, to avoid repalloc() in most cases (and to avoid pallocing more options that you're going to need in a bunch of cases, perhaps that function should the number of times you expect to call AddItems for that catalog (since you do it immediately afterwards in all cases I can see), and allocate that number. If later another item arrives, then repalloc using the same code you already have in AddItems(). Something is wrong with leading whitespace in many places; either you added too many tabs, or the wrong number spaces; not sure which but visually it's clearly wrong. ... Actually there are whitespace-style violations in several places; please fix using pgindent (after adding any new typedefs your defining to typedefs.list). -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
