remove pg_restrict workaround

Started by Peter Eisentraut3 months ago7 messages
#1Peter Eisentraut
peter@eisentraut.org
2 attachment(s)

When in C11 mode, MSVC supports the standard "restrict" keyword, so we
don't need the workaround with using "pg_restrict" instead anymore.
(Just for clarification, restrict is a C99 feature, but MSVC only
accepts it properly in C11 mode.) So I'm proposing to remove that
workaround here, so that code can use the standard restrict keyword
without having to worry about the alternative spelling.

However, "restrict" does not exist in C++, but all supported compilers
support the spelling "__restrict" in C++. Therefore, I'm leaving in
place the Autoconf test and the equivalent Meson business, to maintain
the status quo with respect to C++. I'm updating the comments about
that a bit. (In a C++-free world, we could have plausibly removed the
Autoconf test altogether.)

While making the required adjustments, I found a few pieces of code that
use restrict (previously pg_restrict) in what appears to me to be
nonsensical ways, like

memcpy((char *pg_restrict) (buf->data + buf->len), &ni, sizeof(uint16));

The restrict qualifier is not meaningful in casts, so I'm confused about
why it is used here. I did not find any indications either in the old
discussions leading up to this change or on the wider web that there are
any other interpretations or compiler extensions or perhaps a C++ angle
that would make this meaningful. Also, the generated code appears to be
the same if I remove this. So I'm proposing to remove redundant casts
like this.

Attachments:

0001-Remove-meaninglist-restrict-qualifiers.patchtext/plain; charset=UTF-8; name=0001-Remove-meaninglist-restrict-qualifiers.patchDownload
From 708a9e51e7a73df72c1632ab2dab0c450d925954 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 15 Oct 2025 10:14:06 +0200
Subject: [PATCH 1/2] Remove meaninglist restrict qualifiers

The use of the restrict qualifier in casts is meaningless, so remove
it.
---
 src/include/libpq/pqformat.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index 9a1534be521..55442caf0e4 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -48,7 +48,7 @@ pq_writeint8(StringInfoData *pg_restrict buf, uint8 i)
 	uint8		ni = i;
 
 	Assert(buf->len + (int) sizeof(uint8) <= buf->maxlen);
-	memcpy((char *pg_restrict) (buf->data + buf->len), &ni, sizeof(uint8));
+	memcpy(buf->data + buf->len, &ni, sizeof(uint8));
 	buf->len += sizeof(uint8);
 }
 
@@ -62,7 +62,7 @@ pq_writeint16(StringInfoData *pg_restrict buf, uint16 i)
 	uint16		ni = pg_hton16(i);
 
 	Assert(buf->len + (int) sizeof(uint16) <= buf->maxlen);
-	memcpy((char *pg_restrict) (buf->data + buf->len), &ni, sizeof(uint16));
+	memcpy(buf->data + buf->len, &ni, sizeof(uint16));
 	buf->len += sizeof(uint16);
 }
 
@@ -76,7 +76,7 @@ pq_writeint32(StringInfoData *pg_restrict buf, uint32 i)
 	uint32		ni = pg_hton32(i);
 
 	Assert(buf->len + (int) sizeof(uint32) <= buf->maxlen);
-	memcpy((char *pg_restrict) (buf->data + buf->len), &ni, sizeof(uint32));
+	memcpy(buf->data + buf->len, &ni, sizeof(uint32));
 	buf->len += sizeof(uint32);
 }
 
@@ -90,7 +90,7 @@ pq_writeint64(StringInfoData *pg_restrict buf, uint64 i)
 	uint64		ni = pg_hton64(i);
 
 	Assert(buf->len + (int) sizeof(uint64) <= buf->maxlen);
-	memcpy((char *pg_restrict) (buf->data + buf->len), &ni, sizeof(uint64));
+	memcpy(buf->data + buf->len, &ni, sizeof(uint64));
 	buf->len += sizeof(uint64);
 }
 
