Re: [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM

В письме от 17 марта 2017 14:21:26 пользователь Alvaro Herrera написал:
> I gave this patch a quick skim.
Thanks!

> 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.
Yes "catalog" is quite confusing. I did not find better name while I was
writing the code, so I take first one, that came into my mind.
If you, or somebody else, have better idea how to call this sets of options
definitions I will gladly rename it, as catalog is a bad name. May be
OptionsDefSet instead of OptionsCatalog?

> 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.

If you grep, for example, gist index code for "buffering_mode" option, you will
see, that this option is used only in gistbuild.c. There it is written into
the meta page, and afterwards, value from meta page is used, and one from
options, is just ignored.
Nowdays you can successfully alter this value, but this will not affect
anything until index is recreated... I thought it is very confusing behavior
and decided that we should just forbid such alters.

> Please make sure to mark functions as static (e.g. bringetreloptcatalog).
Ok. Will do.

> 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?
struct_size -- very good idea!
postprocess_fn -- now it is rarely used. In most cases it is NULL. May be it
would be ok to set it afterwards in that rare cases when it is needed.

> 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().
I've copied this code from reloptions code for custom options. Without much
thinking about it.

If I would think about it now: we always know how many options we will have.
So we can just pass this number to palloc and assert if somebody adds more
options then expected... What do yo think about it.

> 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).
I will run pgindent on my code.

--
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.