Re: [HACKERS] upgrade failure from 9.5 to head

[Bruce Momjian]

On Wed, Jul 29, 2015 at 11:01:40AM -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2015-07-29 10:38:19 -0400, Tom Lane wrote:
> >> Now as far as dummy_seclabel is concerned, the easy answer is "we don't
> >> care".  But on reflection, doesn't this mean that the entire
> >> implementation of SECURITY LABEL is broken?  At least to the extent that
> >> it can't work during pg_upgrade unless the user takes manual action to
> >> configure the relevant providers' .so libraries into the new installation
> >> *before* he runs pg_upgrade.  That doesn't say "production ready" to me.
> 
> > Hm, I don't think that particular issue is that bad. We decided labels
> > are only going to work if they're in shared_preload_libararies, and they
> > really only do if that's the case.
> 
> In that case, where in the documentation of the pg_upgrade process does
> it say "you must configure the new installation with all security label
> providers installed in shared_preload_libraries after initdb'ing the
> new installation and before running pg_upgrade"?  And how can you meet
> that requirement if you are using a canned script that does both those
> steps for you?  (Red Hat certainly ships such a script in their packaging,
> and I rather imagine that the Debian-style packages do too.)

The pg_upgrade docs have:

Install custom shared object files


 Install any custom shared object files (or DLLs) used by the old cluster
 into the new cluster, e.g. pgcrypto.so,
 whether they are from contrib
 or some other source. Do not install the schema definitions, e.g.
 pgcrypto.sql, because these will be upgraded from the old 
cluster.
 Also, any custom full text search files (dictionary, synonym,
 thesaurus, stop words) must also be copied to the new cluster.

   

Is that sufficient?  I think the thing that is missing is the need to
modify postgresql.conf, but you had to do that for the original setup
too.  Odds are they get an error, look at the message, figure out they
need shared_preload_libraries, and re-test it.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Robert Haas]

On Sun, Aug 2, 2015 at 8:20 PM, Stephen Frost sfr...@snowman.net wrote:
 +1.

 I was doing testing the other day and ran into the pg_dump doesn't
 support shell types issue and it was annoyingly confusing.

Is anyone working on this?  Should it be added to the open items list?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 On Sun, Aug 2, 2015 at 8:20 PM, Stephen Frost sfr...@snowman.net wrote:
 +1.
 
 I was doing testing the other day and ran into the pg_dump doesn't
 support shell types issue and it was annoyingly confusing.

 Is anyone working on this?  Should it be added to the open items list?

I plan to work on it as soon as I'm done fixing the planner issue Andreas
found (which I'm almost done with).  Not sure whether we should consider
it a back-patchable bug fix or something to do only in HEAD, though ---
comments?

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

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 On Tue, Aug 4, 2015 at 1:54 PM, Andres Freund and...@anarazel.de wrote:
 On 2015-08-04 13:52:54 -0400, Tom Lane wrote:
 Not sure whether we should consider it a back-patchable bug fix or
 something to do only in HEAD, though --- comments?

 Tentatively I'd say it's a bug and should be back-patched.

 Agreed.  If investigation turns up reasons to worry about
 back-patching it, I'd possibly back-track on that position, but I
 think we should start with the notion that it is back-patchable and
 retreat from that position only at need.

OK.  Certainly we can fix 9.5 the same way as HEAD; the pg_dump code
hasn't diverged much yet.  I'll plan to push it as far back as convenient,
but I won't expend any great effort on making the older branches do it if
they turn out to be too different.

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

Re: [HACKERS] upgrade failure from 9.5 to head

[Andres Freund]

On 2015-08-04 13:52:54 -0400, Tom Lane wrote:
 Not sure whether we should consider it a back-patchable bug fix or
 something to do only in HEAD, though --- comments?

Tentatively I'd say it's a bug and should be back-patched.

Andres


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Robert Haas]

On Tue, Aug 4, 2015 at 1:54 PM, Andres Freund and...@anarazel.de wrote:
 On 2015-08-04 13:52:54 -0400, Tom Lane wrote:
 Not sure whether we should consider it a back-patchable bug fix or
 something to do only in HEAD, though --- comments?

 Tentatively I'd say it's a bug and should be back-patched.

Agreed.  If investigation turns up reasons to worry about
back-patching it, I'd possibly back-track on that position, but I
think we should start with the notion that it is back-patchable and
retreat from that position only at need.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Andrew Dunstan]

On 08/04/2015 02:09 PM, Tom Lane wrote:

Robert Haas robertmh...@gmail.com writes:

On Tue, Aug 4, 2015 at 1:54 PM, Andres Freund and...@anarazel.de wrote:

On 2015-08-04 13:52:54 -0400, Tom Lane wrote:

Not sure whether we should consider it a back-patchable bug fix or
something to do only in HEAD, though --- comments?

Tentatively I'd say it's a bug and should be back-patched.

Agreed.  If investigation turns up reasons to worry about
back-patching it, I'd possibly back-track on that position, but I
think we should start with the notion that it is back-patchable and
retreat from that position only at need.

OK.  Certainly we can fix 9.5 the same way as HEAD; the pg_dump code
hasn't diverged much yet.  I'll plan to push it as far back as convenient,
but I won't expend any great effort on making the older branches do it if
they turn out to be too different.





From my POV 9.5 is the one that's most critical, because it's the one 
that introduced a regression test that leaves a shell type lying around. 
But as far back as convenient works for me.


cheers

andrew


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Andres Freund]

On 2015-08-01 19:13:05 -0400, Noah Misch wrote:
 On Wed, Jul 29, 2015 at 04:42:55PM -0400, Andrew Dunstan wrote:
  The next hump is this, in restoring contrib_regression_test_ddl_parse:
  
 pg_restore: creating FUNCTION public.text_w_default_in(cstring)
 pg_restore: [archiver (db)] Error while PROCESSING TOC:
 pg_restore: [archiver (db)] Error from TOC entry 243; 1255 62534
 FUNCTION text_w_default_in(cstring) buildfarm
 pg_restore: [archiver (db)] could not execute query: ERROR: pg_type
 OID value not set when in binary upgrade mode
  Command was: CREATE FUNCTION text_w_default_in(cstring)
 RETURNS text_w_default
  LANGUAGE internal STABLE STRICT
  AS $$texti...
  
  Is this worth bothering about, or should I simply remove the database before
  trying to upgrade?
 
 That's a bug.  The test_ddl_deparse suite leaves a shell type, which
 pg_upgrade fails to reproduce.  Whether to have pg_upgrade support that or
 just error out cleanly is another question.

There seems little justification to not support shell types. We should
also add a shell type to the standard regression testing database,
they're weird enough that some increased exposure seems like a good
idea.


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Andres Freund and...@anarazel.de writes:
 On 2015-08-01 19:13:05 -0400, Noah Misch wrote:
 That's a bug.  The test_ddl_deparse suite leaves a shell type, which
 pg_upgrade fails to reproduce.  Whether to have pg_upgrade support that or
 just error out cleanly is another question.

 There seems little justification to not support shell types. We should
 also add a shell type to the standard regression testing database,
 they're weird enough that some increased exposure seems like a good
 idea.

Agreed.  I was a bit surprised to find that pg_dump skips shell types,
actually.  Probably that's a hangover from when create function foo()
returns bogus would autocreate a shell type named bogus.  In all
modern releases, it's fairly hard to accidentally create a shell type,
so we should probably assume that the user meant it to be there.

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

Re: [HACKERS] upgrade failure from 9.5 to head

[Andrew Dunstan]

On 08/02/2015 04:00 PM, Tom Lane wrote:

Andres Freund and...@anarazel.de writes:

On 2015-08-01 19:13:05 -0400, Noah Misch wrote:

That's a bug.  The test_ddl_deparse suite leaves a shell type, which
pg_upgrade fails to reproduce.  Whether to have pg_upgrade support that or
just error out cleanly is another question.

There seems little justification to not support shell types. We should
also add a shell type to the standard regression testing database,
they're weird enough that some increased exposure seems like a good
idea.

