From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
cc: Robert Haas <robertmhaas@gmail.com>,
        Alvaro Herrera <alvherre@alvh.no-ip.org>,
        Andrew Dunstan <andrew@dunslane.net>,
        pgsql-hackers@lists.postgresql.org
Subject: Re: Inefficiency in parallel pg_restore with many tables
In-reply-to: <20230904230829.GA3826808@nathanxps13>
References: <20230718160713.GA1139177@nathanxps13> <20230720190644.GA1724613@nathanxps13> <20230722231941.GA2020225@nathanxps13> <1225141.1690069670@sss.pgh.pa.us> <20230723055703.GA2421212@nathanxps13> <20230724190015.GA2528613@nathanxps13> <20230725185336.GA2911441@nathanxps13> <CA+TgmoYtJ+ad+ej=BCPWRNPyNoLy2R2O5wBSZUeyaQ54jrPk4A@mail.gmail.com> <20230901205248.GA3184752@nathanxps13> <20230902185521.GA3414119@nathanxps13> <20230904230829.GA3826808@nathanxps13>
Comments: In-reply-to Nathan Bossart <nathandbossart@gmail.com>
	message dated "Mon, 04 Sep 2023 16:08:29 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2393312.1694363710.1@sss.pgh.pa.us>
Date: Sun, 10 Sep 2023 12:35:10 -0400
Message-ID: <2393313.1694363710@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/2393313.1694363710%40sss.pgh.pa.us>
Precedence: bulk

Nathan Bossart <nathandbossart@gmail.com> writes:
> I spent some more time on this patch and made the relevant adjustments to
> the rest of the set.

Hmm ... I do not like v7 very much at all.  It requires rather ugly
changes to all of the existing callers, and what are we actually
buying?  If anything, it makes things slower for pass-by-value items
like integers.  I'd stick with the Datum convention in the backend.

Instead, I took a closer look through the v6 patch set.
I think that's in pretty good shape and nearly committable,
but I have a few thoughts:

* I'm not sure about defining bh_node_type as a macro:

+#ifdef FRONTEND
+#define bh_node_type void *
+#else
+#define bh_node_type Datum
+#endif

rather than an actual typedef:

+#ifdef FRONTEND
+typedef void *bh_node_type;
+#else
+typedef Datum bh_node_type;
+#endif

My concern here is that bh_node_type is effectively acting as a
typedef, so that pgindent might misbehave if it's not declared as a
typedef.  On the other hand, there doesn't seem to be any indentation
problem in the patchset as it stands, and we don't expect any code
outside binaryheap.h/.c to refer to bh_node_type, so maybe it's fine.
(If you do choose to make it a typedef, remember to add it to
typedefs.list.)

* As a matter of style, I'd recommend adding braces in places
like this:

 	if (heap->bh_size >= heap->bh_space)
+	{
+#ifdef FRONTEND
+		pg_fatal("out of binary heap slots");
+#else
 		elog(ERROR, "out of binary heap slots");
+#endif
+	}
 	heap->bh_nodes[heap->bh_size] = d;

It's not wrong as you have it, but I think it's more readable
and less easy to accidentally break with the extra braces.

* In 0002, isn't the comment for binaryheap_remove_node wrong?

+ * Removes the nth node from the heap.  The caller must ensure that there are
+ * at least (n - 1) nodes in the heap.  O(log n) worst case.

Shouldn't that be "(n + 1)"?  Also, I'd specify "n'th (zero based) node"
for clarity.

* I would say that this bit in 0004:

-		j = removeHeapElement(pendingHeap, heapLength--);
+		j = (intptr_t) binaryheap_remove_first(pendingHeap);

needs an explicit cast to int:

+		j = (int) (intptr_t) binaryheap_remove_first(pendingHeap);

otherwise some compilers might complain about the result possibly
not fitting in "j".

Other than those nitpicks, I like v6.  I'll mark this RfC.

			regards, tom lane