Horton
Menu

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. Count individual SQL commands in pg_restore's --transaction-size mode.

  2. Reduce number of commands dumpTableSchema emits for binary upgrade.

  3. Invent --transaction-size option for pg_restore.

  4. Rearrange pg_dump's handling of large objects for better efficiency.

  5. Add temporal PRIMARY KEY and UNIQUE constraints

  6. Fix typo and case in messages

  1. Re: pg_upgrade failing for 200+ million Large Objects

    Tharakan, Robins <tharar@amazon.com> — 2021-03-08T11:02:04Z

    Thanks Peter.

The original email [1] had some more context that somehow didn't get
associated with this recent email. Apologies for any confusion.

In short, pg_resetxlog (and pg_resetwal) employs a magic constant [2] (for
both v9.6 as well as master) which seems to have been selected to force an
aggressive autovacuum as soon as the upgrade completes. Although that works
as planned, it narrows the window of Transaction IDs available for the
upgrade (before which XID wraparound protection kicks and aborts the
upgrade) to 146 Million.

Reducing this magic constant allows a larger XID window, which is what the
patch is trying to do. With the patch, I was able to upgrade a cluster with
500m Large Objects successfully (which otherwise reliably fails). In the
original email [1] I had also listed a few other possible workarounds, but
was unsure which would be a good direction to start working on.... thus this
patch to make a start.

Reference:
1) https://www.postgresql.org/message-id/12601596dbbc4c01b86b4ac4d2bd4d48%40
EX13D05UWC001.ant.amazon.com
2) https://github.com/postgres/postgres/blob/master/src/bin/pg_resetwal/pg_r
esetwal.c#L444

-
robins | tharar@ | syd12


> -----Original Message-----
> From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
> Sent: Monday, 8 March 2021 9:25 PM
> To: Tharakan, Robins <tharar@amazon.com>; pgsql-hackers@postgresql.org
> Subject: [EXTERNAL] [UNVERIFIED SENDER] Re: pg_upgrade failing for 200+
> million Large Objects
> 
> CAUTION: This email originated from outside of the organization. Do not
> click links or open attachments unless you can confirm the sender and
> know the content is safe.
> 
> 
> 
> On 07.03.21 09:43, Tharakan, Robins wrote:
> > Attached is a proof-of-concept patch that allows Postgres to perform
> > pg_upgrade if the instance has Millions of objects.
> >
> > It would be great if someone could take a look and see if this patch
> > is in the right direction. There are some pending tasks (such as
> > documentation / pg_resetxlog vs pg_resetwal related changes) but for
> > now, the patch helps remove a stalemate where if a Postgres instance
> > has a large number (accurately speaking 146+ Million) of Large
> > Objects, pg_upgrade fails. This is easily reproducible and besides
> > deleting Large Objects before upgrade, there is no other (apparent) way
> for pg_upgrade to complete.
> >
> > The patch (attached):
> > - Applies cleanly on REL9_6_STABLE -
> > c7a4fc3dd001646d5938687ad59ab84545d5d043
> > - 'make check' passes
> > - Allows the user to provide a constant via pg_upgrade command-line,
> > that overrides the 2 billion constant in pg_resetxlog [1] thereby
> > increasing the (window of) Transaction IDs available for pg_upgrade to
> complete.
> 
> Could you explain what your analysis of the problem is and why this patch
> (might) fix it?
> 
> Right now, all I see here is, pass a big number via a command-line option
> and hope it works.

  2. Re: pg_upgrade failing for 200+ million Large Objects

    Magnus Hagander <magnus@hagander.net> — 2021-03-08T12:33:58Z

    On Mon, Mar 8, 2021 at 12:02 PM Tharakan, Robins <tharar@amazon.com> wrote:
>
> Thanks Peter.
>
> The original email [1] had some more context that somehow didn't get
> associated with this recent email. Apologies for any confusion.

Please take a look at your email configuration -- all your emails are
lacking both References and In-reply-to headers, so every email starts
a new thread, both for each reader and in the archives. It seems quite
broken. It makes it very hard to follow.


> In short, pg_resetxlog (and pg_resetwal) employs a magic constant [2] (for
> both v9.6 as well as master) which seems to have been selected to force an
> aggressive autovacuum as soon as the upgrade completes. Although that works
> as planned, it narrows the window of Transaction IDs available for the
> upgrade (before which XID wraparound protection kicks and aborts the
> upgrade) to 146 Million.
>
> Reducing this magic constant allows a larger XID window, which is what the
> patch is trying to do. With the patch, I was able to upgrade a cluster with
> 500m Large Objects successfully (which otherwise reliably fails). In the
> original email [1] I had also listed a few other possible workarounds, but
> was unsure which would be a good direction to start working on.... thus this
> patch to make a start.

This still seems to just fix the symptoms and not the actual problem.

What part of the pg_upgrade process is it that actually burns through
that many transactions?

Without looking, I would guess it's the schema reload using
pg_dump/pg_restore and not actually pg_upgrade itself. This is a known
issue in pg_dump/pg_restore. And if that is the case -- perhaps just
running all of those in a single transaction would be a better choice?
One could argue it's still not a proper fix, because we'd still have a
huge memory usage etc, but it would then only burn 1 xid instead of
500M...

AFAICT at a quick check, pg_dump in binary upgrade mode emits one
lo_create() and one ALTER ... OWNER TO for each large object - so with
500M large objects that would be a billion statements, and thus a
billion xids. And without checking, I'm fairly sure it doesn't load in
a single transaction...

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

  3. Re: pg_upgrade failing for 200+ million Large Objects

    Robins Tharakan <tharakan@gmail.com> — 2021-03-08T14:13:02Z

    Hi Magnus,

On Mon, 8 Mar 2021 at 23:34, Magnus Hagander <magnus@hagander.net> wrote:

> AFAICT at a quick check, pg_dump in binary upgrade mode emits one

lo_create() and one ALTER ... OWNER TO for each large object - so with
> 500M large objects that would be a billion statements, and thus a
> billion xids. And without checking, I'm fairly sure it doesn't load in
> a single transaction...
>

Your assumptions are pretty much correct.

The issue isn't with pg_upgrade itself. During pg_restore, each Large
Object (and separately each ALTER LARGE OBJECT OWNER TO) consumes an XID
each. For background, that's the reason the v9.5 production instance I was
reviewing, was unable to process more than 73 Million large objects since
each object required a CREATE + ALTER. (To clarify, 73 million = (2^31 - 2
billion magic constant - 1 Million wraparound protection) / 2)


Without looking, I would guess it's the schema reload using
> pg_dump/pg_restore and not actually pg_upgrade itself. This is a known
> issue in pg_dump/pg_restore. And if that is the case -- perhaps just
> running all of those in a single transaction would be a better choice?
> One could argue it's still not a proper fix, because we'd still have a
> huge memory usage etc, but it would then only burn 1 xid instead of
> 500M...
>
(I hope I am not missing something but) When I tried to force pg_restore to
use a single transaction (by hacking pg_upgrade's pg_restore call to use
--single-transaction), it too failed owing to being unable to lock so many
objects in a single transaction.


This still seems to just fix the symptoms and not the actual problem.
>

I agree that the patch doesn't address the root-cause, but it did get the
upgrade to complete on a test-setup. Do you think that (instead of all
objects) batching multiple Large Objects in a single transaction (and
allowing the caller to size that batch via command line) would be a good /
acceptable idea here?

Please take a look at your email configuration -- all your emails are
> lacking both References and In-reply-to headers.
>

Thanks for highlighting the cause here. Hopefully switching mail clients
would help.
-
Robins Tharakan

  4. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-08T16:33:02Z

    Robins Tharakan <tharakan@gmail.com> writes:
> On Mon, 8 Mar 2021 at 23:34, Magnus Hagander <magnus@hagander.net> wrote:
>> Without looking, I would guess it's the schema reload using
>> pg_dump/pg_restore and not actually pg_upgrade itself. This is a known
>> issue in pg_dump/pg_restore. And if that is the case -- perhaps just
>> running all of those in a single transaction would be a better choice?
>> One could argue it's still not a proper fix, because we'd still have a
>> huge memory usage etc, but it would then only burn 1 xid instead of
>> 500M...

> (I hope I am not missing something but) When I tried to force pg_restore to
> use a single transaction (by hacking pg_upgrade's pg_restore call to use
> --single-transaction), it too failed owing to being unable to lock so many
> objects in a single transaction.

It does seem that --single-transaction is a better idea than fiddling with
the transaction wraparound parameters, since the latter is just going to
put off the onset of trouble.  However, we'd have to do something about
the lock consumption.  Would it be sane to have the backend not bother to
take any locks in binary-upgrade mode?

			regards, tom lane

  5. Re: pg_upgrade failing for 200+ million Large Objects

    Magnus Hagander <magnus@hagander.net> — 2021-03-08T16:35:56Z

    On Mon, Mar 8, 2021 at 5:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Robins Tharakan <tharakan@gmail.com> writes:
> > On Mon, 8 Mar 2021 at 23:34, Magnus Hagander <magnus@hagander.net> wrote:
> >> Without looking, I would guess it's the schema reload using
> >> pg_dump/pg_restore and not actually pg_upgrade itself. This is a known
> >> issue in pg_dump/pg_restore. And if that is the case -- perhaps just
> >> running all of those in a single transaction would be a better choice?
> >> One could argue it's still not a proper fix, because we'd still have a
> >> huge memory usage etc, but it would then only burn 1 xid instead of
> >> 500M...
>
> > (I hope I am not missing something but) When I tried to force pg_restore to
> > use a single transaction (by hacking pg_upgrade's pg_restore call to use
> > --single-transaction), it too failed owing to being unable to lock so many
> > objects in a single transaction.
>
> It does seem that --single-transaction is a better idea than fiddling with
> the transaction wraparound parameters, since the latter is just going to
> put off the onset of trouble.  However, we'd have to do something about
> the lock consumption.  Would it be sane to have the backend not bother to
> take any locks in binary-upgrade mode?

I believe the problem occurs when writing them rather than when
reading them, and I don't think we have a binary upgrade mode there.

We could invent one of course. Another option might be to exclusively
lock pg_largeobject, and just say that if you do that, we don't have
to lock the individual objects (ever)?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

  6. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-08T16:58:39Z

    Magnus Hagander <magnus@hagander.net> writes:
> On Mon, Mar 8, 2021 at 5:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It does seem that --single-transaction is a better idea than fiddling with
>> the transaction wraparound parameters, since the latter is just going to
>> put off the onset of trouble.  However, we'd have to do something about
>> the lock consumption.  Would it be sane to have the backend not bother to
>> take any locks in binary-upgrade mode?

> I believe the problem occurs when writing them rather than when
> reading them, and I don't think we have a binary upgrade mode there.

You're confusing pg_dump's --binary-upgrade switch (indeed applied on
the dumping side) with the backend's -b switch (IsBinaryUpgrade,
applied on the restoring side).

> We could invent one of course. Another option might be to exclusively
> lock pg_largeobject, and just say that if you do that, we don't have
> to lock the individual objects (ever)?

What was in the back of my mind is that we've sometimes seen complaints
about too many locks needed to dump or restore a database with $MANY
tables; so the large-object case seems like just a special case.

The answer up to now has been "raise max_locks_per_transaction enough
so you don't see the failure".  Having now consumed a little more
caffeine, I remember that that works in pg_upgrade scenarios too,
since the user can fiddle with the target cluster's postgresql.conf
before starting pg_upgrade.

So it seems like the path of least resistance is

(a) make pg_upgrade use --single-transaction when calling pg_restore

(b) document (better) how to get around too-many-locks failures.

			regards, tom lane

  7. Re: pg_upgrade failing for 200+ million Large Objects

    Magnus Hagander <magnus@hagander.net> — 2021-03-08T17:18:12Z

    On Mon, Mar 8, 2021 at 5:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Magnus Hagander <magnus@hagander.net> writes:
> > On Mon, Mar 8, 2021 at 5:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> It does seem that --single-transaction is a better idea than fiddling with
> >> the transaction wraparound parameters, since the latter is just going to
> >> put off the onset of trouble.  However, we'd have to do something about
> >> the lock consumption.  Would it be sane to have the backend not bother to
> >> take any locks in binary-upgrade mode?
>
> > I believe the problem occurs when writing them rather than when
> > reading them, and I don't think we have a binary upgrade mode there.
>
> You're confusing pg_dump's --binary-upgrade switch (indeed applied on
> the dumping side) with the backend's -b switch (IsBinaryUpgrade,
> applied on the restoring side).

Ah. Yes, I am.


> > We could invent one of course. Another option might be to exclusively
> > lock pg_largeobject, and just say that if you do that, we don't have
> > to lock the individual objects (ever)?
>
> What was in the back of my mind is that we've sometimes seen complaints
> about too many locks needed to dump or restore a database with $MANY
> tables; so the large-object case seems like just a special case.

It is -- but I guess it's more likely to have 100M large objects than
to have 100M tables. (and the cutoff point comes a lot earlier than
100M). But the fundamental onei s the same.


> The answer up to now has been "raise max_locks_per_transaction enough
> so you don't see the failure".  Having now consumed a little more
> caffeine, I remember that that works in pg_upgrade scenarios too,
> since the user can fiddle with the target cluster's postgresql.conf
> before starting pg_upgrade.
>
> So it seems like the path of least resistance is
>
> (a) make pg_upgrade use --single-transaction when calling pg_restore
>
> (b) document (better) how to get around too-many-locks failures.

Agreed. Certainly seems like a better path forward than arbitrarily
pushing the limit on number of transactions which just postpones the
problem.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

  8. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-20T04:39:10Z

    On 3/8/21 11:58 AM, Tom Lane wrote:
> The answer up to now has been "raise max_locks_per_transaction enough
> so you don't see the failure".  Having now consumed a little more
> caffeine, I remember that that works in pg_upgrade scenarios too,
> since the user can fiddle with the target cluster's postgresql.conf
> before starting pg_upgrade.
> 
> So it seems like the path of least resistance is
> 
> (a) make pg_upgrade use --single-transaction when calling pg_restore
> 
> (b) document (better) how to get around too-many-locks failures.

That would first require to fix how pg_upgrade is creating the 
databases. It uses "pg_restore --create", which is mutually exclusive 
with --single-transaction because we cannot create a database inside of 
a transaction. On the way pg_upgrade also mangles the pg_database.datdba 
(all databases are owned by postgres after an upgrade; will submit a 
separate patch for that as I consider that a bug by itself).

All that aside, the entire approach doesn't scale.

In a hacked up pg_upgrade that does "createdb" first before calling 
pg_upgrade with --single-transaction. I can upgrade 1M large objects with
     max_locks_per_transaction = 5300
     max_connectinons=100
which contradicts the docs. Need to find out where that math went off 
the rails because that config should only have room for 530,000 locks, 
not 1M. The same test fails with max_locks_per_transaction = 5200.

But this would mean that one has to modify the postgresql.conf to 
something like 530,000 max_locks_per_transaction at 100 max_connections 
in order to actually run a successful upgrade of 100M large objects. 
This config requires 26GB of memory just for locks. Add to that the 
memory pg_restore needs to load the entire TOC before even restoring a 
single object.

Not going to work. But tests are still ongoing ...


Regards, Jan


-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  9. Re: pg_upgrade failing for 200+ million Large Objects

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-20T15:17:41Z

    On 3/20/21 12:39 AM, Jan Wieck wrote:
> On 3/8/21 11:58 AM, Tom Lane wrote:
>> The answer up to now has been "raise max_locks_per_transaction enough
>> so you don't see the failure".  Having now consumed a little more
>> caffeine, I remember that that works in pg_upgrade scenarios too,
>> since the user can fiddle with the target cluster's postgresql.conf
>> before starting pg_upgrade.
>>
>> So it seems like the path of least resistance is
>>
>> (a) make pg_upgrade use --single-transaction when calling pg_restore
>>
>> (b) document (better) how to get around too-many-locks failures.
>
> That would first require to fix how pg_upgrade is creating the
> databases. It uses "pg_restore --create", which is mutually exclusive
> with --single-transaction because we cannot create a database inside
> of a transaction. On the way pg_upgrade also mangles the
> pg_database.datdba (all databases are owned by postgres after an
> upgrade; will submit a separate patch for that as I consider that a
> bug by itself).
>
> All that aside, the entire approach doesn't scale.
>
> In a hacked up pg_upgrade that does "createdb" first before calling
> pg_upgrade with --single-transaction. I can upgrade 1M large objects with
>     max_locks_per_transaction = 5300
>     max_connectinons=100
> which contradicts the docs. Need to find out where that math went off
> the rails because that config should only have room for 530,000 locks,
> not 1M. The same test fails with max_locks_per_transaction = 5200.
>
> But this would mean that one has to modify the postgresql.conf to
> something like 530,000 max_locks_per_transaction at 100
> max_connections in order to actually run a successful upgrade of 100M
> large objects. This config requires 26GB of memory just for locks. Add
> to that the memory pg_restore needs to load the entire TOC before even
> restoring a single object.
>
> Not going to work. But tests are still ongoing ...



I thought Tom's suggestion upthread:


> Would it be sane to have the backend not bother to
> take any locks in binary-upgrade mode?


was interesting. Could we do that on the restore side? After all, what
are we locking against in binary upgrade mode?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  10. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-20T15:23:19Z

    Jan Wieck <jan@wi3ck.info> writes:
> On 3/8/21 11:58 AM, Tom Lane wrote:
>> So it seems like the path of least resistance is
>> (a) make pg_upgrade use --single-transaction when calling pg_restore
>> (b) document (better) how to get around too-many-locks failures.

> That would first require to fix how pg_upgrade is creating the 
> databases. It uses "pg_restore --create", which is mutually exclusive 
> with --single-transaction because we cannot create a database inside of 
> a transaction.

Ugh.

> All that aside, the entire approach doesn't scale.

Yeah, agreed.  When we gave large objects individual ownership and ACL
info, it was argued that pg_dump could afford to treat each one as a
separate TOC entry because "you wouldn't have that many of them, if
they're large".  The limits of that approach were obvious even at the
time, and I think now we're starting to see people for whom it really
doesn't work.

I wonder if pg_dump could improve matters cheaply by aggregating the
large objects by owner and ACL contents.  That is, do

select distinct lomowner, lomacl from pg_largeobject_metadata;

and make just *one* BLOB TOC entry for each result.  Then dump out
all the matching blobs under that heading.

A possible objection is that it'd reduce the ability to restore blobs
selectively, so maybe we'd need to make it optional.

Of course, that just reduces the memory consumption on the client
side; it does nothing for the locks.  Can we get away with releasing the
lock immediately after doing an ALTER OWNER or GRANT/REVOKE on a blob?

			regards, tom lane

  11. Re: pg_upgrade failing for 200+ million Large Objects

    Bruce Momjian <bruce@momjian.us> — 2021-03-20T16:45:36Z

    On Sat, Mar 20, 2021 at 11:23:19AM -0400, Tom Lane wrote:
> I wonder if pg_dump could improve matters cheaply by aggregating the
> large objects by owner and ACL contents.  That is, do
> 
> select distinct lomowner, lomacl from pg_largeobject_metadata;
> 
> and make just *one* BLOB TOC entry for each result.  Then dump out
> all the matching blobs under that heading.
> 
> A possible objection is that it'd reduce the ability to restore blobs
> selectively, so maybe we'd need to make it optional.
> 
> Of course, that just reduces the memory consumption on the client
> side; it does nothing for the locks.  Can we get away with releasing the
> lock immediately after doing an ALTER OWNER or GRANT/REVOKE on a blob?

Well, in pg_upgrade mode you can, since there are no other cluster
users, but you might be asking for general pg_dump usage.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

  12. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-20T16:53:40Z

    Bruce Momjian <bruce@momjian.us> writes:
> On Sat, Mar 20, 2021 at 11:23:19AM -0400, Tom Lane wrote:
>> Of course, that just reduces the memory consumption on the client
>> side; it does nothing for the locks.  Can we get away with releasing the
>> lock immediately after doing an ALTER OWNER or GRANT/REVOKE on a blob?

> Well, in pg_upgrade mode you can, since there are no other cluster
> users, but you might be asking for general pg_dump usage.

Yeah, this problem doesn't only affect pg_upgrade scenarios, so it'd
really be better to find a way that isn't dependent on binary-upgrade
mode.

			regards, tom lane

  13. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-20T16:55:24Z

    On 3/20/21 11:23 AM, Tom Lane wrote:
> Jan Wieck <jan@wi3ck.info> writes:
>> All that aside, the entire approach doesn't scale.
> 
> Yeah, agreed.  When we gave large objects individual ownership and ACL
> info, it was argued that pg_dump could afford to treat each one as a
> separate TOC entry because "you wouldn't have that many of them, if
> they're large".  The limits of that approach were obvious even at the
> time, and I think now we're starting to see people for whom it really
> doesn't work.

It actually looks more like some users have millions of "small objects". 
I am still wondering where that is coming from and why they are abusing 
LOs in that way, but that is more out of curiosity. Fact is that they 
are out there and that they cannot upgrade from their 9.5 databases, 
which are now past EOL.

> 
> I wonder if pg_dump could improve matters cheaply by aggregating the
> large objects by owner and ACL contents.  That is, do
> 
> select distinct lomowner, lomacl from pg_largeobject_metadata;
> 
> and make just *one* BLOB TOC entry for each result.  Then dump out
> all the matching blobs under that heading.

What I am currently experimenting with is moving the BLOB TOC entries 
into the parallel data phase of pg_restore "when doing binary upgrade". 
It seems to scale nicely with the number of cores in the system. In 
addition to that have options for pg_upgrade and pg_restore that cause 
the restore to batch them into transactions, like 10,000 objects at a 
time. There was a separate thread for that but I guess it is better to 
keep it all together here now.

> 
> A possible objection is that it'd reduce the ability to restore blobs
> selectively, so maybe we'd need to make it optional.

I fully intend to make all this into new "options". I am afraid that 
there is no one-size-fits-all solution here.
> 
> Of course, that just reduces the memory consumption on the client
> side; it does nothing for the locks.  Can we get away with releasing the
> lock immediately after doing an ALTER OWNER or GRANT/REVOKE on a blob?

I'm not very fond of the idea going lockless when at the same time 
trying to parallelize the restore phase. That can lead to really nasty 
race conditions. For now I'm aiming at batches in transactions.


Regards, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  14. Re: pg_upgrade failing for 200+ million Large Objects

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-21T11:47:12Z

    On 3/20/21 12:55 PM, Jan Wieck wrote:
> On 3/20/21 11:23 AM, Tom Lane wrote:
>> Jan Wieck <jan@wi3ck.info> writes:
>>> All that aside, the entire approach doesn't scale.
>>
>> Yeah, agreed.  When we gave large objects individual ownership and ACL
>> info, it was argued that pg_dump could afford to treat each one as a
>> separate TOC entry because "you wouldn't have that many of them, if
>> they're large".  The limits of that approach were obvious even at the
>> time, and I think now we're starting to see people for whom it really
>> doesn't work.
>
> It actually looks more like some users have millions of "small
> objects". I am still wondering where that is coming from and why they
> are abusing LOs in that way, but that is more out of curiosity. Fact
> is that they are out there and that they cannot upgrade from their 9.5
> databases, which are now past EOL.
>

One possible (probable?) source is the JDBC driver, which currently
treats all Blobs (and Clobs, for that matter) as LOs. I'm working on
improving that some: <https://github.com/pgjdbc/pgjdbc/pull/2093>


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  15. Fix pg_upgrade to preserve datdba (was: Re: pg_upgrade failing for 200+ million Large Objects)

    Jan Wieck <jan@wi3ck.info> — 2021-03-21T16:50:46Z

    On 3/20/21 12:39 AM, Jan Wieck wrote:
> On the way pg_upgrade also mangles the pg_database.datdba
> (all databases are owned by postgres after an upgrade; will submit a
> separate patch for that as I consider that a bug by itself).

Patch attached.


Regards, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  16. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-21T16:56:02Z

    On 3/21/21 7:47 AM, Andrew Dunstan wrote:
> One possible (probable?) source is the JDBC driver, which currently
> treats all Blobs (and Clobs, for that matter) as LOs. I'm working on
> improving that some: <https://github.com/pgjdbc/pgjdbc/pull/2093>

You mean the user is using OID columns pointing to large objects and the 
JDBC driver is mapping those for streaming operations?

Yeah, that would explain a lot.


Thanks, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  17. Re: Fix pg_upgrade to preserve datdba (was: Re: pg_upgrade failing for 200+ million Large Objects)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-21T16:57:12Z

    Jan Wieck <jan@wi3ck.info> writes:
> On 3/20/21 12:39 AM, Jan Wieck wrote:
>> On the way pg_upgrade also mangles the pg_database.datdba
>> (all databases are owned by postgres after an upgrade; will submit a
>> separate patch for that as I consider that a bug by itself).

> Patch attached.

Hmm, doesn't this lose all *other* database-level properties?

I think maybe what we have here is a bug in pg_restore, its
--create switch ought to be trying to update the database's
ownership.

			regards, tom lane

  18. Re: Fix pg_upgrade to preserve datdba

    Jan Wieck <jan@wi3ck.info> — 2021-03-21T17:15:54Z

    On 3/21/21 12:57 PM, Tom Lane wrote:
> Jan Wieck <jan@wi3ck.info> writes:
>> On 3/20/21 12:39 AM, Jan Wieck wrote:
>>> On the way pg_upgrade also mangles the pg_database.datdba
>>> (all databases are owned by postgres after an upgrade; will submit a
>>> separate patch for that as I consider that a bug by itself).
> 
>> Patch attached.
> 
> Hmm, doesn't this lose all *other* database-level properties?
> 
> I think maybe what we have here is a bug in pg_restore, its
> --create switch ought to be trying to update the database's
> ownership.

Possibly. I didn't look into that route.


Regards, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  19. Re: Fix pg_upgrade to preserve datdba

    Jan Wieck <jan@wi3ck.info> — 2021-03-21T17:50:45Z

    On 3/21/21 1:15 PM, Jan Wieck wrote:
> On 3/21/21 12:57 PM, Tom Lane wrote:
>> Jan Wieck <jan@wi3ck.info> writes:
>>> On 3/20/21 12:39 AM, Jan Wieck wrote:
>>>> On the way pg_upgrade also mangles the pg_database.datdba
>>>> (all databases are owned by postgres after an upgrade; will submit a
>>>> separate patch for that as I consider that a bug by itself).
>> 
>>> Patch attached.
>> 
>> Hmm, doesn't this lose all *other* database-level properties?
>> 
>> I think maybe what we have here is a bug in pg_restore, its
>> --create switch ought to be trying to update the database's
>> ownership.
> 
> Possibly. I didn't look into that route.

Thanks for that. I like this patch a lot better.


Regards, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  20. Re: pg_upgrade failing for 200+ million Large Objects

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-21T18:18:59Z

    On 3/21/21 12:56 PM, Jan Wieck wrote:
> On 3/21/21 7:47 AM, Andrew Dunstan wrote:
>> One possible (probable?) source is the JDBC driver, which currently
>> treats all Blobs (and Clobs, for that matter) as LOs. I'm working on
>> improving that some: <https://github.com/pgjdbc/pgjdbc/pull/2093>
>
> You mean the user is using OID columns pointing to large objects and
> the JDBC driver is mapping those for streaming operations?
>
> Yeah, that would explain a lot.
>
>
>


Probably in most cases the database is designed by Hibernate, and the
front end programmers know nothing at all of Oids or LOs, they just ask
for and get a Blob.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  21. Re: Fix pg_upgrade to preserve datdba

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-21T18:23:38Z

    Jan Wieck <jan@wi3ck.info> writes:
>> On 3/21/21 12:57 PM, Tom Lane wrote:
>>> I think maybe what we have here is a bug in pg_restore, its
>>> --create switch ought to be trying to update the database's
>>> ownership.

> Thanks for that. I like this patch a lot better.

Needs a little more work than that --- we should allow it to respond
to the --no-owner switch, for example.  But I think likely we can do
it where other object ownership is handled.  I'll look in a bit.

			regards, tom lane

  22. Re: Fix pg_upgrade to preserve datdba

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-21T18:34:58Z

    I wrote:
> Needs a little more work than that --- we should allow it to respond
> to the --no-owner switch, for example.  But I think likely we can do
> it where other object ownership is handled.  I'll look in a bit.

Actually ... said code already DOES do that, so now I'm confused.
I tried

regression=# create user joe;
CREATE ROLE
regression=# create database joe owner joe;
CREATE DATABASE
regression=# \q
$ pg_dump -Fc joe >joe.dump
$ pg_restore --create -f - joe.dump | more

and I see

--
-- Name: joe; Type: DATABASE; Schema: -; Owner: joe
--

CREATE DATABASE joe WITH TEMPLATE = template0 ENCODING = 'SQL_ASCII' LOCALE = 'C';


ALTER DATABASE joe OWNER TO joe;

so at least in this case it's doing the right thing.  We need a bit
more detail about the context in which it's doing the wrong thing
for you.

			regards, tom lane

  23. Re: Fix pg_upgrade to preserve datdba

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-21T19:29:10Z

    I wrote:
> ... so at least in this case it's doing the right thing.  We need a bit
> more detail about the context in which it's doing the wrong thing
> for you.

Just to cross-check, I tried modifying pg_upgrade's regression test
as attached, and it still passes.  (And inspection of the leftover
dump2.sql file verifies that the database ownership was correct.)
So I'm not sure what's up here.

			regards, tom lane

  24. Re: Fix pg_upgrade to preserve datdba

    Jan Wieck <jan@wi3ck.info> — 2021-03-21T19:36:51Z

    On 3/21/21 2:34 PM, Tom Lane wrote:
> and I see
> 
> --
> -- Name: joe; Type: DATABASE; Schema: -; Owner: joe
> --
> 
> CREATE DATABASE joe WITH TEMPLATE = template0 ENCODING = 'SQL_ASCII' LOCALE = 'C';
> 
> 
> ALTER DATABASE joe OWNER TO joe;
> 
> so at least in this case it's doing the right thing.  We need a bit
> more detail about the context in which it's doing the wrong thing
> for you.

After moving all of this to a pristine postgresql.org based repo I see 
the same. My best guess at this point is that the permission hoops, that 
RDS and Aurora PostgreSQL are jumping through, was messing with this. 
But that has nothing to do with the actual topic.

So let's focus on the actual problem of running out of XIDs and memory 
while doing the upgrade involving millions of small large objects.


Regards, Jan


-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  25. Re: Fix pg_upgrade to preserve datdba

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-21T19:56:14Z

    Jan Wieck <jan@wi3ck.info> writes:
> So let's focus on the actual problem of running out of XIDs and memory 
> while doing the upgrade involving millions of small large objects.

Right.  So as far as --single-transaction vs. --create goes, that's
mostly a definitional problem.  As long as the contents of a DB are
restored in one transaction, it's not gonna matter if we eat one or
two more XIDs while creating the DB itself.  So we could either
relax pg_restore's complaint, or invent a different switch that's
named to acknowledge that it's not really only one transaction.

That still leaves us with the lots-o-locks problem.  However, once
we've crossed the Rubicon of "it's not really only one transaction",
you could imagine that the switch is "--fewer-transactions", and the
idea is for pg_restore to commit after every (say) 100000 operations.
That would both bound its lock requirements and greatly cut its XID
consumption.

The work you described sounded like it could fit into that paradigm,
with the additional ability to run some parallel restore tasks
that are each consuming a bounded number of locks.

			regards, tom lane

  26. Re: Fix pg_upgrade to preserve datdba

    Jan Wieck <jan@wi3ck.info> — 2021-03-22T18:07:34Z

    On 3/21/21 3:56 PM, Tom Lane wrote:
> Jan Wieck <jan@wi3ck.info> writes:
>> So let's focus on the actual problem of running out of XIDs and memory 
>> while doing the upgrade involving millions of small large objects.
> 
> Right.  So as far as --single-transaction vs. --create goes, that's
> mostly a definitional problem.  As long as the contents of a DB are
> restored in one transaction, it's not gonna matter if we eat one or
> two more XIDs while creating the DB itself.  So we could either
> relax pg_restore's complaint, or invent a different switch that's
> named to acknowledge that it's not really only one transaction.
> 
> That still leaves us with the lots-o-locks problem.  However, once
> we've crossed the Rubicon of "it's not really only one transaction",
> you could imagine that the switch is "--fewer-transactions", and the
> idea is for pg_restore to commit after every (say) 100000 operations.
> That would both bound its lock requirements and greatly cut its XID
> consumption.

It leaves us with three things.

1) tremendous amounts of locks
2) tremendous amounts of memory needed
3) taking forever because it is single threaded.

I created a pathological case here on a VM with 24GB of RAM, 80GB of 
SWAP sitting on NVME. The database has 20 million large objects, each of 
which has 2 GRANTS, 1 COMMENT and 1 SECURITY LABEL (dummy). Each LO only 
contains a string "large object <oid>", so the whole database in 9.5 is 
about 15GB in size.

A stock pg_upgrade to version 14devel using --link takes about 15 hours. 
This is partly because the pg_dump and pg_restore both grow to something 
like 50GB+ to hold the TOC. Which sounds out of touch considering that 
the entire system catalog on disk is less than 15GB. But aside from the 
ridiculous amount of swapping, the whole thing also suffers from 
consuming about 80 million transactions and apparently having just as 
many network round trips with a single client.

> 
> The work you described sounded like it could fit into that paradigm,
> with the additional ability to run some parallel restore tasks
> that are each consuming a bounded number of locks.

I have attached a POC patch that implements two new options for pg_upgrade.

   --restore-jobs=NUM             --jobs parameter passed to pg_restore
   --restore-blob-batch-size=NUM  number of blobs restored in one xact

It does a bit more than just that. It rearranges the way large objects 
are dumped so that most of the commands are all in one TOC entry and the 
entry is emitted into SECTION_DATA when in binary upgrade mode (which 
guarantees that there isn't any actual BLOB data in the dump). This 
greatly reduces the number of network round trips and when using 8 
parallel restore jobs, almost saturates the 4-core VM. Reducing the 
number of TOC entries also reduces the total virtual memory need of 
pg_restore to 15G, so there is a lot less swapping going on.

It cuts down the pg_upgrade time from 15 hours to 1.5 hours. In that run 
I used --restore-jobs=8 and --restore-blob-batch-size=10000 (with a 
max_locks_per_transaction=12000).

As said, this isn't a "one size fits all" solution. The pg_upgrade 
parameters for --jobs and --restore-jobs will really depend on the 
situation. Hundreds of small databases want --jobs, but one database 
with millions of large objects wants --restore-jobs.


Regards, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  27. Re: pg_upgrade failing for 200+ million Large Objects

    Zhihong Yu <zyu@yugabyte.com> — 2021-03-22T21:36:38Z

    >
> Hi,
>
w.r.t. pg_upgrade_improvements.v2.diff.

+       blobBatchCount = 0;
+       blobInXact = false;

The count and bool flag are always reset in tandem. It seems
variable blobInXact is not needed.

Cheers

  28. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-22T23:18:33Z

    On 3/22/21 5:36 PM, Zhihong Yu wrote:
>     Hi,
> 
> w.r.t. pg_upgrade_improvements.v2.diff.
> 
> +       blobBatchCount = 0;
> +       blobInXact = false;
> 
> The count and bool flag are always reset in tandem. It seems 
> variable blobInXact is not needed.

You are right. I will fix that.


Thanks, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  29. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-23T12:51:32Z

    On 3/22/21 7:18 PM, Jan Wieck wrote:
> On 3/22/21 5:36 PM, Zhihong Yu wrote:
>>     Hi,
>> 
>> w.r.t. pg_upgrade_improvements.v2.diff.
>> 
>> +       blobBatchCount = 0;
>> +       blobInXact = false;
>> 
>> The count and bool flag are always reset in tandem. It seems 
>> variable blobInXact is not needed.
> 
> You are right. I will fix that.

New patch v3 attached.


Thanks, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  30. Re: pg_upgrade failing for 200+ million Large Objects

    Bruce Momjian <bruce@momjian.us> — 2021-03-23T14:56:28Z

    On Tue, Mar 23, 2021 at 08:51:32AM -0400, Jan Wieck wrote:
> On 3/22/21 7:18 PM, Jan Wieck wrote:
> > On 3/22/21 5:36 PM, Zhihong Yu wrote:
> > >     Hi,
> > > 
> > > w.r.t. pg_upgrade_improvements.v2.diff.
> > > 
> > > +       blobBatchCount = 0;
> > > +       blobInXact = false;
> > > 
> > > The count and bool flag are always reset in tandem. It seems
> > > variable blobInXact is not needed.
> > 
> > You are right. I will fix that.
> 
> New patch v3 attached.

Would it be better to allow pg_upgrade to pass arbitrary arguments to
pg_restore, instead of just these specific ones?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

  31. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-23T17:25:15Z

    On 3/23/21 10:56 AM, Bruce Momjian wrote:
> On Tue, Mar 23, 2021 at 08:51:32AM -0400, Jan Wieck wrote:
>> On 3/22/21 7:18 PM, Jan Wieck wrote:
>> > On 3/22/21 5:36 PM, Zhihong Yu wrote:
>> > >     Hi,
>> > > 
>> > > w.r.t. pg_upgrade_improvements.v2.diff.
>> > > 
>> > > +       blobBatchCount = 0;
>> > > +       blobInXact = false;
>> > > 
>> > > The count and bool flag are always reset in tandem. It seems
>> > > variable blobInXact is not needed.
>> > 
>> > You are right. I will fix that.
>> 
>> New patch v3 attached.
> 
> Would it be better to allow pg_upgrade to pass arbitrary arguments to
> pg_restore, instead of just these specific ones?
> 

That would mean arbitrary parameters to pg_dump as well as pg_restore. 
But yes, that would probably be better in the long run.

Any suggestion as to how that would actually look like? Unfortunately 
pg_restore has -[dDoOr] already used, so it doesn't look like there will 
be any naturally intelligible short options for that.


Regards, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  32. Re: pg_upgrade failing for 200+ million Large Objects

    Bruce Momjian <bruce@momjian.us> — 2021-03-23T18:06:46Z

    On Tue, Mar 23, 2021 at 01:25:15PM -0400, Jan Wieck wrote:
> On 3/23/21 10:56 AM, Bruce Momjian wrote:
> > Would it be better to allow pg_upgrade to pass arbitrary arguments to
> > pg_restore, instead of just these specific ones?
> > 
> 
> That would mean arbitrary parameters to pg_dump as well as pg_restore. But
> yes, that would probably be better in the long run.
> 
> Any suggestion as to how that would actually look like? Unfortunately
> pg_restore has -[dDoOr] already used, so it doesn't look like there will be
> any naturally intelligible short options for that.

We have the postmaster which can pass arbitrary arguments to postgres
processes using -o.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

  33. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-23T18:23:03Z

    On 3/23/21 2:06 PM, Bruce Momjian wrote:
> We have the postmaster which can pass arbitrary arguments to postgres
> processes using -o.

Right, and -o is already taken in pg_upgrade for sending options to the 
old postmaster.

What we are looking for are options for sending options to pg_dump and 
pg_restore, which are not postmasters or children of postmaster, but 
rather clients. There is no option to send options to clients of 
postmasters.

So the question remains, how do we name this?

     --pg-dump-options "<string>"
     --pg-restore-options "<string>"

where "<string>" could be something like "--whatever[=NUM] [...]" would 
be something unambiguous.


Regards, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  34. Re: pg_upgrade failing for 200+ million Large Objects

    Bruce Momjian <bruce@momjian.us> — 2021-03-23T18:25:01Z

    On Tue, Mar 23, 2021 at 02:23:03PM -0400, Jan Wieck wrote:
> On 3/23/21 2:06 PM, Bruce Momjian wrote:
> > We have the postmaster which can pass arbitrary arguments to postgres
> > processes using -o.
> 
> Right, and -o is already taken in pg_upgrade for sending options to the old
> postmaster.
> 
> What we are looking for are options for sending options to pg_dump and
> pg_restore, which are not postmasters or children of postmaster, but rather
> clients. There is no option to send options to clients of postmasters.
> 
> So the question remains, how do we name this?
> 
>     --pg-dump-options "<string>"
>     --pg-restore-options "<string>"
> 
> where "<string>" could be something like "--whatever[=NUM] [...]" would be
> something unambiguous.

Sure.  I don't think the letter you use is a problem.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

  35. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-23T18:35:46Z

    Jan Wieck <jan@wi3ck.info> writes:
> So the question remains, how do we name this?

>      --pg-dump-options "<string>"
>      --pg-restore-options "<string>"

If you're passing multiple options, that is

	--pg-dump-options "--foo=x --bar=y"

it seems just horribly fragile.  Lose the double quotes and suddenly
--bar is a separate option to pg_upgrade itself, not part of the argument
for the previous option.  That's pretty easy to do when passing things
through shell scripts, too.  So it'd likely be safer to write

	--pg-dump-option=--foo=x --pg-dump-option=--bar=y

which requires pg_upgrade to allow aggregating multiple options,
but you'd probably want it to act that way anyway.

			regards, tom lane

  36. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-23T18:54:29Z

    On 3/23/21 2:35 PM, Tom Lane wrote:
> Jan Wieck <jan@wi3ck.info> writes:
>> So the question remains, how do we name this?
> 
>>      --pg-dump-options "<string>"
>>      --pg-restore-options "<string>"
> 
> If you're passing multiple options, that is
> 
> 	--pg-dump-options "--foo=x --bar=y"
> 
> it seems just horribly fragile.  Lose the double quotes and suddenly
> --bar is a separate option to pg_upgrade itself, not part of the argument
> for the previous option.  That's pretty easy to do when passing things
> through shell scripts, too.  So it'd likely be safer to write
> 
> 	--pg-dump-option=--foo=x --pg-dump-option=--bar=y
> 
> which requires pg_upgrade to allow aggregating multiple options,
> but you'd probably want it to act that way anyway.

... which would be all really easy if pg_upgrade wouldn't be assembling 
a shell script string to pass into parallel_exec_prog() by itself.

But I will see what I can do ...


Regards, Jan


-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  37. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-23T18:59:24Z

    Jan Wieck <jan@wi3ck.info> writes:
> On 3/23/21 2:35 PM, Tom Lane wrote:
>> If you're passing multiple options, that is
>> --pg-dump-options "--foo=x --bar=y"
>> it seems just horribly fragile.  Lose the double quotes and suddenly
>> --bar is a separate option to pg_upgrade itself, not part of the argument
>> for the previous option.  That's pretty easy to do when passing things
>> through shell scripts, too.

> ... which would be all really easy if pg_upgrade wouldn't be assembling 
> a shell script string to pass into parallel_exec_prog() by itself.

No, what I was worried about is shell script(s) that invoke pg_upgrade
and have to pass down some of these options through multiple levels of
option parsing.

BTW, it doesn't seem like the "pg-" prefix has any value-add here,
so maybe "--dump-option" and "--restore-option" would be suitable
spellings.

			regards, tom lane

  38. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-23T19:22:04Z

    On 3/23/21 2:59 PM, Tom Lane wrote:
> Jan Wieck <jan@wi3ck.info> writes:
>> On 3/23/21 2:35 PM, Tom Lane wrote:
>>> If you're passing multiple options, that is
>>> --pg-dump-options "--foo=x --bar=y"
>>> it seems just horribly fragile.  Lose the double quotes and suddenly
>>> --bar is a separate option to pg_upgrade itself, not part of the argument
>>> for the previous option.  That's pretty easy to do when passing things
>>> through shell scripts, too.
> 
>> ... which would be all really easy if pg_upgrade wouldn't be assembling 
>> a shell script string to pass into parallel_exec_prog() by itself.
> 
> No, what I was worried about is shell script(s) that invoke pg_upgrade
> and have to pass down some of these options through multiple levels of
> option parsing.

The problem here is that pg_upgrade itself is invoking a shell again. It 
is not assembling an array of arguments to pass into exec*(). I'd be a 
happy camper if it did the latter. But as things are we'd have to add 
full shell escapeing for arbitrary strings.

> 
> BTW, it doesn't seem like the "pg-" prefix has any value-add here,
> so maybe "--dump-option" and "--restore-option" would be suitable
> spellings.

Agreed.


Regards, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  39. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-23T19:35:59Z

    Jan Wieck <jan@wi3ck.info> writes:
> The problem here is that pg_upgrade itself is invoking a shell again. It 
> is not assembling an array of arguments to pass into exec*(). I'd be a 
> happy camper if it did the latter. But as things are we'd have to add 
> full shell escapeing for arbitrary strings.

Surely we need that (and have it already) anyway?

I think we've stayed away from exec* because we'd have to write an
emulation for Windows.  Maybe somebody will get fed up and produce
such code, but it's not likely to be the least-effort route to the
goal.

			regards, tom lane

  40. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-23T19:59:48Z

    On 3/23/21 3:35 PM, Tom Lane wrote:
> Jan Wieck <jan@wi3ck.info> writes:
>> The problem here is that pg_upgrade itself is invoking a shell again. It 
>> is not assembling an array of arguments to pass into exec*(). I'd be a 
>> happy camper if it did the latter. But as things are we'd have to add 
>> full shell escapeing for arbitrary strings.
> 
> Surely we need that (and have it already) anyway?

There are functions to shell escape a single string, like

     appendShellString()

but that is hardly enough when a single optarg for --restore-option 
could look like any of

     --jobs 8
     --jobs=8
     --jobs='8'
     --jobs '8'
     --jobs "8"
     --jobs="8"
     --dont-bother-about-jobs

When placed into a shell string, those things have very different 
effects on your args[].

I also want to say that we are overengineering this whole thing. Yes, 
there is the problem of shell quoting possibly going wrong as it passes 
from one shell to another. But for now this is all about passing a few 
numbers down from pg_upgrade to pg_restore (and eventually pg_dump).

Have we even reached a consensus yet on that doing it the way, my patch 
is proposing, is the right way to go? Like that emitting BLOB TOC 
entries into SECTION_DATA when in binary upgrade mode is a good thing? 
Or that bunching all the SQL statements for creating the blob, changing 
the ACL and COMMENT and SECLABEL all in one multi-statement-query is.

Maybe we should focus on those details before getting into all the 
parameter naming stuff.


Regards, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  41. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-23T20:55:50Z

    Jan Wieck <jan@wi3ck.info> writes:
> Have we even reached a consensus yet on that doing it the way, my patch 
> is proposing, is the right way to go? Like that emitting BLOB TOC 
> entries into SECTION_DATA when in binary upgrade mode is a good thing? 
> Or that bunching all the SQL statements for creating the blob, changing 
> the ACL and COMMENT and SECLABEL all in one multi-statement-query is.

Now you're asking for actual review effort, which is a little hard
to come by towards the tail end of the last CF of a cycle.  I'm
interested in this topic, but I can't justify spending much time
on it right now.

			regards, tom lane

  42. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-24T16:04:26Z

    On 3/23/21 4:55 PM, Tom Lane wrote:
> Jan Wieck <jan@wi3ck.info> writes:
>> Have we even reached a consensus yet on that doing it the way, my patch 
>> is proposing, is the right way to go? Like that emitting BLOB TOC 
>> entries into SECTION_DATA when in binary upgrade mode is a good thing? 
>> Or that bunching all the SQL statements for creating the blob, changing 
>> the ACL and COMMENT and SECLABEL all in one multi-statement-query is.
> 
> Now you're asking for actual review effort, which is a little hard
> to come by towards the tail end of the last CF of a cycle.  I'm
> interested in this topic, but I can't justify spending much time
> on it right now.

Understood.

In any case I changed the options so that they behave the same way, the 
existing -o and -O (for old/new postmaster options) work. I don't think 
it would be wise to have option forwarding work differently between 
options for postmaster and options for pg_dump/pg_restore.


Regards, Jan

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  43. Re: pg_upgrade failing for 200+ million Large Objects

    Jan Wieck <jan@wi3ck.info> — 2021-03-24T16:05:27Z

    On 3/24/21 12:04 PM, Jan Wieck wrote:
> In any case I changed the options so that they behave the same way, the
> existing -o and -O (for old/new postmaster options) work. I don't think
> it would be wise to have option forwarding work differently between
> options for postmaster and options for pg_dump/pg_restore.

Attaching the actual diff might help.

-- 
Jan Wieck
Principle Database Engineer
Amazon Web Services

  44. Re: pg_upgrade failing for 200+ million Large Objects

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-11T22:43:08Z

    On Wed, Mar 24, 2021 at 12:05:27PM -0400, Jan Wieck wrote:
> On 3/24/21 12:04 PM, Jan Wieck wrote:
> > In any case I changed the options so that they behave the same way, the
> > existing -o and -O (for old/new postmaster options) work. I don't think
> > it would be wise to have option forwarding work differently between
> > options for postmaster and options for pg_dump/pg_restore.
> 
> Attaching the actual diff might help.

I think the original issue with XIDs was fixed by 74cf7d46a.

Are you still planning to progress the patches addressing huge memory use of
pg_restore?

Note this other, old thread on -general, which I believe has variations on the
same patches.
https://www.postgresql.org/message-id/flat/7bf19bf2-e6b7-01a7-1d96-f0607c728c49@wi3ck.info

There was discussion about using pg_restore --single.  Note that that was used
at some point in the past: see 12ee6ec71 and 861ad67bd.

The immediate problem is that --single conflicts with --create.
I cleaned up a patch I'd written to work around that.  It preserves DB settings
and passes pg_upgrade's test.  It's probably not portable as written, but if need be
could pass an empty file instead of /dev/null...

diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 3628bd74a7..9c504aff79 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -364,6 +364,16 @@ create_new_objects(void)
 		DbInfo	   *old_db = &old_cluster.dbarr.dbs[dbnum];
 		const char *create_opts;
 
+		PQExpBufferData connstr,
+				escaped_connstr;
+
+		initPQExpBuffer(&connstr);
+		initPQExpBuffer(&escaped_connstr);
+		appendPQExpBufferStr(&connstr, "dbname=");
+		appendConnStrVal(&connstr, old_db->db_name);
+		appendShellString(&escaped_connstr, connstr.data);
+		termPQExpBuffer(&connstr);
+
 		/* Skip template1 in this pass */
 		if (strcmp(old_db->db_name, "template1") == 0)
 			continue;
@@ -378,18 +388,31 @@ create_new_objects(void)
 		 * propagate its database-level properties.
 		 */
 		if (strcmp(old_db->db_name, "postgres") == 0)
-			create_opts = "--clean --create";
+			create_opts = "--clean";
 		else
-			create_opts = "--create";
+			create_opts = "";
 
+		/* Create the DB but exclude all objects */
 		parallel_exec_prog(log_file_name,
 						   NULL,
 						   "\"%s/pg_restore\" %s %s --exit-on-error --verbose "
+							"--create -L /dev/null "
 						   "--dbname template1 \"%s\"",
 						   new_cluster.bindir,
 						   cluster_conn_opts(&new_cluster),
 						   create_opts,
 						   sql_file_name);
+
+		parallel_exec_prog(log_file_name,
+						   NULL,
+						   "\"%s/pg_restore\" %s %s --exit-on-error --verbose --single "
+						   "--dbname=%s \"%s\"",
+						   new_cluster.bindir,
+						   cluster_conn_opts(&new_cluster),
+						   create_opts,
+							escaped_connstr.data,
+						   sql_file_name);
+
 	}
 
 	/* reap all children */

  45. Re: pg_upgrade failing for 200+ million Large Objects

    Nathan Bossart <nathandbossart@gmail.com> — 2022-08-25T00:32:27Z

    On Wed, Mar 24, 2021 at 12:05:27PM -0400, Jan Wieck wrote:
> On 3/24/21 12:04 PM, Jan Wieck wrote:
>> In any case I changed the options so that they behave the same way, the
>> existing -o and -O (for old/new postmaster options) work. I don't think
>> it would be wise to have option forwarding work differently between
>> options for postmaster and options for pg_dump/pg_restore.
> 
> Attaching the actual diff might help.

I'd like to revive this thread, so I've created a commitfest entry [0] and
attached a hastily rebased patch that compiles and passes the tests.  I am
aiming to spend some more time on this in the near future.

[0] https://commitfest.postgresql.org/39/3841/

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

  46. Re: pg_upgrade failing for 200+ million Large Objects

    Jacob Champion <jchampion@timescale.com> — 2022-09-07T21:42:05Z

    On 8/24/22 17:32, Nathan Bossart wrote:
> I'd like to revive this thread, so I've created a commitfest entry [0] and
> attached a hastily rebased patch that compiles and passes the tests.  I am
> aiming to spend some more time on this in the near future.

Just to clarify, was Justin's statement upthread (that the XID problem
is fixed) correct? And is this patch just trying to improve the
remaining memory and lock usage problems?

I took a quick look at the pg_upgrade diffs. I agree with Jan that the
escaping problem is a pretty bad smell, but even putting that aside for
a bit, is it safe to expose arbitrary options to pg_dump/restore during
upgrade? It's super flexible, but I can imagine that some of those flags
might really mess up the new cluster...

And yeah, if you choose to do that then you get to keep both pieces, I
guess, but I like that pg_upgrade tries to be (IMO) fairly bulletproof.

--Jacob

  47. Re: pg_upgrade failing for 200+ million Large Objects

    Nathan Bossart <nathandbossart@gmail.com> — 2022-09-08T23:18:07Z

    On Wed, Sep 07, 2022 at 02:42:05PM -0700, Jacob Champion wrote:
> Just to clarify, was Justin's statement upthread (that the XID problem
> is fixed) correct? And is this patch just trying to improve the
> remaining memory and lock usage problems?

I think "fixed" might not be totally accurate, but that is the gist.

> I took a quick look at the pg_upgrade diffs. I agree with Jan that the
> escaping problem is a pretty bad smell, but even putting that aside for
> a bit, is it safe to expose arbitrary options to pg_dump/restore during
> upgrade? It's super flexible, but I can imagine that some of those flags
> might really mess up the new cluster...
> 
> And yeah, if you choose to do that then you get to keep both pieces, I
> guess, but I like that pg_upgrade tries to be (IMO) fairly bulletproof.

IIUC the main benefit of this approach is that it isn't dependent on
binary-upgrade mode, which seems to be a goal based on the discussion
upthread [0].  I think it'd be easily possible to fix only pg_upgrade by
simply dumping and restoring pg_largeobject_metadata, as Andres suggested
in 2018 [1].  In fact, it seems like it ought to be possible to just copy
pg_largeobject_metadata's files as was done before 12a53c7.  AFAICT this
would only work for clusters upgrading from v12 and newer, and it'd break
if any of the underlying data types change their storage format.  This
seems unlikely for OIDs, but there is ongoing discussion about changing
aclitem.

I still think this is a problem worth fixing, but it's not yet clear how to
proceed.

[0] https://postgr.es/m/227228.1616259220%40sss.pgh.pa.us
[1] https://postgr.es/m/20181122001415.ef5bncxqin2y3esb%40alap3.anarazel.de

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

  48. Re: pg_upgrade failing for 200+ million Large Objects

    Jacob Champion <jchampion@timescale.com> — 2022-09-08T23:29:10Z

    On Thu, Sep 8, 2022 at 4:18 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> IIUC the main benefit of this approach is that it isn't dependent on
> binary-upgrade mode, which seems to be a goal based on the discussion
> upthread [0].

To clarify, I agree that pg_dump should contain the core fix. What I'm
questioning is the addition of --dump-options to make use of that fix
from pg_upgrade, since it also lets the user do "exciting" new things
like --exclude-schema and --include-foreign-data and so on. I don't
think we should let them do that without a good reason.

Thanks,
--Jacob

  49. Re: pg_upgrade failing for 200+ million Large Objects

    Nathan Bossart <nathandbossart@gmail.com> — 2022-09-08T23:34:07Z

    On Thu, Sep 08, 2022 at 04:29:10PM -0700, Jacob Champion wrote:
> On Thu, Sep 8, 2022 at 4:18 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> IIUC the main benefit of this approach is that it isn't dependent on
>> binary-upgrade mode, which seems to be a goal based on the discussion
>> upthread [0].
> 
> To clarify, I agree that pg_dump should contain the core fix. What I'm
> questioning is the addition of --dump-options to make use of that fix
> from pg_upgrade, since it also lets the user do "exciting" new things
> like --exclude-schema and --include-foreign-data and so on. I don't
> think we should let them do that without a good reason.

Ah, yes, I think that is a fair point.

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

  50. Re: pg_upgrade failing for 200+ million Large Objects

    Michael Paquier <michael@paquier.xyz> — 2022-10-12T05:50:11Z

    On Thu, Sep 08, 2022 at 04:34:07PM -0700, Nathan Bossart wrote:
> On Thu, Sep 08, 2022 at 04:29:10PM -0700, Jacob Champion wrote:
>> To clarify, I agree that pg_dump should contain the core fix. What I'm
>> questioning is the addition of --dump-options to make use of that fix
>> from pg_upgrade, since it also lets the user do "exciting" new things
>> like --exclude-schema and --include-foreign-data and so on. I don't
>> think we should let them do that without a good reason.
> 
> Ah, yes, I think that is a fair point.

It has been more than four weeks since the last activity of this
thread and there has been what looks like some feedback to me, so
marked as RwF for the time being.
--
Michael

  51. Re: pg_upgrade failing for 200+ million Large Objects

    Kumar, Sachin <ssetiya@amazon.com> — 2023-11-09T17:35:01Z

    


Hi Everyone , I want to continue this thread , I have rebased the patch to latest
master and fixed an issue when pg_restore prints to file.

`
╰─$ pg_restore  dump_small.custom  --restore-blob-batch-size=2 --file=a
--
-- End BLOB restore batch
--
COMMIT;
`

> On 09/11/2023, 17:05, "Jacob Champion" <jchampion@timescale.com <mailto:jchampion@timescale.com>> wrote:
> To clarify, I agree that pg_dump should contain the core fix. What I'm
> questioning is the addition of --dump-options to make use of that fix
> from pg_upgrade, since it also lets the user do "exciting" new things
> like --exclude-schema and --include-foreign-data and so on. I don't
> think we should let them do that without a good reason.

Earlier idea was to not expose these options to users and use [1]
   --restore-jobs=NUM             --jobs parameter passed to pg_restore
   --restore-blob-batch-size=NUM  number of blobs restored in one xact
But this was later expanded to use --dump-options and --restore-options [2].
With --restore-options user can use --exclude-schema , 
So maybe we can go back to [1]

[1] https://www.postgresql.org/message-id/a1e200e6-adde-2561-422b-a166ec084e3b%40wi3ck.info
[2] https://www.postgresql.org/message-id/8d8d3961-8e8b-3dbe-f911-6f418c5fb1d3%40wi3ck.info

Regards
Sachin
Amazon Web Services: https://aws.amazon.com

  52. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-11-09T18:41:01Z

    [ Jacob's email address updated ]

"Kumar, Sachin" <ssetiya@amazon.com> writes:
> Hi Everyone , I want to continue this thread , I have rebased the patch to latest
> master and fixed an issue when pg_restore prints to file.

Um ... you didn't attach the patch?

FWIW, I agree with Jacob's concern about it being a bad idea to let
users of pg_upgrade pass down arbitrary options to pg_dump/pg_restore.
I think we'd regret going there, because it'd hugely expand the set
of cases pg_upgrade has to deal with.

Also, pg_upgrade is often invoked indirectly via scripts, so I do
not especially buy the idea that we're going to get useful control
input from some human somewhere.  I think we'd be better off to
assume that pg_upgrade is on its own to manage the process, so that
if we need to switch strategies based on object count or whatever,
we should put in a heuristic to choose the strategy automatically.
It might not be perfect, but that will give better results for the
pretty large fraction of users who are not going to mess with
weird little switches.

			regards, tom lane

  53. Re: pg_upgrade failing for 200+ million Large Objects

    Andres Freund <andres@anarazel.de> — 2023-11-09T23:12:50Z

    Hi, 

On November 9, 2023 10:41:01 AM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Also, pg_upgrade is often invoked indirectly via scripts, so I do
>not especially buy the idea that we're going to get useful control
>input from some human somewhere.  I think we'd be better off to
>assume that pg_upgrade is on its own to manage the process, so that
>if we need to switch strategies based on object count or whatever,
>we should put in a heuristic to choose the strategy automatically.
>It might not be perfect, but that will give better results for the
>pretty large fraction of users who are not going to mess with
>weird little switches.

+1 - even leaving everything else aside, just about no user would know about the option. There are cases where we can't do better than giving the user control, but we are certainly adding options at a rate that doesn't seem sustainable. And here it doesn't seem that hard to do better. 

Andres 
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  54. Re: pg_upgrade failing for 200+ million Large Objects

    Kumar, Sachin <ssetiya@amazon.com> — 2023-11-13T15:06:31Z

    > On 09/11/2023, 18:41, "Tom Lane" <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:
> Um ... you didn't attach the patch?

Sorry , patch attached

Regards
Sachin

  55. Re: pg_upgrade failing for 200+ million Large Objects

    Kumar, Sachin <ssetiya@amazon.com> — 2023-12-04T16:07:59Z

    
> "Tom Lane" <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:

> FWIW, I agree with Jacob's concern about it being a bad idea to let
> users of pg_upgrade pass down arbitrary options to pg_dump/pg_restore.
> I think we'd regret going there, because it'd hugely expand the set
> of cases pg_upgrade has to deal with.

> Also, pg_upgrade is often invoked indirectly via scripts, so I do
> not especially buy the idea that we're going to get useful control
> input from some human somewhere. I think we'd be better off to
> assume that pg_upgrade is on its own to manage the process, so that
> if we need to switch strategies based on object count or whatever,
> we should put in a heuristic to choose the strategy automatically.
> It might not be perfect, but that will give better results for the
> pretty large fraction of users who are not going to mess with
> weird little switches.


I have updated the patch to use heuristic, During pg_upgrade we count
Large objects per database. During pg_restore execution if db large_objects
count is greater than LARGE_OBJECTS_THRESOLD (1k) we will use 
--restore-blob-batch-size.
I also modified pg_upgrade --jobs behavior if we have large_objects (> LARGE_OBJECTS_THRESOLD)

+  /*  Restore all the dbs where LARGE_OBJECTS_THRESOLD is not breached */
+  restore_dbs(stats, true);
+  /* reap all children */
+  while (reap_child(true) == true)
+     ;
+  /*  Restore rest of the dbs one by one  with pg_restore --jobs = user_opts.jobs */
+  restore_dbs(stats, false);
   /* reap all children */
   while (reap_child(true) == true)
      ;

Regards
Sachin

  56. Re: pg_upgrade failing for 200+ million Large Objects

    Kumar, Sachin <ssetiya@amazon.com> — 2023-12-07T14:05:13Z

    > I have updated the patch to use heuristic, During pg_upgrade we count
> Large objects per database. During pg_restore execution if db large_objects
> count is greater than LARGE_OBJECTS_THRESOLD (1k) we will use 
> --restore-blob-batch-size.


I think both SECTION_DATA and SECTION_POST_DATA can be parallelized by pg_restore, So instead of storing 
large objects in heuristics, we can store SECTION_DATA + SECTION_POST_DATA.

Regards
Sachin

  57. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-11T01:42:42Z

    I spent some time looking at the v7 patch.  I can't help feeling
that this is going off in the wrong direction, primarily for
these reasons:

* It focuses only on cutting the number of transactions needed
to restore a large number of blobs (large objects).  Certainly
that's a pain point, but it's not the only one of this sort.
If you have a lot of tables, restore will consume just as many
transactions as it would for a similar number of blobs --- probably
more, in fact, since we usually need more commands per table than
per blob.

* I'm not too thrilled with the (undocumented) rearrangements in
pg_dump.  I really don't like the idea of emitting a fundamentally
different TOC layout in binary-upgrade mode; that seems unmaintainably
bug-prone.  Plus, the XID-consumption problem is not really confined
to pg_upgrade.

What I think we actually ought to do is one of the alternatives
discussed upthread: teach pg_restore to be able to commit
every so often, without trying to provide the all-or-nothing
guarantees of --single-transaction mode.  This cuts its XID
consumption by whatever multiple "every so often" is, while
allowing us to limit the number of locks taken during any one
transaction.  It also seems a great deal safer than the idea
I floated of not taking locks at all during a binary upgrade;
plus, it has some usefulness with regular pg_restore that's not
under control of pg_upgrade.

So I had a go at coding that, and attached is the result.
It invents a --transaction-size option, and when that's active
it will COMMIT after every N TOC items.  (This seems simpler to
implement and less bug-prone than every-N-SQL-commands.)

I had initially supposed that in a parallel restore we could
have child workers also commit after every N TOC items, but was
soon disabused of that idea.  After a worker processes a TOC
item, any dependent items (such as index builds) might get
dispatched to some other worker, which had better be able to
see the results of the first worker's step.  So at least in
this implementation, we disable the multi-command-per-COMMIT
behavior during the parallel part of the restore.  Maybe that
could be improved in future, but it seems like it'd add a
lot more complexity, and it wouldn't make life any better for
pg_upgrade (which doesn't use parallel pg_restore, and seems
unlikely to want to in future).

I've not spent a lot of effort on pg_upgrade changes here:
I just hard-wired it to select --transaction-size=1000.
Given the default lock table size of 64*100, that gives us
enough headroom for each TOC to take half a dozen locks.
We could go higher than that by making pg_upgrade force the
destination postmaster to create a larger-than-default lock
table, but I'm not sure if it's worth any trouble.  We've
already bought three orders of magnitude improvement as it
stands, which seems like enough ambition for today.  (Also,
having pg_upgrade override the user's settings in the
destination cluster might not be without downsides.)

Another thing I'm wondering about is why this is only a pg_restore
option not also a pg_dump/pg_dumpall option.  I did it like that
because --single-transaction is pg_restore only, but that seems more
like an oversight or laziness than a well-considered decision.
Maybe we should back-fill that omission; but it could be done later.

Thoughts?

			regards, tom lane

  58. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-20T23:47:44Z

    I have spent some more effort in this area and developed a patch
series that I think addresses all of the performance issues that
we've discussed in this thread, both for pg_upgrade and more
general use of pg_dump/pg_restore.  Concretely, it absorbs
the pg_restore --transaction-size switch that I proposed before
to cut the number of transactions needed during restore, and
rearranges the representation of BLOB-related TOC entries to
reduce the client-side memory requirements, and fixes some
ancient mistakes that prevent both selective restore of BLOBs
and parallel restore of BLOBs.

As a demonstration, I made a database containing 100K empty blobs,
and measured the time needed to dump/restore that using -Fd
and -j 10.  HEAD doesn't get any useful parallelism on blobs,
but with this patch series we do:

		dump	restore
HEAD:		14sec	15sec
after 0002:	7sec	10sec
after 0003:	7sec	3sec

There are a few loose ends:

* I did not invent a switch to control the batching of blobs; it's
just hard-wired at 1000 blobs per group here.  Probably we need some
user knob for that, but I'm unsure if we want to expose a count or
just a boolean for one vs more than one blob per batch.  The point of
forcing one blob per batch would be to allow exact control during
selective restore, and I'm not sure if there's any value in random
other settings.  On the other hand, selective restore of blobs has
been completely broken for the last dozen years and I can't recall any
user complaints about that; so maybe nobody cares and we could just
leave this as an internal choice.

* Likewise, there's no user-accessible knob to control what
transaction size pg_upgrade uses.  Do we need one?  In any case, it's
likely that the default needs a bit more thought than I've given it.
I used 1000, but if pg_upgrade is launching parallel restore jobs we
likely need to divide that by the number of restore jobs.

* As the patch stands, we still build a separate TOC entry for each
comment or seclabel or ACL attached to a blob.  If you have a lot of
blobs with non-default properties then the TOC bloat problem comes
back again.  We could do something about that, but it would take a bit
of tedious refactoring, and the most obvious way to handle it probably
re-introduces too-many-locks problems.  Is this a scenario that's
worth spending a lot of time on?

More details appear in the commit messages below.  Patch 0004
is nearly the same as the v8 patch I posted before, although
it adds some logic to ensure that a large blob metadata batch
doesn't create too many locks.

Comments?

			regards, tom lane

PS: I don't see any active CF entry for this thread, so
I'm going to go make one.

  59. Re: pg_upgrade failing for 200+ million Large Objects

    Nathan Bossart <nathandbossart@gmail.com> — 2023-12-21T03:16:14Z

    On Wed, Dec 20, 2023 at 06:47:44PM -0500, Tom Lane wrote:
> I have spent some more effort in this area and developed a patch
> series that I think addresses all of the performance issues that
> we've discussed in this thread, both for pg_upgrade and more
> general use of pg_dump/pg_restore.  Concretely, it absorbs
> the pg_restore --transaction-size switch that I proposed before
> to cut the number of transactions needed during restore, and
> rearranges the representation of BLOB-related TOC entries to
> reduce the client-side memory requirements, and fixes some
> ancient mistakes that prevent both selective restore of BLOBs
> and parallel restore of BLOBs.
> 
> As a demonstration, I made a database containing 100K empty blobs,
> and measured the time needed to dump/restore that using -Fd
> and -j 10.  HEAD doesn't get any useful parallelism on blobs,
> but with this patch series we do:
> 
> 		dump	restore
> HEAD:		14sec	15sec
> after 0002:	7sec	10sec
> after 0003:	7sec	3sec

Wow, thanks for putting together these patches.  I intend to help review,
but I'm not sure I'll find much time to do so before the new year.

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

  60. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-21T04:03:29Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> Wow, thanks for putting together these patches.  I intend to help review,

Thanks!

> but I'm not sure I'll find much time to do so before the new year.

There's no urgency, surely.  If we can get these in during the
January CF, I'll be happy.

			regards, tom lane

  61. Re: pg_upgrade failing for 200+ million Large Objects

    Robins Tharakan <tharakan@gmail.com> — 2023-12-27T13:28:01Z

    >
> On Thu, 21 Dec 2023 at 10:17, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
I have spent some more effort in this area and developed a patch
> series that I think addresses all of the performance issues that
> we've discussed in this thread, both for pg_upgrade and more
> general use of pg_dump/pg_restore.



Thanks for picking this up!

Applying all 4 patches, I also see good performance improvement.

With more Large Objects, although pg_dump improved significantly,
pg_restore is now comfortably an order of magnitude faster.

pg_dump times (seconds):
    NumLOs     dump-patch004      dump-HEAD    improvement (%)
         1       0.09               0.09          ~
        10       0.10               0.12          ~
       100       0.12               0.12          ~
     1,000       0.41               0.44          ~
    10,000       3                  5            76%
   100,000      35                 47            36%
 1,000,000     111                251           126%


pg_restore times (seconds):
    NumLOs     restore-patch0004  restore-HEAD improvement (%)
         1      0.02                 0.02         ~
        10      0.03                 0.03         ~
       100      0.13                 0.12         ~
     1,000      0.98                 0.97         ~
    10,000      2                    9           ~5x
   100,000      6                   93           13x
 1,000,000     53                  973           17x


Test details:
- pg_dump -Fd -j32 / pg_restore -j32
- 32vCPU / Ubuntu 20.04 / 260GB Memory / r6id.8xlarge
- Client & Server on same machine
- Empty LOs / Empty ACLs
- HEAD = 7d7ef075d2b3f3bac4db323c2a47fb15a4a9a817
- See attached graphs

IMHO the knob (for configuring batch size) is a non-blocker. The
default (1k) here is already way better than what we have today.

Look forward to feedback on the tests, or I'll continue testing
whether ACLs / non-empty LOs etc. adversely affect these numbers.

-
Robins Tharakan
Amazon Web Services

  62. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-27T15:18:21Z

    Robins Tharakan <tharakan@gmail.com> writes:
> Applying all 4 patches, I also see good performance improvement.
> With more Large Objects, although pg_dump improved significantly,
> pg_restore is now comfortably an order of magnitude faster.

Yeah.  The key thing here is that pg_dump can only parallelize
the data transfer, while (with 0004) pg_restore can parallelize
large object creation and owner-setting as well as data transfer.
I don't see any simple way to improve that on the dump side,
but I'm not sure we need to.  Zillions of empty objects is not
really the use case to worry about.  I suspect that a more realistic
case with moderate amounts of data in the blobs would make pg_dump
look better.

			regards, tom lane

  63. Re: pg_upgrade failing for 200+ million Large Objects

    Robins Tharakan <tharakan@gmail.com> — 2023-12-28T11:38:46Z

    On Thu, 28 Dec 2023 at 01:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Robins Tharakan <tharakan@gmail.com> writes:
> > Applying all 4 patches, I also see good performance improvement.
> > With more Large Objects, although pg_dump improved significantly,
> > pg_restore is now comfortably an order of magnitude faster.
>
> Yeah.  The key thing here is that pg_dump can only parallelize
> the data transfer, while (with 0004) pg_restore can parallelize
> large object creation and owner-setting as well as data transfer.
> I don't see any simple way to improve that on the dump side,
> but I'm not sure we need to.  Zillions of empty objects is not
> really the use case to worry about.  I suspect that a more realistic
> case with moderate amounts of data in the blobs would make pg_dump
> look better.
>


Thanks for elaborating, and yes pg_dump times do reflect that
expectation.

The first test involved a fixed number (32k) of
Large Objects (LOs) with varying sizes - I chose that number
intentionally since this was being tested on a 32vCPU instance
and the patch employs 1k batches.


We again see that pg_restore is an order of magnitude faster.

 LO Size (bytes)  restore-HEAD restore-patched  improvement (Nx)
               1    24.182         1.4          17x
              10    24.741         1.5          17x
             100    24.574         1.6          15x
           1,000    25.314         1.7          15x
          10,000    25.644         1.7          15x
         100,000    50.046         4.3          12x
       1,000,000   281.549        30.0           9x


pg_dump also sees improvements. Really small sized LOs
see a decent ~20% improvement which grows considerably as LOs
get bigger (beyond ~10-100kb).


 LO Size (bytes)  dump-HEAD  dump-patched    improvement (%)
               1    12.9          10.7          18%
              10    12.9          10.4          19%
             100    12.8          10.3          20%
           1,000    13.0          10.3          21%
          10,000    14.2          10.3          27%
         100,000    32.8          11.5          65%
       1,000,000   211.8          23.6          89%


To test pg_restore scaling, 1 Million LOs (100kb each)
were created and pg_restore times tested for increasing
concurrency (on a 192vCPU instance). We see major speedup
upto -j64 and the best time was at -j96, after which
performance decreases slowly - see attached image.

Concurrency    pg_restore-patched
    384              75.87
    352              75.63
    320              72.11
    288              70.05
    256              70.98
    224              66.98
    192              63.04
    160              61.37
    128              58.82
     96              58.55
     64              60.46
     32              77.29
     16             115.51
      8             203.48
      4             366.33



Test details:
- Command used to generate SQL - create 1k LOs of 1kb each
  - echo "SELECT lo_from_bytea(0, '\x`  printf 'ff%.0s' {1..1000}`') FROM
generate_series(1,1000);" > /tmp/tempdel
- Verify the LO size: select pg_column_size(lo_get(oid));
- Only GUC changed: max_connections=1000 (for the last test)

-
Robins Tharakan
Amazon Web Services

  64. Re: pg_upgrade failing for 200+ million Large Objects

    Kumar, Sachin <ssetiya@amazon.com> — 2024-01-02T17:33:00Z

    > On 11/12/2023, 01:43, "Tom Lane" <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:

> I had initially supposed that in a parallel restore we could
> have child workers also commit after every N TOC items, but was
> soon disabused of that idea. After a worker processes a TOC
> item, any dependent items (such as index builds) might get
> dispatched to some other worker, which had better be able to
> see the results of the first worker's step. So at least in
> this implementation, we disable the multi-command-per-COMMIT
> behavior during the parallel part of the restore. Maybe that
> could be improved in future, but it seems like it'd add a
> lot more complexity, and it wouldn't make life any better for
> pg_upgrade (which doesn't use parallel pg_restore, and seems
> unlikely to want to in future).

I was not able to find email thread which details why we are not using
parallel pg_restore for pg_upgrade. IMHO most of the customer will have single large
database, and not using parallel restore will cause slow pg_upgrade.

I am attaching a patch which enables parallel pg_restore for DATA and POST-DATA part
of dump. It will push down --jobs value to pg_restore and will restore database sequentially.

Benchmarks

{5 million LOs 1 large DB}
Patched {v9}
time pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir ~/upgrade/data/pub --new-datadir ~/data/sub --jobs=20
pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir      17.51s user 65.80s system 35% cpu 3:56.64 total


time pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir ~/upgrade/data/pub --new-datadir ~/data/sub -r
pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir      17.51s user 65.85s system 34% cpu 3:58.39 total


HEAD
time pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir ~/upgrade/data/pub --new-datadir ~/data/sub -r --jobs=20
pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir      53.95s user 82.44s system 41% cpu 5:25.23 total

time pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir ~/upgrade/data/pub --new-datadir ~/data/sub -r
pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir      54.94s user 81.26s system 41% cpu 5:24.86 total



Fix with --jobs propagation to pg_restore {on top of v9}
time pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir ~/upgrade/data/pub --new-datadir ~/data/sub -r --jobs=20
pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir      29.12s user 69.85s system 275% cpu 35.930 total 


Although parallel restore does have small regression in ideal case of pg_upgrade --jobs


Multiple DBs {4 DBs each having 2 million LOs}

Fix with --jobs scheduling
time pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir ~/upgrade/data/pub --new-datadir ~/data/sub -r --jobs=4
pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir      31.80s user 109.52s system 120% cpu 1:57.35 total


Patched {v9}
time pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir ~/upgrade/data/pub --new-datadir ~/data/sub -r --jobs=4
pg_upgrade --old-bindir ~/15/bin --new-bindir ~/install/bin --old-datadir      30.88s user 110.05s system 135% cpu 1:43.97 total


Regards
Sachin

  65. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-01-02T18:06:18Z

    "Kumar, Sachin" <ssetiya@amazon.com> writes:
>> On 11/12/2023, 01:43, "Tom Lane" <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:
>> ... Maybe that
>> could be improved in future, but it seems like it'd add a
>> lot more complexity, and it wouldn't make life any better for
>> pg_upgrade (which doesn't use parallel pg_restore, and seems
>> unlikely to want to in future).

> I was not able to find email thread which details why we are not using
> parallel pg_restore for pg_upgrade.

Well, it's pretty obvious isn't it?  The parallelism is being applied
at the per-database level instead.

> IMHO most of the customer will have single large
> database, and not using parallel restore will cause slow pg_upgrade.

You've offered no justification for that opinion ...

> I am attaching a patch which enables parallel pg_restore for DATA and POST-DATA part
> of dump. It will push down --jobs value to pg_restore and will restore
> database sequentially.

I don't think I trust this patch one bit.  It makes way too many
assumptions about how the --section options work, or even that they
will work at all in a binary-upgrade situation.  I've spent enough
time with that code to know that --section is pretty close to being
a fiction.  One point in particular is that this would change the
order of ACL restore relative to other steps, which almost certainly
will cause problems for somebody.

			regards, tom lane

  66. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-01-05T20:02:34Z

    I wrote:
> "Kumar, Sachin" <ssetiya@amazon.com> writes:
>> I was not able to find email thread which details why we are not using
>> parallel pg_restore for pg_upgrade.

> Well, it's pretty obvious isn't it?  The parallelism is being applied
> at the per-database level instead.

On further reflection, there is a very good reason why it's done like
that.  Because pg_upgrade is doing schema-only dump and restore,
there's next to no opportunity for parallelism within either pg_dump
or pg_restore.  There's no data-loading steps, and there's no
index-building either, so the time-consuming stuff that could be
parallelized just isn't happening in pg_upgrade's usage.

Now it's true that my 0003 patch moves the needle a little bit:
since it makes BLOB creation (as opposed to loading) parallelizable,
there'd be some hope for parallel pg_restore doing something useful in
a database with very many blobs.  But it makes no sense to remove the
existing cross-database parallelism in pursuit of that; you'd make
many more people unhappy than happy.

Conceivably something could be salvaged of your idea by having
pg_upgrade handle databases with many blobs differently from
those without, applying parallelism within pg_restore for the
first kind and then using cross-database parallelism for the
rest.  But that seems like a lot of complexity compared to the
possible win.

In any case I'd stay far away from using --section in pg_upgrade.
Too many moving parts there.

			regards, tom lane

  67. Re: pg_upgrade failing for 200+ million Large Objects

    Nathan Bossart <nathandbossart@gmail.com> — 2024-01-12T22:42:27Z

    On Wed, Dec 20, 2023 at 06:47:44PM -0500, Tom Lane wrote:
> * I did not invent a switch to control the batching of blobs; it's
> just hard-wired at 1000 blobs per group here.  Probably we need some
> user knob for that, but I'm unsure if we want to expose a count or
> just a boolean for one vs more than one blob per batch.  The point of
> forcing one blob per batch would be to allow exact control during
> selective restore, and I'm not sure if there's any value in random
> other settings.  On the other hand, selective restore of blobs has
> been completely broken for the last dozen years and I can't recall any
> user complaints about that; so maybe nobody cares and we could just
> leave this as an internal choice.

I think the argument for making this configurable is that folks might have
fewer larger blobs, in which case it might make sense to lower the batch
size, or they might have many smaller blobs, in which case they might want
to increase it.  But I'm a bit skeptical that will make any sort of
tremendous difference in practice, and I'm not sure how a user would decide
on the right value besides trial-and-error.  In any case, at the moment I
think it'd be okay to keep this an internal setting and wait for feedback
from the field.

> * As the patch stands, we still build a separate TOC entry for each
> comment or seclabel or ACL attached to a blob.  If you have a lot of
> blobs with non-default properties then the TOC bloat problem comes
> back again.  We could do something about that, but it would take a bit
> of tedious refactoring, and the most obvious way to handle it probably
> re-introduces too-many-locks problems.  Is this a scenario that's
> worth spending a lot of time on?

I'll ask around about this.

> Subject: [PATCH v9 1/4] Some small preliminaries for pg_dump changes.

This one looked good to me.

> Subject: [PATCH v9 2/4] In dumps, group large objects into matching metadata
>  and data entries.

I spent most of my review time reading this one.  Overall, it looks pretty
good to me, and I think you've called out most of the interesting design
choices.

> +			char	   *cmdEnd = psprintf(" OWNER TO %s", fmtId(te->owner));
> +
> +			IssueCommandPerBlob(AH, te, "ALTER LARGE OBJECT ", cmdEnd);

This is just a nitpick, but is there any reason not to have
IssueCommandPerBlob() accept a format string and the corresponding
arguments?

> +		while (n < 1000 && i + n < ntups)

Another nitpick: it might be worth moving these hard-wired constants to
macros.  Without a control switch, that'd at least make it easy for anyone
determined to change the value for their installation.

> Subject: [PATCH v9 3/4] Move BLOBS METADATA TOC entries into SECTION_DATA.

This one looks reasonable, too.

> In this patch I have just hard-wired pg_upgrade to use
> --transaction-size 1000.  Perhaps there would be value in adding
> another pg_upgrade option to allow user control of that, but I'm
> unsure that it's worth the trouble; I think few users would use it,
> and any who did would see not that much benefit.  However, we
> might need to adjust the logic to make the size be 1000 divided
> by the number of parallel restore jobs allowed.

I wonder if it'd be worth making this configurable for pg_upgrade as an
escape hatch in case the default setting is problematic.

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

  68. Re: pg_upgrade failing for 200+ million Large Objects

    Nathan Bossart <nathandbossart@gmail.com> — 2024-01-12T22:48:20Z

    On Fri, Jan 05, 2024 at 03:02:34PM -0500, Tom Lane wrote:
> On further reflection, there is a very good reason why it's done like
> that.  Because pg_upgrade is doing schema-only dump and restore,
> there's next to no opportunity for parallelism within either pg_dump
> or pg_restore.  There's no data-loading steps, and there's no
> index-building either, so the time-consuming stuff that could be
> parallelized just isn't happening in pg_upgrade's usage.
> 
> Now it's true that my 0003 patch moves the needle a little bit:
> since it makes BLOB creation (as opposed to loading) parallelizable,
> there'd be some hope for parallel pg_restore doing something useful in
> a database with very many blobs.  But it makes no sense to remove the
> existing cross-database parallelism in pursuit of that; you'd make
> many more people unhappy than happy.

I assume the concern is that we'd end up multiplying the effective number
of workers if we parallelized both in-database and cross-database?  Would
it be sufficient to make those separately configurable with a note about
the multiplicative effects of setting both?  I think it'd be unfortunate if
pg_upgrade completely missed out on this improvement.

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

  69. Re: pg_upgrade failing for 200+ million Large Objects

    Nathan Bossart <nathandbossart@gmail.com> — 2024-01-12T22:56:35Z

    On Fri, Jan 12, 2024 at 04:42:27PM -0600, Nathan Bossart wrote:
> On Wed, Dec 20, 2023 at 06:47:44PM -0500, Tom Lane wrote:
>> +			char	   *cmdEnd = psprintf(" OWNER TO %s", fmtId(te->owner));
>> +
>> +			IssueCommandPerBlob(AH, te, "ALTER LARGE OBJECT ", cmdEnd);
> 
> This is just a nitpick, but is there any reason not to have
> IssueCommandPerBlob() accept a format string and the corresponding
> arguments?

Eh, I guess you'd have to find some other way of specifying where the OID
is supposed to go, which would probably be weird.  Please disregard this
one.

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

  70. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-01-12T22:57:24Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> On Wed, Dec 20, 2023 at 06:47:44PM -0500, Tom Lane wrote:
>> +			char	   *cmdEnd = psprintf(" OWNER TO %s", fmtId(te->owner));
>> +
>> +			IssueCommandPerBlob(AH, te, "ALTER LARGE OBJECT ", cmdEnd);

> This is just a nitpick, but is there any reason not to have
> IssueCommandPerBlob() accept a format string and the corresponding
> arguments?

The problem is how to combine the individual per-blob OID with the
command string given by the caller.  If we want the caller to also be
able to inject data values, I don't really see how to combine the OID
with the va_args list from the caller.  If we stick with the design
that the caller provides separate "front" and "back" strings, but ask
to be able to inject data values into those, then we need two va_args
lists which C doesn't support, or else an arbitrary decision about
which one gets the va_args.  (Admittedly, with only one caller that
needs a non-constant string, we could make such a decision; but by the
same token we'd gain little.)

It'd be notationally simpler if we could have the caller supply one
string that includes %u where the OID is supposed to go; but then
we have problems if an owner name includes %.  So on the whole I'm
not seeing anything much better than what I did.  Maybe I missed
an idea though.

> Another nitpick: it might be worth moving these hard-wired constants to
> macros.  Without a control switch, that'd at least make it easy for anyone
> determined to change the value for their installation.

OK.

			regards, tom lane

  71. Re: pg_upgrade failing for 200+ million Large Objects

    vignesh C <vignesh21@gmail.com> — 2024-01-26T14:42:44Z

    On Tue, 2 Jan 2024 at 23:03, Kumar, Sachin <ssetiya@amazon.com> wrote:
>
> > On 11/12/2023, 01:43, "Tom Lane" <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:
>
> > I had initially supposed that in a parallel restore we could
> > have child workers also commit after every N TOC items, but was
> > soon disabused of that idea. After a worker processes a TOC
> > item, any dependent items (such as index builds) might get
> > dispatched to some other worker, which had better be able to
> > see the results of the first worker's step. So at least in
> > this implementation, we disable the multi-command-per-COMMIT
> > behavior during the parallel part of the restore. Maybe that
> > could be improved in future, but it seems like it'd add a
> > lot more complexity, and it wouldn't make life any better for
> > pg_upgrade (which doesn't use parallel pg_restore, and seems
> > unlikely to want to in future).
>
> I was not able to find email thread which details why we are not using
> parallel pg_restore for pg_upgrade. IMHO most of the customer will have single large
> database, and not using parallel restore will cause slow pg_upgrade.
>
> I am attaching a patch which enables parallel pg_restore for DATA and POST-DATA part
> of dump. It will push down --jobs value to pg_restore and will restore database sequentially.

CFBot shows that the patch does not apply anymore as in [1]:
=== Applying patches on top of PostgreSQL commit ID
46a0cd4cefb4d9b462d8cc4df5e7ecdd190bea92 ===
=== applying patch ./v9-005-parallel_pg_restore.patch
patching file src/bin/pg_upgrade/pg_upgrade.c
Hunk #3 FAILED at 650.
1 out of 3 hunks FAILED -- saving rejects to file
src/bin/pg_upgrade/pg_upgrade.c.rej

Please post an updated version for the same.

[1] - http://cfbot.cputube.org/patch_46_4713.log

Regards,
Vignesh

  72. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-01-26T16:44:26Z

    vignesh C <vignesh21@gmail.com> writes:
> CFBot shows that the patch does not apply anymore as in [1]:
> === Applying patches on top of PostgreSQL commit ID
> 46a0cd4cefb4d9b462d8cc4df5e7ecdd190bea92 ===
> === applying patch ./v9-005-parallel_pg_restore.patch
> patching file src/bin/pg_upgrade/pg_upgrade.c
> Hunk #3 FAILED at 650.
> 1 out of 3 hunks FAILED -- saving rejects to file
> src/bin/pg_upgrade/pg_upgrade.c.rej

That's because v9-005 was posted by itself.  But I don't think
we should use it anyway.

Here's 0001-0004 again, updated to current HEAD (only line numbers
changed) and with Nathan's suggestion to define some macros for
the magic constants.

			regards, tom lane

  73. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-15T23:18:41Z

    This patch seems to have stalled out again.  In hopes of getting it
over the finish line, I've done a bit more work to address the two
loose ends I felt were probably essential to deal with:

* Duplicative blob ACLs are now merged into a single TOC entry
(per metadata group) with the GRANT/REVOKE commands stored only
once.  This is to address the possibly-common case where a database
has a ton of blobs that have identical-but-not-default ACLs.

I have not done anything about improving efficiency for blob comments
or security labels.  I think it's reasonable to assume that blobs with
comments are pets not cattle, and there won't be many of them.
I suppose it could be argued that seclabels might be used like ACLs
with a lot of duplication, but I doubt that there's anyone out there
at all putting seclabels on blobs in practice.  So I don't care to
expend effort on that.

* Parallel pg_upgrade cuts the --transaction-size given to concurrent
pg_restore jobs by the -j factor.  This is to ensure we keep the
shared locks table within bounds even in parallel mode.

Now we could go further than that and provide some direct user
control over these hard-wired settings, but I think that could
be left for later, getting some field experience before we design
an API.  In short, I think this patchset is more or less commitable.

0001-0004 are rebased up to HEAD, but differ only in line numbers
from the v10 patchset.  0005 handles ACL merging, and 0006 does
the other thing.

			regards, tom lane

  74. Re: pg_upgrade failing for 200+ million Large Objects

    Laurenz Albe <laurenz.albe@cybertec.at> — 2024-03-16T21:59:45Z

    On Fri, 2024-03-15 at 19:18 -0400, Tom Lane wrote:
> This patch seems to have stalled out again.  In hopes of getting it
> over the finish line, I've done a bit more work to address the two
> loose ends I felt were probably essential to deal with:

Applies and builds fine.

I didn't scrutinize the code, but I gave it a spin on a database with
15 million (small) large objects.  I tried pg_upgrade --link with and
without the patch on a debug build with the default configuration.

Without the patch:

Runtime: 74.5 minutes
Memory usage: ~7GB
Disk usage: an extra 5GB dump file + log file during the dump

With the patch:

Runtime: 70 minutes
Memory usage: ~1GB
Disk usage: an extra 0.5GB during the dump

Memory usage stayed stable once it reached its peak, so no noticeable
memory leaks.

The reduced memory usage is great.  I was surprised by the difference
in disk usage: the lion's share is the dump file, and that got substantially
smaller.  But also the log file shrank considerably, because not every
individual large object gets logged.

I had a look at "perf top", and the profile looked pretty similar in
both cases.

The patch is a clear improvement.

Yours,
Laurenz Albe

  75. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-16T22:46:15Z

    Laurenz Albe <laurenz.albe@cybertec.at> writes:
> On Fri, 2024-03-15 at 19:18 -0400, Tom Lane wrote:
>> This patch seems to have stalled out again.  In hopes of getting it
>> over the finish line, I've done a bit more work to address the two
>> loose ends I felt were probably essential to deal with:

> Applies and builds fine.
> I didn't scrutinize the code, but I gave it a spin on a database with
> 15 million (small) large objects.  I tried pg_upgrade --link with and
> without the patch on a debug build with the default configuration.

Thanks for looking at it!

> Without the patch:
> Runtime: 74.5 minutes

> With the patch:
> Runtime: 70 minutes

Hm, I'd have hoped for a bit more runtime improvement.  But perhaps
not --- most of the win we saw upthread was from parallelism, and
I don't think you'd get any parallelism in a pg_upgrade with all
the data in one database.  (Perhaps there is more to do there later,
but I'm still not clear on how this should interact with the existing
cross-DB parallelism; so I'm content to leave that question for
another patch.)

			regards, tom lane

  76. Re: pg_upgrade failing for 200+ million Large Objects

    Laurenz Albe <laurenz.albe@cybertec.at> — 2024-03-17T17:57:45Z

    On Sat, 2024-03-16 at 18:46 -0400, Tom Lane wrote:
> > Without the patch:
> > Runtime: 74.5 minutes
> 
> > With the patch:
> > Runtime: 70 minutes
> 
> Hm, I'd have hoped for a bit more runtime improvement.

I did a second run with the patch, and that finished in 66 minutes,
so there is some jitter there.

I think the reduced memory footprint and the reduced transaction ID
consumption alone make this patch worthwhile.

Yours,
Laurenz Albe

  77. Re: pg_upgrade failing for 200+ million Large Objects

    Michael Banck <mbanck@gmx.net> — 2024-03-27T09:20:31Z

    Hi,

On Sat, Mar 16, 2024 at 06:46:15PM -0400, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > On Fri, 2024-03-15 at 19:18 -0400, Tom Lane wrote:
> >> This patch seems to have stalled out again.  In hopes of getting it
> >> over the finish line, I've done a bit more work to address the two
> >> loose ends I felt were probably essential to deal with:
> 
> > Applies and builds fine.
> > I didn't scrutinize the code, but I gave it a spin on a database with
> > 15 million (small) large objects.  I tried pg_upgrade --link with and
> > without the patch on a debug build with the default configuration.
> 
> Thanks for looking at it!
> 
> > Without the patch:
> > Runtime: 74.5 minutes
> 
> > With the patch:
> > Runtime: 70 minutes
> 
> Hm, I'd have hoped for a bit more runtime improvement.  

I also think that this is quite a large runtime for pg_upgrade, but the
more important savings should be the memory usage.

> But perhaps not --- most of the win we saw upthread was from
> parallelism, and I don't think you'd get any parallelism in a
> pg_upgrade with all the data in one database.  (Perhaps there is more
> to do there later, but I'm still not clear on how this should interact
> with the existing cross-DB parallelism; so I'm content to leave that
> question for another patch.)

What is the status of this? In the commitfest, this patch is marked as
"Needs Review" with Nathan as reviewer - Nathan, were you going to take
another look at this or was your mail from January 12th a full review?

My feeling is that this patch is "Ready for Committer" and it is Tom's
call to commit it during the next days or not.

I am +1 that this is an important feature/bug fix to have. Because we
have customers stuck on older versions due to their pathological large
objects usage, I did some benchmarks (jsut doing pg_dump, not
pg_upgarde) a while ago which were also very promising; however, I lost
the exact numbers/results. I am happy to do further tests if that is
required for this patch to go forward.

Also, is there a chance this is going to be back-patched? I guess it
would be enough if the ugprade target is v17 so it is less of a concern,
but it would be nice if people with millions of large objects are not
stuck until they are ready to ugprade to v17.


Michael

  78. Re: pg_upgrade failing for 200+ million Large Objects

    Laurenz Albe <laurenz.albe@cybertec.at> — 2024-03-27T09:53:51Z

    On Wed, 2024-03-27 at 10:20 +0100, Michael Banck wrote:
> Also, is there a chance this is going to be back-patched? I guess it
> would be enough if the ugprade target is v17 so it is less of a concern,
> but it would be nice if people with millions of large objects are not
> stuck until they are ready to ugprade to v17.

It is a quite invasive patch, and it adds new features (pg_restore in
bigger transaction patches), so I think this is not for backpatching,
desirable as it may seem from the usability angle.

Yours,
Laurenz Albe

  79. Re: pg_upgrade failing for 200+ million Large Objects

    Michael Banck <mbanck@gmx.net> — 2024-03-27T10:54:54Z

    Hi,

On Wed, Mar 27, 2024 at 10:53:51AM +0100, Laurenz Albe wrote:
> On Wed, 2024-03-27 at 10:20 +0100, Michael Banck wrote:
> > Also, is there a chance this is going to be back-patched? I guess it
> > would be enough if the ugprade target is v17 so it is less of a concern,
> > but it would be nice if people with millions of large objects are not
> > stuck until they are ready to ugprade to v17.
> 
> It is a quite invasive patch, and it adds new features (pg_restore in
> bigger transaction patches), so I think this is not for backpatching,
> desirable as it may seem from the usability angle.

Right, I forgot about those changes, makes sense.


Michael

  80. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-27T14:54:05Z

    Michael Banck <mbanck@gmx.net> writes:
> What is the status of this? In the commitfest, this patch is marked as
> "Needs Review" with Nathan as reviewer - Nathan, were you going to take
> another look at this or was your mail from January 12th a full review?

In my mind the ball is in Nathan's court.  I feel it's about
committable, but he might not agree.

> Also, is there a chance this is going to be back-patched?

No chance of that I'm afraid.  The patch bumps the archive version
number, because it creates TOC entries that older pg_restore would
not know what to do with.  We can't put that kind of compatibility
break into stable branches.

			regards, tom lane

  81. Re: pg_upgrade failing for 200+ million Large Objects

    Nathan Bossart <nathandbossart@gmail.com> — 2024-03-27T15:08:26Z

    On Wed, Mar 27, 2024 at 10:54:05AM -0400, Tom Lane wrote:
> Michael Banck <mbanck@gmx.net> writes:
>> What is the status of this? In the commitfest, this patch is marked as
>> "Needs Review" with Nathan as reviewer - Nathan, were you going to take
>> another look at this or was your mail from January 12th a full review?
> 
> In my mind the ball is in Nathan's court.  I feel it's about
> committable, but he might not agree.

I'll prioritize another round of review on this one.  FWIW I don't remember
having any major concerns on a previous version of the patch set I looked
at.

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

  82. Re: pg_upgrade failing for 200+ million Large Objects

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

    On Wed, Mar 27, 2024 at 10:08:26AM -0500, Nathan Bossart wrote:
> On Wed, Mar 27, 2024 at 10:54:05AM -0400, Tom Lane wrote:
>> Michael Banck <mbanck@gmx.net> writes:
>>> What is the status of this? In the commitfest, this patch is marked as
>>> "Needs Review" with Nathan as reviewer - Nathan, were you going to take
>>> another look at this or was your mail from January 12th a full review?
>> 
>> In my mind the ball is in Nathan's court.  I feel it's about
>> committable, but he might not agree.
> 
> I'll prioritize another round of review on this one.  FWIW I don't remember
> having any major concerns on a previous version of the patch set I looked
> at.

Sorry for taking so long to get back to this one.  Overall, I think the
code is in decent shape.  Nothing stands out after a couple of passes.  The
small amount of runtime improvement cited upthread is indeed a bit
disappointing, but IIUC this at least sets the stage for additional
parallelism in the future, and the memory/disk usage improvements are
nothing to sneeze at, either.

The one design point that worries me a little is the non-configurability of
--transaction-size in pg_upgrade.  I think it's fine to default it to 1,000
or something, but given how often I've had to fiddle with
max_locks_per_transaction, I'm wondering if we might regret hard-coding it.

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

  83. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-01T19:28:26Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> Sorry for taking so long to get back to this one.  Overall, I think the
> code is in decent shape.

Thanks for looking at it!

> The one design point that worries me a little is the non-configurability of
> --transaction-size in pg_upgrade.  I think it's fine to default it to 1,000
> or something, but given how often I've had to fiddle with
> max_locks_per_transaction, I'm wondering if we might regret hard-coding it.

Well, we could add a command-line switch to pg_upgrade, but I'm
unconvinced that it'd be worth the trouble.  I think a very large
fraction of users invoke pg_upgrade by means of packager-supplied
scripts that are unlikely to provide a way to pass through such
a switch.  I'm inclined to say let's leave it as-is until we get
some actual field requests for a switch.

			regards, tom lane

  84. Re: pg_upgrade failing for 200+ million Large Objects

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

    On Mon, Apr 01, 2024 at 03:28:26PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> The one design point that worries me a little is the non-configurability of
>> --transaction-size in pg_upgrade.  I think it's fine to default it to 1,000
>> or something, but given how often I've had to fiddle with
>> max_locks_per_transaction, I'm wondering if we might regret hard-coding it.
> 
> Well, we could add a command-line switch to pg_upgrade, but I'm
> unconvinced that it'd be worth the trouble.  I think a very large
> fraction of users invoke pg_upgrade by means of packager-supplied
> scripts that are unlikely to provide a way to pass through such
> a switch.  I'm inclined to say let's leave it as-is until we get
> some actual field requests for a switch.

Okay.  I'll let you know if I see anything.  IIRC usually the pg_dump side
of pg_upgrade is more prone to lock exhaustion, so you may very well be
right that this is unnecessary.

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

  85. Re: pg_upgrade failing for 200+ million Large Objects

    Justin Pryzby <pryzby@telsasoft.com> — 2024-07-24T14:17:51Z

    On Mon, Apr 01, 2024 at 03:28:26PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
> > The one design point that worries me a little is the non-configurability of
> > --transaction-size in pg_upgrade.  I think it's fine to default it to 1,000
> > or something, but given how often I've had to fiddle with
> > max_locks_per_transaction, I'm wondering if we might regret hard-coding it.
> 
> Well, we could add a command-line switch to pg_upgrade, but I'm
> unconvinced that it'd be worth the trouble.  I think a very large
> fraction of users invoke pg_upgrade by means of packager-supplied
> scripts that are unlikely to provide a way to pass through such
> a switch.  I'm inclined to say let's leave it as-is until we get
> some actual field requests for a switch.

I've been importing our schemas and doing upgrade testing, and was
surprised when a postgres backend was killed for OOM during pg_upgrade:

Killed process 989302 (postgres) total-vm:5495648kB, anon-rss:5153292kB, ...

Upgrading from v16 => v16 doesn't use nearly as much RAM.

While tracking down the responsible commit, I reproduced the problem
using a subset of tables; at 959b38d770, the backend process used
~650 MB RAM, and at its parent commit used at most ~120 MB.

959b38d770b Invent --transaction-size option for pg_restore.

By changing RESTORE_TRANSACTION_SIZE to 100, backend RAM use goes to
180 MB during pg_upgrade, which is reasonable.

With partitioning, we have a lot of tables, some of them wide (126
partitioned tables, 8942 childs, total 1019315 columns).  I didn't track
if certain parts of our schema contribute most to the high backend mem
use, just that it's now 5x (while testing a subset) to 50x higher.

We'd surely prefer that the transaction size be configurable.

-- 
Justin

  86. Re: pg_upgrade failing for 200+ million Large Objects

    Alexander Korotkov <aekorotkov@gmail.com> — 2024-07-26T19:53:30Z

    Hi, Justin!

Thank you for sharing this.

On Wed, Jul 24, 2024 at 5:18 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Mon, Apr 01, 2024 at 03:28:26PM -0400, Tom Lane wrote:
> > Nathan Bossart <nathandbossart@gmail.com> writes:
> > > The one design point that worries me a little is the non-configurability of
> > > --transaction-size in pg_upgrade.  I think it's fine to default it to 1,000
> > > or something, but given how often I've had to fiddle with
> > > max_locks_per_transaction, I'm wondering if we might regret hard-coding it.
> >
> > Well, we could add a command-line switch to pg_upgrade, but I'm
> > unconvinced that it'd be worth the trouble.  I think a very large
> > fraction of users invoke pg_upgrade by means of packager-supplied
> > scripts that are unlikely to provide a way to pass through such
> > a switch.  I'm inclined to say let's leave it as-is until we get
> > some actual field requests for a switch.
>
> I've been importing our schemas and doing upgrade testing, and was
> surprised when a postgres backend was killed for OOM during pg_upgrade:
>
> Killed process 989302 (postgres) total-vm:5495648kB, anon-rss:5153292kB, ...
>
> Upgrading from v16 => v16 doesn't use nearly as much RAM.
>
> While tracking down the responsible commit, I reproduced the problem
> using a subset of tables; at 959b38d770, the backend process used
> ~650 MB RAM, and at its parent commit used at most ~120 MB.
>
> 959b38d770b Invent --transaction-size option for pg_restore.
>
> By changing RESTORE_TRANSACTION_SIZE to 100, backend RAM use goes to
> 180 MB during pg_upgrade, which is reasonable.
>
> With partitioning, we have a lot of tables, some of them wide (126
> partitioned tables, 8942 childs, total 1019315 columns).  I didn't track
> if certain parts of our schema contribute most to the high backend mem
> use, just that it's now 5x (while testing a subset) to 50x higher.

Do you think there is a way to anonymize the schema and share it?

> We'd surely prefer that the transaction size be configurable.

I think we can add an option to pg_upgrade.  But I wonder if there is
something else we can do.  It seems that restoring some objects is
much more expensive than restoring others.  It would be nice to
identify such cases and check which memory contexts are growing and
why.  It would be helpful if you could share your data schema, so we
could dig into it.

I can imagine we need to count some DDL commands in aspect of maximum
restore transaction size in a different way than others.  Also, we
probably need to change the default restore transaction size.

------
Regards,
Alexander Korotkov
Supabase

  87. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-26T20:05:50Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Wed, Jul 24, 2024 at 5:18 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>> We'd surely prefer that the transaction size be configurable.

> I think we can add an option to pg_upgrade.  But I wonder if there is
> something else we can do.

Yeah, I'm not enamored of adding a command-line option, if only
because I think a lot of people invoke pg_upgrade through
vendor-provided scripts that aren't going to cooperate with that.
If we can find some way to make it adapt without help, that
would be much better.

			regards, tom lane

  88. Re: pg_upgrade failing for 200+ million Large Objects

    Justin Pryzby <pryzby@telsasoft.com> — 2024-07-26T20:36:20Z

    On Wed, Jul 24, 2024 at 09:17:51AM -0500, Justin Pryzby wrote:
> With partitioning, we have a lot of tables, some of them wide (126
> partitioned tables, 8942 childs, total 1019315 columns).

On Fri, Jul 26, 2024 at 10:53:30PM +0300, Alexander Korotkov wrote:
> It would be nice to identify such cases and check which memory contexts are
> growing and why.

I reproduced the problem with this schema:

SELECT format('CREATE TABLE p(i int, %s) PARTITION BY RANGE(i)', array_to_string(a, ', ')) FROM (SELECT array_agg(format('i%s int', i))a FROM generate_series(1,999)i);
SELECT format('CREATE TABLE t%s PARTITION OF p FOR VALUES FROM (%s)TO(%s)', i,i,i+1) FROM generate_series(1,999)i;

This used over 4 GB of RAM.
3114201 pryzbyj   20   0 5924520   4.2g  32476 T   0.0  53.8   0:27.35 postgres: pryzbyj postgres [local] UPDATE

The large context is:
2024-07-26 15:22:19.280 CDT [3114201] LOG:  level: 1; CacheMemoryContext: 5211209088 total in 50067 blocks; 420688 free (14 chunks); 5210788400 used

Note that there seemed to be no issue when I created 999 tables without
partitioning:

SELECT format('CREATE TABLE t%s(LIKE p)', i,i,i+1) FROM generate_series(1,999)i;

-- 
Justin

  89. Re: pg_upgrade failing for 200+ million Large Objects

    Alexander Korotkov <aekorotkov@gmail.com> — 2024-07-26T21:42:23Z

    On Fri, Jul 26, 2024 at 11:36 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Wed, Jul 24, 2024 at 09:17:51AM -0500, Justin Pryzby wrote:
> > With partitioning, we have a lot of tables, some of them wide (126
> > partitioned tables, 8942 childs, total 1019315 columns).
>
> On Fri, Jul 26, 2024 at 10:53:30PM +0300, Alexander Korotkov wrote:
> > It would be nice to identify such cases and check which memory contexts are
> > growing and why.
>
> I reproduced the problem with this schema:
>
> SELECT format('CREATE TABLE p(i int, %s) PARTITION BY RANGE(i)', array_to_string(a, ', ')) FROM (SELECT array_agg(format('i%s int', i))a FROM generate_series(1,999)i);
> SELECT format('CREATE TABLE t%s PARTITION OF p FOR VALUES FROM (%s)TO(%s)', i,i,i+1) FROM generate_series(1,999)i;
>
> This used over 4 GB of RAM.
> 3114201 pryzbyj   20   0 5924520   4.2g  32476 T   0.0  53.8   0:27.35 postgres: pryzbyj postgres [local] UPDATE
>
> The large context is:
> 2024-07-26 15:22:19.280 CDT [3114201] LOG:  level: 1; CacheMemoryContext: 5211209088 total in 50067 blocks; 420688 free (14 chunks); 5210788400 used
>
> Note that there seemed to be no issue when I created 999 tables without
> partitioning:
>
> SELECT format('CREATE TABLE t%s(LIKE p)', i,i,i+1) FROM generate_series(1,999)i;

Thank you!  That was quick.
I'm looking into this.

------
Regards,
Alexander Korotkov
Supabase

  90. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-26T22:37:10Z

    Justin Pryzby <pryzby@telsasoft.com> writes:
> On Fri, Jul 26, 2024 at 10:53:30PM +0300, Alexander Korotkov wrote:
>> It would be nice to identify such cases and check which memory contexts are
>> growing and why.

> I reproduced the problem with this schema:

> SELECT format('CREATE TABLE p(i int, %s) PARTITION BY RANGE(i)', array_to_string(a, ', ')) FROM (SELECT array_agg(format('i%s int', i))a FROM generate_series(1,999)i);
> SELECT format('CREATE TABLE t%s PARTITION OF p FOR VALUES FROM (%s)TO(%s)', i,i,i+1) FROM generate_series(1,999)i;

> This used over 4 GB of RAM.

Interesting.  This doesn't bloat particularly much in a regular
pg_restore, even with --transaction-size=1000; but it does in
pg_upgrade, as you say.  I found that the bloat was occurring
during these long sequences of UPDATE commands issued by pg_upgrade:

-- For binary upgrade, recreate inherited column.
UPDATE pg_catalog.pg_attribute
SET attislocal = false
WHERE attname = 'i'
  AND attrelid = '\"public\".\"t139\"'::pg_catalog.regclass;

-- For binary upgrade, recreate inherited column.
UPDATE pg_catalog.pg_attribute
SET attislocal = false
WHERE attname = 'i1'
  AND attrelid = '\"public\".\"t139\"'::pg_catalog.regclass;

-- For binary upgrade, recreate inherited column.
UPDATE pg_catalog.pg_attribute
SET attislocal = false
WHERE attname = 'i2'
  AND attrelid = '\"public\".\"t139\"'::pg_catalog.regclass;

I think the problem is basically that each one of these commands
causes a relcache inval, for which we can't reclaim space right
away, so that we end up consuming O(N^2) cache space for an
N-column inherited table.

It's fairly easy to fix things so that this example doesn't cause
that to happen: we just need to issue these updates as one command
not N commands per table.  See attached.  However, I fear this should
just be considered a draft, because the other code for binary upgrade
in the immediate vicinity is just as aggressively stupid and
unoptimized as this bit, and can probably also be driven to O(N^2)
behavior with enough CHECK constraints etc.  We've gone out of our way
to make ALTER TABLE capable of handling many updates to a table's DDL
in one command, but whoever wrote this code appears not to have read
that memo, or at least to have believed that performance of pg_upgrade
isn't of concern.

> Note that there seemed to be no issue when I created 999 tables without
> partitioning:
> SELECT format('CREATE TABLE t%s(LIKE p)', i,i,i+1) FROM generate_series(1,999)i;

Yeah, because then we don't need to play games with attislocal.

			regards, tom lane

  91. Re: pg_upgrade failing for 200+ million Large Objects

    Alexander Korotkov <aekorotkov@gmail.com> — 2024-07-26T22:55:00Z

    On Sat, Jul 27, 2024 at 1:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > On Fri, Jul 26, 2024 at 10:53:30PM +0300, Alexander Korotkov wrote:
> >> It would be nice to identify such cases and check which memory contexts are
> >> growing and why.
>
> > I reproduced the problem with this schema:
>
> > SELECT format('CREATE TABLE p(i int, %s) PARTITION BY RANGE(i)', array_to_string(a, ', ')) FROM (SELECT array_agg(format('i%s int', i))a FROM generate_series(1,999)i);
> > SELECT format('CREATE TABLE t%s PARTITION OF p FOR VALUES FROM (%s)TO(%s)', i,i,i+1) FROM generate_series(1,999)i;
>
> > This used over 4 GB of RAM.
>
> Interesting.  This doesn't bloat particularly much in a regular
> pg_restore, even with --transaction-size=1000; but it does in
> pg_upgrade, as you say.  I found that the bloat was occurring
> during these long sequences of UPDATE commands issued by pg_upgrade:
>
> -- For binary upgrade, recreate inherited column.
> UPDATE pg_catalog.pg_attribute
> SET attislocal = false
> WHERE attname = 'i'
>   AND attrelid = '\"public\".\"t139\"'::pg_catalog.regclass;
>
> -- For binary upgrade, recreate inherited column.
> UPDATE pg_catalog.pg_attribute
> SET attislocal = false
> WHERE attname = 'i1'
>   AND attrelid = '\"public\".\"t139\"'::pg_catalog.regclass;
>
> -- For binary upgrade, recreate inherited column.
> UPDATE pg_catalog.pg_attribute
> SET attislocal = false
> WHERE attname = 'i2'
>   AND attrelid = '\"public\".\"t139\"'::pg_catalog.regclass;
>
> I think the problem is basically that each one of these commands
> causes a relcache inval, for which we can't reclaim space right
> away, so that we end up consuming O(N^2) cache space for an
> N-column inherited table.

I was about to report the same.

> It's fairly easy to fix things so that this example doesn't cause
> that to happen: we just need to issue these updates as one command
> not N commands per table.  See attached.  However, I fear this should
> just be considered a draft, because the other code for binary upgrade
> in the immediate vicinity is just as aggressively stupid and
> unoptimized as this bit, and can probably also be driven to O(N^2)
> behavior with enough CHECK constraints etc.  We've gone out of our way
> to make ALTER TABLE capable of handling many updates to a table's DDL
> in one command, but whoever wrote this code appears not to have read
> that memo, or at least to have believed that performance of pg_upgrade
> isn't of concern.

I was thinking about counting actual number of queries, not TOC
entries for transaction number as a more universal solution.  But that
would require usage of psql_scan() or writing simpler alternative for
this particular purpose.  That looks quite annoying.  What do you
think?


------
Regards,
Alexander Korotkov
Supabase

  92. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-26T23:06:21Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Sat, Jul 27, 2024 at 1:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It's fairly easy to fix things so that this example doesn't cause
>> that to happen: we just need to issue these updates as one command
>> not N commands per table.

> I was thinking about counting actual number of queries, not TOC
> entries for transaction number as a more universal solution.  But that
> would require usage of psql_scan() or writing simpler alternative for
> this particular purpose.  That looks quite annoying.  What do you
> think?

The assumption underlying what we're doing now is that the number
of SQL commands per TOC entry is limited.  I'd prefer to fix the
code so that that assumption is correct, at least in normal cases.
I confess I'd not looked closely enough at the binary-upgrade support
code to realize it wasn't correct already :-(.  If we go that way,
we can fix this while also making pg_upgrade faster rather than
slower.  I also expect that it'll be a lot simpler than putting
a full SQL parser in pg_restore.

			regards, tom lane

  93. Re: pg_upgrade failing for 200+ million Large Objects

    Alexander Korotkov <aekorotkov@gmail.com> — 2024-07-27T03:00:47Z

    On Sat, Jul 27, 2024 at 2:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > On Sat, Jul 27, 2024 at 1:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> It's fairly easy to fix things so that this example doesn't cause
> >> that to happen: we just need to issue these updates as one command
> >> not N commands per table.
>
> > I was thinking about counting actual number of queries, not TOC
> > entries for transaction number as a more universal solution.  But that
> > would require usage of psql_scan() or writing simpler alternative for
> > this particular purpose.  That looks quite annoying.  What do you
> > think?
>
> The assumption underlying what we're doing now is that the number
> of SQL commands per TOC entry is limited.  I'd prefer to fix the
> code so that that assumption is correct, at least in normal cases.
> I confess I'd not looked closely enough at the binary-upgrade support
> code to realize it wasn't correct already :-(.  If we go that way,
> we can fix this while also making pg_upgrade faster rather than
> slower.  I also expect that it'll be a lot simpler than putting
> a full SQL parser in pg_restore.

I'm good with that as soon as we're not going to meet many cases of
high number SQL commands per TOC entry.

J4F, I have an idea to count number of ';' sings and use it for
transaction size counter, since it is as upper bound estimate of
number of SQL commands :-)

------
Regards,
Alexander Korotkov
Supabase

  94. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-27T03:08:06Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> J4F, I have an idea to count number of ';' sings and use it for
> transaction size counter, since it is as upper bound estimate of
> number of SQL commands :-)

Hmm ... that's not a completely silly idea.  Let's keep it in
the back pocket in case we can't easily reduce the number of
SQL commands in some cases.

It's late here, and I've got some other commitments tomorrow,
but I'll try to produce a patch to merge more of the SQL
commands in a day or two.

			regards, tom lane

  95. Re: pg_upgrade failing for 200+ million Large Objects

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-28T21:24:29Z

    I wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
>> J4F, I have an idea to count number of ';' sings and use it for
>> transaction size counter, since it is as upper bound estimate of
>> number of SQL commands :-)

> Hmm ... that's not a completely silly idea.  Let's keep it in
> the back pocket in case we can't easily reduce the number of
> SQL commands in some cases.

After poking at this for awhile, we can fix Justin's example
case by avoiding repeated UPDATEs on pg_attribute, so I think
we should do that.  It seems clearly a win, with no downside
other than a small increment of complexity in pg_dump.

However, that's probably not sufficient to mark this issue
as closed.  It seems likely that there are other patterns
that would cause backend memory bloat.  One case that I found
is tables with a lot of inherited constraints (not partitions,
but old-style inheritance).  For example, load the output of
this Perl script into a database:

-----
for (my $i = 0; $i < 100; $i++)
{
	print "CREATE TABLE test_inh_check$i (\n";
	for (my $j = 0; $j < 1000; $j++)
	{
		print "a$j float check (a$j > 10.2),\n";
	}
	print "b float);\n";
	print "CREATE TABLE test_inh_check_child$i() INHERITS(test_inh_check$i);\n";
}
-----

pg_dump is horrendously slow on this, thanks to O(N^2) behavior in
ruleutils.c, and pg_upgrade is worse --- and leaks memory too in
HEAD/v17.  The slowness was there before, so I think the lack of
field complaints indicates that this isn't a real-world use case.
Still, it's bad if pg_upgrade fails when it would not have before,
and there may be other similar issues.

So I'm forced to the conclusion that we'd better make the transaction
size adaptive as per Alexander's suggestion.

In addition to the patches attached, I experimented with making
dumpTableSchema fold all the ALTER TABLE commands for a single table
into one command.  That's do-able without too much effort, but I'm now
convinced that we shouldn't.  It would break the semicolon-counting
hack for detecting that tables like these involve extra work.
I'm also not very confident that the backend won't have trouble with
ALTER TABLE commands containing hundreds of subcommands.  That's
something we ought to work on probably, but it's not a project that
I want to condition v17 pg_upgrade's stability on.

Anyway, proposed patches attached.  0001 is some trivial cleanup
that I noticed while working on the failed single-ALTER-TABLE idea.
0002 merges the catalog-UPDATE commands that dumpTableSchema issues,
and 0003 is Alexander's suggestion.

			regards, tom lane

  96. Re: pg_upgrade failing for 200+ million Large Objects

    Alexander Korotkov <aekorotkov@gmail.com> — 2024-07-31T13:39:19Z

    On Mon, Jul 29, 2024 at 12:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So I'm forced to the conclusion that we'd better make the transaction
> size adaptive as per Alexander's suggestion.
>
> In addition to the patches attached, I experimented with making
> dumpTableSchema fold all the ALTER TABLE commands for a single table
> into one command.  That's do-able without too much effort, but I'm now
> convinced that we shouldn't.  It would break the semicolon-counting
> hack for detecting that tables like these involve extra work.
> I'm also not very confident that the backend won't have trouble with
> ALTER TABLE commands containing hundreds of subcommands.  That's
> something we ought to work on probably, but it's not a project that
> I want to condition v17 pg_upgrade's stability on.
>
> Anyway, proposed patches attached.  0001 is some trivial cleanup
> that I noticed while working on the failed single-ALTER-TABLE idea.
> 0002 merges the catalog-UPDATE commands that dumpTableSchema issues,
> and 0003 is Alexander's suggestion.

Nice to see you picked up my idea.  I took a look over the patchset.
Looks good to me.

------
Regards,
Alexander Korotkov
Supabase

  97. Re: pg_upgrade failing for 200+ million Large Objects

    PP L <flowerstair@gmail.com> — 2026-03-23T19:47:28Z

    Hello hackers,

I wanted to revive this thread specifically around the attislocal
optimization discussion. As part of
https://github.com/postgres/postgres/commit/b3f0e0503f3, we now batch all
the attislocal UPDATEs together, hence making it more performant.

I think we might be able to go one step further and completely skip the
attislocal UPDATE for partition tables. This is because the attislocal
UPDATE is done immediately after 'CREATE TABLE', during the 'ATTACH
PARTITION' step(see attislocal being set to false in
MergeAttributesIntoExisting). The UPDATEs emitted by pg_dump are therefore
redundant. Even with batching, the single UPDATE still modifies N(no of
columns) rows causing N relcache invalidations. This same workflow is then
repeated by ATTACH PARTITION causing another N relcache invalidations.

Skipping the attislocal UPDATE definitely speeds up the runtime if there
are a lot of partition tables because we will avoid quite a lot of relcache
invalidations and rebuild calls. Since this optimization removes the
attislocal UPDATE completely, the effect will be even more pronounced for
wider partition tables.

There's already precedent for this:
* attinhcount is never explicitly set by pg_dump. It is only modified by
MergeAttributesIntoExisting during ATTACH PARTITION
* conislocal for CHECK constraints is explicitly not fixed for partitions.
See comment "No need to fix conislocal: ATTACH PARTITION does that" in
dumpTableSchema

The only risk I can foresee is the window between CREATE TABLE and ATTACH
PARTITION where attislocal will be incorrectly set to true. But I think
this window is small enough to not worry about since ATTACH PARTITION
immediately succeeds CREATE TABLE(maybe barring some other minor updates)

Here is a simple patch
```
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 137161aa5e0..d3d7403228a 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -17734,7 +17734,8 @@ dumpTableSchema(Archive *fout, const TableInfo
*tbinfo)
                        for (j = 0; j < tbinfo->numatts; j++)
                        {
                                if (!tbinfo->attisdropped[j] &&
-                                       !tbinfo->attislocal[j])
+                                       !tbinfo->attislocal[j] &&
+                                       !tbinfo->ispartition)
                                {
                                        if (firstitem)
                                        {
```

I tried a few experiments on my local macbook and noticed an improvement of
around ~33-36% (% can vary mostly depending on the number of columns)

Setup(master branch): 300 partitioned root tables with 200 leaves each =
60000 partition tables

300 columns per partition:
    baseline    : ~30 mins
    with patch : ~20 mins (~33% faster)

700 columns per partition:
    baseline   : ~66 mins
    with patch : ~42 mins (~36% faster)

Thanks
Nikhil
Broadcom Inc.


On Mon, 23 Mar 2026 at 11:50, Alexander Korotkov <aekorotkov@gmail.com>
wrote:

> On Mon, Jul 29, 2024 at 12:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > So I'm forced to the conclusion that we'd better make the transaction
> > size adaptive as per Alexander's suggestion.
> >
> > In addition to the patches attached, I experimented with making
> > dumpTableSchema fold all the ALTER TABLE commands for a single table
> > into one command.  That's do-able without too much effort, but I'm now
> > convinced that we shouldn't.  It would break the semicolon-counting
> > hack for detecting that tables like these involve extra work.
> > I'm also not very confident that the backend won't have trouble with
> > ALTER TABLE commands containing hundreds of subcommands.  That's
> > something we ought to work on probably, but it's not a project that
> > I want to condition v17 pg_upgrade's stability on.
> >
> > Anyway, proposed patches attached.  0001 is some trivial cleanup
> > that I noticed while working on the failed single-ALTER-TABLE idea.
> > 0002 merges the catalog-UPDATE commands that dumpTableSchema issues,
> > and 0003 is Alexander's suggestion.
>
> Nice to see you picked up my idea.  I took a look over the patchset.
> Looks good to me.
>
> ------
> Regards,
> Alexander Korotkov
> Supabase
>
>
>
>
>