Re: [HACKERS] [PATCH] Add use of asprintf()
On Tue, Oct 15, 2013 at 10:55 AM, David Rowley dgrowle...@gmail.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 Win_isolationtester_fix.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
On Tue, Oct 15, 2013 at 8:00 PM, Asif Naeem anaeem...@gmail.com wrote: On Tue, Oct 15, 2013 at 10:55 AM, David Rowley dgrowle...@gmail.comwrote: 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 asprintf_windows_fix.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
On Sat, Oct 12, 2013 at 2:58 AM, Alvaro Herrera alvhe...@2ndquadrant.comwrote: 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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
On Mon, Oct 14, 2013 at 9:45 PM, David Rowley dgrowle...@gmail.com wrote: On Sat, Oct 12, 2013 at 2:58 AM, Alvaro Herrera alvhe...@2ndquadrant.comwrote: 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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
On Tue, Oct 15, 2013 at 2:18 AM, Peter Eisentraut pete...@gmx.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
+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.kapil...@gmail.comwrote:
On Tue, Oct 15, 2013 at 2:18 AM, Peter Eisentraut pete...@gmx.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
Re: [HACKERS] [PATCH] Add use of asprintf()
On Tue, Oct 15, 2013 at 9:48 AM, Peter Eisentraut pete...@gmx.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
Re: [HACKERS] [PATCH] Add use of asprintf()
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
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 pete...@gmx.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.
Re: [HACKERS] [PATCH] Add use of asprintf()
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 pete...@gmx.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.
Re: [HACKERS] [PATCH] Add use of asprintf()
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
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 alvhe...@2ndquadrant.comwrote:
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
On Tue, Sep 17, 2013 at 3:13 PM, Asif Naeem anaeem...@gmail.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
alvhe...@2ndquadrant.comwrote:
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add use of asprintf()
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
