On 2019-01-17 10:23:39 -0500, Tom Lane wrote: > Alvaro Herrera <alvhe...@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