bootstrap pg_shseclabel in relcache initialization

Started by Adam Brightwellabout 10 years ago21 messages
#1Adam Brightwell
adam.brightwell@crunchydata.com
1 attachment(s)

Hi All,

While working on an auth hook, I found that I was unable to access the
pg_shseclabel system table while processing the hook. I discovered
that the only tables that were bootstrapped and made available at this
stage of the the auth process were pg_database, pg_authid and
pg_auth_members. Unfortunately, this is problematic if you have
security labels that are associated with a role which are needed to
determine auth decisions/actions.

Given that the shared relations currently exposed can also have
security labels that can be used for auth purposes, I believe it makes
sense to make those available as well. I have attached a patch that
adds this functionality for review/discussion. If this functionality
makes sense I'll add it to the commitfest.

Thanks,
Adam

Attachments:

bootstrap-pg_shseclabel-relcache.patchtext/x-diff; charset=US-ASCII; name=bootstrap-pg_shseclabel-relcache.patchDownload
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 9c3d096..c38a8ac 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -51,6 +51,7 @@
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_rewrite.h"
+#include "catalog/pg_shseclabel.h"
 #include "catalog/pg_tablespace.h"
 #include "catalog/pg_trigger.h"
 #include "catalog/pg_type.h"
@@ -98,6 +99,7 @@ static const FormData_pg_attribute Desc_pg_database[Natts_pg_database] = {Schema
 static const FormData_pg_attribute Desc_pg_authid[Natts_pg_authid] = {Schema_pg_authid};
 static const FormData_pg_attribute Desc_pg_auth_members[Natts_pg_auth_members] = {Schema_pg_auth_members};
 static const FormData_pg_attribute Desc_pg_index[Natts_pg_index] = {Schema_pg_index};
+static const FormData_pg_attribute Desc_pg_shseclabel[Natts_pg_shseclabel] = {Schema_pg_shseclabel};
 
 /*
  *		Hash tables that index the relation cache
@@ -3187,13 +3189,14 @@ RelationCacheInitialize(void)
 /*
  *		RelationCacheInitializePhase2
  *
- *		This is called to prepare for access to shared catalogs during startup.
- *		We must at least set up nailed reldescs for pg_database, pg_authid,
- *		and pg_auth_members.  Ideally we'd like to have reldescs for their
- *		indexes, too.  We attempt to load this information from the shared
- *		relcache init file.  If that's missing or broken, just make phony
- *		entries for the catalogs themselves.  RelationCacheInitializePhase3
- *		will clean up as needed.
+ *		This is called to prepare for access to shared catalogs during
+ *		startup.  We must at least set up nailed reldescs for
+ *		pg_database, pg_authid, pg_auth_members, and pg_shseclabel.
+ *		Ideally we'd like to have reldescs for their indexes, too.  We
+ *		attempt to load this information from the shared relcache init
+ *		file.  If that's missing or broken, just make phony entries for
+ *		the catalogs themselves.  RelationCacheInitializePhase3 will
+ *		clean up as needed.
  */
 void
 RelationCacheInitializePhase2(void)
@@ -3229,8 +3232,10 @@ RelationCacheInitializePhase2(void)
 				  true, Natts_pg_authid, Desc_pg_authid);
 		formrdesc("pg_auth_members", AuthMemRelation_Rowtype_Id, true,
 				  false, Natts_pg_auth_members, Desc_pg_auth_members);
+		formrdesc("pg_shseclabel", SharedSecLabelRelation_Rowtype_Id, true,
+				  false, Natts_pg_shseclabel, Desc_pg_shseclabel);
 
-#define NUM_CRITICAL_SHARED_RELS	3	/* fix if you change list above */
+#define NUM_CRITICAL_SHARED_RELS	4	/* fix if you change list above */
 	}
 
 	MemoryContextSwitchTo(oldcxt);
