Re: Packed short varlenas, what next?

Lists: pgsql-hackers
From: Gregory Stark <stark(at)enterprisedb(dot)com>
To: PostgreSQL Development <pgsql-hackers(at)postgresql(dot)org>
Subject: Packed short varlenas, what next?
Date: 2007-02-27 13:46:18
Message-ID: 87fy8rhfx1.fsf@stark.xeocode.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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/varlena/patch-varvarlena-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


From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: pgsql-hackers(at)postgresql(dot)org
Cc: Gregory Stark <stark(at)enterprisedb(dot)com>
Subject: Re: Packed short varlenas, what next?
Date: 2007-02-27 14:18:09
Message-ID: 200702271518.10335.peter_e@gmx.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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/


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers(at)postgresql(dot)org, Gregory Stark <stark(at)enterprisedb(dot)com>
Subject: Re: Packed short varlenas, what next?
Date: 2007-02-27 14:50:22
Message-ID: 17492.1172587822@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Peter Eisentraut <peter_e(at)gmx(dot)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


From: Gregory Stark <gsstark(at)mit(dot)edu>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Packed short varlenas, what next?
Date: 2007-02-27 15:16:46
Message-ID: 87zm6zeild.fsf@stark.xeocode.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Peter Eisentraut <peter_e(at)gmx(dot)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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Gregory Stark <gsstark(at)mit(dot)edu>
Cc: Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Packed short varlenas, what next?
Date: 2007-02-27 15:52:05
Message-ID: 20432.1172591525@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Gregory Stark <gsstark(at)mit(dot)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


