Re: ArchiveEntry optional arguments refactoring - Mailing list pgsql-hackers

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2019-01-17 10:23:39 -0500, Tom Lane wrote:
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > > On 2019-Jan-16, Dmitry Dolgov wrote:
> > >> ArchiveEntry((ArchiveArgs){.tablespace = 3,
> > >> .dumpFn = somefunc,
> > >> ...});
> >
> > > Is there real savings to be had by doing this?  What would be the
> > > arguments to each function?  Off-hand, I'm not liking this idea too
> > > much.
> >
> > I'm not either.  What this looks like it will mainly do is create
> > a back-patching barrier, with little if any readability improvement.
>
> I don't really buy this. We've whacked around the arguments to
> ArchiveEntry() repeatedly over the last few releases, so there's already
> a hindrance to backpatching. And given the current setup we've to whack
> around all 70+ callsites whenever a single argument is added. With the
> setup I'd suggested you don't, because the designated initializer syntax
> allows you to omit items that ought to be zero-initialized.
>
> And given the number of arguments to ArchiveEntry() having a name for
> each argument would help for readability too. It's currently not exactly
> obvious what is an argument for what:
>     ArchiveEntry(AH, nilCatalogId, createDumpId(),
>                  "ENCODING", NULL, NULL, "",
>                  "ENCODING", SECTION_PRE_DATA,
>                  qry->data, "", NULL,
>                  NULL, 0,
>                  NULL, NULL);
>
> If you compare that with
>
>     ArchiveEntry(AH,
>                  (ArchiveEntry){.catalogId = nilCatalogId,
>                                 .dumpId = createDumpId(),
>                                 .tag = "ENCODING",
>                                 .desc = "ENCODING",
>                                 .section = SECTION_PRE_DATA,
>                                 .defn = qry->data});
>
> it's definitely easier to see what argument is what.

+1.  I was on the fence about this approach when David started using it
in pgBackRest but I've come to find that it's actually pretty nice and
being able to omit things which should be zero/default is very nice.  I
feel like it's quite similar to what we do in other places too- just
look for things like:

utils/adt/jsonfuncs.c:600

        sem = palloc0(sizeof(JsonSemAction));

...

        sem->semstate = (void *) state;
        sem->array_start = okeys_array_start;
        sem->scalar = okeys_scalar;
        sem->object_field_start = okeys_object_field_start;
        /* remainder are all NULL, courtesy of palloc0 above */

        pg_parse_json(lex, sem);

...

        pfree(sem);

> > I don't buy the argument that this would move the goalposts in terms
> > of how much work it is to add a new argument.  You'd still end up
> > touching every call site.
>
> Why? A lot of arguments that'd be potentially added or removed would not
> be set by each callsites.
>
> If you e.g. look at
>
> you can see that a lot of changes where like
>         ArchiveEntry(fout, nilCatalogId, createDumpId(),
>                      "pg_largeobject", NULL, NULL, "",
> -                    false, "pg_largeobject", SECTION_PRE_DATA,
> +                    "pg_largeobject", SECTION_PRE_DATA,
>                      loOutQry->data, "", NULL,
>                      NULL, 0,
>                      NULL, NULL);
>
> i.e. just removing a 'false' argument. In like 70+ callsites. With the
> above scheme, we'd instead just have removed a single .withoids = true,
> from dumpTableSchema()'s ArchiveEntry() call.

Agreed.  Using this approach in more places, when appropriate and
sensible, seems like a good direction to go in.  To be clear, I don't
think we should go rewrite pieces of code just for the sake of it as
that would make back-patching more difficult, but when we're making
changes anyway, or where it wouldn't really change the landscape for
back-patching, then it seems like a good change.

Thanks!

Stephen