@@ -3365,6 +3370,8 @@ RelationCacheInitializePhase3(void)
 							AuthIdRelationId);
 		load_critical_index(AuthMemMemRoleIndexId,
 							AuthMemRelationId);
+		load_critical_index(SharedSecLabelObjectIndexId,
+							SharedSecLabelRelationId);
 
 #define NUM_CRITICAL_SHARED_INDEXES 5	/* fix if you change list above */
 
diff --git a/src/include/catalog/pg_shseclabel.h b/src/include/catalog/pg_shseclabel.h
index 0ff41f3..d8334bf 100644
--- a/src/include/catalog/pg_shseclabel.h
+++ b/src/include/catalog/pg_shseclabel.h
@@ -18,9 +18,10 @@
  *		typedef struct FormData_pg_shseclabel
  * ----------------
  */
-#define SharedSecLabelRelationId		3592
+#define SharedSecLabelRelationId			3592
+#define SharedSecLabelRelation_Rowtype_Id	4066
 
-CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
+CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_ROWTYPE_OID(4066) BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO
 {
 	Oid			objoid;			/* OID of the shared object itself */
 	Oid			classoid;		/* OID of table containing the shared object */
@@ -31,6 +32,8 @@ CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
 #endif
 } FormData_pg_shseclabel;
 
+typedef FormData_pg_shseclabel *Form_pg_shseclabel;
+
 /* ----------------
  *		compiler constants for pg_shseclabel
  * ----------------
#2Craig Ringer
craig@2ndquadrant.com
In reply to: Adam Brightwell (#1)
Re: bootstrap pg_shseclabel in relcache initialization

On 9 November 2015 at 12:40, Adam Brightwell
<adam.brightwell@crunchydata.com> wrote:

Hi All,

While working on an auth hook, I found that I was unable to access the
pg_shseclabel system table while processing the hook. I discovered
that the only tables that were bootstrapped and made available at this
stage of the the auth process were pg_database, pg_authid and
pg_auth_members. Unfortunately, this is problematic if you have
security labels that are associated with a role which are needed to
determine auth decisions/actions.

Given that the shared relations currently exposed can also have
security labels that can be used for auth purposes, I believe it makes
sense to make those available as well. I have attached a patch that
adds this functionality for review/discussion. If this functionality
makes sense I'll add it to the commitfest.

Your reasoning certainly makes sense to me. I'm a little surprised
this didn't cause issues for SEPostgreSQL already.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Joe Conway
mail@joeconway.com
In reply to: Craig Ringer (#2)
Re: bootstrap pg_shseclabel in relcache initialization

On 11/08/2015 11:17 PM, Craig Ringer wrote:

On 9 November 2015 at 12:40, Adam Brightwell
<adam.brightwell@crunchydata.com> wrote:

Hi All,

While working on an auth hook, I found that I was unable to access the
pg_shseclabel system table while processing the hook. I discovered
that the only tables that were bootstrapped and made available at this
stage of the the auth process were pg_database, pg_authid and
pg_auth_members. Unfortunately, this is problematic if you have
security labels that are associated with a role which are needed to
determine auth decisions/actions.

Given that the shared relations currently exposed can also have
security labels that can be used for auth purposes, I believe it makes
sense to make those available as well. I have attached a patch that
adds this functionality for review/discussion. If this functionality
makes sense I'll add it to the commitfest.

Your reasoning certainly makes sense to me. I'm a little surprised
this didn't cause issues for SEPostgreSQL already.

Currently sepgsql at least does not support security labels on roles,
even though nominally postgres does. If the label provider does not
support them (as in sepgsql) you just get a "feature not supported" type
of error when trying to create the label. I'm not sure if there are any
other label providers in the wild other than sepgsql, but I should think
they would all benefit from this change.

+1 for adding to the next commitfest.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

#4Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Adam Brightwell (#1)
Re: bootstrap pg_shseclabel in relcache initialization

Adam Brightwell wrote:

@@ -3365,6 +3370,8 @@ RelationCacheInitializePhase3(void)
AuthIdRelationId);
load_critical_index(AuthMemMemRoleIndexId,
AuthMemRelationId);
+		load_critical_index(SharedSecLabelObjectIndexId,
+							SharedSecLabelRelationId);

#define NUM_CRITICAL_SHARED_INDEXES 5 /* fix if you change list above */

Need to bump this #define? If you didn't get the error that this is
supposed to throw, perhaps there's a bug somewhere worth investigating.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Adam Brightwell
adam.brightwell@crunchydata.com
In reply to: Alvaro Herrera (#4)
Re: bootstrap pg_shseclabel in relcache initialization
@@ -3365,6 +3370,8 @@ RelationCacheInitializePhase3(void)
AuthIdRelationId);
load_critical_index(AuthMemMemRoleIndexId,
AuthMemRelationId);
+             load_critical_index(SharedSecLabelObjectIndexId,
+                                                     SharedSecLabelRelationId);

#define NUM_CRITICAL_SHARED_INDEXES 5 /* fix if you change list above */

Need to bump this #define? If you didn't get the error that this is
supposed to throw, perhaps there's a bug somewhere worth investigating.

Hmm... I thought that I had, are you not seeing the following change?

-#define NUM_CRITICAL_SHARED_RELS    3    /* fix if you change list above */
+#define NUM_CRITICAL_SHARED_RELS    4    /* fix if you change list above */

-Adam

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Andres Freund
andres@anarazel.de
In reply to: Adam Brightwell (#5)
Re: bootstrap pg_shseclabel in relcache initialization

On 2015-11-09 23:38:57 -0500, Adam Brightwell wrote:

@@ -3365,6 +3370,8 @@ RelationCacheInitializePhase3(void)
AuthIdRelationId);
load_critical_index(AuthMemMemRoleIndexId,
AuthMemRelationId);
+             load_critical_index(SharedSecLabelObjectIndexId,
+                                                     SharedSecLabelRelationId);

#define NUM_CRITICAL_SHARED_INDEXES 5 /* fix if you change list above */

Need to bump this #define? If you didn't get the error that this is
supposed to throw, perhaps there's a bug somewhere worth investigating.

Hmm... I thought that I had, are you not seeing the following change?

-#define NUM_CRITICAL_SHARED_RELS    3    /* fix if you change list above */
+#define NUM_CRITICAL_SHARED_RELS    4    /* fix if you change list above */

