Subject: Re: pg_upgrade failing for 200+ million Large Objects
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Bruce Momjian <bruce@momjian.us>, Zhihong Yu <zyu@yugabyte.com>,
 Andrew Dunstan <andrew@dunslane.net>, Magnus Hagander <magnus@hagander.net>,
 Robins Tharakan <tharakan@gmail.com>,
 Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <181907.1616253799@sss.pgh.pa.us>
 <147fa478-510b-18ef-5323-9c1725b2493c@wi3ck.info>
 <c2a43a97-e551-ea6d-7a4f-a4709b4e0cbd@dunslane.net>
 <5bdcb010-ecdd-c69a-b441-68002fc38483@wi3ck.info>
 <ee7d96b8-7b0e-bb76-9724-900606efe69a@dunslane.net>
 <CALNJ-vSPnudYcS8YowHeq7VAf7PBRyu0vpsbmb_VvTJgguVyHA@mail.gmail.com>
 <3886649c-c77d-dfd7-08a4-d1606bc71254@wi3ck.info>
 <91ccdb0d-42fd-7413-4e7c-3d6445655d2e@wi3ck.info>
 <20210323145628.GD579@momjian.us>
 <8d8d3961-8e8b-3dbe-f911-6f418c5fb1d3@wi3ck.info>
 <20210323180646.GG579@momjian.us>
 <91b02dc1-f0d9-e50d-849c-18d9a66484fb@wi3ck.info>
 <985941.1616524546@sss.pgh.pa.us>
 <c0f5fde5-5cad-65ed-dd30-cd852301d074@wi3ck.info>
 <986904.1616525964@sss.pgh.pa.us>
 <6cccaa33-c263-b8a2-b064-985605d33d25@wi3ck.info>
 <988415.1616528159@sss.pgh.pa.us>
From: Jan Wieck <jan@wi3ck.info>
Message-ID: <872315a8-99fc-da4e-463d-784cfb5a025d@wi3ck.info>
Date: Tue, 23 Mar 2021 15:59:48 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <988415.1616528159@sss.pgh.pa.us>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/872315a8-99fc-da4e-463d-784cfb5a025d%40wi3ck.info>
Precedence: bulk

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