Additional minor pg_dump cleanups

Started by Daniel Gustafssonover 1 year ago4 messages

daniel@yesql.se

over 1 year ago

2 attachment(s)

Re-reading Nathans recent 8213df9effaf I noticed a few more small things which
can be cleaned up. In two of the get<Object> functions we lack a fast-path for
when no tuples are found which leads to pg_malloc(0) calls. Another thing is
that we in one place reset the PQExpBuffer immediately after creating it which
isn't required.

--
Daniel Gustafsson

Attachments:

0001-Remove-superfluous-PQExpBuffer-resetting.patchapplication/octet-stream; name=0001-Remove-superfluous-PQExpBuffer-resetting.patch; x-unix-mode=0644Download

From f1a0952d441dfcfe45e77d939598b427c1e40bb9 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Wed, 3 Jul 2024 09:11:05 +0200
Subject: [PATCH 1/2] Remove superfluous PQExpBuffer resetting

Since the buffer was just created, there is no reason to immediately
reset it.
---
 src/bin/pg_dump/pg_dump.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7aec016a9f..7ccf943522 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4198,8 +4198,6 @@ getPublications(Archive *fout)
 
 	query = createPQExpBuffer();
 
-	resetPQExpBuffer(query);
-
 	/* Get the publications. */
 	if (fout->remoteVersion >= 130000)
 		appendPQExpBufferStr(query,
-- 
2.39.3 (Apple Git-146)

0002-Add-fastpaths-for-when-no-objects-are-found.patchapplication/octet-stream; name=0002-Add-fastpaths-for-when-no-objects-are-found.patch; x-unix-mode=0644Download

From 4777fc98ee642b99c2bdab250e2a6311ee3022e6 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Wed, 3 Jul 2024 09:12:12 +0200
Subject: [PATCH 2/2] Add fastpaths for when no objects are found

If there are no objects found, there is no reason to inspect the
result columns and mallocing a zero-sized (which will be 1 byte
in reality) heap buffer for it. Add fast-paths like how other
object inspection functions are already doing it.
---
 src/bin/pg_dump/pg_dump.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7ccf943522..6e0ddb637e 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4222,6 +4222,9 @@ getPublications(Archive *fout)
 
 	ntups = PQntuples(res);
 
+	if (ntups == 0)
+		goto cleanup;
+
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
 	i_pubname = PQfnumber(res, "pubname");
@@ -4260,6 +4263,8 @@ getPublications(Archive *fout)
 		/* Decide whether we want to dump it */
 		selectDumpableObject(&(pubinfo[i].dobj), fout);
 	}
+
+cleanup:
 	PQclear(res);
 
 	destroyPQExpBuffer(query);
@@ -5701,6 +5706,8 @@ getExtensions(Archive *fout, int *numExtensions)
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
 
 	ntups = PQntuples(res);
+	if (ntups == 0)
+		goto cleanup;
 
 	extinfo = (ExtensionInfo *) pg_malloc(ntups * sizeof(ExtensionInfo));
 
@@ -5730,6 +5737,7 @@ getExtensions(Archive *fout, int *numExtensions)
 		selectDumpableExtension(&(extinfo[i]), dopt);
 	}
 
+cleanup:
 	PQclear(res);
 	destroyPQExpBuffer(query);
 
-- 
2.39.3 (Apple Git-146)

Ranier Vilela

ranier.vf@gmail.com

over 1 year ago

In reply to: Daniel Gustafsson (#1)

Re: Additional minor pg_dump cleanups

Em qua., 3 de jul. de 2024 às 04:37, Daniel Gustafsson <daniel@yesql.se>
escreveu:

Re-reading Nathans recent 8213df9effaf I noticed a few more small things
which
can be cleaned up. In two of the get<Object> functions we lack a
fast-path for
when no tuples are found which leads to pg_malloc(0) calls. Another thing
is
that we in one place reset the PQExpBuffer immediately after creating it
which
isn't required.

0001 Looks good to me.

0002:
With the function *getPublications* I think it would be good to free up the
allocated memory?

     }
+     pg_free(pubinfo);
+cleanup:
      PQclear(res);

With the function *getExtensions* I think it would be good to return NULL
in case ntups = 0?
Otherwise we may end up with an uninitialized variable.

- ExtensionInfo *extinfo;
+ ExtensionInfo *extinfo = NULL;

Funny, the function *getExtensionMembership* does not use the parameter
ExtensionInfo extinfo.
getExtensions does not have another caller, Is it really necessary?

best regards,
Ranier Vilela

Daniel Gustafsson

daniel@yesql.se

over 1 year ago

In reply to: Ranier Vilela (#2)

2 attachment(s)

Re: Additional minor pg_dump cleanups

On 3 Jul 2024, at 13:29, Ranier Vilela <ranier.vf@gmail.com> wrote:

With the function *getPublications* I think it would be good to free up the allocated memory?
}
+     pg_free(pubinfo);
+cleanup:
PQclear(res);

Since the pubinfo is recorded in the DumpableObject and is responsible for
keeping track of which publications to dump, it would be quite incorrect to
free it here.

