[PATCH] refactor ATExec{En,Dis}ableRowSecurity
Started by Justin Pryzbyalmost 5 years ago4 messages
tablecmds.c is 17k lines long, this makes it ~30 lines shorter.
Attachments:
0001-Refactor-ATExec-En-Dis-ableRowSecurity-in-the-style-.patchtext/x-diff; charset=us-asciiDownload
From 2e9500227d45142eb00e9e1ebee001642a834518 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 27 Feb 2021 20:39:10 -0600
Subject: [PATCH 1/2] Refactor ATExec{En,Dis}ableRowSecurity in the style of
ATExecForceNoForceRowSecurity
commit 088c83363a11200f2225f279d4a5c6cc6f9db3d2
ALTER TABLE .. FORCE ROW LEVEL SECURITY
commit 491c029dbc4206779cf659aa0ff986af7831d2ff
Row-Level Security Policies (RLS)
---
src/backend/commands/tablecmds.c | 34 +++++---------------------------
1 file changed, 5 insertions(+), 29 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7adeaedd0e..69e3184c86 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -526,8 +526,7 @@ static ObjectAddress ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKM
static void ATExecDropOf(Relation rel, LOCKMODE lockmode);
static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode);
static void ATExecGenericOptions(Relation rel, List *options);
-static void ATExecEnableRowSecurity(Relation rel);
-static void ATExecDisableRowSecurity(Relation rel);
+static void ATExecSetRowSecurity(Relation rel, bool rls);
static void ATExecForceNoForceRowSecurity(Relation rel, bool force_rls);
static void index_copy_data(Relation rel, RelFileNode newrnode);
@@ -4865,10 +4864,10 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) cmd->def, lockmode);
break;
case AT_EnableRowSecurity:
- ATExecEnableRowSecurity(rel);
+ ATExecSetRowSecurity(rel, true);
break;
case AT_DisableRowSecurity:
- ATExecDisableRowSecurity(rel);
+ ATExecSetRowSecurity(rel, false);
break;
case AT_ForceRowSecurity:
ATExecForceNoForceRowSecurity(rel, true);
@@ -14883,30 +14882,7 @@ ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode
* ALTER TABLE ENABLE/DISABLE ROW LEVEL SECURITY
*/
static void
-ATExecEnableRowSecurity(Relation rel)
-{
- Relation pg_class;
- Oid relid;
- HeapTuple tuple;
-
- relid = RelationGetRelid(rel);
-
- pg_class = table_open(RelationRelationId, RowExclusiveLock);
-
- tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid));
-
- if (!HeapTupleIsValid(tuple))
- elog(ERROR, "cache lookup failed for relation %u", relid);
-
- ((Form_pg_class) GETSTRUCT(tuple))->relrowsecurity = true;
- CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
-
- table_close(pg_class, RowExclusiveLock);
- heap_freetuple(tuple);
-}
-
-static void
-ATExecDisableRowSecurity(Relation rel)
+ATExecSetRowSecurity(Relation rel, bool rls)
{
Relation pg_class;
Oid relid;
@@ -14922,7 +14898,7 @@ ATExecDisableRowSecurity(Relation rel)
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for relation %u", relid);
- ((Form_pg_class) GETSTRUCT(tuple))->relrowsecurity = false;
+ ((Form_pg_class) GETSTRUCT(tuple))->relrowsecurity = rls;
CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
table_close(pg_class, RowExclusiveLock);
--
2.17.0
0002-Further-refactoring.patchtext/x-diff; charset=us-asciiDownload
From bf200702203eebe08011eaa007e93cae46a63ccb Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 27 Feb 2021 20:52:35 -0600
Subject: [PATCH 2/2] Further refactoring
---
src/backend/commands/tablecmds.c | 46 +++++++++-----------------------
1 file changed, 13 insertions(+), 33 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 69e3184c86..2e322e78ae 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -526,8 +526,7 @@ static ObjectAddress ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKM
static void ATExecDropOf(Relation rel, LOCKMODE lockmode);
static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode);
static void ATExecGenericOptions(Relation rel, List *options);
-static void ATExecSetRowSecurity(Relation rel, bool rls);
-static void ATExecForceNoForceRowSecurity(Relation rel, bool force_rls);
+static void ATExecSetRowSecurity(Relation rel, int rls, int force_rls);
static void index_copy_data(Relation rel, RelFileNode newrnode);
static const char *storage_name(char c);
@@ -4864,16 +4863,16 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) cmd->def, lockmode);
break;
case AT_EnableRowSecurity:
- ATExecSetRowSecurity(rel, true);
+ ATExecSetRowSecurity(rel, true, -1);
break;
case AT_DisableRowSecurity:
- ATExecSetRowSecurity(rel, false);
+ ATExecSetRowSecurity(rel, false, -1);
break;
case AT_ForceRowSecurity:
- ATExecForceNoForceRowSecurity(rel, true);
+ ATExecSetRowSecurity(rel, -1, true);
break;
case AT_NoForceRowSecurity:
- ATExecForceNoForceRowSecurity(rel, false);
+ ATExecSetRowSecurity(rel, -1, false);
break;
case AT_GenericOptions:
ATExecGenericOptions(rel, (List *) cmd->def);
@@ -14880,13 +14879,15 @@ ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode
/*
* ALTER TABLE ENABLE/DISABLE ROW LEVEL SECURITY
+ * ALTER TABLE FORCE/NO FORCE ROW LEVEL SECURITY
*/
static void
-ATExecSetRowSecurity(Relation rel, bool rls)
+ATExecSetRowSecurity(Relation rel, int rls, int force_rls)
{
Relation pg_class;
Oid relid;
HeapTuple tuple;
+ Form_pg_class classform;
relid = RelationGetRelid(rel);
@@ -14898,33 +14899,12 @@ ATExecSetRowSecurity(Relation rel, bool rls)
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for relation %u", relid);
- ((Form_pg_class) GETSTRUCT(tuple))->relrowsecurity = rls;
- CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
-
- table_close(pg_class, RowExclusiveLock);
- heap_freetuple(tuple);
-}
-
-/*
- * ALTER TABLE FORCE/NO FORCE ROW LEVEL SECURITY
- */
-static void
-ATExecForceNoForceRowSecurity(Relation rel, bool force_rls)
-{
- Relation pg_class;
- Oid relid;
- HeapTuple tuple;
-
- relid = RelationGetRelid(rel);
-
- pg_class = table_open(RelationRelationId, RowExclusiveLock);
-
- tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid));
-
- if (!HeapTupleIsValid(tuple))
- elog(ERROR, "cache lookup failed for relation %u", relid);
+ classform = (Form_pg_class) GETSTRUCT(tuple);
+ if (rls != -1)
+ classform->relrowsecurity = (bool) rls;
+ if (force_rls != -1)
+ classform->relforcerowsecurity = (bool) force_rls;
- ((Form_pg_class) GETSTRUCT(tuple))->relforcerowsecurity = force_rls;
CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
table_close(pg_class, RowExclusiveLock);
--
2.17.0
Re: [PATCH] refactor ATExec{En,Dis}ableRowSecurity
Hi,
For 0002-Further-refactoring.patch, should there be assertion
inside ATExecSetRowSecurity() on the values for rls and force_rls ?
There could be 3 possible values: -1, 0 and 1.
Cheers
On Sun, Feb 28, 2021 at 1:19 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
Show quoted text
tablecmds.c is 17k lines long, this makes it ~30 lines shorter.
Re: [PATCH] refactor ATExec{En,Dis}ableRowSecurity
On Sun, Feb 28, 2021 at 02:27:44PM -0800, Zhihong Yu wrote:
For 0002-Further-refactoring.patch, should there be assertion
inside ATExecSetRowSecurity() on the values for rls and force_rls ?
There could be 3 possible values: -1, 0 and 1.
0001 is a clean simplification and a good catch, so I'll see about
applying it. 0002 just makes the code more confusing to the reader
IMO, and its interface could easily lead to unwanted errors.
--
Michael
Re: [PATCH] refactor ATExec{En,Dis}ableRowSecurity
On Mon, Mar 01, 2021 at 03:30:44PM +0900, Michael Paquier wrote:
0001 is a clean simplification and a good catch, so I'll see about
applying it. 0002 just makes the code more confusing to the reader
IMO, and its interface could easily lead to unwanted errors.
0001 has been applied as of fabde52.
--
Michael