Re: Pluggable Storage - Andres's take

> On Sat, Jan 12, 2019 at 1:44 AM Andres Freund <andres@anarazel.de> wrote:
>
> > +     appendPQExpBuffer(cmd, "SET default_table_access_method = %s;", tableam);
>
> This needs escaping, at the very least with "", but better with proper
> routines for dealing with identifiers.

Thanks for noticing, fixed.

> > @@ -5914,7 +5922,7 @@ getTables(Archive *fout, int *numTables)
> >                                                 "tc.relfrozenxid AS tfrozenxid, "
> >                                                 "tc.relminmxid AS tminmxid, "
> >                                                 "c.relpersistence, c.relispopulated, "
> > -                                               "c.relreplident, c.relpages, "
> > +                                               "c.relreplident, c.relpages, am.amname AS amname, "
>
> That AS doesn't do anything, does it?

Rigth, I've renamed it few times and forgot to get rid of it. Removed.

>
> >               /* other fields were zeroed above */
> >
> > @@ -9355,7 +9370,7 @@ dumpComment(Archive *fout, const char *type, const char *name,
> >                * post-data.
> >                */
> >               ArchiveEntry(fout, nilCatalogId, createDumpId(),
> > -                                      tag->data, namespace, NULL, owner,
> > +                                      tag->data, namespace, NULL, owner, NULL,
> >                                        "COMMENT", SECTION_NONE,
> >                                        query->data, "", NULL,
> >                                        &(dumpId), 1,
>
> We really ought to move the arguments to a struct, so we don't generate
> quite as much useless diffs whenever we do a change around one of
> these...

That's what I though too. Maybe then I'll suggest a mini-patch to the master to
refactor these arguments out into a separate struct, so we can leverage it here.