[PATCH] Extend the length of BackgroundWorker.bgw_library_name
Hi,
I want to suggest a patch against master (it may also be worth backporting
it) that makes it possible to use longer filenames (such as those with
absolute paths) in `BackgroundWorker.bgw_library_name`.
`BackgroundWorker.bgw_library_name` currently allows names up to
BGW_MAXLEN-1, which is generally sufficient if `$libdir` expansion is used.
However, there are use cases where [potentially] longer names are
expected/desired; for example, test benches (where library files may not
[or can not] be copied to Postgres installation) or alternative library
installation methods that do not put them into $libdir.
The patch is backwards-compatible and ensures that bgw_library_name stays
*at least* as long as BGW_MAXLEN. Existing external code that uses
BGW_MAXLEN is a length boundary (for example, in `strncpy`) will continue
to work as expected.
The trade-off of this patch is that the `BackgroundWorker` structure
becomes larger. From my perspective, this is a reasonable cost (less than a
kilobyte of extra space per worker).
The patch builds and `make check` succeeds.
Any feedback is welcome!
--
http://omnigres.org
Yurii
Attachments:
v1-0001-Extend-the-length-of-BackgroundWorker.bgw_library_name.patchapplication/octet-stream; name=v1-0001-Extend-the-length-of-BackgroundWorker.bgw_library_name.patchDownload
From 667c226488bc90fbd53c03c9391632547cc301e3 Mon Sep 17 00:00:00 2001
From: Yurii Rashkovskii <yrashk@gmail.com>
Date: Mon, 13 Mar 2023 07:39:41 -0700
Subject: [PATCH] Extend the length of BackgroundWorker.bgw_library_name.
It is currently set at BGW_MAXLEN, which is often insufficient for
absolute paths. While it may be acceptable when used in conjunction with
$libdir, this makes it tricky to use libraries that have longer paths to
them.
The use cases where this is necessary may include:
* Local test benches
* Extensions provisioned outside of standard packaging or build
procedures
---
doc/src/sgml/bgworker.sgml | 2 +-
src/backend/postmaster/bgworker.c | 2 +-
src/backend/replication/logical/launcher.c | 4 ++--
src/include/postmaster/bgworker.h | 7 ++++++-
4 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 7ba5da27e5..9ad1146ba0 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -59,7 +59,7 @@ typedef struct BackgroundWorker
int bgw_flags;
BgWorkerStartTime bgw_start_time;
int bgw_restart_time; /* in seconds, or BGW_NEVER_RESTART */
- char bgw_library_name[BGW_MAXLEN];
+ char bgw_library_name[MAXPGPATH];
char bgw_function_name[BGW_MAXLEN];
Datum bgw_main_arg;
char bgw_extra[BGW_EXTRALEN];
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 0dd22b2351..ade8b38984 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -362,7 +362,7 @@ BackgroundWorkerStateChange(bool allow_new_workers)
ascii_safe_strlcpy(rw->rw_worker.bgw_type,
slot->worker.bgw_type, BGW_MAXLEN);
ascii_safe_strlcpy(rw->rw_worker.bgw_library_name,
- slot->worker.bgw_library_name, BGW_MAXLEN);
+ slot->worker.bgw_library_name, sizeof(slot->worker.bgw_library_name));
ascii_safe_strlcpy(rw->rw_worker.bgw_function_name,
slot->worker.bgw_function_name, BGW_MAXLEN);
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 970d170e73..4f69b03cb3 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -456,7 +456,7 @@ retry:
bgw.bgw_flags = BGWORKER_SHMEM_ACCESS |
BGWORKER_BACKEND_DATABASE_CONNECTION;
bgw.bgw_start_time = BgWorkerStart_RecoveryFinished;
- snprintf(bgw.bgw_library_name, BGW_MAXLEN, "postgres");
+ snprintf(bgw.bgw_library_name, sizeof(bgw.bgw_library_name), "postgres");
if (is_parallel_apply_worker)
snprintf(bgw.bgw_function_name, BGW_MAXLEN, "ParallelApplyWorkerMain");
@@ -889,7 +889,7 @@ ApplyLauncherRegister(void)
bgw.bgw_flags = BGWORKER_SHMEM_ACCESS |
BGWORKER_BACKEND_DATABASE_CONNECTION;
bgw.bgw_start_time = BgWorkerStart_RecoveryFinished;
- snprintf(bgw.bgw_library_name, BGW_MAXLEN, "postgres");
+ snprintf(bgw.bgw_library_name, sizeof(bgw.bgw_library_name), "postgres");
snprintf(bgw.bgw_function_name, BGW_MAXLEN, "ApplyLauncherMain");
snprintf(bgw.bgw_name, BGW_MAXLEN,
"logical replication launcher");
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 845d4498e6..76e9f9f894 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -86,6 +86,11 @@ typedef enum
#define BGW_MAXLEN 96
#define BGW_EXTRALEN 128
+/*
+ * Ensure bgw_function_name's size is backwards-compatible and sensible
+ */
+StaticAssertDecl(MAXPGPATH >= BGW_MAXLEN, "MAXPGPATH must be at least equal to BGW_MAXLEN");
+
typedef struct BackgroundWorker
{
char bgw_name[BGW_MAXLEN];
@@ -93,7 +98,7 @@ typedef struct BackgroundWorker
int bgw_flags;
BgWorkerStartTime bgw_start_time;
int bgw_restart_time; /* in seconds, or BGW_NEVER_RESTART */
- char bgw_library_name[BGW_MAXLEN];
+ char bgw_library_name[MAXPGPATH];
char bgw_function_name[BGW_MAXLEN];
Datum bgw_main_arg;
char bgw_extra[BGW_EXTRALEN];
--
2.37.1 (Apple Git-137.1)
On Mon, Mar 13, 2023 at 07:57:47AM -0700, Yurii Rashkovskii wrote:
However, there are use cases where [potentially] longer names are
expected/desired; for example, test benches (where library files may not
[or can not] be copied to Postgres installation) or alternative library
installation methods that do not put them into $libdir.The patch is backwards-compatible and ensures that bgw_library_name stays
*at least* as long as BGW_MAXLEN. Existing external code that uses
BGW_MAXLEN is a length boundary (for example, in `strncpy`) will continue
to work as expected.
I see that BGW_MAXLEN was originally set to 64 in 2013 (7f7485a) [0]/messages/by-id/CA+TgmoYtQQ-JqAJPxZg3Mjg7EqugzqQ+ZBrpnXo95chWMCZsXw@mail.gmail.com, but
was increased to 96 in 2018 (3a4b891) [1]/messages/by-id/304a21ab-a9d6-264a-f688-912869c0d7c6@2ndquadrant.com. It seems generally reasonable
to me to increase the length of bgw_library_name further for the use-case
you describe, but I wonder if it'd be better to simply increase BGW_MAXLEN
again. However, IIUC bgw_library_name is the only field that is likely to
be used for absolute paths, so only increasing that one to MAXPGPATH makes
sense.
[0]: /messages/by-id/CA+TgmoYtQQ-JqAJPxZg3Mjg7EqugzqQ+ZBrpnXo95chWMCZsXw@mail.gmail.com
[1]: /messages/by-id/304a21ab-a9d6-264a-f688-912869c0d7c6@2ndquadrant.com
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Nathan,
Thank you for your review.
Indeed, my motivation for doing the change the way I did it was that only
bgw_library_name is expected to be longer, whereas it is much less of a
concern for other fields. If we have increased BGW_MAXLEN, it would have
increased the size of BackgroundWorker for little to no benefit.
On Mon, Mar 13, 2023 at 10:35 AM Nathan Bossart <nathandbossart@gmail.com>
wrote:
On Mon, Mar 13, 2023 at 07:57:47AM -0700, Yurii Rashkovskii wrote:
However, there are use cases where [potentially] longer names are
expected/desired; for example, test benches (where library files may not
[or can not] be copied to Postgres installation) or alternative library
installation methods that do not put them into $libdir.The patch is backwards-compatible and ensures that bgw_library_name stays
*at least* as long as BGW_MAXLEN. Existing external code that uses
BGW_MAXLEN is a length boundary (for example, in `strncpy`) will continue
to work as expected.I see that BGW_MAXLEN was originally set to 64 in 2013 (7f7485a) [0], but
was increased to 96 in 2018 (3a4b891) [1]. It seems generally reasonable
to me to increase the length of bgw_library_name further for the use-case
you describe, but I wonder if it'd be better to simply increase BGW_MAXLEN
again. However, IIUC bgw_library_name is the only field that is likely to
be used for absolute paths, so only increasing that one to MAXPGPATH makes
sense.[0]
/messages/by-id/CA+TgmoYtQQ-JqAJPxZg3Mjg7EqugzqQ+ZBrpnXo95chWMCZsXw@mail.gmail.com
[1]
/messages/by-id/304a21ab-a9d6-264a-f688-912869c0d7c6@2ndquadrant.com--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
--
http://omnigres.org
Yurii
On 13 Mar 2023, at 18:35, Nathan Bossart <nathandbossart@gmail.com> wrote:
On Mon, Mar 13, 2023 at 07:57:47AM -0700, Yurii Rashkovskii wrote:
However, there are use cases where [potentially] longer names are
expected/desired; for example, test benches (where library files may not
[or can not] be copied to Postgres installation) or alternative library
installation methods that do not put them into $libdir.The patch is backwards-compatible and ensures that bgw_library_name stays
*at least* as long as BGW_MAXLEN. Existing external code that uses
BGW_MAXLEN is a length boundary (for example, in `strncpy`) will continue
to work as expected.I see that BGW_MAXLEN was originally set to 64 in 2013 (7f7485a) [0], but
was increased to 96 in 2018 (3a4b891) [1]. It seems generally reasonable
to me to increase the length of bgw_library_name further for the use-case
you describe, but I wonder if it'd be better to simply increase BGW_MAXLEN
again. However, IIUC bgw_library_name is the only field that is likely to
be used for absolute paths, so only increasing that one to MAXPGPATH makes
sense.
Yeah, raising just bgw_library_name to MAXPGPATH seems reasonable here. While
the memory usage does grow it's still quite modest, and has an upper limit in
max_worker_processes.
While here, I wonder if we should document what BGW_MAXLEN is defined as in
bgworker.sgml?
--
Daniel Gustafsson
On Wed, Mar 15, 2023 at 10:38:34AM +0100, Daniel Gustafsson wrote:
While here, I wonder if we should document what BGW_MAXLEN is defined as in
bgworker.sgml?
I am -0.5 for this. If you are writing a new background worker, it's
probably reasonable to expect that you can locate the definition of
BGW_MAXLEN. Also, I think there's a good chance that we'd forget to update
such documentation the next time we adjust it.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On 21 Apr 2023, at 01:32, Nathan Bossart <nathandbossart@gmail.com> wrote:
On Wed, Mar 15, 2023 at 10:38:34AM +0100, Daniel Gustafsson wrote:
While here, I wonder if we should document what BGW_MAXLEN is defined as in
bgworker.sgml?I am -0.5 for this. If you are writing a new background worker, it's
probably reasonable to expect that you can locate the definition of
BGW_MAXLEN.
Of course. The question is if it's a helpful addition for someone who is
reading the documentation section on implementing background workers where we
explicitly mention BGW_MAXLEN without saying what it is.
Also, I think there's a good chance that we'd forget to update
such documentation the next time we adjust it.
There is that, but once set to MAXPGPATH it seems unlikely to change
particularly often so it seems the wrong thing to optimize for.
--
Daniel Gustafsson
On Fri, Apr 21, 2023 at 10:49:48AM +0200, Daniel Gustafsson wrote:
On 21 Apr 2023, at 01:32, Nathan Bossart <nathandbossart@gmail.com> wrote:
I am -0.5 for this. If you are writing a new background worker, it's
probably reasonable to expect that you can locate the definition of
BGW_MAXLEN.Of course. The question is if it's a helpful addition for someone who is
reading the documentation section on implementing background workers where we
explicitly mention BGW_MAXLEN without saying what it is.
IMHO it's better to have folks use the macro so that their calls to
snprintf(), etc. are updated when BGW_MAXLEN is changed. But I can't say
I'm strongly opposed to adding the value to the docs if you think it is
helpful.
Also, I think there's a good chance that we'd forget to update
such documentation the next time we adjust it.There is that, but once set to MAXPGPATH it seems unlikely to change
particularly often so it seems the wrong thing to optimize for.
True.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Hi,
The trade-off of this patch is that the `BackgroundWorker` structure becomes larger. From my perspective, this is a reasonable cost (less than a kilobyte of extra space per worker).
Agree.
The patch is backwards-compatible and ensures that bgw_library_name stays *at least* as long as BGW_MAXLEN. Existing external code that uses BGW_MAXLEN is a length boundary (for example, in `strncpy`) will continue to work as expected.
There is a mistake in the comment though:
```
+/*
+ * Ensure bgw_function_name's size is backwards-compatible and sensible
+ */
+StaticAssertDecl(MAXPGPATH >= BGW_MAXLEN, "MAXPGPATH must be at least
equal to BGW_MAXLEN");
```library_name, not function_name. Also I think the comment should be
more detailed, something like "prior to PG17 we used ... but since
PG17 ... which may cause problems if ...".
--
Best regards,
Aleksander Alekseev
Aleksander,
On Mon, Apr 24, 2023 at 1:01 PM Aleksander Alekseev <
aleksander@timescale.com> wrote:
The patch is backwards-compatible and ensures that bgw_library_name
stays *at least* as long as BGW_MAXLEN. Existing external code that uses
BGW_MAXLEN is a length boundary (for example, in `strncpy`) will continue
to work as expected.There is a mistake in the comment though:
``` +/* + * Ensure bgw_function_name's size is backwards-compatible and sensible + */ +StaticAssertDecl(MAXPGPATH >= BGW_MAXLEN, "MAXPGPATH must be at least equal to BGW_MAXLEN"); ```library_name, not function_name. Also I think the comment should be
more detailed, something like "prior to PG17 we used ... but since
PG17 ... which may cause problems if ...".
This is a very good catch and a suggestion. I've slightly reworked the
patch, and I also made this static assertion to have less indirection and,
therefore, make it easier to understand the premise.
Version 2 is attached.
--
Y.
Attachments:
v2-0001-Extend-the-length-of-BackgroundWorker.bgw_library_na.patchapplication/octet-stream; name=v2-0001-Extend-the-length-of-BackgroundWorker.bgw_library_na.patchDownload
From b7813f94c8546356a00218cf49db67a9cf194d96 Mon Sep 17 00:00:00 2001
From: Yurii Rashkovskii <yrashk@gmail.com>
Date: Mon, 24 Apr 2023 13:39:44 +0200
Subject: [PATCH] Extend the length of BackgroundWorker.bgw_library_name.
It is currently set at BGW_MAXLEN, which is often insufficient for
absolute paths. While it may be acceptable when used in conjunction with
$libdir, this makes it tricky to use libraries that have longer paths to
them.
The use cases where this is necessary may include:
* Local test benches
* Extensions provisioned outside of standard packaging or build
procedures
---
doc/src/sgml/bgworker.sgml | 2 +-
src/backend/postmaster/bgworker.c | 2 +-
src/backend/replication/logical/launcher.c | 4 ++--
src/include/postmaster/bgworker.h | 14 +++++++++++++-
4 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 7ba5da27e5..9ad1146ba0 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -59,7 +59,7 @@ typedef struct BackgroundWorker
int bgw_flags;
BgWorkerStartTime bgw_start_time;
int bgw_restart_time; /* in seconds, or BGW_NEVER_RESTART */
- char bgw_library_name[BGW_MAXLEN];
+ char bgw_library_name[MAXPGPATH];
char bgw_function_name[BGW_MAXLEN];
Datum bgw_main_arg;
char bgw_extra[BGW_EXTRALEN];
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 0dd22b2351..ade8b38984 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -362,7 +362,7 @@ BackgroundWorkerStateChange(bool allow_new_workers)
ascii_safe_strlcpy(rw->rw_worker.bgw_type,
slot->worker.bgw_type, BGW_MAXLEN);
ascii_safe_strlcpy(rw->rw_worker.bgw_library_name,
- slot->worker.bgw_library_name, BGW_MAXLEN);
+ slot->worker.bgw_library_name, sizeof(slot->worker.bgw_library_name));
ascii_safe_strlcpy(rw->rw_worker.bgw_function_name,
slot->worker.bgw_function_name, BGW_MAXLEN);
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 970d170e73..4f69b03cb3 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -456,7 +456,7 @@ retry:
bgw.bgw_flags = BGWORKER_SHMEM_ACCESS |
BGWORKER_BACKEND_DATABASE_CONNECTION;
bgw.bgw_start_time = BgWorkerStart_RecoveryFinished;
- snprintf(bgw.bgw_library_name, BGW_MAXLEN, "postgres");
+ snprintf(bgw.bgw_library_name, sizeof(bgw.bgw_library_name), "postgres");
if (is_parallel_apply_worker)
snprintf(bgw.bgw_function_name, BGW_MAXLEN, "ParallelApplyWorkerMain");
@@ -889,7 +889,7 @@ ApplyLauncherRegister(void)
bgw.bgw_flags = BGWORKER_SHMEM_ACCESS |
BGWORKER_BACKEND_DATABASE_CONNECTION;
bgw.bgw_start_time = BgWorkerStart_RecoveryFinished;
- snprintf(bgw.bgw_library_name, BGW_MAXLEN, "postgres");
+ snprintf(bgw.bgw_library_name, sizeof(bgw.bgw_library_name), "postgres");
snprintf(bgw.bgw_function_name, BGW_MAXLEN, "ApplyLauncherMain");
snprintf(bgw.bgw_name, BGW_MAXLEN,
"logical replication launcher");
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 845d4498e6..0ecb0f9804 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -93,13 +93,25 @@ typedef struct BackgroundWorker
int bgw_flags;
BgWorkerStartTime bgw_start_time;
int bgw_restart_time; /* in seconds, or BGW_NEVER_RESTART */
- char bgw_library_name[BGW_MAXLEN];
+ char bgw_library_name[MAXPGPATH];
char bgw_function_name[BGW_MAXLEN];
Datum bgw_main_arg;
char bgw_extra[BGW_EXTRALEN];
pid_t bgw_notify_pid; /* SIGUSR1 this backend on start/stop */
} BackgroundWorker;
+/*
+ * Ensure bgw_library_name's size is backwards-compatible and sensible.
+ *
+ * Before PostgreSQL 17, we used BGW_MAXLEN for bgw_library_name which was
+ * too short for some use cases, particularly, absolute paths.
+ *
+ * The assertion below ensures that the new length is at least as long as
+ * it was before.
+ */
+StaticAssertDecl(sizeof((BackgroundWorker *)NULL)->bgw_library_name >= BGW_MAXLEN,
+ "BackgroundWorker.bgw_library_name size must be at least equal to BGW_MAXLEN");
+
typedef enum BgwHandleStatus
{
BGWH_STARTED, /* worker is running */
--
2.39.2 (Apple Git-143)
Hi,
This is a very good catch and a suggestion. I've slightly reworked the patch, and I also made this static assertion to have less indirection and, therefore, make it easier to understand the premise.
Version 2 is attached.
```
sizeof((BackgroundWorker *)NULL)->bgw_library_name
```
I'm pretty confident something went wrong with the parentheses in v2.
--
Best regards,
Aleksander Alekseev
You're absolutely right. Here's v3.
On Mon, Apr 24, 2023 at 6:30 PM Aleksander Alekseev <
aleksander@timescale.com> wrote:
Hi,
This is a very good catch and a suggestion. I've slightly reworked the
patch, and I also made this static assertion to have less indirection and,
therefore, make it easier to understand the premise.Version 2 is attached.
```
sizeof((BackgroundWorker *)NULL)->bgw_library_name
```I'm pretty confident something went wrong with the parentheses in v2.
--
Best regards,
Aleksander Alekseev
--
Y.
Attachments:
v3-0001-Extend-the-length-of-BackgroundWorker.bgw_library_na.patchapplication/octet-stream; name=v3-0001-Extend-the-length-of-BackgroundWorker.bgw_library_na.patchDownload
From cba5ae258f4f8218cc046f03760d8dbf6ef96984 Mon Sep 17 00:00:00 2001
From: Yurii Rashkovskii <yrashk@gmail.com>
Date: Mon, 24 Apr 2023 19:42:27 +0200
Subject: [PATCH] Extend the length of BackgroundWorker.bgw_library_name.
It is currently set at BGW_MAXLEN, which is often insufficient for
absolute paths. While it may be acceptable when used in conjunction with
$libdir, this makes it tricky to use libraries that have longer paths to
them.
The use cases where this is necessary may include:
* Local test benches
* Extensions provisioned outside of standard packaging or build
procedures
---
doc/src/sgml/bgworker.sgml | 2 +-
src/backend/postmaster/bgworker.c | 2 +-
src/backend/replication/logical/launcher.c | 4 ++--
src/include/postmaster/bgworker.h | 14 +++++++++++++-
4 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 7ba5da27e5..9ad1146ba0 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -59,7 +59,7 @@ typedef struct BackgroundWorker
int bgw_flags;
BgWorkerStartTime bgw_start_time;
int bgw_restart_time; /* in seconds, or BGW_NEVER_RESTART */
- char bgw_library_name[BGW_MAXLEN];
+ char bgw_library_name[MAXPGPATH];
char bgw_function_name[BGW_MAXLEN];
Datum bgw_main_arg;
char bgw_extra[BGW_EXTRALEN];
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 0dd22b2351..ade8b38984 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -362,7 +362,7 @@ BackgroundWorkerStateChange(bool allow_new_workers)
ascii_safe_strlcpy(rw->rw_worker.bgw_type,
slot->worker.bgw_type, BGW_MAXLEN);
ascii_safe_strlcpy(rw->rw_worker.bgw_library_name,
- slot->worker.bgw_library_name, BGW_MAXLEN);
+ slot->worker.bgw_library_name, sizeof(slot->worker.bgw_library_name));
ascii_safe_strlcpy(rw->rw_worker.bgw_function_name,
slot->worker.bgw_function_name, BGW_MAXLEN);
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 970d170e73..4f69b03cb3 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -456,7 +456,7 @@ retry:
bgw.bgw_flags = BGWORKER_SHMEM_ACCESS |
BGWORKER_BACKEND_DATABASE_CONNECTION;
bgw.bgw_start_time = BgWorkerStart_RecoveryFinished;
- snprintf(bgw.bgw_library_name, BGW_MAXLEN, "postgres");
+ snprintf(bgw.bgw_library_name, sizeof(bgw.bgw_library_name), "postgres");
if (is_parallel_apply_worker)
snprintf(bgw.bgw_function_name, BGW_MAXLEN, "ParallelApplyWorkerMain");
@@ -889,7 +889,7 @@ ApplyLauncherRegister(void)
bgw.bgw_flags = BGWORKER_SHMEM_ACCESS |
BGWORKER_BACKEND_DATABASE_CONNECTION;
bgw.bgw_start_time = BgWorkerStart_RecoveryFinished;
- snprintf(bgw.bgw_library_name, BGW_MAXLEN, "postgres");
+ snprintf(bgw.bgw_library_name, sizeof(bgw.bgw_library_name), "postgres");
snprintf(bgw.bgw_function_name, BGW_MAXLEN, "ApplyLauncherMain");
snprintf(bgw.bgw_name, BGW_MAXLEN,
"logical replication launcher");
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 845d4498e6..e7ca0e8d88 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -93,13 +93,25 @@ typedef struct BackgroundWorker
int bgw_flags;
BgWorkerStartTime bgw_start_time;
int bgw_restart_time; /* in seconds, or BGW_NEVER_RESTART */
- char bgw_library_name[BGW_MAXLEN];
+ char bgw_library_name[MAXPGPATH];
char bgw_function_name[BGW_MAXLEN];
Datum bgw_main_arg;
char bgw_extra[BGW_EXTRALEN];
pid_t bgw_notify_pid; /* SIGUSR1 this backend on start/stop */
} BackgroundWorker;
+/*
+ * Ensure bgw_library_name's size is backwards-compatible and sensible.
+ *
+ * Before PostgreSQL 17, we used BGW_MAXLEN for bgw_library_name which was
+ * too short for some use cases, particularly, absolute paths.
+ *
+ * The assertion below ensures that the new length is at least as long as
+ * it was before.
+ */
+StaticAssertDecl(sizeof(((BackgroundWorker *)NULL)->bgw_library_name) >= BGW_MAXLEN,
+ "BackgroundWorker.bgw_library_name size must be at least equal to BGW_MAXLEN");
+
typedef enum BgwHandleStatus
{
BGWH_STARTED, /* worker is running */
--
2.39.2 (Apple Git-143)
Hi,
You're absolutely right. Here's v3.
Please avoid using top posting [1]https://wiki.postgresql.org/wiki/Mailing_Lists#Email_etiquette_mechanics.
The commit message may require a bit of tweaking by the committer but
other than that the patch seems to be fine. I'm going to mark it as
RfC in a bit unless anyone objects.
[1]: https://wiki.postgresql.org/wiki/Mailing_Lists#Email_etiquette_mechanics
--
Best regards,
Aleksander Alekseev
On Wed, Apr 26, 2023 at 03:07:18PM +0300, Aleksander Alekseev wrote:
The commit message may require a bit of tweaking by the committer but
other than that the patch seems to be fine. I'm going to mark it as
RfC in a bit unless anyone objects.
In v4, I've introduced a new BGW_LIBLEN macro and set it to the default
value of MAXPGPATH (1024). This way, the value can live in bgworker.h like
the other BGW_* macros do. Plus, this should make the assertion that
checks for backward compatibility unnecessary. Since bgw_library_name is
essentially a path, I can see the argument that we should just set
BGW_LIBLEN to MAXPGPATH directly. I'm curious what folks think about this.
I also changed the added sizeofs to use the macro for consistency with the
surrounding code.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v4-0001-extend-bgw_library_name.patchtext/x-diff; charset=us-asciiDownload
From 3760f8efd0a20f60f7eda19450f97c5b2e5daf8f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 30 Jun 2023 14:25:20 -0700
Subject: [PATCH v4 1/1] extend bgw_library_name
---
doc/src/sgml/bgworker.sgml | 2 +-
src/backend/postmaster/bgworker.c | 2 +-
src/backend/replication/logical/launcher.c | 4 ++--
src/include/postmaster/bgworker.h | 3 ++-
4 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 7ba5da27e5..f890216333 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -59,7 +59,7 @@ typedef struct BackgroundWorker
int bgw_flags;
BgWorkerStartTime bgw_start_time;
int bgw_restart_time; /* in seconds, or BGW_NEVER_RESTART */
- char bgw_library_name[BGW_MAXLEN];
+ char bgw_library_name[BGW_LIBLEN];
char bgw_function_name[BGW_MAXLEN];
Datum bgw_main_arg;
char bgw_extra[BGW_EXTRALEN];
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 0dd22b2351..bee2b895e3 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -362,7 +362,7 @@ BackgroundWorkerStateChange(bool allow_new_workers)
ascii_safe_strlcpy(rw->rw_worker.bgw_type,
slot->worker.bgw_type, BGW_MAXLEN);
ascii_safe_strlcpy(rw->rw_worker.bgw_library_name,
- slot->worker.bgw_library_name, BGW_MAXLEN);
+ slot->worker.bgw_library_name, BGW_LIBLEN);
ascii_safe_strlcpy(rw->rw_worker.bgw_function_name,
slot->worker.bgw_function_name, BGW_MAXLEN);
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 8395ae7b23..e1ed820b31 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -456,7 +456,7 @@ retry:
bgw.bgw_flags = BGWORKER_SHMEM_ACCESS |
BGWORKER_BACKEND_DATABASE_CONNECTION;
bgw.bgw_start_time = BgWorkerStart_RecoveryFinished;
- snprintf(bgw.bgw_library_name, BGW_MAXLEN, "postgres");
+ snprintf(bgw.bgw_library_name, BGW_LIBLEN, "postgres");
if (is_parallel_apply_worker)
snprintf(bgw.bgw_function_name, BGW_MAXLEN, "ParallelApplyWorkerMain");
@@ -910,7 +910,7 @@ ApplyLauncherRegister(void)
bgw.bgw_flags = BGWORKER_SHMEM_ACCESS |
BGWORKER_BACKEND_DATABASE_CONNECTION;
bgw.bgw_start_time = BgWorkerStart_RecoveryFinished;
- snprintf(bgw.bgw_library_name, BGW_MAXLEN, "postgres");
+ snprintf(bgw.bgw_library_name, BGW_LIBLEN, "postgres");
snprintf(bgw.bgw_function_name, BGW_MAXLEN, "ApplyLauncherMain");
snprintf(bgw.bgw_name, BGW_MAXLEN,
"logical replication launcher");
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 845d4498e6..f8c401093e 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -84,6 +84,7 @@ typedef enum
#define BGW_DEFAULT_RESTART_INTERVAL 60
#define BGW_NEVER_RESTART -1
#define BGW_MAXLEN 96
+#define BGW_LIBLEN 1024
#define BGW_EXTRALEN 128
typedef struct BackgroundWorker
@@ -93,7 +94,7 @@ typedef struct BackgroundWorker
int bgw_flags;
BgWorkerStartTime bgw_start_time;
int bgw_restart_time; /* in seconds, or BGW_NEVER_RESTART */
- char bgw_library_name[BGW_MAXLEN];
+ char bgw_library_name[BGW_LIBLEN];
char bgw_function_name[BGW_MAXLEN];
Datum bgw_main_arg;
char bgw_extra[BGW_EXTRALEN];
--
2.25.1
Hi Nathan,
On Fri, Jun 30, 2023 at 2:39 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:
In v4, I've introduced a new BGW_LIBLEN macro and set it to the default
value of MAXPGPATH (1024). This way, the value can live in bgworker.h like
the other BGW_* macros do. Plus, this should make the assertion that
checks for backward compatibility unnecessary. Since bgw_library_name is
essentially a path, I can see the argument that we should just set
BGW_LIBLEN to MAXPGPATH directly. I'm curious what folks think about this.
Thank you for revising the patch. While this is relatively minor, I think
it should be set to MAXPGPATH directly to clarify their relationship.
--
Y.
On Sun, Jul 02, 2023 at 04:37:52PM -0700, Yurii Rashkovskii wrote:
Thank you for revising the patch. While this is relatively minor, I think
it should be set to MAXPGPATH directly to clarify their relationship.
Committed. I set the size to MAXPGPATH directly instead of inventing a new
macro with the same value.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Nathan,
On Mon, Jul 3, 2023 at 3:08 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:
On Sun, Jul 02, 2023 at 04:37:52PM -0700, Yurii Rashkovskii wrote:
Thank you for revising the patch. While this is relatively minor, I think
it should be set to MAXPGPATH directly to clarify their relationship.Committed. I set the size to MAXPGPATH directly instead of inventing a new
macro with the same value.
Great, thank you! The reason I was leaving the other constant in place to
make upgrading extensions trivial (so that they don't need to adjust for
this), but if you think this is a better way, I am fine with it.
--
Y.
On Mon, Jul 03, 2023 at 06:00:12PM -0700, Yurii Rashkovskii wrote:
Great, thank you! The reason I was leaving the other constant in place to
make upgrading extensions trivial (so that they don't need to adjust for
this), but if you think this is a better way, I am fine with it.
Sorry, I'm not following. Which constant are you referring to?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Nathan,
On Mon, Jul 3, 2023 at 8:12 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:
On Mon, Jul 03, 2023 at 06:00:12PM -0700, Yurii Rashkovskii wrote:
Great, thank you! The reason I was leaving the other constant in place to
make upgrading extensions trivial (so that they don't need to adjust for
this), but if you think this is a better way, I am fine with it.Sorry, I'm not following. Which constant are you referring to?
Apologies, I misread the final patch. All good!
--
Y.