On Wed, Feb 25, 2015 at 8:10 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2015-02-24 20:51:42 +0200, Heikki Linnakangas wrote:
>> On 02/20/2015 05:21 PM, Andres Freund wrote:
>> >There's one bit that I'm not so sure about though: To avoid duplication
>> >I've added Parse(Commit/Abort)Record(), but unfortunately that has to be
>> >available both in front and backend code - so it's currently living in
>> >xactdesc.c. I think we can live with that, but it's certainly not
>> >pretty.
>>
>> Yeah, that's ugly. Why does frontend code need that? The old format
>> isn't exactly trivial for frontend code to decode either.
>
> pg_xlogdump outputs subxacts and such; I don't forsee other
> usages. Sure, we could copy the code around, but I think that's worse
> than having it in xactdesc.c. Needs a comment explaining why it's there
> if I haven't added one already.
FWIW, I think they would live better in frontend code for client applications.
That's a nice patch. +1 for merging them. Here are a couple of comments:
+/* Parse the WAL format of a xact abort into a easier to understand format. */
+void
+ParseCommitRecord(uint8 info, xl_xact_commit *xlrec,
xl_xact_parsed_commit *parsed)
I think that you mean here of "an xact commit", not abort.
+ * Emit, but don't insert, a abort record.
s/a abort/an abort/
XactEmitAbortRecord has some problems with tabs replaced by 4 spaces
at a couple of places.
+/* free opcode 0x70 */
+
+#define XLOG_XACT_OPMASK 0x70
There is a contradiction here, 0x70 is not free.
Regards,
--
Michael
