Re: pg_xlogdump --stats

At 2014-09-19 10:44:48 +0200, andres@2ndquadrant.com wrote:
>
> >  # snprintfs that use %lld, %qd, or %I64d as the format.  If none of these
> > -# work, fall back to our own snprintf emulation (which we know uses %lld).
> > +# works, fall back to our own snprintf emulation (which we know uses %lld).
> 
> spurious independent change?

It was part of the original patch, but I guess Heikki didn't commit it,
so it was left over in the rebase.

> Also I actually think the original version is correct?

It is not. I suspect it will begin to sound wrong to you if you replace
"none" with "not one" in the sentence. But I don't care enough to argue
about it. It's a very common error.

> > +    Stats        record_stats[RM_NEXT_ID][16];
> > +} XLogDumpStats;
> 
> I dislike the literal 16 here and someplace later. A define for the max
> number of records would make it clearer.

OK, will change.

> Perhaps we should move these kind of checks outside?

OK, will change.

> a) Whoever introduced the notion of rec_len vs tot_len in regards to
>    including/excluding SizeOfXLogRecord ...

(It wasn't me, honest!)

> b) I'm not against it, but I wonder if the best way to add the
>    SizeOfXLogRecord to the record size. It's just as much part of the
>    FPI. And this means that the record length will be > 0 even if all
>    the record data has been removed due to the FPI.

I'm not sure I understand what you are proposing here.

> What was the reason you moved away from --stats=record/rmgr? I think
> we possibly will add further ones, so that seems more extensible?

It was because I wanted --stats to default to "=rmgr", so I tried to
make the argument optional, but getopt in Windows didn't like that.
Here's an excerpt from the earlier discussion:
   > 3. Some compilation error in windows   > .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2065:
'optional_argument': undeclared identifier   > .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2099: initializer is
nota constant   >   > optional_argument should be added to getopt_long.h file for windows.
 
   Hmm. I have no idea what to do about this. I did notice when I wrote the   code that nothing else used
optional_argument,but I didn't realise that   it wouldn't work on Windows.
 
   It may be that the best thing to do would be to avoid using   optional_argument altogether, and have separate
--statsand   --stats-per-record options. Thoughts?
 

I have no objection to doing it differently if someone tells me how to
make Windows happy (preferably without making me unhappy).

> It's trivial to separate in this case, but I'd much rather have
> patches like this rm_identity stuff split up in the future.

Sorry. I'd split it up that way in the past, but forgot to do it again
in this round. Will do when I resend with the changes above.

> That means the returned value from heap_identity() is only valid until
> the next call. That at the very least needs to be written down
> explicitly somewhere.

Where? In the comment in xlog_internal.h, perhaps?

> That's already in there afaics:

Will fix (rebase cruft).

> Given that you've removed the UNKNOWNs from the rm_descs, this really
> should add it here.

You would prefer to see HEAP/UNKNOWN rather than HEAP/32 for an
unidentifiable xl_info?

-- Abhijit