Agreed.  I was a bit surprised to find that pg_dump skips shell types,
actually.  Probably that's a hangover from when create function foo()
returns bogus would autocreate a shell type named bogus.  In all
modern releases, it's fairly hard to accidentally create a shell type,
so we should probably assume that the user meant it to be there.





I'm fine with fixing it, but what's the actual use case for a long lived 
shell type?


cheers

andrew



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

Re: [HACKERS] upgrade failure from 9.5 to head

[Andres Freund]

On 2015-08-02 19:06:49 -0400, Andrew Dunstan wrote:
 I'm fine with fixing it, but what's the actual use case for a long lived
 shell type?

The use-case imo is that we shouldn't just break if somebody did
something stupid or was busy creating a new type while a scheduled
backup ran.

Andres


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
 On 2015-08-01 19:13:05 -0400, Noah Misch wrote:
  On Wed, Jul 29, 2015 at 04:42:55PM -0400, Andrew Dunstan wrote:
   The next hump is this, in restoring contrib_regression_test_ddl_parse:
   
  pg_restore: creating FUNCTION public.text_w_default_in(cstring)
  pg_restore: [archiver (db)] Error while PROCESSING TOC:
  pg_restore: [archiver (db)] Error from TOC entry 243; 1255 62534
  FUNCTION text_w_default_in(cstring) buildfarm
  pg_restore: [archiver (db)] could not execute query: ERROR: pg_type
  OID value not set when in binary upgrade mode
   Command was: CREATE FUNCTION text_w_default_in(cstring)
  RETURNS text_w_default
   LANGUAGE internal STABLE STRICT
   AS $$texti...
   
   Is this worth bothering about, or should I simply remove the database 
   before
   trying to upgrade?
  
  That's a bug.  The test_ddl_deparse suite leaves a shell type, which
  pg_upgrade fails to reproduce.  Whether to have pg_upgrade support that or
  just error out cleanly is another question.
 
 There seems little justification to not support shell types. We should
 also add a shell type to the standard regression testing database,
 they're weird enough that some increased exposure seems like a good
 idea.

+1.

I was doing testing the other day and ran into the pg_dump doesn't
support shell types issue and it was annoyingly confusing.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] upgrade failure from 9.5 to head

[Andrew Dunstan]

On 08/01/2015 07:13 PM, Noah Misch wrote:

On Wed, Jul 29, 2015 at 04:42:55PM -0400, Andrew Dunstan wrote:

The next hump is this, in restoring contrib_regression_test_ddl_parse:

pg_restore: creating FUNCTION public.text_w_default_in(cstring)
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 243; 1255 62534
FUNCTION text_w_default_in(cstring) buildfarm
pg_restore: [archiver (db)] could not execute query: ERROR: pg_type
OID value not set when in binary upgrade mode
 Command was: CREATE FUNCTION text_w_default_in(cstring)
RETURNS text_w_default
 LANGUAGE internal STABLE STRICT
 AS $$texti...

Is this worth bothering about, or should I simply remove the database before
trying to upgrade?

That's a bug.  The test_ddl_deparse suite leaves a shell type, which
pg_upgrade fails to reproduce.  Whether to have pg_upgrade support that or
just error out cleanly is another question.



Well, for now I'll just drop the database.  Thanks for the diagnosis.

cheers

andrew


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Noah Misch]

On Wed, Jul 29, 2015 at 04:42:55PM -0400, Andrew Dunstan wrote:
 The next hump is this, in restoring contrib_regression_test_ddl_parse:
 
pg_restore: creating FUNCTION public.text_w_default_in(cstring)
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 243; 1255 62534
FUNCTION text_w_default_in(cstring) buildfarm
pg_restore: [archiver (db)] could not execute query: ERROR: pg_type
OID value not set when in binary upgrade mode
 Command was: CREATE FUNCTION text_w_default_in(cstring)
RETURNS text_w_default
 LANGUAGE internal STABLE STRICT
 AS $$texti...
 
 Is this worth bothering about, or should I simply remove the database before
 trying to upgrade?

