Re:Re: Feature: Add reloption support for table access method

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