Thread: should we add a XLogRecPtr/LSN SQL type?
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
On Wed, Dec 11, 2013 at 7:41 AM, Andres Freund <andres@2ndquadrant.com> wrote: > 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? I'm pretty sure that this was discussed last year, and I voted for it -- except for the implicit casts part, perhaps -- but more people voted against it, so it died. I still think that was a mistake, but I just work here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 11-12-2013 09:41, Andres Freund wrote: > 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. > While discussing pg_xlog_location_diff function, Robert posted a lsn datatype [1]. At that time we wouldn't go that far (a new datatype) to cover only one function. If your proposal is just validation, I think generic validation functions is the way to follow. However, if you are thinking in adding operators, the lsn datatype should be implemented. > It seems relatively simple to add a proper type, with implicit casts > from text, instead? > Do you want to change the function signatures too? [1] http://www.postgresql.org/message-id/CA+TgmoZRMNN0eVEsD-kxB9e-MvdmwoTi6guuJUvQP_8q2C5Cyg@mail.gmail.com -- Euler Taveira Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte24x7 e Treinamento
<p dir="ltr">Bonus points if you implement a (explicit) cast to and from timestamptz :)<p dir="ltr">-- <br /> greg<div class="gmail_quote">On11 Dec 2013 12:41, "Andres Freund" <<a href="mailto:andres@2ndquadrant.com">andres@2ndquadrant.com</a>>wrote:<br type="attribution" /><blockquote class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Hi,<br /><br /> There's alreadya couple of SQL function dealing with XLogRecPtrs and<br /> the logical replication work will add a couple of more.Currently each<br /> of those funtions taking/returning an LSN does sprintf/scanf to<br /> print/parse the strings.Which both is awkward and potentially<br /> noticeable performancewise.<br /><br /> It seems relatively simple toadd a proper type, with implicit casts<br /> from text, instead?<br /><br /> Greetings,<br /><br /> Andres Freund<br /><br/> --<br /> Andres Freund <a href="http://www.2ndQuadrant.com/" target="_blank">http://www.2ndQuadrant.com/</a><br/> PostgreSQL Development, 24x7 Support, Training & Services<br /><br/><br /> --<br /> Sent via pgsql-hackers mailing list (<a href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br/> To make changes to your subscription:<br/><a href="http://www.postgresql.org/mailpref/pgsql-hackers" target="_blank">http://www.postgresql.org/mailpref/pgsql-hackers</a><br/></blockquote></div>
On 2013-12-11 08:13:18 -0500, Robert Haas wrote: > On Wed, Dec 11, 2013 at 7:41 AM, Andres Freund <andres@2ndquadrant.com> wrote: > > 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? > > I'm pretty sure that this was discussed last year, and I voted for it > but more people > voted against it, so it died. I still think that was a mistake, but I > just work here. Ah. I missed or forgot that discussion. The primary argument seemed to be that we were are only talking about a single function using it. I don't think that really was true back then, but there definitely are some more uses coming up. E.g. the changeset extraction SRF will return the LSN of every extracted change... I don't really buy the argument that it can wholl be replaced by representing LSNs as numeric - those look significantly different enough that that doesn't seem to make much sense to me. Also, they are a pass-by-reference type... > -- except for the implicit casts part, perhaps -- I'd like to convert some existing functions to use the new type, and without added casts that seems too likely to break existing usages. If we just consistently use the new type everywhere, I can't see the usual troubles with the added casts. On 2013-12-11 10:13:51 -0300, Euler Taveira wrote: > On 11-12-2013 09:41, Andres Freund wrote: > While discussing pg_xlog_location_diff function, Robert posted a lsn > datatype [1]. At that time we wouldn't go that far (a new datatype) to > cover only one function. If your proposal is just validation, I think > generic validation functions is the way to follow. However, if you are > thinking in adding operators, the lsn datatype should be implemented. I am mostly thinking of returning many such datums - representing them as text is just pointless overhead. But even if it just were validation - why sprinkle validation over dozens of places if we actually have a typesystem to handle that kind of thing? > > It seems relatively simple to add a proper type, with implicit casts > > from text, instead? > Do you want to change the function signatures too? Yes. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 12 December 2013 12:27, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-12-11 08:13:18 -0500, Robert Haas wrote: >> On Wed, Dec 11, 2013 at 7:41 AM, Andres Freund <andres@2ndquadrant.com> wrote: >> > 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? >> >> I'm pretty sure that this was discussed last year, and I voted for it >> but more people >> voted against it, so it died. I still think that was a mistake, but I >> just work here. > > Ah. I missed or forgot that discussion. Hmm, don't recall that. Just in case I opposed it, its a good idea now. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Dec 12, 2013 at 8:20 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 12 December 2013 12:27, Andres Freund <andres@2ndquadrant.com> wrote: >> On 2013-12-11 08:13:18 -0500, Robert Haas wrote: >>> On Wed, Dec 11, 2013 at 7:41 AM, Andres Freund <andres@2ndquadrant.com> wrote: >>> > 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? >>> >>> I'm pretty sure that this was discussed last year, and I voted for it >>> but more people >>> voted against it, so it died. I still think that was a mistake, but I >>> just work here. >> >> Ah. I missed or forgot that discussion. > > Hmm, don't recall that. Just in case I opposed it, its a good idea now. I am happy to have my old patch resurrected - could become a trend. But someone should probably go back and check who objected and for what reasons. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas escribió: > I am happy to have my old patch resurrected - could become a trend. > But someone should probably go back and check who objected and for > what reasons. Here it is FWIW: http://www.postgresql.org/message-id/flat/CA+TgmoZRMNN0eVEsD-kxB9e-MvdmwoTi6guuJUvQP_8q2C5Cyg@mail.gmail.com -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes: > Robert Haas escribi�: >> I am happy to have my old patch resurrected - could become a trend. >> But someone should probably go back and check who objected and for >> what reasons. > Here it is FWIW: > http://www.postgresql.org/message-id/flat/CA+TgmoZRMNN0eVEsD-kxB9e-MvdmwoTi6guuJUvQP_8q2C5Cyg@mail.gmail.com AFAICS the objections were "why bother with a datatype for just one function". If we've now got multiple use-cases, that loses its force. I'm not, however, terribly thrilled with the suggestions to add implicit casts associated with this type. Implicit casts are generally dangerous. regards, tom lane
Hi, 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. But just leaving all those as-is seems quite unattractive. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
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.
regards, tom lane
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. 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... Regards, -- Michael
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. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
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
Attachment
Michael Paquier <michael.paquier@gmail.com> 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.
regards, tom lane
On Tue, Feb 4, 2014 at 10:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.com> 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
Attachment
Hi, On 2014-02-04 10:23:14 +0900, Michael Paquier wrote: > On Tue, Feb 4, 2014 at 10:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Michael Paquier <michael.paquier@gmail.com> 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. Yes, that sounds sensible. > + /*---------------------------------------------------------- > + * Relational operators for LSNs > + *---------------------------------------------------------*/ Isn't it just operators? They aren't really relational... > *** 302,307 **** extern struct varlena *pg_detoast_datum_packed(struct varlena * datum); > --- 303,309 ---- > #define PG_RETURN_CHAR(x) return CharGetDatum(x) > #define PG_RETURN_BOOL(x) return BoolGetDatum(x) > #define PG_RETURN_OID(x) return ObjectIdGetDatum(x) > + #define PG_RETURN_LSN(x) return LogSeqNumGetDatum(x) > #define PG_RETURN_POINTER(x) return PointerGetDatum(x) > #define PG_RETURN_CSTRING(x) return CStringGetDatum(x) > #define PG_RETURN_NAME(x) return NameGetDatum(x) > *** a/src/include/postgres.h > --- b/src/include/postgres.h > *************** > *** 484,489 **** typedef Datum *DatumPtr; > --- 484,503 ---- > #define ObjectIdGetDatum(X) ((Datum) SET_4_BYTES(X)) > > /* > + * DatumGetLogSeqNum > + * Returns log sequence number of a datum. > + */ > + > + #define DatumGetLogSeqNum(X) ((XLogRecPtr) GET_8_BYTES(X)) I am not a fan of LogSegNum. I think at this point fewer people understand that than LSN. There's also no reason to invent a third term for LSNs. We'd have LSN, XLogRecPtr, and LogSeqNum. > *** a/src/backend/replication/slotfuncs.c > --- b/src/backend/replication/slotfuncs.c > *************** > *** 141,148 **** pg_get_replication_slots(PG_FUNCTION_ARGS) > bool active; > Oid database; > const char *slot_name; > - > - char restart_lsn_s[MAXFNAMELEN]; > int i; > > SpinLockAcquire(&slot->mutex); > --- 141,146 ---- Unrelated change. Looks reasonable on a first look. Thanks! Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Feb 4, 2014 at 6:15 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>
>> + /*----------------------------------------------------------
>> + * Relational operators for LSNs
>> + *---------------------------------------------------------*/
>
> Isn't it just operators? They aren't really relational...
Operators for LSNs?
>> *** 302,307 **** extern struct varlena *pg_detoast_datum_packed(struct varlena * datum);
>> --- 303,309 ----
>> #define PG_RETURN_CHAR(x) return CharGetDatum(x)
>> #define PG_RETURN_BOOL(x) return BoolGetDatum(x)
>> #define PG_RETURN_OID(x) return ObjectIdGetDatum(x)
>> + #define PG_RETURN_LSN(x) return LogSeqNumGetDatum(x)
>> #define PG_RETURN_POINTER(x) return PointerGetDatum(x)
>> #define PG_RETURN_CSTRING(x) return CStringGetDatum(x)
>> #define PG_RETURN_NAME(x) return NameGetDatum(x)
>> *** a/src/include/postgres.h
>> --- b/src/include/postgres.h
>> ***************
>> *** 484,489 **** typedef Datum *DatumPtr;
>> --- 484,503 ----
>> #define ObjectIdGetDatum(X) ((Datum) SET_4_BYTES(X))
>>
>> /*
>> + * DatumGetLogSeqNum
>> + * Returns log sequence number of a datum.
>> + */
>> +
>> + #define DatumGetLogSeqNum(X) ((XLogRecPtr) GET_8_BYTES(X))
>
> I am not a fan of LogSegNum. I think at this point fewer people
> understand that than LSN. There's also no reason to invent a third term
> for LSNs. We'd have LSN, XLogRecPtr, and LogSeqNum.
So let's go with DatumGetLSN and LSNGetDatum instead...
>> *** a/src/backend/replication/slotfuncs.c
>> --- b/src/backend/replication/slotfuncs.c
>> ***************
>> *** 141,148 **** pg_get_replication_slots(PG_FUNCTION_ARGS)
>> bool active;
>> Oid database;
>> const char *slot_name;
>> -
>> - char restart_lsn_s[MAXFNAMELEN];
>> int i;
>>
>> SpinLockAcquire(&slot->mutex);
>> --- 141,146 ----
>
> Unrelated change.
Funnily, the patch attached in my previous mail did not include all
the diffs, it is an error with filterdiff that I use to generate
context diff patches... My original branch includes the following
diffs as well in slotfuncs.c for the second patch:
diff --git a/src/backend/replication/slotfuncs.c
b/src/backend/replication/slotfuncs.c
index 98a860e..68ecdcd 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -141,8 +141,6 @@ pg_get_replication_slots(PG_FUNCTION_ARGS) bool active; Oid
database; const char *slot_name;
-
- char restart_lsn_s[MAXFNAMELEN]; int i;
SpinLockAcquire(&slot->mutex);
@@ -164,9 +162,6 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
memset(nulls, 0, sizeof(nulls));
- snprintf(restart_lsn_s, sizeof(restart_lsn_s), "%X/%X",
- (uint32) (restart_lsn >> 32),
(uint32) restart_lsn);
- i = 0; values[i++] = CStringGetTextDatum(slot_name); if (database ==
InvalidOid)
@@ -180,7 +175,7 @@ pg_get_replication_slots(PG_FUNCTION_ARGS) else nulls[i++] =
true; if (restart_lsn != InvalidTransactionId)
- values[i++] = CStringGetTextDatum(restart_lsn_s);
+ values[i++] = restart_lsn; else nulls[i++] = true;
Anything else?
--
Michael
On 2014-02-04 19:17:51 +0900, Michael Paquier wrote: > On Tue, Feb 4, 2014 at 6:15 PM, Andres Freund <andres@2ndquadrant.com> wrote: > > > >> + /*---------------------------------------------------------- > >> + * Relational operators for LSNs > >> + *---------------------------------------------------------*/ > > > > Isn't it just operators? They aren't really relational... > Operators for LSNs? Fine with me. > >> + #define DatumGetLogSeqNum(X) ((XLogRecPtr) GET_8_BYTES(X)) > > > > I am not a fan of LogSegNum. I think at this point fewer people > > understand that than LSN. There's also no reason to invent a third term > > for LSNs. We'd have LSN, XLogRecPtr, and LogSeqNum. > So let's go with DatumGetLSN and LSNGetDatum instead... Sup. > >> *** a/src/backend/replication/slotfuncs.c > >> --- b/src/backend/replication/slotfuncs.c > >> *************** > >> *** 141,148 **** pg_get_replication_slots(PG_FUNCTION_ARGS) > >> bool active; > >> Oid database; > >> const char *slot_name; > >> - > >> - char restart_lsn_s[MAXFNAMELEN]; > >> int i; > >> > >> SpinLockAcquire(&slot->mutex); > >> --- 141,146 ---- > > > > Unrelated change. > Funnily, the patch attached in my previous mail did not include all > the diffs, it is an error with filterdiff that I use to generate > context diff patches... My original branch includes the following Ah, then it makes more sense. > diffs as well in slotfuncs.c for the second patch: > diff --git a/src/backend/replication/slotfuncs.c > b/src/backend/replication/slotfuncs.c > index 98a860e..68ecdcd 100644 > --- a/src/backend/replication/slotfuncs.c > +++ b/src/backend/replication/slotfuncs.c > @@ -141,8 +141,6 @@ pg_get_replication_slots(PG_FUNCTION_ARGS) > bool active; > Oid database; > const char *slot_name; > - > - char restart_lsn_s[MAXFNAMELEN]; > int i; > > SpinLockAcquire(&slot->mutex); > @@ -164,9 +162,6 @@ pg_get_replication_slots(PG_FUNCTION_ARGS) > > memset(nulls, 0, sizeof(nulls)); > > - snprintf(restart_lsn_s, sizeof(restart_lsn_s), "%X/%X", > - (uint32) (restart_lsn >> 32), > (uint32) restart_lsn); > - > i = 0; > values[i++] = CStringGetTextDatum(slot_name); > if (database == InvalidOid) > @@ -180,7 +175,7 @@ pg_get_replication_slots(PG_FUNCTION_ARGS) > else > nulls[i++] = true; > if (restart_lsn != InvalidTransactionId) > - values[i++] = CStringGetTextDatum(restart_lsn_s); > + values[i++] = restart_lsn; > else > nulls[i++] = true; Isn't that missing a LSNGetDatum()? Also, isn't it lacking the corresponding pg_proc change? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Feb 4, 2014 at 7:22 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-02-04 19:17:51 +0900, Michael Paquier wrote:
>> @@ -180,7 +175,7 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
>> else
>> nulls[i++] = true;
>> if (restart_lsn != InvalidTransactionId)
>> - values[i++] = CStringGetTextDatum(restart_lsn_s);
>> + values[i++] = restart_lsn;
>> else
>> nulls[i++] = true;
>
> Isn't that missing a LSNGetDatum()?
Oops yes. Will fix.
> Also, isn't it lacking the corresponding pg_proc change?
restart_lsn is the 6th argument of pg_get_replication_slots, and the
list of arguments of this function is already changed like that in my
patch:
{25,25,26,16,28,25} => {25,25,26,16,28,3220}
Regards,
--
Michael
On 2014-02-04 21:04:13 +0900, Michael Paquier wrote:
> On Tue, Feb 4, 2014 at 7:22 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2014-02-04 19:17:51 +0900, Michael Paquier wrote:
> >> @@ -180,7 +175,7 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
> >> else
> >> nulls[i++] = true;
> >> if (restart_lsn != InvalidTransactionId)
> >> - values[i++] = CStringGetTextDatum(restart_lsn_s);
> >> + values[i++] = restart_lsn;
> >> else
> >> nulls[i++] = true;
> >
> > Isn't that missing a LSNGetDatum()?
> Oops yes. Will fix.
>
> > Also, isn't it lacking the corresponding pg_proc change?
> restart_lsn is the 6th argument of pg_get_replication_slots, and the
> list of arguments of this function is already changed like that in my
> patch:
> {25,25,26,16,28,25} => {25,25,26,16,28,3220}
> Regards,
Ok.
I think the patch should also adapt pageinspect...
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
Perhaps this type should be called pglsn, since it's an implementation-specific detail and not a universal concept like int, point, or uuid.
On Wed, Feb 5, 2014 at 5:26 AM, Peter Eisentraut <peter_e@gmx.net> 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. It makes sense. I'll update the patches according to that. -- Michael
On Wed, Feb 5, 2014 at 8:59 AM, Michael Paquier <michael.paquier@gmail.com> 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
Attachment
On Wed, Feb 5, 2014 at 9:38 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Wed, Feb 5, 2014 at 8:59 AM, Michael Paquier > <michael.paquier@gmail.com> 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. I just forgot to mention that the 2nd patch does not use context diffs but git diffs because of filterdiff not able to catch all the new content of slotfuncs.c. Regards, -- Michael
On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut <peter_e@gmx.net> 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2/5/14, 1:31 PM, Robert Haas wrote: > On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut <peter_e@gmx.net> 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.
On Thu, Feb 6, 2014 at 3:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > On 2/5/14, 1:31 PM, Robert Haas wrote: >> On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut <peter_e@gmx.net> 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
Attachment
On Thu, Feb 6, 2014 at 11:26 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > Here are updated patches to use pg_lsn instead of pglsn... Should I register this patch somewhere to avoid having it lost in the void? Regards, -- Michael
On Fri, Feb 14, 2014 at 9:32 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Feb 6, 2014 at 11:26 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> Here are updated patches to use pg_lsn instead of pglsn... > Should I register this patch somewhere to avoid having it lost in the void? > Regards, Well, I committed this, but the buildfarm's deeply unhappy with it. Apparently the use of GET_8_BYTES() and SET_8_BYTES() is no good on some platforms... and I'm not sure what to do about that, right off-hand. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2014-02-19 09:24:03 -0500, Robert Haas wrote: > On Fri, Feb 14, 2014 at 9:32 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: > > On Thu, Feb 6, 2014 at 11:26 AM, Michael Paquier > > <michael.paquier@gmail.com> wrote: > >> Here are updated patches to use pg_lsn instead of pglsn... > > Should I register this patch somewhere to avoid having it lost in the void? > > Regards, > > Well, I committed this, but the buildfarm's deeply unhappy with it. > Apparently the use of GET_8_BYTES() and SET_8_BYTES() is no good on > some platforms... and I'm not sure what to do about that, right > off-hand. The relevant bit probably is: pg_lsn.c: In function 'pg_lsn_out': pg_lsn.c:59:2: warning: implicit declaration of function 'GET_8_BYTES' [-Wimplicit-function-declaration] GET_8_BYTES only exists for 64bit systems. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Feb 19, 2014 at 9:30 AM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2014-02-19 09:24:03 -0500, Robert Haas wrote: >> On Fri, Feb 14, 2014 at 9:32 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >> > On Thu, Feb 6, 2014 at 11:26 AM, Michael Paquier >> > <michael.paquier@gmail.com> wrote: >> >> Here are updated patches to use pg_lsn instead of pglsn... >> > Should I register this patch somewhere to avoid having it lost in the void? >> > Regards, >> >> Well, I committed this, but the buildfarm's deeply unhappy with it. >> Apparently the use of GET_8_BYTES() and SET_8_BYTES() is no good on >> some platforms... and I'm not sure what to do about that, right >> off-hand. > > The relevant bit probably is: > > pg_lsn.c: In function 'pg_lsn_out': > pg_lsn.c:59:2: warning: implicit declaration of function 'GET_8_BYTES' [-Wimplicit-function-declaration] > > GET_8_BYTES only exists for 64bit systems. Right, I got that far. So it looks like float8, int8, timestamp, timestamptz, and money all have behavior contingent on USE_FLOAT8_BYVAL, making that symbol a misnomer in every way. But since we've already marched pretty far down that path I suppose we should keep marching. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Feb 19, 2014 at 9:30 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> GET_8_BYTES only exists for 64bit systems.
> Right, I got that far. So it looks like float8, int8, timestamp,
> timestamptz, and money all have behavior contingent on
> USE_FLOAT8_BYVAL, making that symbol a misnomer in every way. But
> since we've already marched pretty far down that path I suppose we
> should keep marching.
You need somebody to help you with getting that working on 32-bit
platforms? Because it needs to get fixed, or reverted, PDQ.
regards, tom lane
On Wed, Feb 19, 2014 at 10:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, Feb 19, 2014 at 9:30 AM, Andres Freund <andres@2ndquadrant.com> wrote: >>> GET_8_BYTES only exists for 64bit systems. > >> Right, I got that far. So it looks like float8, int8, timestamp, >> timestamptz, and money all have behavior contingent on >> USE_FLOAT8_BYVAL, making that symbol a misnomer in every way. But >> since we've already marched pretty far down that path I suppose we >> should keep marching. > > You need somebody to help you with getting that working on 32-bit > platforms? Because it needs to get fixed, or reverted, PDQ. Hopefully the commit I just pushed will fix it. It now works on my machine with and without --disable-float8-byval. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Feb 19, 2014 at 3:11 PM, Robert Haas <robertmhaas@gmail.com> wrote: > Hopefully the commit I just pushed will fix it. It now works on my > machine with and without --disable-float8-byval. It builds and passes here on 32bits -- greg
On Wed, Feb 19, 2014 at 11:03 AM, Greg Stark <stark@mit.edu> wrote: > On Wed, Feb 19, 2014 at 3:11 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> Hopefully the commit I just pushed will fix it. It now works on my >> machine with and without --disable-float8-byval. > > It builds and passes here on 32bits The buildfarm seems happy so far, too. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Feb 6, 2014 at 3:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >> On 2/5/14, 1:31 PM, Robert Haas wrote: >>> On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut <peter_e@gmx.net> 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Feb 20, 2014 at 2:47 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Thu, Feb 6, 2014 at 3:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >>> On 2/5/14, 1:31 PM, Robert Haas wrote: >>>> On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut <peter_e@gmx.net> 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. Regards, -- Michael
On Thu, Feb 20, 2014 at 9:43 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Feb 20, 2014 at 2:47 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> On Thu, Feb 6, 2014 at 3:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >>>> On 2/5/14, 1:31 PM, Robert Haas wrote: >>>>> On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut <peter_e@gmx.net> 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
Attachment
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, -- Michael
Attachment
On Wed, Feb 19, 2014 at 8:06 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Feb 20, 2014 at 9:43 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Thu, Feb 20, 2014 at 2:47 AM, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Wed, Feb 5, 2014 at 9:26 PM, Michael Paquier >>> <michael.paquier@gmail.com> wrote: >>>> On Thu, Feb 6, 2014 at 3:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >>>>> On 2/5/14, 1:31 PM, Robert Haas wrote: >>>>>> On Tue, Feb 4, 2014 at 3:26 PM, Peter Eisentraut <peter_e@gmx.net> 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, committed. But I left out the whitespace change you included. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Feb 19, 2014 at 8:27 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > 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, Committed. Do we want to do anything about pageinspect? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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