Re: pg_xlogdump --stats

On 2014-07-04 18:59:07 +0530, Abhijit Menon-Sen wrote:
> At 2014-07-04 11:34:21 +0200, andres(at)2ndquadrant(dot)com wrote:
> >
> > I think we're going to have to redefine the
> > PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT callsite in configure.in to
> > define INT64_MODIFIER='"ll/l/I64D"'
>
> I've attached a patch to do this, and also add INT64_MODIFIER to
> pg_config.h.in: 0001-modifier.diff
>
> I reran autoconf, and just for convenience I've attached the resulting
> changes to configure: 0002-configure.diff
>
> Then there are the rm_identify changes: 0003-rmid.diff
>
> Finally, the xlogdump patch using INT64_MODIFIER: 0004-xlogdump.diff
>
> I can confirm that this series applies in-order to master, and that the
> result builds cleanly (including after each patch) on my machine, and
> that the resulting pg_xlogdump works as expected.

> Two of the extra calls to psprintf in pg_xlogdump happen at most
> RM_MAX_ID*16 (i.e. O(record-types) not O(records)) times, and the other
> two happen just before exit. It would be easy to use a static buffer and
> snprintf, but I don't think it's worth doing in this case.

Agreed.

> diff --git a/config/c-library.m4 b/config/c-library.m4
> index 8f45010..4821a61 100644
> --- a/config/c-library.m4
> +++ b/config/c-library.m4
> @@ -221,22 +221,22 @@ HAVE_POSIX_SIGNALS=$pgac_cv_func_posix_signals
> AC_SUBST(HAVE_POSIX_SIGNALS)])# PGAC_FUNC_POSIX_SIGNALS
>
>
> -# PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT
> +# PGAC_FUNC_SNPRINTF_LONG_LONG_INT_MODIFIER
> # ---------------------------------------
> -# Determine which format snprintf uses for long long int. We handle
> -# %lld, %qd, %I64d. The result is in shell variable
> -# LONG_LONG_INT_FORMAT.
> +# Determine which length modifier snprintf uses for long long int. We
> +# handle ll, q, and I64. The result is in shell variable
> +# LONG_LONG_INT_MODIFIER.
> #
> # MinGW uses '%I64d', though gcc throws an warning with -Wall,
> # while '%lld' doesn't generate a warning, but doesn't work.
> #
> -AC_DEFUN([PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT],
> -[AC_MSG_CHECKING([snprintf format for long long int])
> -AC_CACHE_VAL(pgac_cv_snprintf_long_long_int_format,
> -[for pgac_format in '%lld' '%qd' '%I64d'; do
> +AC_DEFUN([PGAC_FUNC_SNPRINTF_LONG_LONG_INT_MODIFIER],
> +[AC_MSG_CHECKING([snprintf length modifier for long long int])
> +AC_CACHE_VAL(pgac_cv_snprintf_long_long_int_modifier,
> +[for pgac_modifier in 'll' 'q' 'I64'; do
> AC_TRY_RUN([#include <stdio.h>
> typedef long long int ac_int64;
> -#define INT64_FORMAT "$pgac_format"
> +#define INT64_FORMAT "%${pgac_modifier}d"

I'd rather not define INT64_FORMAT here.

> +INT64_FORMAT="\"%${LONG_LONG_INT_MODIFIER}d\""
> +UINT64_FORMAT="\"%${LONG_LONG_INT_MODIFIER}u\""
> +INT64_MODIFIER="\"$LONG_LONG_INT_MODIFIER\""
> +

> AC_DEFINE_UNQUOTED(INT64_FORMAT, $INT64_FORMAT,
> [Define to the appropriate snprintf format for 64-bit ints.])
>
> AC_DEFINE_UNQUOTED(UINT64_FORMAT, $UINT64_FORMAT,
> [Define to the appropriate snprintf format for unsigned 64-bit ints.])
>
> +AC_DEFINE_UNQUOTED(INT64_MODIFIER, $INT64_MODIFIER,
> + [Define to the appropriate snprintf length modifier for 64-bit ints.])
> +

I'd suggest only defining INT64_MODIFIER here and build INT64_FORMAT,
UINT64_FORMAT ontop, in c.h.

> NOTE: I do not know what to do about pg_config.h.win32. If someone tells
> me what to do, I can submit another patch.

Which would also take care of pg_config.h.win32 - just define
INT64_MODIFIER in there instead of INT64_FORMAT/UINT64_FORMAT and you
should be good.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services