[PATCH] New [relation] option engine

From: Nikolay Shaplov <dhyan(at)nataraj(dot)su>
To: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Amit Langote <amitlangote09(at)gmail(dot)com>
Subject: [PATCH] New [relation] option engine
Date: 2022-02-13 21:43:36
Message-ID: 3766675.7eaCOWfIcx@thinkpad-pgpro
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi!

I'd like to introduce a patch that reworks options processing.

This patch detaches code for options processing and validating from the code
that uses these options. This will allow to reuse same code in any part of
postgres that may need options. Currently there are three ways of defining
options, and this code is scattered along the source tree, that leads to a
problems: code is hard to understand, hard to maintain, I saw several options
added or changed, and and in one of two cases, there was one or another
mistake done because of code unclearity.

General idea:

There is an object OptionSpec that describes how single option should be
parsed, validated, stored into bytea, etc.

OptionSpecSet is a list of OptionSpecs, that describes all possible options
for certain object (e.g. options for nbtree index relation)

All options related functions that are not depended on context were moved to
src/backend/access/common/options.c. These functions receives OptionSpecSet
(or OptionSpec) and option data that should be processed. And OptionSpecSet
should contain all information that is needed for proper processing.

Options from the perspective of the option engine can exist in four
representation:
- defList - the way they came from SQL parser
- TEXT[] - the way they are stored in pg_class or similar place
- Bytea - the way they stored in C-structure, for caching and using in
postgres code that uses these options
- Value List - is an internal representation that is used while parsing,
validating and converting between representations

See code comments for more info.

There are functions that is used for conversion from one representation to
another:
- optionsDefListToRawValues : defList -> Values
- optionsTextArrayToDefList :TEXT[] -> defList

- optionsTextArrayToRawValues : TEXT[] -> Values
- optionsValuesToTextArray: Values -> TEXT[]

- optionsValuesToBytea: Values -> Bytea

This functions are called from meta-functions that is used when postgres
receives an SQL command for creating or updating options:

- optionsDefListToTextArray - when options are created
- optionsUpdateTexArrayWithDefList - when option are updated.

They also trigger validation while processing.

There are also functions for SpecSet processing:
- allocateOptionsSpecSet
- optionsSpecSetAddBool
- optionsSpecSetAddBool
- optionsSpecSetAddReal
- optionsSpecSetAddEnum
- optionsSpecSetAddString

For index access methods "amoptions" member function that preformed option
processing, were replaced with "amreloptspecset" member function that provided
an SpecSet for reloptions for this AM, so caller can trigger option processing
himself.

For other relation types options have been processing by "fixed" functions, so
these processing were replaced with "fixed" SpecSet providers + processing
using that SpecSet. Later on these should be moved to access method the same
way it is done in indexes. I plan to do it after this patch is commit.

As for local options, that is used of opclass options, I kept all current API,
but made this API a wrapper around new option engine. Local options should be
moved to unified option engine API later on. I hope to do it too.

This patch does not change any of postgres behaviour (even if this behaviour
is not quite right). The only change is text of the warning for unexisting
option in toast namespace. But I hope this change is for better.

The only problem I am not sure how to solve is an obtaining a LockMode.
To get a LockMode for option , you need a SpecSet. For indexes we get SpecSets
via AccessMethod. To get access for AccessMethod, you need relation C-
structure. To get relation structure you need open relation with NoLock mode.
But if I do it, I get Assert in relation_open. (There were no such Assert when
I started this work, it appeared later).
All code related to this issue is marked with FIXME comments. I've commented
that Assert out, but not quite sure I was right. Here I need a help of more
experienced members of community.

This quite a big patch. Unfortunately it can't be split into smaller parts.
I also suggest to consider this patch as an implementation of general idea,
that works well. I have ideas in mind to make it better, but it will be
infinite improvement process that will never lead to final commit. So if the
concept is good and implementation is good enough, I suggest to commit it, and
make it better later on by smaller patches over it. If it is not good enough,
let me know, I will try to make it good.

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

Attachment Content-Type Size
new_options_take_two_v01.diff text/x-patch 190.9 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2022-02-13 21:43:59 Nonrandom scanned_pages distorts pg_class.reltuples set by VACUUM
Previous Message Andres Freund 2022-02-13 21:43:17 Re: [PATCH] Avoid open and lock the table Extendend Statistics (src/backend/commands/statscmds.c)