Remove superuser() checks from pgstattuple

On 8/23/16 5:22 PM, Stephen Frost wrote:

Now that we track initial privileges on extension objects and changes to
those permissions, we can drop the superuser() checks from the various
functions which are part of the pgstattuple extension.

Since a pg_upgrade will preserve the version of the extension which
existed prior to the upgrade, we can't simply modify the existing
functions but instead need to create new functions which remove the
checks and update the SQL-level functions to use the new functions

I think this is a good change to pursue, and we'll likely want to do
more similar changes in contrib. But I'm worried that what is logically
a 10-line change will end up a 20 KiB patch every time.

Have we explored other options for addressing the upgrade problems?
Maybe the function could check that non-default privileges have been
granted?

--
Peter Eisentraut 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

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 8/23/16 5:22 PM, Stephen Frost wrote:

Now that we track initial privileges on extension objects and changes to
those permissions, we can drop the superuser() checks from the various
functions which are part of the pgstattuple extension.

Since a pg_upgrade will preserve the version of the extension which
existed prior to the upgrade, we can't simply modify the existing
functions but instead need to create new functions which remove the
checks and update the SQL-level functions to use the new functions

I think this is a good change to pursue, and we'll likely want to do
more similar changes in contrib. But I'm worried that what is logically
a 10-line change will end up a 20 KiB patch every time.

This is primairly due to how we handle new versions of an extension.
Any change to an extension is going to involve a new upgrade script and
the removal of the prior version install script and addition of the new
version install scripts.

Have we explored other options for addressing the upgrade problems?

We did discuss the upgrade issue and Noah proposed the current approach,
which appears to be the best option.

Maybe the function could check that non-default privileges have been
granted?

Simply changing the function to behave differently depending on what
privileges have or havn't been granted doesn't seem like a very good
idea.

Consider an existing installation where the admin tried to grant access
to one of these functions:

GRANT EXECUTE ON pgstattuple_func() TO bob;

This would result in a GRANT to bob explicitly, and the GRANT to public
would still be there also.

Then an upgrade of PG, without upgrading the extension, would lead to
any user being able to execute the function. An upgrade of the
extension would revoke the GRANT to PUBLIC and, further, would
hopefuflly cause the admin to consider checking the documentation about
the upgrade (which needs to be added; I'll do that).

We also created a new version to add the PARALLEL SAFE markings to the
functions. In general, I believe it's better to use a new version when
we're making these kinds of changes.

Thanks!

Stephen

[ warning, thread hijack ahead ]

Stephen Frost <sfrost@snowman.net> writes:

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

I think this is a good change to pursue, and we'll likely want to do
more similar changes in contrib. But I'm worried that what is logically
a 10-line change will end up a 20 KiB patch every time.

We also created a new version to add the PARALLEL SAFE markings to the
functions. In general, I believe it's better to use a new version when
we're making these kinds of changes.

It is becoming clear that the current extension update mechanism is kind
of brute-force for this sort of change. I have no ideas offhand about a
better way to do it, but like Peter, I was dismayed by the amount of pure
overhead involved in the PARALLEL SAFE updates.

It's not only development overhead, either: users have to remember to
run around and issue ALTER EXTENSION UPDATE for every extension they
have, in every database they have it installed in. Anyone want to
lay a side bet on how many users will actually do that? Given that
the release notes don't currently suggest doing so, I'd be willing
to put money on "none at all" :-(

I wonder whether pg_upgrade ought to be changed to attempt upgrading
every extension after it's completed the basic migration. Or at
least offer a script containing all the needed commands.

Anyway, it's not this particular patch's job to do better, but we
oughta think about better ways to do it.

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

Tom,

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

[ warning, thread hijack ahead ]

quite.

Stephen Frost <sfrost@snowman.net> writes:

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

I think this is a good change to pursue, and we'll likely want to do
more similar changes in contrib. But I'm worried that what is logically
a 10-line change will end up a 20 KiB patch every time.

We also created a new version to add the PARALLEL SAFE markings to the
functions. In general, I believe it's better to use a new version when
we're making these kinds of changes.

It is becoming clear that the current extension update mechanism is kind
of brute-force for this sort of change. I have no ideas offhand about a
better way to do it, but like Peter, I was dismayed by the amount of pure
overhead involved in the PARALLEL SAFE updates.

To make the patches smaller, it seems that we'd need a way to avoid the
removal and re-addition of the entire install script and a way to avoid
having to add a new upgrade script.

Perhaps if the versioned install script was actually a non-versioned
install script in the source tree, and the Makefile simply installed it
into the correct place, then we could eliminate that part. (All very
hand-wavy, I've not looked at what it'd take to do.)

I don't have any great answers for the upgrade script off-hand. We
could come up with a way for one file to handle multiple upgrade
options, but that doesn't really make the patch any smaller.

It's not only development overhead, either: users have to remember to
run around and issue ALTER EXTENSION UPDATE for every extension they
have, in every database they have it installed in. Anyone want to
lay a side bet on how many users will actually do that? Given that
the release notes don't currently suggest doing so, I'd be willing
to put money on "none at all" :-(

I agree, that's also an issue.

I wonder whether pg_upgrade ought to be changed to attempt upgrading
every extension after it's completed the basic migration. Or at
least offer a script containing all the needed commands.

I like the idea of having an option and documentation to go along with
it. Hopefully that will help administrators at least realize that it's
something they'll want to look at doing.

Anyway, it's not this particular patch's job to do better, but we
oughta think about better ways to do it.

Agreed.

Thanks!

Stephen

On 2016-09-04 11:55:06 -0400, Tom Lane wrote:

[ warning, thread hijack ahead ]

Stephen Frost <sfrost@snowman.net> writes:

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

I think this is a good change to pursue, and we'll likely want to do
more similar changes in contrib. But I'm worried that what is logically
a 10-line change will end up a 20 KiB patch every time.

We also created a new version to add the PARALLEL SAFE markings to the
functions. In general, I believe it's better to use a new version when
we're making these kinds of changes.

It is becoming clear that the current extension update mechanism is kind
of brute-force for this sort of change. I have no ideas offhand about a
better way to do it, but like Peter, I was dismayed by the amount of pure
overhead involved in the PARALLEL SAFE updates.

Agreed. I think one way, which a few extensions are taking, is to have a
base version and then incremental version upgrades. Currently CREATE
EXTENSION doesn't natively support that, so you have to concatenate the
upgrade scripts. I think it'd be great if we could add a 'baseversion'
property to the extension control file. When you create a new extension,
it'll start with the base version and then use the existing code to find
a path to upgrade to the target version. That also makes it a lot
easier to actually properly test extension upgrade paths, something
we've not really been good at.

It's not only development overhead, either: users have to remember to
run around and issue ALTER EXTENSION UPDATE for every extension they
have, in every database they have it installed in. Anyone want to
lay a side bet on how many users will actually do that? Given that
the release notes don't currently suggest doing so, I'd be willing
to put money on "none at all" :-(

Some certainly are, but I'm afraid that you're right that it's not too
many. If we don't make pg_upgrade upgrade all extensions, we should at
least have it generate a script doing so.

Greetings,

Andres Freund

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

Andres Freund <andres@anarazel.de> writes:

On 2016-09-04 11:55:06 -0400, Tom Lane wrote:

It is becoming clear that the current extension update mechanism is kind
of brute-force for this sort of change. I have no ideas offhand about a
better way to do it, but like Peter, I was dismayed by the amount of pure
overhead involved in the PARALLEL SAFE updates.

Agreed. I think one way, which a few extensions are taking, is to have a
base version and then incremental version upgrades. Currently CREATE
EXTENSION doesn't natively support that, so you have to concatenate the
upgrade scripts. I think it'd be great if we could add a 'baseversion'
property to the extension control file. When you create a new extension,
it'll start with the base version and then use the existing code to find
a path to upgrade to the target version. That also makes it a lot
easier to actually properly test extension upgrade paths, something
we've not really been good at.

Hm, couldn't we do that automatically? At least in the case where only
one base-version script is available?

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

On 2016-09-04 21:09:41 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2016-09-04 11:55:06 -0400, Tom Lane wrote:

It is becoming clear that the current extension update mechanism is kind
of brute-force for this sort of change. I have no ideas offhand about a
better way to do it, but like Peter, I was dismayed by the amount of pure
overhead involved in the PARALLEL SAFE updates.

Agreed. I think one way, which a few extensions are taking, is to have a
base version and then incremental version upgrades. Currently CREATE
EXTENSION doesn't natively support that, so you have to concatenate the
upgrade scripts. I think it'd be great if we could add a 'baseversion'
property to the extension control file. When you create a new extension,
it'll start with the base version and then use the existing code to find
a path to upgrade to the target version. That also makes it a lot
easier to actually properly test extension upgrade paths, something
we've not really been good at.

Hm, couldn't we do that automatically? At least in the case where only
one base-version script is available?

You mean determining the baseversion? Hm, yes, that might work. But I'm
not sure how much we can rely on no earlier scripts being already
installed or such. Seems like a problem you'd possibly only encounter in
the field or dirty development environments.

Andres

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

Andres Freund <andres@anarazel.de> writes:

On 2016-09-04 21:09:41 -0400, Tom Lane wrote:

Hm, couldn't we do that automatically? At least in the case where only
one base-version script is available?

You mean determining the baseversion? Hm, yes, that might work. But I'm
not sure how much we can rely on no earlier scripts being already
installed or such. Seems like a problem you'd possibly only encounter in
the field or dirty development environments.

Actually, why would we care? Pick one, with some tiebreaker rules
(shortest update path, for starters). I think nearly all of the
infrastructure for this is already there in extension.c.

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

On September 4, 2016 6:33:30 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

On 2016-09-04 21:09:41 -0400, Tom Lane wrote:

Hm, couldn't we do that automatically? At least in the case where

only

one base-version script is available?

You mean determining the baseversion? Hm, yes, that might work. But

I'm

not sure how much we can rely on no earlier scripts being already
installed or such. Seems like a problem you'd possibly only encounter

in

the field or dirty development environments.

Actually, why would we care? Pick one, with some tiebreaker rules
(shortest update path, for starters).

Fair point.

I think nearly all of the
infrastructure for this is already there in extension.c.

Yes, it doesn't sound very hard...

Andres

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

Andres Freund <andres@anarazel.de> writes:

On September 4, 2016 6:33:30 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think nearly all of the
infrastructure for this is already there in extension.c.

Yes, it doesn't sound very hard...

I poked at this a bit, and indeed it's not that hard. Attached is a
proposed patch (code-complete, I think, but no docs as yet).
Aside from touching CREATE EXTENSION itself, this modifies the
pg_available_extension_versions view to show versions that are installable
on the basis of update chains. I think pg_extension_update_paths() needs
no changes, though.

Ordinarily I'd be willing to stick this on the queue for the next
commitfest, but it seems like we ought to try to get it pushed now
so that Stephen can make use of the feature for his superuser changes.
Thoughts?

regards, tom lane

On 2016-09-05 22:24:09 -0400, Tom Lane wrote:

Ordinarily I'd be willing to stick this on the queue for the next
commitfest, but it seems like we ought to try to get it pushed now
so that Stephen can make use of the feature for his superuser changes.
Thoughts?

Seems sensible to me. I can have a look at it one of the next few days
if you want.

Andres

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

Andres Freund <andres@anarazel.de> writes:

On 2016-09-05 22:24:09 -0400, Tom Lane wrote:

Ordinarily I'd be willing to stick this on the queue for the next
commitfest, but it seems like we ought to try to get it pushed now
so that Stephen can make use of the feature for his superuser changes.
Thoughts?

Seems sensible to me. I can have a look at it one of the next few days
if you want.

Thanks for offering. Attached is an updated patch that addresses a
couple of issues I noticed:

1. When I did the previous patch, I was thinking that any parameters
specified in an auxiliary .control file for the base version would
apply to any non-base versions based on it; so I had the
pg_available_extension_versions view just duplicate those parameters.
But actually, most of those parameters are just applied on-the-fly
while processing the update script, so it *does* work for them to be
different for different versions. The exceptions are schema (which
you can't modify during an update) and comment (while we could replace
the comment during an update, it doesn't seem like a good idea because
we might overwrite a user-written comment if we did). (I think this
behavior is undocumented BTW :-(.) So this fixes the view to only
inherit those two parameters from the base version.

2. I noticed that CASCADE was not implemented for required extensions
processed by ApplyExtensionUpdates, which seems like a bad thing if
CREATE processing is going to rely more heavily on that. This only
matters if different versions have different requires lists, but that
is supposed to be supported, and all the catalog-hacking for it is there.
So I made this work. It took a fair amount of refactoring in order to
avoid code duplication, but it wasn't really a very big change.

At this point it's awfully tempting to make ALTER EXTENSION UPDATE grow
a CASCADE option to allow automatic installation of new requirements
of the new version, but I didn't do that here.

Still no SGML doc updates.

regards, tom lane

I wrote:

Still no SGML doc updates.

And here's a doc addition.

regards, tom lane

On 9/4/16 7:36 PM, Stephen Frost wrote:

Perhaps if the versioned install script was actually a non-versioned
install script in the source tree, and the Makefile simply installed it
into the correct place, then we could eliminate that part. (All very
hand-wavy, I've not looked at what it'd take to do.)

A number of external extensions actually do it that way, but it also has
its problems. For example, if you don't preserve the old base install
scripts, you can't actually test whether your upgrade scripts work in
the sense that they upgrade you to the same place.

This is now being obsoleted by the later idea of allowing base installs
from a chain of upgrade scripts. But if your upgrade scripts contain
ALTER TABLE commands, you will probably still want to write base install
scripts that do a fresh CREATE TABLE instead.

--
Peter Eisentraut 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

Hi,

On 2016-09-07 09:46:32 -0400, Tom Lane wrote:

At this point it's awfully tempting to make ALTER EXTENSION UPDATE grow
a CASCADE option to allow automatic installation of new requirements
of the new version, but I didn't do that here.

Sounds like a plan. After the refactoring that should be easy.

@@ -1086,6 +1097,9 @@ identify_update_path(ExtensionControlFil
* Apply Dijkstra's algorithm to find the shortest path from evi_start to
* evi_target.
*
+ * If reject_indirect is true, ignore paths that go through installable
+ * versions (presumably, caller will consider starting from such versions).

Reading this I was initially confused, only after reading
find_install_path() this made sense. It's to cut the search short,
right? Rephrasing this a bit might make sense.

+		/*
+		 * No FROM, so we're installing from scratch.  If there is an install
+		 * script for the desired version, we only need to run that one.
+		 */
+		char	   *filename;
+		struct stat fst;
+
oldVersionName = NULL;
-		updateVersions = NIL;
+
+		filename = get_extension_script_filename(pcontrol, NULL, versionName);
+		if (stat(filename, &fst) == 0)
+		{
+			/* Easy, no extra scripts */
+			updateVersions = NIL;
+		}
+		else
+		{
+			/* Look for best way to install this version */
+			List	   *evi_list;
+			ExtensionVersionInfo *evi_target;
+
+			/* Extract the version update graph from the script directory */
+			evi_list = get_ext_ver_list(pcontrol);
+
+			/* Identify the target version */
+			evi_target = get_ext_ver_info(versionName, &evi_list);
+
+			/*
+			 * We don't expect it to be installable, but maybe somebody added
+			 * a suitable script right after our stat() test.
+			 */
+			if (evi_target->installable)
+			{
+				/* Easy, no extra scripts */
+				updateVersions = NIL;
+			}

Heh, that's an odd thing to handle.

+			else
+			{
+				/* Identify best path to reach target */
+				ExtensionVersionInfo *evi_start;
+
+				evi_start = find_install_path(evi_list, evi_target,
+											  &updateVersions);
+
+				/* Fail if no path ... */
+				if (evi_start == NULL)
+					ereport(ERROR,
+							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+							 errmsg("extension \"%s\" has no installation script for version \"%s\"",
+									pcontrol->name, versionName)));

Maybe, and I mean maybe, we should rephrase this to hint at indirect
installability?

Looks good to me.

Greetings,

Andres Freund

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

On 2016-09-07 13:44:28 -0400, Tom Lane wrote:

+   <sect2>
+    <title>Installing Extensions using Update Scripts</title>
+
+    <para>
+     An extension that has been around for awhile will probably exist in

Wanted to cry typo for 'awhile' here, but apparently that's actually a
word. The German in me is pleased.

+     several versions, for which the author will need to write update scripts.
+     For example, if you have released a <literal>foo</> extension in
+     versions <literal>1.0</>, <literal>1.1</>, and <literal>1.2</>, there
+     should be update scripts <filename>foo--1.0--1.1.sql</>
+     and <filename>foo--1.1--1.2.sql</>.
+     Before <productname>PostgreSQL</> 10, it was necessary to create new
+     script files <filename>foo--1.1.sql</> and <filename>foo--1.2.sql</>
+     containing the same changes, or else the newer versions could not be

Maybe replace "same changes" "directly creating the extension's
contents" or such?

- Andres

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

Andres Freund <andres@anarazel.de> writes:

On 2016-09-07 09:46:32 -0400, Tom Lane wrote:

+ * If reject_indirect is true, ignore paths that go through installable
+ * versions (presumably, caller will consider starting from such versions).

Reading this I was initially confused, only after reading
find_install_path() this made sense. It's to cut the search short,
right? Rephrasing this a bit might make sense.

Hm, I think I had a reason why that was important and not just an
optimization, but right now I don't remember why. Will take a look
and clarify the comment.

+			/*
+			 * We don't expect it to be installable, but maybe somebody added
+			 * a suitable script right after our stat() test.
+			 */
+			if (evi_target->installable)
+			{
+				/* Easy, no extra scripts */
+				updateVersions = NIL;
+			}

Heh, that's an odd thing to handle.

Yeah, it's an unlikely race condition, but if it did happen we'd hit the
"Assert(!evi_target->installable)" in find_install_path, and then the
"Assert(evi_start != evi_target)" in find_update_path. Maybe that's not
worth worrying about, since it looks like there'd be no ill effects in
non-assert builds: AFAICS we'd correctly deduce that we should use the
now-installable script with no update steps.

+					ereport(ERROR,
+							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+							 errmsg("extension \"%s\" has no installation script for version \"%s\"",
+									pcontrol->name, versionName)));

Maybe, and I mean maybe, we should rephrase this to hint at indirect
installability?

Not sure, what would better wording be?

+     several versions, for which the author will need to write update scripts.
+     For example, if you have released a <literal>foo</> extension in
+     versions <literal>1.0</>, <literal>1.1</>, and <literal>1.2</>, there
+     should be update scripts <filename>foo--1.0--1.1.sql</>
+     and <filename>foo--1.1--1.2.sql</>.
+     Before <productname>PostgreSQL</> 10, it was necessary to create new
+     script files <filename>foo--1.1.sql</> and <filename>foo--1.2.sql</>
+     containing the same changes, or else the newer versions could not be

Maybe replace "same changes" "directly creating the extension's
contents" or such?

Well, the main point is that you'd have to duplicate the effects of the
update script. Not sure how to improve it without drawing attention
away from that.

Thanks for reviewing!

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

Pushed with adjustments for the review points. Hopefully this will make
Stephen's life easier, along with others submitting contrib-module
updates. We should urge anyone who submits an old-style extension update
patch to consider whether they really want to bother with a new base
script.

At some point it might be nice to have an articulated project policy
about when a new base script is or isn't appropriate, but I doubt we
have enough experience to lay one down now.

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

Peter, all,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

This is now being obsoleted by the later idea of allowing base installs
from a chain of upgrade scripts. But if your upgrade scripts contain
ALTER TABLE commands, you will probably still want to write base install
scripts that do a fresh CREATE TABLE instead.

I've updated the patch to remove the new base version script and to rely
on the changes made by Tom to install the 1.4 version and then upgrade
to 1.5.

Based on my testing, it appears to all work correctly.

Updated (much smaller) patch attached.

Thanks!

Stephen