BUG #19086: pg_dump --data-only selects and do not uses index definitions for the dumped tables.

Patches for 17- and 18+

Regards, Andrew

On Tue, Oct 14, 2025 at 3:14 PM PG Bug reporting form <
noreply@postgresql.org> wrote:

On Tue, Oct 14, 2025 at 08:13:52AM +0000, PG Bug reporting form wrote:

In case of system indexes corruption the collecting of index definitions can
take a really long time.

I don't think this qualifies as a bug, but avoiding pg_dump queries when
possible seems like a good idea. Is this a hypothetical problem or
something you've experienced?

-       pg_log_info("reading indexes");
-       getIndexes(fout, tblinfo, numTables);
+       if (!dataOnly)
+       {
+               pg_log_info("reading indexes");
+               getIndexes(fout, tblinfo, numTables);

-       pg_log_info("flagging indexes in partitioned tables");
-       flagInhIndexes(fout, tblinfo, numTables);
+               pg_log_info("flagging indexes in partitioned tables");
+               flagInhIndexes(fout, tblinfo, numTables);
+       }

I think we ordinarily leave it up to the get*() functions to return early.
For example, getPartitioningInfo() has this near the top:

/* needn't bother if not dumping data */
if (!fout->dopt->dumpData)
return;

Also, we need to be certain that nothing getIndexes() gathers is ever used
for --data-only dumps. That seems plausible, but I haven't looked closely.

--
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:

On Tue, Oct 14, 2025 at 08:13:52AM +0000, PG Bug reporting form wrote:

In case of system indexes corruption the collecting of index definitions can
take a really long time.

I don't think this qualifies as a bug, but avoiding pg_dump queries when
possible seems like a good idea. Is this a hypothetical problem or
something you've experienced?

Even if it's been seen in the field, it hardly qualifies as a
justification for complicating pg_dump's behavior. There's no reason
to think that catalog corruption preferentially affects indexes, or
does so in this particular way. Should we also not collect data on
functions, types, etc etc?

Also, we need to be certain that nothing getIndexes() gathers is ever used
for --data-only dumps. That seems plausible, but I haven't looked closely.

Yeah, that's the main thing I'd worry about here. For example,
ignoring some objects would likely lead to different conclusions about
the database objects' dependency web, possibly leading to changes in
dump order or other effects. Maybe there's no problem in practice,
but this unsupported bug report doesn't really motivate me to do the
analysis to see if it'd be all right.

regards, tom lane

On Tue, Oct 14, 2025 at 03:50:15PM -0400, Tom Lane wrote:

Even if it's been seen in the field, it hardly qualifies as a
justification for complicating pg_dump's behavior. There's no reason
to think that catalog corruption preferentially affects indexes, or
does so in this particular way. Should we also not collect data on
functions, types, etc etc?

FWIW the getIndexes() query does tend to be one of the slowest, even with
intact system indexes. I've no concrete proposals, but there might be some
room for improvement. I don't think we gain all that much by simply
avoiding the query in probably-somewhat-rare use-cases. IMHO it ought to
be reworked for efficiency.

--
nathan

On Wed, 15 Oct 2025 at 11:03, Nathan Bossart <nathandbossart@gmail.com> wrote:

FWIW the getIndexes() query does tend to be one of the slowest, even with
intact system indexes. I've no concrete proposals, but there might be some
room for improvement. I don't think we gain all that much by simply
avoiding the query in probably-somewhat-rare use-cases. IMHO it ought to
be reworked for efficiency.

The extra slowness comes from all the subqueries in the targetlist, 3
of which are going to pg_attribute using the same join condition. That
results in 3 separate scans of pg_attribute, 2 more than needed.

The query could be made more efficient generally by doing a left join
to pg_attribute instead and then GROUP BY i.indexrelid.

I tried rewriting the query so that pg_attribute is joined to rather
than subqueries. With 1500 tables I get:

master:

ignore_system_indexes = on
Execution Time: 6853.262 ms

ignore_system_indexes = off
Execution Time: 66.781 ms

Rewritten query:

ignore_system_indexes = on
Execution Time: 53.351 ms

ignore_system_indexes = off
Execution Time: 56.965 ms

David

Is this a hypothetical problem or something you've experienced?

The problem is real: when trying to rescue data from a large corrupted
database, we couldn't wait for the index definition query for more than 3
hours.

On Wed, Oct 15, 2025 at 2:36 AM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On 2025-Oct-15, Andrew Bille wrote:

Is this a hypothetical problem or something you've experienced?

The problem is real: when trying to rescue data from a large corrupted
database, we couldn't wait for the index definition query for more than 3
hours.

Maybe it'd be reasonable to start the recovery by doing VACUUM FULL of
all/some system catalogs.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

Thank you, that case has already been resolved, and the data has been
recovered.
System reindex and vacuum full didn't help.
The schema and indexes were not needed, a modified pg_dump helped us.

