At 2014-09-19 10:44:48 +0200, and...@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];
> > +} 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
not a 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 --stats and
--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
Sent via pgsql-hackers mailing list (firstname.lastname@example.org)
To make changes to your subscription: