Обсуждение: 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.GQ10850@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
Вложения
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
* 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
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
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
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
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
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.
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
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_length(*best_path) ||
+ (list_length(path) == list_length(*best_path) &&
+ strcmp(evi_start->name, evi1->name) < 0))
+ {
+ evi_start = evi1;
+ *best_path = path;
+ }
+ }
+
+ return evi_start;
+ }
+
+ /*
* CREATE EXTENSION worker
*
* When CASCADE is specified, CreateExtensionInternal() recurses if required
*************** CreateExtensionInternal(CreateExtensionS
*** 1264,1274 ****
check_valid_version_name(versionName);
/*
! * Determine the (unpackaged) version to update from, if any, and then
! * figure out what sequence of update scripts we need to apply.
*/
if (d_old_version && d_old_version->arg)
{
oldVersionName = strVal(d_old_version->arg);
check_valid_version_name(oldVersionName);
--- 1331,1348 ----
check_valid_version_name(versionName);
/*
! * Figure out which script(s) we need to run to install the desired
! * version of the extension. If we do not have a script that directly
! * does what is needed, we try to find a sequence of update scripts that
! * will get us there.
*/
if (d_old_version && d_old_version->arg)
{
+ /*
+ * "FROM old_version" was specified, indicating that we're trying to
+ * update from some unpackaged version of the extension. Locate a
+ * series of update scripts that will do it.
+ */
oldVersionName = strVal(d_old_version->arg);
check_valid_version_name(oldVersionName);
*************** CreateExtensionInternal(CreateExtensionS
*** 1304,1311 ****
}
else
{
oldVersionName = NULL;
! updateVersions = NIL;
}
/*
--- 1378,1438 ----
}
else
{
+ /*
+ * 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;
!
! 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;
! }
! 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)));
!
! /* Otherwise, install best starting point and then upgrade */
! versionName = evi_start->name;
! }
! }
}
/*
*************** get_available_versions_for_extension(Ext
*** 1910,1952 ****
Tuplestorestate *tupstore,
TupleDesc tupdesc)
{
! int extnamelen = strlen(pcontrol->name);
! char *location;
! DIR *dir;
! struct dirent *de;
! location = get_extension_script_directory(pcontrol);
! dir = AllocateDir(location);
! /* Note this will fail if script directory doesn't exist */
! while ((de = ReadDir(dir, location)) != NULL)
{
ExtensionControlFile *control;
- char *vername;
Datum values[7];
bool nulls[7];
! /* must be a .sql file ... */
! if (!is_extension_script_filename(de->d_name))
! continue;
!
! /* ... matching extension name followed by separator */
! if (strncmp(de->d_name, pcontrol->name, extnamelen) != 0 ||
! de->d_name[extnamelen] != '-' ||
! de->d_name[extnamelen + 1] != '-')
! continue;
!
! /* extract version name from 'extname--something.sql' filename */
! vername = pstrdup(de->d_name + extnamelen + 2);
! *strrchr(vername, '.') = '\0';
!
! /* ignore it if it's an update script */
! if (strstr(vername, "--"))
continue;
/*
* Fetch parameters for specific version (pcontrol is not changed)
*/
! control = read_extension_aux_control_file(pcontrol, vername);
memset(values, 0, sizeof(values));
memset(nulls, 0, sizeof(nulls));
--- 2037,2064 ----
Tuplestorestate *tupstore,
TupleDesc tupdesc)
{
! List *evi_list;
! ListCell *lc;
! /* Extract the version update graph from the script directory */
! evi_list = get_ext_ver_list(pcontrol);
!
! /* For each installable version ... */
! foreach(lc, evi_list)
{
+ ExtensionVersionInfo *evi = (ExtensionVersionInfo *) lfirst(lc);
ExtensionControlFile *control;
Datum values[7];
bool nulls[7];
+ ListCell *lc2;
! if (!evi->installable)
continue;
/*
* Fetch parameters for specific version (pcontrol is not changed)
*/
! control = read_extension_aux_control_file(pcontrol, evi->name);
memset(values, 0, sizeof(values));
memset(nulls, 0, sizeof(nulls));
*************** get_available_versions_for_extension(Ext
*** 1955,1961 ****
values[0] = DirectFunctionCall1(namein,
CStringGetDatum(control->name));
/* version */
! values[1] = CStringGetTextDatum(vername);
/* superuser */
values[2] = BoolGetDatum(control->superuser);
/* relocatable */
--- 2067,2073 ----
values[0] = DirectFunctionCall1(namein,
CStringGetDatum(control->name));
/* version */
! values[1] = CStringGetTextDatum(evi->name);
/* superuser */
values[2] = BoolGetDatum(control->superuser);
/* relocatable */
*************** get_available_versions_for_extension(Ext
*** 1998,2006 ****
values[6] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
- }
! FreeDir(dir);
}
/*
--- 2110,2137 ----
values[6] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
! /*
! * Find all non-directly-installable versions that would be installed
! * starting from this version, and report them with the same
! * control-file parameters.
! */
! foreach(lc2, evi_list)
! {
! ExtensionVersionInfo *evi2 = (ExtensionVersionInfo *) lfirst(lc2);
! List *best_path;
!
! if (evi2->installable)
! continue;
! if (find_install_path(evi_list, evi2, &best_path) == evi)
! {
! /* replace only the version column */
! values[1] = CStringGetTextDatum(evi2->name);
!
! tuplestore_putvalues(tupstore, tupdesc, values, nulls);
! }
! }
! }
}
/*
*************** pg_extension_update_paths(PG_FUNCTION_AR
*** 2072,2078 ****
continue;
/* Find shortest path from evi1 to evi2 */
! path = find_update_path(evi_list, evi1, evi2, true);
/* Emit result row */
memset(values, 0, sizeof(values));
--- 2203,2209 ----
continue;
/* Find shortest path from evi1 to evi2 */
! path = find_update_path(evi_list, evi1, evi2, false, true);
/* Emit result row */
memset(values, 0, sizeof(values));
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
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
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 ****
--- 1111,1117 ----
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 ****
--- 1120,1127 ----
/* 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 ****
--- 1148,1156 ----
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 ****
--- 1187,1249 ----
}
/*
+ * 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_length(*best_path) ||
+ (list_length(path) == list_length(*best_path) &&
+ strcmp(evi_start->name, evi1->name) < 0))
+ {
+ evi_start = evi1;
+ *best_path = path;
+ }
+ }
+
+ return evi_start;
+ }
+
+ /*
* CREATE EXTENSION worker
*
* When CASCADE is specified, CreateExtensionInternal() recurses if required
*************** find_update_path(List *evi_list,
*** 1175,1191 ****
* installed, allowing us to error out if we recurse to one of those.
*/
static ObjectAddress
! CreateExtensionInternal(ParseState *pstate, CreateExtensionStmt *stmt, List *parents)
{
! DefElem *d_schema = NULL;
! DefElem *d_new_version = NULL;
! DefElem *d_old_version = NULL;
! DefElem *d_cascade = NULL;
! char *schemaName = NULL;
Oid schemaOid = InvalidOid;
- char *versionName;
- char *oldVersionName;
- bool cascade = false;
Oid extowner = GetUserId();
ExtensionControlFile *pcontrol;
ExtensionControlFile *control;
--- 1252,1267 ----
* installed, allowing us to error out if we recurse to one of those.
*/
static ObjectAddress
! CreateExtensionInternal(char *extensionName,
! char *schemaName,
! char *versionName,
! char *oldVersionName,
! bool cascade,
! List *parents,
! bool is_create)
{
! char *origSchemaName = schemaName;
Oid schemaOid = InvalidOid;
Oid extowner = GetUserId();
ExtensionControlFile *pcontrol;
ExtensionControlFile *control;
*************** CreateExtensionInternal(ParseState *psta
*** 1193,1279 ****
List *requiredExtensions;
List *requiredSchemas;
Oid extensionOid;
- ListCell *lc;
ObjectAddress address;
/*
* Read the primary control file. Note we assume that it does not contain
* any non-ASCII data, so there is no need to worry about encoding at this
* point.
*/
! pcontrol = read_extension_control_file(stmt->extname);
!
! /*
! * Read the statement option list
! */
! foreach(lc, stmt->options)
! {
! DefElem *defel = (DefElem *) lfirst(lc);
!
! if (strcmp(defel->defname, "schema") == 0)
! {
! if (d_schema)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("conflicting or redundant options"),
! parser_errposition(pstate, defel->location)));
! d_schema = defel;
! }
! else if (strcmp(defel->defname, "new_version") == 0)
! {
! if (d_new_version)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("conflicting or redundant options"),
! parser_errposition(pstate, defel->location)));
! d_new_version = defel;
! }
! else if (strcmp(defel->defname, "old_version") == 0)
! {
! if (d_old_version)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("conflicting or redundant options"),
! parser_errposition(pstate, defel->location)));
! d_old_version = defel;
! }
! else if (strcmp(defel->defname, "cascade") == 0)
! {
! if (d_cascade)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("conflicting or redundant options"),
! parser_errposition(pstate, defel->location)));
! d_cascade = defel;
! cascade = defGetBoolean(d_cascade);
! }
! else
! elog(ERROR, "unrecognized option: %s", defel->defname);
! }
/*
* Determine the version to install
*/
! if (d_new_version && d_new_version->arg)
! versionName = strVal(d_new_version->arg);
! else if (pcontrol->default_version)
! versionName = pcontrol->default_version;
! else
{
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg("version to install must be specified")));
! versionName = NULL; /* keep compiler quiet */
}
check_valid_version_name(versionName);
/*
! * Determine the (unpackaged) version to update from, if any, and then
! * figure out what sequence of update scripts we need to apply.
*/
! if (d_old_version && d_old_version->arg)
{
! oldVersionName = strVal(d_old_version->arg);
check_valid_version_name(oldVersionName);
if (strcmp(oldVersionName, versionName) == 0)
--- 1269,1311 ----
List *requiredExtensions;
List *requiredSchemas;
Oid extensionOid;
ObjectAddress address;
+ ListCell *lc;
/*
* Read the primary control file. Note we assume that it does not contain
* any non-ASCII data, so there is no need to worry about encoding at this
* point.
*/
! pcontrol = read_extension_control_file(extensionName);
/*
* Determine the version to install
*/
! if (versionName == NULL)
{
! if (pcontrol->default_version)
! versionName = pcontrol->default_version;
! else
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg("version to install must be specified")));
}
check_valid_version_name(versionName);
/*
! * Figure out which script(s) we need to run to install the desired
! * version of the extension. If we do not have a script that directly
! * does what is needed, we try to find a sequence of update scripts that
! * will get us there.
*/
! if (oldVersionName)
{
! /*
! * "FROM old_version" was specified, indicating that we're trying to
! * update from some unpackaged version of the extension. Locate a
! * series of update scripts that will do it.
! */
check_valid_version_name(oldVersionName);
if (strcmp(oldVersionName, versionName) == 0)
*************** CreateExtensionInternal(ParseState *psta
*** 1308,1315 ****
}
else
{
oldVersionName = NULL;
! updateVersions = NIL;
}
/*
--- 1340,1400 ----
}
else
{
+ /*
+ * 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;
!
! 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;
! }
! 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)));
!
! /* Otherwise, install best starting point and then upgrade */
! versionName = evi_start->name;
! }
! }
}
/*
*************** CreateExtensionInternal(ParseState *psta
*** 1320,1332 ****
/*
* Determine the target schema to install the extension into
*/
! if (d_schema && d_schema->arg)
{
- /*
- * User given schema, CREATE EXTENSION ... WITH SCHEMA ...
- */
- schemaName = strVal(d_schema->arg);
-
/* If the user is giving us the schema name, it must exist already. */
schemaOid = get_namespace_oid(schemaName, false);
}
--- 1405,1412 ----
/*
* Determine the target schema to install the extension into
*/
! if (schemaName)
{
/* If the user is giving us the schema name, it must exist already. */
schemaOid = get_namespace_oid(schemaName, false);
}
*************** CreateExtensionInternal(ParseState *psta
*** 1374,1380 ****
else if (!OidIsValid(schemaOid))
{
/*
! * Neither user nor author of the extension specified schema, use the
* current default creation namespace, which is the first explicit
* entry in the search_path.
*/
--- 1454,1460 ----
else if (!OidIsValid(schemaOid))
{
/*
! * Neither user nor author of the extension specified schema; use the
* current default creation namespace, which is the first explicit
* entry in the search_path.
*/
*************** CreateExtensionInternal(ParseState *psta
*** 1415,1480 ****
Oid reqext;
Oid reqschema;
! reqext = get_extension_oid(curreq, true);
! if (!OidIsValid(reqext))
! {
! if (cascade)
! {
! /* Must install it. */
! CreateExtensionStmt *ces;
! ListCell *lc2;
! ObjectAddress addr;
! List *cascade_parents;
!
! /* Check extension name validity before trying to cascade. */
! check_valid_extension_name(curreq);
!
! /* Check for cyclic dependency between extensions. */
! foreach(lc2, parents)
! {
! char *pname = (char *) lfirst(lc2);
!
! if (strcmp(pname, curreq) == 0)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_RECURSION),
! errmsg("cyclic dependency detected between extensions \"%s\" and \"%s\"",
! curreq, stmt->extname)));
! }
!
! ereport(NOTICE,
! (errmsg("installing required extension \"%s\"",
! curreq)));
!
! /* Build a CREATE EXTENSION statement to pass down. */
! ces = makeNode(CreateExtensionStmt);
! ces->extname = curreq;
! ces->if_not_exists = false;
!
! /* Propagate the CASCADE option. */
! ces->options = list_make1(d_cascade);
!
! /* Propagate the SCHEMA option if given. */
! if (d_schema && d_schema->arg)
! ces->options = lappend(ces->options, d_schema);
!
! /* Add current extension to list of parents to pass down. */
! cascade_parents =
! lappend(list_copy(parents), stmt->extname);
!
! /* Create the required extension. */
! addr = CreateExtensionInternal(pstate, ces, cascade_parents);
!
! /* Get its newly-assigned OID. */
! reqext = addr.objectId;
! }
! else
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_OBJECT),
! errmsg("required extension \"%s\" is not installed",
! curreq),
! errhint("Use CREATE EXTENSION ... CASCADE to install required extensions too.")));
! }
!
reqschema = get_extension_schema(reqext);
requiredExtensions = lappend_oid(requiredExtensions, reqext);
requiredSchemas = lappend_oid(requiredSchemas, reqschema);
--- 1495,1506 ----
Oid reqext;
Oid reqschema;
! reqext = get_required_extension(curreq,
! extensionName,
! origSchemaName,
! cascade,
! parents,
! is_create);
reqschema = get_extension_schema(reqext);
requiredExtensions = lappend_oid(requiredExtensions, reqext);
requiredSchemas = lappend_oid(requiredSchemas, reqschema);
*************** CreateExtensionInternal(ParseState *psta
*** 1510,1526 ****
* though a series of ALTER EXTENSION UPDATE commands were given
*/
ApplyExtensionUpdates(extensionOid, pcontrol,
! versionName, updateVersions);
return address;
}
/*
* CREATE EXTENSION
*/
ObjectAddress
CreateExtension(ParseState *pstate, CreateExtensionStmt *stmt)
{
/* Check extension name validity before any filesystem access */
check_valid_extension_name(stmt->extname);
--- 1536,1635 ----
* though a series of ALTER EXTENSION UPDATE commands were given
*/
ApplyExtensionUpdates(extensionOid, pcontrol,
! versionName, updateVersions,
! origSchemaName, cascade, is_create);
return address;
}
/*
+ * Get the OID of an extension listed in "requires", possibly creating it.
+ */
+ static Oid
+ get_required_extension(char *reqExtensionName,
+ char *extensionName,
+ char *origSchemaName,
+ bool cascade,
+ List *parents,
+ bool is_create)
+ {
+ Oid reqExtensionOid;
+
+ reqExtensionOid = get_extension_oid(reqExtensionName, true);
+ if (!OidIsValid(reqExtensionOid))
+ {
+ if (cascade)
+ {
+ /* Must install it. */
+ ObjectAddress addr;
+ List *cascade_parents;
+ ListCell *lc;
+
+ /* Check extension name validity before trying to cascade. */
+ check_valid_extension_name(reqExtensionName);
+
+ /* Check for cyclic dependency between extensions. */
+ foreach(lc, parents)
+ {
+ char *pname = (char *) lfirst(lc);
+
+ if (strcmp(pname, reqExtensionName) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_RECURSION),
+ errmsg("cyclic dependency detected between extensions \"%s\" and \"%s\"",
+ reqExtensionName, extensionName)));
+ }
+
+ ereport(NOTICE,
+ (errmsg("installing required extension \"%s\"",
+ reqExtensionName)));
+
+ /* Add current extension to list of parents to pass down. */
+ cascade_parents = lappend(list_copy(parents), extensionName);
+
+ /*
+ * Create the required extension. We propagate the SCHEMA option
+ * if any, and CASCADE, but no other options.
+ */
+ addr = CreateExtensionInternal(reqExtensionName,
+ origSchemaName,
+ NULL,
+ NULL,
+ cascade,
+ cascade_parents,
+ is_create);
+
+ /* Get its newly-assigned OID. */
+ reqExtensionOid = addr.objectId;
+ }
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("required extension \"%s\" is not installed",
+ reqExtensionName),
+ is_create ?
+ errhint("Use CREATE EXTENSION ... CASCADE to install required extensions too.") : 0));
+ }
+
+ return reqExtensionOid;
+ }
+
+ /*
* CREATE EXTENSION
*/
ObjectAddress
CreateExtension(ParseState *pstate, CreateExtensionStmt *stmt)
{
+ DefElem *d_schema = NULL;
+ DefElem *d_new_version = NULL;
+ DefElem *d_old_version = NULL;
+ DefElem *d_cascade = NULL;
+ char *schemaName = NULL;
+ char *versionName = NULL;
+ char *oldVersionName = NULL;
+ bool cascade = false;
+ ListCell *lc;
+
/* Check extension name validity before any filesystem access */
check_valid_extension_name(stmt->extname);
*************** CreateExtension(ParseState *pstate, Crea
*** 1556,1563 ****
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("nested CREATE EXTENSION is not supported")));
! /* Finally create the extension. */
! return CreateExtensionInternal(pstate, stmt, NIL);
}
/*
--- 1665,1727 ----
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("nested CREATE EXTENSION is not supported")));
! /* Deconstruct the statement option list */
! foreach(lc, stmt->options)
! {
! DefElem *defel = (DefElem *) lfirst(lc);
!
! if (strcmp(defel->defname, "schema") == 0)
! {
! if (d_schema)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("conflicting or redundant options"),
! parser_errposition(pstate, defel->location)));
! d_schema = defel;
! schemaName = defGetString(d_schema);
! }
! else if (strcmp(defel->defname, "new_version") == 0)
! {
! if (d_new_version)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("conflicting or redundant options"),
! parser_errposition(pstate, defel->location)));
! d_new_version = defel;
! versionName = defGetString(d_new_version);
! }
! else if (strcmp(defel->defname, "old_version") == 0)
! {
! if (d_old_version)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("conflicting or redundant options"),
! parser_errposition(pstate, defel->location)));
! d_old_version = defel;
! oldVersionName = defGetString(d_old_version);
! }
! else if (strcmp(defel->defname, "cascade") == 0)
! {
! if (d_cascade)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("conflicting or redundant options"),
! parser_errposition(pstate, defel->location)));
! d_cascade = defel;
! cascade = defGetBoolean(d_cascade);
! }
! else
! elog(ERROR, "unrecognized option: %s", defel->defname);
! }
!
! /* Call CreateExtensionInternal to do the real work. */
! return CreateExtensionInternal(stmt->extname,
! schemaName,
! versionName,
! oldVersionName,
! cascade,
! NIL,
! true);
}
/*
*************** get_available_versions_for_extension(Ext
*** 1914,1956 ****
Tuplestorestate *tupstore,
TupleDesc tupdesc)
{
! int extnamelen = strlen(pcontrol->name);
! char *location;
! DIR *dir;
! struct dirent *de;
! location = get_extension_script_directory(pcontrol);
! dir = AllocateDir(location);
! /* Note this will fail if script directory doesn't exist */
! while ((de = ReadDir(dir, location)) != NULL)
{
ExtensionControlFile *control;
- char *vername;
Datum values[7];
bool nulls[7];
! /* must be a .sql file ... */
! if (!is_extension_script_filename(de->d_name))
! continue;
!
! /* ... matching extension name followed by separator */
! if (strncmp(de->d_name, pcontrol->name, extnamelen) != 0 ||
! de->d_name[extnamelen] != '-' ||
! de->d_name[extnamelen + 1] != '-')
! continue;
!
! /* extract version name from 'extname--something.sql' filename */
! vername = pstrdup(de->d_name + extnamelen + 2);
! *strrchr(vername, '.') = '\0';
!
! /* ignore it if it's an update script */
! if (strstr(vername, "--"))
continue;
/*
* Fetch parameters for specific version (pcontrol is not changed)
*/
! control = read_extension_aux_control_file(pcontrol, vername);
memset(values, 0, sizeof(values));
memset(nulls, 0, sizeof(nulls));
--- 2078,2105 ----
Tuplestorestate *tupstore,
TupleDesc tupdesc)
{
! List *evi_list;
! ListCell *lc;
! /* Extract the version update graph from the script directory */
! evi_list = get_ext_ver_list(pcontrol);
!
! /* For each installable version ... */
! foreach(lc, evi_list)
{
+ ExtensionVersionInfo *evi = (ExtensionVersionInfo *) lfirst(lc);
ExtensionControlFile *control;
Datum values[7];
bool nulls[7];
+ ListCell *lc2;
! if (!evi->installable)
continue;
/*
* Fetch parameters for specific version (pcontrol is not changed)
*/
! control = read_extension_aux_control_file(pcontrol, evi->name);
memset(values, 0, sizeof(values));
memset(nulls, 0, sizeof(nulls));
*************** get_available_versions_for_extension(Ext
*** 1959,1965 ****
values[0] = DirectFunctionCall1(namein,
CStringGetDatum(control->name));
/* version */
! values[1] = CStringGetTextDatum(vername);
/* superuser */
values[2] = BoolGetDatum(control->superuser);
/* relocatable */
--- 2108,2114 ----
values[0] = DirectFunctionCall1(namein,
CStringGetDatum(control->name));
/* version */
! values[1] = CStringGetTextDatum(evi->name);
/* superuser */
values[2] = BoolGetDatum(control->superuser);
/* relocatable */
*************** get_available_versions_for_extension(Ext
*** 1974,2000 ****
if (control->requires == NIL)
nulls[5] = true;
else
! {
! Datum *datums;
! int ndatums;
! ArrayType *a;
! ListCell *lc;
!
! ndatums = list_length(control->requires);
! datums = (Datum *) palloc(ndatums * sizeof(Datum));
! ndatums = 0;
! foreach(lc, control->requires)
! {
! char *curreq = (char *) lfirst(lc);
!
! datums[ndatums++] =
! DirectFunctionCall1(namein, CStringGetDatum(curreq));
! }
! a = construct_array(datums, ndatums,
! NAMEOID,
! NAMEDATALEN, false, 'c');
! values[5] = PointerGetDatum(a);
! }
/* comment */
if (control->comment == NULL)
nulls[6] = true;
--- 2123,2129 ----
if (control->requires == NIL)
nulls[5] = true;
else
! values[5] = convert_requires_to_datum(control->requires);
/* comment */
if (control->comment == NULL)
nulls[6] = true;
*************** get_available_versions_for_extension(Ext
*** 2002,2010 ****
values[6] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
! FreeDir(dir);
}
/*
--- 2131,2205 ----
values[6] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
+
+ /*
+ * Find all non-directly-installable versions that would be installed
+ * starting from this version, and report them, inheriting the
+ * appropriate parameters from this version.
+ */
+ foreach(lc2, evi_list)
+ {
+ ExtensionVersionInfo *evi2 = (ExtensionVersionInfo *) lfirst(lc2);
+ List *best_path;
+
+ if (evi2->installable)
+ continue;
+ if (find_install_path(evi_list, evi2, &best_path) == evi)
+ {
+ /*
+ * Fetch parameters for this version (pcontrol is not changed)
+ */
+ control = read_extension_aux_control_file(pcontrol, evi2->name);
+
+ /* name stays the same */
+ /* version */
+ values[1] = CStringGetTextDatum(evi2->name);
+ /* superuser */
+ values[2] = BoolGetDatum(control->superuser);
+ /* relocatable */
+ values[3] = BoolGetDatum(control->relocatable);
+ /* schema stays the same */
+ /* requires */
+ if (control->requires == NIL)
+ nulls[5] = true;
+ else
+ {
+ values[5] = convert_requires_to_datum(control->requires);
+ nulls[5] = false;
+ }
+ /* comment stays the same */
+
+ tuplestore_putvalues(tupstore, tupdesc, values, nulls);
+ }
+ }
}
+ }
! /*
! * Convert a list of extension names to a name[] Datum
! */
! static Datum
! convert_requires_to_datum(List *requires)
! {
! Datum *datums;
! int ndatums;
! ArrayType *a;
! ListCell *lc;
!
! ndatums = list_length(requires);
! datums = (Datum *) palloc(ndatums * sizeof(Datum));
! ndatums = 0;
! foreach(lc, requires)
! {
! char *curreq = (char *) lfirst(lc);
!
! datums[ndatums++] =
! DirectFunctionCall1(namein, CStringGetDatum(curreq));
! }
! a = construct_array(datums, ndatums,
! NAMEOID,
! NAMEDATALEN, false, 'c');
! return PointerGetDatum(a);
}
/*
*************** pg_extension_update_paths(PG_FUNCTION_AR
*** 2076,2082 ****
continue;
/* Find shortest path from evi1 to evi2 */
! path = find_update_path(evi_list, evi1, evi2, true);
/* Emit result row */
memset(values, 0, sizeof(values));
--- 2271,2277 ----
continue;
/* Find shortest path from evi1 to evi2 */
! path = find_update_path(evi_list, evi1, evi2, false, true);
/* Emit result row */
memset(values, 0, sizeof(values));
*************** ExecAlterExtensionStmt(ParseState *pstat
*** 2808,2814 ****
* time
*/
ApplyExtensionUpdates(extensionOid, control,
! oldVersionName, updateVersions);
ObjectAddressSet(address, ExtensionRelationId, extensionOid);
--- 3003,3010 ----
* time
*/
ApplyExtensionUpdates(extensionOid, control,
! oldVersionName, updateVersions,
! NULL, false, false);
ObjectAddressSet(address, ExtensionRelationId, extensionOid);
*************** static void
*** 2827,2833 ****
ApplyExtensionUpdates(Oid extensionOid,
ExtensionControlFile *pcontrol,
const char *initialVersion,
! List *updateVersions)
{
const char *oldVersionName = initialVersion;
ListCell *lcv;
--- 3023,3032 ----
ApplyExtensionUpdates(Oid extensionOid,
ExtensionControlFile *pcontrol,
const char *initialVersion,
! List *updateVersions,
! char *origSchemaName,
! bool cascade,
! bool is_create)
{
const char *oldVersionName = initialVersion;
ListCell *lcv;
*************** ApplyExtensionUpdates(Oid extensionOid,
*** 2906,2913 ****
heap_close(extRel, RowExclusiveLock);
/*
! * Look up the prerequisite extensions for this version, and build
! * lists of their OIDs and the OIDs of their target schemas.
*/
requiredExtensions = NIL;
requiredSchemas = NIL;
--- 3105,3113 ----
heap_close(extRel, RowExclusiveLock);
/*
! * Look up the prerequisite extensions for this version, install them
! * if necessary, and build lists of their OIDs and the OIDs of their
! * target schemas.
*/
requiredExtensions = NIL;
requiredSchemas = NIL;
*************** ApplyExtensionUpdates(Oid extensionOid,
*** 2917,2932 ****
Oid reqext;
Oid reqschema;
! /*
! * We intentionally don't use get_extension_oid's default error
! * message here, because it would be confusing in this context.
! */
! reqext = get_extension_oid(curreq, true);
! if (!OidIsValid(reqext))
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_OBJECT),
! errmsg("required extension \"%s\" is not installed",
! curreq)));
reqschema = get_extension_schema(reqext);
requiredExtensions = lappend_oid(requiredExtensions, reqext);
requiredSchemas = lappend_oid(requiredSchemas, reqschema);
--- 3117,3128 ----
Oid reqext;
Oid reqschema;
! reqext = get_required_extension(curreq,
! control->name,
! origSchemaName,
! cascade,
! NIL,
! is_create);
reqschema = get_extension_schema(reqext);
requiredExtensions = lappend_oid(requiredExtensions, reqext);
requiredSchemas = lappend_oid(requiredSchemas, reqschema);
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 ----
</para>
</sect2>
+ <sect2>
+ <title>Installing Extensions using Update Scripts</title>
+
+ <para>
+ 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 <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
+ installed directly, only by installing <literal>1.0</> and then updating.
+ Now, <command>CREATE EXTENSION</> can do that automatically —
+ for example, if only the script
+ files <filename>foo--1.0.sql</>, <filename>foo--1.0--1.1.sql</>,
+ and <filename>foo--1.1--1.2.sql</> are available then a request to
+ install version <literal>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.
+ </para>
+
+ <para>
+ 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 <filename>foo--1.0.control</> specifies <literal>requires
+ = 'bar'</> but <literal>foo</>'s other control files do not, the
+ extension's dependency on <literal>bar</> will be dropped when updating
+ from <literal>1.0</> to another version.
+ </para>
+ </sect2>
+
<sect2 id="extend-extensions-example">
<title>Extension Example</title>
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
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
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
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
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
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
Вложения
On Tue, Sep 27, 2016 at 4:43 AM, Stephen Frost <sfrost@snowman.net> wrote: > * 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. 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
Michael, * Michael Paquier (michael.paquier@gmail.com) wrote: > On Tue, Sep 27, 2016 at 4:43 AM, Stephen Frost <sfrost@snowman.net> wrote: > > * 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. > > 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