Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_dump: avoid unsafe function calls in getPolicies().

  2. Postpone calls of unsafe server-side functions in pg_dump.

  3. Account for TOAST data while scheduling parallel dumps.

  4. Use PREPARE/EXECUTE for repetitive per-object queries in pg_dump.

  5. Avoid per-object queries in performance-critical paths in pg_dump.

  6. Rethink pg_dump's handling of object ACLs.

  7. Refactor pg_dump's tracking of object components to be dumped.

  8. pg_dump: fix mis-dumping of non-global default privileges.

  1. Re: Assorted improvements in pg_dump

    Hans Buschmann <buschmann@nidsa.net> — 2021-10-22T16:36:27Z

    Hello Tom!

I noticed you are improving pg_dump just now.

Some time ago I experimented with a customer database dump in parallel directory mode -F directory -j (2-4)

I noticed it took quite long to complete.

Further investigation showed that in this mode with multiple jobs the tables are processed in decreasing size order, which makes sense to avoid a long tail of a big table in one of the jobs prolonging overall dump time.

Exactly one table took very long, but seemed to be of moderate size.

But the size-determination fails to consider the size of toast tables and this table had a big associated toast-table of bytea column(s).
Even with an analyze at loading time there where no size information of the toast-table in the catalog tables.

I thought of the following alternatives to ameliorate:

1. Using pg_table_size() function in the catalog query
Pos: This reflects the correct size of every relation
Neg: This goes out to disk and may take a huge impact on databases with very many tables

2. Teaching vacuum to set the toast-table size like it sets it on normal tables

3. Have a command/function for occasionly setting the (approximate) size of toast tables 

I think with further work under the way (not yet ready), pg_dump can really profit from parallel/not compressing mode, especially considering the huge amount of bytea/blob/string data in many big customer scenarios.

Thoughts?

Hans Buschmann

  2. Re: Assorted improvements in pg_dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-24T21:10:55Z

    Hans Buschmann <buschmann@nidsa.net> writes:
> Some time ago I experimented with a customer database dump in parallel directory mode -F directory -j (2-4)
> I noticed it took quite long to complete.
> Further investigation showed that in this mode with multiple jobs the tables are processed in decreasing size order, which makes sense to avoid a long tail of a big table in one of the jobs prolonging overall dump time.
> Exactly one table took very long, but seemed to be of moderate size.
> But the size-determination fails to consider the size of toast tables and this table had a big associated toast-table of bytea column(s).

Hmm, yeah, we just use pg_class.relpages for scheduling parallel dumps.
I'd supposed that would be fine, but maybe it's worth being smarter.
I think it should be sufficient to add on the toast table's relpages
value; that's maintained by autovacuum on the same terms as relpages
for regular tables.  See 0005 below.

Here's an update of this patch series:

0001 is the same as before, except I changed collectComments and
collectSecLabels to strdup the strings they want and then release
their PGresults.  The previous behavior confused valgrind's leak
tracking, which is only a minor annoyance, but I think we can
justify changing it now that these functions don't save all of
the collected comments or seclabels.  In particular, we've got
no need for the several thousand comments on built-in objects,
so that that PGresult is at least 100KB bigger than what we're
going to keep.

0002 is updated to account for commit 2acc84c6f.

0003 is the same except I added a missing free().

0004 is a new patch based on an idea from Andres Freund [1]:
in the functions that repetitively issue the same query against
different tables, issue just one query and use a WHERE clause
to restrict the output to the tables we care about.  I was
skeptical about this to start with, but it turns out to be
quite a spectacular win.  On my machine, the time to pg_dump
the regression database (with "-s") drops from 0.91 seconds
to 0.39 seconds.  For a database with 10000 toy tables, the
time drops from 18.1 seconds to 2.3 seconds.

0004 is not committable as-is, because it assumes that the source
server has single-array unnest(), which is not true before 8.4.
We could fix that by using "oid = ANY(array-constant)" conditions
instead, but I'm unsure about the performance properties of that
for large OID arrays on those old server versions.  There's a
discussion at [2] about whether it'd be okay to drop pg_dump's
support for pre-8.4 servers; if we do so, then it would become
unnecessary to do anything more here.

0005 implements your suggestion of accounting for TOAST data while
scheduling parallel dumps.  I realized while looking at that that
there's a pre-existing bug, which this'd exacerbate: on machines
with 32-bit off_t, dataLength can overflow.  Admittedly such machines
are just about extinct in the wild, but we do still trouble to support
the case.  So 0005 also includes code to check for overflow and clamp
the result to INT_MAX blocks.

Maybe we should back-patch 0005.  OTOH, how likely is it that anyone
is wrangling tables exceeding 16TB on a machine with 32-bit off_t?
Or that poor parallel dump scheduling would be a real problem in
such a case?

Lastly, 0006 implements the other idea we'd discussed in the other
thread: for queries that are issued repetitively but not within a
single pg_dump function invocation, use PREPARE/EXECUTE to cut down
the overhead.  This gets only diminishing returns after 0004, but
it still brings "pg_dump -s regression" down from 0.39s to 0.33s,
so maybe it's worth doing.  I stopped after caching the plans for
functions/aggregates/operators/types, though.  The remaining sorts
of objects aren't likely to appear in typical databases enough times
to be worth worrying over.  (This patch will be a net loss if there
are more than zero but less than perhaps 10 instances of an object type,
so there's definitely reasons beyond laziness for not doing more.)

			regards, tom lane

[1] https://www.postgresql.org/message-id/20211022055939.z6fihsm7hdzbjttf%40alap3.anarazel.de
[2] https://www.postgresql.org/message-id/flat/2923349.1634942313%40sss.pgh.pa.us

  3. Re: Assorted improvements in pg_dump

    Justin Pryzby <pryzby@telsasoft.com> — 2021-10-24T22:03:37Z

    On Sun, Oct 24, 2021 at 05:10:55PM -0400, Tom Lane wrote:
> 0003 is the same except I added a missing free().
> 
> 0004 is a new patch based on an idea from Andres Freund [1]:
> in the functions that repetitively issue the same query against
> different tables, issue just one query and use a WHERE clause
> to restrict the output to the tables we care about.  I was
> skeptical about this to start with, but it turns out to be
> quite a spectacular win.  On my machine, the time to pg_dump
> the regression database (with "-s") drops from 0.91 seconds
> to 0.39 seconds.  For a database with 10000 toy tables, the
> time drops from 18.1 seconds to 2.3 seconds.

+               if (tbloids->len > 1)                                                                                                                                                                          
+                       appendPQExpBufferChar(tbloids, ',');                                                                                                                                                   
+               appendPQExpBuffer(tbloids, "%u", tbinfo->dobj.catId.oid);                                                                                                                                      

I think this should say 

+               if (tbloids->len > 0)                                                                                                                                                                          

That doesn't matter much since catalogs aren't dumped as such, and we tend to
count in base 10 and not base 10000.

BTW, the ACL patch makes the overhead 6x lower (6.9sec vs 1.2sec) for pg_dump -t
of a single, small table.  Thanks for that.

-- 
Justin

  4. Re: Assorted improvements in pg_dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-24T22:58:41Z

    Justin Pryzby <pryzby@telsasoft.com> writes:
> On Sun, Oct 24, 2021 at 05:10:55PM -0400, Tom Lane wrote:
>> +               if (tbloids->len > 1)

> I think this should say 
> +               if (tbloids->len > 0)

No, >1 is the correct test, because it's checking the string length
and we started by stuffing a '{' into the string.  Maybe needs a
comment.

> BTW, the ACL patch makes the overhead 6x lower (6.9sec vs 1.2sec) for pg_dump -t
> of a single, small table.  Thanks for that.

Yeah --- I haven't done any formal measurements of the case where you're
selecting a small number of tables, but I did note that it decreased a
good deal compared to HEAD.

			regards, tom lane

  5. AW: Assorted improvements in pg_dump

    Hans Buschmann <buschmann@nidsa.net> — 2021-10-25T12:23:59Z

    ________________________________________
1. Von: Tom Lane <tgl@sss.pgh.pa.us>
>Maybe we should back-patch 0005.  OTOH, how likely is it that anyone
>is wrangling tables exceeding 16TB on a machine with 32-bit off_t?
>Or that poor parallel dump scheduling would be a real problem in
>such a case?

I tested your patch on Windows x64, pg15_devel_25.10.2021 against the customer database
( 2 core/4 thread NUC,32 GB RAM, 1 NVME-SSD, 4 jobs)

pg_dump manually patched with your changes
database pg14.0, 20 GB shared buffers.

The dump of the database tables took 3min7sec for a 16 GB database resulting in a directory of 31.1 GB with 1628 files!

The dump worked like a rush: full cpu-usage, finish.

I don't have the old performance data available, but it is a real improvement, so backpatching may be really woth the effort.

The former slowing-down table has  a ratio from 5169 relpages to 673024 toastpages.

Despite of the great disk usage (about dubbling the size from the db) directory mode seems to be by far the fastest mode especially for databases in the range 1TB+.

For archiving purposes an extern_to_ postgres tool often fits better for compression and can be applied to the dumped data not withholding the dump process.

I am still working on another big speedup in this scenario (comming soon).

-----------------------------------------------------------

2. Another suggestion considering pg_dump

With some customer databases I follow a yearly practice of pg_dump/pg_restore to the new major version.
This eliminates all bloat and does a full reindex, so the disk data layout is already quite clean.

It would be very favorable to dump the pages according to the CLUSTER index when defined for a table. This would only concern the select to retrieve the rows and not harm pg_dump's logic.

This would give perfectly reorganized tables in a pg_dump/pg_restore round.

If a cluster index is defined by the customer, this expresses the whish to have the table layout in this way and nothing is introduced arbitrarily.

I would suggest to have a flag (--cluster) for pg_dump to activate this new behavior.

I think this is not immediately part of the current patchset, but should be taken into account for pg_dump ameliorations in PG15.

In the moment I have not yet enough knowledge to propose a patch of this kind ( a logic similar to the cluster command itself). Perhaps someone could jump in ...

Thanks for the patch and awaiting your thoughts

Hans Buschmann

  6. Re: Assorted improvements in pg_dump

    Andres Freund <andres@anarazel.de> — 2021-10-25T19:30:24Z

    Hi,

On 2021-10-24 17:10:55 -0400, Tom Lane wrote:
> 0004 is not committable as-is, because it assumes that the source
> server has single-array unnest(), which is not true before 8.4.
> We could fix that by using "oid = ANY(array-constant)" conditions
> instead, but I'm unsure about the performance properties of that
> for large OID arrays on those old server versions.

It doesn't seem bad at all. 8.3 assert:

CREATE TABLE foo(oid oid primary key);
INSERT INTO foo SELECT generate_series(1, 1000000);
postgres[1164129][1]=# explain ANALYZE SELECT count(*) FROM foo WHERE oid = ANY(ARRAY(SELECT generate_series(1100000, 1, -1)));
┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│                                                             QUERY PLAN                                                              │
├─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Aggregate  (cost=81.54..81.55 rows=1 width=0) (actual time=2433.656..2433.656 rows=1 loops=1)                                       │
│   InitPlan                                                                                                                          │
│     ->  Result  (cost=0.00..0.01 rows=1 width=0) (actual time=0.004..149.425 rows=1100000 loops=1)                                  │
│   ->  Bitmap Heap Scan on foo  (cost=42.70..81.50 rows=10 width=0) (actual time=2275.137..2369.478 rows=1000000 loops=1)            │
│         Recheck Cond: (oid = ANY (($0)::oid[]))                                                                                     │
│         ->  Bitmap Index Scan on foo_pkey  (cost=0.00..42.69 rows=10 width=0) (actual time=2274.077..2274.077 rows=1000000 loops=1) │
│               Index Cond: (oid = ANY (($0)::oid[]))                                                                                 │
│ Total runtime: 2436.201 ms                                                                                                          │
└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
(8 rows)

Time: 2437.568 ms (00:02.438)


> Lastly, 0006 implements the other idea we'd discussed in the other
> thread: for queries that are issued repetitively but not within a
> single pg_dump function invocation, use PREPARE/EXECUTE to cut down
> the overhead.  This gets only diminishing returns after 0004, but
> it still brings "pg_dump -s regression" down from 0.39s to 0.33s,
> so maybe it's worth doing.

I think it's worth doing. There's things that the batch approach won't help
with and even if it doesn't help a lot with the regression test database, I'd
expect it to help plenty with other cases.

A test database I had around with lots of functions got drastically faster to
dump (7.4s to 2.5s), even though the number of queries didn't change
significantly. According to pg_stat_statements plan_time for the dumpFunc
query went from 2352ms to 0.4ms - interestingly execution time nearly halves
as well.


> I stopped after caching the plans for
> functions/aggregates/operators/types, though.  The remaining sorts
> of objects aren't likely to appear in typical databases enough times
> to be worth worrying over.  (This patch will be a net loss if there
> are more than zero but less than perhaps 10 instances of an object type,
> so there's definitely reasons beyond laziness for not doing more.)

Seems reasonable.


> @@ -7340,25 +7340,37 @@ getDomainConstraints(Archive *fout, TypeInfo *tyinfo)
>  				i_consrc;
>  	int			ntups;
>  
> -	query = createPQExpBuffer();
> +	static bool query_prepared = false;
>  
> -	if (fout->remoteVersion >= 90100)
> -		appendPQExpBuffer(query, "SELECT tableoid, oid, conname, "
> -						  "pg_catalog.pg_get_constraintdef(oid) AS consrc, "
> -						  "convalidated "
> -						  "FROM pg_catalog.pg_constraint "
> -						  "WHERE contypid = '%u'::pg_catalog.oid "
> -						  "ORDER BY conname",
> -						  tyinfo->dobj.catId.oid);
> +	if (!query_prepared)
> +	{

I wonder if it'd be better to store this in Archive or such. The approach with
static variables might run into problems with parallel pg_dump at some
point. These objects aren't dumped in parallel yet, but still...

Greetings,

Andres Freund

  7. Re: Assorted improvements in pg_dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-25T20:02:34Z

    Andres Freund <andres@anarazel.de> writes:
> On 2021-10-24 17:10:55 -0400, Tom Lane wrote:
>> +	static bool query_prepared = false;

> I wonder if it'd be better to store this in Archive or such. The approach with
> static variables might run into problems with parallel pg_dump at some
> point. These objects aren't dumped in parallel yet, but still...

Yeah, I wasn't too happy with the static bools either.  However, each
function would need its own field in the struct, which seems like a
maintenance annoyance, plus a big hazard for future copy-and-paste
changes (ie, copy and paste the wrong flag name -> trouble).  Also
the Archive struct is shared between dump and restore cases, so
adding a dozen fields that are irrelevant for restore didn't feel
right.  So I'd like a better idea, but I'm not sure that that one
is better.

			regards, tom lane

  8. Re: Assorted improvements in pg_dump

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-10-25T20:42:23Z

    On 2021-Oct-25, Tom Lane wrote:

> Yeah, I wasn't too happy with the static bools either.  However, each
> function would need its own field in the struct, which seems like a
> maintenance annoyance, plus a big hazard for future copy-and-paste
> changes (ie, copy and paste the wrong flag name -> trouble).  Also
> the Archive struct is shared between dump and restore cases, so
> adding a dozen fields that are irrelevant for restore didn't feel
> right.  So I'd like a better idea, but I'm not sure that that one
> is better.

What about a separate struct passed from pg_dump's main() to the
functions that execute queries, containing a bunch of bools?  This'd
still have the problem that mindless copy and paste would cause a bug,
but I wonder if that isn't overstated: if you use the wrong flag,
pg_dump would fail as soon as you try to invoke your query when it
hasn't been prepared yet.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I'm impressed how quickly you are fixing this obscure issue. I came from 
MS SQL and it would be hard for me to put into words how much of a better job
you all are doing on [PostgreSQL]."
 Steve Midgley, http://archives.postgresql.org/pgsql-sql/2008-08/msg00000.php

  9. Re: Assorted improvements in pg_dump

    Andres Freund <andres@anarazel.de> — 2021-10-25T21:35:50Z

    Hi,

On 2021-10-25 16:02:34 -0400, Tom Lane wrote:
> So I'd like a better idea, but I'm not sure that that one is better.

I guess we could move the prepared-statement handling into a query execution
helper. That could then use a hashtable or something similar to check if a
certain prepared statement already exists. That'd then centrally be extensible
to deal with multiple connects etc.

Greetings,

Andres Freund

  10. Re: Assorted improvements in pg_dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-26T22:31:05Z

    Andres Freund <andres@anarazel.de> writes:
> I guess we could move the prepared-statement handling into a query execution
> helper. That could then use a hashtable or something similar to check if a
> certain prepared statement already exists. That'd then centrally be extensible
> to deal with multiple connects etc.

That seems like more mechanism than is warranted.  I tried it with a
simple array of booleans, and that seems like not too much of a mess;
see revised 0006 attached.

(0001-0005 are the same as before; including them just to satisfy
the cfbot.)

			regards, tom lane

  11. Re: Assorted improvements in pg_dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-03T21:33:42Z

    Here's an updated version of this patch set.  The only non-line-number
changes are

(1) in 0004, I dealt with the issue of not having unnest() in old branches
by bumping the minimum remote server version to 8.4.  Seeing that we seem
to have consensus in the other thread to push the minimum up to somewhere
around 9.2, I see no point in making this patch put in conditional code
that we'd shortly rip out again.

(2) I also added some comments to 0004 to hopefully address Justin's
confusion about string lengths.

I feel quite fortunate that a month's worth of commitfest hacking
didn't break any of these patches.  Unless someone intends to
review these more thoroughly than they already have, I'd like to
go ahead and push them.

			regards, tom lane

  12. AW: Assorted improvements in pg_dump

    Hans Buschmann <buschmann@nidsa.net> — 2021-12-07T08:05:32Z

    Hello Tom,

from your mail from 25.10.2021:

>0005 implements your suggestion of accounting for TOAST data while
>scheduling parallel dumps.  I realized while looking at that that
>there's a pre-existing bug, which this'd exacerbate: on machines
>with 32-bit off_t, dataLength can overflow.  Admittedly such machines
>are just about extinct in the wild, but we do still trouble to support
>the case.  So 0005 also includes code to check for overflow and clamp
>the result to INT_MAX blocks.

>Maybe we should back-patch 0005.  OTOH, how likely is it that anyone
>is wrangling tables exceeding 16TB on a machine with 32-bit off_t?
>Or that poor parallel dump scheduling would be a real problem in
>such a case?

I noticed that you patched master with all the improvements in pg_dump.

Did you change your mind about backpatching patch 0005 to fix the toast size matter?

It would be rather helpfull for handling existent user data in active branches.


On the matter of 32bit versions I think they are used only in much more little instances.

BTW the 32 bit build of postgres on Windows does not work any more with more modern tool sets (tested with VS2019 and VS2022) albeit not excluded explicitly in the docs. But no one complained yet (for a long time now...).

Thanks

Hans Buschmann

  13. Re: AW: Assorted improvements in pg_dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-07T16:18:17Z

    Hans Buschmann <buschmann@nidsa.net> writes:
> I noticed that you patched master with all the improvements in pg_dump.
> Did you change your mind about backpatching patch 0005 to fix the toast size matter?

I looked briefly at that and found that the patch would have to be
largely rewritten, because getTables() looks quite different in the
older branches.  I'm not really sufficiently excited about the point
to do that rewriting and re-testing.  I think that cases where the
old logic gets the scheduling badly wrong are probably rare.

			regards, tom lane