Re: Feature: Add reloption support for table access method

Hi,

On 2023-05-05 16:44:39 +0800, 吴昊 wrote:
> When I wrote an extension to implement a new storage by table access method. I found some issues
> that the existing code has strong assumptions for heap tables now. Here are 3 issues that I currently have:
>
>
> 1. Index access method has a callback to handle reloptions, but table access method hasn't. We can't
> add storage-specific parameters by `WITH` clause when creating a table.

Makes sense to add that.

> 2. Existing code strongly assumes that the data file of a table structures by a serial physical files named
> in a hard coded rule: <relfilenode>[.<segno>]. It may only fit for heap like tables. A new storage may have its
> owner structure on how the data files are organized. The problem happens when dropping a table.

I agree that it's not great, but I don't think it's particularly easy to fix
(because things like transactional DDL require fairly tight integration). Most
of the time it should be possible can also work around the limitations though.

> 3. The existing code also assumes that the data file consists of a series of fix-sized block. It may not
> be desired by other storage. Is there any suggestions on this situation?

That's a requirement of using the buffer manager, but if you don't want to
rely on that, you can use a different pattern. There's some limitations
(format of TIDs, most prominently), but you should be able to deal with that.

I don't think it would make sense to support other block sizes in the buffer
manager.

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

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?

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

> }
> + return table_reloptions(tam->amoptions, reloptions, relkind, validate);
> }

I'd just pass the tam, instead of an individual function.

> @@ -866,6 +866,11 @@ typedef struct TableAmRoutine
> struct SampleScanState *scanstate,
> TupleTableSlot *slot);
>
> + /*
> + * This callback is used to parse reloptions for relation/matview/toast.
> + */
> + bytea *(*amoptions)(Datum reloptions, char relkind, bool validate);
> +
> } TableAmRoutine;

Did you mean table instead of relation in the comment?

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

Greetings,

Andres Freund