Horton

Thread

Commits

  1. Improve performance of binary_upgrade_set_pg_class_oids().

  2. Remove is_index parameter from binary_upgrade_set_pg_class_oids().

  3. pg_upgrade: Preserve relfilenodes and tablespace OIDs.

  4. pg_dump: minor performance improvements from eliminating sub-SELECTs.

  1. improve performance of pg_dump --binary-upgrade

    Nathan Bossart <nathandbossart@gmail.com> — 2024-04-18T04:17:12Z

    While examining pg_upgrade on a cluster with many tables (created with the
command in [0]), I noticed that a huge amount of pg_dump time goes towards
the binary_upgrade_set_pg_class_oids() function.  This function executes a
rather expensive query for a single row, and this function appears to be
called for most of the rows in pg_class.

The attached work-in-progress patch speeds up 'pg_dump --binary-upgrade'
for this case.  Instead of executing the query in every call to the
function, we can execute it once during the first call and store all the
required information in a sorted array that we can bsearch() in future
calls.  For the aformentioned test, pg_dump on my machine goes from ~2
minutes to ~18 seconds, which is much closer to the ~14 seconds it takes
without --binary-upgrade.

One downside of this approach is the memory usage.  This was more-or-less
the first approach that crossed my mind, so I wouldn't be surprised if
there's a better way.  I tried to keep the pg_dump output the same, but if
that isn't important, maybe we could dump all the pg_class OIDs at once
instead of calling binary_upgrade_set_pg_class_oids() for each one.

[0] https://postgr.es/m/3612876.1689443232%40sss.pgh.pa.us

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  2. Re: improve performance of pg_dump --binary-upgrade

    Corey Huinker <corey.huinker@gmail.com> — 2024-04-18T06:08:28Z

    >
> One downside of this approach is the memory usage.  This was more-or-less
>
>
Bar-napkin math tells me in a worst-case architecture and braindead byte
alignment, we'd burn 64 bytes per struct, so the 100K tables cited would be
about 6.25MB of memory.

The obvious low-memory alternative would be to make a prepared statement,
though that does nothing to cut down on the roundtrips.

I think this is a good trade off.

  3. Re: improve performance of pg_dump --binary-upgrade

    Michael Paquier <michael@paquier.xyz> — 2024-04-18T06:24:12Z

    On Thu, Apr 18, 2024 at 02:08:28AM -0400, Corey Huinker wrote:
> Bar-napkin math tells me in a worst-case architecture and braindead byte
> alignment, we'd burn 64 bytes per struct, so the 100K tables cited would be
> about 6.25MB of memory.
> 
> The obvious low-memory alternative would be to make a prepared statement,
> though that does nothing to cut down on the roundtrips.
> 
> I think this is a good trade off.

I've not checked the patch in details or tested it, but caching this
information to gain this speed sounds like a very good thing.
--
Michael

  4. Re: improve performance of pg_dump --binary-upgrade

    Daniel Gustafsson <daniel@yesql.se> — 2024-04-18T07:24:53Z

    > On 18 Apr 2024, at 06:17, Nathan Bossart <nathandbossart@gmail.com> wrote:

> The attached work-in-progress patch speeds up 'pg_dump --binary-upgrade'
> for this case.  Instead of executing the query in every call to the
> function, we can execute it once during the first call and store all the
> required information in a sorted array that we can bsearch() in future
> calls.

That does indeed seem like a saner approach.  Since we look up the relkind we
can also remove the is_index parameter to binary_upgrade_set_pg_class_oids
since we already know that without the caller telling us?

> One downside of this approach is the memory usage.

I'm not too worried about the worst-case performance of this.

> This was more-or-less
> the first approach that crossed my mind, so I wouldn't be surprised if
> there's a better way.  I tried to keep the pg_dump output the same, but if
> that isn't important, maybe we could dump all the pg_class OIDs at once
> instead of calling binary_upgrade_set_pg_class_oids() for each one.

Without changing the backend handling of the Oid's we can't really do that
AFAICT, the backend stores the Oid for the next call so it needs to be per
relation like now?

For Greenplum we moved this to the backend by first dumping all Oids which were
read into backend cache, and during relation creation the Oid to use was looked
up in the backend.  (This wasn't a performance change, it was to allow multiple
shared-nothing clusters to have a unified view of Oids, so I never benchmarked
it all that well.) The upside of that is that the magic Oid variables in the
backend can be removed, but it obviously adds slight overhead in others.

--
Daniel Gustafsson

  5. Re: improve performance of pg_dump --binary-upgrade

    Nathan Bossart <nathandbossart@gmail.com> — 2024-04-18T14:57:18Z

    On Thu, Apr 18, 2024 at 02:08:28AM -0400, Corey Huinker wrote:
> Bar-napkin math tells me in a worst-case architecture and braindead byte
> alignment, we'd burn 64 bytes per struct, so the 100K tables cited would be
> about 6.25MB of memory.