On Wed, Oct 15, 2025 at 3:56 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

On Wed, Oct 15, 2025 at 12:56:25PM +1300, David Rowley wrote:

I tried rewriting the query so that pg_attribute is joined to rather
than subqueries. With 1500 tables I get:

master:

ignore_system_indexes = on
Execution Time: 6853.262 ms

ignore_system_indexes = off
Execution Time: 66.781 ms

Rewritten query:

ignore_system_indexes = on
Execution Time: 53.351 ms

ignore_system_indexes = off
Execution Time: 56.965 ms

I tried this with 100K tables and saw the following (ignore_system_indexes
= off):

master:
Planning Time: 1.672 ms
Execution Time: 4077.008 ms

rewritten:
Planning Time: 1.641 ms
Execution Time: 3427.206 ms

I tried this with ignore_system_indexes = on, too, but it was very slow.

--
nathan

On Thu, 16 Oct 2025 at 05:24, Nathan Bossart <nathandbossart@gmail.com> wrote:

I tried this with ignore_system_indexes = on, too, but it was very slow.

Seems to be due to pg_get_indexdef / pg_get_constraintdef operating on
a cold cat cache. Getting rid of those the rewritten version runs in
1.8 seconds with 100k tables for me.

That invalidates my timings from yesterday as I must have run the
rewritten query once the catcache was populated. Here are updated
results.

ignore_system_indexes = on 1.5k tables:
master: Execution Time: 26167.741 ms
rewritten: Execution Time: 16661.774 ms

ignore_system_indexes = off 1.5k tables:
master: Execution Time: 105.415 ms
rewritten: Execution Time: 97.030 ms

So not as good.

David

David Rowley <dgrowleyml@gmail.com> writes:

Seems to be due to pg_get_indexdef / pg_get_constraintdef operating on
a cold cat cache. Getting rid of those the rewritten version runs in
1.8 seconds with 100k tables for me.

I wonder how much win could be had by postponing those function calls
so that they only act on indexes we're going to dump. It might be
a net loss in the default dump-everything case, though.

Also, it looks to me like getIndexes does not even look at the result
of pg_get_constraintdef unless contype == 'x'. So there should be
some low-hanging fruit with

-                         "pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
+                         "CASE WHEN c.contype = 'x' THEN "
+                         "pg_catalog.pg_get_constraintdef(c.oid, false) "
+                         "END AS condef, "

This wouldn't matter except with primary/unique constraints, but
surely there are plenty of those in a typical DB.

regards, tom lane

On Thu, 16 Oct 2025 at 10:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <dgrowleyml@gmail.com> writes:

Seems to be due to pg_get_indexdef / pg_get_constraintdef operating on
a cold cat cache. Getting rid of those the rewritten version runs in
1.8 seconds with 100k tables for me.

I wonder how much win could be had by postponing those function calls
so that they only act on indexes we're going to dump. It might be
a net loss in the default dump-everything case, though.

Just to make sure I understand correctly, that means run a query in
dumpIndex() specifically just for the index being dumped to call
pg_get_indexdef()?

It would mean firing off quite a large number of queries to the
server, which might be especially bad when pg_dump is being run
remotely. I suppose ideally we'd have some matrix to indicate
everything we're going to need based on the given options and just
fetch those things. That'd be a pretty big overhaul.

Also, it looks to me like getIndexes does not even look at the result
of pg_get_constraintdef unless contype == 'x'. So there should be
some low-hanging fruit with

-                         "pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
+                         "CASE WHEN c.contype = 'x' THEN "
+                         "pg_catalog.pg_get_constraintdef(c.oid, false) "
+                         "END AS condef, "

This wouldn't matter except with primary/unique constraints, but
surely there are plenty of those in a typical DB.

I expect that would help quite a bit. We do have NOT NULL constraints
in that table now, so I expect it might be bigger than pg_index in
most cases for recent versions, so the full table scan in
pg_get_constraintdef_worker() with ignore_system_indexes = on could be
more painful than the same thing in pg_get_indexdef_worker().

David

David

David Rowley <dgrowleyml@gmail.com> writes:

On Thu, 16 Oct 2025 at 10:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wonder how much win could be had by postponing those function calls
so that they only act on indexes we're going to dump. It might be
a net loss in the default dump-everything case, though.

Just to make sure I understand correctly, that means run a query in
dumpIndex() specifically just for the index being dumped to call
pg_get_indexdef()?

We could do it like that, but it's probably better to keep the
single-query approach. A way that could work is

(1) Drop pg_get_indexdef and pg_get_constraintdef from getIndexes'
initial query.

(2) As we scan the results of that query, build new OID-list
strings listing just the index OIDs and constraint OIDs that
we require definition strings for.

(3) Still within getIndexes, run those queries and insert the
results into the arrays built in step 1.

On its face, this will be slower than doing it all in one query,
at least in the normal case where we need most or all indexes.
But what I'm hoping is that we need at most one of the two
function calls for any one index, depending on whether it's a
constraint or not. That could buy back enough to justify the
extra overhead, perhaps.

(Hmm ... but on the third hand, if we only need one of the
two strings, couldn't we mechanize that by wrapping the
pg_get_indexdef call in CASE WHEN c.contype IS DISTINCT FROM 'x'
?)

regards, tom lane

On Thu, 16 Oct 2025 at 12:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:

(Hmm ... but on the third hand, if we only need one of the
two strings, couldn't we mechanize that by wrapping the
pg_get_indexdef call in CASE WHEN c.contype IS DISTINCT FROM 'x'
?)

Unless I'm mistaken, it looks like the "indexdef" field is used only
when there's no corresponding constraint with contype 'p,', 'u' or
'x'. Wouldn't it be more like:

CASE WHEN c.conrelid IS NULL THEN
pg_catalog.pg_get_indexdef(i.indexrelid) ELSE '' END AS indexdef

which saves calling the function a bit more often than what you said... ?

The code I'm looking at is:

/* Plain secondary index */
indxinfo[j].indexconstraint = 0;

and "if (!is_constraint)" in dumpIndex().

David

David Rowley <dgrowleyml@gmail.com> writes:

On Thu, 16 Oct 2025 at 12:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:

(Hmm ... but on the third hand, if we only need one of the
two strings, couldn't we mechanize that by wrapping the
pg_get_indexdef call in CASE WHEN c.contype IS DISTINCT FROM 'x'
?)

Unless I'm mistaken, it looks like the "indexdef" field is used only
when there's no corresponding constraint with contype 'p,', 'u' or
'x'.

Ah. I hadn't researched that, but it makes sense.

Wouldn't it be more like:

CASE WHEN c.conrelid IS NULL THEN
pg_catalog.pg_get_indexdef(i.indexrelid) ELSE '' END AS indexdef

I'd leave out the ELSE so that you get a null if the function
isn't run, but yeah. (The places saving these query results would
need PQgetisnull tests, too.)

regards, tom lane

On Thu, 16 Oct 2025 at 13:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <dgrowleyml@gmail.com> writes:

Wouldn't it be more like:

CASE WHEN c.conrelid IS NULL THEN
pg_catalog.pg_get_indexdef(i.indexrelid) ELSE '' END AS indexdef

I'd leave out the ELSE so that you get a null if the function
isn't run, but yeah. (The places saving these query results would
need PQgetisnull tests, too.)

Just to put that to the test, I tried the attached.

select 'create table t'||t||'(a int primary key, b int not null);
create index on t'||t||'(b)' from generate_Series(1,10000)t;
\gexec

master
$ PGOPTIONS='-c ignore_system_indexes=1' time pg_dump --schema-only
postgres >> /dev/null
2:23.75elapsed

$ PGOPTIONS='-c ignore_system_indexes=0' time pg_dump --schema-only
postgres >> /dev/null
0:01.08 elapsed

patched:
$ PGOPTIONS='-c ignore_system_indexes=1' time pg_dump --schema-only
postgres >> /dev/null
0:40.28elapsed

$ PGOPTIONS='-c ignore_system_indexes=0' time pg_dump --schema-only
postgres >> /dev/null
0:00.78elapsed

i.e about 3.5x faster with ignore_system_indexes and 38% faster without.

David

David Rowley <dgrowleyml@gmail.com> writes:

Just to put that to the test, I tried the attached.

I'm confused by all the extraneous changes in this?

i.e about 3.5x faster with ignore_system_indexes and 38% faster without.

Very promising, but I'm still confused.

regards, tom lane

On Thu, 16 Oct 2025 at 14:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <dgrowleyml@gmail.com> writes:

Just to put that to the test, I tried the attached.

I'm confused by all the extraneous changes in this?

It includes the query rewrite too in order to get rid of the
subqueries in the targetlist to pg_attribute. There are 3 of those in
total. When ignore_system_indexes is on, that means a 3x Seq Scans to
pg_attribute per returned row. The rewrite gets rid of that and turns
that into a single join to pg_attribute which allows the planner to
hash or merge join to it. We could just do the conditional calling of
the pg_get_*def() functions, but performance would still be terrible
for ignore_system_indexes=on due to the Seq Scans, and slightly worse
overall.

David

David Rowley <dgrowleyml@gmail.com> writes:

On Thu, 16 Oct 2025 at 14:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm confused by all the extraneous changes in this?

It includes the query rewrite too in order to get rid of the
subqueries in the targetlist to pg_attribute.

Ah, I've not reviewed that bit.

We could just do the conditional calling of
the pg_get_*def() functions, but performance would still be terrible
for ignore_system_indexes=on due to the Seq Scans, and slightly worse
overall.

Got it. Definitely looks promising, but I'm too tired to review
the whole change.

regards, tom lane