pgsql: Support reloptions of enum type

Started by Alvaro Herreraalmost 7 years ago6 messagescomitters
Jump to latest
#1Alvaro Herrera
alvherre@2ndquadrant.com

Support reloptions of enum type

All our current in core relation options of type string (not many,
admittedly) behave in reality like enums. But after seeing an
implementation for enum reloptions, it's clear that strings are messier,
so introduce the new reloption type. Switch all string options to be
enums instead.

Fortunately we have a recently introduced test module for reloptions, so
we don't lose coverage of string reloptions, which may still be used by
third-party modules.

Authors: Nikolay Shaplov, Álvaro Herrera
Reviewed-by: Nikita Glukhov, Aleksandr Parfenov
Discussion: /messages/by-id/43332102.S2V5pIjXRx@x200m

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/773df883e8f7543958d0d719c025b5f47c5a67f0

Modified Files
--------------
src/backend/access/common/reloptions.c | 132 +++++++++++++++++++--
src/backend/access/gist/gistbuild.c | 25 +---
src/backend/access/gist/gistutil.c | 2 +-
src/backend/commands/view.c | 18 ---
src/include/access/gist_private.h | 10 +-
src/include/access/reloptions.h | 24 ++++
src/include/commands/view.h | 2 -
src/include/utils/rel.h | 25 ++--
src/test/modules/dummy_index_am/dummy_index_am.c | 37 ++++--
src/test/modules/dummy_index_am/sql/reloptions.sql | 10 ++
src/test/regress/expected/gist.out | 2 +-
src/test/regress/expected/updatable_views.out | 2 +-
12 files changed, 214 insertions(+), 75 deletions(-)

#2Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#1)
Re: pgsql: Support reloptions of enum type

On 2019-Sep-25, Alvaro Herrera wrote:

Support reloptions of enum type

I forgot to "git add" an expected output file. Pushing in a minute ..

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#3Michael Paquier
michael@paquier.xyz
In reply to: Alvaro Herrera (#1)
Re: pgsql: Support reloptions of enum type

Hi Alvaro,

On Wed, Sep 25, 2019 at 06:59:24PM +0000, Alvaro Herrera wrote:

Support reloptions of enum type

All our current in core relation options of type string (not many,
admittedly) behave in reality like enums. But after seeing an
implementation for enum reloptions, it's clear that strings are messier,
so introduce the new reloption type. Switch all string options to be
enums instead.

Fortunately we have a recently introduced test module for reloptions, so
we don't lose coverage of string reloptions, which may still be used by
third-party modules.

Authors: Nikolay Shaplov, Álvaro Herrera
Reviewed-by: Nikita Glukhov, Aleksandr Parfenov
Discussion: /messages/by-id/43332102.S2V5pIjXRx@x200m

This part from commit 773df88 is incorrect:
@@ -661,6 +700,13 @@ allocate_reloption(bits32 kinds, int type, const
char *name, const char *desc,
newoption->type = type;
newoption->lockmode = lockmode;

+   /*
+    * Set the default lock mode for this option.  There is no actual way
+    * for a module to enforce it when declaring a custom relation option,
+    * so just use the highest level, which is safe for all cases.
+    */
+   newoption->lockmode = AccessExclusiveLock;

Any caller of allocate_reloption() passes down its own lockmode
definition, hence you are removing the pluggability of the API. I
think that you just got trapped by an incorrect rebase.

Do you mind if I apply the attached to fix the issue? Or perhaps you
would prefer fixing the issue yourself? I noted some inconsistencies
with the rest while on it (please see attached).
--
Michael

Attachments:

reloptions-lockmode.patchtext/x-diff; charset=us-asciiDownload+5-8
#4Michael Paquier
michael@paquier.xyz
In reply to: Michael Paquier (#3)
Re: pgsql: Support reloptions of enum type

On Thu, Sep 26, 2019 at 08:41:52AM +0900, Michael Paquier wrote:

Do you mind if I apply the attached to fix the issue? Or perhaps you
would prefer fixing the issue yourself? I noted some inconsistencies
with the rest while on it (please see attached).

And applied as of fbfa566.
--
Michael

#5Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Michael Paquier (#3)
Re: pgsql: Support reloptions of enum type

Hello, sorry I didn't notice this email.

On 2019-Sep-26, Michael Paquier wrote:

This part from commit 773df88 is incorrect:

+   /*
+    * Set the default lock mode for this option.  There is no actual way
+    * for a module to enforce it when declaring a custom relation option,
+    * so just use the highest level, which is safe for all cases.
+    */
+   newoption->lockmode = AccessExclusiveLock;

You're right, this was a merge blunder :-( Sorry about that.

Do you mind if I apply the attached to fix the issue? Or perhaps you
would prefer fixing the issue yourself? I noted some inconsistencies
with the rest while on it (please see attached).

Thanks for fixing. It all looks correct to me.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#6Michael Paquier
michael@paquier.xyz
In reply to: Alvaro Herrera (#5)
Re: pgsql: Support reloptions of enum type

On Thu, Sep 26, 2019 at 10:19:47PM -0300, Alvaro Herrera wrote:

Thanks for fixing. It all looks correct to me.

Thanks for double-checking.
--
Michael