Re: Support for XLogRecPtr in expand_fmt_string?

Hi,

I wonder if we just should add a format code like %R or something similar as a
replacement for the %X/%X notion. Having to type something like "(uint32)
(state->curptr >> 32), (uint32)state->curptr" everywhere is somewhat annoying.

Opinions?

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

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> I wonder if we just should add a format code like %R or something similar as a
> replacement for the %X/%X notion.

Only if you can explain how to teach gcc what it means for elog argument
match checking. %m is a special case in that it matches up with a
longstanding glibc-ism that gcc knows about. Adding format codes of our
own invention would be problematic.

> Having to type something like "(uint32)
> (state->curptr >> 32), (uint32)state->curptr" everywhere is somewhat annoying.

If we really feel this is worth doing something about, we could invent a
formatting subroutine that converts XLogRecPtr to string (and then we
just use %s in the messages).

regards, tom lane

On tis, 2012-07-03 at 19:35 +0200, Andres Freund wrote:
> I wonder if we just should add a format code like %R or something similar as a
> replacement for the %X/%X notion. Having to type something like "(uint32)
> (state->curptr >> 32), (uint32)state->curptr" everywhere is somewhat annoying.

Maybe just print it as a single 64-bit value from now on.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On tis, 2012-07-03 at 19:35 +0200, Andres Freund wrote:
>> I wonder if we just should add a format code like %R or something similar as a
>> replacement for the %X/%X notion.

> Maybe just print it as a single 64-bit value from now on.

That'd be problematic also, because of the lack of standardization of
the format code for uint64. We could write things like
"message... " UINT64_FORMAT " ...more message"
but I wonder how well the translation tools would work with that;
and anyway it would at least double the translation effort for
messages containing such things.

regards, tom lane

On Tuesday, July 03, 2012 08:09:40 PM Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > I wonder if we just should add a format code like %R or something similar
> > as a replacement for the %X/%X notion.
> Only if you can explain how to teach gcc what it means for elog argument
> match checking. %m is a special case in that it matches up with a
> longstanding glibc-ism that gcc knows about. Adding format codes of our
> own invention would be problematic.
Ah. Yes. That kills the idea.

> > Having to type something like "(uint32)
> > (state->curptr >> 32), (uint32)state->curptr" everywhere is somewhat
> > annoying.
> If we really feel this is worth doing something about, we could invent a
> formatting subroutine that converts XLogRecPtr to string (and then we
> just use %s in the messages).
I think that would make memory management annoying. Using a static buffer
isn't going to work very well either because its valid to pass two recptr's to
elog/ereport/....

I think at that point the current state is not worth the hassle, sorry for the
noise.

Greetings,

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

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On Tuesday, July 03, 2012 08:09:40 PM Tom Lane wrote:
>> If we really feel this is worth doing something about, we could invent a
>> formatting subroutine that converts XLogRecPtr to string (and then we
>> just use %s in the messages).

> I think that would make memory management annoying. Using a static buffer
> isn't going to work very well either because its valid to pass two recptr's to
> elog/ereport/....

Hm. I was assuming that we could probably get away with the
static-buffer trick, but if you think not ...

One possibility is to make call sites that need this pass local-variable
buffers to the formatting subroutine:

char xrp_buffer[XLOGRECPTR_BUF_LEN];
char xrp_buffer2[XLOGRECPTR_BUF_LEN];

ereport(....,
format_xlogrecptr(xrp_buffer, xlogval1),
format_xlogrecptr(xrp_buffer2, xlogval2));

but it may not be worth the trouble.

regards, tom lane

On tis, 2012-07-03 at 14:52 -0400, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > On tis, 2012-07-03 at 19:35 +0200, Andres Freund wrote:
> >> I wonder if we just should add a format code like %R or something similar as a
> >> replacement for the %X/%X notion.
>
> > Maybe just print it as a single 64-bit value from now on.
>
> That'd be problematic also, because of the lack of standardization of
> the format code for uint64. We could write things like
> "message... " UINT64_FORMAT " ...more message"
> but I wonder how well the translation tools would work with that;
> and anyway it would at least double the translation effort for
> messages containing such things.

The existing uses of INT64_FORMAT and UINT64_FORMAT show how this is
done: You print the value in a temporary buffer and use %s in the final
string. It's not terribly pretty, but it's been done this way forever,
including in xlog code, so there shouldn't be a reason to hesitate about
the use for this particular case.

On 07.07.2012 01:03, Peter Eisentraut wrote:
> On tis, 2012-07-03 at 14:52 -0400, Tom Lane wrote:
>> Peter Eisentraut<peter_e(at)gmx(dot)net> writes:
>>> On tis, 2012-07-03 at 19:35 +0200, Andres Freund wrote:
>>>> I wonder if we just should add a format code like %R or something similar as a
>>>> replacement for the %X/%X notion.
>>
>>> Maybe just print it as a single 64-bit value from now on.
>>
>> That'd be problematic also, because of the lack of standardization of
>> the format code for uint64. We could write things like
>> "message... " UINT64_FORMAT " ...more message"
>> but I wonder how well the translation tools would work with that;
>> and anyway it would at least double the translation effort for
>> messages containing such things.
>
> The existing uses of INT64_FORMAT and UINT64_FORMAT show how this is
> done: You print the value in a temporary buffer and use %s in the final
> string. It's not terribly pretty, but it's been done this way forever,
> including in xlog code, so there shouldn't be a reason to hesitate about
> the use for this particular case.

That's hardly any simpler than what we have now.

On 03.07.2012 21:09, Tom Lane wrote:
> Andres Freund<andres(at)2ndquadrant(dot)com> writes:
>> I wonder if we just should add a format code like %R or something
similar as a
>> replacement for the %X/%X notion.
>
> Only if you can explain how to teach gcc what it means for elog argument
> match checking. %m is a special case in that it matches up with a
> longstanding glibc-ism that gcc knows about. Adding format codes of our
> own invention would be problematic.

One idea would be to use a macro, like this:

#define XLOGRECPTR_FMT_ARGS(recptr) (uint32) ((recptr) >> 32), (uint32)
(recptr)

elog(LOG, "current WAL location is %X/%X", XLOGRECPTR_FMT_ARGS(RecPtr));

One downside is that at first glance, that elog() looks broken, because
the number of arguments don't appear to match the format string.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On tor, 2012-07-12 at 10:13 +0300, Heikki Linnakangas wrote:
> One idea would be to use a macro, like this:
>
> #define XLOGRECPTR_FMT_ARGS(recptr) (uint32) ((recptr) >> 32),
> (uint32)
> (recptr)
>
> elog(LOG, "current WAL location is %X/%X",
> XLOGRECPTR_FMT_ARGS(RecPtr));
>
I would rather get rid of this %X/%X notation. I know we have all grown
to like it, but it's always been a workaround. We're now making the
move to simplify this whole business by saying, the WAL location is an
unsigned 64-bit number -- which everyone can understand -- but then why
is it printed in some funny format?

On Fri, Jul 13, 2012 at 10:34:35PM +0300, Peter Eisentraut wrote:
> On tor, 2012-07-12 at 10:13 +0300, Heikki Linnakangas wrote:
> > One idea would be to use a macro, like this:
> >
> > #define XLOGRECPTR_FMT_ARGS(recptr) (uint32) ((recptr) >> 32),
> > (uint32)
> > (recptr)
> >
> > elog(LOG, "current WAL location is %X/%X",
> > XLOGRECPTR_FMT_ARGS(RecPtr));
> >
> I would rather get rid of this %X/%X notation. I know we have all grown
> to like it, but it's always been a workaround. We're now making the
> move to simplify this whole business by saying, the WAL location is an
> unsigned 64-bit number -- which everyone can understand -- but then why
> is it printed in some funny format?

+1

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> On Fri, Jul 13, 2012 at 10:34:35PM +0300, Peter Eisentraut wrote:
>> I would rather get rid of this %X/%X notation.

> +1

I'm for it if we can find a less messy way of dealing with the
platform-specific-format-code issue. I don't want to be plugging
UINT64_FORMAT into string literals in a pile of places.

Personally I think that a function returning a static string
buffer is probably good enough for this. If there are places
where we need to print more than one XLogRecPtr value in a message,
we could have two of them. (Yeah, it's ugly, but less so than
dealing with platform-specific format codes everywhere.)

regards, tom lane

On Jul 13, 2012, at 2:34 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> I would rather get rid of this %X/%X notation. I know we have all grown
> to like it, but it's always been a workaround. We're now making the
> move to simplify this whole business by saying, the WAL location is an
> unsigned 64-bit number -- which everyone can understand -- but then why
> is it printed in some funny format?

We should take care that whatever format we pick can be easily matched to a WAL file name. So a 64-bit number printed as 16 hex digits would perhaps be OK, but a 64-bit number printed in base 10 would be a large usability regression.

Personally, I'm not convinced we should change anything at all. It's not that easy to visually parse a string of many digits; a little punctuation in the middle is not a bad thing.

...Robert