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
Date: Fri, 17 Mar 2017 14:21:26 -0300
Message-ID: <20170317172126.o452q2cjds6o3pfx@alvherre.pgsql> (raw)
In-Reply-To: <3123308.RiobK7ytJP@x200m>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@2ndquadrant.com, dhyan@nataraj.su
Subject: Re: [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
In-Reply-To: <20170317172126.o452q2cjds6o3pfx@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox