SEO

I'm really curious to know how people feel about the varlena patch. In
particular I know these issues may elicit comment:

1) Do we really need a special case for little-endian machines? I think it
would be trivial to add but having two code paths may be annoying to
maintain. The flip side is it would make it easier to read varlena headers
in gdb which I found kind of annoying with them in network byte order.

2) How do people feel about the way I inlined most of the VARATT_IS_SHORT
cases in heaptuple.c. I tried at first to hide that all in the att_align
and att_addlength macros but a) it would never be possible to hide most of
it and b) it would require a few more redundant tests.

3) How do people feel about not allowing an escape hatch for new types and
explicitly exempting int2vector and oidvector. The alternatives are either
a) adding a new column to pg_type and pg_attribute and setting that on
catalog attributes that are accessed through GETSTRUCT (as the first
varlena in the table) and also setting it on oidvector and int2vector
because they don't call pg_detoast_datum(). Or b) fixing int2vector and
oidvector to pass through pg_detoast_datum and fix all the accesses to the
first int2vector/oidvector in every catalog table to use fastgetattr
instead. or c) keep things as they are now.

4) Should I start hitting the more heavily trod codepaths in text.c and
numeric.c to avoid detoasting short varlenas? The macro api is not quite
complete enough for this yet so it may make sense to tackle at least one
code site before merging it to be sure we have a workable api for data
types that want to avoid unnecessary detoasting.

The latest patch is at

http://community.enterprisedb.com/va...na-12.patch.gz

I've been doing some benchmarking, I see a 9.7% space saving on the
Benchmark-SQL 5.2 schema which translates into about a 8% performance gain.
The DBT2 benchmarks show a smaller 5.3% space saving because we had already
done a lot more optimizing of the schema.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Gregory Stark wrote:
> I'm really curious to know how people feel about the varlena patch.

As I has mentioned earlier, I'm missing a plan to allow 8-byte varlena
sizes.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Peter Eisentraut <peter_e@gmx.net> writes:
> As I has mentioned earlier, I'm missing a plan to allow 8-byte varlena
> sizes.

I don't think it's entirely fair to expect this patch to solve that
problem. In the first place, that is not what the patch's goal is,
but merely tangentially related to the same code. In the second place,
I don't see any way we could possibly do that without wide-ranging code
changes; to take just one point, much of the code that works with
varlenas uses "int" or "int32" variables to compute sizes. So it would
certainly expand the scope of the patch quite a lot to try to put that
in place, and it's mighty late in the devel cycle to be thinking about
that sort of thing.

For the moment I think it should be enough to expect that the patch
allow for more than one format of TOAST pointer, so that if we ever did
try to support 8-byte varlenas, there'd be a way to represent them
on-disk. Some of the alternatives that we discussed last year used up
all of the "prefix space" and wouldn't have allowed expansion in this
particular direction.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Tom Lane <tgl@sss.pgh.pa.us> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

> Peter Eisentraut <peter_e@gmx.net> writes:
> > As I has mentioned earlier, I'm missing a plan to allow 8-byte varlena
> > sizes.

Hm, change VARHDRSZ to 8 and change all the varlena data types to have an
int64 leading field? I suppose it could be done, and it would give us more
bits to play with in the codespace since then we could limit 4-byte headers to
128M or something. But yes, there are tons of places in the code that
currently do arithmetic on sizes using integers -- and often signed integers
at that.

But that's a change to what a *detoasted* datum looks like. My patch mainly
changes what a *toasted* datum looks like. (Admittedly after making more data
fall in that category than previously.) The only change to a detoasted datum
is that the size is stored in network byte order.

> For the moment I think it should be enough to expect that the patch
> allow for more than one format of TOAST pointer, so that if we ever did
> try to support 8-byte varlenas, there'd be a way to represent them
> on-disk. Some of the alternatives that we discussed last year used up
> all of the "prefix space" and wouldn't have allowed expansion in this
> particular direction.

Ah yes, I had intended to include the bit-pattern choice in the list as well.

There are two issues there:

1) The lack of 2-byte patterns which is quite annoying as really *any* on-disk
datum would fit in a 2-byte header varlena. However it became quite tricky
to convert things to 2-byte headers, especially for compressed data, it
would have made for a much bigger patch to tuptoaster.c and pg_lzcompress.
And I became convinced that it was best to get the most important gain
first, saving 2 bytes on wider tuples is less important than 3-6 bytes on
narrow tuples.

2) The choice of encoding for toast pointers. Note that currently they don't
actually save *any* space due to the alignment requirements of the OIDs.
which seems kind of silly but I didn't see any reasonable way around that.
The flip side is that gives us 24 bits to play with if we want to have
different types of external pointers or more meta-information about the
toasted data.

One of the details here is that I didn't store the compressed bit anywhere
for external toast pointers. I just made the macro compare the rawsize and
extsize. If that strikes anyone as evil we could take a byte out of those 3
padding bytes for flags and store a compressed flag there.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com


