public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v6 1/7] pg_dump: make CLUSTER ON a separate dump object..
3+ messages / 3 participants
[nested] [flat]
* [PATCH v6 1/7] pg_dump: make CLUSTER ON a separate dump object..
@ 2020-11-26 20:37 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Justin Pryzby @ 2020-11-26 20:37 UTC (permalink / raw)
..since it needs to be restored after any child indexes are restored *and
attached*. The order needs to be:
1) restore child and parent index (order doesn't matter);
2) attach child index;
3) set cluster on child and parent index (order doesn't matter);
---
src/bin/pg_dump/pg_dump.c | 86 ++++++++++++++++++++++++++--------
src/bin/pg_dump/pg_dump.h | 8 ++++
src/bin/pg_dump/pg_dump_sort.c | 8 ++++
3 files changed, 82 insertions(+), 20 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 798d14580e..e6526392e5 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -208,6 +208,7 @@ static void dumpSequence(Archive *fout, TableInfo *tbinfo);
static void dumpSequenceData(Archive *fout, TableDataInfo *tdinfo);
static void dumpIndex(Archive *fout, IndxInfo *indxinfo);
static void dumpIndexAttach(Archive *fout, IndexAttachInfo *attachinfo);
+static void dumpIndexClusterOn(Archive *fout, IndexClusterInfo *clusterinfo);
static void dumpStatisticsExt(Archive *fout, StatsExtInfo *statsextinfo);
static void dumpConstraint(Archive *fout, ConstraintInfo *coninfo);
static void dumpTableConstraintComment(Archive *fout, ConstraintInfo *coninfo);
@@ -7092,6 +7093,11 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
i_inddependcollversions;
int ntups;
+ int ncluster = 0;
+ IndexClusterInfo *clusterinfo;
+ clusterinfo = (IndexClusterInfo *)
+ pg_malloc0(numTables * sizeof(IndexClusterInfo));
+
for (i = 0; i < numTables; i++)
{
TableInfo *tbinfo = &tblinfo[i];
@@ -7471,6 +7477,27 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
/* Plain secondary index */
indxinfo[j].indexconstraint = 0;
}
+
+ /* Record each table's CLUSTERed index, if any */
+ if (indxinfo[j].indisclustered)
+ {
+ IndxInfo *index = &indxinfo[j];
+ IndexClusterInfo *cluster = &clusterinfo[ncluster];
+
+ cluster->dobj.objType = DO_INDEX_CLUSTER_ON;
+ cluster->dobj.catId.tableoid = 0;
+ cluster->dobj.catId.oid = 0;
+ AssignDumpId(&cluster->dobj);
+ cluster->dobj.name = pg_strdup(index->dobj.name);
+ cluster->dobj.namespace = index->indextable->dobj.namespace;
+ cluster->index = index;
+ cluster->indextable = &tblinfo[i];
+
+ /* The CLUSTER ON depends on its index.. */
+ addObjectDependency(&cluster->dobj, index->dobj.dumpId);
+
+ ncluster++;
+ }
}
PQclear(res);
@@ -10296,6 +10323,9 @@ dumpDumpableObject(Archive *fout, DumpableObject *dobj)
case DO_SUBSCRIPTION:
dumpSubscription(fout, (SubscriptionInfo *) dobj);
break;
+ case DO_INDEX_CLUSTER_ON:
+ dumpIndexClusterOn(fout, (IndexClusterInfo *) dobj);
+ break;
case DO_PRE_DATA_BOUNDARY:
case DO_POST_DATA_BOUNDARY:
/* never dumped, nothing to do */
@@ -16516,6 +16546,41 @@ getAttrName(int attrnum, TableInfo *tblInfo)
return NULL; /* keep compiler quiet */
}
+/*
+ * dumpIndexClusterOn
+ * record that the index is clustered.
+ */
+static void
+dumpIndexClusterOn(Archive *fout, IndexClusterInfo *clusterinfo)
+{
+ DumpOptions *dopt = fout->dopt;
+ TableInfo *tbinfo = clusterinfo->indextable;
+ char *qindxname;
+ PQExpBuffer q;
+
+ if (dopt->dataOnly)
+ return;
+
+ q = createPQExpBuffer();
+ qindxname = pg_strdup(fmtId(clusterinfo->dobj.name));
+
+ /* index name is not qualified in this syntax */
+ appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER ON %s;\n",
+ fmtQualifiedDumpable(tbinfo), qindxname);
+
+ if (clusterinfo->dobj.dump & DUMP_COMPONENT_DEFINITION)
+ ArchiveEntry(fout, clusterinfo->dobj.catId, clusterinfo->dobj.dumpId,
+ ARCHIVE_OPTS(.tag = clusterinfo->dobj.name,
+ .namespace = tbinfo->dobj.namespace->dobj.name,
+ .owner = tbinfo->rolname,
+ .description = "INDEX CLUSTER ON",
+ .section = SECTION_POST_DATA,
+ .createStmt = q->data));
+
+ destroyPQExpBuffer(q);
+ free(qindxname);
+}
+
/*
* dumpIndex
* write out to fout a user-defined index
@@ -16570,16 +16635,6 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
* similar code in dumpConstraint!
*/
- /* If the index is clustered, we need to record that. */
- if (indxinfo->indisclustered)
- {
- appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER",
- fmtQualifiedDumpable(tbinfo));
- /* index name is not qualified in this syntax */
- appendPQExpBuffer(q, " ON %s;\n",
- qindxname);
- }
-
/*
* If the index has any statistics on some of its columns, generate
* the associated ALTER INDEX queries.
@@ -16906,16 +16961,6 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo)
* similar code in dumpIndex!
*/
- /* If the index is clustered, we need to record that. */
- if (indxinfo->indisclustered)
- {
- appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER",
- fmtQualifiedDumpable(tbinfo));
- /* index name is not qualified in this syntax */
- appendPQExpBuffer(q, " ON %s;\n",
- fmtId(indxinfo->dobj.name));
- }
-
/* If the index defines identity, we need to record that. */
if (indxinfo->indisreplident)
{
@@ -18421,6 +18466,7 @@ addBoundaryDependencies(DumpableObject **dobjs, int numObjs,
break;
case DO_INDEX:
case DO_INDEX_ATTACH:
+ case DO_INDEX_CLUSTER_ON:
case DO_STATSEXT:
case DO_REFRESH_MATVIEW:
case DO_TRIGGER:
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 1290f9659b..57def4c009 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -54,6 +54,7 @@ typedef enum
DO_ATTRDEF,
DO_INDEX,
DO_INDEX_ATTACH,
+ DO_INDEX_CLUSTER_ON,
DO_STATSEXT,
DO_RULE,
DO_TRIGGER,
@@ -386,6 +387,13 @@ typedef struct _indxInfo
DumpId indexconstraint;
} IndxInfo;
+typedef struct _indexClusterInfo
+{
+ DumpableObject dobj;
+ TableInfo *indextable; /* link to table the index is for */
+ IndxInfo *index; /* link to index itself */
+} IndexClusterInfo;
+
typedef struct _indexAttachInfo
{
DumpableObject dobj;
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 46461fb6a1..dd5b233196 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -75,6 +75,7 @@ enum dbObjectTypePriorities
PRIO_CONSTRAINT,
PRIO_INDEX,
PRIO_INDEX_ATTACH,
+ PRIO_INDEX_CLUSTER_ON,
PRIO_STATSEXT,
PRIO_RULE,
PRIO_TRIGGER,
@@ -108,6 +109,7 @@ static const int dbObjectTypePriority[] =
PRIO_ATTRDEF, /* DO_ATTRDEF */
PRIO_INDEX, /* DO_INDEX */
PRIO_INDEX_ATTACH, /* DO_INDEX_ATTACH */
+ PRIO_INDEX_CLUSTER_ON, /* DO_INDEX_CLUSTER_ON */
PRIO_STATSEXT, /* DO_STATSEXT */
PRIO_RULE, /* DO_RULE */
PRIO_TRIGGER, /* DO_TRIGGER */
@@ -136,6 +138,7 @@ static const int dbObjectTypePriority[] =
PRIO_PUBLICATION, /* DO_PUBLICATION */
PRIO_PUBLICATION_REL, /* DO_PUBLICATION_REL */
PRIO_SUBSCRIPTION /* DO_SUBSCRIPTION */
+
};
StaticAssertDecl(lengthof(dbObjectTypePriority) == (DO_SUBSCRIPTION + 1),
@@ -1348,6 +1351,11 @@ describeDumpableObject(DumpableObject *obj, char *buf, int bufsize)
"INDEX ATTACH %s (ID %d)",
obj->name, obj->dumpId);
return;
+ case DO_INDEX_CLUSTER_ON:
+ snprintf(buf, bufsize,
+ "INDEX CLUSTER ON %s (ID %d)",
+ obj->name, obj->dumpId);
+ return;
case DO_STATSEXT:
snprintf(buf, bufsize,
"STATISTICS %s (ID %d OID %u)",
--
2.17.0
--FsscpQKzF/jJk6ya
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0002-Implement-CLUSTER-of-partitioned-table.patch"
^ permalink raw reply [nested|flat] 3+ messages in thread
* Inefficient nbtree behavior with row-comparison quals
@ 2024-05-11 19:19 Tom Lane <[email protected]>
2025-06-26 00:29 ` Re: Inefficient nbtree behavior with row-comparison quals Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Tom Lane @ 2024-05-11 19:19 UTC (permalink / raw)
To: [email protected]; +Cc: Peter Geoghegan <[email protected]>
I spent some time looking into the performance complaint at [1],
which for the sake of self-containedness is
CREATE TABLE t(a int, b int);
INSERT INTO t(a, b)
SELECT
(random() * 123456)::int AS a,
(random() * 123456)::int AS b
FROM
generate_series(1, 12345678);
CREATE INDEX my_idx ON t USING BTREE (a, b);
VACUUM ANALYZE t;
explain (analyze, buffers) select * from t
where row(a, b) > row(123450, 123444) and a = 0
order by a, b;
This produces something like
Index Only Scan using my_idx on t (cost=0.43..8.46 rows=1 width=8) (actual time=475.713..475.713 rows=0 loops=1)
Index Cond: ((ROW(a, b) > ROW(123450, 123444)) AND (a = 0))
Heap Fetches: 0
Buffers: shared hit=1 read=33731
Planning:
Buffers: shared read=4
Planning Time: 0.247 ms
Execution Time: 475.744 ms
showing that we are reading practically the whole index, which
is pretty sad considering the index conditions are visibly
mutually contradictory. What's going on? I find that:
1. _bt_preprocess_keys, which is responsible for detecting
mutually-contradictory index quals, fails to do so because it
really just punts on row-comparison quals: it shoves them
directly from input to output scankey array without any
comparisons to other keys. (In particular, that causes the
row-comparison qual to appear before the a = 0 one in the
output scankey array.)
2. The initial-positioning logic in _bt_first chooses "a = 0"
as determining where to start the scan, because it always
prefers equality over inequality keys. (This seems reasonable.)
3. We really should stop the scan once we're past the last a = 0
index entry, which'd at least limit the damage. However, at both
a = 0 and later entries, the row-comparison qual fails causing
_bt_check_compare to return immediately, without examining the
a = 0 key which is marked as SK_BT_REQFWD, and thus it does not
clear "continuescan". Only when we finally reach an index entry
for which the row-comparison qual succeeds do we notice that
a = 0 is failing so we could stop the scan.
So this seems pretty horrid. It would be nice if _bt_preprocess_keys
were smart enough to notice the contradictory nature of these quals,
but I grant that (a) that routine is dauntingly complex already and
(b) this doesn't seem like a common enough kind of query to be worth
moving heaven and earth to optimize.
However, I do think we should do something about the unstated
assumption that _bt_preprocess_keys can emit the quals (for a given
column) in any random order it feels like. This is evidently not so,
and it's probably capable of pessimizing other examples besides this
one. Unless we want to slow down _bt_check_compare by making it
continue to examine quals after the first failure, we need to insist
that required quals appear before non-required quals, thus ensuring
that a comparison failure will clear continuescan if possible.
Even that looks a little nontrivial, because it seems like nbtree
may be making some assumptions about the order in which array keys
appear. I see the bit about
* ... Some reordering of the keys
* within each attribute may be done as a byproduct of the processing here.
* That process must leave array scan keys (within an attribute) in the same
* order as corresponding entries from the scan's BTArrayKeyInfo array info.
which I could cope with, but then there's this down around line 2967:
* Note: We do things this way around so that our arrays are
* always in the same order as their corresponding scan keys,
* even with incomplete opfamilies. _bt_advance_array_keys
* depends on this.
However, despite the rather over-the-top verbosity of commenting in
_bt_advance_array_keys, it's far from clear why or how it depends on
that. So I feel a little stuck about what needs to be done here.
regards, tom lane
[1] https://www.postgresql.org/message-id/CAAdwFAxBjyrYUkH7u%2BEceTaztd1QxBtBY1Teux8K%3DvcGKe%3D%3D-A%40...
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Inefficient nbtree behavior with row-comparison quals
2024-05-11 19:19 Inefficient nbtree behavior with row-comparison quals Tom Lane <[email protected]>
@ 2025-06-26 00:29 ` Peter Geoghegan <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Peter Geoghegan @ 2025-06-26 00:29 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]
On Sat, May 11, 2024 at 3:19 PM Tom Lane <[email protected]> wrote:
> showing that we are reading practically the whole index, which
> is pretty sad considering the index conditions are visibly
> mutually contradictory. What's going on? I find that:
>
> 1. _bt_preprocess_keys, which is responsible for detecting
> mutually-contradictory index quals, fails to do so because it
> really just punts on row-comparison quals: it shoves them
> directly from input to output scankey array without any
> comparisons to other keys. (In particular, that causes the
> row-comparison qual to appear before the a = 0 one in the
> output scankey array.)
It turns out that this behavior also causes problems for
_bt_advance_array_keys (not performance problems, correctness
problems). I found that out when working on a bugfix commit for an
issue affecting Postgres 18, with incomplete opfamilies:
https://postgr.es/m/CAH2-Wz=ds4M+3NXMgwxYxqU8MULaLf696_v5g=9WNmWL2=Uo2A@mail.gmail.com
The bugfix that I posted earlier today fixes the issue highlighted by
your test case from this thread. Your test case will only read a tiny
fraction of the index, since with the patch in place, everybody looks
at the "a = 0" key first.
Note that the issue I'm seeing isn't unique to row compares, even
though I say plenty about them on the same thread. All contradictory
inequalities are equally affected with an incomplete opfamily in
cross-type scenarios.
I'm now motivated to deal with key ordering issues in the most
general way possible, due to the implications for
_bt_advance_array_keys highlighted on that other thread.
> 2. The initial-positioning logic in _bt_first chooses "a = 0"
> as determining where to start the scan, because it always
> prefers equality over inequality keys. (This seems reasonable.)
Part of the problem, as I see it, is that _bt_first gets to
independently decide for itself that it should prefer "a = 0" over the
row compare qual. That happens to be the right thing to do here, but
_bt_first shouldn't get to go its own way. And neither should
_bt_readpage/_bt_checkkeys.
Everybody should agree on which one scan key on each index column
should be used for starting the scan in one direction, and ending the
scan in the other direction. That should be decided during
preprocessing. There is always exactly one best way to order and
required-mark the keys output by preprocessing -- even with an
incomplete opfamily. (And even with row compares that happen to be
redundant or contradictory, which are similar.)
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2025-06-26 00:29 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-26 20:37 [PATCH v6 1/7] pg_dump: make CLUSTER ON a separate dump object.. Justin Pryzby <[email protected]>
2024-05-11 19:19 Inefficient nbtree behavior with row-comparison quals Tom Lane <[email protected]>
2025-06-26 00:29 ` Re: Inefficient nbtree behavior with row-comparison quals Peter Geoghegan <[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