With the function *getExtensions* I think it would be good to return NULL in case ntups = 0?
Otherwise we may end up with an uninitialized variable.
- ExtensionInfo *extinfo;
+ ExtensionInfo *extinfo = NULL;

I guess that won't hurt, though any code inspecting extinfo when numExtensions
is returned as zero is flat-out wrong. It may however silence a static
analyzer so there is that.

Funny, the function *getExtensionMembership* does not use the parameter ExtensionInfo extinfo.
getExtensions does not have another caller, Is it really necessary?

Yes, see processExtensionTables().

--
Daniel Gustafsson

Attachments:

v2-0002-Add-fastpaths-for-when-no-objects-are-found.patchapplication/octet-stream; name=v2-0002-Add-fastpaths-for-when-no-objects-are-found.patch; x-unix-mode=0644Download

From be809f16ecdeadd64a56dfad0a853be6a6af1658 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Wed, 3 Jul 2024 09:12:12 +0200
Subject: [PATCH v2 2/2] Add fastpaths for when no objects are found

If there are no objects found, there is no reason to inspect the
result columns and mallocing a zero-sized (which will be 1 byte
in reality) heap buffer for it. Add fast-paths like how other
object inspection functions are already doing it.
---
 src/bin/pg_dump/pg_dump.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 48efc61406..dc99964be6 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4243,6 +4243,9 @@ getPublications(Archive *fout)
 
 	ntups = PQntuples(res);
 
+	if (ntups == 0)
+		goto cleanup;
+
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
 	i_pubname = PQfnumber(res, "pubname");
@@ -4281,6 +4284,8 @@ getPublications(Archive *fout)
 		/* Decide whether we want to dump it */
 		selectDumpableObject(&(pubinfo[i].dobj), fout);
 	}
+
+cleanup:
 	PQclear(res);
 
 	destroyPQExpBuffer(query);
@@ -5729,7 +5734,7 @@ getExtensions(Archive *fout, int *numExtensions)
 	int			ntups;
 	int			i;
 	PQExpBuffer query;
-	ExtensionInfo *extinfo;
+	ExtensionInfo *extinfo = NULL;
 	int			i_tableoid;
 	int			i_oid;
 	int			i_extname;
@@ -5749,6 +5754,8 @@ getExtensions(Archive *fout, int *numExtensions)
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
 
 	ntups = PQntuples(res);
+	if (ntups == 0)
+		goto cleanup;
 
 	extinfo = (ExtensionInfo *) pg_malloc(ntups * sizeof(ExtensionInfo));
 
@@ -5778,6 +5785,7 @@ getExtensions(Archive *fout, int *numExtensions)
 		selectDumpableExtension(&(extinfo[i]), dopt);
 	}
 
+cleanup:
 	PQclear(res);
 	destroyPQExpBuffer(query);
 
-- 
2.39.3 (Apple Git-146)

v2-0001-Remove-superfluous-PQExpBuffer-resetting.patchapplication/octet-stream; name=v2-0001-Remove-superfluous-PQExpBuffer-resetting.patch; x-unix-mode=0644Download

From 98897b554cc6dbe0d2162b813b85c7a79f980268 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Wed, 3 Jul 2024 09:11:05 +0200
Subject: [PATCH v2 1/2] Remove superfluous PQExpBuffer resetting

Since the buffer was just created, there is no reason to immediately
reset it.
---
 src/bin/pg_dump/pg_dump.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 5426f1177c..48efc61406 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4219,8 +4219,6 @@ getPublications(Archive *fout)
 
 	query = createPQExpBuffer();
 
-	resetPQExpBuffer(query);
-
 	/* Get the publications. */
 	if (fout->remoteVersion >= 130000)
 		appendPQExpBufferStr(query,
-- 
2.39.3 (Apple Git-146)

Ranier Vilela

ranier.vf@gmail.com

over 1 year ago

In reply to: Daniel Gustafsson (#3)

Re: Additional minor pg_dump cleanups

Em qui., 4 de jul. de 2024 às 05:18, Daniel Gustafsson <daniel@yesql.se>
escreveu:

On 3 Jul 2024, at 13:29, Ranier Vilela <ranier.vf@gmail.com> wrote:

With the function *getPublications* I think it would be good to free up

the allocated memory?
}
+     pg_free(pubinfo);
+cleanup:
PQclear(res);
Since the pubinfo is recorded in the DumpableObject and is responsible for
keeping track of which publications to dump, it would be quite incorrect to
free it here.

With the function *getExtensions* I think it would be good to return

NULL in case ntups = 0?
Otherwise we may end up with an uninitialized variable.
- ExtensionInfo *extinfo;
+ ExtensionInfo *extinfo = NULL;
I guess that won't hurt, though any code inspecting extinfo when
numExtensions
is returned as zero is flat-out wrong. It may however silence a static
analyzer so there is that.

Funny, the function *getExtensionMembership* does not use the parameter

ExtensionInfo extinfo.

getExtensions does not have another caller, Is it really necessary?

Yes, see processExtensionTables().

I saw, thanks.

LGTM.

best regards,
Ranier Vilela