NUM_CRITICAL_SHARED_RELS != NUM_CRITICAL_SHARED_INDEXES

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Adam Brightwell
adam.brightwell@crunchydata.com
In reply to: Andres Freund (#6)
1 attachment(s)
Re: bootstrap pg_shseclabel in relcache initialization

#define NUM_CRITICAL_SHARED_INDEXES 5 /* fix if you change list above */

Need to bump this #define? If you didn't get the error that this is
supposed to throw, perhaps there's a bug somewhere worth investigating.

Hmm... I thought that I had, are you not seeing the following change?

-#define NUM_CRITICAL_SHARED_RELS    3    /* fix if you change list above */
+#define NUM_CRITICAL_SHARED_RELS    4    /* fix if you change list above */

NUM_CRITICAL_SHARED_RELS != NUM_CRITICAL_SHARED_INDEXES

Whoops! It must be getting late... updated patch attached.

-Adam

Attachments:

bootstrap-pg_shseclabel-relcache-v2.patchtext/x-diff; charset=US-ASCII; name=bootstrap-pg_shseclabel-relcache-v2.patchDownload
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 9c3d096..b701d82 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -51,6 +51,7 @@
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_rewrite.h"
+#include "catalog/pg_shseclabel.h"
 #include "catalog/pg_tablespace.h"
 #include "catalog/pg_trigger.h"
 #include "catalog/pg_type.h"
@@ -98,6 +99,7 @@ static const FormData_pg_attribute Desc_pg_database[Natts_pg_database] = {Schema
 static const FormData_pg_attribute Desc_pg_authid[Natts_pg_authid] = {Schema_pg_authid};
 static const FormData_pg_attribute Desc_pg_auth_members[Natts_pg_auth_members] = {Schema_pg_auth_members};
 static const FormData_pg_attribute Desc_pg_index[Natts_pg_index] = {Schema_pg_index};
+static const FormData_pg_attribute Desc_pg_shseclabel[Natts_pg_shseclabel] = {Schema_pg_shseclabel};
 
 /*
  *		Hash tables that index the relation cache
@@ -3187,13 +3189,14 @@ RelationCacheInitialize(void)
 /*
  *		RelationCacheInitializePhase2
  *
- *		This is called to prepare for access to shared catalogs during startup.
- *		We must at least set up nailed reldescs for pg_database, pg_authid,
- *		and pg_auth_members.  Ideally we'd like to have reldescs for their
- *		indexes, too.  We attempt to load this information from the shared
- *		relcache init file.  If that's missing or broken, just make phony
- *		entries for the catalogs themselves.  RelationCacheInitializePhase3
- *		will clean up as needed.
+ *		This is called to prepare for access to shared catalogs during
+ *		startup.  We must at least set up nailed reldescs for
+ *		pg_database, pg_authid, pg_auth_members, and pg_shseclabel.
+ *		Ideally we'd like to have reldescs for their indexes, too.  We
+ *		attempt to load this information from the shared relcache init
+ *		file.  If that's missing or broken, just make phony entries for
+ *		the catalogs themselves.  RelationCacheInitializePhase3 will
+ *		clean up as needed.
  */
 void
 RelationCacheInitializePhase2(void)
@@ -3229,8 +3232,10 @@ RelationCacheInitializePhase2(void)
 				  true, Natts_pg_authid, Desc_pg_authid);
 		formrdesc("pg_auth_members", AuthMemRelation_Rowtype_Id, true,
 				  false, Natts_pg_auth_members, Desc_pg_auth_members);
+		formrdesc("pg_shseclabel", SharedSecLabelRelation_Rowtype_Id, true,
+				  false, Natts_pg_shseclabel, Desc_pg_shseclabel);
 
-#define NUM_CRITICAL_SHARED_RELS	3	/* fix if you change list above */
+#define NUM_CRITICAL_SHARED_RELS	4	/* fix if you change list above */
 	}
 
 	MemoryContextSwitchTo(oldcxt);
@@ -3365,8 +3370,10 @@ RelationCacheInitializePhase3(void)
 							AuthIdRelationId);
 		load_critical_index(AuthMemMemRoleIndexId,
 							AuthMemRelationId);
+		load_critical_index(SharedSecLabelObjectIndexId,
+							SharedSecLabelRelationId);
 
-#define NUM_CRITICAL_SHARED_INDEXES 5	/* fix if you change list above */
+#define NUM_CRITICAL_SHARED_INDEXES 6	/* fix if you change list above */
 
 		criticalSharedRelcachesBuilt = true;
 	}
diff --git a/src/include/catalog/pg_shseclabel.h b/src/include/catalog/pg_shseclabel.h
index 0ff41f3..d8334bf 100644
--- a/src/include/catalog/pg_shseclabel.h
+++ b/src/include/catalog/pg_shseclabel.h
@@ -18,9 +18,10 @@
  *		typedef struct FormData_pg_shseclabel
  * ----------------
  */
-#define SharedSecLabelRelationId		3592
+#define SharedSecLabelRelationId			3592
+#define SharedSecLabelRelation_Rowtype_Id	4066
 
-CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
+CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_ROWTYPE_OID(4066) BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO
 {
 	Oid			objoid;			/* OID of the shared object itself */
 	Oid			classoid;		/* OID of table containing the shared object */
@@ -31,6 +32,8 @@ CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
 #endif
 } FormData_pg_shseclabel;
 
