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

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.


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

Greetings,

Andres Freund