[PATCH] Add use of asprintf()

There is a lot of code around that does something like

char *val = malloc(...obscure computation...);
sprintf(val, "...", ...);

This can be had simpler, with asprintf(). It is not standard, but it's
more widely accepted in C libraries than strlcpy for example. The above
code would simply become

char * val;
asprintf(&val, "...", ...);

and asprintf() will figure out how much memory it needs by itself.

The attached patch should speak for itself.

I have supplied a few variants:

- asprintf() is the standard function, supplied by libpgport if
necessary.

- pg_asprintf() is asprintf() with automatic error handling (like
pg_malloc(), etc.)

- psprintf() is the same idea but with palloc.

I didn't touch most places involving MAXPGPATH. There was a discussion
recently about reducing its usage to save memory. That would work well
on top of my patch.

I did some desultory performance testing, which didn't result in any
concerns. Most of the changes are not in performance-critical paths.

If you're very picky you might notice that the psprintf() implementation
asserts that vsnprintf() does not return negative results. Very old
versions of glibc did that (before 2.1/February 1999), and this issue is
referenced in the current code of copy.c (via be4b8a86). But I think
this issue is too ancient now to be of concern.

Peter Eisentraut wrote:

> The attached patch should speak for itself.

Yeah, it's a very nice cleanup.

> I have supplied a few variants:
>
> - asprintf() is the standard function, supplied by libpgport if
> necessary.
>
> - pg_asprintf() is asprintf() with automatic error handling (like
> pg_malloc(), etc.)
>
> - psprintf() is the same idea but with palloc.

Looks good to me, except that pg_asprintf seems to be checking ret
instead of rc.

Is there a reason for the API discrepancy of pg_asprintf vs. psprintf?
I don't see that we use the integer return value anywhere. Callers
interested in the return value can use asprintf directly (and you have
already inserted callers that do nonstandard things using direct
asprintf).

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

Hi,

I did put some time review the patch, please see my findings below i.e.

1. It seems that you have used strdup() on multiple places in the patch,
e.g. in the below code snippet is it going to lead crash if newp->ident is
NULL because of strdup() failure ?

static EPlan *
> find_plan(char *ident, EPlan **eplan, int *nplans)
> {
> ...
> newp->ident = strdup(ident);
> ...
> }

2. Can we rely on return value of asprintf() instead of recompute length as
strlen(cmdbuf) ?

if (asprintf(&cmdbuf, "COMMENT ON LARGE OBJECT %u IS '", loid) <
> 0)
> return fail_lo_xact("\\lo_import", own_transaction);
> bufptr = cmdbuf + strlen(cmdbuf);

3. There seems naming convention for functions like pfree (that seems
similar to free()), pstrdup etc; but psprintf seems different than sprintf.
Can't we use pg_asprintf instead in the patch, as it seems that both
(psprintf and pg_asprintf) provide almost same functionality ?

4. It seems that "ret < 0" need to be changed to "rc < 0" in the following
code i.e.

int
> pg_asprintf(char **ret, const char *format, ...)
> {
> va_list ap;
> int rc;
> va_start(ap, format);
> rc = vasprintf(ret, format, ap);
> va_end(ap);
> if (ret < 0)
> {
> fprintf(stderr, _("out of memory\n"));
> exit(EXIT_FAILURE);
> }
> return rc;
> }

5. It seems that in the previously implementation, error handling was not
there, is it appropriate here that if there is issue in duplicating
environment, quit the application ? i.e.

/*
> * Handy subroutine for setting an environment variable "var" to "val"
> */
> static void
> doputenv(const char *var, const char *val)
> {
> char *s;
> pg_asprintf(&s, "%s=%s", var, val);
> putenv(s);
> }

Please do let me know if I missed something or more info is required. Thank
you.

Regards,
Muhammad Asif Naeem

On Tue, Sep 17, 2013 at 1:31 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:

