Horton

Thread

Commits

  1. Increase size of bgw_library_name.

  1. [PATCH] Extend the length of BackgroundWorker.bgw_library_name

    Yurii Rashkovskii <yrashk@gmail.com> — 2023-03-13T14:57:47Z

    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

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

    Nathan Bossart <nathandbossart@gmail.com> — 2023-03-13T17:35:28Z

    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] https://postgr.es/m/CA%2BTgmoYtQQ-JqAJPxZg3Mjg7EqugzqQ%2BZBrpnXo95chWMCZsXw%40mail.gmail.com
[1] https://postgr.es/m/304a21ab-a9d6-264a-f688-912869c0d7c6%402ndquadrant.com

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

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

    Yurii Rashkovskii <yrashk@gmail.com> — 2023-03-13T17:48:26Z

    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]
> https://postgr.es/m/CA%2BTgmoYtQQ-JqAJPxZg3Mjg7EqugzqQ%2BZBrpnXo95chWMCZsXw%40mail.gmail.com
> [1]
> https://postgr.es/m/304a21ab-a9d6-264a-f688-912869c0d7c6%402ndquadrant.com
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>


--
http://omnigres.org
Yurii

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-03-15T09:38:34Z

    > 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

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

    Nathan Bossart <nathandbossart@gmail.com> — 2023-04-20T23:32:43Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-04-21T08:49:48Z

    > 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

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

    Nathan Bossart <nathandbossart@gmail.com> — 2023-04-21T20:44:51Z

    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

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

    Aleksander Alekseev <aleksander@timescale.com> — 2023-04-24T11:01:30Z

    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

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

    Yurii Rashkovskii <yrashk@gmail.com> — 2023-04-24T11:40:10Z

    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.

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

    Aleksander Alekseev <aleksander@timescale.com> — 2023-04-24T16:30:19Z

    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

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

    Yurii Rashkovskii <yrashk@gmail.com> — 2023-04-24T17:43:51Z

    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.

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

    Aleksander Alekseev <aleksander@timescale.com> — 2023-04-26T12:07:18Z

    Hi,

> You're absolutely right. Here's v3.

Please avoid using top posting [1].

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

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

    Nathan Bossart <nathandbossart@gmail.com> — 2023-06-30T21:39:56Z

    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

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

    Yurii Rashkovskii <yrashk@gmail.com> — 2023-07-02T23:37:52Z

    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.

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

    Nathan Bossart <nathandbossart@gmail.com> — 2023-07-03T22:08:42Z

    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

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

    Yurii Rashkovskii <yrashk@gmail.com> — 2023-07-04T01:00:12Z

    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.

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

    Nathan Bossart <nathandbossart@gmail.com> — 2023-07-04T03:12:26Z

    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

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

    Yurii Rashkovskii <yrashk@gmail.com> — 2023-07-04T14:40:14Z

    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.