Re: vacuum and row type

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(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

Teodor Sigaev <teodor(at)sigaev(dot)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

> 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(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

Teodor Sigaev <teodor(at)sigaev(dot)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

> 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(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

Teodor Sigaev <teodor(at)sigaev(dot)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

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