@@ -116,7 +116,7 @@ pq_writestring(StringInfoData *pg_restrict buf, const char *pg_restrict str)
 
 	Assert(buf->len + slen + 1 <= buf->maxlen);
 
-	memcpy(((char *pg_restrict) buf->data + buf->len), p, slen + 1);
+	memcpy(buf->data + buf->len, p, slen + 1);
 	buf->len += slen + 1;
 
 	if (p != str)
-- 
2.51.0

0002-Replace-pg_restrict-by-standard-restrict.patchtext/plain; charset=UTF-8; name=0002-Replace-pg_restrict-by-standard-restrict.patchDownload
From 0cc4796822a97d265927fd260ace56c24ca95ac9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 15 Oct 2025 13:35:55 +0200
Subject: [PATCH 2/2] Replace pg_restrict by standard restrict

MSVC in C11 mode supports the standard restrict qualifier, so we don't
need the workaround naming pg_restrict anymore.

Even though restrict is in C99 and should be supported by all
supported compilers, we keep the configure test and the hardcoded
redirection to __restrict, because that will also work in C++ in all
supported compilers.  (restrict is not part of the C++ standard.)
---
 configure                                 | 18 ++++--------------
 configure.ac                              | 16 ++++------------
 meson.build                               | 13 ++++++-------
 src/bin/pg_verifybackup/pg_verifybackup.c |  4 ++--
 src/bin/pg_verifybackup/pg_verifybackup.h |  4 ++--
 src/common/logging.c                      |  4 ++--
 src/common/string.c                       |  2 +-
 src/include/common/logging.h              |  4 ++--
 src/include/common/string.h               |  2 +-
 src/include/libpq/pqformat.h              | 12 ++++++------
 src/include/pg_config.h.in                |  4 ----
 11 files changed, 30 insertions(+), 53 deletions(-)

diff --git a/configure b/configure
index 22cd866147b..7ce52173dd8 100755
--- a/configure
+++ b/configure
@@ -14999,10 +14999,10 @@ _ACEOF
 fi
 
 
-# MSVC doesn't cope well with defining restrict to __restrict, the
-# spelling it understands, because it conflicts with
-# __declspec(restrict). Therefore we define pg_restrict to the
-# appropriate definition, which presumably won't conflict.
+# Even though restrict is in C99 and should be supported by all
+# supported compilers, but this macro is useful because it will prefer
+# a spelling that also works in C++ (often __restrict).  (restrict is
+# not part of the C++ standard.)
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking for C/C++ restrict keyword" >&5
 $as_echo_n "checking for C/C++ restrict keyword... " >&6; }
 if ${ac_cv_c_restrict+:} false; then :
@@ -15049,16 +15049,6 @@ _ACEOF
  ;;
  esac
 
-if test "$ac_cv_c_restrict" = "no"; then
-  pg_restrict=""
-else
-  pg_restrict="$ac_cv_c_restrict"
-fi
-
-cat >>confdefs.h <<_ACEOF
-#define pg_restrict $pg_restrict
-_ACEOF
-
 
 ac_fn_c_check_type "$LINENO" "struct option" "ac_cv_type_struct_option" "#ifdef HAVE_GETOPT_H
 #include <getopt.h>
diff --git a/configure.ac b/configure.ac
index e44943aa6fe..19c93d0b391 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1685,19 +1685,11 @@ PGAC_UNION_SEMUN
 AC_CHECK_TYPES(socklen_t, [], [], [#include <sys/socket.h>])
 PGAC_STRUCT_SOCKADDR_SA_LEN
 
-# MSVC doesn't cope well with defining restrict to __restrict, the
-# spelling it understands, because it conflicts with
-# __declspec(restrict). Therefore we define pg_restrict to the
-# appropriate definition, which presumably won't conflict.
+# Even though restrict is in C99 and should be supported by all
+# supported compilers, but this macro is useful because it will prefer
+# a spelling that also works in C++ (often __restrict).  (restrict is
+# not part of the C++ standard.)
 AC_C_RESTRICT
-if test "$ac_cv_c_restrict" = "no"; then
-  pg_restrict=""
-else
-  pg_restrict="$ac_cv_c_restrict"
-fi
-AC_DEFINE_UNQUOTED([pg_restrict], [$pg_restrict],
-[Define to keyword to use for C99 restrict support, or to nothing if not
-supported])
 
 AC_CHECK_TYPES([struct option], [], [],
 [#ifdef HAVE_GETOPT_H
diff --git a/meson.build b/meson.build
index 395416a6060..d2315701114 100644
--- a/meson.build
+++ b/meson.build
@@ -2803,13 +2803,12 @@ int main(void)
 endforeach
 
 
-# MSVC doesn't cope well with defining restrict to __restrict, the spelling it
-# understands, because it conflicts with __declspec(restrict). Therefore we
-# define pg_restrict to the appropriate definition, which presumably won't
-# conflict.
-#
-# We assume C99 support, so we don't need to make this conditional.
-cdata.set('pg_restrict', '__restrict')
+# Even though restrict is in C99 and should be supported by all
+# supported compilers, but this indirection is useful because
+# __restrict also works in C++ in all supported compilers.  (If not,
+# then we might have to write a real test.)  (restrict is not part of
+# the C++ standard.)
+cdata.set('restrict', '__restrict')
 
 
 # Most libraries are included only if they demonstrably provide a function we
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index 5e6c13bb921..8d5befa947f 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -1228,7 +1228,7 @@ parse_required_wal(verifier_context *context, char *pg_waldump_path,
  * context says we should.
  */
 void
-report_backup_error(verifier_context *context, const char *pg_restrict fmt,...)
+report_backup_error(verifier_context *context, const char *restrict fmt,...)
 {
 	va_list		ap;
 
@@ -1245,7 +1245,7 @@ report_backup_error(verifier_context *context, const char *pg_restrict fmt,...)
  * Report a fatal error and exit
  */
 void
-report_fatal_error(const char *pg_restrict fmt,...)
+report_fatal_error(const char *restrict fmt,...)
 {
 	va_list		ap;
 
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.h b/src/bin/pg_verifybackup/pg_verifybackup.h
index 8cb6f9c53ad..3bdae62b822 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.h
+++ b/src/bin/pg_verifybackup/pg_verifybackup.h
@@ -96,9 +96,9 @@ typedef struct verifier_context
 } verifier_context;
 
 extern void report_backup_error(verifier_context *context,
-								const char *pg_restrict fmt,...)
+								const char *restrict fmt,...)
 			pg_attribute_printf(2, 3);
-pg_noreturn extern void report_fatal_error(const char *pg_restrict fmt,...)
+pg_noreturn extern void report_fatal_error(const char *restrict fmt,...)
 			pg_attribute_printf(1, 2);
 extern bool should_ignore_relpath(verifier_context *context,
 								  const char *relpath);
diff --git a/src/common/logging.c b/src/common/logging.c
index 125a172af80..7319a5b4e20 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -206,7 +206,7 @@ pg_logging_set_locus_callback(void (*cb) (const char **filename, uint64 *lineno)
 
 void
 pg_log_generic(enum pg_log_level level, enum pg_log_part part,
-			   const char *pg_restrict fmt,...)
+			   const char *restrict fmt,...)
 {
 	va_list		ap;
 
@@ -217,7 +217,7 @@ pg_log_generic(enum pg_log_level level, enum pg_log_part part,
 
 void
 pg_log_generic_v(enum pg_log_level level, enum pg_log_part part,
-				 const char *pg_restrict fmt, va_list ap)
+				 const char *restrict fmt, va_list ap)
 {
 	int			save_errno = errno;
 	const char *filename = NULL;
diff --git a/src/common/string.c b/src/common/string.c
index d8a3129c3ba..95c7c07d502 100644
--- a/src/common/string.c
+++ b/src/common/string.c
@@ -47,7 +47,7 @@ pg_str_endswith(const char *str, const char *end)
  * strtoint --- just like strtol, but returns int not long
  */
 int
-strtoint(const char *pg_restrict str, char **pg_restrict endptr, int base)
+strtoint(const char *restrict str, char **restrict endptr, int base)
 {
 	long		val;
 
diff --git a/src/include/common/logging.h b/src/include/common/logging.h
index 81529ee8f29..2e0333dc009 100644
--- a/src/include/common/logging.h
+++ b/src/include/common/logging.h
@@ -93,10 +93,10 @@ void		pg_logging_set_pre_callback(void (*cb) (void));
 void		pg_logging_set_locus_callback(void (*cb) (const char **filename, uint64 *lineno));
 
 void		pg_log_generic(enum pg_log_level level, enum pg_log_part part,
-						   const char *pg_restrict fmt,...)
+						   const char *restrict fmt,...)
 			pg_attribute_printf(3, 4);
 void		pg_log_generic_v(enum pg_log_level level, enum pg_log_part part,
-							 const char *pg_restrict fmt, va_list ap)
+							 const char *restrict fmt, va_list ap)
 			pg_attribute_printf(3, 0);
 
 /*
diff --git a/src/include/common/string.h b/src/include/common/string.h
index 55ed8774364..32a97c24b22 100644
--- a/src/include/common/string.h
+++ b/src/include/common/string.h
@@ -25,7 +25,7 @@ typedef struct PromptInterruptContext
 
 /* functions in src/common/string.c */
 extern bool pg_str_endswith(const char *str, const char *end);
-extern int	strtoint(const char *pg_restrict str, char **pg_restrict endptr,
+extern int	strtoint(const char *restrict str, char **restrict endptr,
 					 int base);
 extern char *pg_clean_ascii(const char *str, int alloc_flags);
 extern int	pg_strip_crlf(char *str);
diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index 55442caf0e4..127a74e299a 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -34,7 +34,7 @@ extern void pq_sendfloat8(StringInfo buf, float8 f);
  * Append a [u]int8 to a StringInfo buffer, which already has enough space
  * preallocated.
  *
- * The use of pg_restrict allows the compiler to optimize the code based on
+ * The use of restrict allows the compiler to optimize the code based on
  * the assumption that buf, buf->len, buf->data and *buf->data don't
  * overlap. Without the annotation buf->len etc cannot be kept in a register
  * over subsequent pq_writeintN calls.
@@ -43,7 +43,7 @@ extern void pq_sendfloat8(StringInfo buf, float8 f);
  * overly picky and demanding a * before a restrict.
  */
 static inline void
-pq_writeint8(StringInfoData *pg_restrict buf, uint8 i)
+pq_writeint8(StringInfoData *restrict buf, uint8 i)
 {
 	uint8		ni = i;
 
@@ -57,7 +57,7 @@ pq_writeint8(StringInfoData *pg_restrict buf, uint8 i)
  * preallocated.
  */
 static inline void
-pq_writeint16(StringInfoData *pg_restrict buf, uint16 i)
+pq_writeint16(StringInfoData *restrict buf, uint16 i)
 {
 	uint16		ni = pg_hton16(i);
 
@@ -71,7 +71,7 @@ pq_writeint16(StringInfoData *pg_restrict buf, uint16 i)
  * preallocated.
  */
 static inline void
-pq_writeint32(StringInfoData *pg_restrict buf, uint32 i)
+pq_writeint32(StringInfoData *restrict buf, uint32 i)
 {
 	uint32		ni = pg_hton32(i);
 
@@ -85,7 +85,7 @@ pq_writeint32(StringInfoData *pg_restrict buf, uint32 i)
  * preallocated.
  */
 static inline void
-pq_writeint64(StringInfoData *pg_restrict buf, uint64 i)
+pq_writeint64(StringInfoData *restrict buf, uint64 i)
 {
 	uint64		ni = pg_hton64(i);
 
@@ -105,7 +105,7 @@ pq_writeint64(StringInfoData *pg_restrict buf, uint64 i)
  * sent to the frontend.
  */
 static inline void
-pq_writestring(StringInfoData *pg_restrict buf, const char *pg_restrict str)
+pq_writestring(StringInfoData *restrict buf, const char *restrict str)
 {
 	int			slen = strlen(str);
 	char	   *p;
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index c4dc5d72bdb..08d7bfbee10 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -796,10 +796,6 @@
 #undef inline
 #endif
 
-/* Define to keyword to use for C99 restrict support, or to nothing if not
-   supported */
-#undef pg_restrict
-
 /* Define to the equivalent of the C99 'restrict' keyword, or to
    nothing if this is not supported.  Do not define if restrict is
    supported directly.  */
-- 
2.51.0

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#1)
Re: remove pg_restrict workaround

Peter Eisentraut <peter@eisentraut.org> writes:

When in C11 mode, MSVC supports the standard "restrict" keyword, so we
don't need the workaround with using "pg_restrict" instead anymore.
(Just for clarification, restrict is a C99 feature, but MSVC only
accepts it properly in C11 mode.) So I'm proposing to remove that
workaround here, so that code can use the standard restrict keyword
without having to worry about the alternative spelling.

Won't this break extensions that are using pg_restrict? Sure, they
could update their code, but then maybe it wouldn't work anymore
against previous branches. Seems like it'd be better to leave
pg_restrict in place (for awhile anyway) but always #define it
as "restrict". I don't mind ceasing to use it within our own tree
though.

regards, tom lane

#3Peter Eisentraut
peter@eisentraut.org
In reply to: Tom Lane (#2)
Re: remove pg_restrict workaround

On 15.10.25 15:58, Tom Lane wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

When in C11 mode, MSVC supports the standard "restrict" keyword, so we
don't need the workaround with using "pg_restrict" instead anymore.
(Just for clarification, restrict is a C99 feature, but MSVC only
accepts it properly in C11 mode.) So I'm proposing to remove that
workaround here, so that code can use the standard restrict keyword
without having to worry about the alternative spelling.

Won't this break extensions that are using pg_restrict? Sure, they
could update their code, but then maybe it wouldn't work anymore
against previous branches. Seems like it'd be better to leave
pg_restrict in place (for awhile anyway) but always #define it
as "restrict". I don't mind ceasing to use it within our own tree
though.

Committed with a backward compatibility define.

#4Jelte Fennema-Nio
postgres@jeltef.nl
In reply to: Peter Eisentraut (#3)
Re: remove pg_restrict workaround

On Wed, 29 Oct 2025 at 08:04, Peter Eisentraut <peter@eisentraut.org> wrote:

Committed with a backward compatibility define.

I'm working on adding a C++ extension module to my copyObject
patchset[1]/messages/by-id/CAGECzQR21OnnKiZO_1rLWO0-16kg1JBxnVq-wymYW0-_1cUNtg@mail.gmail.com. But turns out that this change has completely broken
compiling C++ extensions on MSVC. This is happening due to
__declspec(restrict) being replaced by __declspec(__restrict), which
the original comments also mentioned as the reason for having our own
flavor. This replacement then makes the core of a corecrt_malloc.h
MSVC header invalid[2]https://cirrus-ci.com/task/5516067944005632?logs=build#L2031-L2044.

So I think we should revert this patch.

[1]: /messages/by-id/CAGECzQR21OnnKiZO_1rLWO0-16kg1JBxnVq-wymYW0-_1cUNtg@mail.gmail.com
[2]: https://cirrus-ci.com/task/5516067944005632?logs=build#L2031-L2044

#5Andres Freund
andres@anarazel.de
In reply to: Jelte Fennema-Nio (#4)
Re: remove pg_restrict workaround

Hi,

On 2026-01-03 00:23:37 +0100, Jelte Fennema-Nio wrote:

On Wed, 29 Oct 2025 at 08:04, Peter Eisentraut <peter@eisentraut.org> wrote:

Committed with a backward compatibility define.

I'm working on adding a C++ extension module to my copyObject
patchset[1]. But turns out that this change has completely broken
compiling C++ extensions on MSVC. This is happening due to
__declspec(restrict) being replaced by __declspec(__restrict), which
the original comments also mentioned as the reason for having our own
flavor. This replacement then makes the core of a corecrt_malloc.h
MSVC header invalid[2].

So I think we should revert this patch.

+1

Greetings,

Andres Freund

#6Peter Eisentraut
peter@eisentraut.org
In reply to: Jelte Fennema-Nio (#4)
Re: remove pg_restrict workaround

On 03.01.26 00:23, Jelte Fennema-Nio wrote:

On Wed, 29 Oct 2025 at 08:04, Peter Eisentraut <peter@eisentraut.org> wrote:

Committed with a backward compatibility define.

I'm working on adding a C++ extension module to my copyObject
patchset[1]. But turns out that this change has completely broken
compiling C++ extensions on MSVC. This is happening due to
__declspec(restrict) being replaced by __declspec(__restrict), which
the original comments also mentioned as the reason for having our own
flavor. This replacement then makes the core of a corecrt_malloc.h
MSVC header invalid[2].

meson.build already contains a hint about the solution:

# Even though restrict is in C99 and should be supported by all
# supported compilers, this indirection is useful because __restrict
# also works in C++ in all supported compilers. (If not, then we
# might have to write a real test.) (restrict is not part of the C++
# standard.)
cdata.set('restrict', '__restrict')

Or maybe instead of writing a test, we should add something like this to
c.h:

#ifdef __cplusplus
#ifdef __GNUC__
#define restrict __restrict
#else
#define restrict
#endif
#endif

#7Jelte Fennema-Nio
postgres@jeltef.nl
In reply to: Peter Eisentraut (#6)
Re: remove pg_restrict workaround

On Sat, 3 Jan 2026 at 18:14, Peter Eisentraut <peter@eisentraut.org> wrote:

meson.build already contains a hint about the solution:

The comment that was there before f0f2c0c1a was better imo, because it
explained the exact problem I ran into with the new definition:

# MSVC doesn't cope well with defining restrict to __restrict, the spelling it
# understands, because it conflicts with __declspec(restrict). Therefore we
# define pg_restrict to the appropriate definition, which presumably won't
# conflict.
#
# We assume C99 support, so we don't need to make this conditional.
cdata.set('pg_restrict', '__restrict')

Or maybe instead of writing a test, we should add something like this to
c.h:

#ifdef __cplusplus
#ifdef __GNUC__
#define restrict __restrict
#else
#define restrict
#endif
#endif

I don't think a test or such a define can solve this problem. The
problem is that restrict already has a meaning in MSVC C++. But it's a
different one from C restrict. It's one of the allowed arguments to
__declspec(<arg>). So if we define restrict to anything (whether
__restrict, <empty string>, or foobar) that will always cause issues
in MSVC C++, because then any usage of __declspec(restrict) in the
MSVC stdlib will be replaced with
__declspec(__restrict)/__declspec()/__declspec(foobar).

So for MSVC C++ we cannot do:
#define restrict <whatever>

Which then means that if we continue to declare functions with
restrict in for arguments in headers, then we cannot use any of those
headers in MSVC C++. Because restrict would not be valid in that
position.

I don't see any way out except a revert. To be clear, that would solve
it because we can totally do the following on MSVC C++:
#define pg_restrict __restrict