Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1thttM-008Gbq-7i for pgsql-hackers@arkaria.postgresql.org; Tue, 11 Feb 2025 17:17:48 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1thttK-00DHir-LK for pgsql-hackers@arkaria.postgresql.org; Tue, 11 Feb 2025 17:17:46 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1thttK-00DHh4-2A for pgsql-hackers@lists.postgresql.org; Tue, 11 Feb 2025 17:17:46 +0000 Received: from mail-qt1-x82b.google.com ([2607:f8b0:4864:20::82b]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1thttH-000G2z-2F for pgsql-hackers@lists.postgresql.org; Tue, 11 Feb 2025 17:17:44 +0000 Received: by mail-qt1-x82b.google.com with SMTP id d75a77b69052e-47194858ee8so19424241cf.0 for ; Tue, 11 Feb 2025 09:17:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1739294263; x=1739899063; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=b2mLMHwHo4ddNtGteZlmFsGw323OZRRb1XdC3DcHD2E=; b=EbSz9ejoPgSuzmkb2SsnPRnPuEvs7EVLycH+2yyVIxJE+0WMhvMKxWiWsbcTnxVR5n szMhga/xrga+NHSu+v5KukSFB2KctGsoPoZdsAxm1SO+ko74IICHmEd5B+9QGMwxVJpB vUFk4OgHezf2lIioC76Xwzog2XXMJ2IX7T7EzWAef6MuvvxBtpR0iwzeicPVHJOOpZaM tpMhOXMXQnxs+CC8CtVQjVGET9Tbo2el8P3YYAogZKNYE/FzYEk5j3qctM1AnVrrfH+7 1/D0s+ucVMjJbQZ9gkz+eq63aEk98rsdcW15xrNDI12ajDWCElhDmrCXhpKOZm5qF/g4 gJzQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1739294263; x=1739899063; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=b2mLMHwHo4ddNtGteZlmFsGw323OZRRb1XdC3DcHD2E=; b=hqS83V35Xkd3mvTCkpWTDCwVWvh2MWDJGGHDdlRMLHWua+lptDpasFDztxGpiJDNx0 cKmdldKFknS2mxH6xpjG/Go4s5e/+TRQwJq4sxoiumT1yVUah6AiCC5x8zJ0DWRqgoOZ D/4iovammRMp0pdFIevjTaYh5xxw2w89K9LBXmEl2xDeKQ0OQODXouk1okd966LeeqOl QYxALc/r6t7YJYCIXLDvKcH8GpOZktds3KWqSWXLEJktXNmgTw8jl54HIkWuG2u2ThSK aqtgoQczyzkAJurlJXaM6lTUoafqM8/qbojdMhJzRMoWAsXbZiQrjbg72pJLYcByZqHZ 7NNg== X-Forwarded-Encrypted: i=1; AJvYcCUnMyJXtuw3xJ7G4pKt0Ze2LxB+6UhxW+wDueIxU3xyWK3dRUTep+/RyF03S1fSNgnGEmy78Xsk9wu1bgb4@lists.postgresql.org X-Gm-Message-State: AOJu0Yw7sc9/9V2mLdH/ClQSqbVY9nSFwVs8DwGrJCR/EPEn6JVXo93u Qufj/eBFU4a+EHwqhIevcIBu2AO67sPuYLWAoSBxvJPnoy515WGvqMvC/6Hiurr7Xm7+GpxlA+8 /XMrNaGbWWfh4nCgRaG7UUQq8H24= X-Gm-Gg: ASbGnct2hPfyg7OuiBcd3kiHe6uyWcvH0oEo41gt1F8Sl7r+VfB9yeB2eqUP5j9guLm upbrk258rEdW4y9G6JcVSMJV3DRR/qI5xwSA+WGNSrUDLw5+U+xaKv1K2DcWPYkGZztVquYr9SZ mqnNtKUDRGs4x3/6HgPFrIGxqi49Lk4Q== X-Google-Smtp-Source: AGHT+IEBx7dLIGT4GN7J9pSiYuCqxd39bJinjQfcWoMaombgQ2uQBOK2T7S5aYl7UOVRFCHggmPnMYkaYzWB77sfjb8= X-Received: by 2002:a05:622a:148e:b0:462:b7c9:10e with SMTP id d75a77b69052e-471a1288c90mr54500291cf.13.1739294262641; Tue, 11 Feb 2025 09:17:42 -0800 (PST) MIME-Version: 1.0 References: <202501110844.5ztsym4vbflm@alvherre.pgsql> In-Reply-To: From: Mahendra Singh Thalor Date: Tue, 11 Feb 2025 22:47:29 +0530 X-Gm-Features: AWEUYZmtfbhEKEZX2H1Zd9cFbmRqldwWYIVrMN1HdcBz5zq_pUoXKMCXn1utQB0 Message-ID: Subject: Re: Non-text mode for pg_dumpall To: jian he Cc: Srinath Reddy , pgsql-hackers@lists.postgresql.org Content-Type: multipart/alternative; boundary="000000000000af4da6062de1004b" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000af4da6062de1004b Content-Type: text/plain; charset="UTF-8" On Tue, 11 Feb 2025 at 20:40, jian he wrote: > > hi. > review based on v16. > > because of > https://postgr.es/m/CAFC+b6pWQiSL+3rvLxN9vhC8aONp4OV9c6u+BVD6kmWmDbd1WQ@mail.gmail.com > > in copy_global_file_to_out_file, now it is: > if (strcmp(outfile, "-") == 0) > OPF = stdout; > I am confused, why "-" means stdout. > ``touch ./- `` command works fine. > i think dash is not special character, you may see > https://stackoverflow.com/a/40650391/15603477 "-" is used for stdout. This is mentioned in the doc. pg_restore link > -f *filename* > --file=*filename* > > Specify output file for generated script, or for the listing when used > with -l. Use - for stdout. > > > > + /* Create a subdirectory with 'databases' name under main directory. */ > + if (mkdir(db_subdir, 0755) != 0) > + pg_log_error("could not create subdirectory \"%s\": %m", db_subdir); > here we should use pg_fatal? Yes, we should use pg_fatal. > > > pg_log_info("executing %s", sqlstatement.data); > change to > pg_log_info("executing query: %s", sqlstatement.data); > message would be more similar to the next pg_log_error(...) message. Okay. > > > + /* > + * User is suggested to use single database dump for --list option. > + */ > + if (opts->tocSummary) > + pg_fatal("option -l/--list cannot be used when using dump of pg_dumpall"); > maybe change to > + pg_fatal("option -l/--list cannot be used when restoring multiple databases"); okay. > > $BIN10/pg_restore --format=directory --list dir10_x > if the directory only has one database, then we can actually print out > the tocSummary. > if the directory has more than one database then pg_fatal. > To tolerate this corner case (only one database) means that pg_restore > --list requires a DB connection, > but I am not sure that is fine. > anyway, the attached patch allows this corner case. No, we don't need this corner case. If a user wants to restore a single database with --list option, then the user should give a particular dump file with pg_restore. > > > PrintTOCSummary can only print out summary for a single database. > so we don't need to change PrintTOCSummary. > > > + /* > + * To restore multiple databases, -C (create database) option should > be specified > + * or all databases should be created before pg_restore. > + */ > + if (opts->createDB != 1) > + pg_log_info("restoring dump of pg_dumpall without -C option, there > might be multiple databases in directory."); > > we can change it to > + if (opts->createDB != 1 && num_db_restore > 0) > + pg_log_info("restoring multiple databases without -C option."); okay. > > > Bug. > when pg_restore --globals-only can be applied when we are restoring a > single database (can be an output of pg_dump). As of now, we are ignoring this option. We can add an error in the "else" part of the global.dat file. Ex: option --globals-only is only supported with dump of pg_dumpall. Similarly --exclude-database also. > > > There are some tests per https://commitfest.postgresql.org/52/5495, I > will check it later. > The attached patch is the change for the above reviews. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com --000000000000af4da6062de1004b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Tue, 11 Feb 2025 at 20:40, jian he <jian.universality@gmail.com> wrote:>
> hi.
> review based on v16.
>
> because of<= br>> https://postgr.es/m/CAFC+b6pWQiSL+3rvLxN9vh= C8aONp4OV9c6u+BVD6kmWmDbd1WQ@mail.gmail.com
>
> in copy_glo= bal_file_to_out_file, now it is:
> =C2=A0 =C2=A0 if (strcmp(outfile, = "-") =3D=3D 0)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 OPF =3D stdout= ;
> I am confused, why "-" means stdout.
> ``touch ./= - `` command works fine.
> i think dash is not special character, you= may see
> = https://stackoverflow.com/a/40650391/15603477

"= -" is used for stdout. This is mentioned=C2=A0in the doc.
pg_res= tore link
<= code class=3D"gmail-option" style=3D"box-sizing:border-box;font-family:mono= space,monospace;font-size:1em;border-radius:0.25rem;margin:0.6rem 0px">-f= =C2=A0filename=
--file=3Dfilename

Speci= fy output file for generated script, or for the listing when used with=C2= =A0= -l. Use=C2=A0-=C2=A0for=C2=A0stdout.

>
>
> + /* Create a subdirectory with 'databases' n= ame under main directory. */
> + if (mkdir(db_subdir, 0755) !=3D 0)> + pg_log_error("could not create subdirectory \"%s\":= %m", db_subdir);
> here we should use pg_fatal?

<= /div>
Yes, we should use pg_fatal.

>
>
> p= g_log_info("executing %s", sqlstatement.data);
> change to<= br>> pg_log_info("executing query: %s", sqlstatement.data);> message would be more similar to the next pg_log_error(...) message.<= /div>

Okay.

>
>
> + /*
&= gt; + * User is suggested to use single database dump for --list option.> + */
> + if (opts->tocSummary)
> + pg_fatal("opti= on -l/--list cannot be used when using dump of pg_dumpall");
> m= aybe change to
> + pg_fatal("option -l/--list cannot be used whe= n restoring multiple databases");

okay.
=

>
> $BIN10/pg_restore --format=3Ddirectory --list dir10_x=
> if the directory only has one database, then we can actually print= out
> the tocSummary.
> if the directory has more than one dat= abase then pg_fatal.
> To tolerate this corner case (only one databas= e) means that pg_restore
> --list requires a DB connection,
> b= ut I am not sure that is fine.
> anyway, the attached patch allows th= is corner case.

No, we don't=C2=A0need this co= rner case. If a user wants to restore a single=C2=A0database with --list op= tion, then the user should give a particular dump file with pg_restore.

>
>
> PrintTOCSummary can only print out summary = for a single database.
> so we don't need to change PrintTOCSumma= ry.
>
>
> + /*
> + * To restore multiple databases,= -C (create database) option should
> be specified
> + * or all= databases should be created before pg_restore.
> + */
> + if (= opts->createDB !=3D 1)
> + pg_log_info("restoring dump of pg_= dumpall without -C option, there
> might be multiple databases in dir= ectory.");
>
> we can change it to
> + if (opts->= createDB !=3D 1 && num_db_restore > 0)
> + pg_log_info(&qu= ot;restoring multiple databases without -C option.");

okay.

>
>
> Bug.
> when pg_rest= ore --globals-only can be applied when we are restoring a
> single da= tabase (can be an output of pg_dump).

As of now, w= e are ignoring this option. We can add an error in the "else" par= t of the global.dat file.
Ex: option --globals-only is only suppo= rted with dump of pg_dumpall. Similarly --exclude-database also.
=
>
>
> There are some tests per https://commitfest.postgresql.org/5= 2/5495, I
> will check it later.
> The attached patch is th= e change for the above reviews.


--
Thanks and Regards
Mah= endra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
--000000000000af4da6062de1004b--