MIME-Version: 1.0
References: <CA+RLCQyjFV5Y8tG5QgUb6gjteL4S3p+1gcyqWTqigyM93WZ9Pg@mail.gmail.com>
 <20230313173528.GA195204@nathanxps13>
In-Reply-To: <20230313173528.GA195204@nathanxps13>
From: Yurii Rashkovskii <yrashk@gmail.com>
Date: Mon, 13 Mar 2023 10:48:26 -0700
Message-ID: <CA+RLCQzdWuwvtX14HO-5T0f5oTBHE9Yh4-C+=bHk0GTF2jDiOw@mail.gmail.com>
Subject: Re: [PATCH] Extend the length of BackgroundWorker.bgw_library_name
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="0000000000007c150505f6cbb878"
Archived-At: <https://www.postgresql.org/message-id/CA%2BRLCQzdWuwvtX14HO-5T0f5oTBHE9Yh4-C%2B%3DbHk0GTF2jDiOw%40mail.gmail.com>
Precedence: bulk

--0000000000007c150505f6cbb878
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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=E2=80=AFAM Nathan Bossart <nathandbossart@gma=
il.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 no=
t
> > [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 sta=
ys
> > *at least* as long as BGW_MAXLEN. Existing external code that uses
> > BGW_MAXLEN is a length boundary (for example, in `strncpy`) will contin=
ue
> > 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_MAXLE=
N
> again.  However, IIUC bgw_library_name is the only field that is likely t=
o
> be used for absolute paths, so only increasing that one to MAXPGPATH make=
s
> sense.
>
> [0]
> https://postgr.es/m/CA%2BTgmoYtQQ-JqAJPxZg3Mjg7EqugzqQ%2BZBrpnXo95chWMCZs=
Xw%40mail.gmail.com
> [1]
> https://postgr.es/m/304a21ab-a9d6-264a-f688-912869c0d7c6%402ndquadrant.co=
m
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>


--
http://omnigres.org
Yurii

--0000000000007c150505f6cbb878
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Nathan,<div><br></div><div>Thank you for =
your review.=C2=A0</div><div><br></div><div>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 hav=
e increased BGW_MAXLEN, it would have increased the size of BackgroundWorke=
r for little to no benefit.=C2=A0</div></div><br><div class=3D"gmail_quote"=
><div dir=3D"ltr" class=3D"gmail_attr">On Mon, Mar 13, 2023 at 10:35=E2=80=
=AFAM Nathan Bossart &lt;<a href=3D"mailto:nathandbossart@gmail.com">nathan=
dbossart@gmail.com</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote=
" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);=
padding-left:1ex">On Mon, Mar 13, 2023 at 07:57:47AM -0700, Yurii Rashkovsk=
ii wrote:<br>
&gt; However, there are use cases where [potentially] longer names are<br>
&gt; expected/desired; for example, test benches (where library files may n=
ot<br>
&gt; [or can not] be copied to Postgres installation) or alternative librar=
y<br>
&gt; installation methods that do not put them into $libdir.<br>
&gt; <br>
&gt; The patch is backwards-compatible and ensures that bgw_library_name st=
ays<br>
&gt; *at least* as long as BGW_MAXLEN. Existing external code that uses<br>
&gt; BGW_MAXLEN is a length boundary (for example, in `strncpy`) will conti=
nue<br>
&gt; to work as expected.<br>
<br>
I see that BGW_MAXLEN was originally set to 64 in 2013 (7f7485a) [0], but<b=
r>
was increased to 96 in 2018 (3a4b891) [1].=C2=A0 It seems generally reasona=
ble<br>
to me to increase the length of bgw_library_name further for the use-case<b=
r>
you describe, but I wonder if it&#39;d be better to simply increase BGW_MAX=
LEN<br>
again.=C2=A0 However, IIUC bgw_library_name is the only field that is likel=
y to<br>
be used for absolute paths, so only increasing that one to MAXPGPATH makes<=
br>
sense.<br>
<br>
[0] <a href=3D"https://postgr.es/m/CA%2BTgmoYtQQ-JqAJPxZg3Mjg7EqugzqQ%2BZBr=
pnXo95chWMCZsXw%40mail.gmail.com" rel=3D"noreferrer" target=3D"_blank">http=
s://postgr.es/m/CA%2BTgmoYtQQ-JqAJPxZg3Mjg7EqugzqQ%2BZBrpnXo95chWMCZsXw%40m=
ail.gmail.com</a><br>
[1] <a href=3D"https://postgr.es/m/304a21ab-a9d6-264a-f688-912869c0d7c6%402=
ndquadrant.com" rel=3D"noreferrer" target=3D"_blank">https://postgr.es/m/30=
4a21ab-a9d6-264a-f688-912869c0d7c6%402ndquadrant.com</a><br>
<br>
-- <br>
Nathan Bossart<br>
Amazon Web Services: <a href=3D"https://aws.amazon.com" rel=3D"noreferrer" =
target=3D"_blank">https://aws.amazon.com</a><br>
</blockquote></div><br clear=3D"all"><div><br></div><span class=3D"gmail_si=
gnature_prefix">--</span><div><span class=3D"gmail_signature_prefix"></span=
><a href=3D"http://omnigres.org">http://omnigres.org</a><br><div dir=3D"ltr=
" class=3D"gmail_signature"><div dir=3D"ltr"><div><div dir=3D"ltr"><div><di=
v dir=3D"ltr"><div>Yurii</div><div><br></div></div></div></div></div></div>=
</div></div></div>

--0000000000007c150505f6cbb878--