Thread: upgrade failure from 9.5 to head
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 CREATEROLENOCREATEDB 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
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.
Andrew Dunstan <andrew@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
Andres Freund <andres@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
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.
* Andres Freund (andres@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
Andres Freund <andres@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
Stephen Frost <sfrost@snowman.net> writes:
> * Andres Freund (andres@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
Tom,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Andres Freund <andres@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
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Andres Freund (andres@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
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@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
Tom Lane wrote: > Andres Freund <andres@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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@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
Stephen Frost <sfrost@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
On Wed, Jul 29, 2015 at 11:28 AM, Tom Lane <tgl@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
On 07/29/2015 11:28 AM, Tom Lane wrote:
> Stephen Frost <sfrost@snowman.net> writes:
>> * Tom Lane (tgl@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
PROCESSINGTOC: 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
notset 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
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.
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
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.
Andres Freund <andres@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
On 08/02/2015 04:00 PM, Tom Lane wrote: > Andres Freund <andres@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
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
* Andres Freund (andres@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
On Sun, Aug 2, 2015 at 8:20 PM, Stephen Frost <sfrost@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
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Aug 2, 2015 at 8:20 PM, Stephen Frost <sfrost@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
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
On Tue, Aug 4, 2015 at 1:54 PM, Andres Freund <andres@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
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 4, 2015 at 1:54 PM, Andres Freund <andres@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
On 08/04/2015 02:09 PM, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Aug 4, 2015 at 1:54 PM, Andres Freund <andres@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
On Wed, Jul 29, 2015 at 11:01:40AM -0400, Tom Lane wrote: > Andres Freund <andres@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 pg_upgrade docs have: <title>Install custom shared object files</title> <para> Install any custom shared object files (or DLLs) used by the old cluster into the new cluster, e.g. <filename>pgcrypto.so</filename>, whether they are from <filename>contrib</filename> or some other source. Do not installthe schema definitions, e.g. <filename>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. </para> </step> 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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +