Обсуждение: rm_desc signature

As part of the WAL-format changing patch I've been working on, I changed
the signature of the rm_desc function from:

void (*rm_desc) (StringInfo buf, uint8 xl_info, char *rec);
void (*rm_desc) (StringInfo buf, XLogRecord *record);

The WAL-format patch needed that because it added more functions/macros
for working with XLogRecords, also used by rm_desc routines, but it
seems like a sensible change anyway. IMHO it was always a bit strange
that rm_desc was passed the info field and record payload separately.

So I propose to do that change as a separate commit. Per attached. This
has no functional changes, it's just refactoring.

Any objections?

- Heikki

On 2014-06-13 14:37:33 +0300, Heikki Linnakangas wrote:
> As part of the WAL-format changing patch I've been working on, I changed the
> signature of the rm_desc function from:
> 
> void (*rm_desc) (StringInfo buf, uint8 xl_info, char *rec);
> void (*rm_desc) (StringInfo buf, XLogRecord *record);
> 
> The WAL-format patch needed that because it added more functions/macros for
> working with XLogRecords, also used by rm_desc routines, but it seems like a
> sensible change anyway. IMHO it was always a bit strange that rm_desc was
> passed the info field and record payload separately.

+1. I've found this annoying in the past.

Greetings,

Andres Freund

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

On Fri, Jun 13, 2014 at 8:39 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-06-13 14:37:33 +0300, Heikki Linnakangas wrote:
>> As part of the WAL-format changing patch I've been working on, I changed the
>> signature of the rm_desc function from:
>>
>> void (*rm_desc) (StringInfo buf, uint8 xl_info, char *rec);
>> void (*rm_desc) (StringInfo buf, XLogRecord *record);
>>
>> The WAL-format patch needed that because it added more functions/macros for
>> working with XLogRecords, also used by rm_desc routines, but it seems like a
>> sensible change anyway. IMHO it was always a bit strange that rm_desc was
>> passed the info field and record payload separately.

+1, too.

-/* #define WAL_DEBUG */
+#define WAL_DEBUG

ISTM you just forgot to exclude this change from the patch.

Regards,

-- 
Fujii Masao

At 2014-06-13 13:39:58 +0200, andres@2ndquadrant.com wrote:
>
> > void (*rm_desc) (StringInfo buf, uint8 xl_info, char *rec);
> > void (*rm_desc) (StringInfo buf, XLogRecord *record);
> > 
> > […]
> 
> +1. I've found this annoying in the past.

I like it too. I was just moving some code from pg_xlogdump into another
(new) rm_desc-like callback, and passing in the XLogRecord makes much
more sense to me.

-- Abhijit

As part of the WAL-format changing patch I've been working on, I changed the signature of the rm_desc function from:

void (*rm_desc) (StringInfo buf, uint8 xl_info, char *rec);
void (*rm_desc) (StringInfo buf, XLogRecord *record);

The WAL-format patch needed that because it added more functions/macros for working with XLogRecords, also used by rm_desc routines, but it seems like a sensible change anyway. IMHO it was always a bit strange that rm_desc was passed the info field and record payload separately.

So I propose to do that change as a separate commit. Per attached. This has no functional changes, it's just refactoring.

Any objections?

On 06/17/2014 04:19 AM, Jeff Janes wrote:
> This commit, or a related one, changed the default (i.e. commented out)
> nature of:
>
> #define WAL_DEBUG

Oops. Fixed, thanks!

- Heikki