Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

Hi,

On 2022-03-22 16:32:24 +0530, Bharath Rupireddy wrote:
> On Tue, Mar 22, 2022 at 12:20 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > Something like attached
> > > v6-0001-Deduplicate-checkpoint-restartpoint-complete-mess.patch?
> >
> > Mostly. I don't see a reason for the use of the stringinfo. And I think
> > LogCheckpointStart() should be dealt with similarly.
> >
> > I'd just make it a restartpoint ? _("restartpoint") : _("checkpoint") or such
> > in the argument? Then translators don't need to translate the two messages
> > separately.
>
> Do you mean like this?
> ereport(LOG,
> /* translator: the placeholders show checkpoint options */
> (errmsg("%s starting:%s%s%s%s%s%s%s%s",
> restartpoint ? _("restartpoint") : _("checkpoint"),
> (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "",
> (flags & CHECKPOINT_END_OF_RECOVERY) ? "
> end-of-recovery" : "",
> (flags & CHECKPOINT_IMMEDIATE) ? " immediate" : "",
> (flags & CHECKPOINT_FORCE) ? " force" : "",
> (flags & CHECKPOINT_WAIT) ? " wait" : "",
> (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "",
> (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "",
> (flags & CHECKPOINT_FLUSH_ALL) ? " flush-all" : "")));

Yes.

> Are we allowed to use _("foo") with errmsg? Isn't "foo" going to get
> translated twice that way?

The format string gets translated, arguments don't. Choosing the German
translation (don't speak other languages :(), you can for example see:

#. translator: the placeholders show checkpoint options
#: access/transam/xlog.c:8692
#, c-format
msgid "checkpoint starting:%s%s%s%s%s%s%s%s"
msgstr "Checkpoint beginnt:%s%s%s%s%s%s%s%s"

you can see that the first words are translated, but the arguments are the
same. So there shouldn't be be any double translation.

> > Or we could just not translate restartpoint/checkpoint - after all we don't
> > translate the flags in LogCheckpointStart() either. But on balance I'd lean
> > towards the above.
>
> I'm not sure if it's correct to translate some parts of the message
> and leave others. What exactly is the reason for this?

Yea, it's a bit odd. I'd still separate combining the messages (and thus
translating "restartpoint" and "checkpoint" separately) from translating
flags.

> I think the reason in this case might be that some flag names with hyphens
> and spaces before words may not have the right/matching words in all
> languages. What happens if we choose to translate/not translate the entire
> message?

If individual words aren't translated the "original" word would be used.

> How about just doing this for LogCheckpointStart?
> StringInfoData logmsg;
> initStringInfo(&logmsg);
> appendStringInfo(&logmsg,
> /* translator: the placeholders show checkpoint options */
> restartpoint ? _("restartpoint
> starting:%s%s%s%s%s%s%s%s") :
> _("checkpoint starting:%s%s%s%s%s%s%s%s"),
> (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "",
> (flags & CHECKPOINT_END_OF_RECOVERY) ? "
> end-of-recovery" : "",
> (flags & CHECKPOINT_IMMEDIATE) ? " immediate" : "",
> (flags & CHECKPOINT_FORCE) ? " force" : "",
> (flags & CHECKPOINT_WAIT) ? " wait" : "",
> (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "",
> (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "",
> (flags & CHECKPOINT_FLUSH_ALL) ? " flush-all" : "");
> ereport(LOG, errmsg_internal("%s", logmsg.data));
> pfree(logmsg.data);

Why do you insist on using stringinfos? It doesn't add *anything* here over
just a plain ereport()? And is considerably more verbose?

> > Both seem still very long. I still am doubtful this level of detail is
> > appropriate. Seems more like a thing for a tracepoint or such. How about just
> > printing the time for the logical decoding operations in aggregate, without
> > breaking down into files, adding LSNs etc?
>
> The distinction that the patch makes right now is for snapshot and
> rewrite mapping files and it makes sense to have them separately.

-1. The line also needs to be readable...

Greetings,

Andres Freund