Lists: | pgsql-hackers |
---|
From: | "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> |
---|---|
To: | <pgsql-hackers(at)postgresql(dot)org>,<m_lists(at)yahoo(dot)it> |
Subject: | Re: Review: Patch: Allow substring/replace() to get/set bit values |
Date: | 2010-01-19 15:00:44 |
Message-ID: | 4B5574BC020000250002E73A@gw.wicourts.gov |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Leonardo F wrote:
>> In the documentation, the get_bit and set_bit methods are added
>> to a list where we state "The following SQL-standard functions
>> work on bit strings as well as character strings"; however they
>> are not SQL-standard functions and are implemented on binary
>> strings, not character strings.
>
> Ok, I didn't change that, but I guess I can fix it.
The items already on that list, and substring function that you
added, *do* fit the description; get_bit and set_bit don't fit that
description, so they don't belong on that list. They must be
described in some other way.
>> There is a peculiar omission from the patch -- it adds support
>> for the overlay function to bit strings but not binary strings.
>> Since the standard drops the bit string as a standard type in the
>> 2003 standard, in favor of boolean type and binary string types,
>> it seems odd to omit binary string support (which is defined in
>> the standard) but include bit string support (which isn't). What
>> the patch adds seems useful, and I don't think the omission
>> should be considered fatal, but it would be nice to see us also
>> cover binary strings if we can.
>
> Mmh, don't know how complicated that would be, but I can give it a
> try: I guess it could be a simple SQL replacement (as for char
> strings and bit strings?)
The standard explicitly defines it as being equivalent to the
substring and concatenation technique, so that should work.
>> Well, there was one thing which is beyond my ken in this regard:
>> I'm not sure that the cases from bits8 to (unsigned char *) are
>> needed or appropriate, although on my machine they don't seem to
>> hurt. Perhaps someone with a better grasp of such issues can
>> comment.
>
> That's some kind of copy&paste from varlena.c byteaGetBit: I don't
> know if they can be avoided.
Ah, but there it's casting the result of VARDATA_ANY. Here you
already have the value in a bits8 variable. I looked it over some
more and I'm pretty confident we can omit those casts in bitsetbit.
> [pointer is zero-based]
>
>> Ok; I found them bizarre too, but I kept it consistent.
>> [int32 used for bit values]
>
> Same as above: I tried to be consistent.
>> [ERRCODE_ARRAY_SUBSCRIPT_ERROR used when query has no array]
>
> Again, same as above: consistency; but I can change it if you want
I'm not suggesting the above require changes; I mentioned those
primarily for the benefit of whoever eventually commits this, to try
to save time chasing down the issues.
>> [questions use of oldByte and newByte local variables]
>
> I agree, you version is cleaner.
Cool. Should look even tidier without those casts. :-)
-Kevin
From: | Leonardo F <m_lists(at)yahoo(dot)it> |
---|---|
To: | Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)gov>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Review: Patch: Allow substring/replace() to get/set bit values |
Date: | 2010-01-20 10:27:49 |
Message-ID: | 685786.18208.qm@web29015.mail.ird.yahoo.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
New version of the patch, let me know if I can fix/change something else.
Leonardo
Attachment | Content-Type | Size |
---|---|---|
getsetbit.patch | application/octet-stream | 12.7 KB |