public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v9 2/8] Implement CLUSTER of partitioned table..
12+ messages / 6 participants
[nested] [flat]
* [PATCH v9 2/8] Implement CLUSTER of partitioned table..
@ 2020-06-07 21:58 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Justin Pryzby @ 2020-06-07 21:58 UTC (permalink / raw)
This requires either specification of a partitioned index on which to cluster,
or that an partitioned index was previously set clustered.
VACUUM (including vacuum full) has recursed into partition hierarchies since
partitions were introduced in v10 (3c3bb9933).
---
doc/src/sgml/ref/cluster.sgml | 7 ++
src/backend/commands/cluster.c | 173 +++++++++++++++++++-------
src/bin/psql/tab-complete.c | 1 +
src/include/commands/cluster.h | 1 +
src/test/regress/expected/cluster.out | 58 ++++++++-
src/test/regress/sql/cluster.sql | 24 +++-
6 files changed, 209 insertions(+), 55 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 0d9720fd8e..17509b35eb 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -197,6 +197,13 @@ CLUSTER [VERBOSE]
in the <structname>pg_stat_progress_cluster</structname> view. See
<xref linkend="cluster-progress-reporting"/> for details.
</para>
+
+ <para>
+ Clustering a partitioned table clusters each of its partitions using the
+ partition of the specified partitioned index or (if not specified) the
+ partitioned index marked as clustered.
+ </para>
+
</refsect1>
<refsect1>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 096a06f7b3..0c08ac56dc 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -32,7 +32,9 @@
#include "catalog/index.h"
#include "catalog/namespace.h"
#include "catalog/objectaccess.h"
+#include "catalog/partition.h"
#include "catalog/pg_am.h"
+#include "catalog/pg_inherits.h"
#include "catalog/toasting.h"
#include "commands/cluster.h"
#include "commands/defrem.h"
@@ -73,6 +75,9 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
static List *get_tables_to_cluster(MemoryContext cluster_context);
+static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
+ Oid indexOid);
+static void cluster_multiple_rels(List *rvs, int options);
/*---------------------------------------------------------------------------
@@ -135,7 +140,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
AccessExclusiveLock,
0,
RangeVarCallbackOwnsTable, NULL);
- rel = table_open(tableOid, NoLock);
+ rel = table_open(tableOid, ShareUpdateExclusiveLock);
/*
* Reject clustering a remote temp table ... their local buffer
@@ -146,14 +151,6 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot cluster temporary tables of other sessions")));
- /*
- * Reject clustering a partitioned table.
- */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot cluster a partitioned table")));
-
if (stmt->indexname == NULL)
{
ListCell *index;
@@ -189,10 +186,34 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
}
/* close relation, keep lock till commit */
- table_close(rel, NoLock);
+ table_close(rel, ShareUpdateExclusiveLock);
- /* Do the job. */
- cluster_rel(tableOid, indexOid, ¶ms);
+ if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+ {
+ /* Do the job. */
+ cluster_rel(tableOid, indexOid, ¶ms);
+ }
+ else
+ {
+ List *rvs;
+ MemoryContext cluster_context;
+
+ /* Refuse to hold strong locks in a user transaction */
+ PreventInTransactionBlock(isTopLevel, "CLUSTER");
+
+ cluster_context = AllocSetContextCreate(PortalContext,
+ "Cluster",
+ ALLOCSET_DEFAULT_SIZES);
+
+ rvs = get_tables_to_cluster_partitioned(cluster_context, indexOid);
+ cluster_multiple_rels(rvs, params.options);
+
+ /* Start a new transaction for the cleanup work. */
+ StartTransactionCommand();
+
+ /* Clean up working storage */
+ MemoryContextDelete(cluster_context);
+ }
}
else
{
@@ -202,7 +223,6 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
*/
MemoryContext cluster_context;
List *rvs;
- ListCell *rv;
/*
* We cannot run this form of CLUSTER inside a user transaction block;
@@ -225,28 +245,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
* cluster_context.
*/
rvs = get_tables_to_cluster(cluster_context);
-
- /* Commit to get out of starting transaction */
- PopActiveSnapshot();
- CommitTransactionCommand();
-
- /* Ok, now that we've got them all, cluster them one by one */
- foreach(rv, rvs)
- {
- RelToCluster *rvtc = (RelToCluster *) lfirst(rv);
- ClusterParams cluster_params = params;
-
- /* Start a new transaction for each relation. */
- StartTransactionCommand();
- /* functions in indexes may want a snapshot set */
- PushActiveSnapshot(GetTransactionSnapshot());
- /* Do the job. */
- cluster_params.options |= CLUOPT_RECHECK;
- cluster_rel(rvtc->tableOid, rvtc->indexOid,
- &cluster_params);
- PopActiveSnapshot();
- CommitTransactionCommand();
- }
+ cluster_multiple_rels(rvs, params.options | CLUOPT_RECHECK_ISCLUSTERED);
/* Start a new transaction for the cleanup work. */
StartTransactionCommand();
@@ -352,9 +351,10 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
}
/*
- * Check that the index is still the one with indisclustered set.
+ * Check that the index is still the one with indisclustered set, if needed.
*/
- if (!get_index_isclustered(indexOid))
+ if ((params->options & CLUOPT_RECHECK_ISCLUSTERED) != 0 &&
+ !get_index_isclustered(indexOid))
{
relation_close(OldHeap, AccessExclusiveLock);
pgstat_progress_end_command();
@@ -398,8 +398,13 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
/* Check heap and index are valid to cluster on */
if (OidIsValid(indexOid))
+ {
check_index_is_clusterable(OldHeap, indexOid, recheck, AccessExclusiveLock);
+ /* Mark the index as clustered */
+ mark_index_clustered(OldHeap, indexOid, true);
+ }
+
/*
* Quietly ignore the request if this is a materialized view which has not
* been populated from its query. No harm is done because there is no data
@@ -415,6 +420,14 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
return;
}
+ /* For a partitioned rel, we're done. */
+ if (!RELKIND_HAS_STORAGE(get_rel_relkind(tableOid)))
+ {
+ relation_close(OldHeap, AccessExclusiveLock);
+ pgstat_progress_end_command();
+ return;
+ }
+
/*
* All predicate locks on the tuples or pages are about to be made
* invalid, because we move tuples around. Promote them to relation
@@ -483,6 +496,9 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
* the worst consequence of following broken HOT chains would be that we
* might put recently-dead tuples out-of-order in the new table, and there
* is little harm in that.)
+ *
+ * This also refuses to cluster on an "incomplete" partitioned index
+ * created with "ON ONLY".
*/
if (!OldIndex->rd_index->indisvalid)
ereport(ERROR,
@@ -507,12 +523,6 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
Relation pg_index;
ListCell *index;
- /* Disallow applying to a partitioned table */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot mark index clustered in partitioned table")));
-
/*
* If the index is already marked clustered, no need to do anything.
*/
@@ -584,10 +594,6 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
TransactionId frozenXid;
MultiXactId cutoffMulti;
- /* Mark the correct index as clustered */
- if (OidIsValid(indexOid))
- mark_index_clustered(OldHeap, indexOid, true);
-
/* Remember info about rel before closing OldHeap */
relpersistence = OldHeap->rd_rel->relpersistence;
is_system_catalog = IsSystemRelation(OldHeap);
@@ -1582,3 +1588,76 @@ get_tables_to_cluster(MemoryContext cluster_context)
return rvs;
}
+
+/*
+ * Return a List of tables and associated index, where each index is a
+ * partition of the given index
+ */
+static List *
+get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
+{
+ List *inhoids;
+ ListCell *lc;
+ List *rvs = NIL;
+ MemoryContext old_context;
+
+ inhoids = find_all_inheritors(indexOid, NoLock, NULL);
+
+ /*
+ * We have to build the list in a different memory context so it will
+ * survive the cross-transaction processing
+ */
+ old_context = MemoryContextSwitchTo(cluster_context);
+
+ foreach(lc, inhoids)
+ {
+ Oid indexrelid = lfirst_oid(lc);
+ Oid relid = IndexGetRelation(indexrelid, false);
+ RelToCluster *rvtc;
+
+ /*
+ * Partitioned rels (including the top indexOid) are included,
+ * so as to be processed by cluster_rel, which calls
+ * check_index_is_clusterable() and mark_index_clustered().
+ */
+ rvtc = (RelToCluster *) palloc(sizeof(RelToCluster));
+ rvtc->tableOid = relid;
+ rvtc->indexOid = indexrelid;
+ rvs = lappend(rvs, rvtc);
+ }
+
+ MemoryContextSwitchTo(old_context);
+ return rvs;
+}
+
+/* Cluster each relation in a separate transaction */
+static void
+cluster_multiple_rels(List *rvs, int options)
+{
+ ListCell *lc;
+
+ /* Commit to get out of starting transaction */
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+
+ /* Ok, now that we've got them all, cluster them one by one */
+ foreach(lc, rvs)
+ {
+ RelToCluster *rvtc = (RelToCluster *) lfirst(lc);
+ ClusterParams cluster_params = { .options = options, };
+
+ /* Start a new transaction for each relation. */
+ StartTransactionCommand();
+
+ /* functions in indexes may want a snapshot set */
+ PushActiveSnapshot(GetTransactionSnapshot());
+
+ /* Do the job. */
+ cluster_params.options |= CLUOPT_RECHECK;
+ cluster_rel(rvtc->tableOid, rvtc->indexOid,
+ &cluster_params);
+
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+ }
+}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 9f0208ac49..53d585c4d3 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -588,6 +588,7 @@ static const SchemaQuery Query_for_list_of_clusterables = {
.catname = "pg_catalog.pg_class c",
.selcondition =
"c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
+ CppAsString2(RELKIND_PARTITIONED_TABLE) ", "
CppAsString2(RELKIND_MATVIEW) ")",
.viscondition = "pg_catalog.pg_table_is_visible(c.oid)",
.namespace = "c.relnamespace",
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index a941f2accd..c30ca01726 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -22,6 +22,7 @@
/* flag bits for ClusterParams->flags */
#define CLUOPT_RECHECK 0x01 /* recheck relation state */
#define CLUOPT_VERBOSE 0x02 /* print progress info */
+#define CLUOPT_RECHECK_ISCLUSTERED 0x04 /* recheck relation state for indisclustered */
/* options for CLUSTER */
typedef struct ClusterParams
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index bdae8fe00c..c74cfa88cc 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -439,14 +439,62 @@ select * from clstr_temp;
drop table clstr_temp;
RESET SESSION AUTHORIZATION;
--- Check that partitioned tables cannot be clustered
+-- Check that partitioned tables can be clustered
CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10);
+CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a);
+CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30);
+CREATE TABLE clstrpart3 PARTITION OF clstrpart DEFAULT PARTITION BY RANGE(a);
+CREATE TABLE clstrpart33 PARTITION OF clstrpart3 DEFAULT;
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+CREATE INDEX clstrpart_only_idx ON ONLY clstrpart (a);
+CLUSTER clstrpart USING clstrpart_only_idx; -- fails
+ERROR: cannot cluster on invalid index "clstrpart_only_idx"
+DROP INDEX clstrpart_only_idx;
CREATE INDEX clstrpart_idx ON clstrpart (a);
-ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
-ERROR: cannot mark index clustered in partitioned table
+-- Check that clustering sets new relfilenodes:
+CREATE TEMP TABLE old_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
CLUSTER clstrpart USING clstrpart_idx;
-ERROR: cannot cluster a partitioned table
-DROP TABLE clstrpart;
+CREATE TEMP TABLE new_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
+SELECT relname, old.level, old.relkind, old.relfilenode = new.relfilenode FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname) ORDER BY relname COLLATE "C";
+ relname | level | relkind | ?column?
+-------------+-------+---------+----------
+ clstrpart | 0 | p | t
+ clstrpart1 | 1 | p | t
+ clstrpart11 | 2 | r | f
+ clstrpart12 | 2 | p | t
+ clstrpart2 | 1 | r | f
+ clstrpart3 | 1 | p | t
+ clstrpart33 | 2 | r | f
+(7 rows)
+
+-- Check that clustering sets new indisclustered:
+SELECT relname, relkind, indisclustered FROM pg_partition_tree('clstrpart_idx'::regclass) AS tree JOIN pg_index i ON i.indexrelid=tree.relid JOIN pg_class c ON c.oid=indexrelid ORDER BY relname COLLATE "C";
+ relname | relkind | indisclustered
+-------------------+---------+----------------
+ clstrpart11_a_idx | i | t
+ clstrpart12_a_idx | I | t
+ clstrpart1_a_idx | I | t
+ clstrpart2_a_idx | i | t
+ clstrpart33_a_idx | i | t
+ clstrpart3_a_idx | I | t
+ clstrpart_idx | I | t
+(7 rows)
+
+CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned
+CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
+CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
+\d clstrpart
+ Partitioned table "public.clstrpart"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition key: RANGE (a)
+Indexes:
+ "clstrpart_idx" btree (a) CLUSTER
+Number of partitions: 3 (Use \d+ to list them.)
+
-- Test CLUSTER with external tuplesorting
create table clstr_4 as select * from tenk1;
create index cluster_sort on clstr_4 (hundred, thousand, tenthous);
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 188183647c..9bcc77695c 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -196,12 +196,30 @@ drop table clstr_temp;
RESET SESSION AUTHORIZATION;
--- Check that partitioned tables cannot be clustered
+-- Check that partitioned tables can be clustered
CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10);
+CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a);
+CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30);
+CREATE TABLE clstrpart3 PARTITION OF clstrpart DEFAULT PARTITION BY RANGE(a);
+CREATE TABLE clstrpart33 PARTITION OF clstrpart3 DEFAULT;
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+CREATE INDEX clstrpart_only_idx ON ONLY clstrpart (a);
+CLUSTER clstrpart USING clstrpart_only_idx; -- fails
+DROP INDEX clstrpart_only_idx;
CREATE INDEX clstrpart_idx ON clstrpart (a);
-ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+-- Check that clustering sets new relfilenodes:
+CREATE TEMP TABLE old_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
CLUSTER clstrpart USING clstrpart_idx;
-DROP TABLE clstrpart;
+CREATE TEMP TABLE new_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
+SELECT relname, old.level, old.relkind, old.relfilenode = new.relfilenode FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname) ORDER BY relname COLLATE "C";
+-- Check that clustering sets new indisclustered:
+SELECT relname, relkind, indisclustered FROM pg_partition_tree('clstrpart_idx'::regclass) AS tree JOIN pg_index i ON i.indexrelid=tree.relid JOIN pg_class c ON c.oid=indexrelid ORDER BY relname COLLATE "C";
+CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned
+CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
+CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
+\d clstrpart
-- Test CLUSTER with external tuplesorting
--
2.17.0
--fmvA4kSBHQVZhkR6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="v9-0003-f-progress-reporting.patch"
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PATCH] Support % wildcard in extension upgrade filenames
@ 2023-01-09 22:51 Tom Lane <[email protected]>
2023-01-10 20:52 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
2023-04-03 02:26 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Yurii Rashkovskii <[email protected]>
0 siblings, 2 replies; 12+ messages in thread
From: Tom Lane @ 2023-01-09 22:51 UTC (permalink / raw)
To: Regina Obe <[email protected]>; +Cc: [email protected]; Sandro Santilli <[email protected]>
I continue to think that this is a fundamentally bad idea. It creates
all sorts of uncertainties about what is a valid update path and what
is not. Restrictions like
+ Such wildcard update
+ scripts will only be used when no explicit path is found from
+ old to target version.
are just band-aids to try to cover up the worst problems.
Have you considered the idea of instead inventing a "\include" facility
for extension scripts? Then, if you want to use one-monster-script
to handle different upgrade cases, you still need one script file for
each supported upgrade step, but those can be one-liners including the
common script file. Plus, such a facility could be of use to people
who want intermediate factorization solutions (that is, some sharing
of code without buying all the way into one-monster-script).
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PATCH] Support % wildcard in extension upgrade filenames
2023-01-09 22:51 Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
@ 2023-01-10 20:52 ` Sandro Santilli <[email protected]>
2023-01-10 23:50 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Sandro Santilli @ 2023-01-10 20:52 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Regina Obe <[email protected]>; [email protected]
On Mon, Jan 09, 2023 at 05:51:49PM -0500, Tom Lane wrote:
> Have you considered the idea of instead inventing a "\include" facility
[...]
> cases, you still need one script file for each supported upgrade step
That's exactly the problem we're trying to solve here.
The include support is nice on itself, but won't solve our problem.
--strk;
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PATCH] Support % wildcard in extension upgrade filenames
2023-01-09 22:51 Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-01-10 20:52 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
@ 2023-01-10 23:50 ` Tom Lane <[email protected]>
2023-01-11 10:10 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
2023-03-07 17:39 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Robert Haas <[email protected]>
0 siblings, 2 replies; 12+ messages in thread
From: Tom Lane @ 2023-01-10 23:50 UTC (permalink / raw)
To: Sandro Santilli <[email protected]>; +Cc: Regina Obe <[email protected]>; [email protected]
Sandro Santilli <[email protected]> writes:
> On Mon, Jan 09, 2023 at 05:51:49PM -0500, Tom Lane wrote:
>> ... you still need one script file for each supported upgrade step
> That's exactly the problem we're trying to solve here.
> The include support is nice on itself, but won't solve our problem.
The script-file-per-upgrade-path aspect solves a problem that you
have, whether you admit it or not; I think you simply aren't realizing
that because you have not had to deal with the consequences of
your proposed feature. Namely that you won't have any control
over what the backend will try to do in terms of upgrade paths.
As an example, suppose that a database has foo 4.0 installed, and
the DBA decides to try to downgrade to 3.0. With the system as it
stands, if you've provided foo--4.0--3.0.sql then the conversion
will go through, and presumably it will work because you tested that
that script does what it is intended to. If you haven't provided
any such downgrade script, then ALTER EXTENSION UPDATE will say
"Sorry Dave, I'm afraid I can't do that" and no harm is done.
With the proposed % feature, if foo--%--3.0.sql exists then the
system will invoke it and expect the end result to be a valid
3.0 installation, whether or not the script actually has any
ability to do a downgrade. Moreover, there isn't any very
good way to detect or prevent unsupported version transitions.
(I suppose you could add code to look at pg_extension.extversion,
but I'm not sure if that works: it looks to me like we update that
before we run the extension script. Besides which, if you have
to add such code is that really better than having a number of
one-liner scripts implementing the same check declaratively?)
It gets worse though, because above I'm supposing that 4.0 at
least existed when this copy of foo--%--3.0.sql was made.
Suppose that somebody fat-fingered a package upgrade, such that
the extension fileset available to a database containing foo 4.0
now corresponds to foo 3.0, and there's no knowledge of 4.0 at all
in the extension scripts. The DBA trustingly issues ALTER EXTENSION
UPDATE, which will conclude from foo.control that it should update to
3.0, and invoke foo--%--3.0.sql to do it. Maybe the odds of success
are higher than zero, but not by much; almost certainly you are
going to end with an extension containing some leftover 4.0
objects, some 3.0 objects, and maybe some objects with properties
that don't exactly match either 3.0 or 4.0. Even if that state
of affairs manages not to cause immediate problems, it'll surely
be a mess whenever somebody tries to re-update to 4.0 or later.
So I really think this is a case of "be careful what you ask
for, you might get it". Even if PostGIS is willing to put in
the amount of infrastructure legwork needed to make such a
design bulletproof, I'm quite sure nobody else will manage
to use such a thing successfully. I'd rather spend our
development effort on a feature that has more than one use-case.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PATCH] Support % wildcard in extension upgrade filenames
2023-01-09 22:51 Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-01-10 20:52 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
2023-01-10 23:50 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
@ 2023-01-11 10:10 ` Sandro Santilli <[email protected]>
1 sibling, 0 replies; 12+ messages in thread
From: Sandro Santilli @ 2023-01-11 10:10 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Regina Obe <[email protected]>; [email protected]
On Tue, Jan 10, 2023 at 06:50:31PM -0500, Tom Lane wrote:
> With the proposed % feature, if foo--%--3.0.sql exists then the
> system will invoke it and expect the end result to be a valid
> 3.0 installation, whether or not the script actually has any
> ability to do a downgrade.
It is sane, for the system to expect the end result
to be a valid 3.0 installation if no exception is
thrown by the script itself.
If we ship foo--%--3.0.sql we must have been taken care of
protecting from unsupported downgrades/upgrades (and we did,
having the script throw an exception if anything is unexpected).
> (I suppose you could add code to look at pg_extension.extversion,
We actually added code looking at our own version-extracting
function (which existed since before PostgreSQL added support
for extensions). Is the function not there ? Raise an exception.
Is the function not owned by the extension ? Raise an exception.
In other cases -> trust the output of that function to tell what
version we're coming from, throw an exception if upgrade to the
target version is unsupported.
> to add such code is that really better than having a number of
> one-liner scripts implementing the same check declaratively?)
Yes, it is, because no matter how many one-liner scripts we install
(we currently install 87 * 6 such scripts, we always end up missing
some of them upon releasing a new bug-fix release in stable branches.
> almost certainly you are
> going to end with an extension containing some leftover 4.0
> objects, some 3.0 objects, and maybe some objects with properties
> that don't exactly match either 3.0 or 4.0.
This is already possible, depending on WHO writes those upgrade
scripts. This proposal just gives more expressiveness power to
those script authors.
> So I really think this is a case of "be careful what you ask
> for, you might get it". Even if PostGIS is willing to put in
> the amount of infrastructure legwork needed to make such a
> design bulletproof, I'm quite sure nobody else will manage
> to use such a thing successfully.
This is why I initially made this something to be explicitly enabled
by the .control file, which I can do again if it feels safer for you.
> I'd rather spend our
> development effort on a feature that has more than one use-case.
Use case is any extension willing to support more than a single stable
branch while still allowing upgrading from newer-stable-bugfix-release
to older-feature-release (ie: 3.2.10 -> 3.4.0 ). Does not seem so
uncommon to me, for a big project. Maybe there aren't enough big
extension-based projects out there ?
--strk;
Libre GIS consultant/developer
https://strk.kbt.io/services.html
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PATCH] Support % wildcard in extension upgrade filenames
2023-01-09 22:51 Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-01-10 20:52 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
2023-01-10 23:50 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
@ 2023-03-07 17:39 ` Robert Haas <[email protected]>
2023-03-07 19:13 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-03-07 19:25 ` RE: [PATCH] Support % wildcard in extension upgrade filenames Regina Obe <[email protected]>
1 sibling, 2 replies; 12+ messages in thread
From: Robert Haas @ 2023-03-07 17:39 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Sandro Santilli <[email protected]>; Regina Obe <[email protected]>; [email protected]
On Tue, Jan 10, 2023 at 6:50 PM Tom Lane <[email protected]> wrote:
> The script-file-per-upgrade-path aspect solves a problem that you
> have, whether you admit it or not; I think you simply aren't realizing
> that because you have not had to deal with the consequences of
> your proposed feature. Namely that you won't have any control
> over what the backend will try to do in terms of upgrade paths.
>
> As an example, suppose that a database has foo 4.0 installed, and
> the DBA decides to try to downgrade to 3.0. With the system as it
> stands, if you've provided foo--4.0--3.0.sql then the conversion
> will go through, and presumably it will work because you tested that
> that script does what it is intended to. If you haven't provided
> any such downgrade script, then ALTER EXTENSION UPDATE will say
> "Sorry Dave, I'm afraid I can't do that" and no harm is done.
I mean, there is nothing to prevent the extension author from writing
a script which ensures that the extension membership after the
downgrade is exactly what it should be for version 3.0, regardless of
what it was before. SQL is Turing-complete.
The thing that confuses me here is why the PostGIS folks are ending up
with so many files. We certainly don't have that problem with the
extension that are being maintained in contrib, and I guess there is
some difference in versioning practice that is making it an issue for
them but not for us. I wish I understood what was going on there.
But that to one side, there is clearly a problem here, and I think
PostgreSQL ought to provide some infrastructure to help solve it, even
if the ultimate cause of that problem is that the PostGIS folks
managed their extension versions in some less-than-ideal way. I can't
shake the feeling that you're just holding your breath here and hoping
the problem goes away by itself, and judging by the responses, that
doesn't seem like it's going to happen.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PATCH] Support % wildcard in extension upgrade filenames
2023-01-09 22:51 Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-01-10 20:52 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
2023-01-10 23:50 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-03-07 17:39 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Robert Haas <[email protected]>
@ 2023-03-07 19:13 ` Tom Lane <[email protected]>
2023-03-07 19:39 ` RE: [PATCH] Support % wildcard in extension upgrade filenames Regina Obe <[email protected]>
2023-03-08 12:32 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
1 sibling, 2 replies; 12+ messages in thread
From: Tom Lane @ 2023-03-07 19:13 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Sandro Santilli <[email protected]>; Regina Obe <[email protected]>; [email protected]
Robert Haas <[email protected]> writes:
> On Tue, Jan 10, 2023 at 6:50 PM Tom Lane <[email protected]> wrote:
>> As an example, suppose that a database has foo 4.0 installed, and
>> the DBA decides to try to downgrade to 3.0. With the system as it
>> stands, if you've provided foo--4.0--3.0.sql then the conversion
>> will go through, and presumably it will work because you tested that
>> that script does what it is intended to. If you haven't provided
>> any such downgrade script, then ALTER EXTENSION UPDATE will say
>> "Sorry Dave, I'm afraid I can't do that" and no harm is done.
> I mean, there is nothing to prevent the extension author from writing
> a script which ensures that the extension membership after the
> downgrade is exactly what it should be for version 3.0, regardless of
> what it was before. SQL is Turing-complete.
It may be Turing-complete, but I seriously doubt that that's sufficient
for the problem I'm thinking of, which is to downgrade from an extension
version that you never heard of at the time the script was written.
In the worst case, that version might even contain objects of types
you never heard of and don't know how to drop.
You can imagine various approaches to deal with that; for instance,
maybe we could provide some kind of command to deal with dropping an
object identified by classid and objid, which the upgrade script
could scrape out of pg_depend. After writing even more code to issue
those drops in dependency-aware order, you could get on with modifying
the objects you do know about ... but maybe those now have properties
you never heard of and don't know how to adjust.
Whether this is all theoretically possible is sort of moot. What
I am maintaining is that no extension author is actually going to
write such a script, indeed they probably won't trouble to write
any downgrade-like actions at all. Which makes the proposed design
mostly a foot-gun.
> But that to one side, there is clearly a problem here, and I think
> PostgreSQL ought to provide some infrastructure to help solve it, even
> if the ultimate cause of that problem is that the PostGIS folks
> managed their extension versions in some less-than-ideal way. I can't
> shake the feeling that you're just holding your breath here and hoping
> the problem goes away by itself, and judging by the responses, that
> doesn't seem like it's going to happen.
I'm not unsympathetic to the idea of trying to support multiple upgrade
paths in one script. I just don't like this particular design for that,
because it requires the extension author to make promises that nobody
is actually going to deliver on.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* RE: [PATCH] Support % wildcard in extension upgrade filenames
2023-01-09 22:51 Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-01-10 20:52 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
2023-01-10 23:50 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-03-07 17:39 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Robert Haas <[email protected]>
2023-03-07 19:13 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
@ 2023-03-07 19:39 ` Regina Obe <[email protected]>
1 sibling, 0 replies; 12+ messages in thread
From: Regina Obe @ 2023-03-07 19:39 UTC (permalink / raw)
To: 'Tom Lane' <[email protected]>; 'Robert Haas' <[email protected]>; +Cc: 'Sandro Santilli' <[email protected]>; 'Regina Obe' <[email protected]>; [email protected]
> I'm not unsympathetic to the idea of trying to support multiple upgrade paths
> in one script. I just don't like this particular design for that, because it
> requires the extension author to make promises that nobody is actually going
> to deliver on.
>
> regards, tom lane
How about the idea I mentioned, of we revise the patch to read versioned upgrades from the control file
rather than relying on said file to exist.
https://www.postgresql.org/message-id/000201d92572%247dcd8260%2479688720%24%40pcorp.us
Even better, we have an additional control file, something like
postgis--paths.control
That has separate lines to itemize those paths. It would be nice if we could allow wild-cards in that, but I could live without that if we can stop shipping 300 empty files.
Thanks,
Regina
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PATCH] Support % wildcard in extension upgrade filenames
2023-01-09 22:51 Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-01-10 20:52 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
2023-01-10 23:50 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-03-07 17:39 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Robert Haas <[email protected]>
2023-03-07 19:13 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
@ 2023-03-08 12:32 ` Sandro Santilli <[email protected]>
1 sibling, 0 replies; 12+ messages in thread
From: Sandro Santilli @ 2023-03-08 12:32 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Regina Obe <[email protected]>; [email protected]
On Tue, Mar 07, 2023 at 02:13:07PM -0500, Tom Lane wrote:
> What I am maintaining is that no extension author is actually going
> to write such a script, indeed they probably won't trouble to write
> any downgrade-like actions at all. Which makes the proposed design
> mostly a foot-gun.
What I'm maintaining is that such authors should be warned about
the risk, and discouraged from installing any wildcard-containing
script UNLESS they deal with downgrade protection.
PostGIS does deal with that kind of protection (yes, could be helped
somehow in doing that by PostgreSQL).
> I'm not unsympathetic to the idea of trying to support multiple upgrade
> paths in one script. I just don't like this particular design for that,
> because it requires the extension author to make promises that nobody
> is actually going to deliver on.
Would you be ok with a stricter pattern matching ? Something like:
postgis--3.3.%--3.3.ANY.sql
postgis--3.3.ANY--3.4.0.sql
Would that be easier to promise something about ?
--strk;
^ permalink raw reply [nested|flat] 12+ messages in thread
* RE: [PATCH] Support % wildcard in extension upgrade filenames
2023-01-09 22:51 Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-01-10 20:52 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
2023-01-10 23:50 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-03-07 17:39 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Robert Haas <[email protected]>
@ 2023-03-07 19:25 ` Regina Obe <[email protected]>
1 sibling, 0 replies; 12+ messages in thread
From: Regina Obe @ 2023-03-07 19:25 UTC (permalink / raw)
To: 'Robert Haas' <[email protected]>; 'Tom Lane' <[email protected]>; +Cc: 'Sandro Santilli' <[email protected]>; 'Regina Obe' <[email protected]>; [email protected]
> The thing that confuses me here is why the PostGIS folks are ending up with
> so many files.
> We certainly don't have that problem with the extension that
> are being maintained in contrib, and I guess there is some difference in
> versioning practice that is making it an issue for them but not for us. I wish I
> understood what was going on there.
The contrib files are minor versioned. PostGIS is micro versioned.
So we have for example postgis--3.3.0--3.3.1.sql
Also we have 5 extensions we ship all micro versions, so multiply that issue by 5
postgis
postgis_raster
postgis_topology
postgis_tiger_geocoder
postgis_sfcgal
The other wrinkle we have that I don't think postgresql's contrib have is that we support
Each version across multiple PostgreSQL versions.
So for example our 3.3 series supports PostgreSQL 12-15 (with plans to also support 16).
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PATCH] Support % wildcard in extension upgrade filenames
2023-01-09 22:51 Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
@ 2023-04-03 02:26 ` Yurii Rashkovskii <[email protected]>
2023-04-09 20:46 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Yurii Rashkovskii @ 2023-04-03 02:26 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Regina Obe <[email protected]>; [email protected]; Sandro Santilli <[email protected]>
I want to chime in on the issue of lower-number releases that are released
after higher-number releases. The way I see this particular problem is that
we always put upgrade SQL files in release "packages," and they obviously
become static resources.
While I [intentionally] overlook some details here, what if (as a
convention, for projects where it matters) we shipped extensions with
non-upgrade SQL files only, and upgrades were available as separate
downloads? This way, we're not tying releases themselves to upgrade paths.
This also requires no changes to Postgres.
I know this may be a big delivery layout departure for well-established
projects; I also understand that this won't solve the problem of having to
have these files in the first place (though in many cases, they can be
automatically generated once, I suppose, if they are trivial).
On Tue, Jan 10, 2023 at 5:52 AM Tom Lane <[email protected]> wrote:
> I continue to think that this is a fundamentally bad idea. It creates
> all sorts of uncertainties about what is a valid update path and what
> is not. Restrictions like
>
> + Such wildcard update
> + scripts will only be used when no explicit path is found from
> + old to target version.
>
> are just band-aids to try to cover up the worst problems.
>
> Have you considered the idea of instead inventing a "\include" facility
> for extension scripts? Then, if you want to use one-monster-script
> to handle different upgrade cases, you still need one script file for
> each supported upgrade step, but those can be one-liners including the
> common script file. Plus, such a facility could be of use to people
> who want intermediate factorization solutions (that is, some sharing
> of code without buying all the way into one-monster-script).
>
> regards, tom lane
>
>
>
--
Y.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PATCH] Support % wildcard in extension upgrade filenames
2023-01-09 22:51 Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-04-03 02:26 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Yurii Rashkovskii <[email protected]>
@ 2023-04-09 20:46 ` Sandro Santilli <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Sandro Santilli @ 2023-04-09 20:46 UTC (permalink / raw)
To: Yurii Rashkovskii <[email protected]>; +Cc: Tom Lane <[email protected]>; Regina Obe <[email protected]>; [email protected]
On Mon, Apr 03, 2023 at 09:26:25AM +0700, Yurii Rashkovskii wrote:
> I want to chime in on the issue of lower-number releases that are released
> after higher-number releases. The way I see this particular problem is that
> we always put upgrade SQL files in release "packages," and they obviously
> become static resources.
>
> While I [intentionally] overlook some details here, what if (as a
> convention, for projects where it matters) we shipped extensions with
> non-upgrade SQL files only, and upgrades were available as separate
> downloads? This way, we're not tying releases themselves to upgrade paths.
> This also requires no changes to Postgres.
This is actually something that's on the plate, and we recently
added a --disable-extension-upgrades-install configure switch
and a `install-extension-upgrades-from-known-versions` make target
in PostGIS to help going in that direction. I guess the ball would
now be in the hands of packagers.
> I know this may be a big delivery layout departure for well-established
> projects; I also understand that this won't solve the problem of having to
> have these files in the first place (though in many cases, they can be
> automatically generated once, I suppose, if they are trivial).
We will now also be providing a `postgis` script for administration
that among other things will support a `install-extension-upgrades`
command to install upgrade paths from specific old versions, but will
not hard-code any list of "known" old versions as such a list will
easily become outdated.
Note that all upgrade scripts installed by the Makefile target or by
the `postgis` scripts will only be empty upgrade paths from the source
version to the fake "ANY" version, as the ANY--<current> upgrade path
will still be the "catch-all" upgrade script.
--strk;
> On Tue, Jan 10, 2023 at 5:52 AM Tom Lane <[email protected]> wrote:
>
> > I continue to think that this is a fundamentally bad idea. It creates
> > all sorts of uncertainties about what is a valid update path and what
> > is not. Restrictions like
> >
> > + Such wildcard update
> > + scripts will only be used when no explicit path is found from
> > + old to target version.
> >
> > are just band-aids to try to cover up the worst problems.
> >
> > Have you considered the idea of instead inventing a "\include" facility
> > for extension scripts? Then, if you want to use one-monster-script
> > to handle different upgrade cases, you still need one script file for
> > each supported upgrade step, but those can be one-liners including the
> > common script file. Plus, such a facility could be of use to people
> > who want intermediate factorization solutions (that is, some sharing
> > of code without buying all the way into one-monster-script).
> >
> > regards, tom lane
> >
> >
> >
>
> --
> Y.
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2023-04-09 20:46 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-06-07 21:58 [PATCH v9 2/8] Implement CLUSTER of partitioned table.. Justin Pryzby <[email protected]>
2023-01-09 22:51 Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-01-10 20:52 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
2023-01-10 23:50 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-01-11 10:10 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
2023-03-07 17:39 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Robert Haas <[email protected]>
2023-03-07 19:13 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Tom Lane <[email protected]>
2023-03-07 19:39 ` RE: [PATCH] Support % wildcard in extension upgrade filenames Regina Obe <[email protected]>
2023-03-08 12:32 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
2023-03-07 19:25 ` RE: [PATCH] Support % wildcard in extension upgrade filenames Regina Obe <[email protected]>
2023-04-03 02:26 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Yurii Rashkovskii <[email protected]>
2023-04-09 20:46 ` Re: [PATCH] Support % wildcard in extension upgrade filenames Sandro Santilli <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox