Re: ArchiveEntry optional arguments refactoring

On 2019-01-17 10:23:39 -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)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