> Peter Eisentraut wrote:
>
> > The attached patch should speak for itself.
>
> Yeah, it's a very nice cleanup.
>
> > I have supplied a few variants:
> >
> > - asprintf() is the standard function, supplied by libpgport if
> > necessary.
> >
> > - pg_asprintf() is asprintf() with automatic error handling (like
> > pg_malloc(), etc.)
> >
> > - psprintf() is the same idea but with palloc.
>
> Looks good to me, except that pg_asprintf seems to be checking ret
> instead of rc.
>
> Is there a reason for the API discrepancy of pg_asprintf vs. psprintf?
> I don't see that we use the integer return value anywhere. Callers
> interested in the return value can use asprintf directly (and you have
> already inserted callers that do nonstandard things using direct
> asprintf).
>
> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On Tue, Sep 17, 2013 at 3:13 PM, Asif Naeem <anaeem(dot)it(at)gmail(dot)com> wrote:

> Hi,
>
> I did put some time review the patch, please see my findings below i.e.
>
> 1. It seems that you have used strdup() on multiple places in the patch,
> e.g. in the below code snippet is it going to lead crash if newp->ident is
> NULL because of strdup() failure ?
>
> static EPlan *
>> find_plan(char *ident, EPlan **eplan, int *nplans)
>> {
>> ...
>> newp->ident = strdup(ident);
>> ...
>> }
>
>
> 2. Can we rely on return value of asprintf() instead of recompute length
> as strlen(cmdbuf) ?
>
> if (asprintf(&cmdbuf, "COMMENT ON LARGE OBJECT %u IS '", loid) <
>> 0)
>> return fail_lo_xact("\\lo_import", own_transaction);
>> bufptr = cmdbuf + strlen(cmdbuf);
>
>
> 3. There seems naming convention for functions like pfree (that seems
> similar to free()), pstrdup etc; but psprintf seems different than sprintf.
> Can't we use pg_asprintf instead in the patch, as it seems that both
> (psprintf and pg_asprintf) provide almost same functionality ?
>
> 4. It seems that "ret < 0" need to be changed to "rc < 0" in the following
> code i.e.
>
> int
>> pg_asprintf(char **ret, const char *format, ...)
>> {
>> va_list ap;
>> int rc;
>> va_start(ap, format);
>> rc = vasprintf(ret, format, ap);
>> va_end(ap);
>> if (ret < 0)
>> {
>> fprintf(stderr, _("out of memory\n"));
>> exit(EXIT_FAILURE);
>> }
>> return rc;
>> }
>
>
It seems this point is already mentioned by Alvaro. Thanks.

>
> 5. It seems that in the previously implementation, error handling was not
> there, is it appropriate here that if there is issue in duplicating
> environment, quit the application ? i.e.
>
> /*
>> * Handy subroutine for setting an environment variable "var" to "val"
>> */
>> static void
>> doputenv(const char *var, const char *val)
>> {
>> char *s;
>> pg_asprintf(&s, "%s=%s", var, val);
>> putenv(s);
>> }
>
>
>
> Please do let me know if I missed something or more info is required.
> Thank you.
>
> Regards,
> Muhammad Asif Naeem
>
>
> On Tue, Sep 17, 2013 at 1:31 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:
>
>> Peter Eisentraut wrote:
>>
>> > The attached patch should speak for itself.
>>
>> Yeah, it's a very nice cleanup.
>>
>> > I have supplied a few variants:
>> >
>> > - asprintf() is the standard function, supplied by libpgport if
>> > necessary.
>> >
>> > - pg_asprintf() is asprintf() with automatic error handling (like
>> > pg_malloc(), etc.)
>> >
>> > - psprintf() is the same idea but with palloc.
>>
>> Looks good to me, except that pg_asprintf seems to be checking ret
>> instead of rc.
>>
>> Is there a reason for the API discrepancy of pg_asprintf vs. psprintf?
>> I don't see that we use the integer return value anywhere. Callers
>> interested in the return value can use asprintf directly (and you have
>> already inserted callers that do nonstandard things using direct
>> asprintf).
>>
>> --
>> Álvaro Herrera http://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Training & Services
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>

On Mon, 2013-09-16 at 17:31 -0300, Alvaro Herrera wrote:
> Looks good to me, except that pg_asprintf seems to be checking ret
> instead of rc.

Ah, good catch!

> Is there a reason for the API discrepancy of pg_asprintf vs. psprintf?
> I don't see that we use the integer return value anywhere. Callers
> interested in the return value can use asprintf directly (and you have
> already inserted callers that do nonstandard things using direct
> asprintf).

I wanted to keep pg_asprintf the same as asprintf. I think there is
some value in that, but it's not supremely important.

On Tue, 2013-09-17 at 15:13 +0500, Asif Naeem wrote:

> 1. It seems that you have used strdup() on multiple places in the
> patch, e.g. in the below code snippet is it going to lead crash if
> newp->ident is NULL because of strdup() failure ?
>
> static EPlan *
> find_plan(char *ident, EPlan **eplan, int *nplans)
> {
> ...
> newp->ident = strdup(ident);
> ...
> }
>
The previous code used unchecked malloc, so this doesn't change
anything. It's only example code anyway. (The use of malloc instead of
palloc is dubious anyway.)

>
> 2. Can we rely on return value of asprintf() instead of recompute
> length as strlen(cmdbuf) ?
>
> if (asprintf(&cmdbuf, "COMMENT ON LARGE OBJECT %u IS
> '", loid) < 0)
> return fail_lo_xact("\\lo_import",
> own_transaction);
> bufptr = cmdbuf + strlen(cmdbuf);
>
Yes, good point.

> 3. There seems naming convention for functions like pfree (that seems
> similar to free()), pstrdup etc; but psprintf seems different than
> sprintf. Can't we use pg_asprintf instead in the patch, as it seems
> that both (psprintf and pg_asprintf) provide almost same
> functionality ?

pg_asprintf uses malloc, psprintf uses palloc, so they have to be
different functions.

The naming convention where

psomething = something with memory context management
pg_something = something with error checking

isn't very helpful, but it's what we have. Better ideas welcome.

>
> 5. It seems that in the previously implementation, error handling was
> not there, is it appropriate here that if there is issue in
> duplicating environment, quit the application ? i.e.
>
>
> /*
> * Handy subroutine for setting an environment variable "var"
> to "val"
> */
> static void
> doputenv(const char *var, const char *val)
> {
> char *s;
> pg_asprintf(&s, "%s=%s", var, val);
> putenv(s);
> }
>
I think so, yes.

On Tue, 2013-09-17 at 15:13 +0500, Asif Naeem wrote:
> I did put some time review the patch, please see my findings below
> i.e.

Updated patch for this.

Thank you Peter.
When I try to apply the patch on latest PG source code on master branch, it
gives the following error message i.e.

> asif$ git apply /Users/asif/core/Patch\:\ Add\ use\ of\
> asprintf\(\)/v2-0001-Add-use-of-asprintf.patch
> /Users/asif/core/Patch: Add use of
> asprintf()/v2-0001-Add-use-of-asprintf.patch:76: trailing whitespace.
> /Users/asif/core/Patch: Add use of
> asprintf()/v2-0001-Add-use-of-asprintf.patch:77: trailing whitespace.
> for ac_func in asprintf crypt fls getopt getrusage inet_aton random rint
> srandom strerror strlcat strlcpy
> /Users/asif/core/Patch: Add use of
> asprintf()/v2-0001-Add-use-of-asprintf.patch:90: trailing whitespace.
> AC_REPLACE_FUNCS([asprintf crypt fls getopt getrusage inet_aton random
> rint srandom strerror strlcat strlcpy])
> /Users/asif/core/Patch: Add use of
> asprintf()/v2-0001-Add-use-of-asprintf.patch:104: trailing whitespace.
> values[1] = psprintf("%s/%s", fctx->location, de->d_name);
> /Users/asif/core/Patch: Add use of
> asprintf()/v2-0001-Add-use-of-asprintf.patch:118: trailing whitespace.
> pg_asprintf(&todo,
> fatal: git apply: bad git-diff - expected /dev/null on line 1557

Neither git nor patch command apply the patch successfully. Can you please
guide ?. Thanks.

Best Regards,
Muhammad Asif Naeem

On Wed, Oct 2, 2013 at 7:29 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> On Tue, 2013-09-17 at 15:13 +0500, Asif Naeem wrote:
> > I did put some time review the patch, please see my findings below
> > i.e.
>
> Updated patch for this.
>
>

On 10/2/13 5:12 AM, Asif Naeem wrote:
> Neither git nor patch command apply the patch successfully. Can you
> please guide ?. Thanks.

Works for me. Check that your email client isn't mangling line endings.

You are right, wget worked. Latest patch looks good to me. make check run
fine. Thank you Peter.

On Wed, Oct 2, 2013 at 5:02 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> On 10/2/13 5:12 AM, Asif Naeem wrote:
> > Neither git nor patch command apply the patch successfully. Can you
> > please guide ?. Thanks.
>
> Works for me. Check that your email client isn't mangling line endings.
>
>

Peter Eisentraut escribió:
> On Tue, 2013-09-17 at 15:13 +0500, Asif Naeem wrote:
> > I did put some time review the patch, please see my findings below
> > i.e.
>
> Updated patch for this.

Looks good to me.

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

On Sat, Oct 12, 2013 at 2:58 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:

> Peter Eisentraut escribió:
> > On Tue, 2013-09-17 at 15:13 +0500, Asif Naeem wrote:
> > > I did put some time review the patch, please see my findings below
> > > i.e.
> >
> > Updated patch for this.
>
> Looks good to me.
>
>
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=5b6d08cd2992922b667564a49f19580f11676050

This commit is has broken the visual studios windows build. I'm looking
into it now

31 errors similar to:

"D:\Postgres\c\pgsql.sln" (default target) (1) ->
"D:\Postgres\c\pg_archivecleanup.vcxproj" (default target) (56) ->
libpgport.lib(asprintf.obj) : error LNK2019: unresolved external symbol
_va_copy referenced in function _vasprintf
[D:\Postgres\c\pg_archivecleanup.vcxproj]
.\Release\pg_archivecleanup\pg_archivecleanup.exe : fatal error LNK1120:
1 unresolved externals [D:\Postgres\c\pg_archivecleanup.vcxproj]

Regards

David Rowley

--
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On Mon, Oct 14, 2013 at 9:45 PM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

> On Sat, Oct 12, 2013 at 2:58 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:
>
>> Peter Eisentraut escribió:
>> > On Tue, 2013-09-17 at 15:13 +0500, Asif Naeem wrote:
>> > > I did put some time review the patch, please see my findings below
>> > > i.e.
>> >
>> > Updated patch for this.
>>
>> Looks good to me.
>>
>>
>
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=5b6d08cd2992922b667564a49f19580f11676050
>
> This commit is has broken the visual studios windows build. I'm looking
> into it now
>

Looks like something like:

#ifndef WIN32
#define HAVE_VA_COPY 1
#endif

would need to be added to asprintf.c, but also some work needs to be done
with mcxt.c as it uses va_copy unconditionally. Perhaps just defining a
macro for va_copy would be better for windows. I was not quite sure the
best header file for such a macro so I did not write a patch to fix it.

Regards

David Rowley

>
> 31 errors similar to:
>
> "D:\Postgres\c\pgsql.sln" (default target) (1) ->
> "D:\Postgres\c\pg_archivecleanup.vcxproj" (default target) (56) ->
> libpgport.lib(asprintf.obj) : error LNK2019: unresolved external symbol
> _va_copy referenced in function _vasprintf
> [D:\Postgres\c\pg_archivecleanup.vcxproj]
> .\Release\pg_archivecleanup\pg_archivecleanup.exe : fatal error LNK1120:
> 1 unresolved externals [D:\Postgres\c\pg_archivecleanup.vcxproj]
>
> Regards
>
> David Rowley
>
>
> --
>> Álvaro Herrera http://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Training & Services
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>

On Mon, 2013-10-14 at 23:08 +1300, David Rowley wrote:

>
> Looks like something like:
>
>
> #ifndef WIN32
> #define HAVE_VA_COPY 1
> #endif
>
>
> would need to be added to asprintf.c, but also some work needs to be
> done with mcxt.c as it uses va_copy unconditionally. Perhaps just
> defining a macro for va_copy would be better for windows. I was not
> quite sure the best header file for such a macro so I did not write a
> patch to fix it.

Does Windows not have va_copy? What do they use instead?

On Tue, Oct 15, 2013 at 2:18 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On Mon, 2013-10-14 at 23:08 +1300, David Rowley wrote:
>
>>
>> Looks like something like:
>>
>>
>> #ifndef WIN32
>> #define HAVE_VA_COPY 1
>> #endif
>>
>>
>> would need to be added to asprintf.c, but also some work needs to be
>> done with mcxt.c as it uses va_copy unconditionally. Perhaps just
>> defining a macro for va_copy would be better for windows. I was not
>> quite sure the best header file for such a macro so I did not write a
>> patch to fix it.
>
> Does Windows not have va_copy? What do they use instead?

No, Windows doesn't have va_copy, instead they use something like below:
#define va_copy(dest, src) (dest = src)

Please refer below link for details of porting va_copy() on Windows:
http://stackoverflow.com/questions/558223/va-copy-porting-to-visual-c

I could see that there is similar handling in code of vasprintf(),
such that if va_copy is not available then directly assign src to dst.

#if defined(HAVE_VA_COPY)
va_copy(ap2, ap);
#define my_va_end(ap2) va_end(ap2)
#elif defined(HAVE___BUILTIN_VA_COPY)
__builtin_va_copy(ap2, ap);
#define my_va_end(ap2) __builtin_va_end(ap2)
#else
ap2 = ap;
#define my_va_end(ap2) do {} while (0)
#endif

I think rather than having writing code like above at places where
va_copy is used, we can use something like:
#ifdef WIN32
#define va_copy(dest, src) (dest = src)
#endif

and define HAVE_VA_COPY to 1 for non-windows platform.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

+1

I think you can safely use va_list without copy on Windows. va_copy is
available in Visual Studio 2013 as part of support for C99, previous
versions don't have it.

Regards,
Muhammad Asif Naeem

On Tue, Oct 15, 2013 at 10:33 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>wrote:

> On Tue, Oct 15, 2013 at 2:18 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > On Mon, 2013-10-14 at 23:08 +1300, David Rowley wrote:
> >
> >>
> >> Looks like something like:
> >>
> >>
> >> #ifndef WIN32
> >> #define HAVE_VA_COPY 1
> >> #endif
> >>
> >>
> >> would need to be added to asprintf.c, but also some work needs to be
> >> done with mcxt.c as it uses va_copy unconditionally. Perhaps just
> >> defining a macro for va_copy would be better for windows. I was not
> >> quite sure the best header file for such a macro so I did not write a
> >> patch to fix it.
> >
> > Does Windows not have va_copy? What do they use instead?
>
> No, Windows doesn't have va_copy, instead they use something like below:
> #define va_copy(dest, src) (dest = src)
>
> Please refer below link for details of porting va_copy() on Windows:
> http://stackoverflow.com/questions/558223/va-copy-porting-to-visual-c
>
> I could see that there is similar handling in code of vasprintf(),
> such that if va_copy is not available then directly assign src to dst.
>
> #if defined(HAVE_VA_COPY)
> va_copy(ap2, ap);
> #define my_va_end(ap2) va_end(ap2)
> #elif defined(HAVE___BUILTIN_VA_COPY)
> __builtin_va_copy(ap2, ap);
> #define my_va_end(ap2) __builtin_va_end(ap2)
> #else
> ap2 = ap;
> #define my_va_end(ap2) do {} while (0)
> #endif
>
> I think rather than having writing code like above at places where
> va_copy is used, we can use something like:
> #ifdef WIN32
> #define va_copy(dest, src) (dest = src)
> #endif
>
> and define HAVE_VA_COPY to 1 for non-windows platform.
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>

On Tue, Oct 15, 2013 at 9:48 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> On Mon, 2013-10-14 at 23:08 +1300, David Rowley wrote:
>
> >
> > Looks like something like:
> >
> >
> > #ifndef WIN32
> > #define HAVE_VA_COPY 1
> > #endif
> >
> >
> > would need to be added to asprintf.c, but also some work needs to be
> > done with mcxt.c as it uses va_copy unconditionally. Perhaps just
> > defining a macro for va_copy would be better for windows. I was not
> > quite sure the best header file for such a macro so I did not write a
> > patch to fix it.
>
> Does Windows not have va_copy? What do they use instead?
>
>
Not quite sure what is used instead as I've never had the need to use it
before, but Mircosoft do seem to be getting around to implementing the C99
standard for Visual Studios 2013. See here.

http://msdn.microsoft.com/en-us/library/vstudio/kb57fad8(v=vs.120).aspx

If we skip back to VS2012, it does not exist:

http://msdn.microsoft.com/en-us/library/vstudio/kb57fad8(v=vs.110).aspx

So maybe this is the fix for it, which I think should be forwards
compatible for vs2013 and beyond when we go there.

diff --git a/src/include/c.h b/src/include/c.h
index 8916310..30e68ff 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -63,6 +63,11 @@
#undef errcode
#endif

+/* Visual Studios 2012 and earlier don't have va_copy() */
+#if _MSC_VER <= 1700
+#define va_copy(dest, src) ((dest) = (src))
+#endif
+
/*
* We have to include stdlib.h here because it defines many of these macros
* on some platforms, and we only want our definitions used if stdlib.h
doesn't

Though this is not yet enough to get the windows build work with me... I'm
still getting link failures for isolationtester.c

"D:\Postgres\c\pgsql.sln" (default target) (1) ->
"D:\Postgres\c\isolationtester.vcxproj" (default target) (89) ->
(Link target) ->
isolationtester.obj : error LNK2019: unresolved external symbol
_pg_strdup referenced in function _try_complete_step
[D:\Postgres\c\isolationtester.vcxproj]
isolationtester.obj : error LNK2019: unresolved external symbol
_pg_asprintf referenced in function _try_complete_step
[D:\Postgres\c\isolationtester.vcxproj
]
.\Release\isolationtester\isolationtester.exe : fatal error LNK1120: 2
unresolved externals [D:\Postgres\c\isolationtester.vcxproj]

1 Warning(s)

I guess this is down to a make file error somewhere.

David

On Tue, Oct 15, 2013 at 10:55 AM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

> Though this is not yet enough to get the windows build work with me... I'm
> still getting link failures for isolationtester.c
>
>
> "D:\Postgres\c\pgsql.sln" (default target) (1) ->
> "D:\Postgres\c\isolationtester.vcxproj" (default target) (89) ->
> (Link target) ->
> isolationtester.obj : error LNK2019: unresolved external symbol
> _pg_strdup referenced in function _try_complete_step
> [D:\Postgres\c\isolationtester.vcxproj]
> isolationtester.obj : error LNK2019: unresolved external symbol
> _pg_asprintf referenced in function _try_complete_step
> [D:\Postgres\c\isolationtester.vcxproj
> ]
> .\Release\isolationtester\isolationtester.exe : fatal error LNK1120: 2
> unresolved externals [D:\Postgres\c\isolationtester.vcxproj]
>
> 1 Warning(s)
>
> I guess this is down to a make file error somewhere.
>

Can you please try the attached patch ?. I hope it will solve the problem.

>
> David
>
>
>
>

On Tue, Oct 15, 2013 at 8:00 PM, Asif Naeem <anaeem(dot)it(at)gmail(dot)com> wrote:

>
>
>
> On Tue, Oct 15, 2013 at 10:55 AM, David Rowley <dgrowleyml(at)gmail(dot)com>wrote:
>
>> Though this is not yet enough to get the windows build work with me...
>> I'm still getting link failures for isolationtester.c
>>
>>
>> "D:\Postgres\c\pgsql.sln" (default target) (1) ->
>> "D:\Postgres\c\isolationtester.vcxproj" (default target) (89) ->
>> (Link target) ->
>> isolationtester.obj : error LNK2019: unresolved external symbol
>> _pg_strdup referenced in function _try_complete_step
>> [D:\Postgres\c\isolationtester.vcxproj]
>> isolationtester.obj : error LNK2019: unresolved external symbol
>> _pg_asprintf referenced in function _try_complete_step
>> [D:\Postgres\c\isolationtester.vcxproj
>> ]
>> .\Release\isolationtester\isolationtester.exe : fatal error LNK1120: 2
>> unresolved externals [D:\Postgres\c\isolationtester.vcxproj]
>>
>> 1 Warning(s)
>>
>> I guess this is down to a make file error somewhere.
>>
>
> Can you please try the attached patch ?. I hope it will solve the problem.
>

Thanks that combined with my patch above seems to get the build running
again.
In summary I've attached a patch which is both of these combined.

I've not run any regression tests yet as that seems to be broken, but
perhaps it is something changed with my build environment. The attached is
probably worth applying to get the windows build farm members building
again to see if they'll go green.

Regards

David Rowley

>
>
>>
>> David
>>
>>
>>
>>
>