Subject: Re: [PATCH] Extend the length of BackgroundWorker.bgw_library_name

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 15 Mar 2023 10:38:34 +0100

> On 13 Mar 2023, at 18:35, Nathan Bossart <[email-masked]> 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

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 20 Apr 2023 16:32:43 -0700

> While here, I wonder if we should document what BGW_MAXLEN is defined as in

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

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 21 Apr 2023 10:49:48 +0200

> On 21 Apr 2023, at 01:32, Nathan Bossart <[email-masked]> wrote:
> 
> 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

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

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 21 Apr 2023 13:44:51 -0700

> 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.

> particularly often so it seems the wrong thing to optimize for.

True.