s/shm_mq_iovec/struct iovec/

Started by Thomas Munroover 1 year ago3 messages
#1Thomas Munro
thomas.munro@gmail.com
1 attachment(s)

Hi,

I was grepping for iovec users and noticed that the shm_mq stuff
defines its own iovec struct. Is there any reason not to use the
standard one, now that we can? Will add to next commitfest.

Attachments:

0001-Use-standard-iovec-in-shm_mq.h-interface.patchtext/x-patch; charset=US-ASCII; name=0001-Use-standard-iovec-in-shm_mq.h-interface.patchDownload
From 20b44cab0bb9f6218270aa0ae150ac0e560b49fe Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Mon, 15 Apr 2024 10:11:10 +1200
Subject: [PATCH] Use standard iovec in shm_mq.h interface.

Commit 2bd9e412 (2014) introduced "shm_mq_iovec", perhaps because
POSIX struct iovec was missing on Windows.  Commit 13a021f3 (2021)
introduced a standard-conforming replacement for Windows, for use in
various other parts of the tree.  Now that we can, we might as well use
the standard struct in the shared memory queue API too.
---
 src/backend/libpq/pqmq.c         | 10 +++++-----
 src/backend/storage/ipc/shm_mq.c | 29 ++++++++++++++++-------------
 src/include/storage/shm_mq.h     | 10 ++--------
 src/tools/pgindent/typedefs.list |  1 -
 4 files changed, 23 insertions(+), 27 deletions(-)

diff --git a/src/backend/libpq/pqmq.c b/src/backend/libpq/pqmq.c
index 00a44ca803f..d737663e230 100644
--- a/src/backend/libpq/pqmq.c
+++ b/src/backend/libpq/pqmq.c
@@ -117,7 +117,7 @@ mq_is_send_pending(void)
 static int
 mq_putmessage(char msgtype, const char *s, size_t len)
 {
-	shm_mq_iovec iov[2];
+	struct iovec iov[2];
 	shm_mq_result result;
 
 	/*
@@ -147,10 +147,10 @@ mq_putmessage(char msgtype, const char *s, size_t len)
 
 	pq_mq_busy = true;
 
-	iov[0].data = &msgtype;
-	iov[0].len = 1;
-	iov[1].data = s;
-	iov[1].len = len;
+	iov[0].iov_base = &msgtype;
+	iov[0].iov_len = 1;
+	iov[1].iov_base = unconstify(char *, s);
+	iov[1].iov_len = len;
 
 	Assert(pq_mq_handle != NULL);
 
diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index 9235fcd08ec..0d73bbf1879 100644
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -21,6 +21,7 @@
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "port/pg_bitutils.h"
+#include "port/pg_iovec.h"
 #include "postmaster/bgworker.h"
 #include "storage/shm_mq.h"
 #include "storage/spin.h"
@@ -329,10 +330,10 @@ shm_mq_result
 shm_mq_send(shm_mq_handle *mqh, Size nbytes, const void *data, bool nowait,
 			bool force_flush)
 {
-	shm_mq_iovec iov;
+	struct iovec iov;
 
-	iov.data = data;
-	iov.len = nbytes;
+	iov.iov_base = unconstify(void *, data);
+	iov.iov_len = nbytes;
 
 	return shm_mq_sendv(mqh, &iov, 1, nowait, force_flush);
 }
@@ -358,7 +359,7 @@ shm_mq_send(shm_mq_handle *mqh, Size nbytes, const void *data, bool nowait,
  * ring size.
  */
 shm_mq_result
-shm_mq_sendv(shm_mq_handle *mqh, shm_mq_iovec *iov, int iovcnt, bool nowait,
+shm_mq_sendv(shm_mq_handle *mqh, struct iovec *iov, int iovcnt, bool nowait,
 			 bool force_flush)
 {
 	shm_mq_result res;
@@ -374,7 +375,7 @@ shm_mq_sendv(shm_mq_handle *mqh, shm_mq_iovec *iov, int iovcnt, bool nowait,
 
 	/* Compute total size of write. */
 	for (i = 0; i < iovcnt; ++i)
-		nbytes += iov[i].len;
+		nbytes += iov[i].iov_len;
 
 	/* Prevent writing messages overwhelming the receiver. */
 	if (nbytes > MaxAllocSize)
@@ -423,9 +424,9 @@ shm_mq_sendv(shm_mq_handle *mqh, shm_mq_iovec *iov, int iovcnt, bool nowait,
 		Size		chunksize;
 
 		/* Figure out which bytes need to be sent next. */
-		if (offset >= iov[which_iov].len)
+		if (offset >= iov[which_iov].iov_len)
 		{
-			offset -= iov[which_iov].len;
+			offset -= iov[which_iov].iov_len;
 			++which_iov;
 			if (which_iov >= iovcnt)
 				break;
@@ -442,16 +443,17 @@ shm_mq_sendv(shm_mq_handle *mqh, shm_mq_iovec *iov, int iovcnt, bool nowait,
 		 * MAXALIGN'd.
 		 */
 		if (which_iov + 1 < iovcnt &&
-			offset + MAXIMUM_ALIGNOF > iov[which_iov].len)
+			offset + MAXIMUM_ALIGNOF > iov[which_iov].iov_len)
 		{
 			char		tmpbuf[MAXIMUM_ALIGNOF];
 			int			j = 0;
 
 			for (;;)
 			{
-				if (offset < iov[which_iov].len)
+				if (offset < iov[which_iov].iov_len)
 				{
-					tmpbuf[j] = iov[which_iov].data[offset];
+					char *data = iov[which_iov].iov_base;
+					tmpbuf[j] = data[offset];
 					j++;
 					offset++;
 					if (j == MAXIMUM_ALIGNOF)
@@ -459,7 +461,7 @@ shm_mq_sendv(shm_mq_handle *mqh, shm_mq_iovec *iov, int iovcnt, bool nowait,
 				}
 				else
 				{
-					offset -= iov[which_iov].len;
+					offset -= iov[which_iov].iov_len;
 					which_iov++;
 					if (which_iov >= iovcnt)
 						break;
@@ -487,10 +489,11 @@ shm_mq_sendv(shm_mq_handle *mqh, shm_mq_iovec *iov, int iovcnt, bool nowait,
 		 * isn't a multiple of MAXIMUM_ALIGNOF.  Otherwise, we need to
 		 * MAXALIGN_DOWN the write size.
 		 */
-		chunksize = iov[which_iov].len - offset;
+		chunksize = iov[which_iov].iov_len - offset;
 		if (which_iov + 1 < iovcnt)
 			chunksize = MAXALIGN_DOWN(chunksize);
-		res = shm_mq_send_bytes(mqh, chunksize, &iov[which_iov].data[offset],
+		res = shm_mq_send_bytes(mqh, chunksize,
+								(char *) iov[which_iov].iov_base + offset,
 								nowait, &bytes_written);
 
 		if (res == SHM_MQ_DETACHED)
diff --git a/src/include/storage/shm_mq.h b/src/include/storage/shm_mq.h
index 80f63f4fba5..de018598aa3 100644
--- a/src/include/storage/shm_mq.h
+++ b/src/include/storage/shm_mq.h
@@ -13,6 +13,7 @@
 #ifndef SHM_MQ_H
 #define SHM_MQ_H
 
+#include "port/pg_iovec.h"
 #include "postmaster/bgworker.h"
 #include "storage/dsm.h"
 #include "storage/proc.h"
@@ -25,13 +26,6 @@ typedef struct shm_mq shm_mq;
 struct shm_mq_handle;
 typedef struct shm_mq_handle shm_mq_handle;
 
-/* Descriptors for a single write spanning multiple locations. */
-typedef struct
-{
-	const char *data;
-	Size		len;
-} shm_mq_iovec;
-
 /* Possible results of a send or receive operation. */
 typedef enum
 {
@@ -72,7 +66,7 @@ extern shm_mq *shm_mq_get_queue(shm_mq_handle *mqh);
 extern shm_mq_result shm_mq_send(shm_mq_handle *mqh,
 								 Size nbytes, const void *data, bool nowait,
 								 bool force_flush);
-extern shm_mq_result shm_mq_sendv(shm_mq_handle *mqh, shm_mq_iovec *iov,
+extern shm_mq_result shm_mq_sendv(shm_mq_handle *mqh, struct iovec *iov,
 								  int iovcnt, bool nowait, bool force_flush);
 extern shm_mq_result shm_mq_receive(shm_mq_handle *mqh,
 									Size *nbytesp, void **datap, bool nowait);
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index cf05701c032..9d8f5d6d731 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3811,7 +3811,6 @@ set_join_pathlist_hook_type
 set_rel_pathlist_hook_type
 shm_mq
 shm_mq_handle
-shm_mq_iovec
 shm_mq_result
 shm_toc
 shm_toc_entry
-- 
2.44.0

#2Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Thomas Munro (#1)
Re: s/shm_mq_iovec/struct iovec/

On 15/04/2024 04:20, Thomas Munro wrote:

Hi,

I was grepping for iovec users and noticed that the shm_mq stuff
defines its own iovec struct. Is there any reason not to use the
standard one, now that we can? Will add to next commitfest.

I think it's better to keep them separate. They serve a similar purpose,
but they belong to completely separate APIs; I think "incidental
deduplication" is the right term for that. shm_mq_iovec is only used by
our shm queue implementation, while struct iovec is part of the POSIX
API. We wouldn't want to leak IOV_MAX into how shm_mq_iovec is used, for
example. Or as a thought experiment, if our shm_mq implementation needed
an extra flag in the struct or something, we would be free to just add
it. But if it we reused struct iovec, then we couldn't, or we'd need to
create a new struct again.

--
Heikki Linnakangas
Neon (https://neon.tech)

#3Thomas Munro
thomas.munro@gmail.com
In reply to: Heikki Linnakangas (#2)
Re: s/shm_mq_iovec/struct iovec/

On Thu, Jul 4, 2024 at 9:26 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 15/04/2024 04:20, Thomas Munro wrote:

I was grepping for iovec users and noticed that the shm_mq stuff
defines its own iovec struct. Is there any reason not to use the
standard one, now that we can? Will add to next commitfest.

I think it's better to keep them separate. They serve a similar purpose,
but they belong to completely separate APIs; I think "incidental
deduplication" is the right term for that. shm_mq_iovec is only used by
our shm queue implementation, while struct iovec is part of the POSIX
API. We wouldn't want to leak IOV_MAX into how shm_mq_iovec is used, for
example. Or as a thought experiment, if our shm_mq implementation needed
an extra flag in the struct or something, we would be free to just add
it. But if it we reused struct iovec, then we couldn't, or we'd need to
create a new struct again.

Thanks for looking. I marked this "returned with feedback".

(For future parallel query work, I have a scheme where queues can
spill to disk instead of blocking, which unblocks a bunch of parallel
execution strategies that are currently impossible due to flow control
deadlock risk. Then the two APIs meet and it annoys me that they are
not the same, so maybe I'll talk about this again some day :-))