---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Gregory Stark <gsstark@mit.edu> writes:
> 2) The choice of encoding for toast pointers. Note that currently they don't
> actually save *any* space due to the alignment requirements of the OIDs.
> which seems kind of silly but I didn't see any reasonable way around that.

I was expecting that we'd store them as unaligned and memcpy a toast
pointer into a suitably-aligned local variable any time we wanted to
look at its contents. Detoasting is expensive enough that that's not
going to add any noticeable percentage time-overhead, and not having to
align toast pointers should be a pretty good percentage space-saving,
seeing that they're only 20-some bytes anyway.

> One of the details here is that I didn't store the compressed bit anywhere
> for external toast pointers. I just made the macro compare the rawsize and
> extsize. If that strikes anyone as evil we could take a byte out of those 3
> padding bytes for flags and store a compressed flag there.

I believe this is OK since the toast code doesn't compress unless space
is actually saved. You should put a note in the code that that behavior
is now necessary for correctness, not just a performance tweak.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Greg,

> I'm really curious to know how people feel about the varlena patch. In
> particular I know these issues may elicit comment:

Haven't tested yet. Will let you know when I do.

--
Josh Berkus
PostgreSQL @ Sun
San Francisco

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

Gregory Stark <stark@enterprisedb.com> writes:
> I'm really curious to know how people feel about the varlena patch.

One thing I think we could do immediately is apply the change to replace
"VARATT_SIZEP(x) = len" with "SET_VARSIZE(x, len)" --- that would
considerably reduce the size of the patch and allow people to focus on
the important changes instead of underbrush. Barring objection I'll go
ahead and do that today.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

I wrote:
> Gregory Stark <stark@enterprisedb.com> writes:
>> I'm really curious to know how people feel about the varlena patch.

> One thing I think we could do immediately is apply the change to replace
> "VARATT_SIZEP(x) = len" with "SET_VARSIZE(x, len)" --- that would
> considerably reduce the size of the patch and allow people to focus on
> the important changes instead of underbrush. Barring objection I'll go
> ahead and do that today.

I've committed this, but in testing with a hack that does ntohl() in the
VARSIZE macro and vice-versa in SET_VARSIZE, I find that core passes
regression but several contrib modules do not. It looks like the
contrib modules were depending on various random structs being
compatible with varlena, while not exposing that dependence in ways that
either of us caught :-(.

I'll work on cleaning up the remaining mess tomorrow, but I think that
we may need to think twice about whether it's OK to expect that every
datatype with typlen = -1 will be compatible with the New Rules. I'm
back to wondering if maybe only types with typalign 'c' should get
caught up in the changes.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

> I've committed this, but in testing with a hack that does ntohl() in the
> VARSIZE macro and vice-versa in SET_VARSIZE, I find that core passes
> regression but several contrib modules do not. It looks like the
> contrib modules were depending on various random structs being
> compatible with varlena, while not exposing that dependence in ways that
> either of us caught :-(.

I just noticed that last night myself. In particular the GIST modules seems to
be a major problem. they define dozens of new objects, many of which are just
passing around C data structures internally but some of which are objects
which get stored in the database. I have no idea which are which and which
ones are varlenas.

Worse, it uses PG_GETARG_POINTER() and explicitly calls PG_DETOAST_DATUM() in
the few places it assumes finding toasted data is possible. That's even harder
to track down.

I can send up a patch for the data types I fixed last night.

> I'll work on cleaning up the remaining mess tomorrow, but I think that
> we may need to think twice about whether it's OK to expect that every
> datatype with typlen = -1 will be compatible with the New Rules. I'm
> back to wondering if maybe only types with typalign 'c' should get
> caught up in the changes.


I don't think we can key off typalign='c'. That would entail changing varlenas
to typalign 'c' which would throw off other consumers of the typalign which
expect it to be the alignment of the detoasted datum. Moreover I still align
them when they have the full 4-byte header by using the typalign.

I think we would want to introduce a new column, or maybe a new attlen value,
or a new typalign value.

I was thinking about that though and it's not so simple. It's easy enough not
to convert to short varlena for data types that don't assert that they support
the packed format. That's not a problem. That takes care of data types which
don't call pg_detoast_datum().

But not storing the varlena header in network byte order sometimes would be
quite tricky. There are a great many places that call VARSIZE that don't look
at the attalign or even have it handy.

If we made it a new attlen value we could have two different macros, but that
will be another quite large patch. It would mean hitting all those datatypes
all over again to change every instance of VARSIZE into NEWVARSIZE or
something like that. Plus all the sites in the core that call VARSIZE would
need to check attlen and call the right one.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Gregory Stark <stark@enterprisedb.com> writes:
> I just noticed that last night myself. In particular the GIST modules seems to
> be a major problem. they define dozens of new objects, many of which are just
> passing around C data structures internally but some of which are objects
> which get stored in the database. I have no idea which are which and which
> ones are varlenas.

FWIW, when I went to bed last night I had hstore and intarray working,
but was still fooling with ltree. Didn't get to the others yet.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate