Horton

Thread

Commits

  1. Offer pnstrdup to frontend code

  1. adding strndup

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-04T14:40:21Z

    I just proposed in
https://postgr.es/m/0191204143715.GA17312@alvherre.pgsql the addition of
strndup() to our src/port.

I think this should be pretty uncontroversial, but wanted to give a
heads-up outside that thread.  I attach the patch here for completeness.

-- 
Álvaro Herrera                                http://www.twitter.com/alvherre

  2. Re: adding strndup

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-04T15:54:53Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I just proposed in
> https://postgr.es/m/0191204143715.GA17312@alvherre.pgsql the addition of
> strndup() to our src/port.
> I think this should be pretty uncontroversial, but wanted to give a
> heads-up outside that thread.  I attach the patch here for completeness.

Grepping, I notice that ecpg has an ecpg_strndup; should that be
replaced with this?

			regards, tom lane

  3. Re: adding strndup

    Andres Freund <andres@anarazel.de> — 2019-12-04T18:04:57Z

    Hi,

On 2019-12-04 11:40:21 -0300, Alvaro Herrera wrote:
> I just proposed in
> https://postgr.es/m/0191204143715.GA17312@alvherre.pgsql the addition of
> strndup() to our src/port.
> 
> I think this should be pretty uncontroversial, but wanted to give a
> heads-up outside that thread.  I attach the patch here for completeness.

Well, I personally think it's a bad idea to add further implementations
for functions that are in standar libraries on some systems. Especially,
but not exclusively, when made available for frontend code, where it's
not unlikely that there might be other applications having their own
implementations of strndup/whatever.

Besides that reason, I think AC_REPLACE_FUNCS is just a bad mechanism,
that yields fragmented source code and needs to implemented differently
for windows.  The code additionally often will also be badly optimized
in general, due to tiny translation units without relevant functions
having knoweldge about each other.

I'd just provide pnstrdup() in the frontend, without adding strndup().

I also see no point in adding both pnstrdup() and pg_strndup(). I'm fine
with moving towards pg_strndup(), but then we just ought to remove
pnstrdup().

- Andres

  4. Re: adding strndup

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-04T18:58:49Z

    Andres Freund <andres@anarazel.de> writes:
> On 2019-12-04 11:40:21 -0300, Alvaro Herrera wrote:
>> I think this should be pretty uncontroversial, but wanted to give a
>> heads-up outside that thread.  I attach the patch here for completeness.

> I'd just provide pnstrdup() in the frontend, without adding strndup().

+1 --- seems like a bunch more mechanism than is warranted.  Let's
just open-code it in pnstrdup.  We can rely on strnlen, since that's
already supported, and there's not much more there beyond that.

> I also see no point in adding both pnstrdup() and pg_strndup(). I'm fine
> with moving towards pg_strndup(), but then we just ought to remove
> pnstrdup().

There's a fair number of uses of pnstrdup in the backend.  While it
wouldn't be too painful to rename them, I'm not sure I see the point.
(What I'd really argue for, if we did rename, is "pstrndup".)

			regards, tom lane

  5. Re: adding strndup

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-04T19:05:01Z

    On 2019-Dec-04, Tom Lane wrote:

> Andres Freund <andres@anarazel.de> writes:
> > On 2019-12-04 11:40:21 -0300, Alvaro Herrera wrote:
> >> I think this should be pretty uncontroversial, but wanted to give a
> >> heads-up outside that thread.  I attach the patch here for completeness.
> 
> > I'd just provide pnstrdup() in the frontend, without adding strndup().
> 
> +1 --- seems like a bunch more mechanism than is warranted.  Let's
> just open-code it in pnstrdup.  We can rely on strnlen, since that's
> already supported, and there's not much more there beyond that.

I can get behind that ... it makes the patch a lot smaller.  I'm gonna
send an updated version in a jiffy.

> > I also see no point in adding both pnstrdup() and pg_strndup(). I'm fine
> > with moving towards pg_strndup(), but then we just ought to remove
> > pnstrdup().
> 
> There's a fair number of uses of pnstrdup in the backend.  While it
> wouldn't be too painful to rename them, I'm not sure I see the point.
> (What I'd really argue for, if we did rename, is "pstrndup".)

*shrug*  I also looked for pstrndup() first.  And Peter E also in
https://postgr.es/m/1339713732.11971.79.camel@vanquo.pezone.net
submitted an implementation of pstrndup().  I'm not opposed to renaming
it, but I hesitate to do it at the same time as putting it in pgport.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: adding strndup

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-04T22:01:46Z

    On 2019-Dec-04, Alvaro Herrera wrote:

> On 2019-Dec-04, Tom Lane wrote:
> 
> > Andres Freund <andres@anarazel.de> writes:
> > > On 2019-12-04 11:40:21 -0300, Alvaro Herrera wrote:
> > >> I think this should be pretty uncontroversial, but wanted to give a
> > >> heads-up outside that thread.  I attach the patch here for completeness.
> > 
> > > I'd just provide pnstrdup() in the frontend, without adding strndup().
> > 
> > +1 --- seems like a bunch more mechanism than is warranted.  Let's
> > just open-code it in pnstrdup.  We can rely on strnlen, since that's
> > already supported, and there's not much more there beyond that.
> 
> I can get behind that ... it makes the patch a lot smaller.

Here it is.

I noticed that ECPG's copy was setting errno.  I had forgot to do that
in my previous patch, but on second look, malloc failure already sets
it, so doing it again is pointless.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  7. Re: adding strndup

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-04T22:17:10Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> I can get behind that ... it makes the patch a lot smaller.

> Here it is.

LGTM.

> I noticed that ECPG's copy was setting errno.  I had forgot to do that
> in my previous patch, but on second look, malloc failure already sets
> it, so doing it again is pointless.

Right.

			regards, tom lane