Date: Fri, 17 Mar 2017 14:21:26 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Nikolay Shaplov <dhyan@nataraj.su>
Cc: pgsql-hackers@postgresql.org
Subject: Re: [PATCH] Move all am-related reloption code into
 src/backend/access/[am-name] and get rid of relopt_kind for custom AM
Message-ID: <20170317172126.o452q2cjds6o3pfx@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3123308.RiobK7ytJP@x200m>
User-Agent: NeoMutt/20170206-195-7adf12 (1.7.2)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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