[PATCH] New [relation] option engine - Mailing list pgsql-hackers

From Nikolay Shaplov
Subject [PATCH] New [relation] option engine
Date
Msg-id 3766675.7eaCOWfIcx@thinkpad-pgpro
Whole thread Raw
Responses Re: [PATCH] New [relation] option engine  (Andres Freund <andres@anarazel.de>)
Re: [PATCH] New [relation] option engine  (Jeff Davis <pgsql@j-davis.com>)
List 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

pgsql-hackers by date:

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