Re:Re: Feature: Add reloption support for table access method - Mailing list pgsql-hackers
| From | 吴昊 |
|---|---|
| Subject | Re:Re: Feature: Add reloption support for table access method |
| Date | |
| Msg-id | AEUAoABOB4ndkCmK3s2GBaoV.3.1683696278361.Hmail.wuhao@hashdata.cn Whole thread Raw |
| In response to | Re: Feature: Add reloption support for table access method (Andres Freund <andres@anarazel.de>) |
| List | pgsql-hackers |
> > The rest of this mail is to talk about the first issue. It looks reasonable to add a similar callback in > > struct TableAmRoutine, and parse reloptions by the callback. This patch is in the attachment file. > > Why did you add relkind to the callbacks? The callbacks are specific to a > certain relkind, so I don't think that makes sense.
An implementation of table access method may be used for table/toast/matview, different relkinds
may define different set of reloptions. If they have the same reloption set, just ignore the relkind
parameter.
> I don't think we really need GetTableAmRoutineByAmId() that raises nice > errors etc - as the AM has already been converted to an oid, we shouldn't need > to recheck?
When defining a relation, the function knows only the access method name, not the AM routine struct.
The AmRoutine needs to be looked-up by its access method name or oid. The existing function
calculates AmRoutine by the handler oid, not by am oid. > > +bytea * > > +table_reloptions_am(Oid accessMethodId, Datum reloptions, char relkind, bool validate) > > { > > ... > > > > + /* built-in table access method put here to fetch TAM fast */ > > + case HEAP_TABLE_AM_OID: > > + tam = GetHeapamTableAmRoutine(); > > + break; > > default: > > - /* other relkinds are not supported */ > > - return NULL; > > + tam = GetTableAmRoutineByAmId(accessMethodId); > > + break; > Why do we need this fastpath? This shouldn't be something called at a > meaningful frequency?
OK, it make sense. > > } > > + return table_reloptions(tam->amoptions, reloptions, relkind, validate); > > } > > I'd just pass the tam, instead of an individual function.
It's aligned to index_reloptions, and the function extractRelOptions also uses
an individual function other a pointer to AmRoutine struct.
> Did you mean table instead of relation in the comment?
Yes, the comment doesn't update.
> > Another thing about reloption is that the current API passes a parameter `validate` to tell the parse > > functioin to check and whether to raise an error. It doesn't have enough context when these reloptioins > > are used: > > 1. CREATE TABLE ... WITH(...) > > 2. ALTER TABLE ... SET ... > > 3. ALTER TABLE ... RESET ... > > The reason why the context matters is that some reloptions are disallowed to change after creating > > the table, while some reloptions are allowed. > > What kind of reloption are you thinking of here?
DRAFT: The amoptions in TableAmRoutine may change to
```
bytea *(*amoptions)(Datum reloptions, char relkind, ReloptionContext context);
enum ReloptionContext {
RELOPTION_INIT, // CREATE TABLE ... WITH(...)
RELOPTION_SET, // ALTER TABLE ... SET ...
RELOPTION_RESET, // ALTER TABLE ... RESET ...
RELOPTION_EXTRACT, // build reloptions from pg_class.reloptions
}
```
The callback always validates the reloptions if the context is not RELOPTION_EXTRACT.
If the TAM disallows to update some reloptions, it may throw an error when the context is
one of (RELOPTION_SET, RELOPTION_RESET).
The similar callback `amoptions` in IndexRoutine also applies this change.
BTW, it's hard to find an appropriate header file to define the ReloptionContext, which is
used by index/table AM.
Regards,
Hao Wu
pgsql-hackers by date: