On Sun, Feb 2, 2014 at 12:07 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-02-02 00:04:41 +0900, Michael Paquier wrote:
>> On Fri, Dec 13, 2013 at 2:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > Andres Freund <andres@2ndquadrant.com> writes:
>> >> On 2013-12-12 11:55:51 -0500, Tom Lane wrote:
>> >>> I'm not, however, terribly thrilled with the suggestions to add implicit
>> >>> casts associated with this type. Implicit casts are generally dangerous.
>> >
>> >> It's a tradeof. Currently we have the following functions returning LSNs
>> >> as text:
>> >> * pg_current_xlog_location
>> >> * pg_current_xlog_insert_location
>> >> * pg_last_xlog_receive_location
>> >> * pg_last_xlog_replay_location
>> >> one view containing LSNs
>> >> * pg_stat_replication
>> >> and the following functions accepting LSNs as textual paramters:
>> >> * pg_xlog_location_diff
>> >> * pg_xlogfile_name
>> >
>> >> The question is how do we deal with backward compatibility when
>> >> introducing a LSN type? There might be some broken code around
>> >> monitoring if we simply replace the type without implicit casts.
>> >
>> > Given the limited usage, how bad would it really be if we simply
>> > made all those take/return the LSN type? As long as the type's
>> > I/O representation looks like the old text format, I suspect
>> > most queries wouldn't notice.
>
> I've asked around inside 2ndq and we could find one single problematic
> query, so it's really not too bad.
>
>> Are there some plans to awaken this patch (including changing the
>> output of the functions of xlogfuncs.c)? This would be useful for the
>> differential backup features I am looking at these days. I imagine
>> that it is too late for 9.4 though...
>
> I think we should definitely go ahead with the patch per-se, we just
> added another system view with lsns in it... I don't have a too strong
> opinion whether to do it in 9.4 or 9.5. It seems fairly low impact to
> me, and it's an old patch, but I personally don't have the tuits to
> refresh the patch right now.
Please find attached a patch implementing lsn as a datatype, based on
the one Robert wrote a couple of years ago. Since XLogRecPtr has been
changed to a simple uint64, this *refresh* has needed some work. In
order to have this data type plugged in more natively with other xlog
system functions, I have added as well PG_RETURN_LSN and PG_GETARG_LSN
to facilitate the interface, making easier code management if
XLogRecPtr or LSN format are changed in the future.
Patch contains regression tests as well as a bit of documentation.
Perhaps this is too late for 9.4, so if there are no objections I'll
simply add this patch to the next commit fest in June for 9.5. Having
the system functions use this data type (pg_start_backup, pg_xlog_*,
create_rep_slot, etc.) does not look to be that difficult as all the
functions in xlogfuncs.c actually use XLogRecPtr before changing it to
text, so it would actually simplify the code. I think I'll simply code
it as I just looking at it now...
Regards,
--
Michael
