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

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Nikolay Shaplov <dhyan(at)nataraj(dot)su>
Cc: pgsql-hackers(at)postgresql(dot)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: 2017-03-17 17:21:26
Message-ID: 20170317172126.o452q2cjds6o3pfx@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-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

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
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-03-17 17:22:39 Re: [COMMITTERS] pgsql: Use asynchronous connect API in libpqwalreceiver
Previous Message Tom Lane 2017-03-17 17:18:56 Re: WIP: Faster Expression Processing v4