+typedef FormData_pg_shseclabel *Form_pg_shseclabel;
+
 /* ----------------
  *		compiler constants for pg_shseclabel
  * ----------------
#8Tom Lane
tgl@sss.pgh.pa.us
In reply to: Adam Brightwell (#7)
Re: bootstrap pg_shseclabel in relcache initialization

Adam Brightwell <adam.brightwell@crunchydata.com> writes:

Need to bump this #define? If you didn't get the error that this is
supposed to throw, perhaps there's a bug somewhere worth investigating.

Whoops! It must be getting late... updated patch attached.

I'm with Alvaro: the most interesting question here is why that mistake
did not blow up on you immediately. I thought we had enough safeguards
in place to catch this type of error.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#9Kouhei Kaigai
kaigai@ak.jp.nec.com
In reply to: Joe Conway (#3)
Re: bootstrap pg_shseclabel in relcache initialization

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Joe Conway
Sent: Tuesday, November 10, 2015 3:08 AM
To: Craig Ringer; Adam Brightwell
Cc: PostgreSQL Hackers
Subject: Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

On 11/08/2015 11:17 PM, Craig Ringer wrote:

On 9 November 2015 at 12:40, Adam Brightwell
<adam.brightwell@crunchydata.com> wrote:

Hi All,

While working on an auth hook, I found that I was unable to access the
pg_shseclabel system table while processing the hook. I discovered
that the only tables that were bootstrapped and made available at this
stage of the the auth process were pg_database, pg_authid and
pg_auth_members. Unfortunately, this is problematic if you have
security labels that are associated with a role which are needed to
determine auth decisions/actions.

Given that the shared relations currently exposed can also have
security labels that can be used for auth purposes, I believe it makes
sense to make those available as well. I have attached a patch that
adds this functionality for review/discussion. If this functionality
makes sense I'll add it to the commitfest.

Your reasoning certainly makes sense to me. I'm a little surprised
this didn't cause issues for SEPostgreSQL already.

Currently sepgsql at least does not support security labels on roles,
even though nominally postgres does. If the label provider does not
support them (as in sepgsql) you just get a "feature not supported" type
of error when trying to create the label. I'm not sure if there are any
other label providers in the wild other than sepgsql, but I should think
they would all benefit from this change.

The sepgsql does not support and its security model intends to assign
client's privileges orthogonal to database role, thus, it didn't touch
pg_shseclabel at that time. However, we can easily expect another label
provider that references database role.

+1 for adding to the next commitfest.

Me also.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10Adam Brightwell
adam.brightwell@crunchydata.com
In reply to: Tom Lane (#8)
Re: bootstrap pg_shseclabel in relcache initialization

I'm with Alvaro: the most interesting question here is why that mistake
did not blow up on you immediately. I thought we had enough safeguards
in place to catch this type of error.

Ok, I'll explore that a bit further as I was able to build and use
with my hook without any issue. :-/

-Adam

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Adam Brightwell
adam.brightwell@crunchydata.com
In reply to: Kouhei Kaigai (#9)
Re: bootstrap pg_shseclabel in relcache initialization

+1 for adding to the next commitfest.

Me also.

Done.

-Adam

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Adam Brightwell
adam.brightwell@crunchydata.com
In reply to: Adam Brightwell (#10)
Re: bootstrap pg_shseclabel in relcache initialization

On Tue, Nov 10, 2015 at 9:18 AM, Adam Brightwell
<adam.brightwell@crunchydata.com> wrote:

I'm with Alvaro: the most interesting question here is why that mistake
did not blow up on you immediately. I thought we had enough safeguards
in place to catch this type of error.

Ok, I'll explore that a bit further as I was able to build and use
with my hook without any issue. :-/

Ok, I have verified that it does indeed eventually raise a warning
about this. It took a few for it to occur/appear in my logs. I was
expecting it to be more "immediate". At any rate, I don't believe
there are any issues with the safeguards in place.

-Adam

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#13Tom Lane
tgl@sss.pgh.pa.us
In reply to: Adam Brightwell (#12)
Re: bootstrap pg_shseclabel in relcache initialization

Adam Brightwell <adam.brightwell@crunchydata.com> writes:

On Tue, Nov 10, 2015 at 9:18 AM, Adam Brightwell
<adam.brightwell@crunchydata.com> wrote:

I'm with Alvaro: the most interesting question here is why that mistake
did not blow up on you immediately. I thought we had enough safeguards
in place to catch this type of error.

Ok, I'll explore that a bit further as I was able to build and use
with my hook without any issue. :-/

Ok, I have verified that it does indeed eventually raise a warning
about this. It took a few for it to occur/appear in my logs. I was
expecting it to be more "immediate".

Hm. Salesforce had also run into the issue that the warnings are just
warnings and relatively easy to miss. What we did there was to change
the elog(WARNING)s near the bottom of load_relcache_init_file to
elog(ERROR), but I'm not sure if I want to recommend doing that in the
community sources. In commit 5d1ff6bd559ea8df I'd expected that the
WARNINGs would certainly show up in regression test output, and I thought
I'd verified that that was the case --- did that not happen for you?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#14Adam Brightwell
adam.brightwell@crunchydata.com
In reply to: Tom Lane (#13)
Re: bootstrap pg_shseclabel in relcache initialization

In commit 5d1ff6bd559ea8df I'd expected that the
WARNINGs would certainly show up in regression test output, and I thought
I'd verified that that was the case --- did that not happen for you?

I just doubled checked with both 'check' and 'check-world' and neither
seemed to have an issue with it. Though, I do see the warning show up
in 'regress/log/postmaster.log'.

-Adam

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#15Tom Lane
tgl@sss.pgh.pa.us
In reply to: Adam Brightwell (#14)
Re: bootstrap pg_shseclabel in relcache initialization

Adam Brightwell <adam.brightwell@crunchydata.com> writes:

In commit 5d1ff6bd559ea8df I'd expected that the
WARNINGs would certainly show up in regression test output, and I thought
I'd verified that that was the case --- did that not happen for you?

I just doubled checked with both 'check' and 'check-world' and neither
seemed to have an issue with it. Though, I do see the warning show up
in 'regress/log/postmaster.log'.

I poked around a bit and figured out what is wrong: for shared catalogs,
this message would be emitted during RelationCacheInitializePhase2(),
which is executed before we perform client authentication (as indeed
is rather your point here). That means ClientAuthInProgress is still
true, which means elog.c doesn't honor client_min_messages --- it
will only send ERROR or higher messages to the client. So the message
goes to the postmaster log, but not to the client.

When I checked the behavior of 5d1ff6bd559ea8df, I must have only
tried it for unshared catalogs. Those are set up by
RelationCacheInitializePhase3, which is post-authentication, so the
message comes out and causes regression test failures as expected.

This is kind of annoying :-(. As noted in elog.c, it doesn't seem
like a terribly good idea to send WARNING messages while the client
is still in authentication mode; we can't be very sure that clients
will react desirably. So we can't fix it by mucking with that.

One answer is to promote the case to an ERROR. We could (probably) keep
a bad initfile from becoming a permanent lockout condition by unlinking
the initfile before reporting ERROR, but this way still seems like a
reliability hazard that could be worse than the original problem.

Another idea, which seems pretty grotty but might be workable, is
to save aside some state about the failure during
RelationCacheInitializePhase2 and then actually send the WARNING
during RelationCacheInitializePhase3. But that seems a little Rube
Goldberg-ish, and who's to say it couldn't break?

But I don't think I want to just do nothing. We already found out
how easy it is to not realize that we have a bug here, leading to
a serious backend-startup-performance issue.

Thoughts?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#16Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#15)
Re: bootstrap pg_shseclabel in relcache initialization

I wrote:

When I checked the behavior of 5d1ff6bd559ea8df, I must have only
tried it for unshared catalogs. Those are set up by
RelationCacheInitializePhase3, which is post-authentication, so the
message comes out and causes regression test failures as expected.

This is kind of annoying :-(. As noted in elog.c, it doesn't seem
like a terribly good idea to send WARNING messages while the client
is still in authentication mode; we can't be very sure that clients
will react desirably. So we can't fix it by mucking with that.

One answer is to promote the case to an ERROR. We could (probably) keep
a bad initfile from becoming a permanent lockout condition by unlinking
the initfile before reporting ERROR, but this way still seems like a
reliability hazard that could be worse than the original problem.

After sleeping on it, the best compromise I can think of is to add an
"Assert(false)" after the WARNING report for the shared-catalogs case.
This will make the failure un-missable in any development build, while
not breaking production builds' ability to recover from corner cases
we might not've foreseen.

Of course, if you run an assert-enabled build in production, you might
possibly lose. But that's never been recommended practice.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#17Joe Conway
mail@joeconway.com
In reply to: Tom Lane (#16)
Re: bootstrap pg_shseclabel in relcache initialization

On 11/11/2015 11:31 AM, Tom Lane wrote:

After sleeping on it, the best compromise I can think of is to add an
"Assert(false)" after the WARNING report for the shared-catalogs case.
This will make the failure un-missable in any development build, while
not breaking production builds' ability to recover from corner cases
we might not've foreseen.

That sounds like a good answer to me.

Of course, if you run an assert-enabled build in production, you might
possibly lose. But that's never been recommended practice.

Yeah, there are plenty of other ways you would lose on that proposition.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

#18Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Adam Brightwell (#1)
Re: bootstrap pg_shseclabel in relcache initialization

Adam Brightwell wrote:

While working on an auth hook, I found that I was unable to access the
pg_shseclabel system table while processing the hook.

[ ... ]

Given that the shared relations currently exposed can also have
security labels that can be used for auth purposes, I believe it makes
sense to make those available as well. I have attached a patch that
adds this functionality for review/discussion. If this functionality
makes sense I'll add it to the commitfest.

So this looks like a bugfix that we should backpatch, but on closer
inspection it turns out that we need the rowtype OID to be fixed, which
it isn't unless this:

-CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
+CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_ROWTYPE_OID(4066) BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO

so I'm afraid this cannot be backpatched at all; if we did, the rowtype
wouldn't match for already-initdb'd installations.

I'm gonna push this to master only, which means you won't be able to
rely on pg_shseclabel until 9.6.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#19Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#18)
Re: bootstrap pg_shseclabel in relcache initialization

Alvaro Herrera wrote:

I'm gonna push this to master only, which means you won't be able to
rely on pg_shseclabel until 9.6.

Pushed, with one cosmetic change (update comment on formrdesc). I also
bumped the catversion, but didn't verify whether this is critical.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#20Adam Brightwell
adam.brightwell@crunchydata.com
In reply to: Alvaro Herrera (#18)
Re: bootstrap pg_shseclabel in relcache initialization

So this looks like a bugfix that we should backpatch, but on closer
inspection it turns out that we need the rowtype OID to be fixed, which
it isn't unless this:

-CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
+CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_ROWTYPE_OID(4066) BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO

so I'm afraid this cannot be backpatched at all; if we did, the rowtype
wouldn't match for already-initdb'd installations.

I'm gonna push this to master only, which means you won't be able to
rely on pg_shseclabel until 9.6.

I thinks that's fair.

-Adam

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#21Adam Brightwell
adam.brightwell@crunchydata.com
In reply to: Alvaro Herrera (#19)
Re: bootstrap pg_shseclabel in relcache initialization

Pushed, with one cosmetic change (update comment on formrdesc). I also
bumped the catversion, but didn't verify whether this is critical.

Excellent! Thanks!

-Adam

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers