Thread: should xlog_outdesc modify its argument?

The function

  static void xlog_outdesc(StringInfo buf, XLogReaderState *record);

in src/backend/access/transam/xlog.c is called by XLogInsertRecord,
and after returning a string describing an XLogRecord, it clears the
state data in its XLogReaderState argument.  That mixes the read-only
semantics of "give me a string that describes this argument" and the
read-write semantics of "clear out the value in this argument".  That
seems ok, except that xlog_outdesc is also called by the function

  static void rm_redo_error_callback(const void *arg);

also in xlog.c, which appears to only want the string description of the
argument, and not the side effect of clearing out the value.  Now,
perhaps under exceptional conditions it won't matter one way or the
other if the xlog record gets modified; perhaps it's going to be discarded
anyway.  I didn't look far enough to find out.  But this is the only function
of all the (void)(void *arg) callback functions in the entire postgresql
source tree that modifies its argument.  I discovered this while trying
to convert all the callback function signatures to (void)(const void *),
and this was the only monkey-wrench in the works.

Is this a bug?  Do I just have to live with the unfortunate lack of
orthogonality in the callback mechanisms?  At the very least, there
should perhaps be some documentation about how all this is intended
to work.

Thanks, please find my work-in-progress attempt and constify-ing
these functions attached.

Mark Dilger

On 09/28/2016 02:35 AM, Mark Dilger wrote:
> The function
>
>   static void xlog_outdesc(StringInfo buf, XLogReaderState *record);
>
> in src/backend/access/transam/xlog.c is called by XLogInsertRecord,
> and after returning a string describing an XLogRecord, it clears the
> state data in its XLogReaderState argument.  That mixes the read-only
> semantics of "give me a string that describes this argument" and the
> read-write semantics of "clear out the value in this argument".

I don't see where the "clears the state data" is happening. Can you 
elaborate?

- Heikki

> On Sep 27, 2016, at 11:25 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 09/28/2016 02:35 AM, Mark Dilger wrote:
>> The function
>>
>>  static void xlog_outdesc(StringInfo buf, XLogReaderState *record);
>>
>> in src/backend/access/transam/xlog.c is called by XLogInsertRecord,
>> and after returning a string describing an XLogRecord, it clears the
>> state data in its XLogReaderState argument.  That mixes the read-only
>> semantics of "give me a string that describes this argument" and the
>> read-write semantics of "clear out the value in this argument".
>
> I don't see where the "clears the state data" is happening. Can you elaborate?

My apologies.  At the bottom of the function, it calls through the function pointer
RmgrTable[rmid].rm_desc(buf, record);

which is set up to call various *_desc functions.  I must have chased through
those function pointers incorrectly, as I can't find the problem now that I am
reviewing all those functions.

Sorry for the noise,

Mark Dilger