Thread

vacuum and row type

Teodor Sigaev <teodor@sigaev.ru> — 2011-06-01T12:23:29Z

Hi!

I found problem while vacuuming with composite type (version 9.0.4). It's not so 
easy to reproduce, but it's clear what happens.

CREATE TYPE mytype AS (p point, r float8);
CREATE TABLE mytable (mt mytype);
-- create opclass fir GiST
CREATE INDEX myidx ON mytable USING gist (mt);

And vacuum fails with message:
ERROR:  could not identify a comparison function for type point

I added an assert to all such error and got a backtrace:
  #0  0x0000000800de8fcc in kill () from /lib/libc.so.7
(gdb) bt
#0  0x0000000800de8fcc in kill () from /lib/libc.so.7
#1  0x0000000800de7dcb in abort () from /lib/libc.so.7
#2  0x00000000007bb05f in ExceptionalCondition (conditionName=Could not find the 
frame base for "ExceptionalCondition".
) at assert.c:57
#3  0x000000000073839a in record_cmp (fcinfo=0x7fffffffcb80) at rowtypes.c:910
#4  0x0000000000739005 in btrecordcmp (fcinfo=0x7fffffffcb80)
     at rowtypes.c:1236
#5  0x00000000007eb63b in myFunctionCall2 (flinfo=0x7fffffffd170,
     arg1=34521714600, arg2=34521722960) at tuplesort.c:2506
#6  0x00000000007eb598 in inlineApplySortFunction (
     sortFunction=0x7fffffffd170, sk_flags=0, datum1=34521714600,
     isNull1=0 '\0', datum2=34521722960, isNull2=0 '\0') at tuplesort.c:2546
#7  0x00000000007eb50a in ApplySortFunction (sortFunction=0x7fffffffd170,
     sortFlags=0, datum1=34521714600, isNull1=0 '\0', datum2=34521722960,
     isNull2=0 '\0') at tuplesort.c:2565
#8  0x000000000055694f in compare_scalars (a=0x809a9f038, b=0x809a9f048,
     arg=0x7fffffffd150) at analyze.c:2702
#9  0x00000000007fd2cc in qsort_arg (a=0x809a9f038, n=611, es=16,
     cmp=0x5568e0 <compare_scalars>, arg=0x7fffffffd150) at qsort_arg.c:129
#10 0x0000000000555bb6 in compute_scalar_stats (stats=0x809a06ca0,
     fetchfunc=0x554920 <ind_fetch_func>, samplerows=611, totalrows=611)
     at analyze.c:2298
#11 0x000000000055279a in compute_index_stats (onerel=0x8011ac828,
     totalrows=611, indexdata=0x8011e10e8, nindexes=1, rows=0x809a0c038,
     numrows=611, col_context=0x8011ceed8) at analyze.c:764
#12 0x0000000000551eb8 in do_analyze_rel (onerel=0x8011ac828,
     vacstmt=0x7fffffffd880, inh=0 '\0') at analyze.c:501
#13 0x0000000000551437 in analyze_rel (relid=16483, vacstmt=0x7fffffffd880,
     bstrategy=0x80117c588) at analyze.c:217
#14 0x00000000005b0b52 in vacuum (vacstmt=0x7fffffffd880, relid=16483,
     do_toast=0 '\0', bstrategy=0x80117c588, for_wraparound=0 '\0',
     isTopLevel=1 '\001') at vacuum.c:246
#15 0x0000000000674f06 in autovacuum_do_vac_analyze (tab=0x80117cf88,
     bstrategy=0x80117c588) at autovacuum.c:2692
#16 0x0000000000674403 in do_autovacuum () at autovacuum.c:2262
....

So, I think, std_typanalyze() does wrong choice between compute_minimal_stats() 
and compute_scalar_stats() because row type has defined comparison function ( 
btrecordcmp() ) but searching of actual set of comparisons functions per row's 
columns occurs too late - when btrecordcmp() is already started.

I don't have in idea how to fix it without massive refactoring. std_typanalyze() 
should be a bit clever to dig possibility of comparison or 
compute_scalar_stats() should switch to compute_minimal_stats() if underlying 
functions fail with such error.

Obviously, workaround is a adding dummy comparison function for points.

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

Re: vacuum and row type

Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-01T22:42:52Z

Teodor Sigaev <teodor@sigaev.ru> writes:
> I found problem while vacuuming with composite type (version 9.0.4). It's not so 
> easy to reproduce, but it's clear what happens.

> CREATE TYPE mytype AS (p point, r float8);
> CREATE TABLE mytable (mt mytype);
> -- create opclass fir GiST
> CREATE INDEX myidx ON mytable USING gist (mt);

> And vacuum fails with message:
> ERROR:  could not identify a comparison function for type point