That's a bug.  The test_ddl_deparse suite leaves a shell type, which
pg_upgrade fails to reproduce.  Whether to have pg_upgrade support that or
just error out cleanly is another question.


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net writes:
  * Andres Freund (and...@anarazel.de) wrote:
  Hm. That issue doesn't particularly concern me. Having all .so's
  available in the installation seems like a pretty basic
  requirement. Security labels are by far not the only things that'll fail
  without an extension's .so present, no?
 
  It's certainly an issue that postgis users are familiar with.
 
 Really?  What aspect of postgis requires mucking with
 shared_preload_libraries?

Having to have the libraries in place is what I was getting at, which is
what Andres was also talking about, if I understood correctly.

Even without having to muck with shared_preload_libraries though, you
had better have those libraries in place if you want things to work.
Having to also deal with shared_preload_libraries for some cases doesn't
strike me as a huge issue.

 If you ask me, shared_preload_libraries was only ever meant as a
 performance optimization.  If user-visible DDL behavior depends on a
 library being preloaded that way, that feature is broken.  There
 are some cases where we probably don't care enough to provide a
 proper solution, but I'm not sure why we would think that security
 labels fall in the don't-really-give-a-damn-if-it-works class.

I agree that labels are important and that we really want the label
provider loaded from shared_preload_libraries.  I also understand that
shared_preload_libraries was originally intended as a performance
optimization and that the security labels system has ended up changing
that.  I don't believe that'll be the last thing which we want to be
loaded and running from the very start (if we end up with auditing as an
extension, or any hooks in the postmaster that we provide for extensions
to use, etc).

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Stephen Frost sfr...@snowman.net writes:
 * Tom Lane (t...@sss.pgh.pa.us) wrote:
 Really?  What aspect of postgis requires mucking with
 shared_preload_libraries?

 Having to have the libraries in place is what I was getting at, which is
 what Andres was also talking about, if I understood correctly.

Right, I agree with that: you need to have installed all the software
you're using, where installed means the executable files are built
and placed where they need to be in the filesystem.

 Having to also deal with shared_preload_libraries for some cases doesn't
 strike me as a huge issue.

I think it is, especially if what we're offering as a workaround is write
a custom script and make sure that your pg_upgrade wrapper script has an
option to call that halfway through.  Rube Goldberg would be proud.

It's possible that the problem here is not so much reliance on
shared_preload_libraries as it is that there's no provision in
pg_upgrade for dealing with the need to set it.  But one way or
the other, this is a usability fail.

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

Re: [HACKERS] upgrade failure from 9.5 to head

[Alvaro Herrera]

Tom Lane wrote:
 Andres Freund and...@anarazel.de writes:

  Ick! So the dummy_seclabel test more or less only works by accident if I
  see that correctly. The .so is only loaded because the CREATE EXTENSION
  in the test triggers a CREATE FUNCTION dummy_seclabel_dummy() ... LANG
  C.

I set it up that way on purpose, because there doesn't seem to be any
other reasonable way.  (It wasn't like that in the original contrib
module).

 But on reflection, doesn't this mean that the entire implementation of
 SECURITY LABEL is broken?

Heck, see the *very first* hunk of this patch:
https://www.postgresql.org/message-id/CACACo5R4VwGddt00qtPmhUhtd%3Dokwu2ECM-j20B6%2BcmgtZF6mQ%40mail.gmail.com
This was found to be necessary during deparsing of SECURITY LABEL
command, and only of that command -- all other DDL is able to pass back
the OID of the corresponding object, so that the deparsing code can look
up the object in catalogs.  But for security label providers there is no
catalog, and there is no way to obtain the identify of the label
provider that I can see.  Thus the ugly hack in the patch: the parse
node has to be altered during execution to make sure the provider is
correctly set up for deparse to be able to obtain it.

That to me seemed pretty broken, but I found no better way.

I don't think this is dummy_seclabel's fault.

-- 
Álvaro Herrerahttp://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

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Stephen Frost sfr...@snowman.net writes:
 More generally, I completely agree that this is something which we can
 improve upon.  It doesn't seem like a release blocker or something which
 we need to fix in the back branches though.

No, it's not a release blocker; it's been like this since we invented
security labels, which seems to have been 9.1.  But security labels were
toys in 9.1, and this deficiency is one reason they still are toys.
We need to have a to-do item to get rid of it, rather than claiming
things are just fine as they are.

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

Re: [HACKERS] upgrade failure from 9.5 to head

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net writes:
  Having to also deal with shared_preload_libraries for some cases doesn't
  strike me as a huge issue.
 
 I think it is, especially if what we're offering as a workaround is write
 a custom script and make sure that your pg_upgrade wrapper script has an
 option to call that halfway through.  Rube Goldberg would be proud.
 
 It's possible that the problem here is not so much reliance on
 shared_preload_libraries as it is that there's no provision in
 pg_upgrade for dealing with the need to set it.  But one way or
 the other, this is a usability fail.

Why would it be pg_upgrade?  When using it, you initdb the new cluster
yourself and you can configure the postgresql.conf in there however you
like before running the actual pg_upgrade.  Sure, if you're using a
wrapper then you need to deal with that, but if you want pg_upgrade to
handle configuration options in postgresql.conf then you're going to
have to pass config info to the wrapper script anyway, which would then
pass it to pg_upgrade.

The wrapper script may even already deal with this if it copies the old
postgresql.conf into place (which I think the Debian one might do, with
a bit of processing for anything that needs to be addressed between the
major versions..  not looking at it right now though).

More generally, I completely agree that this is something which we can
improve upon.  It doesn't seem like a release blocker or something which
we need to fix in the back branches though.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] upgrade failure from 9.5 to head

[Andrew Dunstan]

On 07/29/2015 11:28 AM, Tom Lane wrote:

Stephen Frost sfr...@snowman.net writes:

* Tom Lane (t...@sss.pgh.pa.us) wrote:

Really?  What aspect of postgis requires mucking with
shared_preload_libraries?

Having to have the libraries in place is what I was getting at, which is
what Andres was also talking about, if I understood correctly.

Right, I agree with that: you need to have installed all the software
you're using, where installed means the executable files are built
and placed where they need to be in the filesystem.


Having to also deal with shared_preload_libraries for some cases doesn't
strike me as a huge issue.

I think it is, especially if what we're offering as a workaround is write
a custom script and make sure that your pg_upgrade wrapper script has an
option to call that halfway through.  Rube Goldberg would be proud.

It's possible that the problem here is not so much reliance on
shared_preload_libraries as it is that there's no provision in
pg_upgrade for dealing with the need to set it.  But one way or
the other, this is a usability fail.




FWIW, having the test driver add the shared_preload_libraries setting 
got over this hump - the shared library is indeed present in my setup.


The next hump is this, in restoring contrib_regression_test_ddl_parse:

   pg_restore: creating FUNCTION public.text_w_default_in(cstring)
   pg_restore: [archiver (db)] Error while PROCESSING TOC:
   pg_restore: [archiver (db)] Error from TOC entry 243; 1255 62534
   FUNCTION text_w_default_in(cstring) buildfarm
   pg_restore: [archiver (db)] could not execute query: ERROR: pg_type
   OID value not set when in binary upgrade mode
Command was: CREATE FUNCTION text_w_default_in(cstring)
   RETURNS text_w_default
LANGUAGE internal STABLE STRICT
AS $$texti...

Is this worth bothering about, or should I simply remove the database 
before trying to upgrade?


cheers

andrew


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Robert Haas]

On Wed, Jul 29, 2015 at 11:28 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 It's possible that the problem here is not so much reliance on
 shared_preload_libraries as it is that there's no provision in
 pg_upgrade for dealing with the need to set it.  But one way or
 the other, this is a usability fail.

Andres pretty much put his finger on the problem here when he
mentioned labels on shared objects.  You can imagine trying to design
this API so that when someone makes reference to label provider xyzzy
we look for a function with that name that has some magic return type
and call it, similar to what we already do with FDWs and what you
recently implemented for TABLESAMPLE.  But if the label is on a shared
object, in which database shall we call that function?  Even for
database objects, it won't do at all if the same label provider name
is used to mean different things in different databases.

Speaking as the guy who came up with much of the design for feature
and committed KaiGai's patch implementing it, I have to admit that I
didn't think of what problems this would create for pg_upgrade.  It
seemed to me at the time that this really wasn't that different from
something like pg_stat_statements, which also won't work properly
unless loaded via shared_preload_libraries.  In retrospect, there is a
difference, which is that if you don't load pg_stat_statements, your
DDL and queries will still execute, but in this case, they won't.
That's definitely raising the table stakes, but I'm not sure what to
do about it.  Preserving shared_preload_libraries across pg_upgrade is
a tempting solution, but that isn't guaranteed to solve more problems
than it creates.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Andres Freund]

On 2015-07-29 10:38:19 -0400, Tom Lane wrote:
 Well, there's a larger issue, which is that (a) Andrew's new installation
 very likely doesn't have dummy_seclabel.so built/installed at all

Hm. That issue doesn't particularly concern me. Having all .so's
available in the installation seems like a pretty basic
requirement. Security labels are by far not the only things that'll fail
without an extension's .so present, no?

 (b) even if he did, there's nothing that would cause it to get loaded
 during pg_upgrade's DDL restore run.

Well, generally it's assumed that all security labels are loaded via
shared_preload_libraries. I'm not super happy about that decision, but
given the desire to be able to have labels on shared objects I can see
the reasoning.

 Now as far as dummy_seclabel is concerned, the easy answer is we don't
 care.  But on reflection, doesn't this mean that the entire
 implementation of SECURITY LABEL is broken?  At least to the extent that
 it can't work during pg_upgrade unless the user takes manual action to
 configure the relevant providers' .so libraries into the new installation
 *before* he runs pg_upgrade.  That doesn't say production ready to me.

Hm, I don't think that particular issue is that bad. We decided labels
are only going to work if they're in shared_preload_libararies, and they
really only do if that's the case.

I think if we think we should do something here we should add a check
that label providers are loaded in s_p_l.


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
 On 2015-07-29 10:38:19 -0400, Tom Lane wrote:
  Well, there's a larger issue, which is that (a) Andrew's new installation
  very likely doesn't have dummy_seclabel.so built/installed at all
 
 Hm. That issue doesn't particularly concern me. Having all .so's
 available in the installation seems like a pretty basic
 requirement. Security labels are by far not the only things that'll fail
 without an extension's .so present, no?

It's certainly an issue that postgis users are familiar with.

  (b) even if he did, there's nothing that would cause it to get loaded
  during pg_upgrade's DDL restore run.
 
 Well, generally it's assumed that all security labels are loaded via
 shared_preload_libraries. I'm not super happy about that decision, but
 given the desire to be able to have labels on shared objects I can see
 the reasoning.

Yes.

  Now as far as dummy_seclabel is concerned, the easy answer is we don't
  care.  But on reflection, doesn't this mean that the entire
  implementation of SECURITY LABEL is broken?  At least to the extent that
  it can't work during pg_upgrade unless the user takes manual action to
  configure the relevant providers' .so libraries into the new installation
  *before* he runs pg_upgrade.  That doesn't say production ready to me.
 
 Hm, I don't think that particular issue is that bad. We decided labels
 are only going to work if they're in shared_preload_libararies, and they
 really only do if that's the case.
 
 I think if we think we should do something here we should add a check
 that label providers are loaded in s_p_l.

That has caused issues with the buildfarm in the past..  I'd like to
have a way to do that though, for label providers and potentially other
things which should really only be loaded via s_p_l.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Andres Freund and...@anarazel.de writes:
 On 2015-07-29 10:38:19 -0400, Tom Lane wrote:
 Now as far as dummy_seclabel is concerned, the easy answer is we don't
 care.  But on reflection, doesn't this mean that the entire
 implementation of SECURITY LABEL is broken?  At least to the extent that
 it can't work during pg_upgrade unless the user takes manual action to
 configure the relevant providers' .so libraries into the new installation
 *before* he runs pg_upgrade.  That doesn't say production ready to me.

 Hm, I don't think that particular issue is that bad. We decided labels
 are only going to work if they're in shared_preload_libararies, and they
 really only do if that's the case.

In that case, where in the documentation of the pg_upgrade process does
it say you must configure the new installation with all security label
providers installed in shared_preload_libraries after initdb'ing the
new installation and before running pg_upgrade?  And how can you meet
that requirement if you are using a canned script that does both those
steps for you?  (Red Hat certainly ships such a script in their packaging,
and I rather imagine that the Debian-style packages do too.)

And even more to the point, why exactly should security providers get this
dispensation when we don't make people jump through hoops like that for
anything else?  AFAICS, with the way things are now, if you simply load
a dump script without bothering with setting up shared_preload_libraries,
then you have all the objects loaded and no security labels attached to
them.  Isn't that a security breach by definition?

I think it's fairly broken if pg_upgrade output, or pg_dump output in
general, can't be loaded without such requirements.  Perhaps we could
have the dump script issue a LOAD for the label providers that will be
referenced; or maybe better, fix SECURITY LABEL FOR provider so that
it autoloads the relevant provider, which would require either a mapping
table or some convention about the .so name for a provider.

IMO, the current situation is fine for toy providers like dummy_seclabel,
but if you want the feature to ever be regarded as more than a toy,
this issue needs work.

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

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Stephen Frost sfr...@snowman.net writes:
 * Andres Freund (and...@anarazel.de) wrote:
 Hm. That issue doesn't particularly concern me. Having all .so's
 available in the installation seems like a pretty basic
 requirement. Security labels are by far not the only things that'll fail
 without an extension's .so present, no?

 It's certainly an issue that postgis users are familiar with.

Really?  What aspect of postgis requires mucking with
shared_preload_libraries?

If you ask me, shared_preload_libraries was only ever meant as a
performance optimization.  If user-visible DDL behavior depends on a
library being preloaded that way, that feature is broken.  There
are some cases where we probably don't care enough to provide a
proper solution, but I'm not sure why we would think that security
labels fall in the don't-really-give-a-damn-if-it-works class.

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

Re: [HACKERS] upgrade failure from 9.5 to head

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Andres Freund and...@anarazel.de writes:
  On 2015-07-29 10:38:19 -0400, Tom Lane wrote:
  Now as far as dummy_seclabel is concerned, the easy answer is we don't
  care.  But on reflection, doesn't this mean that the entire
  implementation of SECURITY LABEL is broken?  At least to the extent that
  it can't work during pg_upgrade unless the user takes manual action to
  configure the relevant providers' .so libraries into the new installation
  *before* he runs pg_upgrade.  That doesn't say production ready to me.
 
  Hm, I don't think that particular issue is that bad. We decided labels
  are only going to work if they're in shared_preload_libararies, and they
  really only do if that's the case.
 
 In that case, where in the documentation of the pg_upgrade process does
 it say you must configure the new installation with all security label
 providers installed in shared_preload_libraries after initdb'ing the
 new installation and before running pg_upgrade?  And how can you meet
 that requirement if you are using a canned script that does both those
 steps for you?  (Red Hat certainly ships such a script in their packaging,
 and I rather imagine that the Debian-style packages do too.)

The Debian packages have a place where you can drop scripts to be run
during the process to address exactly this issue.  I specifically asked
for that to be added because of the issues with doing PostGIS upgrades.

 And even more to the point, why exactly should security providers get this
 dispensation when we don't make people jump through hoops like that for
 anything else?  AFAICS, with the way things are now, if you simply load
 a dump script without bothering with setting up shared_preload_libraries,
 then you have all the objects loaded and no security labels attached to
 them.  Isn't that a security breach by definition?

Upgrades are not part of normal operation and if you goof it up then,
yes, you're going to run into problems, but anyone who is going through
that process is going to care quite a bit about making sure they do it
correctly, including testing the process before going through it on a
real system.

 I think it's fairly broken if pg_upgrade output, or pg_dump output in
 general, can't be loaded without such requirements.  Perhaps we could
 have the dump script issue a LOAD for the label providers that will be
 referenced; or maybe better, fix SECURITY LABEL FOR provider so that
 it autoloads the relevant provider, which would require either a mapping
 table or some convention about the .so name for a provider.
 
 IMO, the current situation is fine for toy providers like dummy_seclabel,
 but if you want the feature to ever be regarded as more than a toy,
 this issue needs work.

I'm all for improving on the current situation.  I agree that it's not a
terribly good state to be in.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Andrew Dunstan and...@dunslane.net writes:
 My cross-version upgrade testing tool just threw up this failure, 
 upgrading from 9.5 to head:

 CREATE ROLE dummy_seclabel_user1;
 CREATE ROLE
 ALTER ROLE dummy_seclabel_user1 WITH NOSUPERUSER INHERIT
 CREATEROLE NOCREATEDB LOGIN NOREPLICATION NOBYPASSRLS;
 ALTER ROLE
 SECURITY LABEL FOR dummy ON ROLE dummy_seclabel_user1 IS
 'classified';
 psql:pg_upgrade_dump_globals.sql:25: ERROR:  security label provider
 dummy is not loaded

Apparently you're trying to pg_upgrade an installation that contains
leftover databases from src/test/modules/ testing?  Not sure we have
any intention of supporting that.  Even if it made sense in some cases,
the README for dummy_seclabel is pretty definitive that that one is
not meant for production.

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

Re: [HACKERS] upgrade failure from 9.5 to head

[Andres Freund]

Hi,

On 2015-07-29 10:16:10 -0400, Andrew Dunstan wrote:
 
 My cross-version upgrade testing tool just threw up this failure, upgrading
 from 9.5 to head:
 
CREATE ROLE dummy_seclabel_user1;
CREATE ROLE
ALTER ROLE dummy_seclabel_user1 WITH NOSUPERUSER INHERIT
CREATEROLE NOCREATEDB LOGIN NOREPLICATION NOBYPASSRLS;
ALTER ROLE
SECURITY LABEL FOR dummy ON ROLE dummy_seclabel_user1 IS
'classified';
psql:pg_upgrade_dump_globals.sql:25: ERROR:  security label provider
dummy is not loaded

Ick! So the dummy_seclabel test more or less only works by accident if I
see that correctly. The .so is only loaded because the CREATE EXTENSION
in the test triggers a CREATE FUNCTION dummy_seclabel_dummy() ... LANG
C.

That's pretty damn ugly.


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

Re: [HACKERS] upgrade failure from 9.5 to head

[Tom Lane]

Andres Freund and...@anarazel.de writes:
 On 2015-07-29 10:16:10 -0400, Andrew Dunstan wrote:
 psql:pg_upgrade_dump_globals.sql:25: ERROR:  security label provider
 dummy is not loaded

 Ick! So the dummy_seclabel test more or less only works by accident if I
 see that correctly. The .so is only loaded because the CREATE EXTENSION
 in the test triggers a CREATE FUNCTION dummy_seclabel_dummy() ... LANG
 C.

Well, there's a larger issue, which is that (a) Andrew's new installation
very likely doesn't have dummy_seclabel.so built/installed at all, and
(b) even if he did, there's nothing that would cause it to get loaded
during pg_upgrade's DDL restore run.

Now as far as dummy_seclabel is concerned, the easy answer is we don't
care.  But on reflection, doesn't this mean that the entire
implementation of SECURITY LABEL is broken?  At least to the extent that
it can't work during pg_upgrade unless the user takes manual action to
configure the relevant providers' .so libraries into the new installation
*before* he runs pg_upgrade.  That doesn't say production ready to me.

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

[HACKERS] upgrade failure from 9.5 to head

[Andrew Dunstan]

My cross-version upgrade testing tool just threw up this failure, 
upgrading from 9.5 to head:


   CREATE ROLE dummy_seclabel_user1;
   CREATE ROLE
   ALTER ROLE dummy_seclabel_user1 WITH NOSUPERUSER INHERIT
   CREATEROLE NOCREATEDB LOGIN NOREPLICATION NOBYPASSRLS;
   ALTER ROLE
   SECURITY LABEL FOR dummy ON ROLE dummy_seclabel_user1 IS
   'classified';
   psql:pg_upgrade_dump_globals.sql:25: ERROR:  security label provider
   dummy is not loaded


This error might not be terribly new - it's been masked by another 
earlier problem that I have now catered for in the tool.


cheers

andrew


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