Suggestion: Unified options API. Need help from core team

From: Nikolay Shaplov <dhyan(at)nataraj(dot)su>
To: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Suggestion: Unified options API. Need help from core team
Date: 2021-10-18 13:24:23
Message-ID: 1760969.lJVCjits1C@thinkpad-pgpro
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi!

I am still hoping to finish my work on reloptions I've started some years ago.

I've renewed my patch and I think I need help from core team to finish it.

General idea of the patch: Now we have three ways to define options for
different objects, with more or less different code used for it. It wold be
better to have unified context independent API for processing options, instead.

Long story short:

There is Option Specification object, that has all information about single
option, how it should be parsed and validated.

There is Option Specification Set object, an array of Option Specs, that defines
all options available for certain object (am of some index for example).

When some object (relation, opclass, etc) wants to have an options, it
creates an Option Spec Set for there options, and uses it for converting
options between different representations (to get is from SQL, to store it in
pg_class, to pass it to the core code as bytea etc)

For indexes Option Spec Set is available via Access Method API.

For non-index relations all Option Spec Sets are left in reloption.c file, and
should be moved to heap AM later. (They are not in AM now so will not change
it now)

Main problem:

There are LockModes. LockModes for options is also stored in Option Spec Set.
For indexes Option Spec Sec is accessable via AM. So to get LockMode for
option of an index you need to have access for it's relation object (so you
can call proper AM method to fetch spec set). So you need "Relation rel" in
AlterTableGetRelOptionsLockLevel where Lock Level is determinated (src/
backend/access/common/reloptions.c)
AlterTableGetRelOptionsLockLevel is called from AlterTableGetLockLevel (src/
backend/commands/tablecmds.c) so we need "Relation rel" there too.
AlterTableGetLockLevel is called from AlterTableInternal (/src/backend/
commands/tablecmds.c) There we have "Oid relid" so we can try to open relation
like this

Relation rel = relation_open(relid, NoLock);
cmd_lockmode = AlterTableGetRelOptionsLockLevel(rel,
castNode(List, cmd->def));
relation_close(rel,NoLock);
break;

but this will trigger the assertion

Assert(lockmode != NoLock ||
IsBootstrapProcessingMode() ||
CheckRelationLockedByMe(r, c, true));

in relation_open (b/src/backend/access/common/relation.c)

For now I've commented this assertion out. I've tried to open relation with
AccessShareLock but this caused one test to fail, and I am not sure this
solution is better.

What I have done here I consider a hack, so I need a help of core-team here to
do it in right way.

General problems:

I guess I need a coauthor, or supervisor from core team, to finish this patch.
The amount of code is big, and I guess there are parts that can be made more
in postgres way, then I did them. And I would need an advice there, and I
guess it would be better to do if before sending it to commitfest.

Current patch status:

1. It is Beta. Some minor issues and FIXMEs are not solved. Some code comments
needs revising, but in general it do what it is intended to do.

2. This patch does not intend to change postgres behavior at all, all should
work as before, all changes are internal only.

The only exception is error message for unexciting option name in toast
namespace

CREATE TABLE reloptions_test2 (i int) WITH (toast.not_existing_option = 42);
-ERROR: unrecognized parameter "not_existing_option"
+ERROR: unrecognized parameter "toast.not_existing_option"

New message is better I guess, though I can change it back if needed.

3. I am doing my development in this blanch https://gitlab.com/dhyannataraj/
postgres/-/tree/new_options_take_two I am making changes every day, so last
version will be available there

Would be glad to hear from coreteam before I finish with this patch and made it
ready for commit-fest.

Attachment Content-Type Size
unified_options_api_beta01.diff text/x-patch 187.0 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2021-10-18 13:37:33 Re: can we add subscription TAP test option "vcregress subscriptioncheck" for MSVC builds?
Previous Message Andrew Dunstan 2021-10-18 12:49:28 Re: can we add subscription TAP test option "vcregress subscriptioncheck" for MSVC builds?