It's worse than that, actually: you'll get the same failure from ANALYZE
even without the GIST index, as long as there's some data in the column.
And even if you try to make ANALYZE back off to use
compute_minimal_stats, it still fails, because there's no btree equality
for type point either.

We've also seen similar failures in respect to things like the planner
trying to use sorting with unsortable composite types.  So this issue
isn't really specific to ANALYZE.  I'm inclined to think that the most
reasonable fix is to make get_sort_group_operators() and related
functions recursively verify whether the component types can be compared
before they claim that record_eq, array_eq, etc can be used.  So that
would require special cases for composites and arrays in those
functions, but at least we'd not need to hack up all their callers.

			regards, tom lane

Re: vacuum and row type

Teodor Sigaev <teodor@sigaev.ru> — 2011-06-02T14:34:08Z

> isn't really specific to ANALYZE.  I'm inclined to think that the most
> reasonable fix is to make get_sort_group_operators() and related

Hm, patch is in attach but it doesn't solve all problems. Initial bug is still 
here for array of row type, but when I tried to change that with recursive call 
get_sort_group_operators() as it done for row type then 'gmake check' fails 
because lookup_rowtype_tupdesc fails to find anonymous composite type. As I can 
see anonymous composite type are identified by (RECORD_OID, typmod) pair and 
typmod aren't available here. So, my plan was to add typmod to 
get_sort_group_operators() but I have no idea where is typmod value for element 
type.

In runtime problems are solved by using  HeapTupleHeaderGetTypMod() for record / 
element of array.

With modified get_sort_group_operators() for arrays check actually fails for 
query 'select * from search_graph order by path;' at file 
src/test/regress/sql/with.sql. get_sort_group_operators() is called from 
addTargetToSortList() and fails.

It seems to me that anonymous composite type could force us to teach 
vacuum/analyze code to fallback to simpler analyze algorithm.

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

Re: vacuum and row type

Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-02T15:27:46Z

Teodor Sigaev <teodor@sigaev.ru> writes:
>> isn't really specific to ANALYZE.  I'm inclined to think that the most
>> reasonable fix is to make get_sort_group_operators() and related

> Hm, patch is in attach but it doesn't solve all problems. Initial bug is still 
> here for array of row type, but when I tried to change that with recursive call 
> get_sort_group_operators() as it done for row type then 'gmake check' fails 
> because lookup_rowtype_tupdesc fails to find anonymous composite type.

I think we could just let this code assume success for type RECORD.  It
won't affect VACUUM/ANALYZE, since there are (for reasons that should
now be obvious) no table or index columns of anonymous composite types.

What I was thinking last night is that it'd be smart to move all this
logic into the typcache, instead of repeating all the work each time we
make the check.  I'm not convinced that get_sort_group_operators is the
only place we'd have to change if we keep the logic outside the
typcache, anyway.  (I seem to recall there is someplace in the planner
that has a similar check.)

			regards, tom lane

Re: vacuum and row type

Teodor Sigaev <teodor@sigaev.ru> — 2011-06-02T16:24:54Z

> I think we could just let this code assume success for type RECORD.  It
> won't affect VACUUM/ANALYZE, since there are (for reasons that should
> now be obvious) no table or index columns of anonymous composite types.
Of course, it's impossible to store anonymous composite type anywhere, but
we still have possibility to use it in ORDER BY at least, following query works 
on HEAD but fails with patch:

select ROW(1, n) as r from generate_series(1,5) as n order by r;


> What I was thinking last night is that it'd be smart to move all this
> logic into the typcache, instead of repeating all the work each time we

Agree

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

Re: vacuum and row type

Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-02T16:39:54Z

Teodor Sigaev <teodor@sigaev.ru> writes:
>> I think we could just let this code assume success for type RECORD.  It
>> won't affect VACUUM/ANALYZE, since there are (for reasons that should
>> now be obvious) no table or index columns of anonymous composite types.

> Of course, it's impossible to store anonymous composite type anywhere, but
> we still have possibility to use it in ORDER BY at least, following query works 
> on HEAD but fails with patch:

> select ROW(1, n) as r from generate_series(1,5) as n order by r;

Right, so for type RECORD we should let the parser assume that
comparisons will work.  If the anonymous composite type isn't actually
sortable, it'll fail at runtime, same as now.

			regards, tom lane

Re: vacuum and row type

Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-03T02:21:13Z

I wrote:
> What I was thinking last night is that it'd be smart to move all this
> logic into the typcache, instead of repeating all the work each time we
> make the check.

Attached is a proposed patch that does it that way.  I haven't finished
poking around to see if there are any other places besides
get_sort_group_operators where there is now-redundant code, but this
does pass regression tests.

Although this is arguably a bug fix, I'm not sure whether to back-patch
it.  Given the lack of field complaints, it may not be worth taking any
risk for.  Thoughts?

			regards, tom lane