Re: Status of the table access method work

Hi!

On 2019-04-17 22:02:24 +0200, Dmitry Dolgov wrote:
> > On Fri, Apr 5, 2019 at 10:25 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > A second set of limitations is around making more of tableam
> > optional. Right now it e.g. is not possible to have an AM that doesn't
> > implement insert/update/delete. Obviously an AM can just throw an error
> > in the relevant callbacks, but I think it'd be better if we made those
> > callbacks optional, and threw errors at parse-analysis time (both to
> > make the errors consistent, and to ensure it's consistently thrown,
> > rather than only when e.g. an UPDATE actually finds a row to update).
>
> Agree, but I guess some of tableam still should be mandatory, and then I wonder
> where to put the live between those that are optional and those that are not.
> E.g. looks like it can be relatively straightforward (ignoring `create table as`
> and some other stuff) to make insert/update/delete optional with messages at
> analysis time, but for others like parallel scan related it's probably not.

Thanks for the patch! I assume you're aware, but it's probably not going
to be applied for 12...

I think most of the read-only stuff just needs to be non-optional, and
most of the DML stuff needs to be optional.

On the executor side it'd probably be good to make the sample scan
optional too, but then we also need to check for that during
parse-analysis. In contast to bitmap scans there's no alternative way to
execute them.

> Assert(routine->relation_set_new_filenode != NULL);
> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
> index c39218f8db..36e2dbf1b8 100644
> --- a/src/backend/commands/copy.c
> +++ b/src/backend/commands/copy.c
> @@ -41,6 +41,7 @@
> #include "miscadmin.h"
> #include "optimizer/optimizer.h"
> #include "nodes/makefuncs.h"
> +#include "nodes/nodeFuncs.h"
> #include "parser/parse_coerce.h"
> #include "parser/parse_collate.h"
> #include "parser/parse_expr.h"
> @@ -901,6 +902,13 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
> NULL, false, false);
> rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
>
> + if (is_from && !table_support_multi_insert(rel))
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("Table access method doesn't support the operation"),
> + parser_errposition(pstate,
> + exprLocation((Node *) stmt))));

Probably should fall-back to plain inserts if multi-insert isn't
supported.

And if insert isn't supported either, we should probably talk about
that specifically? I.e. a message like
"access method \"%s\" of table \"%s\" does not support %s"
?

Without knowing at least thatmuch operation it might sometimes be very
hard to figure out what's not supported.

> +static inline bool
> +table_support_speculative(Relation rel)
> +{
> + return rel->rd_tableam == NULL ||
> + (rel->rd_tableam->tuple_insert_speculative != NULL &&
> + rel->rd_tableam->tuple_complete_speculative != NULL);
> +}

In GetTableAmRoutine() I'd assert that either both or none are defined.

> +static inline bool
> +table_support_multi_insert(Relation rel)
> +{
> + return rel->rd_tableam == NULL ||
> + (rel->rd_tableam->multi_insert != NULL &&
> + rel->rd_tableam->finish_bulk_insert != NULL);
> +}

bulk insert already is optional...

I think there's more places that need checks like these - consider
e.g. replication and such that don't go through the full blown executor.

Greetings,

Andres Freund