Re: [Patch] Temporary tables that do not bloat pg_catalog (a.k.a fast temp tables) - Mailing list pgsql-hackers
| From | Dmitry Dolgov |
|---|---|
| Subject | Re: [Patch] Temporary tables that do not bloat pg_catalog (a.k.a fast temp tables) |
| Date | |
| Msg-id | CA+q6zcVLO7VoHHpRn9ExkiWksF6TwJ-rabqgwRJgC2_ci8_UEQ@mail.gmail.com Whole thread Raw |
| In response to | Re: [Patch] Temporary tables that do not bloat pg_catalog (a.k.a fast temp tables) (Robert Haas <robertmhaas@gmail.com>) |
| Responses |
Re: [Patch] Temporary tables that do not bloat pg_catalog
(a.k.a fast temp tables)
(Robert Haas <robertmhaas@gmail.com>)
|
| List | pgsql-hackers |
Hi
I tried to dig into this patch (which seems pretty interesting) to help bring
it in good shape. Here are few random notes, I hope they can be helpful:
> I think we actually need a real API here.
Definitely, there are plenty places in the new code with the same pattern:
* figure out if it's an action related to the fast temporary tables based on
`ItemPointer`/relation OID/etc
* if it is, add some extra logic or skip something in original implementation
* if it is, add some extra logic or skip something in original implementation
in `heapam.c`, `indexam.c`, `xact.c`, `dependency.c`. I believe it's possible to
make it more generic (although it will contain almost the same logic), e.g.
separate regular and fasttable implementations completely, and decide which one
we should choose in that particular case.
Btw, I'm wondering about the `heap_*` functions in `heapam.c` - some of them are
wrapped and never used directly, although they're contains in
`INTERFACE_ROUTINES` (like `simple_heap_delete` -> `heap_delete`), some of them
aren't. It looks like inconsistency in terms of function names, probably it
should be unified?
> What is needed is an overview of the approach, so that the reviewers can read
> that first,
I feel lack of such information even in new version of this patch (but, I'm
probably a noob in these matters). I noted that the `fasttab.c` file contains some
general overview, but in terms of "what are we doing", and "why are we doing
this". I think general overview of "how are we doing this" also may be useful.
And there are several slightly obvious commentaries like:
```
+ /* Is it a virtual TID? */
+ if (IsFasttabItemPointer(tid))
```
but I believe an introduction of a new API (see the previous note) will solve
this eventually.
> Why do we need the new relpersistence value? ISTM we could easily got with
> just RELPERSISTENCE_TEMP, which would got right away of many chances as the
> steps are exactly the same.
I agree, it looks like `RELPERSISTENCE_FAST_TEMP` hasn't any influence on the
code.
> For example, suppose I create a fast temporary table and then I create a
> functional index on the fast temporary table that uses some SQL function
> defined in pg_proc.
Just to clarify, did you mean something like this?
```
create fast temp table fasttab(x int, s text);
create or replace function test_function_for_index(t text) returns text as $$
begin
return lower(t);
end;
$$ language plpgsql immutable;
create index fasttab_s_idx on fasttab (test_function_for_index(s));
drop function test_function_for_index(t text);
```
As far as I understand dependencies should protect in case of fasttable too,
because everything is visible as in regular case, isn't it?
And finally one more question, why items of `FasttabIndexMethodsTable[]` like
this one:
```
+ /* 2187, non-unique */
+ {InheritsParentIndexId, 1,
+ {Anum_pg_inherits_inhparent, 0, 0},
+ {CompareOid, CompareInvalid, CompareInvalid}
+ },
```
have this commentary before them? I assume it's an id and an extra information,
and I'm concerned that they can easily become outdated inside commentary block.
pgsql-hackers by date: