Re: [HACKERS] Add protransform for numeric, varbit, and temporal types
On Wed, Feb 08, 2012 at 09:37:01AM -0500, Robert Haas wrote:
On Tue, Feb 7, 2012 at 12:43 PM, Robert Haas robertmh...@gmail.com wrote:
I've committed the numeric and varbit patches and will look at the
temporal one next.
Committed, after changing the OIDs so they don't conflict.
Here's one more case for you to ponder:
rhaas=# create table noah (i interval day);
CREATE TABLE
rhaas=# alter table noah alter column i set data type interval second(3);
DEBUG: rewriting table noah
ALTER TABLE
Do we really need a rewrite in that case? The code acts like the
interval range and precision are separate beasts, but is that really
true?
The code has a thinko; a given interval typmod ultimately implies a single
point from which we truncate rightward. The precision only matters if the
range covers SECOND. Thanks; the attached patch improves this.
*** a/src/backend/utils/adt/timestamp.c
--- b/src/backend/utils/adt/timestamp.c
***
*** 958,963 interval_transform(PG_FUNCTION_ARGS)
--- 958,964
int new_range = INTERVAL_RANGE(new_typmod);
int new_precis =
INTERVAL_PRECISION(new_typmod);
int new_range_fls;
+ int old_range_fls;
if (old_typmod == -1)
{
***
*** 974,985 interval_transform(PG_FUNCTION_ARGS)
* Temporally-smaller fields occupy higher positions in the
range
* bitmap. Since only the temporally-smallest bit matters for
length
* coercion purposes, we compare the last-set bits in the
ranges.
*/
new_range_fls = fls(new_range);
if (new_typmod == -1 ||
((new_range_fls = SECOND ||
! new_range_fls = fls(old_range))
!(new_precis = MAX_INTERVAL_PRECISION ||
new_precis = old_precis)))
ret = relabel_to_typmod(source, new_typmod);
}
--- 975,990
* Temporally-smaller fields occupy higher positions in the
range
* bitmap. Since only the temporally-smallest bit matters for
length
* coercion purposes, we compare the last-set bits in the
ranges.
+* Precision, which is to say, sub-second precision, only
affects
+* ranges that include SECOND.
*/
new_range_fls = fls(new_range);
+ old_range_fls = fls(old_range);
if (new_typmod == -1 ||
((new_range_fls = SECOND ||
! new_range_fls = old_range_fls)
!(old_range_fls SECOND ||
! new_precis = MAX_INTERVAL_PRECISION ||
new_precis = old_precis)))
ret = relabel_to_typmod(source, new_typmod);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add protransform for numeric, varbit, and temporal types
On Thu, Feb 9, 2012 at 7:18 AM, Noah Misch n...@leadboat.com wrote: The code has a thinko; a given interval typmod ultimately implies a single point from which we truncate rightward. The precision only matters if the range covers SECOND. Thanks; the attached patch improves this. Thanks, committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add protransform for numeric, varbit, and temporal types
On Tue, Feb 7, 2012 at 12:43 PM, Robert Haas robertmh...@gmail.com wrote: I've committed the numeric and varbit patches and will look at the temporal one next. Committed, after changing the OIDs so they don't conflict. Here's one more case for you to ponder: rhaas=# create table noah (i interval day); CREATE TABLE rhaas=# alter table noah alter column i set data type interval second(3); DEBUG: rewriting table noah ALTER TABLE Do we really need a rewrite in that case? The code acts like the interval range and precision are separate beasts, but is that really true? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add protransform for numeric, varbit, and temporal types
On Tue, Jan 3, 2012 at 10:31 AM, Robert Haas robertmh...@gmail.com wrote:
On Sat, Dec 31, 2011 at 7:36 PM, Noah Misch n...@leadboat.com wrote:
Building on commit 8f9fe6edce358f7904e0db119416b4d1080a83aa, this adds
protransform functions to the length coercions for numeric, varbit,
timestamp,
timestamptz, time, timetz and interval. This mostly serves to make more
ALTER
TABLE ALTER TYPE operations avoid a rewrite, including numeric(10,2) -
numeric(12,2), varbit(4) - varbit(8) and timestamptz(2) - timestamptz(4).
The rules for varbit are exactly the same as for varchar. Numeric is
slightly
more complex:
* Flatten calls to our length coercion function that solely represent
* increases in allowable precision. Scale changes mutate every datum, so
* they are unoptimizable. Some values, e.g. 1E-1001, can only fit into an
* unconstrained numeric, so a change from an unconstrained numeric to any
* constrained numeric is also unoptimizable.
time{,stamp}{,tz} are similar to varchar for these purposes, except that, for
example, plain timestamptz is equivalent to timestamptz(6). interval has
a vastly different typmod format, but the principles applicable to length
coercion remain the same.
Under --disable-integer-datetimes, I'm not positive that timestamp_scale() is
always a no-op when one would logically expect as much. Does there exist a
timestamp such that v::timestamp(2) differs from v:timestamp(2)::timestamp(4)
due to floating point rounding? Even if so, I'm fairly comfortable calling
it
a feature rather than a bug to avoid perturbing values that way.
After these patches, the only core length coercion casts not having
protransform functions are those for bpchar and bit. For those, we could
only optimize trivial cases of no length change. I'm not planning to do so.
This is cool stuff. I will plan to review this once the CF starts.
I've committed the numeric and varbit patches and will look at the
temporal one next. I did notice one odd thing, though: sometimes we
seem to get a rebuild on the toast index for no obvious reason:
rhaas=# set client_min_messages=debug1;
SET
rhaas=# create table foo (a serial primary key, b varbit);
NOTICE: CREATE TABLE will create implicit sequence foo_a_seq for
serial column foo.a
DEBUG: building index pg_toast_16430_index on table pg_toast_16430
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
foo_pkey for table foo
DEBUG: building index foo_pkey on table foo
CREATE TABLE
rhaas=# alter table foo alter column b set data type varbit(4);
DEBUG: rewriting table foo
DEBUG: building index foo_pkey on table foo
ALTER TABLE
rhaas=# alter table foo alter column b set data type varbit;
DEBUG: building index pg_toast_16430_index on table pg_toast_16430
ALTER TABLE
Strangely, it doesn't happen if I add another column to the table:
rhaas=# set client_min_messages=debug1;
SET
rhaas=# create table foo (a serial primary key, b varbit, c varbit);
NOTICE: CREATE TABLE will create implicit sequence foo_a_seq for
serial column foo.a
DEBUG: building index pg_toast_16481_index on table pg_toast_16481
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
foo_pkey for table foo
DEBUG: building index foo_pkey on table foo
CREATE TABLE
rhaas=# alter table foo alter column b set data type varbit(4);
DEBUG: building index pg_toast_16490_index on table pg_toast_16490
DEBUG: rewriting table foo
DEBUG: building index foo_pkey on table foo
ALTER TABLE
rhaas=# alter table foo alter column b set data type varbit;
ALTER TABLE
There may not be any particular harm in a useless rebuild of the TOAST
table index (wasted effort excepted), but my lack of understanding of
why this is happening causes me to fear that there's a bug, not so
much in these patches as in the core alter table code that is enabling
skipping table and index rebuilds.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add protransform for numeric, varbit, and temporal types
On Tue, Feb 07, 2012 at 12:43:11PM -0500, Robert Haas wrote: I've committed the numeric and varbit patches and will look at the temporal one next. Thanks. The comment you added to numeric_transform() has a few typos, constained - constrained and nodes - notes. I did notice one odd thing, though: sometimes we seem to get a rebuild on the toast index for no obvious reason: rhaas=# set client_min_messages=debug1; SET rhaas=# create table foo (a serial primary key, b varbit); NOTICE: CREATE TABLE will create implicit sequence foo_a_seq for serial column foo.a DEBUG: building index pg_toast_16430_index on table pg_toast_16430 NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index foo_pkey for table foo DEBUG: building index foo_pkey on table foo CREATE TABLE rhaas=# alter table foo alter column b set data type varbit(4); DEBUG: rewriting table foo DEBUG: building index foo_pkey on table foo ALTER TABLE When we rebuild the table at this point, it has a small maximum tuple size. Therefore, we omit the toast table entirely. rhaas=# alter table foo alter column b set data type varbit; DEBUG: building index pg_toast_16430_index on table pg_toast_16430 ALTER TABLE This command makes the tuple size unbounded again, so we create a toast table. Strangely, it doesn't happen if I add another column to the table: rhaas=# set client_min_messages=debug1; SET rhaas=# create table foo (a serial primary key, b varbit, c varbit); With the extra unconstrained varbit column, the tuple size is continuously unbounded and the table has a toast table at all stages. So, the difference in behavior is correct, albeit unintuitive. NOTICE: CREATE TABLE will create implicit sequence foo_a_seq for serial column foo.a DEBUG: building index pg_toast_16481_index on table pg_toast_16481 NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index foo_pkey for table foo DEBUG: building index foo_pkey on table foo CREATE TABLE rhaas=# alter table foo alter column b set data type varbit(4); DEBUG: building index pg_toast_16490_index on table pg_toast_16490 DEBUG: rewriting table foo DEBUG: building index foo_pkey on table foo ALTER TABLE rhaas=# alter table foo alter column b set data type varbit; ALTER TABLE -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add protransform for numeric, varbit, and temporal types
On Tue, Feb 7, 2012 at 8:45 PM, Noah Misch n...@leadboat.com wrote: On Tue, Feb 07, 2012 at 12:43:11PM -0500, Robert Haas wrote: I've committed the numeric and varbit patches and will look at the temporal one next. Thanks. The comment you added to numeric_transform() has a few typos, constained - constrained and nodes - notes. Yuck, that's pretty bad. Thanks, fixed (I hope). When we rebuild the table at this point, it has a small maximum tuple size. Therefore, we omit the toast table entirely. Ah, OK. The debug messages might be more clear if they mentioned whether or not we were omitting the TOAST table, I suppose. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add protransform for numeric, varbit, and temporal types
On Sat, Dec 31, 2011 at 7:36 PM, Noah Misch n...@leadboat.com wrote:
Building on commit 8f9fe6edce358f7904e0db119416b4d1080a83aa, this adds
protransform functions to the length coercions for numeric, varbit, timestamp,
timestamptz, time, timetz and interval. This mostly serves to make more ALTER
TABLE ALTER TYPE operations avoid a rewrite, including numeric(10,2) -
numeric(12,2), varbit(4) - varbit(8) and timestamptz(2) - timestamptz(4).
The rules for varbit are exactly the same as for varchar. Numeric is slightly
more complex:
* Flatten calls to our length coercion function that solely represent
* increases in allowable precision. Scale changes mutate every datum, so
* they are unoptimizable. Some values, e.g. 1E-1001, can only fit into an
* unconstrained numeric, so a change from an unconstrained numeric to any
* constrained numeric is also unoptimizable.
time{,stamp}{,tz} are similar to varchar for these purposes, except that, for
example, plain timestamptz is equivalent to timestamptz(6). interval has
a vastly different typmod format, but the principles applicable to length
coercion remain the same.
Under --disable-integer-datetimes, I'm not positive that timestamp_scale() is
always a no-op when one would logically expect as much. Does there exist a
timestamp such that v::timestamp(2) differs from v:timestamp(2)::timestamp(4)
due to floating point rounding? Even if so, I'm fairly comfortable calling it
a feature rather than a bug to avoid perturbing values that way.
After these patches, the only core length coercion casts not having
protransform functions are those for bpchar and bit. For those, we could
only optimize trivial cases of no length change. I'm not planning to do so.
This is cool stuff. I will plan to review this once the CF starts.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Add protransform for numeric, varbit, and temporal types
Building on commit 8f9fe6edce358f7904e0db119416b4d1080a83aa, this adds
protransform functions to the length coercions for numeric, varbit, timestamp,
timestamptz, time, timetz and interval. This mostly serves to make more ALTER
TABLE ALTER TYPE operations avoid a rewrite, including numeric(10,2) -
numeric(12,2), varbit(4) - varbit(8) and timestamptz(2) - timestamptz(4).
The rules for varbit are exactly the same as for varchar. Numeric is slightly
more complex:
* Flatten calls to our length coercion function that solely represent
* increases in allowable precision. Scale changes mutate every datum, so
* they are unoptimizable. Some values, e.g. 1E-1001, can only fit into an
* unconstrained numeric, so a change from an unconstrained numeric to any
* constrained numeric is also unoptimizable.
time{,stamp}{,tz} are similar to varchar for these purposes, except that, for
example, plain timestamptz is equivalent to timestamptz(6). interval has
a vastly different typmod format, but the principles applicable to length
coercion remain the same.
Under --disable-integer-datetimes, I'm not positive that timestamp_scale() is
always a no-op when one would logically expect as much. Does there exist a
timestamp such that v::timestamp(2) differs from v:timestamp(2)::timestamp(4)
due to floating point rounding? Even if so, I'm fairly comfortable calling it
a feature rather than a bug to avoid perturbing values that way.
After these patches, the only core length coercion casts not having
protransform functions are those for bpchar and bit. For those, we could
only optimize trivial cases of no length change. I'm not planning to do so.
Thanks,
nm
diff --git a/src/backend/utils/adt/varbit.c b/src/backend/utils/adt/varbit.c
index 3fa8117..e5a03b9 100644
*** a/src/backend/utils/adt/varbit.c
--- b/src/backend/utils/adt/varbit.c
***
*** 18,23
--- 18,25
#include access/htup.h
#include libpq/pqformat.h
+ #include nodes/nodeFuncs.h
+ #include parser/parse_clause.h
#include utils/array.h
#include utils/varbit.h
***
*** 646,651 varbit_send(PG_FUNCTION_ARGS)
--- 648,686
}
/*
+ * varbit_transform()
+ * Flatten calls to our length coercion function that leave the new maximum
+ * length = the previous maximum length. We ignore the isExplicit argument,
+ * which only affects truncation.
+ */
+ Datum
+ varbit_transform(PG_FUNCTION_ARGS)
+ {
+ FuncExpr *expr = (FuncExpr *) PG_GETARG_POINTER(0);
+ Node *typmod;
+ Node *ret = NULL;
+
+ if (!IsA(expr, FuncExpr))
+ PG_RETURN_POINTER(ret);
+
+ Assert(list_length(expr-args) == 3);
+ typmod = lsecond(expr-args);
+
+ if (IsA(typmod, Const))
+ {
+ Node *source = linitial(expr-args);
+ int32 new_typmod = DatumGetInt32(((Const *)
typmod)-constvalue);
+ int32 old_max = exprTypmod(source);
+ int32 new_max = new_typmod;
+
+ if (new_max = 0 || (old_max = 0 old_max = new_max))
+ ret = relabel_to_typmod(source, new_typmod);
+ }
+
+ PG_RETURN_POINTER(ret);
+ }
+
+ /*
* varbit()
* Converts a varbit() type to a specific internal length.
* len is the maximum bitlength specified in the column definition.
diff --git a/src/include/catalog/pg_pindex c893c3a..2a71f82 100644
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
***
*** 2005,2011 DESCR(convert bitstring to int4);
DATA(insert OID = 1685 ( bitPGNSP PGUID 12 1 0 0 0 f f f
t f i 3 0 1560 1560 23 16 _null_ _null_ _null_ _null_ bit _null_ _null_
_null_ ));
DESCR(adjust bit() to typmod length);
! DATA(insert OID = 1687 ( varbit PGNSP PGUID 12 1 0 0 0 f f f
t f i 3 0 1562 1562 23 16 _null_ _null_ _null_ _null_ varbit _null_ _null_
_null_ ));
DESCR(adjust varbit() to typmod length);
DATA(insert OID = 1698 ( position PGNSP PGUID 12 1 0 0 0 f f f
t f i 2 0 23 1560 1560 _null_ _null_ _null_ _null_ bitposition _null_ _null_
_null_ ));
--- 2005,2013
DATA(insert OID = 1685 ( bitPGNSP PGUID 12 1 0 0 0 f f f
t f i 3 0 1560 1560 23 16 _null_ _null_ _null_ _null_ bit _null_ _null_
_null_ ));
DESCR(adjust bit() to typmod length);
! DATA(insert OID = 3145 ( varbit_transform PGNSP PGUID 12 1 0 0 0 f f f t f i
1 0 2281 2281 _null_ _null_ _null_ _null_ varbit_transform _null_ _null_
_null_ ));
! DESCR(transform a varbit length coercion);
! DATA(insert OID = 1687 ( varbit PGNSP PGUID 12 1 0 0 3145 f
f f t f i 3 0 1562 1562 23 16 _null_ _null_ _null_ _null_ varbit _null_
_null_ _null_ ));
DESCR(adjust varbit() to typmod length);
DATA(insert OID = 1698 ( position PGNSP PGUID 12 1 0 0 0 f f f
t f i 2 0 23 1560 1560 _null_ _null_ _null_ _null_ bitposition _null_ _null_
_null_ ));
