Re: should we add a XLogRecPtr/LSN SQL type?
Re: should we add a XLogRecPtr/LSN SQL type?
От:
Michael Paquier <michael.paquier@gmail.com>
Дата:
On Thu, Feb 20, 2014 at 9:43 AM, Michael Paquier wrote: > On Thu, Feb 20, 2014 at 2:47 AM, Robert Haas wrote: >> On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier >> wrote: >>> On Thu, Feb 6, 2014 at 3:48 AM, Peter Eisentraut wrote: >>>> On 2/5/14, 1:31 PM, Robert Haas wrote: >>>>> On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut wrote: >>>>>> Perhaps this type should be called pglsn, since it's an >>>>>> implementation-specific detail and not a universal concept like int, >>>>>> point, or uuid. >>>>> >>>>> If we're going to do that, I suggest pg_lsn rather than pglsn. We >>>>> already have pg_node_tree, so using underscores for separation would >>>>> be more consistent. >>>> >>>> Yes, that's a good precedent in multiple ways. >>> Here are updated patches to use pg_lsn instead of pglsn... >> >> OK, so I think this stuff is all committed now, with assorted changes. >> Thanks for your work on this. > Thanks! > Oops, it looks like I am coming after the battle (time difference does > not help). I'll be more careful to test such patches on 32b platforms > as well in the future. After re-reading the code, I found two incorrect comments in the new regression tests. Patch fixing them is attached. Thanks, -- Michael
Re: should we add a XLogRecPtr/LSN SQL type?
От:
Michael Paquier <michael.paquier@gmail.com>
Дата:
On Thu, Feb 20, 2014 at 8:55 AM, Andres Freund wrote: > On 2014-02-19 12:47:40 -0500, Robert Haas wrote: >> On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier >> wrote: >> >> Yes, that's a good precedent in multiple ways. >> > Here are updated patches to use pg_lsn instead of pglsn... >> >> OK, so I think this stuff is all committed now, with assorted changes. >> Thanks for your work on this. > > cool, thanks you two. > > I wonder if pg_stat_replication shouldn't be updated to use it as well? > SELECT * FROM pg_attribute WHERE attname ~ '(location|lsn)'; only shows > that as names that are possible candidates for conversion. I was sure to have forgotten some views or functions in the previous patch... Please find attached a patch making pg_stat_replication use pg_lsn instead of the existing text fields. Regards, -- Michael
Re: should we add a XLogRecPtr/LSN SQL type?
От:
Michael Paquier <michael.paquier@gmail.com>
Дата:
On Sun, Feb 2, 2014 at 12:07 AM, Andres Freund wrote: > On 2014-02-02 00:04:41 +0900, Michael Paquier wrote: >> On Fri, Dec 13, 2013 at 2:47 AM, Tom Lane wrote: >> > Andres Freund 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
Re: should we add a XLogRecPtr/LSN SQL type?
От:
Michael Paquier <michael.paquier@gmail.com>
Дата:
On Tue, Feb 4, 2014 at 10:10 AM, Tom Lane wrote: > Michael Paquier writes: >> Please find attached a patch implementing lsn as a datatype, based on >> the one Robert wrote a couple of years ago. > >> 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. > > I may have lost count, but aren't a bunch of the affected functions new > in 9.4? If so, there's a good argument to be made that we should get > this in now, rather than waiting and having an API change for those > functions in 9.5. Cool... I have created a second patch that updates all the system functions to use the new lsn datatype introduced in the 1st patch (pg_start|stop_backup, replication_slot stuff, xlog diff things, etc.) and it is attached. This cleans up quite a bit of code in xlogfuncs.c because we do not need anymore the LSN <-> cstring transformations! I am also attaching a v2 of the first patch, I noticed that lsn_in introduced in the first patch was using some error messages not consistent with the ones of validate_xlog_location:xlogfuncs.c. Note as well that validate_xlog_location is removed in the 2nd patch where all the system functions are swicthed to the new datatype. Regards, -- Michael
Re: should we add a XLogRecPtr/LSN SQL type?
От:
Michael Paquier <michael.paquier@gmail.com>
Дата:
On Tue, Feb 25, 2014 at 12:41 AM, Robert Haas <robertmhaas@gmail.com> wrote:
-- On Wed, Feb 19, 2014 at 8:27 PM, Michael Paquier<michael.paquier@gmail.com> wrote:Committed. Do we want to do anything about pageinspect?
> On Thu, Feb 20, 2014 at 8:55 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> On 2014-02-19 12:47:40 -0500, Robert Haas wrote:
>>> On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier
>>> <michael.paquier@gmail.com> wrote:
>>> >> Yes, that's a good precedent in multiple ways.
>>> > Here are updated patches to use pg_lsn instead of pglsn...
>>>
>>> OK, so I think this stuff is all committed now, with assorted changes.
>>> Thanks for your work on this.
>>
>> cool, thanks you two.
>>
>> I wonder if pg_stat_replication shouldn't be updated to use it as well?
>> SELECT * FROM pg_attribute WHERE attname ~ '(location|lsn)'; only shows
>> that as names that are possible candidates for conversion.
> I was sure to have forgotten some views or functions in the previous
> patch... Please find attached a patch making pg_stat_replication use
> pg_lsn instead of the existing text fields.
> Regards,
Thanks. We're dealing with that on another thread, I'll send an updated patch there.
Michael
Re: should we add a XLogRecPtr/LSN SQL type?
От:
Michael Paquier <michael.paquier@gmail.com>
Дата:
On Wed, Feb 5, 2014 at 8:59 AM, Michael Paquier wrote: > I'll update the patches according to that. Here are the updated patches with the following changes (according to previous comments): - Datatype is renamed to pglsn, documentation, file names, regressions and APIs are updated as well. - The DatumGet* and *GetDatum APIs are renamed with PGLSN (Should be PgLsn? But that's a detail) - pg_create_physical_replication_slot uses PGLSNGetDatum for its 6th argument For pageinspect, only page_header is impacted and I think that this should be a separated patch as it makes necessary to dump it to 1.2. I can write it later once the core parts are decided. Thanks, -- Michael
Re: should we add a XLogRecPtr/LSN SQL type?
От:
Michael Paquier <michael.paquier@gmail.com>
Дата:
On Thu, Feb 6, 2014 at 3:48 AM, Peter Eisentraut wrote: > On 2/5/14, 1:31 PM, Robert Haas wrote: >> On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut wrote: >>> Perhaps this type should be called pglsn, since it's an >>> implementation-specific detail and not a universal concept like int, >>> point, or uuid. >> >> If we're going to do that, I suggest pg_lsn rather than pglsn. We >> already have pg_node_tree, so using underscores for separation would >> be more consistent. > > Yes, that's a good precedent in multiple ways. Here are updated patches to use pg_lsn instead of pglsn... -- Michael
Re: should we add a XLogRecPtr/LSN SQL type?
От:
Greg Stark <stark@mit.edu>
Дата:
Bonus points if you implement a (explicit) cast to and from timestamptz :)
--
greg
On 11 Dec 2013 12:41, "Andres Freund" <andres@2ndquadrant.com> wrote:
Hi,
There's already a couple of SQL function dealing with XLogRecPtrs and
the logical replication work will add a couple of more. Currently each
of those funtions taking/returning an LSN does sprintf/scanf to
print/parse the strings. Which both is awkward and potentially
noticeable performancewise.
It seems relatively simple to add a proper type, with implicit casts
from text, instead?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
