From: Nikolay Shaplov <dhyan@nataraj.su>
To: pgsql-hackers@postgresql.org
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Subject: Re: [PATCH] Move all am-related reloption code into
 src/backend/access/[am-name] and get rid of relopt_kind for custom AM
Date: Sun, 26 Mar 2017 20:42:16 +0300
Message-ID: <1556865.P0kqmzk0Jg@x200m>
User-Agent: KMail/4.14.1 (Linux/3.16.0-4-amd64; KDE/4.14.2; x86_64; ; )
In-Reply-To: <20170317172126.o452q2cjds6o3pfx@alvherre.pgsql>
References: <20170317172126.o452q2cjds6o3pfx@alvherre.pgsql>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

=D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 17 =D0=BC=D0=B0=D1=
=80=D1=82=D0=B0 2017 14:21:26 =D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=BE=D0=B2=D0=
=B0=D1=82=D0=B5=D0=BB=D1=8C Alvaro Herrera =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=
=D0=B0=D0=BB:
> 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=20
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=
=20
definitions I will gladly rename it, as catalog is a bad name. May be=20
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=20
see, that this option is used only in gistbuild.c. There it is written into=
=20
the meta page, and afterwards, value from meta page is used, and one from=20
options, is just ignored.
Nowdays you can successfully alter this value, but this will not affect=20
anything until index is recreated... I thought it is very confusing behavio=
r=20
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 i=
t=20
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=
=20
thinking about it.

If I would think about it now: we always know how many options we will have=
.=20
So we can just pass this number to palloc and assert if somebody adds more=
=20
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.


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


--=20
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers