Re: [HACKERS] Remove superuser() checks from pgstattuple
Michael, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Tue, Sep 27, 2016 at 4:43 AM, Stephen Frost wrote: > > * Peter Eisentraut (peter.eisentr...@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. > > Same conclusion from here. Fantastic, thanks for the testing! > > Updated (much smaller) patch attached. > > I had a look at it, and it is doing the work it claims to do. So I am > marking it as "Ready for Committer". Great. I'm going to go over the whole patch closely again and will push it soon. Thanks again! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Remove superuser() checks from pgstattuple
On Tue, Sep 27, 2016 at 4:43 AM, Stephen Frost wrote: > * Peter Eisentraut (peter.eisentr...@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. Same conclusion from here. > Updated (much smaller) patch attached. I had a look at it, and it is doing the work it claims to do. So I am marking it as "Ready for Committer". -- Michael -- 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] Remove superuser() checks from pgstattuple
Peter, all,
* Peter Eisentraut (peter.eisentr...@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
From 508a4db0111607d8d59eef4a8e172f8a7aa8fdfa Mon Sep 17 00:00:00 2001
From: Stephen Frost
Date: Tue, 23 Aug 2016 17:13:09 -0400
Subject: [PATCH] Remove superuser checks in pgstattuple
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
(and to REVOKE EXECUTE rights on those functions from PUBLIC).
Approach suggested by Noah.
---
contrib/pgstattuple/Makefile | 7 +-
contrib/pgstattuple/pgstatapprox.c| 35 ++--
contrib/pgstattuple/pgstatindex.c | 108 +++--
contrib/pgstattuple/pgstattuple--1.4--1.5.sql | 111 ++
contrib/pgstattuple/pgstattuple.c | 36 +
contrib/pgstattuple/pgstattuple.control | 2 +-
6 files changed, 284 insertions(+), 15 deletions(-)
create mode 100644 contrib/pgstattuple/pgstattuple--1.4--1.5.sql
diff --git a/contrib/pgstattuple/Makefile b/contrib/pgstattuple/Makefile
index e732680..294077d 100644
--- a/contrib/pgstattuple/Makefile
+++ b/contrib/pgstattuple/Makefile
@@ -4,9 +4,10 @@ MODULE_big = pgstattuple
OBJS = pgstattuple.o pgstatindex.o pgstatapprox.o $(WIN32RES)
EXTENSION = pgstattuple
-DATA = pgstattuple--1.4.sql pgstattuple--1.3--1.4.sql \
- pgstattuple--1.2--1.3.sql pgstattuple--1.1--1.2.sql \
- pgstattuple--1.0--1.1.sql pgstattuple--unpackaged--1.0.sql
+DATA = pgstattuple--1.4.sql pgstattuple--1.4--1.5.sql \
+ pgstattuple--1.3--1.4.sql pgstattuple--1.2--1.3.sql \
+ pgstattuple--1.1--1.2.sql pgstattuple--1.0--1.1.sql \
+ pgstattuple--unpackaged--1.0.sql
PGFILEDESC = "pgstattuple - tuple-level statistics"
REGRESS = pgstattuple
diff --git a/contrib/pgstattuple/pgstatapprox.c b/contrib/pgstattuple/pgstatapprox.c
index a49ff54..11bae14 100644
--- a/contrib/pgstattuple/pgstatapprox.c
+++ b/contrib/pgstattuple/pgstatapprox.c
@@ -29,6 +29,9 @@
#include "commands/vacuum.h"
PG_FUNCTION_INFO_V1(pgstattuple_approx);
+PG_FUNCTION_INFO_V1(pgstattuple_approx_v1_5);
+
+Datum pgstattuple_approx_internal(Oid relid, FunctionCallInfo fcinfo);
typedef struct output_type
{
@@ -209,6 +212,33 @@ Datum
pgstattuple_approx(PG_FUNCTION_ARGS)
{
Oid relid = PG_GETARG_OID(0);
+
+ if (!superuser())
+ ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser to use pgstattuple functions";
+
+ PG_RETURN_DATUM(pgstattuple_approx_internal(relid, fcinfo));
+}
+
+/*
+ * As of pgstattuple version 1.5, we no longer need to check if the user
+ * is a superuser because we REVOKE EXECUTE on the function from PUBLIC.
+ * Users can then grant access to it based on their policies.
+ *
+ * Otherwise identical to pgstattuple_approx (above).
+ */
+Datum
+pgstattuple_approx_v1_5(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+
+ PG_RETURN_DATUM(pgstattuple_approx_internal(relid, fcinfo));
+}
+
+Datum
+pgstattuple_approx_internal(Oid relid, FunctionCallInfo fcinfo)
+{
Relation rel;
output_type stat = {0};
TupleDesc tupdesc;
@@ -217,11 +247,6 @@ pgstattuple_approx(PG_FUNCTION_ARGS)
HeapTuple ret;
int i = 0;
- if (!superuser())
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- (errmsg("must be superuser to use pgstattuple functions";
-
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 6084589..753e52d 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -54,6 +54,14 @@ PG_FUNCTION_INFO_V1(pg_relpages);
PG_FUNCTION_INFO_V1(pg_relpagesbyid);
PG_FUNCTION_INFO_V1(pgstatginindex);
+PG_FUNCTION_INFO_V1(pgstatindex_v1_5);
+PG_FUNCTION_INFO_V1(pgstatindexbyid_v1_5);
+PG_FUNCTION_INFO_V1(pg_relpages_v1_5);
+PG_FUNCTION_INFO_V1(pg_relpagesbyid_v1_5);
+PG_FUNCTION_INFO_V1(pgstatginindex_v1_5);
+
+Datum pgstatginindex_internal(Oid relid, FunctionCallInfo fcinfo);
+
#define IS_INDEX(r) ((r)->rd_rel-
Re: Install extensions using update scripts (was Re: [HACKERS] Remove superuser() checks from pgstattuple)
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
Re: Install extensions using update scripts (was Re: [HACKERS] Remove superuser() checks from pgstattuple)
Andres Freund 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 foo extension in
>> + versions 1.0, 1.1, and 1.2, there
>> + should be update scripts foo--1.0--1.1.sql
>> + and foo--1.1--1.2.sql.
>> + Before PostgreSQL 10, it was necessary to create new
>> + script files foo--1.1.sql and 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
Re: Install extensions using update scripts (was Re: [HACKERS] Remove superuser() checks from pgstattuple)
On 2016-09-07 13:44:28 -0400, Tom Lane wrote: > + > +Installing Extensions using Update Scripts > + > + > + 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 foo extension in > + versions 1.0, 1.1, and 1.2, there > + should be update scripts foo--1.0--1.1.sql > + and foo--1.1--1.2.sql. > + Before PostgreSQL 10, it was necessary to create new > + script files foo--1.1.sql and 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
Re: Install extensions using update scripts (was Re: [HACKERS] Remove superuser() checks from pgstattuple)
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
Re: [HACKERS] Remove superuser() checks from pgstattuple
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
Re: Install extensions using update scripts (was Re: [HACKERS] Remove superuser() checks from pgstattuple)
I wrote: > Still no SGML doc updates. And here's a doc addition. regards, tom lane diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index df88380..1c8c420 100644 *** a/doc/src/sgml/extend.sgml --- b/doc/src/sgml/extend.sgml *** SELECT * FROM pg_extension_update_paths( *** 885,890 --- 885,927 + + Installing Extensions using Update Scripts + + + An extension that has been around for awhile will probably exist in + several versions, for which the author will need to write update scripts. + For example, if you have released a foo extension in + versions 1.0, 1.1, and 1.2, there + should be update scripts foo--1.0--1.1.sql + and foo--1.1--1.2.sql. + Before PostgreSQL 10, it was necessary to create new + script files foo--1.1.sql and foo--1.2.sql + containing the same changes, or else the newer versions could not be + installed directly, only by installing 1.0 and then updating. + Now, CREATE EXTENSION can do that automatically — + for example, if only the script + files foo--1.0.sql, foo--1.0--1.1.sql, + and foo--1.1--1.2.sql are available then a request to + install version 1.2 is honored by running those three scripts + in sequence. (As with update requests, if multiple pathways are + available then the shortest is preferred.) + Arranging an extension's script files in this style can reduce the amount + of maintenance effort needed to produce small updates. + + + + If you use secondary (version-specific) control files with an extension + maintained in this style, keep in mind that each version needs a control + file even if it has no stand-alone installation script, as that control + file will determine how the implicit update to that version is performed. + For example, if foo--1.0.control specifies requires + = 'bar' but foo's other control files do not, the + extension's dependency on bar will be dropped when updating + from 1.0 to another version. + + + Extension Example -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Install extensions using update scripts (was Re: [HACKERS] Remove superuser() checks from pgstattuple)
Andres Freund 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 diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c index df49a78..59f9e21 100644 *** a/src/backend/commands/extension.c --- b/src/backend/commands/extension.c *** typedef struct ExtensionVersionInfo *** 100,113 static List *find_update_path(List *evi_list, ExtensionVersionInfo *evi_start, ExtensionVersionInfo *evi_target, bool reinitialize); static void get_available_versions_for_extension(ExtensionControlFile *pcontrol, Tuplestorestate *tupstore, TupleDesc tupdesc); static void ApplyExtensionUpdates(Oid extensionOid, ExtensionControlFile *pcontrol, const char *initialVersion, ! List *updateVersions); static char *read_whole_file(const char *filename, int *length); --- 100,124 static List *find_update_path(List *evi_list, ExtensionVersionInfo *evi_start, ExtensionVersionInfo *evi_target, + bool reject_indirect, bool reinitialize); + static Oid get_required_extension(char *reqExtensionName, + char *extensionName, + char *origSchemaName, + bool cascade, + List *parents, + bool is_create); static void get_available_versions_for_extension(ExtensionControlFile *pcontrol, Tuplestorestate *tupstore, TupleDesc tupdesc); + static Datum convert_requires_to_datum(List *requires); static void ApplyExtensionUpdates(Oid extensionOid, ExtensionControlFile *pcontrol, const char *initialVersion, ! List *updateVersions, ! char *origSchemaName, ! bool cascade, ! bool is_create); static char *read_whole_file(const char *filename, int *length); *** identify_update_path(ExtensionControlFil *** 1071,1077 evi_target = get_ext_ver_info(newVersion, &evi_list); /* Find shortest path */ ! result = find_update_path(evi_list, evi_start, evi_target, false); if (result == NIL) ereport(ERROR, --- 1082,1088 evi_target = get_ext_ver_info(newVersion, &evi_list); /* Find shortest path */ ! result = find_update_path(evi_list, evi_start, evi_target, false, false); if (result == NIL) ereport(ERROR, *** identify_update_path(ExtensionControlFil *** 1086,1091 --- 1097,1105 * 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). + * * If reinitialize is false, assume the ExtensionVersionInfo list has not * been used for this before, and the initialization done by get_ext_ver_info * is still good. *** static List * *** 1097,1102 --- ,1117 find_update_path(List *evi_list, ExtensionVersionInfo *evi_start, ExtensionVersionInfo *evi_target, + bool reject_in
Re: Install extensions using update scripts (was Re: [HACKERS] Remove superuser() checks from pgstattuple)
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
Install extensions using update scripts (was Re: [HACKERS] Remove superuser() checks from pgstattuple)
Andres Freund writes:
> On September 4, 2016 6:33:30 PM PDT, Tom Lane 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
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index fa861e6..4d82527 100644
*** a/src/backend/commands/extension.c
--- b/src/backend/commands/extension.c
*** typedef struct ExtensionVersionInfo
*** 100,105
--- 100,106
static List *find_update_path(List *evi_list,
ExtensionVersionInfo *evi_start,
ExtensionVersionInfo *evi_target,
+ bool reject_indirect,
bool reinitialize);
static void get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
*** identify_update_path(ExtensionControlFil
*** 1071,1077
evi_target = get_ext_ver_info(newVersion, &evi_list);
/* Find shortest path */
! result = find_update_path(evi_list, evi_start, evi_target, false);
if (result == NIL)
ereport(ERROR,
--- 1072,1078
evi_target = get_ext_ver_info(newVersion, &evi_list);
/* Find shortest path */
! result = find_update_path(evi_list, evi_start, evi_target, false, false);
if (result == NIL)
ereport(ERROR,
*** identify_update_path(ExtensionControlFil
*** 1086,1091
--- 1087,1095
* 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).
+ *
* If reinitialize is false, assume the ExtensionVersionInfo list has not
* been used for this before, and the initialization done by get_ext_ver_info
* is still good.
*** static List *
*** 1097,1102
--- 1101,1107
find_update_path(List *evi_list,
ExtensionVersionInfo *evi_start,
ExtensionVersionInfo *evi_target,
+ bool reject_indirect,
bool reinitialize)
{
List *result;
*** find_update_path(List *evi_list,
*** 1105,1110
--- 1110,1117
/* Caller error if start == target */
Assert(evi_start != evi_target);
+ /* Caller error if reject_indirect and target is installable */
+ Assert(!(reject_indirect && evi_target->installable));
if (reinitialize)
{
*** find_update_path(List *evi_list,
*** 1131,1136
--- 1138,1146
ExtensionVersionInfo *evi2 = (ExtensionVersionInfo *) lfirst(lc);
int newdist;
+ /* if reject_indirect, treat installable versions as unreachable */
+ if (reject_indirect && evi2->installable)
+ continue;
newdist = evi->distance + 1;
if (newdist < evi2->distance)
{
*** find_update_path(List *evi_list,
*** 1167,1172
--- 1177,1239
}
/*
+ * Given a target version that is not directly installable, find the
+ * best installation sequence starting from a directly-installable version.
+ *
+ * evi_list: previously-collected version update graph
+ * evi_target: member of that list that we want to reach
+ *
+ * Returns the best starting-point version, or NULL if there is none.
+ * On success, *best_path is set to the path from the start point.
+ *
+ * If there's more than one possible start point, prefer shorter update paths,
+ * and break any ties arbitrarily on the basis of strcmp'ing the starting
+ * versions' names.
+ */
+ static ExtensionVersionInfo *
+ find_install_path(List *evi_list, ExtensionVersionInfo *evi_target,
+ List **best_path)
+ {
+ ExtensionVersionInfo *evi_start = NULL;
+ ListCell *lc;
+
+ /* Target should not be installable */
+ Assert(!evi_target->installable);
+
+ *best_path = NIL;
+
+ /* Consider all installable versions as start points */
+ foreach(lc, evi_list)
+ {
+ ExtensionVersionInfo *evi1 = (ExtensionVersionInfo *) lfirst(lc);
+ List *path;
+
+ if (!evi1->installable)
+ continue;
+
+ /*
+ * Find shortest path from evi1 to evi_target; but no need to consider
+ * paths going through other installable versions.
+ */
+ path = find_update_path(evi_list, evi1, evi_target, true, true);
+ if (path == NIL)
+ continue;
+
+ /* Remember best path */
+ if (evi_start == NULL ||
+ list_length(path) < list_leng
Re: [HACKERS] Remove superuser() checks from pgstattuple
On September 4, 2016 6:33:30 PM PDT, Tom Lane wrote: >Andres Freund 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
Re: [HACKERS] Remove superuser() checks from pgstattuple
Andres Freund 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
Re: [HACKERS] Remove superuser() checks from pgstattuple
On 2016-09-04 21:09:41 -0400, Tom Lane wrote: > Andres Freund 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
Re: [HACKERS] Remove superuser() checks from pgstattuple
Andres Freund 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
Re: [HACKERS] Remove superuser() checks from pgstattuple
On 2016-09-04 11:55:06 -0400, Tom Lane wrote: > [ warning, thread hijack ahead ] > > Stephen Frost writes: > > * Peter Eisentraut (peter.eisentr...@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
Re: [HACKERS] Remove superuser() checks from pgstattuple
Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: > [ warning, thread hijack ahead ] quite. > Stephen Frost writes: > > * Peter Eisentraut (peter.eisentr...@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 signature.asc Description: Digital signature
Re: [HACKERS] Remove superuser() checks from pgstattuple
[ warning, thread hijack ahead ] Stephen Frost writes: > * Peter Eisentraut (peter.eisentr...@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
Re: [HACKERS] Remove superuser() checks from pgstattuple
* Peter Eisentraut (peter.eisentr...@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 signature.asc Description: Digital signature
Re: [HACKERS] 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
[HACKERS] Remove superuser() checks from pgstattuple
Greetings,
Attached is an rebased and updated patch to remove the explicit
superuser() checks from the contrib extension pgstattuple, in favor of
using the GRANT system to control access, and give the admin flexibility
to GRANT access to this function without having to write wrapper
functions, similar to what was been done with the backend functions.
This, naturally, REVOKE's EXECUTE from public on installation for these
functions.
Prior discussion was on this thread:
20160408214511.gq10...@tamriel.snowman.net
but it seemed prudent to open a new thread, to avoid anyone missing that
this isn't about default roles (which was the subject of that thread).
Thanks!
Stephen
From b4f04c81c88c126c470a7afc74ea9e4f860be412 Mon Sep 17 00:00:00 2001
From: Stephen Frost
Date: Tue, 23 Aug 2016 17:13:09 -0400
Subject: [PATCH] Remove superuser checks in pgstattuple
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
(and to REVOKE EXECUTE rights on those functions from PUBLIC).
Approach suggested by Noah.
---
contrib/pgstattuple/Makefile | 7 +-
contrib/pgstattuple/pgstatapprox.c| 35 ++--
contrib/pgstattuple/pgstatindex.c | 108 +++--
contrib/pgstattuple/pgstattuple--1.4--1.5.sql | 111 ++
contrib/pgstattuple/pgstattuple--1.4.sql | 95 --
contrib/pgstattuple/pgstattuple--1.5.sql | 111 ++
contrib/pgstattuple/pgstattuple.c | 36 +
contrib/pgstattuple/pgstattuple.control | 2 +-
8 files changed, 395 insertions(+), 110 deletions(-)
create mode 100644 contrib/pgstattuple/pgstattuple--1.4--1.5.sql
delete mode 100644 contrib/pgstattuple/pgstattuple--1.4.sql
create mode 100644 contrib/pgstattuple/pgstattuple--1.5.sql
diff --git a/contrib/pgstattuple/Makefile b/contrib/pgstattuple/Makefile
index e732680..dc5ddba 100644
--- a/contrib/pgstattuple/Makefile
+++ b/contrib/pgstattuple/Makefile
@@ -4,9 +4,10 @@ MODULE_big = pgstattuple
OBJS = pgstattuple.o pgstatindex.o pgstatapprox.o $(WIN32RES)
EXTENSION = pgstattuple
-DATA = pgstattuple--1.4.sql pgstattuple--1.3--1.4.sql \
- pgstattuple--1.2--1.3.sql pgstattuple--1.1--1.2.sql \
- pgstattuple--1.0--1.1.sql pgstattuple--unpackaged--1.0.sql
+DATA = pgstattuple--1.5.sql pgstattuple--1.4--1.5.sql \
+ pgstattuple--1.3--1.4.sql pgstattuple--1.2--1.3.sql \
+ pgstattuple--1.1--1.2.sql pgstattuple--1.0--1.1.sql \
+ pgstattuple--unpackaged--1.0.sql
PGFILEDESC = "pgstattuple - tuple-level statistics"
REGRESS = pgstattuple
diff --git a/contrib/pgstattuple/pgstatapprox.c b/contrib/pgstattuple/pgstatapprox.c
index a49ff54..11bae14 100644
--- a/contrib/pgstattuple/pgstatapprox.c
+++ b/contrib/pgstattuple/pgstatapprox.c
@@ -29,6 +29,9 @@
#include "commands/vacuum.h"
PG_FUNCTION_INFO_V1(pgstattuple_approx);
+PG_FUNCTION_INFO_V1(pgstattuple_approx_v1_5);
+
+Datum pgstattuple_approx_internal(Oid relid, FunctionCallInfo fcinfo);
typedef struct output_type
{
@@ -209,6 +212,33 @@ Datum
pgstattuple_approx(PG_FUNCTION_ARGS)
{
Oid relid = PG_GETARG_OID(0);
+
+ if (!superuser())
+ ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser to use pgstattuple functions";
+
+ PG_RETURN_DATUM(pgstattuple_approx_internal(relid, fcinfo));
+}
+
+/*
+ * As of pgstattuple version 1.5, we no longer need to check if the user
+ * is a superuser because we REVOKE EXECUTE on the function from PUBLIC.
+ * Users can then grant access to it based on their policies.
+ *
+ * Otherwise identical to pgstattuple_approx (above).
+ */
+Datum
+pgstattuple_approx_v1_5(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+
+ PG_RETURN_DATUM(pgstattuple_approx_internal(relid, fcinfo));
+}
+
+Datum
+pgstattuple_approx_internal(Oid relid, FunctionCallInfo fcinfo)
+{
Relation rel;
output_type stat = {0};
TupleDesc tupdesc;
@@ -217,11 +247,6 @@ pgstattuple_approx(PG_FUNCTION_ARGS)
HeapTuple ret;
int i = 0;
- if (!superuser())
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- (errmsg("must be superuser to use pgstattuple functions";
-
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 6084589..753e52d 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -54,6 +54,14 @@ PG_FUNCTION_INFO_V1(pg_relpages);
PG_FUNCTION_INFO_V1(pg_relpagesbyid);
PG_F