From: Josh Berkus <josh(at)agliodbs(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Packed short varlenas, what next?
Date: 2007-02-27 17:34:12
Message-ID: 200702270934.12215.josh@agliodbs.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Gregory Stark <stark(at)enterprisedb(dot)com>
Cc: PostgreSQL Development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Packed short varlenas, what next?
Date: 2007-02-27 18:13:06
Message-ID: 22024.1172599986@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Gregory Stark <stark(at)enterprisedb(dot)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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Gregory Stark <stark(at)enterprisedb(dot)com>
Cc: PostgreSQL Development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Packed short varlenas, what next?
Date: 2007-02-28 05:44:57
Message-ID: 25571.1172641497@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

I wrote:
> Gregory Stark <stark(at)enterprisedb(dot)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


From: Gregory Stark <stark(at)enterprisedb(dot)com>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "PostgreSQL Development" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Packed short varlenas, what next?
Date: 2007-02-28 11:22:09
Message-ID: 877iu2plwe.fsf@stark.xeocode.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Gregory Stark <stark(at)enterprisedb(dot)com>
Cc: "PostgreSQL Development" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Packed short varlenas, what next?
Date: 2007-02-28 16:15:30
Message-ID: 4382.1172679330@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Gregory Stark <stark(at)enterprisedb(dot)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


From: Gregory Stark <stark(at)enterprisedb(dot)com>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "PostgreSQL Development" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Packed short varlenas, what next?
Date: 2007-03-01 04:33:40
Message-ID: 87649ltwez.fsf@stark.xeocode.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> 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.

Thanks, I was getting lost in the gist stuff.

I've disabled packed varlenas for user-defined data types and find tsearch2
and _int still fail. tsearch2 requires the small patch attached. _int seems to
be unrelated.

To make them work with packed varlenas would require ensuring that they're
always detoasted instead of using GETARG_POINTER. I'll look at that tomorrow.
Er, today.

(It would be nice if we made it possible to define gist indexable data types
without so much copy/pasted code though. These data types are all just
defining some basic operations and then copy/pasting the same algorithms to
implement picksplit and the other index support functions in terms of those
basic operations.)

Index: contrib/tsearch2/ts_cfg.c
===================================================================
RCS file: /home/stark/src/REPOSITORY/pgsql/contrib/tsearch2/ts_cfg.c,v
retrieving revision 1.22
diff -c -r1.22 ts_cfg.c
*** contrib/tsearch2/ts_cfg.c 27 Feb 2007 23:48:06 -0000 1.22
--- contrib/tsearch2/ts_cfg.c 1 Mar 2007 04:19:02 -0000
***************
*** 62,70 ****
ts_error(ERROR, "SPI_execp return %d", stat);
if (SPI_processed > 0)
{
! prsname = (text *) DatumGetPointer(
! SPI_getbinval(SPI_tuptable->vals[0], SPI_tuptable->tupdesc, 1, &isnull)
! );
oldcontext = MemoryContextSwitchTo(TopMemoryContext);
prsname = ptextdup(prsname);
MemoryContextSwitchTo(oldcontext);
--- 62,68 ----
ts_error(ERROR, "SPI_execp return %d", stat);
if (SPI_processed > 0)
{
! prsname = DatumGetTextP(SPI_getbinval(SPI_tuptable->vals[0], SPI_tuptable->tupdesc, 1, &isnull));
oldcontext = MemoryContextSwitchTo(TopMemoryContext);
prsname = ptextdup(prsname);
MemoryContextSwitchTo(oldcontext);

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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Gregory Stark <stark(at)enterprisedb(dot)com>
Cc: "PostgreSQL Development" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Packed short varlenas, what next?
Date: 2007-03-01 05:50:58
Message-ID: 15866.1172728258@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> I've disabled packed varlenas for user-defined data types and find tsearch2
> and _int still fail. tsearch2 requires the small patch attached. _int seems to
> be unrelated.

As of when? I committed fixes earlier tonight that seem to handle the
case of VARSIZE-is-ntohl.

The patch you suggest is orthogonal to what I did; it looks like it
might be right, but regression passes without it, so what was your test
case that led you to it?

regards, tom lane


From: "Denis Lussier" <denisl(at)enterprisedb(dot)org>
To: "Gregory Stark" <stark(at)enterprisedb(dot)com>
Cc: "PostgreSQL Development" <pgsql-hackers(at)postgresql(dot)org>, "Affan Salman" <affan(at)enterprisedb(dot)com>, "Peter Yarrow" <peter(dot)yarrow(at)enterprisedb(dot)com>, "Bruce Momjian" <bruce(at)momjian(dot)us>
Subject: Re: Packed short varlenas, what next?
Date: 2007-03-01 06:08:46
Message-ID: 64db53a00702282208o2701e17dj19327b6b69550c6f@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

BenchmarkSQL is open source, but, I don't think anyone has published version
5.2 yet on pgFoundry. Amongst other goodies, version 5.2 allows for the
running of Java based tpcC and/or tpcB like benchmarks from the command line
or the cutsie gui. We've also added consistency checks to the end of the
tpc-c run (which mysql always fails).

Affan is coming out shortly with version 5.3, he'll publish by early next
week.

As a side note and different topic: The tpcB is an example of a verrryy
disk intensive little transaction. Running it with a mocked up version
COMMIT NOWAIT produces a 4x performance increase for disk setups where fsync
is not "free".

--Luss
http://www.enterprisedb.com

On 2/27/07, Gregory Stark <stark(at)enterprisedb(dot)com> wrote:
>
>
>
> 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.
>
>


From: Gregory Stark <stark(at)enterprisedb(dot)com>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "PostgreSQL Development" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Packed short varlenas, what next?
Date: 2007-03-01 09:15:04
Message-ID: 87zm6xs4tj.fsf@stark.xeocode.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Gregory Stark <stark(at)enterprisedb(dot)com> writes:
>> I've disabled packed varlenas for user-defined data types and find tsearch2
>> and _int still fail. tsearch2 requires the small patch attached. _int seems to
>> be unrelated.
>
> As of when? I committed fixes earlier tonight that seem to handle the
> case of VARSIZE-is-ntohl.
>
> The patch you suggest is orthogonal to what I did; it looks like it
> might be right, but regression passes without it, so what was your test
> case that led you to it?

I'm running the regression tests with the full packed varlena changes except
that I've modified it not to pack user defined data types (typid >
FirstNormalObjectId). So all varlenas need to go through detoast_datum if they
come out of a heatuple even if (especially if) they used to be too small to be
toasted.

In fact I think the line I posted is actually a bug anyways. I'm unclear what
the text field it's fetching represents and maybe it's usually small, but it
looks like there's nothing stopping it from being large enough to be toasted
in theory.

To get tsearch et al to work with packed varlena headers without disabling
them for user defined data types will require a lot more detoast_datum calls
throughout the gist data types (or defining proper GETARG macros for them).

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