That doesn't seem too terrible.

> The obvious low-memory alternative would be to make a prepared statement,
> though that does nothing to cut down on the roundtrips.
> 
> I think this is a good trade off.

Cool.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  6. Re: improve performance of pg_dump --binary-upgrade

    Nathan Bossart <nathandbossart@gmail.com> — 2024-04-18T15:23:01Z

    On Thu, Apr 18, 2024 at 09:24:53AM +0200, Daniel Gustafsson wrote:
>> On 18 Apr 2024, at 06:17, Nathan Bossart <nathandbossart@gmail.com> wrote:
> 
>> The attached work-in-progress patch speeds up 'pg_dump --binary-upgrade'
>> for this case.  Instead of executing the query in every call to the
>> function, we can execute it once during the first call and store all the
>> required information in a sorted array that we can bsearch() in future
>> calls.
> 
> That does indeed seem like a saner approach.  Since we look up the relkind we
> can also remove the is_index parameter to binary_upgrade_set_pg_class_oids
> since we already know that without the caller telling us?

Yeah.  It looks like that's been possible since commit 9a974cb, so I can
write a prerequisite patch for this.

>> One downside of this approach is the memory usage.
> 
> I'm not too worried about the worst-case performance of this.

Cool.  That seems to be the general sentiment.

>> This was more-or-less
>> the first approach that crossed my mind, so I wouldn't be surprised if
>> there's a better way.  I tried to keep the pg_dump output the same, but if
>> that isn't important, maybe we could dump all the pg_class OIDs at once
>> instead of calling binary_upgrade_set_pg_class_oids() for each one.
> 
> Without changing the backend handling of the Oid's we can't really do that
> AFAICT, the backend stores the Oid for the next call so it needs to be per
> relation like now?

Right, this would require additional changes.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  7. Re: improve performance of pg_dump --binary-upgrade

    Nathan Bossart <nathandbossart@gmail.com> — 2024-04-18T20:28:13Z

    On Thu, Apr 18, 2024 at 10:23:01AM -0500, Nathan Bossart wrote:
> On Thu, Apr 18, 2024 at 09:24:53AM +0200, Daniel Gustafsson wrote:
>> That does indeed seem like a saner approach.  Since we look up the relkind we
>> can also remove the is_index parameter to binary_upgrade_set_pg_class_oids
>> since we already know that without the caller telling us?
> 
> Yeah.  It looks like that's been possible since commit 9a974cb, so I can
> write a prerequisite patch for this.

Here's a new patch set with this change.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  8. Re: improve performance of pg_dump --binary-upgrade

    Daniel Gustafsson <daniel@yesql.se> — 2024-04-18T20:33:08Z

    > On 18 Apr 2024, at 22:28, Nathan Bossart <nathandbossart@gmail.com> wrote:
> 
> On Thu, Apr 18, 2024 at 10:23:01AM -0500, Nathan Bossart wrote:
>> On Thu, Apr 18, 2024 at 09:24:53AM +0200, Daniel Gustafsson wrote:
>>> That does indeed seem like a saner approach.  Since we look up the relkind we
>>> can also remove the is_index parameter to binary_upgrade_set_pg_class_oids
>>> since we already know that without the caller telling us?
>> 
>> Yeah.  It looks like that's been possible since commit 9a974cb, so I can
>> write a prerequisite patch for this.
> 
> Here's a new patch set with this change.

From a read-through they look good, a very nice performance improvement in an
important path.  I think it would be nice with some comments on the
BinaryUpgradeClassOids struct (since the code using it is thousands of lines
away), and a comment on the if (oids == NULL) block explaining the caching.

--
Daniel Gustafsson

  9. Re: improve performance of pg_dump --binary-upgrade

    Nathan Bossart <nathandbossart@gmail.com> — 2024-04-18T21:19:24Z

    On Thu, Apr 18, 2024 at 10:33:08PM +0200, Daniel Gustafsson wrote:
> From a read-through they look good, a very nice performance improvement in an
> important path.  I think it would be nice with some comments on the
> BinaryUpgradeClassOids struct (since the code using it is thousands of lines
> away), and a comment on the if (oids == NULL) block explaining the caching.

Added.  Thanks for reviewing!  Unfortunately, this one will have to sit for
a couple months...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  10. Re: improve performance of pg_dump --binary-upgrade

    Nathan Bossart <nathandbossart@gmail.com> — 2024-04-22T19:01:03Z

    I noticed that there are some existing examples of this sort of thing in
pg_dump (e.g., commit d5e8930), so I adjusted the patch to match the
surrounding style a bit better.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  11. Re: improve performance of pg_dump --binary-upgrade

    Nathan Bossart <nathandbossart@gmail.com> — 2024-05-17T15:22:44Z

    rebased

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  12. Re: improve performance of pg_dump --binary-upgrade

    Nathan Bossart <nathandbossart@gmail.com> — 2024-07-03T19:28:44Z

    Committed.

-- 
nathan