From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kirk Wolak <wolakk@gmail.com>
cc: "Imseih (AWS), Sami" <simseih@amazon.com>,
        Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [BUG] pg_dump does not properly deal with BEGIN ATOMIC function
In-reply-to: 
 <CACLU5mTMoj8yx-tUFk84S1tfY-byvNVAWvVJ0MqG8Bp5B-wm_A@mail.gmail.com>
References: <2C1933AB-C2F8-499B-9D18-4AC1882256A0@amazon.com>
 <49272.1685708176@sss.pgh.pa.us>
 <CACLU5mTMoj8yx-tUFk84S1tfY-byvNVAWvVJ0MqG8Bp5B-wm_A@mail.gmail.com>
Comments: In-reply-to Kirk Wolak <wolakk@gmail.com>
	message dated "Fri, 02 Jun 2023 11:08:08 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1854.1685816775.0@sss.pgh.pa.us>
Date: Sat, 03 Jun 2023 14:28:11 -0400
Message-ID: <1868.1685816891@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1868.1685816891%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1854.1685816775.1@sss.pgh.pa.us>

Kirk Wolak <wolakk@gmail.com> writes:
> On Fri, Jun 2, 2023 at 8:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, now that I see a case the default printout here seems
>> completely ridiculous.  I think we need to do
>> -       pg_log_info("  %s", buf);
>> +       pg_log_warning("  %s", buf);

>   If I comprehend the suggestion, it will label each line with a warning.
> Which implies I have 6 Warnings.

Right, I'd forgotten that pg_log_warning() will interpose "warning:".
Attached are two more-carefully-thought-out suggestions.  The easy
way is to use pg_log_warning_detail(), which produces output like

pg_dump: warning: could not resolve dependency loop among these items:
pg_dump: detail:   FUNCTION a_f  (ID 216 OID 40532)
pg_dump: detail:   CONSTRAINT a_pkey  (ID 3466 OID 40531)
pg_dump: detail:   POST-DATA BOUNDARY  (ID 3612)
pg_dump: detail:   TABLE DATA a  (ID 3610 OID 40525)
pg_dump: detail:   PRE-DATA BOUNDARY  (ID 3611)

Alternatively, we could assemble the details by hand, as in the
second patch, producing

pg_dump: warning: could not resolve dependency loop among these items:
  FUNCTION a_f  (ID 216 OID 40532)
  CONSTRAINT a_pkey  (ID 3466 OID 40531)
  POST-DATA BOUNDARY  (ID 3612)
  TABLE DATA a  (ID 3610 OID 40525)
  PRE-DATA BOUNDARY  (ID 3611)

I'm not really sure which of these I like better.  The first one
is a much simpler code change, and there is some value in labeling
the output like that.  The second patch's output seems less cluttered,
but it's committing a modularity sin by embedding formatting knowledge
at the caller level.  Thoughts?

BTW, there is a similar abuse of pg_log_info just a few lines
above this, and probably others elsewhere.  I won't bother
writing patches for other places till we have agreement on what
the output ought to look like.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-patch; name="easy-warning-fix.patch";
	charset="us-ascii"
Content-ID: <1854.1685816775.2@sss.pgh.pa.us>
Content-Description: easy-warning-fix.patch

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 745578d855..3c66b8bf1a 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -1253,7 +1253,7 @@ repairDependencyLoop(DumpableObject **loop,
 		char		buf[1024];
 
 		describeDumpableObject(loop[i], buf, sizeof(buf));
-		pg_log_info("  %s", buf);
+		pg_log_warning_detail("  %s", buf);
 	}
 
 	if (nLoop > 1)

------- =_aaaaaaaaaa0
Content-Type: text/x-patch; name="harder-warning-fix.patch";
	charset="us-ascii"
Content-ID: <1854.1685816775.3@sss.pgh.pa.us>
Content-Description: harder-warning-fix.patch

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 745578d855..f0eef849d3 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -971,6 +971,7 @@ static void
 repairDependencyLoop(DumpableObject **loop,
 					 int nLoop)
 {
+	PQExpBuffer msgbuf;
 	int			i,
 				j;
 
@@ -1247,14 +1248,17 @@ repairDependencyLoop(DumpableObject **loop,
 	 * If we can't find a principled way to break the loop, complain and break
 	 * it in an arbitrary fashion.
 	 */
-	pg_log_warning("could not resolve dependency loop among these items:");
+	msgbuf = createPQExpBuffer();
 	for (i = 0; i < nLoop; i++)
 	{
 		char		buf[1024];
 
 		describeDumpableObject(loop[i], buf, sizeof(buf));
-		pg_log_info("  %s", buf);
+		appendPQExpBuffer(msgbuf, "\n  %s", buf);
 	}
+	pg_log_warning("could not resolve dependency loop among these items:%s",
+				   msgbuf->data);
+	destroyPQExpBuffer(msgbuf);
 
 	if (nLoop > 1)
 		removeObjectDependency(loop[0], loop[1]->dumpId);

------- =_aaaaaaaaaa0--