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